singhpk234 commented on code in PR #18220:
URL: https://github.com/apache/iceberg/pull/18220#discussion_r4350097087
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java:
##########
@@ -403,6 +532,117 @@ void cleanupMetadataAndUnlock(CommitStatus commitStatus,
String metadataLocation
}
}
+ @Override
+ public TableOperations temp(TableMetadata uncommittedMetadata) {
+ return new TableOperations() {
+ @Override
+ public TableMetadata current() {
+ return uncommittedMetadata;
+ }
+
+ @Override
+ public TableMetadata refresh() {
+ throw new UnsupportedOperationException(
+ "Cannot call refresh on temporary table operations");
+ }
+
+ @Override
+ public void commit(TableMetadata base, TableMetadata metadata) {
+ throw new UnsupportedOperationException("Cannot call commit on
temporary table operations");
+ }
+
+ @Override
+ public String metadataFileLocation(String fileName) {
+ return
GlueTableOperations.this.metadataFileLocation(uncommittedMetadata, fileName);
+ }
+
+ @Override
+ public LocationProvider locationProvider() {
+ return LocationProviders.locationsFor(
+ uncommittedMetadata.location(), uncommittedMetadata.properties());
+ }
+
+ @Override
+ public FileIO io() {
+
GlueTableOperations.this.encryptionPropsFromMetadata(uncommittedMetadata.properties());
+ return GlueTableOperations.this.io();
+ }
+
+ @Override
+ public EncryptionManager encryption() {
+ return GlueTableOperations.this.encryption();
+ }
+
+ @Override
+ public long newSnapshotId() {
+ return GlueTableOperations.this.newSnapshotId();
+ }
+ };
+ }
+
+ private void encryptionPropsFromMetadata(Map<String, String>
tableProperties) {
+ if (tableKeyId == null) {
+ this.tableKeyId =
tableProperties.get(TableProperties.ENCRYPTION_TABLE_KEY);
+ }
+
+ if (tableKeyId != null && encryptionDekLength <= 0) {
+ this.encryptionDekLength =
+ PropertyUtil.propertyAsInt(
+ tableProperties,
+ TableProperties.ENCRYPTION_DEK_LENGTH,
+ TableProperties.ENCRYPTION_DEK_LENGTH_DEFAULT);
+ }
+ }
+
+ /** The key ID of an encrypted table must stay the same for the whole table
lifetime. */
+ private static void checkEncryptionKeyUnchanged(TableMetadata base,
TableMetadata metadata) {
+ if (base == null) {
+ return;
+ }
+
+ String baseKeyId =
base.properties().get(TableProperties.ENCRYPTION_TABLE_KEY);
+ String newKeyId =
metadata.properties().get(TableProperties.ENCRYPTION_TABLE_KEY);
+
+ Preconditions.checkArgument(
+ baseKeyId == null || newKeyId != null, "Cannot remove key ID from an
encrypted table");
+ Preconditions.checkArgument(
+ Objects.equals(baseKeyId, newKeyId), "Cannot modify key ID of an
encrypted table");
+ }
+
+ private void checkIntegrityForEncryption(
+ String encryptionKeyIdFromGlue, String dekLengthFromGlue, String
metadataHashFromGlue) {
+ TableMetadata metadata = current();
+ if (!Strings.isNullOrEmpty(metadataHashFromGlue)) {
+ EncryptionUtil.verifyMetadataHash(metadata, metadataHashFromGlue);
+ return;
+ }
+
+ LOG.warn(
+ "Full metadata integrity check skipped because no metadata hash was
recorded in Glue for table {}."
+ + " Falling back to encryption property based check.",
+ tableName());
+
+ Map<String, String> propertiesFromMetadata = metadata.properties();
+
+ String encryptionKeyIdFromMetadata =
+ propertiesFromMetadata.get(TableProperties.ENCRYPTION_TABLE_KEY);
+ if (!Objects.equals(encryptionKeyIdFromGlue, encryptionKeyIdFromMetadata))
{
+ throw new RuntimeException(
+ String.format(
+ "Metadata file might have been modified. Encryption key id %s
differs from Glue value %s",
+ encryptionKeyIdFromMetadata, encryptionKeyIdFromGlue));
+ }
+
+ String dekLengthFromMetadata =
+ propertiesFromMetadata.get(TableProperties.ENCRYPTION_DEK_LENGTH);
+ if (!Objects.equals(dekLengthFromGlue, dekLengthFromMetadata)) {
+ throw new RuntimeException(
+ String.format(
+ "Metadata file might have been modified. DEK length %s differs
from Glue value %s",
+ dekLengthFromMetadata, dekLengthFromGlue));
+ }
+ }
Review Comment:
I feel like we are duplicating this logic between here and the Hive if the
whole goal is to have the consistency, why not put it in a helper in core where
both catalog impl can reuse it
https://github.com/apache/iceberg/blob/main/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java#L558
##########
core/src/main/java/org/apache/iceberg/encryption/EncryptionUtil.java:
##########
@@ -202,6 +211,51 @@ static ByteBuffer encryptManifestListKeyMetadata(
return ByteBuffer.wrap(encryptedKeyMetadata);
}
+ /**
+ * Produces a base64-encoded hash of the serialized table metadata.
+ *
+ * <p>Catalogs that keep encryption parameters, such as the table key ID,
outside of the metadata
+ * file can store this hash next to them, in order to detect tampering with
a metadata file kept
+ * in a storage vulnerable to modification.
+ *
+ * @param metadata table metadata to hash
+ * @return base64-encoded hash of the metadata
+ */
+ public static String metadataHash(TableMetadata metadata) {
+ return Base64.getEncoder().encodeToString(hashOf(metadata));
+ }
+
+ /**
+ * Verifies that the table metadata loaded from storage matches a hash
produced by {@link
+ * #metadataHash(TableMetadata)} and kept in a trusted catalog.
+ *
+ * @param metadata table metadata loaded from storage
+ * @param expectedMetadataHash base64-encoded hash retrieved from the catalog
+ */
+ public static void verifyMetadataHash(TableMetadata metadata, String
expectedMetadataHash) {
+ byte[] currentHashBytes = hashOf(metadata);
Review Comment:
This hashes the whole obj for the big metadata json ~10s - 100s of MB
wouldn't this be cost prohibitive ?
its not a action item for the pr but we should think through this ... one
thing is in the input stream itself we can add sha256 computation ... so as we
read the bytes we are calculating sha256 for this too ...
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueTableOperations.java:
##########
@@ -125,10 +195,20 @@ protected String tableName() {
@Override
protected void doRefresh() {
String metadataLocation = null;
+ String tableKeyIdFromGlue = null;
+ String dekLengthFromGlue = null;
+ String metadataHashFromGlue = null;
Table table = getGlueTable();
if (table != null) {
checkIfTableIsIceberg(table, tableName());
metadataLocation = table.parameters().get(METADATA_LOCATION_PROP);
+ /* Table key ID must be retrieved from a catalog service, and not from
untrusted storage
+ (e.g. metadata json file) that can be tampered with. For example, an
attacker can remove
+ the table key parameter (along with existing snapshots) in the file,
making the writers
+ produce unencrypted files. Table key ID is taken directly from the Glue
catalog */
+ tableKeyIdFromGlue =
table.parameters().get(TableProperties.ENCRYPTION_TABLE_KEY);
Review Comment:
I see that makes sense, then i feel we should be consistent in iceberg-java
for all the catalog impl including rest, we just document hash of metadata :
https://iceberg.apache.org/docs/latest/encryption/?h=encryption#catalog-security-requirements,
should we update these too ?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
