Skip to content

CBST2-04: Update JWT secrets on reload and revoke module endpoint #295

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.

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

Open
wants to merge 16 commits into
base: sigp-audit-fixes
Choose a base branch
from

Conversation

ManuelBilbao
Copy link
Collaborator

Now the reload endpoint on the signer module allows to update the JWT secrets too.
Also, a new revoke_module endpoint was added, to quickly remove the permissions for a compromised module.
This two endpoints are now under a new middleware that validates a special "admin" JWT, whose secret is autogenerated on the init command.

@ManuelBilbao ManuelBilbao requested a review from ltitanb April 21, 2025 21:40
@ltitanb ltitanb requested a review from jclapis July 8, 2025 19:40
Copy link
Collaborator

@ltitanb ltitanb left a comment

Choose a reason for hiding this comment

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

this has a breaking change as now it requires ADMIN_JWT_ENV to be set for the module to start, so we should target the sigp-audit-fixes branch instead of main

@ManuelBilbao ManuelBilbao changed the base branch from main to sigp-audit-fixes July 8, 2025 20:00
Copy link
Collaborator

@ltitanb ltitanb left a comment

Choose a reason for hiding this comment

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

some small comments, would also be good to add a unit test for the new behaviour (revoked modules can't get signatures)

@@ -201,7 +214,7 @@ fn check_jwt_rate_limit(state: &SigningState, client_ip: &IpAddr) -> Result<(),
}

/// Checks if a request can successfully authenticate with the JWT secret
fn check_jwt_auth(
async fn check_jwt_auth(
Copy link
Collaborator

Choose a reason for hiding this comment

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

this doesn't need to be async anymore

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed ce562de

) -> Result<Response, SignerModuleError> {
let jwt: Jwt = auth.token().to_string().into();

validate_admin_jwt(jwt, &state.admin_secret.read()).map_err(|e| {
Copy link
Collaborator

Choose a reason for hiding this comment

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

as a sanity check is the read guard dropped immediately after the function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The lock is dropped immediately in case of an Ok() result. In other case, the lock will be kept in the map_err closure, which only logs an error. Would you prefer it to immediately drop in either case?

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