knox-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From kris...@apache.org
Subject [knox] branch master updated: KNOX-1881 - DefaultKeystoreService should use Java NIO API locking as well
Date Fri, 21 Jun 2019 19:05:45 GMT
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 <krisden@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 <krisden@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);
+      }
     }
   }
 


Mime
View raw message