search

opensearch-project / security

🔐 Secure your cluster with TLS, numerous authentication backends, data masking, audit logging as well as role-based access control on indices, documents, and fields
https://opensearch.org/docs/latest/security-plugin/index/
Apache License 2.0
189 stars 272 forks source link

[BUG] Blake2b hashing for Masked Fields does not apply salt correctly #4274

Open terryquigleysas opened 5 months ago

terryquigleysas commented 5 months ago

I think there is possibly a bug in the current Blake2b code. The defaultSalt variable is not passed to the Blake2bDigest constructor using the salt parameter as I would expect. It uses personalization instead. Is this correct?

image

image

In order to proceed which of these options is recommended?

  1. Change to pass the defaultSalt variable to the Blake2bDigest constructor using the salt parameter. This will change the hash values. OR
  2. Continue to pass the defaultSalt variable to the Blake2bDigest constructor using the personalization parameter, assuming there is a valid reason for this. The hash values generated will remain the same.

Originally posted by @terryquigleysas in https://github.com/opensearch-project/security/issues/4212#issuecomment-2063453718

Approach #1 would appear to be the correct thing to do but there are concerns that changing hashes, even to the correct values, may impact existing users.

This bug is derived from discussions on https://github.com/opensearch-project/security/pull/4271 and https://github.com/opensearch-project/security/issues/4212

stephen-crawford commented 4 months ago

[Triage] Hi @terryquigleysas thank you for filing this issue. This sounds like a worthwhile change which could help correct some unexpected behavior. We will need to handle the issues around the backwards compatibility of the code when reviewing the PR.