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

Secret paths shoudn't be hardcoded since vault supports mounting #12

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Albibek
Copy link

@Albibek Albibek commented Sep 29, 2016

Secrets can be anywhere depending of mount types, so secret paths shouldn't be hardcoded into paths.
Also added convenience examples for taking settings from environment variables like mainstream client does.
Fixed some tests that didn't work on my vault -dev instance because some tokens required root token privileges.

@ChrisMacNaughton
Copy link
Owner

Rather than change the API to remove the 'secret' in the get_secret functions, I'd rather see a function that is get that calls it un-prefixed. Once that is added, we can work on re-implementing some of the other apis (get_secret and get_postgresql_backend come to mind) with this new functionality.
Handling it this way would let us keep our existing API stable but would also open up other options.

@Albibek
Copy link
Author

Albibek commented Sep 29, 2016

Vault allow user to mount and remount any backend at any path. Including the default 'secret' mount. The only path(except administration ones) that is unpossible to unmount is 'cubbyhole'. Even in cubbyhole case, there can exist many cubbyholes.

Forcing library user to follow defaults limits him/her too strong.
If your wish to leave api untouchable is so strong, it could be probably better thing to have some customizable mount parameter for VaultClient, that is to be added on every request to the secret path. Setting the default mount to 'secret' should leave API stable.

@ChrisMacNaughton
Copy link
Owner

I like the idea of making it configurable, with a default that matches Vault's default!

@Albibek
Copy link
Author

Albibek commented Sep 30, 2016

Even having this worked around, I think there is also some cases where API can be broken for the sake of making library usable by any Vault user.

I can see at least these at first sight:

  • generic secrets and some other metadata having only "value" key or being HashMap, while they can be any JSON value actually
  • requiring secret to be serializable, while it just can be read in so some string and serialized later
  • having host as a &str instead of being String, that unnesessary limits the lifetime of the client to string's lifetime. It, for example, prevents from having function like VaultClient::from_env for creating client from environment variables

I see some ways of fixing it:

  • change crate versioning to is't own. Following Hashicorp Vault's version is really strange and in my opinion is more disturbing than helping thing. This would allow to make breaking changes in some future versions, letting developers who want older versions to stay on them.
  • I could go my own way developing a separate version of the package. It''s the thing I don't want to do, but the library in it's current version just doesn't allow me to acheve things I want. In this case I'm asking a permission to use your code as a starter for my own project, since it still contains a lot of thigs I could use. Thanks in advance.

@ChrisMacNaughton
Copy link
Owner

One clarification: this crate version is NOT bound to the Vault release, I just end up making API breaking changes every time they release because they change their JSON output 😉

To be clear, I am OK with breaking API compatibility, I just like to consider the effect of it a bit more. I'm very open to changing the secrets to be able to be something that we can serialize OR display, and with making host be a std::Display type so that we can take in a String as well.

@Albibek
Copy link
Author

Albibek commented Sep 30, 2016

So we just need version separation. Development branch maybe?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants