Inconsistent handling of leeway?

[fvkluck]

Hi,

First of all, thank you for creating + maintaining this package.

I'm on drf-oidc-auth version 1.0.0 and Authlib 0.15.3.

I was looking into the code for handling the 'exp' and 'iat' headers, and I have the feeling there's some inconsistency in how the leeway parameter is used/interpreted. Let's introduce two names:
relative leeway: a time delta. Accept the header if the difference between current time and stated time is no more than this time delta.
absolute leeway: a boundary value obtained from subtracting the relative leeway from the current time.

In class JSONWebTokenAuthentication, method validate_claims in oidc_auth/authentication.py, the function id_token.validate is called with an absolute leeway:

            id_token.validate(
                now=int(time.time()),
                leeway=int(time.time()-api_settings.OIDC_LEEWAY)
            )

The DRFIDToken defined in oidc_auth/authentication.py indeed seems to assume an absolute leeway (self['iat'] < leeway):

class DRFIDToken(IDToken):

    def validate_exp(self, now, leeway):
        super(DRFIDToken, self).validate_exp(now, leeway)
        if now > self['exp']:
            msg = _('Invalid Authorization header. JWT has expired.')
            raise AuthenticationFailed(msg)

    def validate_iat(self, now, leeway):
        super(DRFIDToken, self).validate_iat(now, leeway)
        if self['iat'] < leeway:
            msg = _('Invalid Authorization header. JWT too old.')
            raise AuthenticationFailed(msg)

However, the validation of exp in package authlib , file rfc7519/claims.py (which is what the super call leads to) looks as follows:

    def validate_exp(self, now, leeway):
        """The "exp" (expiration time) claim identifies the expiration time on
        or after which the JWT MUST NOT be accepted for processing.  The
        processing of the "exp" claim requires that the current date/time
        MUST be before the expiration date/time listed in the "exp" claim.
        Implementers MAY provide for some small leeway, usually no more than
        a few minutes, to account for clock skew.  Its value MUST be a number
        containing a NumericDate value.  Use of this claim is OPTIONAL.
        """
        if 'exp' in self:
            exp = self['exp']
            if not _validate_numeric_time(exp):
                raise InvalidClaimError('exp')
            if exp < (now - leeway):
                raise ExpiredTokenError()

The line exp < (now - leeway) is a check for a relative leeway.

I notice that there has been a recent change from jwkest to authlib, so I guess this may be related. However, I haven't been able to find exact proof that this is where this inconsistency originates.

Could you confirm if my analysis is correct and/or sounds plausible? And if so, what your ideal solution would look like? I could imagine that the checks in this package's code would also have to change to relative leeways. If so, I might be able to supply a pull request fixing this.

Thanks,
Florian

[kerrermanisNL]

Hi Florian,

First of all, thanks for the kind words! Having a look at the code you provided I think you are correct. It looks like relative leeways should be passed to the authlib package. At least in the case you provided. This was indeed introduced in the move to authlib. A pull request for a fix would be much appreciated indeed!

[fvkluck]

Ok, I've started on a fix here: fvkluck@e70ccd1

The check on expiry is already done in the authlib package, so I've removed it. The check on the issued at (iat) is more interesting: I think as it was currently implemented, it used the leeway as a sort of maximum age that a token was allowed to have. However, according to the specs, the leeway is there to provide some extra margin when checking the expiry to allow for clock differences.
I can either remove the check altogether, or see if I can add a setting to specify the max age (although I'm not sure I'd expect such a check from a package providing oidc).

Also, as a side note, does exp < (now - leeway) in authlib also seem suspicious to you? The leeway here actually makes it harder to conform to the check, rather than loosening it, which seems to be the intent of the leeway.

Be careful, I have not done proper testing yet!

[kerrermanisNL]

Also, as a side note, does exp < (now - leeway) in authlib also seem suspicious to you? The leeway here actually makes it harder to conform to the check, rather than loosening it, which seems to be the intent of the leeway.

Yes now that you put it like that, it would make more sense to add it then subtract it? Unless leeway is supposed to be a negative number, but that seems counter-intuitive. Perhaps something to inform about @authlib.

The check on expiry is already done in the authlib package, so I've removed it. The check on the issued at (iat) is more interesting: I think as it was currently implemented, it used the leeway as a sort of maximum age that a token was allowed to have. However, according to the specs, the leeway is there to provide some extra margin when checking the expiry to allow for clock differences.
I can either remove the check altogether, or see if I can add a setting to specify the max age (although I'm not sure I'd expect such a check from a package providing oidc).

Hm, yes in accordance to the specification that's probably not correct, however there might still be some people who'd want to set a max-age. A different setting for that would probably be more suitable, so people upgrading version are still able to keep their current working situation (with a change to the settings).

Thanks so far!

[fvkluck]

Regarding the exp < (now - leeway): on reading it again, I noticed that it is in fact correct. The full code part is

if exp < (now - leeway):
                raise ExpiredTokenError()

So indeed the leeway makes it 'harder' to conform to the check, but the check is the check for marking it as expired.

I'll look into the fix and adding the setting a bit later.

[BlackVoid]

Related to the handling of leeway I'm not sure why the validation of iat is implemented the way it is.
Currently it checks if the key is newer than the absolute leeway, however to me this does not make sense, since you would use the expiration field to check the key has expired and if it's valid for too long adjust the time the key is valid. I think the iat check should instead be if current time is greater than iat.

I'd be happy to create a PR to change this behavior, but I'd like to get some input before doing it.

Current check:

class DRFIDToken(IDToken):
    [...]
    def validate_iat(self, now, leeway):
        super(DRFIDToken, self).validate_iat(now, leeway)
        if self['iat'] < leeway:
            msg = _('Invalid Authorization header. JWT too old.')
            raise AuthenticationFailed(msg)

Suggested check:

class DRFIDToken(IDToken):
    [...]
    def validate_iat(self, now, leeway):
        super(DRFIDToken, self).validate_iat(now, leeway)
        # Alternatively the check can be `self['iat'] > now - leeway`
        if self['iat'] > now:
            msg = _('Invalid Authorization header. JWT is not yet valid.')
            raise AuthenticationFailed(msg)

[fvkluck]

@kerrermanisNL : I've been staring at the code a bit, and the current state of the authlib package is that it provides the freedom to easily add custom checks. I don't think it would make sense for drf-oidc-auth, as connector between drf and authlib, to add some extra custom way of setting the max age of the token.
Something like this should do the trick:

{
            "iss": {
                "essential": True,
                "values": ["https://example.com", "https://example.org"]
            },
            "sub": {
                "essential": True
                "value": "248289761001"
            },
            "jti": {
                "validate": validate_jti
            }
            "iat": {
                "essential": True,
                "validator": lambda iat: iat - int(time.time()) <= MAX_AGE
            }
 }

This would allow users of this package to upgrade to the newer version, with the same functionality, with the cost of changing their configuration.

Do you agree? My proposal would be that I finish the testing of the work I started on and test my iat validator configuration and then open a pull request. I think that would be a change that, in addition to solving a bug, would also make the code cleaner in that it moves more of the responsibility of (allowing) checks to authlib.
And if I want to add the max age configuration to the documentation, would you prefer it in the changelog or in the readme? Or do you prefer not to mention it in the docs?

[kerrermanisNL]

@fvkluck yes now that you put it like that, that seems to make sense. Making this change does look like it will break existing users their setup (unless they make changes, which is fine, though in that case also increase the minor version/number of the package). Though that's an assumption since it looks like this max-age stuff was more implicitly defined than it was really a feature by the looks of it.

Still it would probably nice to mention in the changelog that the way of handling max age has changed, and refer to the README which should explain how one should now set the max age setting.

Thanks for all the effort you've put in so far, it's much appreciated!

[fvkluck]

Sorry for leaving this hanging for 2 months. I had some time this morning to look into this and have added some documentation and also ran the test suite. The current state of the code can be found here

I don't have Python 3.5, 3.6 and 3.7 installed on my system, so I haven't ran any tests for those. However, something interesting happened with the Python 2.7 tests:

where the error message was:
ImportError: no module named authlib.jose.errors.
Do you have a clue what might be going on here? Otherwise I'll look into the exact versions of the packages involved, and the changes between the versions.

Also, now I just removed the test on old tokens. I've tried to test the particular validator in the test, but I couldn't get that to work. I've tried adding the validator to the tests/settings.py, and to mock the JSONWebTokenAuthentication.claims.options during my test, but neither of those had the desired effect. When debugging it looked like the call to jwt.decode on line 157 of the tests (in JSONWebTokenAuthentication.decode_jwt) seemed to be mocked as well.
I'm not that familiar with testing in Django/DjangoRestFramework. Any ideas on how I can add a test case for this? Or can I leave out a test case because it was more implicitely defined than a feature, so that the package doesn't have to guarantee that it works? It's more of a feature of authlib anyway.

Could you have a general look at the code/docs and confirm that this is as you would accept it? In that case I'll try to hunt down the ImportError. Thanks in advance!

[Guest007]

Ok, I've started on a fix here: fvkluck@e70ccd1

May be you'll make a PR? It is very useful.