flask-saml2 icon indicating copy to clipboard operation
flask-saml2 copied to clipboard

Timestamps shouldn’t have microseconds

Open FMJansen opened this issue 5 years ago • 5 comments

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?

FMJansen avatar May 08 '20 08:05 FMJansen

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.

mx-moth avatar May 23 '20 06:05 mx-moth

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.

FMJansen avatar May 23 '20 08:05 FMJansen

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.

mx-moth avatar May 23 '20 08:05 mx-moth

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.

ianlintner-wf avatar Aug 31 '20 19:08 ianlintner-wf

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 https://github.com/python/cpython/issues/90772)

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

enavarro222 avatar Dec 14 '23 08:12 enavarro222