tenancy icon indicating copy to clipboard operation
tenancy copied to clipboard

Encrypt tenant user password on tenant data column

Open giagara opened this issue 3 years ago • 14 comments

Description

Using PermissionControlledMySQLDatabaseManager Is it possible to crypt the database password in the data column, inside tenant table?

Why this should be added

If I have choose to separate each database, with single user/password , I need to protect stored credentials.

giagara avatar Dec 02 '21 08:12 giagara

I think you need to move your feature to archtechx/virtualcolumn which is the package that transfers the column into the table column data.

Otherwise you'll need to use Laravel the model event retrieved and saving to encrypt and decrypt your value automatically.

stein-j avatar Dec 02 '21 20:12 stein-j

@giagara Can you explain what benefits this would provide and some example use cases?

There have been several requests for encryption features but I closed a few since they don't really provide value. Encryption is a valid goal in some cases but unnecessary complexity in other cases.

stancl avatar Dec 06 '21 12:12 stancl

@stancl in my opinion exposing sensitive data like that is a lack of security.

I need to separate the tenant's database due a segregation of duty policy I MUST adopt. In case of data breach on the central database (Database level), the unauthorized user can gain access to ALL my tenant's data.

giagara avatar Dec 06 '21 12:12 giagara

lack of security

Yeah but most of these requests are completely ignoring how things actually work. It's one thing to want encryption because in theory encryption makes data safe, and another thing to actually prevent any security issues using encryption.

In case of data breach on the central database (Database level), the unauthorized user can gain access to ALL my tenant's data.

Where will you store the encryption key that will encrypt the password in the data column?

stancl avatar Dec 06 '21 13:12 stancl

Where will you store the encryption key that will encrypt the password in the data column?

Of course in my .env file, that's why I specified Database level as data breach.

I know that if someone gain access to my webserver it will get the ability to see ALL the tenant's database; but as told, I MUST follow this path in order to use your package.

Edit: typos

giagara avatar Dec 06 '21 13:12 giagara

MUST follow this path in order to use your package

That's not a problem with the package though, and if you can't use it that's fine. I'm looking for actual technical merit to implementing these features, not arbitrary corporate policies.

Either way, I'll look into the encryption stuff in v4.

stancl avatar Dec 06 '21 14:12 stancl

not complainig, just I'd like to use it because can save me a lot of work. I hope to see it in V4!

Thanks!

giagara avatar Dec 06 '21 15:12 giagara

not complainig, just I'd like to use it because can save me a lot of work. I hope to see it in V4!

Thanks!

giagara avatar Dec 06 '21 15:12 giagara

MUST follow this path in order to use your package

That's not a problem with the package though, and if you can't use it that's fine. I'm looking for actual technical merit to implementing these features, not arbitrary corporate policies.

Either way, I'll look into the encryption stuff in v4.

This is an old issue and while I don't need the feature discussed, the one use case that I could see for encrypting the password is if it is encrypted using the APP_KEY. In the scenario that an attacker can dump just your central tenants database then they won't be able to access the tenants due to the password being encrypted and they'd need another attack vector to get the APP_KEY. This obviously is a very specific situation as they'd also need to have access to connect to your database to utilize such passwords if they got them.

fdabek1 avatar Mar 11 '22 06:03 fdabek1

Hello all!

I recently had to solve this same problem and think I've managed to do it in a way that was easier than I'd expect it to be.

When resolving the tenant's database configuration, this package is using the values in the data column stored as tenancy_db_username and tenancy_db_password to create the values used in the db config as username and password, respectively. So the Tenancy package is looking in the Virtual Column for tenancy_db_username and tenancy_db_password.

Fortunately, these packages also provide you with a place in the migrations for your own custom columns and a method to tell the model that it should be looking for the data in an actual database column instead of the virtual column.

This means, we can do this:

        Schema::create('tenants', function (Blueprint $table) {
            $table->string('id')->primary();

-           // your custom columns may go here
+            $table->string('tenancy_db_username');
+            $table->string('tenancy_db_password');

to create the columns in the database. Then let the model know about them:

namespace App\Tenant;

// imports

class Tenant extends BaseTenant implements TenantWithDatabase
{
    use HasDatabase, HasDomains;

    /**
     * Define custom columns for this model (that shouldn't be accessed via 'data' property).
     *
     * @return array
     */
    public static function getCustomColumns(): array
    {
        return [
            'id',
            'tenancy_db_username',
            'tenancy_db_password',
        ];
    }

And combine this with Laravel encrypted casts (or, better yet, a custom cast) to encrypt and decrypt these when needed:

    /**
     * The attributes that sould be cast.
     *
     * @var string[]
     */
    protected $casts = [
        'tenancy_db_username' => 'encrypted',
        'tenancy_db_password' => 'encrypted',
    ];

Since the package is looking for these keys when establishing db config, this just works.

Hope this you @giagara, or anyone else who finds this Issue.

rickyjohnston avatar Mar 21 '22 12:03 rickyjohnston

Hey @rickyjohnston thanks a lot. I managed this by overriding public function getAttribute($key) and public function setAttribute($key, $value) in App\Models\Tenant by adding an if statement and then encrypt/decrypt.

I like your solution, and I will test in my project.

Thanks!

giagara avatar Mar 21 '22 15:03 giagara

Hey @rickyjohnston thanks a lot. I managed this by overriding public function getAttribute($key) and public function setAttribute($key, $value) in App\Models\Tenant by adding an if statement and then encrypt/decrypt.

I like your solution, and I will test in my project.

Thanks!

The encrypted casts mentioned by @rickyjohnston is a much more Laravel way that is elegant and shorter code, definitely recommend that if going this route.

And @rickyjohnston the encrypted casts is a good solution for the problem.

fdabek1 avatar Mar 21 '22 17:03 fdabek1

@stancl @rickyjohnston

255 characters (default for string method in migrations) is not enough to store the crypted random data generated. Should be increased like:

$table->string('tenancy_db_username', 500);
$table->string('tenancy_db_password', 500);

Thanks

giagara avatar Mar 24 '22 10:03 giagara

@abrardev99 Please PR the instructions from this comment https://github.com/archtechx/tenancy/issues/760#issuecomment-1073844562 to the docs. Also include this suggestion https://github.com/archtechx/tenancy/issues/760#issuecomment-1077483999 and make the strings longer. Probably go with 512 since that's a multiple of 8.

stancl avatar Jun 23 '22 18:06 stancl