[GitHub] [hbase] symat commented on a change in pull request #2539: HBASE-25181 Configure hash algorithm in wrapped encryption keys

[GitBox <gi...@apache.org>]

symat commented on a change in pull request #2539:
URL: https://github.com/apache/hbase/pull/2539#discussion_r516479856



##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/Encryption.java
##########
@@ -578,4 +627,45 @@ public static void incrementIv(byte[] iv, int v) {
     } while (v > 0);
   }
 
+  /**
+   * Return the hash of the concatenation of the supplied arguments, using the 
+   * hash algorithm provided.
+   */
+  public static byte[] hashWithAlg(String algorithm, byte[]... args) {
+    try {
+      MessageDigest md = MessageDigest.getInstance(algorithm);
+      for (byte[] arg: args) {
+        md.update(arg);
+      }
+      return md.digest();
+    } catch (NoSuchAlgorithmException e) {
+      throw new RuntimeException("unable to use hash algorithm: " + algorithm, e);
+    }
+  }
+
+  private static byte[] hashWithAlg(String algorithm, String... args) {

Review comment:
       Originally I decided against it for performance reasons, I wanted to avoid the creation of a new long string or byte array. I wanted to have the same memory footprint as the original implementation had, as this was a function on the public API. 
   
   However, I don't think anyone is using this function on any critical path. And your suggestion makes the code cleaner. I'll change this. I also found the `Bytes.toByteArrays` function so I can fully eliminate this private function.

##########
File path: hbase-shell/src/main/ruby/hbase/admin.rb
##########
@@ -1137,7 +1137,7 @@ def cfd(arg, tdb)
         algorithm = arg.delete(ColumnFamilyDescriptorBuilder::ENCRYPTION).upcase
         cfdb.setEncryptionType(algorithm)
         if arg.include?(ColumnFamilyDescriptorBuilder::ENCRYPTION_KEY)
-          key = org.apache.hadoop.hbase.io.crypto.Encryption.pbkdf128(
+          key = org.apache.hadoop.hbase.io.crypto.Encryption.pbkdf384(

Review comment:
       OK, I'll revert this part. I originally added this change to eliminate SHA1 algorithm use (in the pbkdf128 method). 
   
   Here we use a key generation algorithm to generate a secret key (based on the string provided in the hbase shell) which will be used to encrypt the store/wal files. According to HBASE-10951 (where this algorithm was already changed once), this is only used for testing schema in the shell, the proper secret keys should be generated by using the Java API. 
   
   However, I'm not 100% sure if someone is not using this method in the HBase shell in production now to define the secret key, so maybe it is not a good idea to change. I agree that it worths further discussion.

##########
File path: hbase-protocol-shaded/src/main/protobuf/client/Encryption.proto
##########
@@ -31,4 +31,5 @@ message WrappedKey {
   required bytes data = 3;
   optional bytes iv = 4;
   optional bytes hash = 5;
+  optional string hash_algorithm = 6 [default = "MD5"];

Review comment:
       In general I think it is a good way to ensure backward compatibility. What do you think? Should I change this part somehow?
   
   Some background info: 
   
   We serialize this protobuf data structure e.g. into the tailer of the encrypted HFiles. Also this travels on the network (e.g. when a client sends a create table request and sending column family descriptors). And we use it when we initialize the crypto contexts when we open a region on a RegionServer.
   
   Adding the `hash_algorithm="MD5"` default value here enabling us to open an encrypted HFile written with an older HBase (where the `hash_algorithm` was not defined) and also enabling the use of an older client (or HBase shell) to e.g create table with encrypted column families when it is connecting to the latest HBase master.

##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/Encryption.java
##########
@@ -127,79 +180,63 @@ public static Cipher getCipher(Configuration conf, String name) {
   }
 
   /**
-   * Return the MD5 digest of the concatenation of the supplied arguments.
+   * Returns the Hash Algorithm defined in the crypto configuration.
    */
-  public static byte[] hash128(String... args) {
-    byte[] result = new byte[16];
+  public static String getConfiguredHashAlgorithm(Configuration conf) {
+    return conf.get(CRYPTO_KEY_HASH_ALGORITHM_CONF_KEY,

Review comment:
       nice, I didn't know this function. I changed the code, thanks!




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org