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

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

Open
Slamdunk opened this issue Nov 7, 2024 · 4 comments
Open

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

Slamdunk opened this issue Nov 7, 2024 · 4 comments

Comments

@Slamdunk
Copy link
Collaborator

Slamdunk commented Nov 7, 2024

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 ( 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
@Slamdunk Slamdunk added this to the 6.0.0 milestone Nov 7, 2024
@Ocramius
Copy link
Collaborator

Ocramius commented Nov 7, 2024

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

@lcobucci
Copy link
Owner

lcobucci commented Nov 7, 2024

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
Copy link
Collaborator Author

Slamdunk commented Nov 8, 2024

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 🤔

@Ocramius
Copy link
Collaborator

Ocramius commented Nov 8, 2024

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:

  • JWK (if someone maintains that)
  • New signature algorithm (HS? Can't remember)
  • Immutable/pure interfaces

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants