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.

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

feat: add jwt_signature_pk_urls to state #866

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

esaminu
Copy link

@esaminu esaminu commented Sep 27, 2024

This PR:

  • Replaces jwt_signature_pk_url from SignNodeState and LeadeState with jwt_signature_pk_urls as comma separated list
  • Modifies get_public_keys to fetch and handle public key responses with both firebase format and standard jwks format e.g. google

The purpose is to add support to mpc for non firebase issuers e.g. google, apple etc. When a new issuer is added we need to ensure their public keys are also being fetched by appending their jwks url to this new env

@esaminu esaminu changed the title feat: add jwt_signature_pk_urls to state feat: add jwt_signature_pk_urls to state Sep 27, 2024
Copy link
Collaborator

@volovyks volovyks left a comment

Choose a reason for hiding this comment

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

Looks good overall. I have concerns about security in the process of token verification.
Let's sync and discuss this.

let json: HashMap<String, String> = response.json().await?;
Ok(json.into_values().collect())
jwt_signature_pk_urls: &[String],
) -> Result<Vec<String>> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is better to create a map of keys: "provider" -> "[keys]".
Otherwise, somebody will create a token using provider1, but verify it with key 2.

Copy link
Collaborator

@volovyks volovyks left a comment

Choose a reason for hiding this comment

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

Looks good!

@@ -98,6 +98,21 @@ impl OidcToken {

Ok((header, claims, signature.into()))
}

// NOTE: code taken directly from our implementation of token.decode but without the verification step
pub fn decode_unverified(&self) -> anyhow::Result<(jwt::Header, IdTokenClaims, String)> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's reuse it in decode

}
}

fn parse_jwks_format(obj: &serde_json::Map<String, Value>) -> Result<Vec<String>> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's test this

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.

2 participants