[9.x] Bind the encryption key object into the container #38793

X-Coder264 · 2021-09-13T15:40:23Z

The \Illuminate\Cookie\Middleware\EncryptCookies middleware calls the getKey method which is not on the encrypter contract. This leads to broken applications when the users of the framework create their own encrypter implementation.

This PR fixes this issue by making the key a value object that is bound into the container. By doing so anybody can typehint the Key class in their constructors and get the key that way now so there's no need to break the encrypter encapsulation with the getKey method.

A side bonus of doing this is that people no longer need to copy paste the logic from \Illuminate\Encryption\EncryptionServiceProvider::parseKey every time when they need to construct just the key.

There are only three very minor breaking changes here that won't affect 99% of the applications out there:

The first parameter in the encrypter constructor now takes a Key instance instead of a string
The getKey method on the encrypter has been removed. If the user needs the key they can just typehint it directly in the constructor instead of typehinting the Encrypter just to get the key.
The \Illuminate\Cookie\Middleware\EncryptCookies middleware constructor now gets the Key instance as the second parameter so if anyone overrode the constructor of that class in their app they will have to adjust it (but usually the only thing that changes in that class is the $except array).

Fixes #38772

driesvints · 2021-09-13T15:44:39Z

I still think this is a bit overkill and unnecessary to introduce these breaking changes. I personally think it'd be easier to convert the CookieValuePrefix to a non-static class that's bound to the container with the app key as a dependency for now and then wait until the Symfony PR gets merged so we can leverage those classes and/or interfaces.

X-Coder264 · 2021-09-13T15:46:36Z

Doing that change in just the CookieValuePrefix class wouldn't be enough. Passport is for example breaking the encrypter contract in a few places too:

https://github.com/laravel/passport/search?q=getKey

The Passport example shows that EncryptCookies isn't the only place which has this problem (which means that the problem cannot be fixed by just changing CookieValuePrefix as that wouldn't solve the violations in Passport) and that the key is sometimes needed (even in first party libraries) without needing the encrypter as well which means that the framework should provide a way of getting that key without needing an encrypter instance (and violating its contract in the process) which is exactly what this PR does.

- avoid calling getKey method on the encrypter as it is not part of the contract

inxilpro · 2021-09-14T14:55:22Z

Couldn't we introduce the Key object but add a __toString implementation to it and leave the Encrypter mostly as-is? That way the Key can be used where needed, but the Encrypter continues to hold a string representation of it?

inxilpro

This would be the __toString() approach…

inxilpro · 2021-09-14T14:56:16Z

src/Illuminate/Encryption/Encrypter.php

@@ -39,15 +39,15 @@ class Encrypter implements EncrypterContract, StringEncrypter
    /**
     * Create a new encrypter instance.
     *
-     * @param  string  $key
+     * @param  \Illuminate\Encryption\Key  $key


Suggested change

* @param \Illuminate\Encryption\Key $key

* @param \Illuminate\Encryption\Key|string $key

inxilpro · 2021-09-14T14:56:29Z

src/Illuminate/Encryption/Encrypter.php

     * @param  string  $cipher
     * @return void
     *
     * @throws \RuntimeException
     */
-    public function __construct($key, $cipher = 'aes-128-cbc')
+    public function __construct(Key $key, string $cipher = 'aes-128-cbc')


Suggested change

public function __construct(Key $key, string $cipher = 'aes-128-cbc')

public function __construct($key, string $cipher = 'aes-128-cbc')

inxilpro · 2021-09-14T14:57:05Z

src/Illuminate/Encryption/Encrypter.php

-
-    /**
-     * Get the encryption key.
-     *
-     * @return string
-     */
-    public function getKey()
-    {
-        return $this->key;
-    }


We could leave getKey() for backwards-compatibility reasons.

The whole point of this PR is that this violates the contact so that's why it was removed. It's better to have it removed so that the callers have to fix their contract violations than leaving it in there and by doing so leaving a chance that the packages out there still violate the contract which makes this entire PR useless then.

inxilpro · 2021-09-14T14:59:29Z

src/Illuminate/Encryption/Key.php

+     *
+     * @return string
+     */
+    public function getValue(): string


Suggested change

public function getValue(): string

public function __toString(): string

inxilpro · 2021-09-14T15:00:10Z

src/Illuminate/Cookie/Middleware/EncryptCookies.php

@@ -103,7 +113,7 @@ protected function validateValue(string $key, $value)
    {
        return is_array($value)
                    ? $this->validateArray($key, $value)
-                    : CookieValuePrefix::validate($key, $value, $this->encrypter->getKey());
+                    : CookieValuePrefix::validate($key, $value, $this->key->getValue());


Suggested change

: CookieValuePrefix::validate($key, $value, $this->key->getValue());

: CookieValuePrefix::validate($key, $value, $this->key);

inxilpro · 2021-09-14T15:01:29Z

src/Illuminate/Foundation/Testing/Concerns/MakesHttpRequests.php

@@ -603,7 +604,7 @@ protected function prepareCookiesForRequest()
        }

        return collect($this->defaultCookies)->map(function ($value, $key) {
-            return encrypt(CookieValuePrefix::create($key, app('encrypter')->getKey()).$value, false);
+            return encrypt(CookieValuePrefix::create($key, app(Key::class)->getValue()).$value, false);


Suggested change

return encrypt(CookieValuePrefix::create($key, app(Key::class)->getValue()).$value, false);

return encrypt(CookieValuePrefix::create($key, app(Key::class)).$value, false);

inxilpro · 2021-09-14T15:02:03Z

tests/Cookie/Middleware/EncryptCookiesTest.php

@@ -90,7 +94,7 @@ protected function getEncryptedCookieValue($key, $value)
        $encrypter = $this->container->make(EncrypterContract::class);

        return $encrypter->encrypt(
-            CookieValuePrefix::create($key, $encrypter->getKey()).$value,
+            CookieValuePrefix::create($key, $this->container->make(Key::class)->getValue()).$value,


Suggested change

CookieValuePrefix::create($key, $this->container->make(Key::class)->getValue()).$value,

CookieValuePrefix::create($key, $this->container->make(Key::class)).$value,

inxilpro · 2021-09-14T15:02:23Z

tests/Encryption/EncrypterTest.php

@@ -4,38 +4,39 @@

 use Illuminate\Contracts\Encryption\DecryptException;
 use Illuminate\Encryption\Encrypter;
+use Illuminate\Encryption\Key;
 use PHPUnit\Framework\TestCase;
 use RuntimeException;

 class EncrypterTest extends TestCase


All the changes to this test could be reverted.

inxilpro · 2021-09-14T15:03:20Z

tests/Testing/TestResponseTest.php

        $container->singleton('encrypter', function () use ($encrypter) {
            return $encrypter;
        });

        $cookieName = 'cookie-name';
        $cookieValue = 'cookie-value';
-        $encryptedValue = $encrypter->encrypt(CookieValuePrefix::create($cookieName, $encrypter->getKey()).$cookieValue, false);
+        $encryptedValue = $encrypter->encrypt(CookieValuePrefix::create($cookieName, $key->getValue()).$cookieValue, false);


Suggested change

$encryptedValue = $encrypter->encrypt(CookieValuePrefix::create($cookieName, $key->getValue()).$cookieValue, false);

$encryptedValue = $encrypter->encrypt(CookieValuePrefix::create($cookieName, $key).$cookieValue, false);

inxilpro · 2021-09-14T15:03:50Z

tests/Testing/TestResponseTest.php

        $container->singleton('encrypter', function () use ($encrypter) {
            return $encrypter;
        });

        $cookieName = 'cookie-name';
        $cookieValue = 'cookie-value';
        $encryptedValue = $encrypter->encrypt(
-            CookieValuePrefix::create($cookieName, $encrypter->getKey()).$cookieValue, false
+            CookieValuePrefix::create($cookieName, $key->getValue()).$cookieValue, false


Suggested change

CookieValuePrefix::create($cookieName, $key->getValue()).$cookieValue, false

CookieValuePrefix::create($cookieName, $key).$cookieValue, false

X-Coder264 · 2021-09-14T15:06:37Z

I could do that, but since changing new Encrypter('foo'); to new Encrypter(new Key('foo')); is a very quick change to do I'm not sure if we really need to add __toString to the Key class as it makes sense for the encrypter from a type perspective to use the Key class and not just any random string. I'm open to doing this change if the maintainers want it though.

inxilpro · 2021-09-14T15:09:03Z

changing new Encrypter('foo'); to new Encrypter(new Key('foo')); is a very quick change

But letting the Encrypter continue to use a string value means that all the existing code that expects it to be a string wouldn't be affected. The __toString() approach allows new code to use the Key value object without causing much in the way of issues for existing code…

taylorotwell · 2021-09-20T15:37:14Z

Honestly think the simplest thing is just adding getKey to the contract in 9.x. Who really cares? 😅

X-Coder264 · 2021-09-20T17:07:42Z

IMO the encrypter contract should be about encrypting data, not about fetching the key. From what I see, @driesvints agrees with that too -> #38772 (comment)

And from the user perspective it's a bit weird to need to get the encrypter just to get the key.

Since both approaches have a similar BC impact we may as well go with the current approach, but that's just my 2 cents.

taylorotwell · 2021-09-20T18:04:23Z

But, here's the thing. 99.9% of Laravel users will never implement the encrypter contract or ever care at all that it has a getKey method. So, instead of adding one line of code to a contract that will literally affect maybe 1 in 10,000 Laravel users - if that - we are instead of deciding to edit 10 files, change signatures, add new classes, etc.?

X-Coder264 · 2021-09-20T18:41:54Z

I think we are approaching the same problem in different ways. I'm thinking from a software design perspective (how would I like the encrypter contract to look like), while you have a pragmatic maintainer/user view of the problem which takes into consideration the encrypter contract as it exists currently. Both POVs are perfectly valid. The entire diff consist of 100 added and 50 deleted lines of code and a lot of it are test changes which were done in a find & replace fashion (and which can be reverted by applying @inxilpro suggestion) so I think the diff in general is quite small. However if you wish so I can revert everything and just add that method to the contract.

taylorotwell · 2021-09-20T19:50:35Z

Yeah, honestly I think we should just add the getKey method to the contract for 9.x. I agree that if you were writing a framework from complete scratch you may not do it but it's just such a minor thing and the encrypter contract is not going to be reimplemented by third-parties very often namely because it's possibly a serious security risk to try to roll your own encryption implementation.

I'll send up a commit for the contract.

X-Coder264 force-pushed the encrypter-improvements branch from 55fd8b2 to 3f363c4 Compare September 13, 2021 15:44

X-Coder264 force-pushed the encrypter-improvements branch 2 times, most recently from 2b6a88b to 084dd6d Compare September 13, 2021 15:52

Bind the encryption key object into the container

c8d5a90

- avoid calling getKey method on the encrypter as it is not part of the contract

X-Coder264 force-pushed the encrypter-improvements branch from 084dd6d to c8d5a90 Compare September 13, 2021 15:55

inxilpro reviewed Sep 14, 2021

View reviewed changes

taylorotwell closed this Sep 20, 2021

X-Coder264 deleted the encrypter-improvements branch September 20, 2021 19:59

smknstd mentioned this pull request Sep 27, 2021

Encrypter contract has no getKey() method #38772

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[9.x] Bind the encryption key object into the container #38793

[9.x] Bind the encryption key object into the container #38793

X-Coder264 commented Sep 13, 2021

driesvints commented Sep 13, 2021

X-Coder264 commented Sep 13, 2021 •

edited

Loading

inxilpro commented Sep 14, 2021

inxilpro left a comment

inxilpro Sep 14, 2021

inxilpro Sep 14, 2021

inxilpro Sep 14, 2021

X-Coder264 Sep 14, 2021

inxilpro Sep 14, 2021

inxilpro Sep 14, 2021

inxilpro Sep 14, 2021

inxilpro Sep 14, 2021

inxilpro Sep 14, 2021

inxilpro Sep 14, 2021

inxilpro Sep 14, 2021

X-Coder264 commented Sep 14, 2021 •

edited

Loading

inxilpro commented Sep 14, 2021

taylorotwell commented Sep 20, 2021

X-Coder264 commented Sep 20, 2021

taylorotwell commented Sep 20, 2021 •

edited

Loading

X-Coder264 commented Sep 20, 2021 •

edited

Loading

taylorotwell commented Sep 20, 2021

	* @param \Illuminate\Encryption\Key $key
	* @param \Illuminate\Encryption\Key\|string $key

	public function __construct(Key $key, string $cipher = 'aes-128-cbc')
	public function __construct($key, string $cipher = 'aes-128-cbc')

	public function getValue(): string
	public function __toString(): string

	: CookieValuePrefix::validate($key, $value, $this->key->getValue());
	: CookieValuePrefix::validate($key, $value, $this->key);

	return encrypt(CookieValuePrefix::create($key, app(Key::class)->getValue()).$value, false);
	return encrypt(CookieValuePrefix::create($key, app(Key::class)).$value, false);

	CookieValuePrefix::create($key, $this->container->make(Key::class)->getValue()).$value,
	CookieValuePrefix::create($key, $this->container->make(Key::class)).$value,

	$encryptedValue = $encrypter->encrypt(CookieValuePrefix::create($cookieName, $key->getValue()).$cookieValue, false);
	$encryptedValue = $encrypter->encrypt(CookieValuePrefix::create($cookieName, $key).$cookieValue, false);

	CookieValuePrefix::create($cookieName, $key->getValue()).$cookieValue, false
	CookieValuePrefix::create($cookieName, $key).$cookieValue, false

[9.x] Bind the encryption key object into the container #38793

[9.x] Bind the encryption key object into the container #38793

Conversation

X-Coder264 commented Sep 13, 2021

driesvints commented Sep 13, 2021

X-Coder264 commented Sep 13, 2021 • edited Loading

inxilpro commented Sep 14, 2021

inxilpro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

X-Coder264 commented Sep 14, 2021 • edited Loading

inxilpro commented Sep 14, 2021

taylorotwell commented Sep 20, 2021

X-Coder264 commented Sep 20, 2021

taylorotwell commented Sep 20, 2021 • edited Loading

X-Coder264 commented Sep 20, 2021 • edited Loading

taylorotwell commented Sep 20, 2021

X-Coder264 commented Sep 13, 2021 •

edited

Loading

X-Coder264 commented Sep 14, 2021 •

edited

Loading

taylorotwell commented Sep 20, 2021 •

edited

Loading

X-Coder264 commented Sep 20, 2021 •

edited

Loading