Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support secrets in tool requirements #19084

Draft
wants to merge 52 commits into
base: dev
Choose a base branch
from

Conversation

arash77
Copy link
Collaborator

@arash77 arash77 commented Oct 30, 2024

Related to #19196
Closes #17511
Work in progress...
I will add a full description when it is done.

Adding secrets to tool requirements like this:

<requirements>
    <secret type="vault" user_preferences_key="some_tool/api_key" inject_as_env="some_tool_api_key" label="API Key" required="true"/>
</requirements>

This will make use of the vault key that has been added in user_preferences_extra:

preferences:
  some_tool:
    description: Your Tool settings
    inputs:
      - name: api_key
        label: API Key
        type: secret
        store: vault
        required: False

And inject it as an environmental variable that could be accessed within tools.

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.


```xml
<requirements>
<secret type="vault" user_preferences_key="some_tool|api_key" inject_as_env="some_tool_api_key" label="API Key" required="true"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do any of the use cases involve multiple tools with different IDs requesting the same secret? If not - I would make the tool id (without a version) an implicit prefix here and just attach some similar field as the suffix. This isolation feels more secure-ish to me - we wouldn't risk tools picking up other keys accidentally.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, in some use cases, maybe in the future, it will be good to access an api_key or a token for multiple tools.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi John, Arash, with @Marie59 and @jeremyfix we have such use case (if I well understand) where separate tools need the same credential (here this is Copernicus marine system credentials), so I confirm that this is already a reality ;) It appears to me, at least for ecology and related environmental sciences, that it will be the case for many data providers as Copernicus for satellites remote sensing data and others.

@arash77 arash77 force-pushed the add-secrets-to-tools branch from f14c777 to bdf8667 Compare November 28, 2024 14:15
Copy link
Member

@nuwang nuwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see this feature coming in. One thing I was wondering about though, should this be a requirement or be injected through the <environment_variables> section following the precedent here?
#15300

Comment on lines 325 to 326
seperated_key = user_preferences_key.split("/")
if len(seperated_key) != 2 or not seperated_key[0] or not seperated_key[1]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
seperated_key = user_preferences_key.split("/")
if len(seperated_key) != 2 or not seperated_key[0] or not seperated_key[1]:
separated_key = user_preferences_key.split("/")
if len(separated_key) != 2 or not separated_key[0] or not separated_key[1]:

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the suggestion, but they will all change.
the new discussed plan is in #19196
I am not sure if this should go into environment_variables or not.

@arash77 arash77 force-pushed the add-secrets-to-tools branch 5 times, most recently from dedb684 to ff3ee10 Compare December 3, 2024 16:45
@arash77 arash77 force-pushed the add-secrets-to-tools branch from ab6f860 to 7c8b58f Compare December 19, 2024 15:04
arash77 and others added 6 commits December 19, 2024 17:44
remove unused tests,
add credential yaml test,
refactor tool evaluator (needs more work)
The user ID comparison was comparing against the user object and not the ID.
I think admins should not have access to this API by default so dropping the admin check.
- Make it scoped by user
- Use new API instead of stub
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tool language for defining and requiring specific secrets
5 participants