Skip to content

Conversation

@kalavt
Copy link

@kalavt kalavt commented May 6, 2025

SAML integration with OKTA has issue

Oops! An Error Occurred
The server returned a "400 Bad Request".
Something is broken. Please let us know what you were doing when this error occurred. We will fix it as soon as possible. Sorry for any inconvenience caused.

the root cause is OneLogin SAML PHP library didn't well handle assertionNotOnOrAfter from OKTA response.
hence

  1. patch the saml.php extractData, manually extract NotOnOrAfter from raw xml
  2. remove the Expired SAML Assertion or allow 5 seconds of clock skew if we got null notValidAfter
if ($nowUtc->greaterThanOrEqualTo($notValidAfter->addSeconds(5))) {
    Log::warning('SAML assertion is expired (with 5s clock skew).');
    abort(400, "Expired SAML Assertion");
}

here's a patch of solution 1: patch the saml.php extractData, manually extract NotOnOrAfter from raw xml

@kalavt kalavt requested a review from snipe as a code owner May 6, 2025 14:26
@kalavt
Copy link
Author

kalavt commented May 6, 2025

Hi @snipe
I've messed up with original PR.
#16869 (comment)

here's it, patching on develop branch.

@kalavt
Copy link
Author

kalavt commented May 7, 2025

@snipe may I have your feedback regards, thanks.

@snipe snipe requested a review from uberbrady May 7, 2025 11:12
Copy link
Member

@uberbrady uberbrady left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems simple enough but any time we start to mess around with Authentication it tends to be kinda scary :/

I'd like to know a little bit more about what Okta is sending for its "NotOnOrAfter" value, such that we can't seem to parse it. Are they violating the spec? (That's not uncommon, unfortunately).

I had a couple more comments I sprinkled throughout - would love it if you could get back to me on those.

I know we've had plenty of Okta SAML users in the past, and they don't seem to have needed this change - if there's an Okta-side configuration you can change to make this not needed I would definitely prefer that if possible.

Regardless, thank you for this contribution - it's very easy to read and it's very nicely targeted to what it needs to do!

@kalavt
Copy link
Author

kalavt commented May 8, 2025

hi @uberbrady,
thanks for looking into this.
yes, we have integrated with OKTA years ago and it works well (v6.1.2, we stick on this version for a long while)
but days ago I decided to upgrade the version to v8.1.2, the SSO login was broken.
we haven't change any on our OKTA SAML settings. I thought might be cause by code change.

plus, OKTA implementation of SAML protocol isn't something we can change.

Copy link
Member

@uberbrady uberbrady left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depending on what the output that I asked for looks like ($assertionNotOnOrAfter) - I still think we're going to need to load that into Carbon - with a line similar to the other output that you shared - new Carbon\Carbon($blah, 'UTC'); - because I imagine that's going to come back as a string and not an integer, which is what we're expecting here (I checked the source, and it normally uses a utility class to parse that value. Which obviously won't work for us (because if it did, we wouldn't need this to start with)).

@uberbrady
Copy link
Member

I appreciate your contribution here, but since Okta is one of our most popular SAML integrations, and I haven't heard about any problems about it, I am just a little too uncomfortable with taking code contributions to fix something that we aren't sure is broken, especially when we aren't exactly sure how it works.

So I appreciate you trying to contribute back, but we're going to close this for now.

For your own edification, I would wonder if perhaps the server you're using to connect to Okta might possibly have an incorrect clock? That might be how you're running into this problem.

@uberbrady uberbrady closed this May 14, 2025
@kalavt
Copy link
Author

kalavt commented Jul 14, 2025

<saml2:Assertion ID="id-445620044419653696966749045" IssueInstant="2025-07-14T03:17:30.349Z" Version="2.0"
xmlns:saml2="urn:oasis:names:tc:SAML:2.0:assertion">
<saml2:Issuer Format="urn:oasis:names:tc:SAML:2.0:nameid-format:entity">http://www.okta.com/exk71gs8prg99yTo8607</saml2:Issuer>
saml2:Subject
<saml2:NameID Format="urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress">[email protected]</saml2:NameID>
<saml2:SubjectConfirmation Method="urn:oasis:names:tc:SAML:2.0:cm:bearer">
<saml2:SubjectConfirmationData NotOnOrAfter="2025-07-14T03:22:30.349Z" Recipient="https://snipeit.domain.com/saml/acs"/>
</saml2:SubjectConfirmation>
</saml2:Subject>
<saml2:Conditions NotBefore="2025-07-14T03:12:30.349Z" NotOnOrAfter="2025-07-14T03:22:30.349Z">
saml2:AudienceRestriction
saml2:Audiencehttps://snipeit.domain.com/saml/acs</saml2:Audience>
</saml2:AudienceRestriction>
</saml2:Conditions>
<saml2:AuthnStatement AuthnInstant="2025-07-14T03:16:45.770Z" SessionIndex="id1752463050348.2080915609">
saml2:AuthnContext
saml2:AuthnContextClassRefurn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport</saml2:AuthnContextClassRef>
</saml2:AuthnContext>
</saml2:AuthnStatement>
</saml2:Assertion>

here's the [Preview the SAML Assertion], I don't think its only my issue,
cause this is the standard of OKTA SAML application assertion. @uberbrady

@uberbrady
Copy link
Member

So if I'm reading this right, it looks like this authorization was created at 3:16am UTC. It's not valid for before 3:12am, and not valid on-or-after 3:22am. But the format of the timestamp for each of those fields looks correct to me, and they all look like the exact same format. My gut here is that you have a server who is on the wrong timezone, or set to the wrong time.

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

Successfully merging this pull request may close these issues.

2 participants