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

SAML Metadata parsing does not correctly parse valid duration values. #1697

Open
2 tasks done
lwjameson opened this issue Jul 31, 2024 · 5 comments
Open
2 tasks done
Labels
bug Something isn't working

Comments

@lwjameson
Copy link

Bug report

  • I confirm this is a bug with Supabase, not with my own application.
  • I confirm I have searched the Docs, GitHub Discussions, and Discord.

Describe the bug

We have a customer that we are setting up with SAML authentication. Their metadata EntitiesDescriptor contains the following:

(Sanitized)
<md:EntitiesDescriptor xmlns:md="urn:oasis:names:tc:SAML:2.0:metadata" xmlns:ds="http://www.w3.org/2000/09/xmldsig#" xmlns:mdui="urn:oasis:names:tc:SAML:metadata:ui" xmlns:mdattr="urn:oasis:names:tc:SAML:metadata:attribute" xmlns:mdrpi="urn:oasis:names:tc:SAML:metadata:rpi" xmlns:shibmd="urn:mace:shibboleth:metadata:1.0" xmlns:xrd="http://docs.oasis-open.org/ns/xri/xrd-1.0" ....snip... validUntil="2024-08-09T23:11:46Z" cacheDuration="PT5H">

When adding this provider via supabase sso add we receive the following error:

Unexpected error adding identity provider: {"message":"Unexpected failure, please check server logs for more information"}

The server logs show:
(Sanitized)

{"component":"api","error":"strconv.ParseInt: parsing \"PT5H\": invalid syntax","level":"error","method":"POST","msg":"Unhandled server error: strconv.ParseInt: parsing \"PT5H\": invalid syntax","path":"/admin/sso/providers","referer":"https://<removed>","remote_addr":"3.95.37.194","request_id":"8abf1e44f73505f5-IAD","time":"2024-07-31T16:55:49Z"}

According to the spec:

https://docs.oasis-open.org/security/saml/v2.0/saml-schema-metadata-2.0.xsd

This is the cacheDuration schema:

<attribute name="cacheDuration" type="duration" use="optional"/>

According to the XML schema:

https://www.w3.org/TR/xmlschema-2/#duration

3.2.6.1 Lexical representation The lexical representation for duration is the [[ISO 8601]](https://www.w3.org/TR/xmlschema-2/#ISO8601) extended format PnYn MnDTnH nMnS, where nY represents the number of years, nM the number of months, nD the number of days, 'T' is the date/time separator, nH the number of hours, nM the number of minutes and nS the number of seconds. The number of seconds can include decimal digits to arbitrary precision.

This is a valid way of specifying a duration.

To Reproduce

  1. Create an XML metadata file that uses Lexical representation to specify cacheDuration
  2. Attempt to add this to an instance using supabase sso add
  3. Get error

Expected behavior

Valid SAML metadata is processed successfully.

@lwjameson lwjameson added the bug Something isn't working label Jul 31, 2024
@lwjameson
Copy link
Author

Hi SB Auth. This is a real bug and needs to be addressed. We have a paying customer who cannot access their site.

@kangmingtay
Copy link
Member

hi @lwjameson, can you please open a ticket with us at https://supabase.help and link this issue in the ticket so we can investigate further?

@hf
Copy link
Contributor

hf commented Aug 16, 2024

Looks like this is a bug in the underlying library, the EntityDescriptor type correctly parses the Duration but not the EntitiesDescriptor (plural!): https://github.com/crewjam/saml/blob/main/metadata.go#L30-L103

Will take a while to fix given that the library has been known to slowly take on fixes.

This is because we use this method to parse the SAML Metadata: https://github.com/crewjam/saml/blob/bbccb7933d5f60512ebc6caec7120c604581983d/samlsp/fetch_metadata.go#L36

@hf
Copy link
Contributor

hf commented Aug 16, 2024

Proposed a fix: crewjam/saml#575

@hf
Copy link
Contributor

hf commented Aug 16, 2024

If we don't see any movement on this in the library, the only option we have is to essentially fork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants