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

Vault abstraction for Galaxy #12940

Merged
merged 29 commits into from
Dec 27, 2021
Merged

Vault abstraction for Galaxy #12940

merged 29 commits into from
Dec 27, 2021

Conversation

nuwang
Copy link
Member

@nuwang nuwang commented Nov 17, 2021

This PR adds a vault abstraction for Galaxy with 3 backends - hashicorp, database and custos. It supercedes the work done in: #9876

Storing extra user preferences in the vault

image

Configuring file sources to use the vault

image

Programmatic Usage

Basic vault usage

from galaxy.security.vault import Vault, VaultFactory

vault = VaultFactory.from_app(app)
vault.write_secret("/my/super/secret", "hello secret")
print(vault.read_secret("/my/super/secret"))

User Level Vault

UserVaultWrapper creates a namespaced portion of the vault. Values are stored in the vault at: /{prefix}/users/{user_id}/{key}.

from galaxy.security.vault import UserVaultWrapper 

user_vault = UserVaultWrapper(app.vault, trans.user)
user_vault.write_secret("/my/super/secret", "hello secret")
print(user_vault.read_secret("/my/super/secret"))

Slides:

https://bit.ly/galaxy_vault

Tasks:

  • Add basic vault abstraction
  • Add support for hashicorp vault
  • Add support for database backend with rotatable keys
  • Add unit tests
  • Add integration tests
  • Add support for custos (thanks to some changes in custos that @isururanawaka has made. A few more are pending to make things even better)
  • Add support for saving user pref password creds in vault?
  • Add dedicated vault secret page to user preferences? No need anymore
  • Validate secret key paths
  • Refactor dependencies
  • Conditional installation during unit tests
  • Make sure file sources can use the vault
  • Add support for listing secrets? Custos does not support this atm.
  • Add new "secret" type to extra user prefs which is not sent to the client
  • Use plugin system for vault backends? Not necessary
  • Add documentation on vault configuration

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 contributions under Galaxy's current license.
  • I agree to allow the Galaxy committers to license these and all my past contributions to the core galaxy codebase under the MIT license. If this condition is an issue, uncheck and just let us know why with an e-mail to [email protected].

@dannon
Copy link
Member

dannon commented Nov 17, 2021

@nuwang Fantastic! I had no idea you were working on this -- I see you used hvac here; I looked at that and was hoping it'd make this straightforward, and it looks like it does! Can you outline the extra bits required for 'Add support for custos (pending some changes in custos)'? And, would you want anyone else to help building out the UI bits?

@nuwang
Copy link
Member Author

nuwang commented Nov 18, 2021

@dannon Custos currently requires single sign-on to access the vault api. It's being changed to add support for access through a service account. Isuru is working on this and it should be fairly straightforward to integrate afterwards. I'll take an initial crack at the UI bits just so I'm forced to work on the UI for a change, but may ask for help if I run into issues. I don't expect there to be a lot to do there, but famous last words.

@jdavcs
Copy link
Member

jdavcs commented Nov 18, 2021

@nuwang Would you mind adding a unit test for the new table mapping as well? Here's an example (the top of the file has some documentation on what's what) - it's just for the columns, since there are no relationships. It verifies the mapping is setup correctly (and not accidentally broken down the road), and it keeps it consistent with the other model. (I'll add all this to the docs on how to edit the model as soon as we move to alembic)

@mvdbeek mvdbeek added the highlight Included in user-facing release notes at the top label Dec 15, 2021
Copy link
Member

@mvdbeek mvdbeek left a comment

Choose a reason for hiding this comment

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

Very cool, that's going to be very useful. I guess there's a client part coming in a followup that adds the rendering of secrets ?

doc/source/admin/special_topics/vault.md Outdated Show resolved Hide resolved
doc/source/admin/special_topics/vault.md Outdated Show resolved Hide resolved
lib/galaxy/dependencies/__init__.py Show resolved Hide resolved
test/integration/test_vault_file_source.py Show resolved Hide resolved
@mvdbeek
Copy link
Member

mvdbeek commented Dec 15, 2021

I guess there's a client part coming in a followup that adds the rendering of secrets ?

Not sure what happened there, the first time I tried a secret the entire extra preferences weren't rendering at all, but seems to work now. That's why I thought we still need some client work.

@mvdbeek
Copy link
Member

mvdbeek commented Dec 15, 2021

Should we also stop shipping the password to the client at all ? This seems like a security and transparency issue, since the admins decide how the value is stored a user has no clue that maybe their password is safe on one server but not the other ?

@nuwang
Copy link
Member Author

nuwang commented Dec 15, 2021

Thanks for the detailed review and test @mvdbeek, it's very helpful. I'll work on moving the api test into integration tests, and merging the test you added.

Regarding the password vs secret distinction for the client, I don't have a strong opinion on it. I suppose if we make all password fields "write only", the client would never be able to store and retrieve a secure secret, even if there was some future use case in which it needed to. On the other hand, it would make the defaults more secure yes.

I suppose we could also add a visual distinction in the client, maybe some text distinguishing between write only secrets and read-write secrets?

Copy link
Contributor

@afgane afgane left a comment

Choose a reason for hiding this comment

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

Works really nice. I am having trouble configuring the Custos backend because it keeps complaining about custos-sdk not being installed (even though it is) so perhaps once added into conditional deps that will be resolved...

doc/source/admin/special_topics/vault.md Outdated Show resolved Hide resolved
doc/source/admin/special_topics/vault.md Outdated Show resolved Hide resolved
doc/source/admin/special_topics/vault.md Outdated Show resolved Hide resolved
doc/source/admin/special_topics/vault.md Outdated Show resolved Hide resolved
doc/source/admin/special_topics/vault.md Show resolved Hide resolved
doc/source/admin/special_topics/vault.md Show resolved Hide resolved
doc/source/admin/special_topics/vault.md Outdated Show resolved Hide resolved
Co-authored-by: Enis Afgan <[email protected]>
@nuwang
Copy link
Member Author

nuwang commented Dec 27, 2021

@mvdbeek Have made the suggested changes. Look ok to merge?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

7 participants