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

Correct timestamp format #31

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

Alch-Emi
Copy link

@Alch-Emi Alch-Emi commented Jun 7, 2021

This makes two changes to the timestamp format used in assertions.

  1. It removes the millisecond component, per issue Timestamps shouldn’t have microseconds #23, and
  2. It removes the timezone component, which is explicitly disallowed by the SAML spec (section 1.3.3)

I originally started working on this pull request because I believed my IDP (SimpleSAMLphp) did not tolerate timezones in timestamps, but it turns out that SimpleSAMLphp is actually slightly spec non-compliant itself and requires timestamps to be formatted as having a literal Z at the end.

For now I'm going to use a fork of flask-saml2 to prop up my IDP, and PR the upstream, although this lends credence to @ianlintner-wf's suggestion of making datetimes customizable

Please let me know if you suggest any other changes.

This makes two changes to the timestamp format used in assertions.

First, it removes the millisecond component, per issue mx-moth#23, and
secondly, it removes the timezone component, which is explicitly
disallowed by the SAML spec (section 1.3.3)
@Alch-Emi
Copy link
Author

Alch-Emi commented Jun 7, 2021

It seems like the checks are failing on an unrelated dependency issue, but please let me know if this is something in my code

@Alch-Emi
Copy link
Author

Alch-Emi commented Jun 8, 2021

I was recently informed that the Z at the end of the timestamp is mandatory, and I've added it to the timestamp

eieste added a commit to eieste/flask-saml2 that referenced this pull request Feb 13, 2022
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.

1 participant