Skip to content

Rotate gitlab tokens instead of creating new ones#13

Merged
timakro merged 5 commits intomainfrom
pr-rotate-gitlab-tokens
May 12, 2025
Merged

Rotate gitlab tokens instead of creating new ones#13
timakro merged 5 commits intomainfrom
pr-rotate-gitlab-tokens

Conversation

@timakro
Copy link
Contributor

@timakro timakro commented May 9, 2025

First I wrote validate_or_create, then I noticed the GitLab API does not work as advertised and then wrote validate_or_create_workaround with the following strategy:

  1. Test the current token by simulating a git fetch if it does not work create new token and return it
  2. Check if all active tokens still have enough time remaining. Since our token is an active token (confirmed in step 1) we know that our token also has enough time remaining and we can return AlreadyValid.
  3. Rotate the current token and return the new one

If they ever fix https://gitlab.com/gitlab-org/gitlab/-/issues/523871 we can remove the workaround. The non-workaround function validate_or_create simply calls an endpoint that checks if the current token is still valid and returns the expiry date, but that endpoint is currently broken.

This PR also refactors the secret request type. Previously:

pub enum SecretRequestType {
    ValidateOrCreate {
        current: String,
        request: SecretRequest,
    },
    Create(SecretRequest),
}
pub enum SecretRequest {
    OpenIdConnect(OIDCConfig),
    GitLabProjectAccessToken(GitLabClientConfig),
}

Now:

pub enum SecretType {
    OpenIdConnect(OIDCConfig),
    GitLabProjectAccessToken(GitlabClientConfig),
}
pub enum RequestType {
    ValidateOrCreate(String),
    Create,
}
pub struct SecretRequest {
    pub request_type: RequestType,
    pub secret_type: SecretType,
}

This "decouples" the secret type (GitLab, OIDC) from the request type (ValidOrCreate, Create). This allows us to first match on the secret type and call the dedicated handler, which can than handle the request types in their own fashion. Specifically the old request type handling assumed that we would first validate the token and if that fails create a new one, without any considering of creating a new token with the old token, i.e. token rotation.

@timakro timakro requested review from Threated and lablans May 9, 2025 15:17
Copy link
Member

@Threated Threated left a comment

Choose a reason for hiding this comment

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

Yeah I like the refactor that does make more sense. Only thing we have to account for again is that this change the wire format :/ so we will probably run into some decoding errors again. As long as we upgrade all the central instances quickly it should hopefully be fine.

Copy link
Member

@lablans lablans left a comment

Choose a reason for hiding this comment

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

One more point. There might be cases in which a token is to be created when in fact, an existing token could be rotated (e.g. on each Bridgehead reboot as the token is stored in /tmp). To reduce the number of admin nag e-mails from Gitlab, instead of creating tokens the central component should always try to find an active token and rotate it and only if that fails create a new one.

@timakro
Copy link
Contributor Author

timakro commented May 12, 2025

Strategy changed. Token is considered valid if simulated git fetch works with the token and all active tokens are valid for at least 15 more days.

If token is invalid:

  • Rotate the token with the latest expiration date if any
  • Otherwise create a new token

Funnily enough we are back to the old validate then create strategy, but I think the refactor was a good idea anways. Also I've swapped chrono for time because it works nicely with serde.

@timakro timakro merged commit a1087c7 into main May 12, 2025
2 checks passed
@timakro timakro deleted the pr-rotate-gitlab-tokens branch May 12, 2025 10:22
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