You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "szlta (via GitHub)" <gi...@apache.org> on 2025/11/25 18:29:54 UTC
[PR] [WiP] Hive: Metadata integrity check for encrypted tables [iceberg]
szlta opened a new pull request, #14685:
URL: https://github.com/apache/iceberg/pull/14685
Introducing new Hive table property metadata_hash (to be stored exclusively in HMS) that tracks the hash of the current table metadata.
It is used in HiveTableOperations to carry out integrity check and ensure that the metadata.json has not been tampered with when table encryption is used.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Hive: Metadata integrity check for encrypted tables [iceberg]
Posted by "szlta (via GitHub)" <gi...@apache.org>.
szlta commented on code in PR #14685:
URL: https://github.com/apache/iceberg/pull/14685#discussion_r2598505601
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -574,8 +580,20 @@ private void encryptionPropsFromMetadata(Map<String, String> tableProperties) {
}
}
- private void checkEncryptionProperties(String encryptionKeyIdFromHMS, String dekLengthFromHMS) {
- Map<String, String> propertiesFromMetadata = current().properties();
+ private void checkIntegrityForEncryption(
+ String encryptionKeyIdFromHMS, String dekLengthFromHMS, String metadataHashFromHMS) {
+ TableMetadata metadata = current();
+ if (StringUtils.isNotEmpty(metadataHashFromHMS)) {
+ HMSTablePropertyHelper.verifyMetadataHash(metadata, metadataHashFromHMS);
+ return;
+ }
+
+ LOG.warn(
+ "Full metadata integrity check skipped because no metadata hash was recorded in HMS for table {}."
+ + "Falling back to encryption property based check.",
Review Comment:
Thanks for spotting it. Fixed.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WiP] Hive: Metadata integrity check for encrypted tables [iceberg]
Posted by "ggershinsky (via GitHub)" <gi...@apache.org>.
ggershinsky commented on code in PR #14685:
URL: https://github.com/apache/iceberg/pull/14685#discussion_r2567379462
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HMSTablePropertyHelper.java:
##########
@@ -256,6 +278,42 @@ static void setSchema(
}
}
+ @VisibleForTesting
+ static void setMetadataHash(
+ TableMetadata metadata, Map<String, String> parameters, long maxHiveTablePropertySize) {
+ if (exposeInHmsProperties(maxHiveTablePropertySize)
+ && parameters.containsKey(TableProperties.ENCRYPTION_TABLE_KEY)) {
+ String currentMetadataAsJson = TableMetadataParser.toJson(metadata);
Review Comment:
Implementation-related question(s) - how big can the metadata object become in production deployments? Are there other Iceberg calls to this method (not in unitests)?
If the in-memory serialization of a full metadata object can be a problem, there might be stream methods for a hash calculation.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Hive: Metadata integrity check for encrypted tables [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao merged PR #14685:
URL: https://github.com/apache/iceberg/pull/14685
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WiP] Hive: Metadata integrity check for encrypted tables [iceberg]
Posted by "szlta (via GitHub)" <gi...@apache.org>.
szlta commented on code in PR #14685:
URL: https://github.com/apache/iceberg/pull/14685#discussion_r2581852336
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HMSTablePropertyHelper.java:
##########
@@ -256,6 +278,42 @@ static void setSchema(
}
}
+ @VisibleForTesting
+ static void setMetadataHash(
+ TableMetadata metadata, Map<String, String> parameters, long maxHiveTablePropertySize) {
+ if (exposeInHmsProperties(maxHiveTablePropertySize)
+ && parameters.containsKey(TableProperties.ENCRYPTION_TABLE_KEY)) {
+ String currentMetadataAsJson = TableMetadataParser.toJson(metadata);
Review Comment:
Yes it totally makes sense. In my new iteration I've added a `HashWriter` class that can be used to incrementally calculate hash by output generating classes that take a `Writer` object.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WiP] Hive: Metadata integrity check for encrypted tables [iceberg]
Posted by "szlta (via GitHub)" <gi...@apache.org>.
szlta commented on PR #14685:
URL: https://github.com/apache/iceberg/pull/14685#issuecomment-3577006283
@ggershinsky this is regarding our discussion at https://github.com/apache/iceberg/pull/13225#discussion_r2556769049
Let me know if you think this approach is right, then I'll work on the finishing touches and tests.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Hive: Metadata integrity check for encrypted tables [iceberg]
Posted by "ggershinsky (via GitHub)" <gi...@apache.org>.
ggershinsky commented on code in PR #14685:
URL: https://github.com/apache/iceberg/pull/14685#discussion_r2587844421
##########
core/src/main/java/org/apache/iceberg/util/HashWriter.java:
##########
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.io.IOException;
+import java.io.Writer;
+import java.nio.ByteBuffer;
+import java.nio.CharBuffer;
+import java.nio.charset.Charset;
+import java.nio.charset.CharsetEncoder;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+
+/**
+ * {@link java.io.Writer} implementation that uses a hashing function to produce a hash value based
+ * on the streamed bytes. The output of the writer is not preserved.
+ */
+public class HashWriter extends Writer {
+
+ private final MessageDigest digest;
+ private final CharsetEncoder encoder;
+
+ public HashWriter(String hashAlgorithm, Charset charset) throws NoSuchAlgorithmException {
+ this.digest = MessageDigest.getInstance(hashAlgorithm);
+ this.encoder = charset.newEncoder();
+ }
+
+ @Override
+ public void write(char[] cbuf, int off, int len) throws IOException {
+ CharBuffer chars = CharBuffer.wrap(cbuf, off, len);
+ ByteBuffer byteBuffer = encoder.encode(chars);
+ digest.update(byteBuffer);
+ }
+
+ @Override
+ public void flush() throws IOException {}
+
+ @Override
+ public void close() throws IOException {}
+
+ public byte[] getHash() {
+ return digest.digest();
Review Comment:
nit: an exception if not closed?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Hive: Metadata integrity check for encrypted tables [iceberg]
Posted by "ggershinsky (via GitHub)" <gi...@apache.org>.
ggershinsky commented on PR #14685:
URL: https://github.com/apache/iceberg/pull/14685#issuecomment-3616577161
cc @huaxingao
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Hive: Metadata integrity check for encrypted tables [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #14685:
URL: https://github.com/apache/iceberg/pull/14685#issuecomment-3628238548
Thanks @szlta for the PR! Thanks @ggershinsky for the review!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] [WiP] Hive: Metadata integrity check for encrypted tables [iceberg]
Posted by "ggershinsky (via GitHub)" <gi...@apache.org>.
ggershinsky commented on PR #14685:
URL: https://github.com/apache/iceberg/pull/14685#issuecomment-3580876047
Thanks @szlta , this approach looks good to me.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Hive: Metadata integrity check for encrypted tables [iceberg]
Posted by "szlta (via GitHub)" <gi...@apache.org>.
szlta commented on code in PR #14685:
URL: https://github.com/apache/iceberg/pull/14685#discussion_r2591946707
##########
core/src/main/java/org/apache/iceberg/util/HashWriter.java:
##########
@@ -0,0 +1,60 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.util;
+
+import java.io.IOException;
+import java.io.Writer;
+import java.nio.ByteBuffer;
+import java.nio.CharBuffer;
+import java.nio.charset.Charset;
+import java.nio.charset.CharsetEncoder;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+
+/**
+ * {@link java.io.Writer} implementation that uses a hashing function to produce a hash value based
+ * on the streamed bytes. The output of the writer is not preserved.
+ */
+public class HashWriter extends Writer {
+
+ private final MessageDigest digest;
+ private final CharsetEncoder encoder;
+
+ public HashWriter(String hashAlgorithm, Charset charset) throws NoSuchAlgorithmException {
+ this.digest = MessageDigest.getInstance(hashAlgorithm);
+ this.encoder = charset.newEncoder();
+ }
+
+ @Override
+ public void write(char[] cbuf, int off, int len) throws IOException {
+ CharBuffer chars = CharBuffer.wrap(cbuf, off, len);
+ ByteBuffer byteBuffer = encoder.encode(chars);
+ digest.update(byteBuffer);
+ }
+
+ @Override
+ public void flush() throws IOException {}
+
+ @Override
+ public void close() throws IOException {}
+
+ public byte[] getHash() {
+ return digest.digest();
Review Comment:
You're right, it's needed as calling `digest()` will cause a reset in `MessageDigest` internal state and further calls will produce a wrong value. I've added a closing logic now.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Hive: Metadata integrity check for encrypted tables [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #14685:
URL: https://github.com/apache/iceberg/pull/14685#discussion_r2597352997
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -574,8 +580,20 @@ private void encryptionPropsFromMetadata(Map<String, String> tableProperties) {
}
}
- private void checkEncryptionProperties(String encryptionKeyIdFromHMS, String dekLengthFromHMS) {
- Map<String, String> propertiesFromMetadata = current().properties();
+ private void checkIntegrityForEncryption(
+ String encryptionKeyIdFromHMS, String dekLengthFromHMS, String metadataHashFromHMS) {
+ TableMetadata metadata = current();
+ if (StringUtils.isNotEmpty(metadataHashFromHMS)) {
+ HMSTablePropertyHelper.verifyMetadataHash(metadata, metadataHashFromHMS);
+ return;
+ }
+
+ LOG.warn(
+ "Full metadata integrity check skipped because no metadata hash was recorded in HMS for table {}."
+ + "Falling back to encryption property based check.",
Review Comment:
nit: add a space before `Falling`?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Hive: Metadata integrity check for encrypted tables [iceberg]
Posted by "szlta (via GitHub)" <gi...@apache.org>.
szlta commented on PR #14685:
URL: https://github.com/apache/iceberg/pull/14685#issuecomment-3631930918
Thanks for review @ggershinsky and @huaxingao
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org