You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2021/11/30 23:04:41 UTC
[GitHub] [ozone] vivekratnavel opened a new pull request #2878: HDDS-6031. Add configs to support externalization of root CA
vivekratnavel opened a new pull request #2878:
URL: https://github.com/apache/ozone/pull/2878
## What changes were proposed in this pull request?
- Add configs to support externalization of root CA
- Introduce a flag to enable external root CA
- Introduce configs to feed in keystore and trust store files for each component
- Update the security bootstrap flow of all the components to honor the flag to enable external root CA
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6031
## How was this patch tested?
This patch was tested by replacing jars in a cluster and updating the configs to work with an external root CA signed certs in JKS and Truststore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] vivekratnavel commented on pull request #2878: HDDS-6031. Add configs to support externalization of root CA
Posted by GitBox <gi...@apache.org>.
vivekratnavel commented on pull request #2878:
URL: https://github.com/apache/ozone/pull/2878#issuecomment-991376602
Hi Hanisha, the existing functions now throw an exception because when custom CA is enabled, the keystore and truststore files need to be parsed to get the key and cert. And these exceptions are thrown all the up to bubble it to the top. Please let me know if you find any unwanted exceptions being thrown.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] vivekratnavel merged pull request #2878: HDDS-6031. Add configs to support externalization of root CA
Posted by GitBox <gi...@apache.org>.
vivekratnavel merged pull request #2878:
URL: https://github.com/apache/ozone/pull/2878
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hanishakoneru commented on pull request #2878: HDDS-6031. Add configs to support externalization of root CA
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on pull request #2878:
URL: https://github.com/apache/ozone/pull/2878#issuecomment-991345993
Thanks @vivekratnavel.
> There are a bunch of functions appended with throws exception. Is this required for subsequent patches?
Can you please confirm this? If not, any reason for adding them? They would be redundant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hanishakoneru commented on pull request #2878: HDDS-6031. Add configs to support externalization of root CA
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on pull request #2878:
URL: https://github.com/apache/ozone/pull/2878#issuecomment-991345993
Thanks @vivekratnavel.
> There are a bunch of functions appended with throws exception. Is this required for subsequent patches?
Can you please confirm this? If not, any reason for adding them? They would be redundant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swagle commented on a change in pull request #2878: HDDS-6031. Add configs to support externalization of root CA
Posted by GitBox <gi...@apache.org>.
swagle commented on a change in pull request #2878:
URL: https://github.com/apache/ozone/pull/2878#discussion_r759843642
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/x509/SecurityConfig.java
##########
@@ -175,6 +194,22 @@ public SecurityConfig(ConfigurationSource configuration) {
this.crlName = this.configuration.get(HDDS_X509_CRL_NAME,
HDDS_X509_CRL_NAME_DEFAULT);
+ this.keystoreFilePath =
+ this.configuration.get(HDDS_CUSTOM_KEYSTORE_FILE_PATH);
+ this.truststoreFilePath =
+ this.configuration.get(HDDS_CUSTOM_TRUSTSTORE_FILE_PATH);
+ try {
+ this.keystoreFilePassword =
+ this.configuration.getPassword(HDDS_CUSTOM_KEYSTORE_FILE_PASSWORD);
+ this.keystoreKeyPassword =
+ this.configuration.getPassword(HDDS_CUSTOM_KEYSTORE_KEY_PASSWORD);
+ this.truststorePassword =
+ this.configuration.getPassword(HDDS_CUSTOM_TRUSTSTORE_PASSWORD);
+ } catch (IOException ioException) {
+ LOG.error("Error while getting custom Keystore / Truststore password.",
Review comment:
I think incorrect security config should crash the application instead of logging an error, re-throwing as runtime exception?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] vivekratnavel commented on a change in pull request #2878: HDDS-6031. Add configs to support externalization of root CA
Posted by GitBox <gi...@apache.org>.
vivekratnavel commented on a change in pull request #2878:
URL: https://github.com/apache/ozone/pull/2878#discussion_r760450936
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/x509/SecurityConfig.java
##########
@@ -175,6 +194,22 @@ public SecurityConfig(ConfigurationSource configuration) {
this.crlName = this.configuration.get(HDDS_X509_CRL_NAME,
HDDS_X509_CRL_NAME_DEFAULT);
+ this.keystoreFilePath =
+ this.configuration.get(HDDS_CUSTOM_KEYSTORE_FILE_PATH);
+ this.truststoreFilePath =
+ this.configuration.get(HDDS_CUSTOM_TRUSTSTORE_FILE_PATH);
+ try {
+ this.keystoreFilePassword =
+ this.configuration.getPassword(HDDS_CUSTOM_KEYSTORE_FILE_PASSWORD);
+ this.keystoreKeyPassword =
+ this.configuration.getPassword(HDDS_CUSTOM_KEYSTORE_KEY_PASSWORD);
+ this.truststorePassword =
+ this.configuration.getPassword(HDDS_CUSTOM_TRUSTSTORE_PASSWORD);
+ } catch (IOException ioException) {
+ LOG.error("Error while getting custom Keystore / Truststore password.",
Review comment:
@swagle, thank you for the review.
A run time exception is not thrown here because we only initialize the configs here. It is thrown when we try to read a custom CA certificate or private key of any component - https://github.com/apache/ozone/pull/2878/files#diff-9bfcb7cd5c8cf13a6f3421c441ea6b70fd476418627c911269d6d43e58c7440bR149
And [loadAllCertificates()](https://github.com/apache/ozone/blob/master/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/certificate/client/DefaultCertificateClient.java#L117) is part of security init of all the components when security is enabled. So, we will catch any misconfigurations in the security bootstrap flow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swagle commented on a change in pull request #2878: HDDS-6031. Add configs to support externalization of root CA
Posted by GitBox <gi...@apache.org>.
swagle commented on a change in pull request #2878:
URL: https://github.com/apache/ozone/pull/2878#discussion_r760528348
##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/security/x509/SecurityConfig.java
##########
@@ -175,6 +194,22 @@ public SecurityConfig(ConfigurationSource configuration) {
this.crlName = this.configuration.get(HDDS_X509_CRL_NAME,
HDDS_X509_CRL_NAME_DEFAULT);
+ this.keystoreFilePath =
+ this.configuration.get(HDDS_CUSTOM_KEYSTORE_FILE_PATH);
+ this.truststoreFilePath =
+ this.configuration.get(HDDS_CUSTOM_TRUSTSTORE_FILE_PATH);
+ try {
+ this.keystoreFilePassword =
+ this.configuration.getPassword(HDDS_CUSTOM_KEYSTORE_FILE_PASSWORD);
+ this.keystoreKeyPassword =
+ this.configuration.getPassword(HDDS_CUSTOM_KEYSTORE_KEY_PASSWORD);
+ this.truststorePassword =
+ this.configuration.getPassword(HDDS_CUSTOM_TRUSTSTORE_PASSWORD);
+ } catch (IOException ioException) {
+ LOG.error("Error while getting custom Keystore / Truststore password.",
Review comment:
Sounds good, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] vivekratnavel commented on pull request #2878: HDDS-6031. Add configs to support externalization of root CA
Posted by GitBox <gi...@apache.org>.
vivekratnavel commented on pull request #2878:
URL: https://github.com/apache/ozone/pull/2878#issuecomment-991376602
Hi Hanisha, the existing functions now throw an exception because when custom CA is enabled, the keystore and truststore files need to be parsed to get the key and cert. And these exceptions are thrown all the up to bubble it to the top. Please let me know if you find any unwanted exceptions being thrown.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swagle commented on a change in pull request #2878: HDDS-6031. Add configs to support externalization of root CA
Posted by GitBox <gi...@apache.org>.
swagle commented on a change in pull request #2878:
URL: https://github.com/apache/ozone/pull/2878#discussion_r760527853
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/keys/KeyCodec.java
##########
@@ -406,4 +431,37 @@ private void checkPreconditions(Path basePath) throws IOException {
}
}
+ /**
+ * Returns the custom private key for custom Root CA setup.
+ * @return PrivateKey.
+ * @throws NoSuchAlgorithmException - On Error.
+ * @throws IOException - On Error.
+ */
+ public PrivateKey readCustomPrivateKey() throws NoSuchAlgorithmException,
+ IOException, UnrecoverableKeyException, CertificateException,
+ KeyStoreException {
+ if (customPrivateKey == null) {
+ loadCustomKeys();
+ }
+
+ return customPrivateKey;
+ }
+
+ /**
+ * Reads a Key from the PEM Encoded Store.
+ *
+ * @param path - Path, Location where the Key file is stored.
+ * @return PrivateKey Object.
+ * @throws IOException - on Error.
+ */
+ protected PKCS8EncodedKeySpec readKey(File path)
Review comment:
Sounds good, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hanishakoneru commented on pull request #2878: HDDS-6031. Add configs to support externalization of root CA
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on pull request #2878:
URL: https://github.com/apache/ozone/pull/2878#issuecomment-992859290
Hi Vivek, DefaultCertificateClient#getPrivateKey() and DefaultCertificateClient#getPublicKey() just log the exception and do not throw any exception. We can remove the throws from these functions and their corresponding super-functions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] hanishakoneru commented on a change in pull request #2878: HDDS-6031. Add configs to support externalization of root CA
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #2878:
URL: https://github.com/apache/ozone/pull/2878#discussion_r764384758
##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/replication/ReplicationServer.java
##########
@@ -62,15 +63,15 @@ public ReplicationServer(
ReplicationConfig replicationConfig,
SecurityConfig secConf,
CertificateClient caClient
- ) {
+ ) throws SCMSecurityException {
this.secConf = secConf;
this.caClient = caClient;
this.controller = controller;
this.port = replicationConfig.getPort();
init();
}
- public void init() {
+ public void init() throws SCMSecurityException {
Review comment:
This function only throws IllegalArgumentException. Any reason for making the function throw SCMSecurityException?
##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManager.java
##########
@@ -356,7 +356,9 @@ private StorageContainerManager(OzoneConfiguration conf,
// Authenticate SCM if security is enabled, this initialization can only
// be done after the metadata store is initialized.
if (OzoneSecurityUtil.isSecurityEnabled(conf)) {
- initializeCAnSecurityProtocol(conf, configurator);
+ if (!securityConfig.isCustomCAEnabled()) {
+ initializeCAnSecurityProtocol(conf, configurator);
+ }
Review comment:
What happens when custom root CA is enabled? Where do we initialize?
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestOzoneConfigurationFields.java
##########
@@ -81,6 +81,12 @@ private void addPropertiesNotInXml() {
HddsConfigKeys.HDDS_KEY_ALGORITHM,
HddsConfigKeys.HDDS_SECURITY_PROVIDER,
HddsConfigKeys.HDDS_X509_CRL_NAME, // HDDS-2873
+ HddsConfigKeys.HDDS_CUSTOM_KEYSTORE_FILE_PASSWORD,
+ HddsConfigKeys.HDDS_CUSTOM_KEYSTORE_FILE_PATH,
+ HddsConfigKeys.HDDS_CUSTOM_ROOT_CA_ENABLED,
+ HddsConfigKeys.HDDS_CUSTOM_KEYSTORE_KEY_PASSWORD,
+ HddsConfigKeys.HDDS_CUSTOM_TRUSTSTORE_PASSWORD,
+ HddsConfigKeys.HDDS_CUSTOM_TRUSTSTORE_FILE_PATH,
Review comment:
Will these configs not be exposed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] vivekratnavel commented on a change in pull request #2878: HDDS-6031. Add configs to support externalization of root CA
Posted by GitBox <gi...@apache.org>.
vivekratnavel commented on a change in pull request #2878:
URL: https://github.com/apache/ozone/pull/2878#discussion_r760451414
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/keys/KeyCodec.java
##########
@@ -406,4 +431,37 @@ private void checkPreconditions(Path basePath) throws IOException {
}
}
+ /**
+ * Returns the custom private key for custom Root CA setup.
+ * @return PrivateKey.
+ * @throws NoSuchAlgorithmException - On Error.
+ * @throws IOException - On Error.
+ */
+ public PrivateKey readCustomPrivateKey() throws NoSuchAlgorithmException,
+ IOException, UnrecoverableKeyException, CertificateException,
+ KeyStoreException {
+ if (customPrivateKey == null) {
+ loadCustomKeys();
+ }
+
+ return customPrivateKey;
+ }
+
+ /**
+ * Reads a Key from the PEM Encoded Store.
+ *
+ * @param path - Path, Location where the Key file is stored.
+ * @return PrivateKey Object.
+ * @throws IOException - on Error.
+ */
+ protected PKCS8EncodedKeySpec readKey(File path)
Review comment:
Sure
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] swagle commented on a change in pull request #2878: HDDS-6031. Add configs to support externalization of root CA
Posted by GitBox <gi...@apache.org>.
swagle commented on a change in pull request #2878:
URL: https://github.com/apache/ozone/pull/2878#discussion_r759844189
##########
File path: hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/security/x509/keys/KeyCodec.java
##########
@@ -406,4 +431,37 @@ private void checkPreconditions(Path basePath) throws IOException {
}
}
+ /**
+ * Returns the custom private key for custom Root CA setup.
+ * @return PrivateKey.
+ * @throws NoSuchAlgorithmException - On Error.
+ * @throws IOException - On Error.
+ */
+ public PrivateKey readCustomPrivateKey() throws NoSuchAlgorithmException,
+ IOException, UnrecoverableKeyException, CertificateException,
+ KeyStoreException {
+ if (customPrivateKey == null) {
+ loadCustomKeys();
+ }
+
+ return customPrivateKey;
+ }
+
+ /**
+ * Reads a Key from the PEM Encoded Store.
+ *
+ * @param path - Path, Location where the Key file is stored.
+ * @return PrivateKey Object.
+ * @throws IOException - on Error.
+ */
+ protected PKCS8EncodedKeySpec readKey(File path)
Review comment:
Can we add some integration tests for this new code?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org