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

Encrypter contract has no getKey() method #38772

Closed
smknstd opened this issue Sep 12, 2021 · 9 comments
Closed

Encrypter contract has no getKey() method #38772

smknstd opened this issue Sep 12, 2021 · 9 comments

Comments

@smknstd
Copy link

smknstd commented Sep 12, 2021

  • Laravel Version: 8.x
  • PHP Version: 8.0
  • Database Driver & Version: mysql 8.0

Description:

As reported by @denis-chmel here, contract \Illuminate\Contracts\Encryption\Encrypter has no public getKey() method declared and it probably should as EncryptCookies uses it since this fix.

https://github.com/laravel/framework/blob/8.x/src/Illuminate/Cookie/Middleware/EncryptCookies.php#L86
https://github.com/laravel/framework/blob/8.x/src/Illuminate/Cookie/Middleware/EncryptCookies.php#L148

What do you think ?

@driesvints
Copy link
Member

Yeah, this is an issue. I'm not sure how to resolve this properly as you don't really want the getKey method on the encrypter contract. It should be kept simple. Maybe a refactor of the CookieValuePrefix class to contain the app key would be a good idea for Laravel 9.

@X-Coder264
Copy link
Contributor

X-Coder264 commented Sep 13, 2021

Why not just inject the key value to the EncryptCookies middleware via the constructor (the same way the encrypter gets its key) ? That seems to be the cleanest approach (especially since the CookieValuePrefix class has only static methods).

@driesvints
Copy link
Member

@X-Coder264 because I believe in an ideal scenario, the key is encapsulated in an framework class rather than provided directly to the Middleware.

@X-Coder264
Copy link
Contributor

It's provided directly as a string to the encrypter too. It'd be nice to have it as a class there too then.

@driesvints
Copy link
Member

@X-Coder264 no, it's already wrapped in a class there. The encrypter itself.

@X-Coder264
Copy link
Contributor

I don't know what's your definition of "wrapped" but it's different than mine. My idea of wrapped would be to create a Key class which could be typehinted in any constructor (including the Encrypter) which would be resolved out of the container. That way the Key could be used anywhere where it's needed in a Laravel user project without having to copy paste the logic from \Illuminate\Encryption\EncryptionServiceProvider::parseKey each time when we need the key. The Symfony encrypter component which is currently WIP has a similar Key class approach (symfony/symfony#39344) too.

@driesvints
Copy link
Member

Yeah maybe. Could be a good idea indeed.

@smknstd
Copy link
Author

smknstd commented Sep 27, 2021

#38793 (comment) I think it is worth mentioning that adding getKey() method to contract would make it hard to fit with "envelope encryption" like kms as you don't have access to an encryption key. Cookie encryption is probably not a good target for "envelope encryption", but when using the encrypter separately or with an eloquent cast for sensitive data, I think it makes sense.

@driesvints
Copy link
Member

@smknstd I think you can just return an empty string in that case.

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