Improve attr encryption key rollover

389-ds-bot commented 4 years ago

Cloned from Pagure issue: https://pagure.io/389-ds-base/issue/49525

Created at 2018-01-08 02:01:57 by firstyear (@Firstyear)
Assigned to firstyear (@Firstyear)

Issue Description

The current design of attribute encryption has a reasonably complex key rollover process - the key rollover process is related to changes of the Server-Cert.

Right now we generate a key from the NSSDB with pk11_TokenKeyGenWithFlags. We'll call this the "secret value". We take this "secret value" and then encrypt that with our Server-Cert - the result is the "encrypted secret value".

In the server when we replace the Server-Cert, we no longer can decrypt the "encrypted secret value". This leads to the counter intuitive advice of "delete the encrypted secret value" on certificate change. Once we delete this, the server starts and calls pk11_TokenKeyGenWithFlags again, returning the "secret value" (the same original value) and we then encrypt that again.

What is important to note is that the "secret value" is stored twice here: once in the NSSDB, encrypted with the nss pin.txt. And again with our certificate with "encrypted secret value".

Because this leads to pointless admin overhead for attribute encryption - we have a value that needs deleting counter intutivalely on key rollover, when NSS gives us a way to access it always provided the pin is available.

As a result we should remove our caching of "encrypted secret value", and rely on NSS to do it's job - to store and give us the "secret value".

This means in our documentation

We should discuss the NSSDB files are CRITICAL to the attr encryption system
We should remove the key rollover section once this is implemented.

For us, we should just read nssdb's "secret value" on start up rather than trying to encrypt / decrypt ourselves.

This would be a huge usability gain for attr encryption, making it much much easier to deploy and use, with simple instructions.

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2018-01-08 02:02:15

For reference:

Here's a sample using the calls you are currently using: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/nss_sample_code/NSS_Sample_Code_sample6

Using sdr: https://dxr.mozilla.org/security/source/mozilla/security/nss/cmd/sdrtest

The functions are defined here: https://dxr.mozilla.org/security/source/mozilla/security/nss/lib/pk11wrap/pk11sdr.h

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2018-01-08 02:02:16

Metadata Update from @Firstyear:

Custom field component adjusted to None
Custom field origin adjusted to None
Custom field reviewstatus adjusted to None
Custom field type adjusted to None
Custom field version adjusted to None

389-ds-bot commented 4 years ago

Comment from nhosoi (@nhosoi) at 2018-01-08 23:52:23

Sounds like a good idea. One RFE if I could ask for would be including NSSDB in the DS backup (if not yet -- sorry, I don't recall... :).

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2018-01-09 00:30:55

At the moment db2bak only does the bdb db, but I planned to add nssdb to the dsctl backup tool I'm going to add :)

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2018-01-23 17:34:42

Metadata Update from @mreynolds389:

Issue set to the milestone: 1.4 backlog

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2018-01-24 13:54:57

are you sure that always the same symmetric key is generated ? would this mean that the keys generated and used for attribute encryption and changelog encryption would be identical ? In the procedure described for changelog encryption and cert renewal it says that the changelog needs to be exported in clear and after renewal reimported - so this would be wrong.

do we have any test cases for attr and/or changelog encryption and cert renewal ?

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2018-01-29 02:43:30

@elkris It's not generated, it's stored in the nssdb and retrieved on lookup of a certain "name". This means that given the same "name", both will use the same "static key" aka token.

What this means is that the ENTIRE process we have for using our certs to protect the nssdb token output we have requested is pointless, and we can remove all of that code - our security is solely tied to the pin.txt / password on the nssdb, and we can retrieve the token by name. This hugely improves the usability because we can completely remove the requirement for the complex key rollover process.

This would obliviate the need for cert renewal test cases because they "go away". For older installs we would still required them. Today we have no test cases for this space that I am aware of, and that would absolutely be step one in the process of resolving this ticket to create these test cases to ensure that it works 100% on upgrade, import, export and others.

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2018-01-29 09:20:01

Thanks for the explanation. this makes sense and I am all for this change.

I only need to investigate if this really applies to changelog encryption as we have the recommendation there to export, change cert, reimport - which would indicate that a new key is used, and what should be avoided

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2018-01-30 00:29:05

@elkris I will investigate as part of this change if that makes it easier. :)

389-ds-bot commented 4 years ago

Comment from tbordaz (@tbordaz) at 2019-02-27 18:32:53

I opened #3309 that is likely a duplicate of that ticket. Using the nssdb as storage to the symmetric key rather than in the config (attribute/changelog entry) looks a very good idea. To protect the symmetric key we currently rely on private key (from Server-Cert) or from the nssdb pin password. If someone has access to the nssdb file, @Firstyear do you know if private key encryption (current behavior) would be "stronger" than breaking nssdb ?

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2019-02-28 01:16:55

@tbordaz They are cryptographically equivalent. NSSDB uses aes128 if I recall, and we use similar in the storage that encrypts using the RSA key.

Remember, we are only protecting the symmetric key in dse.ldif as a cached copy of the value that is in the NSSDB as well. So there are technically two possible attack surfaces, and it all relies on the NSSDB protection. Additionally, even if we were storing this value only in dse.ldif, the security of the private key still depends on the security of the NSSDB to access it, so they are equivalent again.

A part of this that is worth discussing is the threats and what the encryption protects from. In our case, the encyrpted database does NOT protect against offline attacks (since pin.txt is often present). It protects against attribute value disclosure (encrypted attrs can only be sent via SSF > 1 channels), attacks against the backups of the system (which may/may not have pin.txt), and code-execution attacks trying to disclose data (but only through obscurity of the required steps to decrypt).

So in these cases, we need to consider the protection of the NSSDB and pin.txt as well, and asses how that relates to the protections attribute encryption is providing. IMO there is no change in the threats and security profile regardless of if we simplify this system, since all of these still hinge on the pin.txt and nssdb encryption.

My summary is the strength of the system, and the threats it protects against is the same in both situations - and infact may be improved by having a simple administration process allowing people to feel more confident to use the attribute encryption mechanism.

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2020-02-25 10:05:27

I spent some time on this to try to get it work and understand what's going on.

The sample code in #2584#comment-487006

worked fine, but in a 389 install I failed to find the key in the nss database. Trying symkeyutil from nss did work on the sample code but didn't show anything for a 389 instance.

Finally I realized the sample code uses:

 key = PK11_TokenKeyGen(slot, cipherMech, 0, 32 /*keysize*/,
                     &keyiditem, PR_TRUE, 0);

which creates a persistent key, but in 389 we are using

   new_symmetric_key = slapd_pk11_TokenKeyGenWithFlags(....)

which does by default NOT create a persistent key. The following patch made it work:

 diff --git a/ldap/servers/slapd/back-ldbm/ldbm_attrcrypt.c b/ldap/servers/slapd/back-ldbm/ldbm_attrcrypt.c
 index b42ec9e3d..6b881e58a 100644
 --- a/ldap/servers/slapd/back-ldbm/ldbm_attrcrypt.c
 +++ b/ldap/servers/slapd/back-ldbm/ldbm_attrcrypt.c
 @@ -317,7 +317,7 @@ attrcrypt_generate_key(attrcrypt_cipher_state *acs, PK11SymKey **symmetric_key)
                                                          acs->ace->key_size,
                                                          NULL /*keyid*/,
                                                          CKF_DECRYPT /*op*/,
 -                                                        CKF_ENCRYPT /*attr*/,
 +                                                        CKF_ENCRYPT | PK11_ATTR_TOKEN | PK11_ATTR_PRIVATE /*attr*/,
                                                          NULL);
      if (new_symmetric_key) {
          *symmetric_key = new_symmetric_key;

In conclusion:

we can generate persistent keys and use them directly from the nss database and get rid of all certificate renewal worries
for existing deployments there is no persistent key. the key os only available in the dse.ldif, wrapped by the cert key. This needs to be handled in migration (don't know if there is a function to import keys)

389-ds-bot commented 4 years ago

Comment from tbordaz (@tbordaz) at 2020-02-25 13:48:25

@elkris nice finding and description !

Does it apply for CL encryption as well (ability to use the persistent nss key instead of the encrypted one in the CL config record) ?

reading @Firstyear, a second call to pk11_TokenKeyGenWithFlags would regenerate the same key that was used for encryption. The first call did not make it persistent. But if we systematically call pk11_TokenKeyGenWithFlags (when there is no key in nss) we should be able to use it to decrypt attr/CL. correct ? In such case migration would only be to remove 'encrypted secret value' from attr/CL record.

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2020-02-25 14:28:05

@tbordaz It is the same for CL, it does call into the attr_encrypt functions (by soem magic)

I do not think that always the same key is generated with pk11_..GEnKey.. functions, otherwise there would only be a single key. ANd my tests, removing the wrapped keys from dse.ldif and restart showed that a new key was created and the old encrypted attributes are no longer decryptable.

The advantag of using a persistent key in nss db is that we no longer need the cert to wrap it and store it in dse.ldif and so will be independent from cert change

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2020-02-26 00:18:34

I spent some time on this to try to get it work and understand what's going on. The sample code in #2584#comment-487006 worked fine, but in a 389 install I failed to find the key in the nss database. Trying symkeyutil from nss did work on the sample code but didn't show anything for a 389 instance. Finally I realized the sample code uses: key = PK11_TokenKeyGen(slot, cipherMech, 0, 32 /keysize/, &keyiditem, PR_TRUE, 0);

which creates a persistent key, but in 389 we are using new_symmetric_key = slapd_pk11_TokenKeyGenWithFlags(....)

which does by default NOT create a persistent key. The following patch made it work: diff --git a/ldap/servers/slapd/back-ldbm/ldbm_attrcrypt.c b/ldap/servers/slapd/back-ldbm/ldbm_attrcrypt.c index b42ec9e3d..6b881e58a 100644 --- a/ldap/servers/slapd/back-ldbm/ldbm_attrcrypt.c +++ b/ldap/servers/slapd/back-ldbm/ldbm_attrcrypt.c @@ -317,7 +317,7 @@ attrcrypt_generate_key(attrcrypt_cipher_state *acs, PK11SymKey *symmetric_key) acs->ace->key_size, NULL /keyid/, CKF_DECRYPT /op*/,

CKF_ENCRYPT /attr/,

CKF_ENCRYPT | PK11_ATTR_TOKEN | PK11_ATTR_PRIVATE /attr/, NULL); if (new_symmetric_key) { *symmetric_key = new_symmetric_key;

In conclusion:

we can generate persistent keys and use them directly from the nss database and get rid of all certificate renewal worries

for existing deployments there is no persistent key. the key os only available in the dse.ldif, wrapped by the cert key. This needs to be handled in migration (don't know if there is a function to import keys)

This is weird, because the current "migration" strategy for consumers is to delete their certificate and saved key, and then re-start so that slapd_pk11_TokenKeyGenWithFlags runs again. Does this mean that the current advice that support is giving to users is incorrect with regard to how to process attr encryption renewal?

Anyway, thanks for your detailed research here. I think we should add the patch as you describe. Perhaps the right way to restructure this logic is:

If we have an attribute that contains an encrypted attr key, we decrypt and attempt to use that.
If we do not have that attribute, we use slapd_pk11_TokenKeyGenWithFlags to get the key
We remove the code that sets the encrypted attr key value after the call to slapd_pk11_TokenKeyGenWithFlags

Is that a reasonable improvement to how we should proceed here?

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2020-02-26 09:14:45

This is weird, because the current "migration" strategy for consumers is to delete their certificate and saved key, and then re-start so that slapd_pk11_TokenKeyGenWithFlags runs again. Does this mean that the current advice that support is giving to users is incorrect with regard to how to process attr encryption renewal?

No. It is correct.

The key is stored in, and only in cn=[AES|DES3],cn=encrypted attribute keys,cn=database_name,cn=ldbm database,cn=plugins,cn=config and is wrapped with the key of the current certificate. If you change the certificate you can no longer access the key. And if you create a new key you can no longer decrypt already encrypted attributes. So the only way to handle certificate renewal currently is:

export the database/changelog and decrypt the attrs
delete the existing keys in the dse.ldif
reimport the clear ldif an tell to encrypt. At startup DS will detect that there is no key and create a new one and encrypt everything with the new key

Anyway, thanks for your detailed research here. I think we should add the patch as you describe. Perhaps the right way to restructure this logic is:

If we have an attribute that contains an encrypted attr key, we decrypt and attempt to use that. If we do not have that attribute, we use slapd_pk11_TokenKeyGenWithFlags to get the key We remove the code that sets the encrypted attr key value after the call to slapd_pk11_TokenKeyGenWithFlags

Is that a reasonable improvement to how we should proceed here?

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2020-02-26 09:19:37

Anyway, thanks for your detailed research here. I think we should add the patch as you describe. Perhaps the right way to restructure this logic is:

If we have an attribute that contains an encrypted attr key, we decrypt and attempt to use that. If we do not have that attribute, we use slapd_pk11_TokenKeyGenWithFlags to get the key We remove the code that sets the encrypted attr key value after the call to slapd_pk11_TokenKeyGenWithFlags

Is that a reasonable improvement to how we should proceed here?

Something like this, my proposal would be:

check for a key in the nss database (should name them aes-db, aes-cl, des3-db,....)
if it exists use it
if it doesn't exist check for the existence of a key below cn=config
if it exists, import it to the nss database and delete the cn=config entry
if it doesn't exist create a new key with slapd_pk11_TokenKeyGenWithFlags()

389-ds-bot commented 4 years ago

Comment from tbordaz (@tbordaz) at 2020-02-26 10:23:31

The procedure looks good to me. In step 4 it removes the existing key after having storing it in nss db. Before removing it, Is there a way to check that the old key, now stored in nss db, can be read and is valid to decrypt some of encrypted data ?

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2020-02-26 12:09:28

The procedure looks good to me. In step 4 it removes the existing key after having storing it in nss db. Before removing it, Is there a way to check that the old key, now stored in nss db, can be read and is valid to decrypt some of encrypted data ?

you don't trust this :-) I think if we have implemented and got QE blessing showing that it works would make it work in real deployments. But we can add some safety and after importing the key looking it up again and test it, but we would need to find an ebcrypted attribute to verify.

So maybe the safety net could be like this: -find the key in the cn=config entry, use it to encrypt some string "test", import the key to the nss db, retrieve it, decrypt the string test again, if it works delete the config entry

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2020-02-27 04:34:58

Thanks for clarifying I had forgotten the export/import steps in the process.

I think you may be right that it's not clear if we can import keys into the nss database though, so maybe your procedure should be:

If a key exists below cn=config and we can decrypt it, use it.
Check for a key in nss database
If it doesn't exist create a key in the nss database.

That seems like the easiest path forward for migration and support. The current process of export/delete key/import would be run "one last time" and then the nss database key would be use forever more. It also means that new install would not have a cn=config key and would 'just work'

Is that reasonable?

389-ds-bot commented 4 years ago

Comment from lkrispen (@elkris) at 2020-02-27 09:32:40

yes, that seems to be the migration path with the least effort, although I would like to find out if the ImportKey would work. But the request to do one final manual renewal should be tolerable

389-ds-bot commented 4 years ago

Comment from firstyear (@Firstyear) at 2020-02-28 02:55:30

Yes, the manual review seems reasonable to me. If import key works, that would be really great. Thanks for your insights into this!

389-ds-bot commented 4 years ago

Comment from mreynolds (@mreynolds389) at 2020-05-20 16:35:01

For reference this ticket could addressed in this: #909

389ds / 389-ds-base

Improve attr encryption key rollover #2584

Issue Description