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

Strict validation breaks due to Microsoft's OIDC noncompliance #122

Open
Redrield opened this issue Jul 24, 2023 · 5 comments · May be fixed by #179
Open

Strict validation breaks due to Microsoft's OIDC noncompliance #122

Redrield opened this issue Jul 24, 2023 · 5 comments · May be fixed by #179

Comments

@Redrield
Copy link

This library strictly validates responses from the server according to spec in ways that make it incompatible with Microsoft's implementation of OIDC in certain situations due to their refusal to make their implementation spec compliant. Currently the library doesn't allow users to configure the validation strictness, meaning the only way to work around this is to copy the repository locally to selectively ignore validation failures caused by this. It would be helpful if users could configure this to disable the validation to support Microsoft OIDC (Or alternatively, to specify alternate valid fields to provide some level of certainty that the payload is mostly correct).

The two checks that fail are in verification.rs:284 (JWT issuer validation), and discovery.rs:386 (Provider metadata issuer validation).

Based on the responses to the above issue (as well as the age of the issue), I don't think a spec compliant implementation of OIDC should be expected from Microsoft any time soon, and as such it would be helpful to allow for exceptions in this case to be compatible.

@ramosbugs
Copy link
Owner

I'm sympathetic to Microsoft's customers' frustrations here while also being annoyed that Microsoft decided to ignore the spec and push this problem to both customers and library maintainers. Since this library focuses on standardized protocols, I think a reasonable goal here is to have escape hatches (but not foot guns) available that allow users to interoperate with Microsoft, but without adding any Microsoft-specific functionality to this crate. A PR adding a working Microsoft code example would be welcomed, though.

This means that Microsoft customers should be able to use this crate, but their code may be more verbose than for users of standards-compliant OIDC implementations. Making Microsoft OIDC integrations work out of the box without any extra code is a non-goal of this crate, although community members should feel free to publish a separate crate built on top of this one to improve the ergonomics.

The two checks that fail are in verification.rs:284 (JWT issuer validation), and discovery.rs:386 (Provider metadata issuer validation).

For JWT issuer validation, there are already two escape hatches available:

  1. When instantiating an IdTokenVerifier, an IssuerUrl should be specified that matches the one that will be present in the ID tokens. This is the recommended approach.
  2. If that doesn't work for some reason, there's also a require_issuer_match method that can be used to disable this check altogether.

Discovery validation is a bit trickier, but the http_client argument provides an escape hatch. Since the OIDC discovery document isn't signed, Microsoft users can pass an http_client shim to discover/discover_async that replaces the invalid issuer field in the response with the one that will be in the ID tokens. That way, by the time the response reaches this crate, it'll be compliant with the spec, and the rest of the library should just work. This is the functionality that I could see living in a separate, Microsoft-specific crate that wraps around this one.

@Redrield
Copy link
Author

Thanks for the guidance, I did manage to put together a shim http client as you suggested to fix the issue with discovery validation. As for JWT validation, I'm not sure how the "recommended" escape hatch could be implemented, even by a library that wraps this one, because the client id/secret gets moved into the oidc Client type, and is held there opaque to any API consumers. Since the instantiation of an IdTokenVerifier happens here, there doesn't seem to be a lot that can be done to get around that.

As for a Microsoft code example, after circumventing the failing validation checks, I was able to get this crate working with the same overall structure as the Google example, albeit with URLs changed.

@ramosbugs
Copy link
Owner

As for JWT validation, I'm not sure how the "recommended" escape hatch could be implemented, even by a library that wraps this one, because the client id/secret gets moved into the oidc Client type, and is held there opaque to any API consumers. Since the instantiation of an IdTokenVerifier happens here, there doesn't seem to be a lot that can be done to get around that.

The automatic instantiation happens within the Client, but Microsoft users can instantiate their own using similar code to:

openidconnect-rs/src/lib.rs

Lines 986 to 1005 in e9b1c53

let verifier = if let Some(ref client_secret) = self.client_secret {
IdTokenVerifier::new_confidential_client(
self.client_id.clone(),
client_secret.clone(),
self.issuer.clone(),
self.jwks.clone(),
)
} else {
IdTokenVerifier::new_public_client(
self.client_id.clone(),
self.issuer.clone(),
self.jwks.clone(),
)
};
if let Some(id_token_signing_algs) = self.id_token_signing_algs.clone() {
verifier.set_allowed_algs(id_token_signing_algs)
} else {
verifier
}

All of these values are available outside the client, either from users knowing their own client ID and client secret (cloning rather than moving them into the Client) or from the OIDC discovery document (making a copy of provider_metadata.jwks() before moving the metadata into Client::from_provider_metadata).

@SirensOfTitan
Copy link

SirensOfTitan commented Nov 9, 2023

LinkedIn's new OIDC implementations seems to adopt a lot of Microsoft's poor behavior regarding OIDC standards, but they also:

  • encode the payload inside of the ID token in a non-standard way, particularly the email_verified field is encoded as a string literal "true", not true, which causes openidconnect-rs to fail when deserializing the payload in question.
  • delimit the scope field inside the token response with commas, which is non-spec since scope should be space delimited (this "fails" silently because the parser splits by space-delimiters and unpacks into Scopes).
  • The nonce is not included in the id token payload response.

It's probably possible to go and work around this using the http shim method, and I've been able to do so to at least reconstruct the id_token and token response to be spec-compliant, but because the incorrect format of email_verified causes IDToken deserialization to fail, it ends up being a bit of an awkward dance (though until I go and try to validate signatures here, I cannot say how awkward). Needless to say, I don't think this is the concern of this library necessarily, except perhaps in the capacity to go and workaround non-compliant OIDC implementors in a natural way.

@Timshel
Copy link
Contributor

Timshel commented Dec 28, 2023

Hey,
For information it appears there is now a Microsoft Entra v2 endpoint which should be compliant.

You should be able to find it on https://entra.microsoft.com following Identity | Applications | App registrations | Endpoints. And it should look something like this : https://login.microsoftonline.com/${tenantguid}/v2.0

Edit:
Looking at the Microsoft issue the endpoint is mentioned as not compliant in Feb (MicrosoftDocs/azure-docs#38427 (comment)) but 2 peoples mentioned it as now working in : Timshel/vaultwarden#8.

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 a pull request may close this issue.

4 participants