Improve lcobucci/jwt usage in Apple provider #1355
Conversation
There was a problem hiding this comment.
This file suggests it might be used to implement the "none" token signature, which is known to create security issues, as "none" is a value that is attacker-provided and might be used to trick vulnerable JWT validators in ignoring the signature.
I have not investigated if this class could be abused for such a scenario, but it looks scary.
There was a problem hiding this comment.
This class is a copy of the InMemory class from lcobucci/jwt with the "empty key" check removed. It is used to get an instance of the Configuration class without needing to provide a key that isn't used at all.
src/Apple/Provider.php
Outdated
| new AppleSignerNone, | ||
| AppleSignerInMemory::plainText('') | ||
| ); | ||
| $token = $jwtContainer->parser()->parse($jwt); |
There was a problem hiding this comment.
I know the documentation from lcobucci/jwt talks about this Configuration object for easier use, but the only things it needs to do for the current use case is to provide a parser and a validator.
The parser only has a simple depencency on the JoseEncoder class, the validator is free of other dependencies. So it does not make sense to deal with providing a signer for "none" (explained above that this is scary because it might be insecure), and a Key class with an empty key that are never going to be used for token validation.
| $token = $jwtContainer->parser()->parse($jwt); | ||
| try { | ||
| $token = (new Parser(new JoseEncoder()))->parse($jwt); | ||
| } catch (Exception $e) { |
There was a problem hiding this comment.
I'm catching all possible Lcobucci\JWT\Exception here and convert them into InvalidStateException, following the existing pattern. Parsing a token may throw an exception.
|
|
||
| if (isset($publicKeys[$kid])) { | ||
| $publicKey = openssl_pkey_get_details($publicKeys[$kid]->getKeyMaterial()); | ||
| if (!isset($publicKeys[$kid])) { |
There was a problem hiding this comment.
This if is inverted to allow shortcut throwing the exception instead of dealing with an entire indentation level for the good case.
src/Apple/Provider.php
Outdated
| try { | ||
| $constraints = [ | ||
| new SignedWith(new Sha256, AppleSignerInMemory::plainText($publicKey['key'])), | ||
| new SignedWith(new Sha256, InMemory::plainText($publicKey['key'])), |
There was a problem hiding this comment.
Switching from AppleSignerInMemory to InMemory, which provides checks against empty keys, and has (in more recent versions) also annotations for #[SensitiveParameter] to hide secrets.
|
This is vaguely related to #1354, triggered by the sub-issue lcobucci/jwt#1110 which made me look at this code. Still the question from #1354 remains whether or not it might be possible to configure a leeway when comparing timestamps, as the current implementation expects clocks to be perfectly in sync, especially when consuming the Apple token on a server that is a bit behind the current time and takes offense that the token was created in the future (even if only by half a second), |
|
Thanks a lot for the detailed PR. Lets hope the authors accept the Pull request |
|
Who needs to be bugged from the maintainers to give some feedback whether or not you want to merge this? @atymic maybe? |
|
Could you please rebase and then I will review, sorry for the delay |
Using the Configuration class forces to know a signer and a key, which is irrelevant for the purpose of the provider, but it introduces some code that looks scary if used incorrectly. The irrelevant classes are removed, additional exception cases are now rethrowing as InvalidStateException. Conditions have been inverted to implement early exit.
SvenRtbg
force-pushed
the
improve-lcobucci-jwt-usage
branch
from
1786e98 to
4e252a6
Compare
| ->withHeader('kid', config('services.apple.key_id')) | ||
| ->getToken($this->jwtConfig->signer(), $this->jwtConfig->signingKey()); | ||
|
|
||
| return $token->toString(); |
There was a problem hiding this comment.
Previously the code used a dedicated class that sounded like it would be used for something, but it turned out that the AppleToken was simply a glorified token string creator with no task beyond being destroyed immediately after usage. Drawback of that was that the Provider::URL was required to have public access.
| if (empty($signer) || !class_exists($signer)) { | ||
| $signer = !empty($private_key_path) ? \Lcobucci\JWT\Signer\Ecdsa\Sha256::class : AppleSignerNone::class; | ||
| if (empty($signerClassName) || !class_exists($signerClassName) || !is_a($signerClassName, Signer::class, true)) { | ||
| $signerClassName = EcdsaSha256::class; |
There was a problem hiding this comment.
The AppleSignerNone is gone.
I don't believe there is a use case for a configurable signer class, as the only signature allowed by Apple is ES256 right now, but I added some sanity check beyond "class exists".
|
Rebased as requested. I have no clue if there are test cases that may be broken, haven't seen any so far. |
|
@SvenRtbg can you foresee any breaking changes? Thinking of releasing a beta so people can test first. |
|
@atymic Obviously any usage of the removed classes will lead to failure, but I understood them as being internal with no intentional public exposure. The detailed usage of secret vs. private key change is something I cannot assess at all, but it is already introduced in Initially I was trying to clean up validating an Apple token - creating one for authentication purposes came as an unexpected feature. :) |
3 participants
Using the Configuration class forces to know a signer and a key, which is irrelevant for the purpose of this provider, but it introduces some code that looks scary if used incorrectly.
The irrelevant classes are removed, additional exception cases are now rethrowing as InvalidStateException. Conditions have been inverted to implement early exit.