-
Notifications
You must be signed in to change notification settings - Fork 566
fix: ensure correct SAML Entity ID in client SSO flow #2225
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
base: master
Are you sure you want to change the base?
fix: ensure correct SAML Entity ID in client SSO flow #2225
Conversation
|
Explanation of existing incorrect behavior: Existing service provider instantiation does not pass the Lines 43 to 50 in 9a8d0df
...which leaves ...so the Entity ID is incorrectly inferred as |
internal/api/saml.go
Outdated
| // getSAMLServiceProvider generates a new service provider object with the | ||
| // (optionally) provided descriptor (metadata) for the identity provider. | ||
| func (a *API) getSAMLServiceProvider(identityProvider *saml.EntityDescriptor, idpInitiated bool) *saml.ServiceProvider { | ||
| func (a *API) getSAMLServiceProvider(identityProvider *saml.EntityDescriptor, entityID string, idpInitiated bool) *saml.ServiceProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The entityDescriptor here already should include the EntityID, maybe extract it from there instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review @hf - makes sense, this was my initial instinct as well.
I made the entityID argument explicit because we currently only pass the EntityDescriptor when we're in 'client mode' (i.e. authenticating with an external provider).
This function also happens to be invoked when exposing the SAML metadata endpoint ('server mode'), in which case we want to retain the existing behavior (inferring the metadata URL).
I've updated my fix to extract the identityProvider.EntityID only if there was an identityProvider provided, falling back to the previous 'inferred metadata URL' behavior if it is nil (as it is when this function is called from the SAMLMetadata endpoint).
Pull Request Test Coverage Report for Build 18756420374Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
1e95484 to
2df8476
Compare
|
I've addressed your feedback @hf - let me know if there's anything else I can do to help get this over the line :) |
When initiating a SAML client flow via the /sso endpoint, the service provider object Entity ID is omitted from the initialization options, causing the underlying saml library to incorrectly use the metadata URL for the SAML server as the Entity ID. This causes some service providers (e.g. Microsoft Entra ID) to reject the SAML authentication request, as the inferred supabase auth server metadata URL does not match the provider's Entity ID. This change ensures the service provider is correctly initialized with the provider Entity ID during the client auth flow, while retaining the existing behavior for the server metadata endpoint.
2df8476 to
4062183
Compare
When initiating a SAML client flow via the /sso endpoint, the service provider object Entity ID is omitted from the initialization options, causing the underlying saml library to incorrectly use the metadata URL for the SAML server as the Entity ID.
This causes some service providers (e.g. Microsoft Entra ID) to reject the SAML authentication request, as the inferred supabase auth server metadata URL does not match the provider's Entity ID.
This change ensures the service provider is correctly initialized with the provider Entity ID during the client auth flow, while retaining the existing behavior for the server metadata endpoint.