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

Timestamps shouldn’t have microseconds #23

Open
FMJansen opened this issue May 8, 2020 · 5 comments
Open

Timestamps shouldn’t have microseconds #23

FMJansen opened this issue May 8, 2020 · 5 comments

Comments

@FMJansen
Copy link

FMJansen commented May 8, 2020

The timestamps produced with .isoformat() contain microseconds, in the idphandler.py and sphandler.py format_datetime functions. According to the specs [PDF] in section 1.3.3:

SAML system entities SHOULD NOT rely on time resolution finer than milliseconds.

(thanks Duo)

So should those functions use return value.strftime('%Y-%m-%dT%H:%M:%SZ') instead?

@mx-moth
Copy link
Owner

mx-moth commented May 23, 2020

Perhaps! I suppose it depends upon your interpretation. It does not say SAML systems should not produce milliseconds, only that the system should not rely on them. My interpretation is that when checking whether a request is within the allowed timestamp range, milliseconds should be ignored and timestamps rounded to only second accuracy.

That being said, flask-saml2 doesn't do that either, so it conforms to neither interpretation of he spec as it currently stands.

@FMJansen
Copy link
Author

Aah, yes that’s fair. I did run into an issue with one IDP with the timestamps, but I guess that is because of the difference in interpretation/implementation of the specs between that IDP and this framework.

@mx-moth
Copy link
Owner

mx-moth commented May 23, 2020

I have also run in to such a picky IdP, which is why I made the format_datetime() method in the first place. Given that picky IdP's seem more common than I anticipated, perhaps dropping the microseconds is the best approach, while keeping the format_datetime() function around so that IdP's that are inevitably picky in yet another specific and frustrating fashion can be accommodated.

@ianlintner-wf
Copy link

ianlintner-wf commented Aug 31, 2020

One option would be provide a timestamp format on the configuration for the specific handler but default to microseconds if no config provided. If there was going to be PR on this.

Personally I needed to override the idphandler class anyways for other business logic reasons in one implementation so I solved it that way.

Alch-Emi added a commit to Alch-Emi/flask-saml2 that referenced this issue Jun 7, 2021
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)
@enavarro222
Copy link

I got an issue to make our IdP work with a Zoho Commerce shop, it turns out that it was that issue with dateformat. Datetime should have milliseconds and not microseconds, and furthermore UTC offset should be encoded with "Z" and not "+00:00"

I guess this comment may help someone who got issue using flask-saml with some service providers.

Here is the custom SPHandler I used to resolv the issue:

class ZohoSPHandler(SPHandler):
    def format_datetime(self, value: datetime) -> str:
        """
        >>> from pytz import UTC
        >>> handler = ZohoSPHandler(None, entity_id=None)
        >>> handler.format_datetime(datetime(2012, 10, 10, 3, 5, tzinfo=UTC))
        '2012-10-10T03:05:00.000Z'
        >>> handler.format_datetime(datetime(2012, 10, 10, 3, 5, 12, 3456, tzinfo=UTC))
        '2012-10-10T03:05:12.003Z'
        """
        return value.isoformat(timespec='milliseconds').replace("+00:00", "Z")

Note that timespec='milliseconds' is only supported since python 3.6. (Also note that, in the future, there may exists a more clean way to get a Z for UTC see python/cpython#90772)

We got that issue with both this package and with infobyte fork (https://github.com/infobyte/flask-saml2).

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

4 participants