From 9fa29902575ac3774bf3728e7bcde7f3eefb1d4c Mon Sep 17 00:00:00 2001 From: Andrew Wang Date: Mon, 1 Dec 2014 21:21:23 -0800 Subject: [PATCH] HADOOP-11337. KeyAuthorizationKeyProvider access checks need to be done atomically. Contributed by Dian Fu. --- .../hadoop-common/CHANGES.txt | 3 + .../server/KeyAuthorizationKeyProvider.java | 137 ++++++++++++++---- 2 files changed, 108 insertions(+), 32 deletions(-) diff --git a/hadoop-common-project/hadoop-common/CHANGES.txt b/hadoop-common-project/hadoop-common/CHANGES.txt index 8a544c1bed..f7274c88e9 100644 --- a/hadoop-common-project/hadoop-common/CHANGES.txt +++ b/hadoop-common-project/hadoop-common/CHANGES.txt @@ -479,6 +479,9 @@ Release 2.7.0 - UNRELEASED HADOOP-11333. Fix deadlock in DomainSocketWatcher when the notification pipe is full (zhaoyunjiong via cmccabe) + HADOOP-11337. KeyAuthorizationKeyProvider access checks need to be done + atomically. (Dian Fu via wang) + Release 2.6.0 - 2014-11-18 INCOMPATIBLE CHANGES diff --git a/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KeyAuthorizationKeyProvider.java b/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KeyAuthorizationKeyProvider.java index 0e43b47b2e..4ce961111b 100644 --- a/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KeyAuthorizationKeyProvider.java +++ b/hadoop-common-project/hadoop-kms/src/main/java/org/apache/hadoop/crypto/key/kms/server/KeyAuthorizationKeyProvider.java @@ -23,6 +23,9 @@ import java.security.NoSuchAlgorithmException; import java.util.List; import java.util.Map; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; import org.apache.hadoop.crypto.key.KeyProvider; import org.apache.hadoop.crypto.key.KeyProviderCryptoExtension; @@ -83,6 +86,8 @@ public boolean hasAccessToKey(String aclName, UserGroupInformation ugi, private final KeyProviderCryptoExtension provider; private final KeyACLs acls; + private Lock readLock; + private Lock writeLock; /** * The constructor takes a {@link KeyProviderCryptoExtension} and an @@ -96,6 +101,9 @@ public KeyAuthorizationKeyProvider(KeyProviderCryptoExtension keyProvider, super(keyProvider, null); this.provider = keyProvider; this.acls = acls; + ReadWriteLock lock = new ReentrantReadWriteLock(true); + readLock = lock.readLock(); + writeLock = lock.writeLock(); } // This method first checks if "key.acl.name" attribute is present as an @@ -146,50 +154,85 @@ private void checkAccess(String aclName, UserGroupInformation ugi, @Override public KeyVersion createKey(String name, Options options) throws NoSuchAlgorithmException, IOException { - authorizeCreateKey(name, options, getUser()); - return provider.createKey(name, options); + writeLock.lock(); + try { + authorizeCreateKey(name, options, getUser()); + return provider.createKey(name, options); + } finally { + writeLock.unlock(); + } } @Override public KeyVersion createKey(String name, byte[] material, Options options) throws IOException { - authorizeCreateKey(name, options, getUser()); - return provider.createKey(name, material, options); + writeLock.lock(); + try { + authorizeCreateKey(name, options, getUser()); + return provider.createKey(name, material, options); + } finally { + writeLock.unlock(); + } } @Override public KeyVersion rollNewVersion(String name) throws NoSuchAlgorithmException, IOException { - doAccessCheck(name, KeyOpType.MANAGEMENT); - return provider.rollNewVersion(name); + writeLock.lock(); + try { + doAccessCheck(name, KeyOpType.MANAGEMENT); + return provider.rollNewVersion(name); + } finally { + writeLock.unlock(); + } } @Override public void deleteKey(String name) throws IOException { - doAccessCheck(name, KeyOpType.MANAGEMENT); - provider.deleteKey(name); + writeLock.lock(); + try { + doAccessCheck(name, KeyOpType.MANAGEMENT); + provider.deleteKey(name); + } finally { + writeLock.unlock(); + } } @Override public KeyVersion rollNewVersion(String name, byte[] material) throws IOException { - doAccessCheck(name, KeyOpType.MANAGEMENT); - return provider.rollNewVersion(name, material); + writeLock.lock(); + try { + doAccessCheck(name, KeyOpType.MANAGEMENT); + return provider.rollNewVersion(name, material); + } finally { + writeLock.unlock(); + } } @Override public void warmUpEncryptedKeys(String... names) throws IOException { - for (String name : names) { - doAccessCheck(name, KeyOpType.GENERATE_EEK); + readLock.lock(); + try { + for (String name : names) { + doAccessCheck(name, KeyOpType.GENERATE_EEK); + } + provider.warmUpEncryptedKeys(names); + } finally { + readLock.unlock(); } - provider.warmUpEncryptedKeys(names); } @Override public EncryptedKeyVersion generateEncryptedKey(String encryptionKeyName) throws IOException, GeneralSecurityException { - doAccessCheck(encryptionKeyName, KeyOpType.GENERATE_EEK); - return provider.generateEncryptedKey(encryptionKeyName); + readLock.lock(); + try { + doAccessCheck(encryptionKeyName, KeyOpType.GENERATE_EEK); + return provider.generateEncryptedKey(encryptionKeyName); + } finally { + readLock.unlock(); + } } private void verifyKeyVersionBelongsToKey(EncryptedKeyVersion ekv) @@ -206,19 +249,29 @@ private void verifyKeyVersionBelongsToKey(EncryptedKeyVersion ekv) @Override public KeyVersion decryptEncryptedKey(EncryptedKeyVersion encryptedKeyVersion) throws IOException, GeneralSecurityException { - verifyKeyVersionBelongsToKey(encryptedKeyVersion); - doAccessCheck( - encryptedKeyVersion.getEncryptionKeyName(), KeyOpType.DECRYPT_EEK); - return provider.decryptEncryptedKey(encryptedKeyVersion); + readLock.lock(); + try { + verifyKeyVersionBelongsToKey(encryptedKeyVersion); + doAccessCheck( + encryptedKeyVersion.getEncryptionKeyName(), KeyOpType.DECRYPT_EEK); + return provider.decryptEncryptedKey(encryptedKeyVersion); + } finally { + readLock.unlock(); + } } @Override public KeyVersion getKeyVersion(String versionName) throws IOException { - KeyVersion keyVersion = provider.getKeyVersion(versionName); - if (keyVersion != null) { - doAccessCheck(keyVersion.getName(), KeyOpType.READ); + readLock.lock(); + try { + KeyVersion keyVersion = provider.getKeyVersion(versionName); + if (keyVersion != null) { + doAccessCheck(keyVersion.getName(), KeyOpType.READ); + } + return keyVersion; + } finally { + readLock.unlock(); } - return keyVersion; } @Override @@ -228,28 +281,48 @@ public List getKeys() throws IOException { @Override public List getKeyVersions(String name) throws IOException { - doAccessCheck(name, KeyOpType.READ); - return provider.getKeyVersions(name); + readLock.lock(); + try { + doAccessCheck(name, KeyOpType.READ); + return provider.getKeyVersions(name); + } finally { + readLock.unlock(); + } } @Override public Metadata getMetadata(String name) throws IOException { - doAccessCheck(name, KeyOpType.READ); - return provider.getMetadata(name); + readLock.lock(); + try { + doAccessCheck(name, KeyOpType.READ); + return provider.getMetadata(name); + } finally { + readLock.unlock(); + } } @Override public Metadata[] getKeysMetadata(String... names) throws IOException { - for (String name : names) { - doAccessCheck(name, KeyOpType.READ); + readLock.lock(); + try { + for (String name : names) { + doAccessCheck(name, KeyOpType.READ); + } + return provider.getKeysMetadata(names); + } finally { + readLock.unlock(); } - return provider.getKeysMetadata(names); } @Override public KeyVersion getCurrentKey(String name) throws IOException { - doAccessCheck(name, KeyOpType.READ); - return provider.getCurrentKey(name); + readLock.lock(); + try { + doAccessCheck(name, KeyOpType.READ); + return provider.getCurrentKey(name); + } finally { + readLock.unlock(); + } } @Override