Add data protection to DatabaseJsonWebKeyStore and FileSystemStore

[sherlock1982]

Recently switch from DataProtectionStore to DatabaseJsonWebKeyStore and noticed that no DataProtection is present. It looks to me that mentioned stores are generally less secure than default one.

Note that for example MsalDistributedTokenCacheAdapterOptions has an option to Encrypt (default false):

        services.Configure<MsalDistributedTokenCacheAdapterOptions>(options =>
        {
            // Just for extra security here
            options.Encrypt = true;
        });

I added protection to DatabaseJsonWebKeyStore like this:

            keyModel.Property(key => key.Parameters).HasColumnName("parameters").HasConversion(
                val => Protect(val), dbVal => Unprotect(dbVal)
            );

With:

    string Protect(string val)
    {
        return dataProtector.Protect(val);
    }

    string Unprotect(string dbVal)
    {
        try
        {
            return dataProtector.Unprotect(dbVal);
        }
        catch
        {
            // Something bad but also maybe unprotected payload
            return dbVal;
        }
    }

But I think would be nice to have it in the stores out of the box.

The other option will be to add protection at a higher level for KeyMaterial but that won't work good for some scenarios. For example I'd like to store a public key separately so I can access it from other services but keep private key only to the specific service.

[brunobritodev]

I agree with you and have some additional thoughts on this matter.

This component was created to handle JWT in high availability scenarios. One of the reasons for not encrypting data before persisting it in other stores is that we cannot guarantee developers will configure DataProtection properly. In high availability scenarios, where multiple instances may exist, some of them might be unable to decrypt the content, leading to exceptions in the component.

However, the DefaultStore (added in more recent versions) uses DataProtection.Repositories, which minimizes this risk. This ensures that in high availability scenarios, DataProtection must be configured, thus maintaining data integrity across instances.

Although I have explained the reasons, I no longer hold a strong opinion on this matter. What are your thoughts on this?

[sherlock1982]

Well first of all thanks for the lib! I was playing with it a lot and found it very nice.

In my final approach I decided to change implementation of JwtService and KeyMaterial. I added extra column to KeyMaterial called PublicKey and store there public key unencrypted. While parameters go encrypted. Revoke simply cleans up Parameters column.

This solved the following things for me:

1. Another service accesing the database who wants to check token signature can do it quite easily. I have some services that in the future should only check signatures but not issue tokens.
2. If DB is somehow stolen or select SQL injection executed private key is not exposed.
3. If I want to make a backup of keys to restore them on a different machine I can easily do it dropping private keys and simply restoring tokens for signature verify only. Therefore even if backup is stolen they can't generate new tokens.
4. I had to change implementation to the one I mentioned in #66 and simply access PublicKey for verification. It means i don't really need GetKeys() function basically and I can access certain key already knowing it's kid
5. If I have an issue with DataProtector. For example key is lost it's ok - I just generate a new key but still can have old keys for verification

I also noticed that I can't really make a key invalid so a new key will be generated if I can't Unprotect the key. This is why I also adjusted it and moved data protection to JwtService layer.

And yes I added a semaphore for GetCurrentCredentials function because it will generate multiple keys at the same time.

And one more thing - you remember the issue with casing in the key. The problem here is the lib doesn't behave well if for some reason such issue happens. Becuase it simply throws the exception from GetSecurityKey. You can of course catch it and generate a new key but in this case GetLastKeys() will throw an exception in the token handler. So another reason for me to change implementation here.

We can close this discussion - just wanted to share my findings on this :-)