You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@knox.apache.org by kr...@apache.org on 2019/06/21 19:05:45 UTC
[knox] branch master updated: KNOX-1881 - DefaultKeystoreService
should use Java NIO API locking as well
This is an automated email from the ASF dual-hosted git repository.
krisden pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/knox.git
The following commit(s) were added to refs/heads/master by this push:
new d9f9924 KNOX-1881 - DefaultKeystoreService should use Java NIO API locking as well
d9f9924 is described below
commit d9f99242b5cbc80809d73e41dd0157f554977f15
Author: Kevin Risden <kr...@apache.org>
AuthorDate: Fri Jun 21 13:05:47 2019 -0400
KNOX-1881 - DefaultKeystoreService should use Java NIO API locking as well
Signed-off-by: Kevin Risden <kr...@apache.org>
---
.../security/impl/DefaultKeystoreService.java | 316 ++++++++-------------
1 file changed, 119 insertions(+), 197 deletions(-)
diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/DefaultKeystoreService.java b/gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/DefaultKeystoreService.java
index 76ed008..7867ed3 100644
--- a/gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/DefaultKeystoreService.java
+++ b/gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/DefaultKeystoreService.java
@@ -35,10 +35,13 @@ import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.net.InetAddress;
+import java.nio.channels.Channels;
+import java.nio.channels.FileChannel;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
+import java.nio.file.StandardOpenOption;
import java.security.GeneralSecurityException;
import java.security.Key;
import java.security.KeyPair;
@@ -55,14 +58,10 @@ import java.util.HashMap;
import java.util.Locale;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.locks.Lock;
-import java.util.concurrent.locks.ReadWriteLock;
-import java.util.concurrent.locks.ReentrantReadWriteLock;
import javax.crypto.spec.SecretKeySpec;
public class DefaultKeystoreService implements KeystoreService, Service {
-
private static final String DN_TEMPLATE = "CN={0},OU=Test,O=Hadoop,L=Test,ST=Test,C=US";
private static final String CREDENTIALS_SUFFIX = "-credentials.jceks";
private static final String CREDENTIALS_STORE_TYPE = "JCEKS";
@@ -74,8 +73,6 @@ public class DefaultKeystoreService implements KeystoreService, Service {
private GatewayConfig config;
private Map<String, Map<String, String>> cache = new ConcurrentHashMap<>();
- private Lock readLock;
- private Lock writeLock;
private MasterService masterService;
private Path keyStoreDirPath;
@@ -87,10 +84,6 @@ public class DefaultKeystoreService implements KeystoreService, Service {
@Override
public void init(GatewayConfig config, Map<String, String> options)
throws ServiceLifecycleException {
- ReadWriteLock lock = new ReentrantReadWriteLock(true);
- readLock = lock.readLock();
- writeLock = lock.writeLock();
-
this.config = config;
this.keyStoreDirPath = Paths.get(config.getGatewayKeystoreDir());
@@ -115,12 +108,8 @@ public class DefaultKeystoreService implements KeystoreService, Service {
@Override
public void createKeystoreForGateway() throws KeystoreServiceException {
- writeLock.lock();
- try {
- createKeyStore(Paths.get(config.getIdentityKeystorePath()), config.getIdentityKeystoreType(), getKeyStorePassword(config.getIdentityKeystorePasswordAlias()));
- } finally {
- writeLock.unlock();
- }
+ createKeyStore(Paths.get(config.getIdentityKeystorePath()), config.getIdentityKeystoreType(),
+ getKeyStorePassword(config.getIdentityKeystorePasswordAlias()));
}
@Override
@@ -162,51 +151,45 @@ public class DefaultKeystoreService implements KeystoreService, Service {
@Override
public void addSelfSignedCertForGateway(String alias, char[] passphrase) throws KeystoreServiceException {
- writeLock.lock();
- try {
- addSelfSignedCertForGateway(alias, passphrase, null);
- }
- finally {
- writeLock.unlock();
- }
+ addSelfSignedCertForGateway(alias, passphrase, null);
}
@Override
public void addSelfSignedCertForGateway(String alias, char[] passphrase, String hostname)
throws KeystoreServiceException {
- writeLock.lock();
+ addCertForGateway(alias, passphrase, hostname);
+ }
+
+ private synchronized void addCertForGateway(String alias, char[] passphrase, String hostname)
+ throws KeystoreServiceException {
+ KeyPairGenerator keyPairGenerator;
try {
- KeyPairGenerator keyPairGenerator;
- try {
- keyPairGenerator = KeyPairGenerator.getInstance("RSA");
- keyPairGenerator.initialize(2048);
- KeyPair KPair = keyPairGenerator.generateKeyPair();
- if (hostname == null) {
- hostname = System.getProperty(CERT_GEN_MODE, CERT_GEN_MODE_LOCALHOST);
- }
- X509Certificate cert;
- if(hostname.equals(CERT_GEN_MODE_HOSTNAME)) {
- String dn = buildDistinguishedName(InetAddress.getLocalHost().getHostName());
- cert = X509CertificateUtil.generateCertificate(dn, KPair, 365, "SHA1withRSA");
- }
- else {
- String dn = buildDistinguishedName(hostname);
- cert = X509CertificateUtil.generateCertificate(dn, KPair, 365, "SHA1withRSA");
- }
+ keyPairGenerator = KeyPairGenerator.getInstance("RSA");
+ keyPairGenerator.initialize(2048);
+ KeyPair KPair = keyPairGenerator.generateKeyPair();
+ if (hostname == null) {
+ hostname = System.getProperty(CERT_GEN_MODE, CERT_GEN_MODE_LOCALHOST);
+ }
+ X509Certificate cert;
+ if(hostname.equals(CERT_GEN_MODE_HOSTNAME)) {
+ String dn = buildDistinguishedName(InetAddress.getLocalHost().getHostName());
+ cert = X509CertificateUtil.generateCertificate(dn, KPair, 365, "SHA1withRSA");
+ }
+ else {
+ String dn = buildDistinguishedName(hostname);
+ cert = X509CertificateUtil.generateCertificate(dn, KPair, 365, "SHA1withRSA");
+ }
- KeyStore privateKS = getKeystoreForGateway();
- privateKS.setKeyEntry(alias, KPair.getPrivate(),
- passphrase,
- new java.security.cert.Certificate[]{cert});
+ KeyStore privateKS = getKeystoreForGateway();
+ privateKS.setKeyEntry(alias, KPair.getPrivate(),
+ passphrase,
+ new java.security.cert.Certificate[]{cert});
- writeKeyStoreToFile(privateKS, Paths.get(config.getIdentityKeystorePath()), getKeyStorePassword(config.getIdentityKeystorePasswordAlias()));
- } catch (GeneralSecurityException | IOException e) {
- LOG.failedToAddSeflSignedCertForGateway( alias, e );
- throw new KeystoreServiceException(e);
- }
- }
- finally {
- writeLock.unlock();
+ writeKeyStoreToFile(privateKS, Paths.get(config.getIdentityKeystorePath()),
+ getKeyStorePassword(config.getIdentityKeystorePasswordAlias()));
+ } catch (GeneralSecurityException | IOException e) {
+ LOG.failedToAddSeflSignedCertForGateway( alias, e );
+ throw new KeystoreServiceException(e);
}
}
@@ -219,74 +202,39 @@ public class DefaultKeystoreService implements KeystoreService, Service {
@Override
public void createCredentialStoreForCluster(String clusterName) throws KeystoreServiceException {
- Path keystoreFilePath = keyStoreDirPath.resolve(clusterName + CREDENTIALS_SUFFIX);
- writeLock.lock();
- try {
- createKeyStore(keystoreFilePath, CREDENTIALS_STORE_TYPE, masterService.getMasterSecret());
- }
- finally {
- writeLock.unlock();
- }
+ createKeyStore(keyStoreDirPath.resolve(clusterName + CREDENTIALS_SUFFIX),
+ CREDENTIALS_STORE_TYPE, masterService.getMasterSecret());
}
@Override
public boolean isCredentialStoreForClusterAvailable(String clusterName) throws KeystoreServiceException {
- boolean rc;
final Path keyStoreFilePath = keyStoreDirPath.resolve(clusterName + CREDENTIALS_SUFFIX);
- readLock.lock();
try {
- try {
- rc = isKeyStoreAvailable(keyStoreFilePath, CREDENTIALS_STORE_TYPE, masterService.getMasterSecret());
- } catch (KeyStoreException | IOException e) {
- throw new KeystoreServiceException(e);
- }
- return rc;
- }
- finally {
- readLock.unlock();
+ return isKeyStoreAvailable(keyStoreFilePath, CREDENTIALS_STORE_TYPE, masterService.getMasterSecret());
+ } catch (KeyStoreException | IOException e) {
+ throw new KeystoreServiceException(e);
}
}
@Override
public boolean isKeystoreForGatewayAvailable() throws KeystoreServiceException {
final Path keyStoreFilePath = Paths.get(config.getIdentityKeystorePath());
- readLock.lock();
try {
- return isKeyStoreAvailable(keyStoreFilePath, config.getIdentityKeystoreType(), getKeyStorePassword(config.getIdentityKeystorePasswordAlias()));
+ return isKeyStoreAvailable(keyStoreFilePath, config.getIdentityKeystoreType(),
+ getKeyStorePassword(config.getIdentityKeystorePasswordAlias()));
} catch (KeyStoreException | IOException e) {
throw new KeystoreServiceException(e);
- } finally {
- readLock.unlock();
}
}
@Override
- public Key getKeyForGateway(String alias, char[] passphrase) throws KeystoreServiceException {
- Key key = null;
- readLock.lock();
- try {
- KeyStore ks = getKeystoreForGateway();
- if (passphrase == null) {
- passphrase = masterService.getMasterSecret();
- LOG.assumingKeyPassphraseIsMaster();
- }
- if (ks != null) {
- try {
- key = ks.getKey(alias, passphrase);
- } catch (UnrecoverableKeyException | NoSuchAlgorithmException | KeyStoreException e) {
- LOG.failedToGetKeyForGateway( alias, e );
- }
- }
- return key;
- }
- finally {
- readLock.unlock();
- }
+ public Key getKeyForGateway(char[] passphrase) throws KeystoreServiceException {
+ return getKeyForGateway(config.getIdentityKeyAlias(), passphrase);
}
@Override
- public Key getKeyForGateway(char[] passphrase) throws KeystoreServiceException {
- return getKeyForGateway(config.getIdentityKeyAlias(), passphrase);
+ public Key getKeyForGateway(String alias, char[] passphrase) throws KeystoreServiceException {
+ return getKeyFromKeystore(getKeystoreForGateway(), alias, passphrase);
}
@Override
@@ -302,26 +250,23 @@ public class DefaultKeystoreService implements KeystoreService, Service {
@Override
public Key getSigningKey(String keystoreName, String alias, char[] passphrase) throws KeystoreServiceException {
+ return getKeyFromKeystore(getSigningKeystore(keystoreName), alias, passphrase);
+ }
+
+ private Key getKeyFromKeystore(KeyStore ks, String alias, char[] passphrase) {
Key key = null;
- readLock.lock();
- try {
- KeyStore ks = getSigningKeystore(keystoreName);
- if (passphrase == null) {
- passphrase = masterService.getMasterSecret();
- LOG.assumingKeyPassphraseIsMaster();
- }
- if (ks != null) {
- try {
- key = ks.getKey(alias, passphrase);
- } catch (UnrecoverableKeyException | NoSuchAlgorithmException | KeyStoreException e) {
- LOG.failedToGetKeyForGateway( alias, e );
- }
- }
- return key;
+ if (passphrase == null) {
+ passphrase = masterService.getMasterSecret();
+ LOG.assumingKeyPassphraseIsMaster();
}
- finally {
- readLock.unlock();
+ if (ks != null) {
+ try {
+ key = ks.getKey(alias, passphrase);
+ } catch (UnrecoverableKeyException | NoSuchAlgorithmException | KeyStoreException e) {
+ LOG.failedToGetKeyForGateway( alias, e );
+ }
}
+ return key;
}
@Override
@@ -329,75 +274,68 @@ public class DefaultKeystoreService implements KeystoreService, Service {
throws KeystoreServiceException {
// Do not fail getting the credential store if the keystore file does not exist. The returned
// KeyStore will be empty. This seems like a potential bug, but is the behavior before KNOX-1812
- return getKeystore(keyStoreDirPath.resolve(clusterName + CREDENTIALS_SUFFIX), CREDENTIALS_STORE_TYPE, null, false);
+ return getKeystore(keyStoreDirPath.resolve(clusterName + CREDENTIALS_SUFFIX),
+ CREDENTIALS_STORE_TYPE, null, false);
}
@Override
public void addCredentialForCluster(String clusterName, String alias, String value)
throws KeystoreServiceException {
- writeLock.lock();
- try {
- removeFromCache(clusterName, alias);
- KeyStore ks = getCredentialStoreForCluster(clusterName);
- addCredential(alias, value, ks);
- final Path keyStoreFilePath = keyStoreDirPath.resolve(clusterName + CREDENTIALS_SUFFIX);
+ removeFromCache(clusterName, alias);
+ KeyStore ks = getCredentialStoreForCluster(clusterName);
+ if (ks != null) {
try {
+ final Key key = new SecretKeySpec(value.getBytes(StandardCharsets.UTF_8), "AES");
+ ks.setKeyEntry(alias, key, masterService.getMasterSecret(), null);
+
+ final Path keyStoreFilePath = keyStoreDirPath.resolve(clusterName + CREDENTIALS_SUFFIX);
writeKeyStoreToFile(ks, keyStoreFilePath, masterService.getMasterSecret());
} catch (KeyStoreException | IOException | CertificateException | NoSuchAlgorithmException e) {
- LOG.failedToAddCredentialForCluster( clusterName, e );
+ LOG.failedToAddCredentialForCluster(clusterName, e);
}
- } finally {
- writeLock.unlock();
}
}
@Override
public char[] getCredentialForCluster(String clusterName, String alias)
throws KeystoreServiceException {
- char[] credential;
- readLock.lock();
- try {
- credential = checkCache(clusterName, alias);
- if (credential == null) {
- KeyStore ks = getCredentialStoreForCluster(clusterName);
- if (ks != null) {
- try {
- char[] masterSecret = masterService.getMasterSecret();
- Key credentialKey = ks.getKey( alias, masterSecret );
- if (credentialKey != null) {
- byte[] credentialBytes = credentialKey.getEncoded();
- String credentialString = new String( credentialBytes, StandardCharsets.UTF_8 );
- credential = credentialString.toCharArray();
- addToCache(clusterName, alias, credentialString);
- }
- } catch (UnrecoverableKeyException | NoSuchAlgorithmException | KeyStoreException e) {
- LOG.failedToGetCredentialForCluster( clusterName, e );
+ char[] credential = checkCache(clusterName, alias);
+ if (credential == null) {
+ KeyStore ks = getCredentialStoreForCluster(clusterName);
+ if (ks != null) {
+ try {
+ char[] masterSecret = masterService.getMasterSecret();
+ Key credentialKey = ks.getKey( alias, masterSecret );
+ if (credentialKey != null) {
+ byte[] credentialBytes = credentialKey.getEncoded();
+ String credentialString = new String( credentialBytes, StandardCharsets.UTF_8 );
+ credential = credentialString.toCharArray();
+ addToCache(clusterName, alias, credentialString);
}
-
+ } catch (UnrecoverableKeyException | NoSuchAlgorithmException | KeyStoreException e) {
+ LOG.failedToGetCredentialForCluster( clusterName, e );
}
+
}
- return credential;
- }
- finally {
- readLock.unlock();
}
+ return credential;
}
@Override
public void removeCredentialForCluster(String clusterName, String alias) throws KeystoreServiceException {
- final Path keyStoreFilePath = keyStoreDirPath.resolve(clusterName + CREDENTIALS_SUFFIX);
- writeLock.lock();
- try {
- removeFromCache(clusterName, alias);
- KeyStore ks = getCredentialStoreForCluster(clusterName);
- removeCredential(alias, ks);
+ removeFromCache(clusterName, alias);
+ KeyStore ks = getCredentialStoreForCluster(clusterName);
+ if (ks != null) {
try {
+ if (ks.containsAlias(alias)) {
+ ks.deleteEntry(alias);
+ }
+
+ final Path keyStoreFilePath = keyStoreDirPath.resolve(clusterName + CREDENTIALS_SUFFIX);
writeKeyStoreToFile(ks, keyStoreFilePath, masterService.getMasterSecret());
} catch (KeyStoreException | IOException | CertificateException | NoSuchAlgorithmException e) {
LOG.failedToRemoveCredentialForCluster(clusterName, e);
}
- } finally {
- writeLock.unlock();
}
}
@@ -460,8 +398,9 @@ public class DefaultKeystoreService implements KeystoreService, Service {
* @return a {@link KeyStore}, or <code>null</code> if the requested keystore cannot be created
* @throws KeystoreServiceException if an error occurs loading the keystore file
*/
- private KeyStore getKeystore(Path keystorePath, String keystoreType, String alias, boolean failIfNotAccessible) throws KeystoreServiceException {
-
+ private synchronized KeyStore getKeystore(Path keystorePath, String keystoreType, String alias,
+ boolean failIfNotAccessible)
+ throws KeystoreServiceException {
if (failIfNotAccessible) {
if (Files.notExists(keystorePath)) {
LOG.keystoreFileDoesNotExist(keystorePath.toString());
@@ -475,16 +414,15 @@ public class DefaultKeystoreService implements KeystoreService, Service {
}
}
- readLock.lock();
- try {
- return loadKeyStore(keystorePath, keystoreType, getKeyStorePassword(alias));
- } finally {
- readLock.unlock();
- }
+ return loadKeyStore(keystorePath, keystoreType, getKeyStorePassword(alias));
}
- private boolean isKeyStoreAvailable(final Path keyStoreFilePath, String storeType, char[] password) throws KeyStoreException, IOException {
- if (Files.exists(keyStoreFilePath)) {
+ private synchronized boolean isKeyStoreAvailable(final Path keyStoreFilePath, String storeType,
+ char[] password)
+ throws KeyStoreException, IOException {
+ if (Files.exists(keyStoreFilePath) &&
+ Files.isRegularFile(keyStoreFilePath) &&
+ Files.isReadable(keyStoreFilePath)) {
try (InputStream input = Files.newInputStream(keyStoreFilePath)) {
final KeyStore keyStore = KeyStore.getInstance(storeType);
keyStore.load(input, password);
@@ -500,7 +438,9 @@ public class DefaultKeystoreService implements KeystoreService, Service {
}
// Package private for unit test access
- KeyStore createKeyStore(Path keystoreFilePath, String keystoreType, char[] password) throws KeystoreServiceException {
+ // We need this to be synchronized to prevent multiple threads from using at once
+ synchronized KeyStore createKeyStore(Path keystoreFilePath, String keystoreType, char[] password)
+ throws KeystoreServiceException {
if (Files.notExists(keystoreFilePath)) {
// Ensure the parent directory exists...
try {
@@ -513,10 +453,10 @@ public class DefaultKeystoreService implements KeystoreService, Service {
}
}
- try (OutputStream out = Files.newOutputStream(keystoreFilePath)) {
+ try {
KeyStore ks = KeyStore.getInstance(keystoreType);
ks.load(null, null);
- ks.store(out, password);
+ writeKeyStoreToFile(ks, keystoreFilePath, password);
return ks;
} catch (NoSuchAlgorithmException | CertificateException | KeyStoreException | IOException e) {
LOG.failedToCreateKeystore(keystoreFilePath.toString(), keystoreType, e);
@@ -524,31 +464,9 @@ public class DefaultKeystoreService implements KeystoreService, Service {
}
}
- private void addCredential(String alias, String value, KeyStore ks) {
- if (ks != null) {
- try {
- final Key key = new SecretKeySpec(value.getBytes(StandardCharsets.UTF_8), "AES");
- ks.setKeyEntry(alias, key, masterService.getMasterSecret(), null);
- } catch (KeyStoreException e) {
- LOG.failedToAddCredential(e);
- }
- }
- }
-
- private void removeCredential(String alias, KeyStore ks) {
- if (ks != null) {
- try {
- if (ks.containsAlias(alias)) {
- ks.deleteEntry(alias);
- }
- } catch (KeyStoreException e) {
- LOG.failedToRemoveCredential(e);
- }
- }
- }
-
// Package private for unit test access
- KeyStore loadKeyStore(final Path keyStoreFilePath, final String storeType, final char[] password) throws KeystoreServiceException {
+ synchronized KeyStore loadKeyStore(final Path keyStoreFilePath, final String storeType,
+ final char[] password) throws KeystoreServiceException {
try {
final KeyStore keyStore = KeyStore.getInstance(storeType);
@@ -568,11 +486,15 @@ public class DefaultKeystoreService implements KeystoreService, Service {
}
// Package private for unit test access
- void writeKeyStoreToFile(final KeyStore keyStore, final Path path, char[] password)
+ synchronized void writeKeyStoreToFile(final KeyStore keyStore, final Path path, char[] password)
throws KeyStoreException, IOException, NoSuchAlgorithmException, CertificateException {
// TODO: backup the keystore on disk before attempting a write and restore on failure
- try (OutputStream out = Files.newOutputStream(path)) {
- keyStore.store(out, password);
+ try (FileChannel fileChannel = FileChannel.open(path, StandardOpenOption.WRITE,
+ StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING)) {
+ fileChannel.lock();
+ try (OutputStream out = Channels.newOutputStream(fileChannel)) {
+ keyStore.store(out, password);
+ }
}
}