erlef / oidcc

OpenId Connect client library in Erlang & Elixir
https://hexdocs.pm/oidcc
Apache License 2.0
184 stars 49 forks source link

Accept issuer with and without trailing slash #349

Closed mworrell closed 6 months ago

mworrell commented 6 months ago

When fetching the configuration, accept an issuer with and without a trailing / on the URL.

This fixes an inconsistency between OIDCC providers where sometimes they need a trailing / (Auth0) and sometimes they do not want a trailing / (Surfconext).

After this patch returned issuers with and without trailing / are accepted.

maennchen commented 6 months ago

Thanks for your PR @mworrell :heart:

Can you elaborate why you wouldn't configure the issuers exactly how the issuer wants it?

https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfigurationValidation

The issuer value returned MUST be identical to the Issuer URL that was used as the prefix to /.well-known/openid-configuration to retrieve the configuration information. This MUST also be identical to the iss Claim value in ID Tokens issued from this Issuer.

Based on the language in the specification, I'd rather not allow the ambiguity.

mworrell commented 6 months ago

That is true, and with Auth0 I see the / at the end, where Surfconext does return the one without the /.

The problem I was facing is that with the Zotonic OIDC module a manager simply adds the issuer to the configuration. Looking at the documents returned at the well-known discover URL I see that both do supply the correct issuer domain.

A mismatch is resulting in a crash of the configuration worker, so I will sniff out the configuration document myself and lookup the issuer in there.

Is it correct that I will need to fetch the document myself before handing it to oidcc_provider_configuration:decode_configuration/1 (as mentioned in the comment of the /2 function?

No problem if needed, just one call away :-)

Please close this pull request!

maennchen commented 6 months ago

@mworrell I see. The issue mismatch is just one error that could happen. It might also yield other validation errors and therefore crash the worker.

If you can't handle the worker crash, you'll indeed have to load the configuration yourself. There's two functions for that:

Your options therefore are:

I'm not exactly sure how you implement it. If this is based on user input, you could theoretically use load_configuration to validate the user input in your controller and only start the worker if the load was successful.

mworrell commented 6 months ago

I have now changed the creation of a new OpenID provider. The user must now give the domain and we fetch the openid-configuration to check for the issuer-url. Only if that succeeds we add the OpenID provider (with issuer-url) to the database.

maennchen commented 6 months ago

@mworrell Sounds good 👍

Just to be sure: When you write “domain”: Do you mean the user provides the issuer or an actual domain?

There can also be multiple issuers per domain in different paths.

mworrell commented 6 months ago

The actual domain - which we then use to find the issuer using the openid-configuration at the well-known location.

maennchen commented 6 months ago

@mworrell You'll have to accept an URL for the issuer. Example:

https://login.microsoftonline.com/common/v2.0

The openid configuration is located under https://login.microsoftonline.com/common/v2.0/.well-known/openid-configuration and https://login.microsoftonline.com/.well-known/openid-configuration will produce a 404.

maennchen commented 6 months ago

https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfigurationRequest

Using path components enables supporting multiple issuers per host. This is required in some multi-tenant hosting configurations. This use of .well-known is for supporting multiple issuers per host; unlike its use in RFC 5785 [RFC5785], it does not provide general information about the host.

mworrell commented 6 months ago

Hmm the configuration at https://login.microsoftonline.com/common/v2.0/.well-known/openid-configuration gives an issuer of https://login.microsoftonline.com/{tenantid}/v2.0. I guess that might be problematic for our issuer matching?

I am wondering what is wise to do now, as I do have the trailing / problem, so I can't just let people configure an issuer - as that will definitely mess the issuer-matching up.

Maybe I can let people define an optional path part for the "domain". That would fix this path-component problem.

But that would not fix the {tenant} problem of MS.

maennchen commented 6 months ago

@mworrel

I guess that might be problematic for our issuer matching?

Not the best example, they are not conforming to the standard. Keycloak for example does that as well though. I can't think of another public example right now....

so I can't just let people configure an issuer - as that will definitely mess the issuer-matching up.

I don't really understand your problem still. You can just let the user input an issuer URL. When validating that input, you call load_configuration. If you get an error back, just ask the user to fix it.

If all validates, store the URL and start a configuration worker.

But that would not fix the {tenant} problem of MS.

They are not complying with the OpenID specification. If you want users to be able to use providers that are not conformant, you'll have to implement ways to specify quirks: https://hexdocs.pm/oidcc/oidcc_provider_configuration.html#t:quirks/0

mworrell commented 6 months ago

My basic problem was that the issuer urls provided were not consistent with what the service returned. That is now fixed by letting the user provide a domain with an optional page-path. This is then used for discovery.

This way they can still copy/paste whatever they received from their auth provider.

So all well now :-)