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

Inconsistent meaning of provider_id between saml and oidc #3477

Open
ryanhiebert opened this issue Oct 12, 2023 · 5 comments
Open

Inconsistent meaning of provider_id between saml and oidc #3477

ryanhiebert opened this issue Oct 12, 2023 · 5 comments

Comments

@ryanhiebert
Copy link

The implementation and documentation have conflicting stories about what the provider_id is for.

  • The OIDC provider uses the provider_id as the URL slug.
  • The provider_login_url template tag looks up the provider using the provider_id.
    • The provider_login_url template tag is documented to be used to look up the saml provider login url.
  • The SAML provider uses the client_id as the URL slug.
  • The SAML docs indicate that the provider_id is a good place for the IdP entity ID.

Looking at the semantics of these fields in OIDC, it seems to me that SAML is using these fields significantly differently from the original intent, and this may be at least partially due to the fact that the client_id is a required field on the model.

The provider_id, in OIDC, is that identifier that is used in the slug, that is the short code to identify the provider, where the client_id is part of the identification of the service provider to the identity provider. In SAML there are some things that half-fit the intent of client_id (IdP entity ID, SP entity ID, certificate), but none that really have the same semantics.


My suggestion is that client_id should not be required, and that the SAML documentation, urls, and views should use the provider_id the same way that the OIDC provider does. This is a breaking change, but that concern may be mitigated somewhat by the fact that the inconsistency is breaking an existing feature (to look up the provider_login_url), and my hope is that using the provider_id consistently will give implementors an easier path to having a proper intuition about how these fields are used across providers.

Because of these inconsistencies, we've opted to ensure that the provider_id and the client_id are always set the same in our system for now, so that the provider_login_url can work and get the correct URL.

/cc @atribed

@pennersr
Copy link
Owner

There is an inherent difference between OIDC and SAML. The situation for OAuth/OIDC is as follows:

  • OAuth providers use URLs like /accounts/<oauth-provider-id>/login/.

  • For OIDC, this was kept, so that we could easily replace "hardcoded" OAuth providers by OIDC configurations without any impact on the URL structure. So, OIDC also uses: /accounts/<oidc-subprovider-id>/login/ instead of /accounts/oidc/<oidc-sub-provider-id>/login/.

  • For both OAuth and OIDC, you have a client ID and secret pair. Note that this information is volatile -- they can change. Also, client IDs tend to be cryptic. All in all, client IDs are not suitable to be used in URLs.

  • When storing accounts (SocialAccount), you need to appoint a key. The (provider specific) user ID could conflict with other providers, so the key needs to be a tuple of a provider key, and the provider specific user ID. For the provider key, we cannot use the client ID as it is volatile. So, for an OAuth provider like Google the provider key is "google". For an OIDC provider, the key cannot be just "openid_connect" as that could again lead to conflicts. So, we use the subprovider ID (socialapp.provider_id). So, if you setup Google as an OIDC provider, the key is again "google".

For SAML, we have the following situation:

  • In contrast to the static nature of OAuth/OIDC, with SAML, you typically have the situation where your customers can dynamically hookup their SAML provider, meaning, the authentication URL structure is altered on the fly, and care must be taken to avoid conflicts, which is why the SAML URL structure uses a parent saml/ path: /accounts/saml/<organization-id>/.... Where, organization ID needs to URL friendly, and unique per customer. That ID is often derived from user input such as the company name (or even user-inputable). Therefore, this ID can be volatile.

  • For storing accounts, we again need a provider key. Here, "saml" is clearly not valid as that could lead to conflicts, but, using the organization ID is dubious as well as it is volatile. If it somehow changes, e.g. by a user editing the company, that would affect accounts stored so far. Therefore, the IdP entity ID is a better candidate as it is unlikely to change.

  • Just as with OAuth/OIDC, the account provider key is taken from socialapp.provider_id. And, the SAML documentation recommends to populate that with the IdP entity ID for reasons mentioned above.

  • Now, client ID/secret are not used for SAML. And, we still need to store the organization ID somewhere, which led to the choice of using the client ID for that purpose.

The above should give some insights as to why things landed this way. Making provider_id and client_id equal may work in your case (depending on how you derive organization IDs), but it is problematic to assume that way of working holds for all other projects.

@pennersr
Copy link
Owner

but that concern may be mitigated somewhat by the fact that the inconsistency is breaking an existing feature (to look up the provider_login_url)

Just to be clear, you can still lookup the provider -- it is not broken. But, the lookup is indeed by provider_id, which in case of a configuration like that in the documentation, means you need to put in the IdP entity ID.

Perhaps this needs to be extended to support:

{% provider_login_url "saml" client_id="acme-inc" %}

@pennersr
Copy link
Owner

@ryanhiebert Would the above suit your needs?

@ryanhiebert
Copy link
Author

Just to be clear, you can still lookup the provider -- it is not broken.

You're correct, this isn't broken. You just use the provider_id in place of the provider, so you don't use saml.

{% provider_login_url "my-provider-id" %}

I was thinking that I should expect whatever provider I put in that template to be part of the URL, but that's not necessarily a good assumption, as you describe. Once I have a SocialApp, it's easy enough to get the provider_id in whatever way.

I could imagine, if I can't convince you to adjust the way you're using provider_id and client_id below, that it might be useful to similarly allow lookup by the client_id in this template tag, either with the syntax you've suggested or perhaps by allowing that as an alternative to the provider argument, but I suspect that I wouldn't end up using it that way anyway.


Thank you for sharing more about how you're thinking about OIDC and SAML and these parameters. I see your motivation, and they make sense. It does seem to me that there is a stronger parallel we should be respecting between OIDC and SAML, and stronger separation between OIDC and OAuth.

OAuth and OpenID Connect are notably different because OAuth isn't actually a provider. It's some generic tooling to make a provider easy to write. OpenID Connect is a provider itself. It makes sense that when you were writing OpenID connect initially you drew parallels to OAuth, but I think the parallels with SAML are actually stronger.

To support my case that you should consider SAML and OIDC to be more strongly related, I'd like to point out that OIDC is commonly used as an SSO protocol just like SAML is. I have particular experience using OIDC for SSO with Amazon Cognito, and I don't think that Cognito is an anomaly among SSO implementation services. SAML is much more common for that purpose, but OIDC seems to be picking up some steam as a preferable protocol for SSO.

If there was another provider that OpenID Connect would arguably be similarly closely related to, that would be the OpenID provider, but TBH I don't even know whether OpenID Connect supports a mode where all you do is enter an OpenID (often an email address) and it will take you to that domain's OpenID login. That was a thing with the original OpenID, and is how the OpenID provider works, but it is not how the current OpenID Connect provider is implemented, anyway. What is currently implemented is more closely related to SAML.


Your concern about necessity for stability of the provider_id is a good one. For all the reasons that you've laid out, changing the provider ID for OpenID Connect is unwise. At minimum, to do it right would mean adjusting any other places in the database that are similarly relying on that provider id, and that is not easy or implemented.

However, because I see that OpenID Connect and SAML are very similarly positioned, all the motivations you've mentioned for SAML seem to also apply to OIDC. To my thinking, this bolsters the case that we should pattern the SAML provider's use of the provider_id from the OIDC provider.


Based on your explanation, I would suggest that you document that the provider_id should not be volatile, and I would maintain that constraint both in OpenID Connect as well as SAML. It could be useful to provide a utility function to allow for changing a provider_id, but that's not necessary. Because it has unintended consequences, you might even make the field readonly after create in the Django admin.


I'd still like the provider_id for SAML to follow the pattern of OIDC to use it in the URL, as I suggested previously, because I hope that this can help make a cohesive and intuitive understanding of the SAML and OIDC SSO protocols easier. This change to the SAML would be a breaking change, so probably the right transition strategy would be to introduce a flag to opt into or out of the new behavior, deprecate it, and then at some point in the future you might be able to eliminate the second code path.

@ryanhiebert
Copy link
Author

Also, and for what it's worth, I consider URLs for both OpenID Connect and SAML to be things that need to be basically immutable. Coordinating those kinds of changes with customers is a big pain, and I don't personally intend to give them the option to change their URL.

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

No branches or pull requests

2 participants