[GitHub] risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox Gateway TLS Keystore and Alias Should be Configurable
risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox
Gateway TLS Keystore and Alias Should be Configurable
URL: https://github.com/apache/knox/pull/54#discussion_r258111920
##
File path:
gateway-server/src/test/java/org/apache/knox/gateway/services/security/impl/DefaultKeystoreServiceTest.java
##
@@ -0,0 +1,384 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.services.security.impl;
+
+import static
org.apache.knox.gateway.config.GatewayConfig.IDENTITY_KEYSTORE_PASSWORD_ALIAS;
+import static
org.apache.knox.gateway.config.GatewayConfig.IDENTITY_KEYSTORE_PATH;
+import static
org.apache.knox.gateway.config.GatewayConfig.IDENTITY_KEYSTORE_TYPE;
+import static org.apache.knox.gateway.config.GatewayConfig.IDENTITY_KEY_ALIAS;
+import static
org.apache.knox.gateway.config.GatewayConfig.IDENTITY_KEY_PASSPHRASE_ALIAS;
+import static
org.apache.knox.gateway.config.GatewayConfig.SIGNING_KEYSTORE_NAME;
+import static org.apache.knox.gateway.config.GatewayConfig.SIGNING_KEY_ALIAS;
+import static org.easymock.EasyMock.eq;
+import static org.easymock.EasyMock.expect;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Paths;
+import java.security.Key;
+import java.security.KeyPair;
+import java.security.KeyPairGenerator;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.util.Collections;
+import java.util.Locale;
+
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.config.impl.GatewayConfigImpl;
+import org.apache.knox.gateway.services.ServiceLifecycleException;
+import org.apache.knox.gateway.services.security.KeystoreService;
+import org.apache.knox.gateway.services.security.KeystoreServiceException;
+import org.apache.knox.gateway.services.security.MasterService;
+import org.easymock.EasyMockSupport;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+public class DefaultKeystoreServiceTest extends EasyMockSupport {
Review comment:
Don't need to extend `EasyMockSupport`
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox Gateway TLS Keystore and Alias Should be Configurable
risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox
Gateway TLS Keystore and Alias Should be Configurable
URL: https://github.com/apache/knox/pull/54#discussion_r258109073
##
File path:
gateway-server/src/test/java/org/apache/knox/gateway/util/KnoxCLITest.java
##
@@ -54,12 +55,14 @@
/**
* @author larry
- *
*/
public class KnoxCLITest {
Review comment:
These changes don't look tied to the PR itself. Would be good to separate.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox Gateway TLS Keystore and Alias Should be Configurable
risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox
Gateway TLS Keystore and Alias Should be Configurable
URL: https://github.com/apache/knox/pull/54#discussion_r258104585
##
File path:
gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java
##
@@ -426,6 +426,11 @@ public String getGatewaySecurityDir() {
return get(SECURITY_DIR, getGatewayDataDir() + File.separator +
DEFAULT_SECURITY_DIR);
}
+ @Override
+ public String getGatewayKeystoreDir() {
+return new File(getGatewaySecurityDir(), "keystores").getAbsolutePath();
Review comment:
Can use `Paths.get` here
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox Gateway TLS Keystore and Alias Should be Configurable
risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox
Gateway TLS Keystore and Alias Should be Configurable
URL: https://github.com/apache/knox/pull/54#discussion_r258104823
##
File path:
gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java
##
@@ -643,14 +648,58 @@ public long getGatewayDeploymentsBackupAgeLimit() {
return d;
}
+ @Override
+ public String getIdentityKeystorePath() {
+String keystorePath = get(IDENTITY_KEYSTORE_PATH);
+if(StringUtils.isEmpty(keystorePath)) {
+ keystorePath = getGatewayKeystoreDir() + File.separatorChar +
"gateway.jks";
Review comment:
`Paths.get`
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox Gateway TLS Keystore and Alias Should be Configurable
risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox
Gateway TLS Keystore and Alias Should be Configurable
URL: https://github.com/apache/knox/pull/54#discussion_r258100235
##
File path:
gateway-spi/src/test/java/org/apache/knox/gateway/services/security/impl/X509CertificateUtilTest.java
##
@@ -0,0 +1,162 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.services.security.impl;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+import java.io.BufferedReader;
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.security.KeyPair;
+import java.security.KeyPairGenerator;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateException;
+import java.security.cert.CertificateExpiredException;
+import java.security.cert.CertificateNotYetValidException;
+import java.security.cert.X509Certificate;
+import java.util.Calendar;
+import java.util.Locale;
+import java.util.TimeZone;
+
+public class X509CertificateUtilTest {
Review comment:
Looks like this would be useful outside of this PR
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox Gateway TLS Keystore and Alias Should be Configurable
risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox
Gateway TLS Keystore and Alias Should be Configurable
URL: https://github.com/apache/knox/pull/54#discussion_r258102739
##
File path:
gateway-spi/src/main/java/org/apache/knox/gateway/services/security/impl/BaseKeystoreService.java
##
@@ -39,28 +39,26 @@
import java.security.cert.CertificateEncodingException;
import java.security.cert.CertificateException;
-public class BaseKeystoreService {
+abstract class BaseKeystoreService {
private static GatewaySpiMessages LOG = MessagesFactory.get(
GatewaySpiMessages.class );
- protected MasterService masterService;
- protected String keyStoreDir;
+ private MasterService masterService;
- private static KeyStore loadKeyStore(final File keyStoreFile, final char[]
masterPassword, String storeType)
+ private static KeyStore loadKeyStore(final File keyStoreFile, final char[]
storePassword, String storeType)
throws CertificateException, IOException, KeyStoreException,
NoSuchAlgorithmException {
final KeyStore keyStore = KeyStore.getInstance(storeType);
if ( keyStoreFile.exists() ) {
try (InputStream input =
Files.newInputStream(keyStoreFile.toPath())) {
- keyStore.load( input, masterPassword );
+ keyStore.load( input, storePassword );
}
} else {
- keyStore.load( null, masterPassword );
+ keyStore.load( null, storePassword );
}
return keyStore;
}
- private static OutputStream createKeyStoreFile(String fileName ) throws
IOException {
-File file = new File( fileName );
+ private static OutputStream createKeyStoreFile(File file) throws IOException
{
Review comment:
Since we are changing the method signature anyway, can we go with `Path`
instead of `File`?
`Path` is preferred to `File`.
https://docs.oracle.com/javase/tutorial/essential/io/legacy.html
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox Gateway TLS Keystore and Alias Should be Configurable
risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox
Gateway TLS Keystore and Alias Should be Configurable
URL: https://github.com/apache/knox/pull/54#discussion_r258108015
##
File path:
gateway-server/src/test/java/org/apache/knox/gateway/services/security/impl/DefaultKeystoreServiceTest.java
##
@@ -0,0 +1,384 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.services.security.impl;
+
+import static
org.apache.knox.gateway.config.GatewayConfig.IDENTITY_KEYSTORE_PASSWORD_ALIAS;
+import static
org.apache.knox.gateway.config.GatewayConfig.IDENTITY_KEYSTORE_PATH;
+import static
org.apache.knox.gateway.config.GatewayConfig.IDENTITY_KEYSTORE_TYPE;
+import static org.apache.knox.gateway.config.GatewayConfig.IDENTITY_KEY_ALIAS;
+import static
org.apache.knox.gateway.config.GatewayConfig.IDENTITY_KEY_PASSPHRASE_ALIAS;
+import static
org.apache.knox.gateway.config.GatewayConfig.SIGNING_KEYSTORE_NAME;
+import static org.apache.knox.gateway.config.GatewayConfig.SIGNING_KEY_ALIAS;
+import static org.easymock.EasyMock.eq;
+import static org.easymock.EasyMock.expect;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Paths;
+import java.security.Key;
+import java.security.KeyPair;
+import java.security.KeyPairGenerator;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.util.Collections;
+import java.util.Locale;
+
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.config.impl.GatewayConfigImpl;
+import org.apache.knox.gateway.services.ServiceLifecycleException;
+import org.apache.knox.gateway.services.security.KeystoreService;
+import org.apache.knox.gateway.services.security.KeystoreServiceException;
+import org.apache.knox.gateway.services.security.MasterService;
+import org.easymock.EasyMockSupport;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+public class DefaultKeystoreServiceTest extends EasyMockSupport {
Review comment:
Looks like this would be useful outside of the PR
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox Gateway TLS Keystore and Alias Should be Configurable
risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox
Gateway TLS Keystore and Alias Should be Configurable
URL: https://github.com/apache/knox/pull/54#discussion_r258103226
##
File path:
gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/JettySSLService.java
##
@@ -159,21 +159,36 @@ private void logAndValidateCertificate() throws
ServiceLifecycleException {
throw new ServiceLifecycleException("Gateway SSL Certificate is not
yet valid. Server will not start.", e);
}
} else {
-throw new ServiceLifecycleException("Public certificate for the
gateway cannot be found with the alias gateway-identity. Plase check the
identity certificate alias.");
+throw new ServiceLifecycleException("Public certificate for the
gateway cannot be found. Please check the identity certificate alias.");
}
} else {
throw new ServiceLifecycleException("Public certificate for the gateway
is not of the expected type of X509Certificate. Something is wrong with the
gateway keystore.");
}
}
@Override
- public Object buildSslContextFactory(String keystoreFileName ) throws
KeyStoreException, IOException, CertificateException, NoSuchAlgorithmException {
+ public Object buildSslContextFactory(GatewayConfig gatewayConfig) throws
KeyStoreException, IOException, CertificateException, NoSuchAlgorithmException {
+char[] master = ms.getMasterSecret();
Review comment:
move this to line 188?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox Gateway TLS Keystore and Alias Should be Configurable
risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox
Gateway TLS Keystore and Alias Should be Configurable
URL: https://github.com/apache/knox/pull/54#discussion_r258100423
##
File path:
gateway-spi/src/test/java/org/apache/knox/gateway/services/security/impl/BaseKeystoreServiceTest.java
##
@@ -0,0 +1,240 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.services.security.impl;
+
+import static org.easymock.EasyMock.expect;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotSame;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.services.security.KeystoreServiceException;
+import org.apache.knox.gateway.services.security.MasterService;
+import org.easymock.EasyMockSupport;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+import java.io.BufferedReader;
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.security.KeyPairGenerator;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateEncodingException;
+import java.security.cert.CertificateException;
+import java.util.Locale;
+
+public class BaseKeystoreServiceTest extends EasyMockSupport {
Review comment:
Looks like this would be useful outside of this PR
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox Gateway TLS Keystore and Alias Should be Configurable
risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox
Gateway TLS Keystore and Alias Should be Configurable
URL: https://github.com/apache/knox/pull/54#discussion_r258097404
##
File path:
gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/DefaultKeystoreService.java
##
@@ -278,22 +262,14 @@ public boolean isKeystoreForGatewayAvailable() throws
KeystoreServiceException {
@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 );
-}
+ try {
Review comment:
nested try/catch block not needed
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox Gateway TLS Keystore and Alias Should be Configurable
risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox
Gateway TLS Keystore and Alias Should be Configurable
URL: https://github.com/apache/knox/pull/54#discussion_r258104177
##
File path:
gateway-provider-security-pac4j/src/main/java/org/apache/knox/gateway/pac4j/Pac4jMessages.java
##
@@ -47,4 +47,8 @@
@Message( level = MessageLevel.INFO, text =
"No private key passphrase alias found. Defaulting to master. Exception
encountered: {0}")
void noPrivateKeyPasshraseProvisioned(Exception e);
+
+ @Message( level = MessageLevel.ERROR, text =
+ "No keystore password alias found. Defaulting to master. Exception
encountered: {0}")
Review comment:
master secret
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox Gateway TLS Keystore and Alias Should be Configurable
risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox
Gateway TLS Keystore and Alias Should be Configurable
URL: https://github.com/apache/knox/pull/54#discussion_r258100857
##
File path:
gateway-spi/src/test/java/org/apache/knox/gateway/services/security/impl/X509CertificateUtilTest.java
##
@@ -0,0 +1,162 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.services.security.impl;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+import java.io.BufferedReader;
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.security.KeyPair;
+import java.security.KeyPairGenerator;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateException;
+import java.security.cert.CertificateExpiredException;
+import java.security.cert.CertificateNotYetValidException;
+import java.security.cert.X509Certificate;
+import java.util.Calendar;
+import java.util.Locale;
+import java.util.TimeZone;
+
+public class X509CertificateUtilTest {
+ @Rule
+ public TemporaryFolder testFolder = new TemporaryFolder();
+
+ static X509Certificate certificate;
+
+ @BeforeClass
+ public static void createCertificate() throws NoSuchAlgorithmException {
+KeyPairGenerator keyPairGenerator = KeyPairGenerator.getInstance("RSA");
+keyPairGenerator.initialize(2048);
+KeyPair keyPair = keyPairGenerator.generateKeyPair();
+String dn = "cn=test,ou=hadoop";
+
+certificate = X509CertificateUtil.generateCertificate(dn, keyPair, 365,
"SHA1withRSA");
+ }
+
+ @Test
+ public void testGenerateCertificate() throws Exception {
+String expectedDn = "CN=test, OU=hadoop";
+
+assertEquals(expectedDn, certificate.getIssuerDN().getName());
+assertEquals(expectedDn, certificate.getSubjectDN().getName());
+assertEquals("SHA1withRSA", certificate.getSigAlgName());
+
+certificate.checkValidity();
+ }
+
+ @Test(expected = CertificateNotYetValidException.class)
+ public void testGenerateCertificateValidityPeriodBefore() throws Exception {
+Calendar calendar = Calendar.getInstance(TimeZone.getDefault(),
Locale.ROOT);
+calendar.add(Calendar.DAY_OF_YEAR, -1);
+certificate.checkValidity(calendar.getTime());
+ }
+
+ @Test(expected = CertificateExpiredException.class)
+ public void testGenerateCertificateValidityPeriodAfter() throws Exception {
+Calendar calendar = Calendar.getInstance(TimeZone.getDefault(),
Locale.ROOT);
+calendar.add(Calendar.DAY_OF_YEAR, 365);
+certificate.checkValidity(calendar.getTime());
+ }
+
+ @Test
+ public void testWriteCertificateToFile() throws Exception {
+File file = testFolder.newFile();
+assertTrue(file.delete());
+
+assertFalse(file.exists());
+X509CertificateUtil.writeCertificateToFile(certificate, file);
+assertTrue(file.exists());
+
+BufferedReader fileReader = Files.newBufferedReader(file.toPath(),
StandardCharsets.UTF_8);
Review comment:
Make sure this gets closed
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox Gateway TLS Keystore and Alias Should be Configurable
risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox
Gateway TLS Keystore and Alias Should be Configurable
URL: https://github.com/apache/knox/pull/54#discussion_r258101064
##
File path:
gateway-test/src/test/java/org/apache/knox/gateway/SimpleDescriptorHandlerFuncTest.java
##
@@ -50,6 +51,9 @@
import static org.junit.Assert.fail;
public class SimpleDescriptorHandlerFuncTest {
+ @Rule
Review comment:
Useful but change should be outside of this PR
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox Gateway TLS Keystore and Alias Should be Configurable
risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox
Gateway TLS Keystore and Alias Should be Configurable
URL: https://github.com/apache/knox/pull/54#discussion_r258101889
##
File path:
gateway-spi/src/main/java/org/apache/knox/gateway/services/security/impl/CMFKeystoreService.java
##
@@ -41,26 +41,24 @@
private static final String TEST_CERT_DN =
"CN=hadoop,OU=Test,O=Hadoop,L=Test,ST=Test,C=US";
private static final String CREDENTIALS_SUFFIX = "-credentials.jceks";
- private String serviceName;
+ private final String serviceName;
+ private final File keyStoreDir;
public CMFKeystoreService(String keystoreDir, String serviceName)
throws ServiceLifecycleException {
this.serviceName = serviceName;
-this.keyStoreDir = keystoreDir + File.separator;
-File ksd = new File(this.keyStoreDir);
-if (!ksd.exists() && !ksd.mkdirs()) {
+this.keyStoreDir = new File(keystoreDir);
+if (!this.keyStoreDir.exists() && !this.keyStoreDir.mkdirs()) {
throw new ServiceLifecycleException("Cannot create the keystore
directory");
}
}
public void createKeystore() throws KeystoreServiceException {
-String filename = keyStoreDir + serviceName + ".jks";
-createKeystore(filename, "JKS");
+createKeystore(new File(keyStoreDir, serviceName + ".jks"), "JKS",
getMasterSecret());
Review comment:
you have keystore type defined elsewhere. Use it here instead of "JKS"?
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox Gateway TLS Keystore and Alias Should be Configurable
risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox
Gateway TLS Keystore and Alias Should be Configurable
URL: https://github.com/apache/knox/pull/54#discussion_r258098677
##
File path:
gateway-spi/src/test/java/org/apache/knox/gateway/services/security/impl/BaseKeystoreServiceTest.java
##
@@ -0,0 +1,240 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.knox.gateway.services.security.impl;
+
+import static org.easymock.EasyMock.expect;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotSame;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import org.apache.knox.gateway.config.GatewayConfig;
+import org.apache.knox.gateway.services.security.KeystoreServiceException;
+import org.apache.knox.gateway.services.security.MasterService;
+import org.easymock.EasyMockSupport;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TemporaryFolder;
+
+import java.io.BufferedReader;
+import java.io.File;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.security.KeyPairGenerator;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.NoSuchAlgorithmException;
+import java.security.UnrecoverableKeyException;
+import java.security.cert.Certificate;
+import java.security.cert.CertificateEncodingException;
+import java.security.cert.CertificateException;
+import java.util.Locale;
+
+public class BaseKeystoreServiceTest extends EasyMockSupport {
+
+ @Rule
+ public TemporaryFolder testFolder = new TemporaryFolder();
+
+ @Test(expected = KeystoreServiceException.class)
+ public void testCreateKeystoreWithBadType() throws IOException,
KeystoreServiceException {
+BaseKeystoreService baseKeystoreService =
createMockBuilder(BaseKeystoreService.class).createMock();
+baseKeystoreService.createKeystore(testFolder.newFile(), "INVALID_TYPE",
"password".toCharArray());
+ }
+
+ @Test(expected = IllegalArgumentException.class)
+ public void testCreateKeystoreWithNullPassword() throws IOException,
KeystoreServiceException {
+BaseKeystoreService baseKeystoreService =
createMockBuilder(BaseKeystoreService.class).createMock();
+baseKeystoreService.createKeystore(testFolder.newFile(), "JKS", null);
+ }
+
+ @Test
+ public void testCreateGetAndCheckKeystore() throws IOException,
KeystoreServiceException, KeyStoreException {
+BaseKeystoreService baseKeystoreService =
createMockBuilder(BaseKeystoreService.class).createMock();
+
+// Test the popular keystore types...
+for (String keystoreType : new String[]{"jks", "jceks", "pkcs12"}) {
+ testCreateGetAndCheckKeystore(baseKeystoreService, keystoreType);
+}
+ }
+
+ @Test
+ public void testCreateGetAndRemoveCredential() throws Exception {
+BaseKeystoreService baseKeystoreService =
createMockBuilder(BaseKeystoreService.class).createMock();
+
+// This appears to only work for JCEKS keystores.
+testCreateGetAndRemoveCredential(baseKeystoreService, "jceks");
+ }
+
+ @Test
+ public void testWriteCertificateToFile() throws IOException,
NoSuchAlgorithmException, CertificateEncodingException {
+BaseKeystoreService baseKeystoreService =
createMockBuilder(BaseKeystoreService.class).createMock();
+
+File file = testFolder.newFile();
+Certificate outCertificate = createCertificate();
+baseKeystoreService.writeCertificateToFile(outCertificate, file);
+
+assertTrue(file.exists());
+
+BufferedReader fileReader = Files.newBufferedReader(file.toPath(),
StandardCharsets.UTF_8);
Review comment:
use try-with-resources to ensure closed
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox Gateway TLS Keystore and Alias Should be Configurable
risdenk commented on a change in pull request #54: [WIP] KNOX-1756 - Knox
Gateway TLS Keystore and Alias Should be Configurable
URL: https://github.com/apache/knox/pull/54#discussion_r258097302
##
File path:
gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/DefaultKeystoreService.java
##
@@ -260,16 +247,13 @@ public boolean
isCredentialStoreForClusterAvailable(String clusterName) throws K
@Override
public boolean isKeystoreForGatewayAvailable() throws
KeystoreServiceException {
-boolean rc;
-final File keyStoreFile = new File( keyStoreDir + GATEWAY_KEYSTORE );
readLock.lock();
try {
try {
Review comment:
Don't need nested try/catch block
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
