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:
[email protected]
