mx-moth / flask-saml2

Flask library for building SAML Service Providers and Identity Providers
MIT License
70 stars 61 forks source link

Timestamps shouldn’t have microseconds #23

Open FMJansen opened 4 years ago

FMJansen commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.

enavarro222 commented 11 months ago

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).