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.

Sign up for GitHub

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

Jump to bottom

feat: allow non-admin config tokens to rotate using rotation API #90

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: allow non-admin config tokens to rotate using rotation API #90

wants to merge 1 commit into from

Conversation

TJM
Copy link

@TJM TJM commented May 23, 2024 •
edited
Loading

Initial attempt at catching non-admin users and using the token rotation API

Rotation tested!

[tmcneely@local vault-plugin-secrets-gitlab]$ vault write gitlab/config base_url=https://gitlab.company.com auto_rotate_token=true token=$GITLAB_TOKEN
WARNING! The following warnings were returned from Vault:

  * auto_rotate_token not specified setting to 24h0m0s

Key                          Value
---                          -----
auto_rotate_before           24h0m0s
auto_rotate_token            true
base_url                     https://gitlab.company.com
revoke_auto_rotated_token    false
token_expires_at             n/a
token_id                     0
token_sha1_hash              2bc4ea551b76c56813aea32fcbd1a47c78795256

[tmcneely@local vault-plugin-secrets-gitlab]$ vault read gitlab/config/rotate
Key                          Value
---                          -----
auto_rotate_before           24h0m0s
auto_rotate_token            true
base_url                     https://gitlab.company.com
revoke_auto_rotated_token    false
token_expires_at             2025-05-23T00:00:00Z
token_id                     5748
token_sha1_hash              ef04d0bf1453f49bbb640606210bc91fc5bb2d95

I was going to add acceptance testing, but it doesn't look like that is very far along yet. ;)

Closes #89

@TJM
Copy link
Author

TJM commented May 24, 2024 •
edited
Loading

I updated the test data to support the usr.isAdmin, and while I was in there, added a unit test for testing a non-admin user. Also updated the readme, fixing a few lint issues while adding a note about non-admin tokens.

@TJM
Copy link
Author

TJM commented May 24, 2024 •
edited
Loading

The "RotateCurrentToken" tries to maintain the current expiration duration. If the initial "main" token has a duration of 1 day, it will rotate every minute (periodicFunc frequency), as the default auto_rotate_before is also 24h. The token would always expire in less than 24h (well there might be one minute per day that it won't try to rotate). This is not related to allowing a non-admin token to rotate. Also, the debug log messages are excessive, so I am going to start a different MR to clean some that up, and maybe look into limiting or at least sending a WARNING when this happens.

@TJM
Copy link
Author

TJM commented May 29, 2024

@ilijamt I don't mean to pressure you (much), but I need to know if you could review/merge/release this, or if I need to try to figure out how to get it released in my fork. Thanks in advance :)
Tommy

@ilijamt
Copy link
Owner

ilijamt commented May 30, 2024

I'm checking it, I'm running into some issues. Need to do some more testing. And checking to see what are the possibilities, and if there is another way.

@TJM
Copy link
Author

TJM commented May 30, 2024

I'm checking it, I'm running into some issues. Need to do some more testing. And checking to see what are the possibilities, and if there is another way.

Let me know, happy to try to help. The "go test -v" worked for me, but the workflows above haven't run yet, so I don't know what other problems there might be.

@TJM TJM mentioned this pull request Jun 1, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow rotation of non-admin config tokens
2 participants
@TJM @ilijamt