jwt icon indicating copy to clipboard operation
jwt copied to clipboard

Added withClaim token constraint

Open james-bw opened this issue 2 years ago • 9 comments

Re: https://github.com/lcobucci/jwt/issues/826

james-bw avatar Mar 30 '22 06:03 james-bw

@Ocramius https://github.com/lcobucci/jwt/runs/5749877948?check_suite_focus=true#step:4:250

[BC] ADDED: Method hasClaimWithValue() was added to interface Lcobucci\JWT\UnencryptedToken
[BC] ADDED: Method hasClaimWithValue() was added to interface Lcobucci\JWT\Token
2 backwards-incompatible changes detected

Why adding a method to an interface should be considered a BC Break?

Slamdunk avatar Mar 30 '22 07:03 Slamdunk

Adding new methods to an interface requires implementation of it in every class implementing the interface. This is incompatible with "I just update and everything works". :)

SvenRtbg avatar Mar 30 '22 07:03 SvenRtbg

Ok, now I see the need for https://wiki.php.net/rfc/sealed_classes as in this context Lcobucci\JWT\UnencryptedToken and Lcobucci\JWT\Token are not meant to be implemented outside this library

Slamdunk avatar Mar 30 '22 07:03 Slamdunk

@lcobucci before adding this, please check the premise in #826 - the entire idea (so far) smells, IMO

Ocramius avatar Mar 31 '22 23:03 Ocramius

@lcobucci before adding this, please check the premise in https://github.com/lcobucci/jwt/issues/826 - the entire idea (so far) smells, IMO

@Ocramius I saw it... I'm honestly abstracting that page because I think it's useful to have a generic constraint for custom claims in the lib.

lcobucci avatar Apr 01 '22 12:04 lcobucci

Just to expand on the reason for a closure... the initial motivation to introduce this is related to hash comparison and doing that operation with === opens a security issue (timing attacks) - that's why we use hash_equals().

@james-bw have you thought about that? This PR does NOT add a safe way to perform hash comparison.

lcobucci avatar Apr 04 '22 08:04 lcobucci

Also, you can run make to execute most of the checks and solve the problems locally.

lcobucci avatar Apr 04 '22 08:04 lcobucci

Just to expand on the reason for a closure... the initial motivation to introduce this is related to hash comparison and doing that operation with === opens a security issue (timing attacks) - that's why we use hash_equals().

@james-bw have you thought about that? This PR does NOT add a safe way to perform hash comparison.

I did not. I am not aware of how === leads to a timing attack. I'll have to investigate further.

...

OK I understand the timing attack now, and understand why === is not to be used for cryptographic stuff. However, custom claim validation is no different to existing code:

    public function isIdentifiedBy(string $id): bool
    {
        return $this->claims->get(RegisteredClaims::ID) === $id;
    }

    public function isRelatedTo(string $subject): bool
    {
        return $this->claims->get(RegisteredClaims::SUBJECT) === $subject;
    }

I think this validation should be consistent.

james-bw avatar Apr 05 '22 00:04 james-bw

Also, you can run make to execute most of the checks and solve the problems locally.

I'm developing on Windows/git bash, so don't have make natively. I've been trying to get my IDE working with the equivalent tests. At this stage I'm about to walk away and leave you to it, I was just trying to help with a quick win and this is turning into ben hur.

james-bw avatar Apr 05 '22 00:04 james-bw