Add secrets manager integration for credential retrieval #897
Conversation
|
I'll be fixing this later please wait before you check so every test and dependency is working fine! |
alberefe
force-pushed
the
credential-manager-integration
branch
2 times, most recently
from
b16dbb2 to
cc2a73e
Compare
sduenas
left a comment
sduenas
left a comment
There was a problem hiding this comment.
Please check my initial comments. Also, my concern is this only makes perceval work if you use the backend command but not the library. Please evaluate if it makes sense to integrate it within fetch or other methods, or if on the other hand, developers should use grimoirelab toolkit instead.
alberefe
force-pushed
the
credential-manager-integration
branch
from
cc2a73e to
447d0c2
Compare
alberefe
force-pushed
the
credential-manager-integration
branch
from
447d0c2 to
f8cc53e
Compare
sduenas
left a comment
sduenas
left a comment
There was a problem hiding this comment.
Please fix the lint errors and add unit test for this functionality.
alberefe
force-pushed
the
credential-manager-integration
branch
from
f8cc53e to
e658562
Compare
|
Getting those errors cause I think it's importing an old version of Toolkit from before it had the las PR changes, is that correct? Not sure how to deal with that or if I'm even correct, but not sure what to do with it. |
We fixed it in #900. Please rebase your branch. |
alberefe
force-pushed
the
credential-manager-integration
branch
from
e658562 to
e98ca15
Compare
alberefe
force-pushed
the
credential-manager-integration
branch
from
e98ca15 to
15eefe8
Compare
Linting errors should be fixed and made tests for the env functinoality. |
sduenas
left a comment
sduenas
left a comment
There was a problem hiding this comment.
Please check my comments. The tests fail because hvac is not installed.
| buff += line | ||
|
|
||
|
|
||
| class TestBackendCommandPreInit(unittest.TestCase): |
There was a problem hiding this comment.
I not sure what the purpose of this set of tests is. They are checking that arguments are parsed but not really injected into the command and replaced using the credentials manager. I think a good set of tests would be that you mock the access to the credential manager and returns the real credentials before running perceval.
There was a problem hiding this comment.
You're right, went over my head.
I modified it to test actual perceval work mocking the toolkit answers.
Signed-off-by: Alberto Ferrer Sánchez <alberefe@gmail.com>
alberefe
force-pushed
the
credential-manager-integration
branch
from
15eefe8 to
9ae098e
Compare
|
Fix the poetry.lock file (needed to update some packages) and the testing now properly tests perceval functioning and not toolkit. |
jjmerchante
left a comment
jjmerchante
left a comment
There was a problem hiding this comment.
Overall, it looks good to me. Just some minor changes
| title: 'Add secrets manager integration for credential retrieval' | ||
| category: added | ||
| notes: > | ||
| Perceval can now retrieve backend credentials from external | ||
| secrets managers (Bitwarden, HashiCorp Vault) instead of | ||
| passing them directly on the command line. This is useful | ||
| for automated pipelines and environments where storing | ||
| credentials in plain text is not acceptable. |
There was a problem hiding this comment.
The format must be:
---
title: xxx
category: added
author: Name <email>
issue: null
notes: >
xxx
| def _pre_init(self): | ||
| """Override to execute before backend is initialized.""" | ||
| pass | ||
| """Override to execute before backend is initialized. |
There was a problem hiding this comment.
Some backends (git, groupsio, hyperkitty, pipermail) don't call super()._pre_init() in their _pre_init() implementations, and this method won't be executed. Could you fix that?
| group.add_argument('--secrets-manager', dest='secrets_manager', | ||
| choices=['bitwarden', 'hashicorp'], | ||
| help="Secrets manager service to use for credential retrieval") | ||
| group.add_argument('--item-name', dest='item_name', |
There was a problem hiding this comment.
Defining --item-name as the parameter name could mean many things. What about renaming it to --secret-name or something more specific?
Please update it in the README as well.
3 participants
Integrate Perceval with external secrets managers (Bitwarden, HashiCorp Vault) so credentials can be resolved at runtime instead of being passed as plain-text CLI arguments.
to BackendCommandArgumentParser