Error validating issuer URI

[pinpox]

I'm not sure what is happening here, as the "expected" value in the error I'm getting is the same as the one I provided.

This is my code snippet:

async fn get_client_from_sso_config(sso_config: &SsoConfig) -> Result<CoreClient, &'static str> {
    let redirect = sso_config.callback_path.to_string();
    let client_id = ClientId::new(sso_config.client_id.as_ref().unwrap().to_string());
    let client_secret = ClientSecret::new(sso_config.client_secret.as_ref().unwrap().to_string());
    let issuer_url =
        IssuerUrl::new(sso_config.authority.as_ref().unwrap().to_string()).or(Err("invalid issuer URL"))?;
    let test = CoreProviderMetadata::discover_async(issuer_url, async_http_client).await;

    println!("TEST43 {:?}", test);

    let provider_metadata = match test {
        Ok(metadata) => {
            println!("TEST46");
            metadata
        }
        Err(_err) => {
            println!("TEST47");
            return Err("Failed to discover OpenID provider");
        }
    };
// other code
}

When executed I get this error:

TEST43 Err(Validation("unexpected issuer URI `https://git.lounge.rocks/` (expected `https://git.lounge.rocks/`)"))
TEST47
[2022-07-04 15:12:35.409][vaultwarden::api::identity][ERROR] Unable to find client from identifier

Notice the line "unexpected issuer URI https://git.lounge.rocks/ (expected https://git.lounge.rocks/)" Not sure if this is a bug on my side or a wrong error message, but I can't figure out what is wrong here.

[ramosbugs]

Hey, thanks for filing this issue. I pushed a patch in the new 2.3.2 release to make the error message clearer by printing the raw issuer URL that's involved in the verification check rather than the canonicalized Url object. I suspect the issue is that one of the issuer URLs has a trailing slash and the other does not, but hopefully updating to the latest version will reveal the underlying issue.

[pinpox]

It was indeed a trailing slash! Should the validation should be changed to remove trailing slashes or is this something outside the scope of openidconnect-rs?

[ramosbugs]

I think the current validation should work; clients just need to be sure that the issuer URL they provide exactly matches the one that the OIDC provider returns in its discovery metadata and JWTs. The actual discovery fetch should work whether or not the trailing slash is included, so it's just a matter of satisfying the validation.

The relevant part of the spec is:

The issuer value returned MUST be identical to the Issuer URL that was directly used to retrieve the configuration information. This MUST also be identical to the iss Claim value in ID Tokens issued from this Issuer.

[ndrsg]

Is it possible to have a discovery-url that does not follow the scheme:

issuer + /.well-known/openid-configuration ?

In my case we have an additional /oauth/ in the path, like:

Issuer: https://my-domain.com Discovery: https://my-domain.com/oauth/.well-known/openid-configuration

This is not against the spec, is it?

But I cannot use discovery, hence the discovery-url hardcoded depends on the issuer.

If I use Issuer-Url: https://my-comain.com/oauth , the validations fails, hence metadata says iss is https://my-domain.com

If I use Issuer-Url: https://my-domain.com , the discovery-url is wrong and returns a 404

[ramosbugs]

Hi @ndrsg,

I think this does violate the spec, which bases many things on the choice of issuer URL:

OpenID Providers supporting Discovery MUST make a JSON document available at the path formed by concatenating the string /.well-known/openid-configuration to the Issuer.

If the issuer URL is https://my-domain.com/, then the discovery document must be at https://my-domain.com/.well-known/openid-configuration.

If the issuer URL is https://my-domain.com/oauth, then the discovery document must be at https://my-domain.com/oauth/.well-known/openid-configuration and list https://my-domain.com/oauth as its issuer field (which must also match the iss claim in each ID token). The spec is clear about requiring this consistency, and it's enforced by this library (e.g., to prevent potential security issues in cases where different parties may control different subpaths within a domain).

I'm going to go ahead and close this issue since I fixed the error message to address OP's issue.

[punmechanic]

Hey @ramosbugs, there's a rather annoying behavior in the url library which is ostensibly spec-compliant which runs counter to:

The actual discovery fetch should work whether or not the trailing slash is included, so it's just a matter of satisfying the validation.

This library uses url::Url which will forcibly include a trailing slash to any URL:

let url: url::Url = "https://example.com".parse().unwrap();
println!("{}", url.to_string());

> "https://example.com/"

Because this library requires the use of url, the easy straight-forward path of using discovery falls over:

        let md = CoreProviderMetadata::discover(
            &IssuerUrl::from_url(url),
            openidconnect::ureq::http_client,
        )

If the openid configuration on the destination server does not contain a trailing slash, and because there's no way of providing a URL without a trailing slash using url.

Would it be possible to provide some kind of option to bypass the spec compliant behavior in openidconnect for this specific use-case? I am trying to interact with a 3rd party openid configuration, so I have no ability to modify it - and, additionally, we have many other applications using primarily Go's openidconnect library which works just fine against this endpoint.

[punmechanic]

ah, I see that this was mentioned in a subsequent issue, which was closed. this is unfortunate because it prevents me from using this very excellent library as-is, even if it's spec compliant.

[ramosbugs]

Hey @trinitroglycerin, for the specific issue you're mentioning (the url crate's own canonicalization), it should work if you use IssuerUrl::new (passing a string without the trailing slash) instead of IssuerUrl::from_url.

For the exact reasons you cite, all of the URL-based newtypes in this crate store both a parsed Url and a String representation of the original URL passed into the new constructor.

See this comment: https://github.com/ramosbugs/openidconnect-rs/blob/b336c20a64311b8f6289ef894324a09036fdc51e/src/macros.rs#L254-L265

Please open a new issue if that doesn't work, because I'd like to fix it.

[ramosbugs]

^ This is exactly what the first example in the docs does.