Add loaders for Vault compatibility #12
Conversation
grift/loaders.py
Outdated
|
|
||
| def _fetch_secrets(self): | ||
| """Read data from the vault path""" | ||
| url = '{}/v1/{}'.format(self._vault_url, self._path) |
There was a problem hiding this comment.
Thoughts about using urljoin for all URL construction? Many services are confused by multiple consecutive slashes, and the self._vault_url and self._path are not checked for leading / trailing slashes. The builtin methods are just safer, in my opinion.
https://docs.python.org/2/library/urlparse.html
https://docs.python.org/3/library/urllib.parse.html
There was a problem hiding this comment.
Thanks for catching this. I ended up avoided urljoin because it's kind of awkward with 2/3, and because it's not super helpful in joining multiple segments
grift/loaders.py
Outdated
| self._vault_url = url | ||
| self._path = path | ||
| self._token = token | ||
| source_dict = self._fetch_secrets() |
There was a problem hiding this comment.
This pattern is problematic:
- most people would be surprised to learn that constructing a new object makes network requests (i.e. can raise IOErrors);
- network access makes it difficult to mock;
- it's generally bad practice to call
selfmethods before the object has been fully initialized, since its state is by definition incomplete, and all such called methods (and methods called by them, and so on) need to mind how much of the object has been constructed up to that point.
I would strongly advise refactoring this into a function that performs all of these checks and network requests, and when done, constructs a loader object and returns it.
grift/loaders.py
Outdated
| """ | ||
| self._role_id = role_id | ||
| self._secret_id = secret_id | ||
| token = self._fetch_token() |
There was a problem hiding this comment.
Same comment as above, on calling methods before the object is fully constructed.
| @@ -1,2 +1,4 @@ | |||
| # These requirements are used for testing and development; please update setup.py if needed | |||
| mock>=2.0.0 | |||
| requests>=2.7.0 | |||
There was a problem hiding this comment.
I'd make this 2.13.0 or relax the requires line to >=2.7.0,<3.0.0
Adding two loaders to support reading secrets from a path in a Vault. For now, supporting authentication via token (which is what all other auth backends map to) or app role.
I also included a few convenience methods in the token loader, to look up info about the token or renew it. The renewal method could be run periodically by a task or cronjob to keep a running service's token from expiring, in case the service is restarted / secrets need to be reloaded.
Would appreciate input on all of this -- I've been toying with Vault locally, but haven't worked with it in production.