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

Add support for private_key_jwt credentials #217

Closed
wants to merge 1 commit into from

Conversation

smndtrl
Copy link

@smndtrl smndtrl commented May 19, 2023

This is a WIP.

Microsofts Active Directory may require authentication with client certificates to access certain APIs. This leverages private_key_jwt 1.

This is a very early quick and dirty way of making it work for my own scenario. It pulls in openssl and the native mess due to the jwt dependencies requirement for it when signing RS256 JWT. I intend to move all of it to pure RustCrypto packages when I have more time.

I wanted to open regardless to start potential discussion on public API of these scenarios. Maybe enum ClientCredentials { ClientSecret, PrivateKey... } would work.

@ramosbugs
Copy link
Owner

Hi @smndtrl,

Thanks for the PR. Since the cited spec is part of OpenID Connect, and given the complexity and additional dependencies required to deal with JWTs, I think this functionality would be better suited to the openidconnect crate (which is built on top of this one).

That crate already has functionality for building/signing/parsing/verifying JWTs, which should avoid introducing any new dependencies.

For flexibility and consistency with the rest of that library, I think that crate's expanded API should look something like (see inline comments):

// CoreClient::new() can also be used if the OP doesn't support OIDC Discovery.
let client = CoreClient::from_provider_metadata(
    provider_metadata,
    client_id,
    // Be sure *not* to set a client_secret here or `oauth2` will try to use the `client_secret_basic`
    // or `client_secret_post` auth method. When it's set to None, `oauth2` will only set the required
    // `client_id` request parameter.
    None,
);

// Returns a `ClientAuthTokenBuilder` for customizing the client auth JWT, which should ideally
// work for both `client_secret_jwt` and `private_key_jwt`. This is a method of
// `openidconnect::Client` so that the `iss` and `sub` claims can be auto-populated based on the
// client's `client_id`, and the `aud` claim can be auto-populated with the client's
// `token_endpoint`. A single `ClientAuthTokenBuilder` instance should be usable for making multiple
// token requests.
let client_auth_token_builder = client
    .client_auth_token_builder(
        // A generic private key that implements the `PrivateSigningKey` trait. Currently we
        // support `CoreHmacKey` (which would work for `client_secret_jwt`), and
        // `CoreRsaPrivateSigningKey` (which would work for private_key_jwt). See `IdToken::new()`
        // for a similar interface.
        &private_key,
        // RS256 (or `CoreJwsSigningAlgorithm::HmacSha256` for HS256 used by `client_secret_jwt`).
        // If the user passes a symmetric algorithm, they're using `client_secret_jwt`. If the user
        // passes an asymmetric algorithm, they're using `private_key_jwt`. In either case, we're
        // setting the same `client_assertion` and `client_assertion_type` request parameters, so
        // the library doesn't need to distinguish between these cases.
        CoreJwsSigningAlgorithm::RsaSsaPkcs1V15Sha256,
        // To avoid `jti` reuse, pass in a function, similar to `state_fn` and `nonce_fn` passed to
        // `Client::authorize_url()`. Since the builder can be reused for multiple requests, this
        // will be called each time. Clients can provide a closure in cases where they need to set
        // a specific JWT ID, and then it'll be their responsibility not to reuse the builder.
        ClientAuthTokenId::new_random,
        // Token's expiration time based on the current timestamp. For
        // convenience, this argument can be a generic type that implements a new `TokenExpiration`
        // trait, which we can impl for both `chrono::Duration` and
        // `Fn(DateTime<Utc>) -> DateTime<Utc>`. This lets users pass a simple `Duration` for
        // simple cases while still allowing them to pass a closure for more complex use cases.
        // See the `NonceVerifier` trait passed to `IdToken::claims()` as an example of this
        // pattern.
        Duration::from_secs(30),
        // Generic type that implements an `AdditionalClientAuthTokenClaims` trait similar to
        // `AdditionalClaims` used for `IdTokenClaims`. This allows users to introduce arbitrary
        // claims required by OPs beyond those defined in the OIDC Core spec. For example,
        // Microsoft Azure Active Directory users would define their own type and pass it in here
        // in order to set `nbf` and other claims required by Microsoft. These are vendor-specific
        // and shouldn't be part of the crate's public API. Including an example of how to use
        // Azure AD in the crate would be appropriate, though, similar to the Google and GitLab
        // examples.
        EmptyAdditionalClientAuthTokenClaims,
    )
    // Optional setter to override the `aud` claim since the spec only says that it SHOULD be the
    // token endpoint (i.e., other values are allowed).
    .set_audience(Audience::new(...))
    // Tells the library to set the `x5t` field in the JWT's JOSE header. Note that currently the
    // internal `JsonWebTokenHeader` type doesn't have this field because it's only being used for
    // ID tokens and user info tokens, which the OIDC Core spec says should not use key
    // identification headers like x5u. `JwsKeyIdMethod` would be a new #[non_exhaustive] enum
    // that could also include variants such as `X509Sha256` (for `x5t#S256`), `KeyId` (for `kid`),
    // `X509Url` (for `x5u`), etc. See https://www.rfc-editor.org/rfc/rfc7515#section-4.1. For
    // `X509Sha1`, the user passes in the X.509 certificate PEM bytes, and the crate automatically
    // computes the base64url-encoded SHA-1 hash and sets the `x5t` header.
    .set_key_id_method(JwsKeyIdMethod::X509Sha1(cert_pem));

let token_response = client
    // This is a modified version of `Client::exchange_code()` that calls
    // `CodeTokenRequest::add_extra_param()` twice to add the `client_assertion` and
    // `client_assertion_type` request parameters. Unlike `exchange_code()`, this method is fallible
    // since it will need to serialize and sign the JWT. Returning
    // `Result<CodeTokenRequest, JsonWebTokenError>` probably makes sense.
    .exchange_code_with_client_auth_token(
        code,
        &client_auth_token_builder,
    )?
    .request(http_client)?;

In order to serialize the JWT, the ClientAuthTokenBuilder can have a build() method that returns a ClientAuthToken struct (which wraps JsonWebToken, similar to IdToken) and looks similar to IdToken::new().

We'll also need appropriate doc comments and test coverage for the new functionality (see existing tests for examples). Since the openidconnect crate aims to help support both clients (OIDC Relying Parties) and servers (OIDC Providers), it's probably worth making sure that ClientAuthToken has claims/into_claims methods similar to IdToken's that verifies the JWT and returns the claims. This will also help with writing tests since we'll be able to do round trips (serialize/sign/verify/deserialize).

Thoughts?

@ramosbugs
Copy link
Owner

Thanks for the PR. Since the cited spec is part of OpenID Connect, and given the complexity and additional dependencies required to deal with JWTs, I think this functionality would be better suited to the openidconnect crate (which is built on top of this one).

Err, I guess technically this is defined in RFC 7523, which is separate from OIDC, but I still think it belongs in openidconnect rather than this crate due to the complexity. You're of course also free to publish a separate crate that could take a CodeTokenRequest and set the client_assertion and client_assertion_type parameters without making changes to either of the two existing crates. That crate could even be vendor-specific.

@smndtrl
Copy link
Author

smndtrl commented May 29, 2023

Thank you for your detailed answer. I was able to implement the functionality inside the openidconnect crate as you proposed. Luckily AzureAD also accepts the base64 encoded version of the sha256 fingerprint as kid and does not necessarily require x5t.

If I can find time over the next week, I will prepare a PR. Might take me a bit longer as this isn't the highest of priorities atm. Just wanted to let you know that it works.

I used String for iss and sub as I feel like from reading the RFC, both can be arbitrary strings and might just be ClientId for AzureAD... I haven't seen/checked how other might handle certificate based auth.

@svix-james
Copy link

svix-james commented Jan 3, 2024

@smndtrl Have you ever raised a PR for the openidconnect crate with your changes?

I personally find it a bit awkward that relevant code for supporting JWTs in oauth2 is only in the openidconnect crate, but regardless, it would be very valuable to include your proposed changes.

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