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

Empty ttl #250

Closed
soyuka opened this issue Oct 17, 2016 · 23 comments
Closed

Empty ttl #250

soyuka opened this issue Oct 17, 2016 · 23 comments

Comments

@soyuka
Copy link

soyuka commented Oct 17, 2016

#117 breaks exp should be optional.
#233

I haven't studied the spomky-labs/jose library so I'm not sure if 0 is considered as unlimited or not.

@Spomky
Copy link
Contributor

Spomky commented Oct 17, 2016

Hi,

Disclaimer: the following point of view is not the POV of the Lexik team but my opinion about the exp claim

You are right as per the RFC7519 the exp claim (and any other claim) is optional. If this claim is not in the payload then the JWT is considered as a JWT that never expires. This is the way a JWT parser should work. This is the way the spomky-labs/jose library you mentionned works.

But this bundle does not break the RFC. You missed a very important line:

The set of claims that a JWT must contain to be considered valid is context dependent and is outside the scope of this specification.

As you can imagine, in an authentication context, an immortal JWT is a serious problem: if it is compromised you allow a malicious application to get access on your data... for life!

There is a way to avoid such threat: a revocation system to reject Jose that are compromised.
The problem is that the basis of the JWT is to trust on digital signatures to validate or reject them. If you have to manage a revocation table then something went wrong. See this answer and this blog post for more information about that topic (thanks @chalasr for those links).

Moreover, there is a way to get a new token when the current one expired: refresh tokens. This bundle does not support those tokens out of the box. However a great bundle adds this feature.

To conclude: in an authentication context,

  • The exp claim MUST be added.
  • The token should have a short lifetime.
  • To prevent user being requested to submit its credentials more than once per session, refresh tokens should be used.

@soyuka
Copy link
Author

soyuka commented Oct 17, 2016

Nice links, but yeah those are totally making sense to me and I'm aware of the consequences of having an endless JWT.

As you can imagine, in an authentication context, an immortal JWT is a serious problem: if it is compromised you allow a malicious application to get access on your data... for life!

Just changing the key would ban every tokens won't it?

there is a way to get a new acces token when the current one expired: refresh tokens.

Those feel more a hack to me. I mean it's weird to just refresh the token before it expires instead of simply having an endless JWT.

I understand your POV on this, but why can't it be a feature for those who don't want their token to expire?

@Spomky
Copy link
Contributor

Spomky commented Oct 17, 2016

With the v2.0 of the bundle, you can create your own encoder.
Just create one that does not include an exp claim.

The problem when you change your keys is that all valid tokens become invalid.

@soyuka
Copy link
Author

soyuka commented Oct 17, 2016

The problem when you change your keys is that all valid tokens become invalid.

Seems a good solution when you've a security issue related to tokens :).

Nice about the custom encoder, I'll do this. Thanks!

@soyuka soyuka closed this as completed Oct 17, 2016
@chalasr
Copy link
Collaborator

chalasr commented Oct 17, 2016

Hi @soyuka,

To be honest, I was not aware of that the exp claim has been optional ever in this bundle, this breaking change was not expected, sorry for that.

As well explained by @Spomky, claims may be judged mandatory depending on the context.
A JWT (as well as a session cookie) with an infinite lifetime may lead to security holes, and so something we neither want to encourage nor support.

The JOSE libraries on which our token encoders are based on may allow to do not have these claims, of course because they don't have to care about the context in which tokens will be used.

I mean it's weird to just refresh the token before it expires instead of simply having an endless JWT.

You do not refresh the token before it expires but after, that is one of the advantages of refresh tokens in addition of you don't need to force your users to resubmit their credentials. It is not a hack at all, but a way to keep tokens secure and do not bother users, just involving one more hit to the datastore, that's something acceptable in an authentication context.

As said by Spomky, you can easily create your own encoder based on ours.

@soyuka
Copy link
Author

soyuka commented Oct 17, 2016

You do not refresh the token before it expires but after, that is one of the advantages of refresh tokens in addition of you don't need to force your users to resubmit their credentials. It is not a hack at all, but a way to keep tokens secure and do not bother users, just involving one more hit to the datastore, that's something acceptable in an authentication context.

Ok I need to take a deeper look into this! Still, imagine the following scenario:

  • do a request
  • get a 401
  • do a refresh
  • issue the first request again

Seems overkill, but ofc it may be a solution!

A JWT (as well as a session cookie) with an infinite lifetime may lead to security holes, and so something we neither want to encourage nor support.

Totally agreed, but my context is not really the same as an open application which would have to consider such issues.

@chalasr
Copy link
Collaborator

chalasr commented Oct 17, 2016

Seems overkill, but ofc it may be a solution!

That's always better than re-login :) and imho widely acceptable.

Depending on the context, I personally use refresh tokens to perform the same validity checks on the authenticated user as during the first username/password authentication, this way I don't perform these checks at each (JWT) authenticated request, and so keep a security net which is to see the changes made on a user become effective as soon as its token expires.

Note that in combination you can set a very long token_ttl.

I need to take a deeper look into this.

Look at the link given by Spomky, it's easy to try.

If you really want to create your own encoder with an infinite lifetime because you don't need to prevent such issues, I can still try to give you my better advices for rewriting the less possible.

// src/AppBundle/Services/ExpUnawareEncoder.php
<?php

namespace AppBundle\Services;

use Lexik\Bundle\JWTAuthenticationBundle\Encoder\DefaultEncoder;
use Lexik\Bundle\JWTAuthenticationBundle\Exception\JWTDecodeFailureException;

final class ExpUnawareEncoder extends DefaultEncoder
{
    public function decode($token)
    {
        try {
            $jws = $this->jwsProvider->load($token);
        } catch (InvalidArgumentException $e) {
            throw new JWTDecodeFailureException(JWTDecodeFailureException::INVALID_TOKEN, 'Invalid JWT Token', $e);
        }

        if ($jws->isInvalid()) {
            throw new JWTDecodeFailureException(JWTDecodeFailureException::INVALID_TOKEN, 'Invalid JWT Token');
        }

        if (!$jws->isVerified()) {
            throw new JWTDecodeFailureException(JWTDecodeFailureException::UNVERIFIED_TOKEN, 'Unable to verify the given JWT through the given configuration. If the "lexik_jwt_authentication.encoder" encryption options have been changed since your last authentication, please renew the token. If the problem persists, verify that the configured keys/passphrase are valid.');
        }

        return $jws->getPayload();
    }
}
# src/AppBundle/Resources/config/services.yml
services:
    # ...
    app.jwt_encoder:
        class: AppBundle\Services\ExpUnawareEncoder
        arguments: [ '@lexik_jwt_authentication.jws_provider.default' ]
# app/config/config.yml
lexik_jwt_authentication:
    # ...
    encoder:
        service: app.jwt_encoder

Because I squashed your contribution and made a breaking change ^^
Hope you don't need to use it!

@soyuka
Copy link
Author

soyuka commented Oct 17, 2016

Because I squashed your contribution and made a breaking change ^^

😸 so kind of you, thanks!!

@soyuka
Copy link
Author

soyuka commented Oct 25, 2016

Ok, so because this also checks for checkExpiration, you have to override the provider, the encoder and the LoadedJWS (as it's not a service).

@chalasr
Copy link
Collaborator

chalasr commented Oct 25, 2016

@soyuka The encoder being the only one to throw an exception, why is it needed to override the provdider and the LoadedJWS class? Thanks for the feedback

@soyuka
Copy link
Author

soyuka commented Oct 25, 2016

Oh nevermind I thought LoadedJWS was setting Invalid when no token, but I just had to put a numeric one so that this condition won't be met https://github.com/lexik/LexikJWTAuthenticationBundle/blob/master/Signature/LoadedJWS.php#L83.

Your implementation works fine, thanks!

@lsmith77
Copy link

my use case for a token with infinite lifetime is development. obviously for production we would use a separate private/public key value. I guess for now I will just extend the lifetime.

@chalasr
Copy link
Collaborator

chalasr commented Apr 24, 2017

Good point I think. We could allow null depending on %kernel.debug%/%kernel.env%

@lsmith77
Copy link

yeah f.e.

@lsmith77
Copy link

are you willing to reopen this ticket if the scope of token's without ttl is limited to debug mode being enabled?

@chalasr
Copy link
Collaborator

chalasr commented Apr 24, 2017

Reopening, I'm on it.

@chalasr
Copy link
Collaborator

chalasr commented May 2, 2017

Fixed in e1a0f37.
The token lifetime can be infinite again by setting the token_ttl option to null which is highly discouraged in production. I did not add any restriction regarding the environment, after all I think a good default value + a warning in the docs (todo) are enough.

@soyuka
Copy link
Author

soyuka commented May 3, 2017

May you release this one? Thanks!

@chalasr
Copy link
Collaborator

chalasr commented May 3, 2017

@soyuka I will do for the end of the week, I would like to see if we can easily leverage some symfony 3.3 features beforehand.

@lsmith77
Copy link

lsmith77 commented May 4, 2017

thx!

@chalasr
Copy link
Collaborator

chalasr commented May 10, 2017

released in 2.4.0

@tameribrahim
Copy link

@lsmith77 you mentioned private/public key value for production.
I found this authenticator in Symfony docs, how can both ( I mean api-authenticator & JWT authenticators) work together?
So if the request has Authentication header, served by LexikJWTAuthentication, and if it has the key/secret pair, served by the APIAuthenticator?

@Briones
Copy link

Briones commented May 22, 2018

With this changes is not suppoused to be changed the UPGRADE file too?
https://github.com/lexik/LexikJWTAuthenticationBundle/blob/master/UPGRADE.md

Right now it says this:

The token_ttl option must be a numeric value, having an infinite token lifetime is no more supported by the built-in encoders (the exp claim is automatically set), see issue #250 for more details.

gzim324 pushed a commit to gzim324/LexikJWTAuthenticationBundle that referenced this issue Jan 10, 2025
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

6 participants