lcobucci / jwt

A simple library to work with JSON Web Token and JSON Web Signature
https://lcobucci-jwt.readthedocs.io/en/stable/
BSD 3-Clause "New" or "Revised" License
7.29k stars 600 forks source link

Retrieve `Key` content the very last moment it's needed #1084

Open Slamdunk opened 6 days ago

Slamdunk commented 6 days ago

Currently the Signer\OpenSSL pulls the Key content way before its actual usage, and this is a problem because we need to carry around the #[SensitiveParameter] attribute, potentially forgetting it.

I suggest to query the Key only when the low-level functions are actually called. It's too early for a PR (https://github.com/lcobucci/jwt/labels/BC-break involved), so I'll leave the diff here for now:

diff --git a/src/Signer/Ecdsa.php b/src/Signer/Ecdsa.php
index d6f8e1f..01442bd 100644
--- a/src/Signer/Ecdsa.php
+++ b/src/Signer/Ecdsa.php
@@ -18,7 +18,7 @@ abstract class Ecdsa extends OpenSSL
     final public function sign(string $payload, Key $key): string
     {
         return $this->converter->fromAsn1(
-            $this->createSignature($key->contents(), $key->passphrase(), $payload),
+            $this->createSignature($key, $payload),
             $this->pointLength(),
         );
     }
@@ -28,7 +28,7 @@ abstract class Ecdsa extends OpenSSL
         return $this->verifySignature(
             $this->converter->toAsn1($expected, $this->pointLength()),
             $payload,
-            $key->contents(),
+            $key,
         );
     }

diff --git a/src/Signer/OpenSSL.php b/src/Signer/OpenSSL.php
index a507752..b3ae7d4 100644
--- a/src/Signer/OpenSSL.php
+++ b/src/Signer/OpenSSL.php
@@ -41,17 +41,14 @@ abstract class OpenSSL implements Signer
      * @throws InvalidKeyProvided
      */
     final protected function createSignature(
-        #[SensitiveParameter]
-        string $pem,
-        #[SensitiveParameter]
-        string $passphrase,
+        Key $key,
         string $payload,
     ): string {
-        $key = $this->getPrivateKey($pem, $passphrase);
+        $opensslKey = $this->getPrivateKey($key);

         $signature = '';

-        if (! openssl_sign($payload, $signature, $key, $this->algorithm())) {
+        if (! openssl_sign($payload, $signature, $opensslKey, $this->algorithm())) {
             throw CannotSignPayload::errorHappened($this->fullOpenSSLErrorString());
         }

@@ -60,30 +57,27 @@ abstract class OpenSSL implements Signer

     /** @throws CannotSignPayload */
     private function getPrivateKey(
-        #[SensitiveParameter]
-        string $pem,
-        #[SensitiveParameter]
-        string $passphrase,
+        Key $key,
     ): OpenSSLAsymmetricKey {
-        return $this->validateKey(openssl_pkey_get_private($pem, $passphrase));
+        return $this->validateKey(openssl_pkey_get_private($key->contents(), $key->passphrase()));
     }

     /** @throws InvalidKeyProvided */
     final protected function verifySignature(
         string $expected,
         string $payload,
-        string $pem,
+        Key $key,
     ): bool {
-        $key    = $this->getPublicKey($pem);
-        $result = openssl_verify($payload, $expected, $key, $this->algorithm());
+        $opensslKey    = $this->getPublicKey($key);
+        $result = openssl_verify($payload, $expected, $opensslKey, $this->algorithm());

         return $result === 1;
     }

     /** @throws InvalidKeyProvided */
-    private function getPublicKey(string $pem): OpenSSLAsymmetricKey
+    private function getPublicKey(Key $key): OpenSSLAsymmetricKey
     {
-        return $this->validateKey(openssl_pkey_get_public($pem));
+        return $this->validateKey(openssl_pkey_get_public($key->contents()));
     }

     /**
diff --git a/src/Signer/Rsa.php b/src/Signer/Rsa.php
index ba7d72d..53d10f3 100644
--- a/src/Signer/Rsa.php
+++ b/src/Signer/Rsa.php
@@ -11,12 +11,12 @@ abstract class Rsa extends OpenSSL

     final public function sign(string $payload, Key $key): string
     {
-        return $this->createSignature($key->contents(), $key->passphrase(), $payload);
+        return $this->createSignature($key, $payload);
     }

     final public function verify(string $expected, string $payload, Key $key): bool
     {
-        return $this->verifySignature($expected, $payload, $key->contents());
+        return $this->verifySignature($expected, $payload, $key);
     }

     final protected function guardAgainstIncompatibleKey(int $type, int $lengthInBits): void
Ocramius commented 5 days ago

Agree on passing around Key instances until the very last moment: this also defers any loading even further :+1:

lcobucci commented 5 days ago

I agree with the proposal.

It's also quite fine to prepare for v6, we don't need to wait too long for it - maybe even slap some more @internal on those abstract classes.

Slamdunk commented 5 days ago

Honest question: do you think a v6 will ever be released? I mean, v5 seems pretty complete on the features and API side, issues like this one aren't worth a new release :thinking:

Ocramius commented 5 days ago

It's fine to bump major release: if packages aren't affected by any of our BC changes, they can widen the dependency ranges.

Things that we still miss have been mentioned recently: