Skip to content

Conversation

@alberefe
Copy link
Contributor

@alberefe alberefe commented Oct 20, 2025

Implement credential management system with Bitwarden integration. This provides automated secrets retrieval.

This relieves the user from hardcoding credentials in files and also automates the retrieval of api keys from pools in an automated way, optimizing speed when the program needs to rotate through several api keys or credentials to access different services in a row.

The following are the contents of the PR:

  • Add Bitwarden manager (bw_manager.py)
  • Custom exception handling
  • Utility functions
  • Test coverage for Bitwarden functionality

@alberefe alberefe force-pushed the feature/credential_manager branch from 4432e5b to 89334e8 Compare October 21, 2025 13:59
@alberefe
Copy link
Contributor Author

I pushed some changes cause some format issues in the code.

Copy link
Member

@sduenas sduenas left a comment

Choose a reason for hiding this comment

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

Thanks again for your PR. I think it will be nice to integrate this. Please check my comments and remove all references to other managers not yet supported (AWS, Hashicorp).

@alberefe alberefe force-pushed the feature/credential_manager branch 3 times, most recently from 1ef95dd to 8f015e9 Compare October 22, 2025 21:07
@alberefe alberefe requested a review from sduenas October 22, 2025 21:09
@alberefe alberefe force-pushed the feature/credential_manager branch from 8f015e9 to 28b5bfe Compare October 22, 2025 21:34
Copy link
Member

@sduenas sduenas left a comment

Choose a reason for hiding this comment

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

Please check my comments.

@alberefe alberefe force-pushed the feature/credential_manager branch 5 times, most recently from 66f5aea to ddcaacf Compare October 28, 2025 15:23
@alberefe alberefe requested a review from sduenas October 28, 2025 15:23
@alberefe alberefe force-pushed the feature/credential_manager branch 5 times, most recently from c423eb4 to d6dbc75 Compare November 6, 2025 14:05
@alberefe
Copy link
Contributor Author

alberefe commented Nov 6, 2025

Did some more changes and pushed. Testing was not correct after last comment changes.

@alberefe
Copy link
Contributor Author

alberefe commented Nov 6, 2025

So, testing, reading and refactoring I've come to the finding that when we use client_id + client_secret (apikey) for login, then we need the master password for unlocking the vault. That means that we have to add the master password somewhere to the login or creating the object so we can unlock the vault.

This does not happen if we use user-password, in which case the vault unlocks by itself. So I've come to two options:

  1. I include the master password in the parameters for creating the BitwardenManager object and use it in unlock function as parameter. In this case, it's just one more argument and would not change it more, but I'm not sure how this would go with big enterprises. In this case, we could go back to use user + password and remove the unlock method so it would simplify the class.
  2. I implement BitwardenManager using their secrets manager, which would make the development much easier but would force people using it to have a suscription and wouldn't be free.
  3. I can write two implementations each one using one of the ways.

I am going to push changes with option 1, cause we have already put in the parameter list two sensitive strings, so adding one more I don't think would be a problem. Awaiting opinions.

@alberefe alberefe force-pushed the feature/credential_manager branch from d6dbc75 to 20424b5 Compare November 6, 2025 19:27
@alberefe
Copy link
Contributor Author

alberefe commented Nov 6, 2025

So it's right now using the master_password as third argument. Rechecked everything and tested. Should be good to go and returns what is shown in the README.

@sduenas
Copy link
Member

sduenas commented Nov 7, 2025

@jjmerchante can you have a look to this PR?

@sduenas sduenas requested a review from jjmerchante November 7, 2025 14:45
Copy link
Contributor

@jjmerchante jjmerchante left a comment

Choose a reason for hiding this comment

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

Overall, it looks good to me. Please review my comments, most are about style.

I don’t like using the master password, but as you said, there isn’t any other way to unlock. I think using API keys is better for authentication, because in some cases you need single sign-on or two-factor authentication, and with API keys, as I understand it, this isn’t required.

Let’s keep this as is for now, we can explore Bitwarden Secrets later.

@alberefe alberefe force-pushed the feature/credential_manager branch from 20424b5 to 1f81c78 Compare November 12, 2025 15:21
@alberefe
Copy link
Contributor Author

Did all the changes and merged the poetry.lock changes too.

I also agree that the master password is not the best solution but could develop a Bitwarden Secrets solution next to this so people can choose which one to use.

@alberefe alberefe force-pushed the feature/credential_manager branch from 365a1b3 to 8c042e7 Compare November 12, 2025 15:36
@alberefe
Copy link
Contributor Author

I am not sure how to resolve that poetry.lock conflict without breaking stuff, can I leave this to you? I am still not 100% sure of my git skills.

@jjmerchante
Copy link
Contributor

I am not sure how to resolve that poetry.lock conflict without breaking stuff, can I leave this to you? I am still not 100% sure of my git skills.

The best way is to remove the poetry.lock file and regenerate it instead of trying to resolve the conflicts manually:

rm poetry.lock
poetry update
git add poetry.lock
git commit --amend

Then push with -f to force-push the commit.

If have any issues, let me know and I can update it for you.

@alberefe alberefe force-pushed the feature/credential_manager branch 2 times, most recently from 72cec79 to f7eed67 Compare November 13, 2025 19:53
@alberefe
Copy link
Contributor Author

Fixed that missing line and tried to fix the poetry.lock following instructions, but it keeps showing as error. Can you fix it? Would also like to know what's going on, I don't understand.

Thanks a lot for the reviews and I'm around for any other changes.

Signed-off-by: Alberto Ferrer Sánchez <alberefe@gmail.com>
@jjmerchante jjmerchante force-pushed the feature/credential_manager branch from f7eed67 to 34ba6d5 Compare November 14, 2025 12:43
@jjmerchante
Copy link
Contributor

I was wrong with my previous instructions. Here’s what I actually did in your branch (assuming origin is the chaoss repository):

git rebase origin/master
# Conflict in poetry.lock
rm poetry.lock
poetry update
git add poetry.lock
git status   # No more conflicts
git rebase --continue

Copy link
Contributor

@jjmerchante jjmerchante left a comment

Choose a reason for hiding this comment

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

LGTM

@jjmerchante jjmerchante merged commit f455a55 into chaoss:main Nov 14, 2025
6 checks passed
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.

3 participants