This is an automated email from the ASF dual-hosted git repository. johnnyv pushed a commit to branch FTPSERVER-491 in repository https://gitbox.apache.org/repos/asf/mina-ftpserver.git
The following commit(s) were added to refs/heads/FTPSERVER-491 by this push: new 36cd856 adds NULL checks for SslConfigurationFactory 36cd856 is described below commit 36cd8561dd198057773454cbc857a8fa26ef81a5 Author: johnnyv <john...@apache.org> AuthorDate: Wed Nov 6 20:30:54 2019 -0500 adds NULL checks for SslConfigurationFactory --- .../ftpserver/ssl/SslConfigurationFactory.java | 259 ++++++++++----------- .../apache/ftpserver/ssl/MinaCipherSuitesTest.java | 52 ++--- 2 files changed, 144 insertions(+), 167 deletions(-) diff --git a/core/src/main/java/org/apache/ftpserver/ssl/SslConfigurationFactory.java b/core/src/main/java/org/apache/ftpserver/ssl/SslConfigurationFactory.java index c36ca3b..a590148 100644 --- a/core/src/main/java/org/apache/ftpserver/ssl/SslConfigurationFactory.java +++ b/core/src/main/java/org/apache/ftpserver/ssl/SslConfigurationFactory.java @@ -36,15 +36,13 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; /** - * Used to configure the SSL settings for the control channel or the data - * channel. + * Used to configure the SSL settings for the control channel or the data channel. * * @author <a href="http://mina.apache.org">Apache MINA Project</a> */ public class SslConfigurationFactory { - private final Logger LOG = LoggerFactory - .getLogger(SslConfigurationFactory.class); + private final Logger LOG = LoggerFactory.getLogger(SslConfigurationFactory.class); private File keystoreFile = new File("./res/.keystore"); @@ -78,7 +76,7 @@ public class SslConfigurationFactory { * @return The key store file */ public File getKeystoreFile() { - return keystoreFile; + return keystoreFile; } /** @@ -88,7 +86,9 @@ public class SslConfigurationFactory { * A path to an existing key store file */ public void setKeystoreFile(File keyStoreFile) { - this.keystoreFile = keyStoreFile; + if (keyStoreFile == null || keyStoreFile.length() == 0) + throw new FtpServerConfigurationException("KeystoreFile must not be null or zero length"); + this.keystoreFile = keyStoreFile; } /** @@ -97,7 +97,7 @@ public class SslConfigurationFactory { * @return The password */ public String getKeystorePassword() { - return keystorePass; + return keystorePass; } /** @@ -107,7 +107,7 @@ public class SslConfigurationFactory { * The password */ public void setKeystorePassword(String keystorePass) { - this.keystorePass = keystorePass; + this.keystorePass = keystorePass; } /** @@ -116,7 +116,7 @@ public class SslConfigurationFactory { * @return The key store type */ public String getKeystoreType() { - return keystoreType; + return keystoreType; } /** @@ -126,7 +126,9 @@ public class SslConfigurationFactory { * The key store type */ public void setKeystoreType(String keystoreType) { - this.keystoreType = keystoreType; + if (keystoreType == null || keystoreType.length() == 0) + throw new FtpServerConfigurationException("KeystoreType must not be null or zero length"); + this.keystoreType = keystoreType; } /** @@ -135,7 +137,7 @@ public class SslConfigurationFactory { * @return The key store algorithm */ public String getKeystoreAlgorithm() { - return keystoreAlgorithm; + return keystoreAlgorithm; } /** @@ -145,49 +147,48 @@ public class SslConfigurationFactory { * The key store algorithm */ public void setKeystoreAlgorithm(String keystoreAlgorithm) { - this.keystoreAlgorithm = keystoreAlgorithm; + if (keystoreAlgorithm == null || keystoreAlgorithm.length() == 0) + throw new FtpServerConfigurationException("KeystoreAlgorithm must not be null or zero length"); + this.keystoreAlgorithm = keystoreAlgorithm; } /** - * The SSL protocol used for this channel. Supported values are "SSL" and - * "TLS". Defaults to "TLS". + * The SSL protocol used for this channel. Supported values are "SSL" and "TLS". Defaults to "TLS". * * @return The SSL protocol */ public String getSslProtocol() { - return sslProtocol; + return sslProtocol; } /** - * Set the SSL protocol used for this channel. Supported values are "SSL" - * and "TLS". Defaults to "TLS". + * Set the SSL protocol used for this channel. Supported values are "SSL" and "TLS". Defaults to "TLS". * * @param sslProtocol * The SSL protocol */ public void setSslProtocol(String sslProtocol) { - this.sslProtocol = sslProtocol; + if (sslProtocol == null || sslProtocol.length() == 0) + throw new FtpServerConfigurationException("SslProcotol must not be null or zero length"); + this.sslProtocol = sslProtocol; } /** - * Set what client authentication level to use, supported values are "yes" - * or "true" for required authentication, "want" for wanted authentication - * and "false" or "none" for no authentication. Defaults to "none". + * Set what client authentication level to use, supported values are "yes" or "true" for required authentication, + * "want" for wanted authentication and "false" or "none" for no authentication. Defaults to "none". * * @param clientAuthReqd * The desired authentication level */ public void setClientAuthentication(String clientAuthReqd) { - if ("true".equalsIgnoreCase(clientAuthReqd) - || "yes".equalsIgnoreCase(clientAuthReqd) - || "need".equalsIgnoreCase(clientAuthReqd)) { - this.clientAuth = ClientAuth.NEED; - } else if ("want".equalsIgnoreCase(clientAuthReqd)) { - this.clientAuth = ClientAuth.WANT; - } else { - this.clientAuth = ClientAuth.NONE; - } + if ("true".equalsIgnoreCase(clientAuthReqd) || "yes".equalsIgnoreCase(clientAuthReqd) || "need".equalsIgnoreCase(clientAuthReqd)) { + this.clientAuth = ClientAuth.NEED; + } else if ("want".equalsIgnoreCase(clientAuthReqd)) { + this.clientAuth = ClientAuth.WANT; + } else { + this.clientAuth = ClientAuth.NONE; + } } /** @@ -196,7 +197,7 @@ public class SslConfigurationFactory { * @return The password */ public String getKeyPassword() { - return keyPass; + return keyPass; } /** @@ -206,15 +207,16 @@ public class SslConfigurationFactory { * The password */ public void setKeyPassword(String keyPass) { - this.keyPass = keyPass; + this.keyPass = keyPass; } /** * Get the file used to load the truststore + * * @return The {@link File} containing the truststore */ public File getTruststoreFile() { - return trustStoreFile; + return trustStoreFile; } /** @@ -224,7 +226,7 @@ public class SslConfigurationFactory { * The password */ public void setTruststoreFile(File trustStoreFile) { - this.trustStoreFile = trustStoreFile; + this.trustStoreFile = trustStoreFile; } /** @@ -233,7 +235,7 @@ public class SslConfigurationFactory { * @return The password */ public String getTruststorePassword() { - return trustStorePass; + return trustStorePass; } /** @@ -243,7 +245,7 @@ public class SslConfigurationFactory { * The password */ public void setTruststorePassword(String trustStorePass) { - this.trustStorePass = trustStorePass; + this.trustStorePass = trustStorePass; } /** @@ -252,7 +254,7 @@ public class SslConfigurationFactory { * @return The trust store type */ public String getTruststoreType() { - return trustStoreType; + return trustStoreType; } /** @@ -262,7 +264,7 @@ public class SslConfigurationFactory { * The trust store type */ public void setTruststoreType(String trustStoreType) { - this.trustStoreType = trustStoreType; + this.trustStoreType = trustStoreType; } /** @@ -271,7 +273,7 @@ public class SslConfigurationFactory { * @return The trust store algorithm */ public String getTruststoreAlgorithm() { - return trustStoreAlgorithm; + return trustStoreAlgorithm; } /** @@ -281,130 +283,111 @@ public class SslConfigurationFactory { * The trust store algorithm */ public void setTruststoreAlgorithm(String trustStoreAlgorithm) { - this.trustStoreAlgorithm = trustStoreAlgorithm; + this.trustStoreAlgorithm = trustStoreAlgorithm; } - private KeyStore loadStore(File storeFile, String storeType, - String storePass) throws IOException, GeneralSecurityException { - InputStream fin = null; - try { - if(storeFile.exists()) { - LOG.debug("Trying to load store from file"); - fin = new FileInputStream(storeFile); - } else { - LOG.debug("Trying to load store from classpath"); - fin = getClass().getClassLoader().getResourceAsStream(storeFile.getPath()); - - if(fin == null) { - throw new FtpServerConfigurationException("Key store could not be loaded from " + storeFile.getPath()); - } - } - - KeyStore store = KeyStore.getInstance(storeType); - store.load(fin, storePass.toCharArray()); - - return store; - } finally { - IoUtils.close(fin); - } + private KeyStore loadStore(File storeFile, String storeType, String storePass) throws IOException, GeneralSecurityException { + InputStream fin = null; + try { + if (storeFile.exists()) { + LOG.debug("Trying to load store from file"); + fin = new FileInputStream(storeFile); + } else { + LOG.debug("Trying to load store from classpath"); + fin = getClass().getClassLoader().getResourceAsStream(storeFile.getPath()); + + if (fin == null) { + throw new FtpServerConfigurationException("Key store could not be loaded from " + storeFile.getPath()); + } + } + + KeyStore store = KeyStore.getInstance(storeType); + store.load(fin, storePass.toCharArray()); + + return store; + } finally { + IoUtils.close(fin); + } } /** - * Create an instance of {@link SslConfiguration} based on the configuration - * of this factory. + * Create an instance of {@link SslConfiguration} based on the configuration of this factory. + * * @return The {@link SslConfiguration} instance */ public SslConfiguration createSslConfiguration() { - try { - // initialize keystore - LOG - .debug( - "Loading key store from \"{}\", using the key store type \"{}\"", - keystoreFile.getAbsolutePath(), keystoreType); - KeyStore keyStore = loadStore(keystoreFile, keystoreType, - keystorePass); - - KeyStore trustStore; - if (trustStoreFile != null) { - LOG - .debug( - "Loading trust store from \"{}\", using the key store type \"{}\"", - trustStoreFile.getAbsolutePath(), - trustStoreType); - trustStore = loadStore(trustStoreFile, trustStoreType, - trustStorePass); - } else { - trustStore = keyStore; - } - - String keyPassToUse; - if (keyPass == null) { - keyPassToUse = keystorePass; - } else { - keyPassToUse = keyPass; - } - // initialize key manager factory - KeyManagerFactory keyManagerFactory = KeyManagerFactory - .getInstance(keystoreAlgorithm); - keyManagerFactory.init(keyStore, keyPassToUse.toCharArray()); - - // initialize trust manager factory - TrustManagerFactory trustManagerFactory = TrustManagerFactory - .getInstance(trustStoreAlgorithm); - trustManagerFactory.init(trustStore); - - return new DefaultSslConfiguration( - keyManagerFactory, trustManagerFactory, - clientAuth, sslProtocol, - enabledCipherSuites, keyAlias); - } catch (Exception ex) { - LOG.error("DefaultSsl.configure()", ex); - throw new FtpServerConfigurationException("DefaultSsl.configure()", - ex); - } + try { + // initialize keystore + LOG.debug("Loading key store from \"{}\", using the key store type \"{}\"", keystoreFile.getAbsolutePath(), keystoreType); + KeyStore keyStore = loadStore(keystoreFile, keystoreType, keystorePass); + + KeyStore trustStore; + if (trustStoreFile != null) { + LOG.debug("Loading trust store from \"{}\", using the key store type \"{}\"", trustStoreFile.getAbsolutePath(), trustStoreType); + trustStore = loadStore(trustStoreFile, trustStoreType, trustStorePass); + } else { + trustStore = keyStore; + } + + String keyPassToUse; + if (keyPass == null) { + keyPassToUse = keystorePass; + } else { + keyPassToUse = keyPass; + } + // initialize key manager factory + KeyManagerFactory keyManagerFactory = KeyManagerFactory.getInstance(keystoreAlgorithm); + keyManagerFactory.init(keyStore, keyPassToUse.toCharArray()); + + // initialize trust manager factory + TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance(trustStoreAlgorithm); + trustManagerFactory.init(trustStore); + + return new DefaultSslConfiguration(keyManagerFactory, trustManagerFactory, clientAuth, sslProtocol, enabledCipherSuites, keyAlias); + } catch (Exception ex) { + LOG.error("DefaultSsl.configure()", ex); + throw new FtpServerConfigurationException("DefaultSsl.configure()", ex); + } } /** * Return the required client authentication setting * - * @return {@link ClientAuth#NEED} if client authentication is required, - * {@link ClientAuth#WANT} is client authentication is wanted or - * {@link ClientAuth#NONE} if no client authentication is the be - * performed + * @return {@link ClientAuth#NEED} if client authentication is required, {@link ClientAuth#WANT} is client + * authentication is wanted or {@link ClientAuth#NONE} if no client authentication is the be performed */ public ClientAuth getClientAuth() { - return clientAuth; + return clientAuth; } /** - * Returns the cipher suites that should be enabled for this connection. - * Must return null if the default (as decided by the JVM) cipher suites - * should be used. + * Returns the cipher suites that should be enabled for this connection. Must return null if the default (as decided + * by the JVM) cipher suites should be used. * * @return An array of cipher suites, or null. */ public String[] getEnabledCipherSuites() { - if (enabledCipherSuites != null) { - return enabledCipherSuites.clone(); - } else { - return null; - } + if (enabledCipherSuites != null) { + return enabledCipherSuites.clone(); + } else { + return null; + } } /** - * Set the allowed cipher suites, note that the exact list of supported - * cipher suites differs between JRE implementations. + * Set the allowed cipher suites, note that the exact list of supported cipher suites differs between JRE + * implementations. * * @param enabledCipherSuites */ public void setEnabledCipherSuites(String[] enabledCipherSuites) { - if (enabledCipherSuites != null) { - this.enabledCipherSuites = enabledCipherSuites.clone(); - } else { - this.enabledCipherSuites = null; - } + if (enabledCipherSuites != null) { + this.enabledCipherSuites = enabledCipherSuites.clone(); + } else { + this.enabledCipherSuites = null; + } } /** @@ -413,19 +396,17 @@ public class SslConfigurationFactory { * @return The alias, or null if none is set */ public String getKeyAlias() { - return keyAlias; + return keyAlias; } /** - * Set the alias for the key to be used for SSL communication. If the - * specified key store contains multiple keys, this alias can be set to - * select a specific key. + * Set the alias for the key to be used for SSL communication. If the specified key store contains multiple keys, + * this alias can be set to select a specific key. * * @param keyAlias - * The alias to use, or null if JSSE should be allowed to choose - * the key. + * The alias to use, or null if JSSE should be allowed to choose the key. */ public void setKeyAlias(String keyAlias) { - this.keyAlias = keyAlias; + this.keyAlias = keyAlias; } } diff --git a/core/src/test/java/org/apache/ftpserver/ssl/MinaCipherSuitesTest.java b/core/src/test/java/org/apache/ftpserver/ssl/MinaCipherSuitesTest.java index cd78b4b..aeda3fa 100644 --- a/core/src/test/java/org/apache/ftpserver/ssl/MinaCipherSuitesTest.java +++ b/core/src/test/java/org/apache/ftpserver/ssl/MinaCipherSuitesTest.java @@ -24,64 +24,60 @@ import javax.net.ssl.SSLHandshakeException; import org.apache.commons.net.ftp.FTPSClient; /** -* -* @author <a href="http://mina.apache.org">Apache MINA Project</a> -* -*/ + * + * @author <a href="http://mina.apache.org">Apache MINA Project</a> + * + */ public class MinaCipherSuitesTest extends SSLTestTemplate { @Override protected String getAuthValue() { - return "TLS"; + return "TLS"; } @Override protected boolean useImplicit() { - return true; + return true; } @Override protected SslConfigurationFactory createSslConfiguration() { - SslConfigurationFactory sslConfigFactory = super.createSslConfiguration(); + SslConfigurationFactory sslConfigFactory = super.createSslConfiguration(); - sslConfigFactory - .setEnabledCipherSuites(new String[] { "SSL_DHE_DSS_WITH_3DES_EDE_CBC_SHA" }); + sslConfigFactory.setEnabledCipherSuites(new String[] { "SSL_DHE_DSS_WITH_3DES_EDE_CBC_SHA" }); - return sslConfigFactory; + return sslConfigFactory; } - + @Override protected FTPSClient createFTPClient() throws Exception { - return new FTPSClient(true); + return new FTPSClient(true); } @Override protected boolean isConnectClient() { - return false; + return false; } /* - * Only certain cipher suites will work with the keys and protocol we're - * using for this test. Two suites known to work is: - * SSL_DHE_DSS_WITH_3DES_EDE_CBC_SHA SSL_DHE_DSS_EXPORT_WITH_DES40_CBC_SHA + * Only certain cipher suites will work with the keys and protocol we're using for this test. Two suites known to + * work is: SSL_DHE_DSS_WITH_3DES_EDE_CBC_SHA SSL_DHE_DSS_EXPORT_WITH_DES40_CBC_SHA */ public void testEnabled() throws Exception { - ((FTPSClient) client) - .setEnabledCipherSuites(new String[] { "SSL_DHE_DSS_WITH_3DES_EDE_CBC_SHA" }); + ((FTPSClient) client).setEnabledCipherSuites(new String[] { "SSL_DHE_DSS_WITH_3DES_EDE_CBC_SHA" }); - connectClient(); + connectClient(); } public void testDisabled() throws Exception { - ((FTPSClient) client) - .setEnabledCipherSuites(new String[] { "SSL_DHE_DSS_EXPORT_WITH_DES40_CBC_SHA" }); - - try { - doConnect(); - fail("Must throw SSLHandshakeException"); - } catch (SSLHandshakeException e) { - // OK - } + ((FTPSClient) client).setEnabledCipherSuites(new String[] { "SSL_DHE_DSS_EXPORT_WITH_DES40_CBC_SHA" }); + + try { + doConnect(); + fail("Must throw SSLHandshakeException"); + } catch (SSLHandshakeException e) { + // OK + } } }