Modify VaultCredentialProvider to accept non-user/pass attributes #7
Conversation
There was a problem hiding this comment.
Hi! Thank you for filing a PR. It's very interesting. I added some initial thoughts.
Additionally to my in-line comments:
- Please update the docstring, it now doesn't fit with your proposed changes
DjangoIntegrationmight be better off asDjangoDatabaseIntegration, what do you think?
I haven't yet looked into the meta-programming you're doing in _attach_secret_attribute, so I might have some additional comments later, but the above is all I managed to do before I now have to go into some meetings :).
| self._leasetime = None # type: datetime.datetime | ||
| self._updatetime = None # type: datetime.datetime | ||
| self._lease_id = None # type: str | ||
| self._refresh() | ||
|
|
||
| def _attach_secret_attribute(instance, attribute): |
There was a problem hiding this comment.
- Please add PEP484 type annotations for
attributeand the return value (None) - Please don't override the
selfconvention (as per PEP-8)
| self.secretpath, | ||
| self._lease_id, str(self._leasetime), result["lease_duration"], self._cache["username"], | ||
| self._cache["password"] if self.debug_output else "Password withheld, debug output is disabled") |
There was a problem hiding this comment.
I think it would be useful to log the returned data similarly to what I did here (i.e. withhold the output if debug_output isn't set, but log it otherwise). I found that always useful for debugging interactions with secret engines etc.
|
|
||
| def _attach_secret_attribute(instance, attribute): | ||
| class_name = instance.__class__.__name__ + 'Child' | ||
| child_class = type(class_name, (instance.__class__,), {attribute: property(lambda self: self._get_or_update(attribute))}) |
There was a problem hiding this comment.
instead either shadow self here or rename this instance of the parameter name? This way this should be able to stay PEP-8 compliant, right?
| if not result: | ||
| try: | ||
| mountpoint, path = self.secretpath.split('/', 1) | ||
| result = vcl.secrets.kv.v2.read_secret_version(mount_point=mountpoint,path=path) |
There was a problem hiding this comment.
| result = vcl.secrets.kv.v2.read_secret_version(mount_point=mountpoint,path=path) | |
| result = vcl.secrets.kv.v2.read_secret_version(mount_point=mountpoint, path=path) |
This modification turns the VaultCredentialProvider into a more generic vault secret object. This opens up use of vault for secrets other than DB username and password.
The change primarily reads the secret keys returned from vault and dynamically assigns them as properties of the credential provider object. This PR also modifies the request logic to properly handle secrets stored under a kv engine, which returns data in a slightly different format and requires a different hvac call.