-
Notifications
You must be signed in to change notification settings - Fork 11k
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
[5.8] Added native support for attribute encryption #29663
Conversation
This PR is created so that models can have native attribute encryption, in the same way that they can be cast. A brief example might be something like the following: ```PHP class User extends Model { $encrypt = [ 'email' ]; } ``` Notice how we now have an array property on the model that can will encrypt and decrypt all attributes that are referenced within that array. In our example, there is no need to create a getter/setter for the attributes that are required to be encrypted and will help reduce clutter of the model, especially if there are multiple columns that require encryption.
I would like to add them through |
@ankurk91 You mean as if to use What would you propose the value of the cast be?
If enough people think that would be a better solution, then I will rewrite this PR accordingly. |
I would go with We should avoid adding more class property to model class. |
I think it should be a separate property. This allows you to combine casting and encryption, .e.g. for an encrypted JSON attribute. |
*/ | ||
public function encryptAttribute($key, $value) | ||
{ | ||
$this->attributes[$key] = is_null($value) ? null : encrypt($value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JustCarty This will break when the illuminate/database package is used outside of a full Laravel framework application.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this the actual problem why previous attempts were not endorsed?
- This would mean that illuminate/database would need a dependency to illuminate/encryption
- you can't use
encrypt()
which callsapp()
which you also can't use here; you would need to directly access the container
Don't forget this package needs to work without Laravel, too.
Maybe a package "on top" would be better suited, say illuminate/database-encryption
which depends on illuminate/database
and illuminate/encryption
and provides some trait or other means to spice up selected models; then illuminate/encryption would only need to provide some hook system to "make" it work, if the current infrastructure isn't enough.
Note: no idea about the feasibility, this was just a quick thought coming to my mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fair to put it on composer suggest and throw an exception saying you should install illuminate/encryption to use this feature. Optional dependencies exist in a lot of places. Migrations has one with doctrine, for instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be merged to Eloquent in any shape or form as this is totally out of scope of what an ORM should be responsible for. Eloquent has (more than) enough responsibilities with the accessor and mutator "magic" as is already IMHO.
If this really needs to be merged though I'd implement it the way @GrahamCampbell suggested, by adding a static property to the Model
class for the encrypter and setting it in the framework booting process and then to check in these methods if it isn't set to do nothing (or throw an exception).
I feel like this needs to go into 6.0, and that the encrypter needs to be set must like the event dispatcher on models, statically at framework boot. |
There's some additional features discussed in laravel/ideas#1244 (Encrypt/decrypt attributes with casts or model property), laravel/ideas#1471 (Allow searching of encrypted data) and laravel/ideas#1531 (Login with encrypted email). I think that adding per-field encryption to Laravel will open up an endless stream of support questions on how to search for encrypted values. It's obvious for us that the encrypted values are lost if the APP_KEY is changed, but do we need support for another encryption key to use for long-term storage? Could we expand on that to have per-user encryption keys? |
My current implementation for this, in my current project, is the following: namespace App;
use App\Traits\Encrypt;
use Illuminate\Foundation\Auth\User as Authenticatable;
class User extends Authenticatable
{
use Encrypt;
$protected $encrypt = [
'email'
];
} With the Trait looking like this: namespace App\Traits;
trait Encrypt
{
public function getAttribute($key)
{
$value = parent::getAttribute($key);
return $this->hasEncryption($key) ? $this->decryptAttribute($value) : $value;
}
public function setAttribute($key, $value)
{
if ($this->hasEncryption($key)) {
$value = $this->encryptAttribute($value);
}
return parent::setAttribute($key, $value);
}
protected function hasEncryption($key)
{
$encrypt = isset($this->encrypt) ? $this->encrypt : [];
return in_array($key, $encrypt);
}
public function encryptAttribute($value)
{
return is_null($value) ? null : encrypt($value);
}
public function decryptAttribute($value)
{
return is_null($value) ? null : decrypt($value);
}
} I did it this way so that I didn't have to modify any vendor code but still have the implementation I want. Is it, therefore, worth creating a new trait within the Encrypter package, for instance, so there are no other dependencies, and then importing a new Trait / Concern? Or have I completely missed the point of everyone's messages... |
It's already fairly easy to hook this up yourself in various ways as demonstrated by the comment above. |
I recommend using something like https://github.com/paragonie/ciphersweet instead of just blindly encrypting. Read https://paragonie.com/blog/2017/05/building-searchable-encrypted-databases-with-php-and-sql to understand why this approach makes sense. AFAIK there's no existing integration with Laravel, but it would be cool if someone made a package! 🔥 Edit: Ah, CipherSweet was already mentioned in one of the issues linked by @sisve. |
We have got a couple of options it seems. Neither look like they are too far through development but worth a look. An official, does not appear to be active: Unofficial, slightly more active but early stages still: |
I have asked this question on Stack Overflow, but wonder what peoples thoughts are here on this: Using database level encryption (RDS) vs. controlling encryption from within Laravel. Is there any need for both? |
This PR is created so that models can have native attribute encryption, in the same way that they can be cast.
A brief example might be something like the following:
Notice how we now have an array property on the model that can will encrypt and decrypt all attributes that are referenced within that array.
In our example, there is no need to create a getter/setter for the attributes that are required to be encrypted and will help reduce clutter of the model, especially if there are multiple columns that require encryption.