You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2020/10/05 14:11:47 UTC
[GitHub] [zookeeper] symat opened a new pull request #1480: ZOOKEEPER-3950: Add support for BCFKS key/trust store format
symat opened a new pull request #1480:
URL: https://github.com/apache/zookeeper/pull/1480
The BCFKS key store format is widely used in the industry, as it provides an open source alternative if someone has to use FIPS compliant key stores due to some regulatory constraints.
Currently in the ZooKeeper java client, only PEM, JKS and PEM12 is supported. I extend the list of supported key store formats with BCFKS.
I also tested this patch on a real FIPS compliant cluster, having the appropriate java security configs, security providers and also running a RedHat-based Linux distro (Centos 7.8) with FIPS mode enabled.
I tested both the client and the quorum SSL too. If someone wants to test this patch, and the keystore/truststore file names are not ending with ".bckfs", then (beside the usual SSL configs) make sure to also set the following parameters in the zoo.cfg:
```
ssl.keyStore.type=bcfks
ssl.trustStore.type=bcfks
ssl.quorum.keyStore.type=bcfks
ssl.quorum.trustStore.type=bcfks
```
and also provide the following parameters for the command line java client:
```
-Dzookeeper.ssl.keyStore.type=bcfks -Dzookeeper.ssl.trustStore.type=bcfks
```
This patch doesn't contain any modification for the c-client (that can be handled with a separate Jira, but I don't plan to work on that part right now).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on pull request #1480: ZOOKEEPER-3950: Add support for BCFKS key/trust store format
Posted by GitBox <gi...@apache.org>.
symat commented on pull request #1480:
URL: https://github.com/apache/zookeeper/pull/1480#issuecomment-704149772
thanks!
FYI, I created https://github.com/apache/zookeeper/pull/1482 for branch 3.6
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on pull request #1480: ZOOKEEPER-3950: Add support for BCFKS key/trust store format
Posted by GitBox <gi...@apache.org>.
symat commented on pull request #1480:
URL: https://github.com/apache/zookeeper/pull/1480#issuecomment-703658692
@anmolnar @nkalmar PTAL
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on pull request #1480: ZOOKEEPER-3950: Add support for BCFKS key/trust store format
Posted by GitBox <gi...@apache.org>.
symat commented on pull request #1480:
URL: https://github.com/apache/zookeeper/pull/1480#issuecomment-704145741
great, thank you @eolivelli and @nkalmar for your comments, I'll merge the PR to the master branch and submit a separate PR for branch-3.6
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1480: ZOOKEEPER-3950: Add support for BCFKS key/trust store format
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1480:
URL: https://github.com/apache/zookeeper/pull/1480#discussion_r500061623
##########
File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
##########
@@ -1531,7 +1531,8 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp
* *ssl.keyStore.type* and *ssl.quorum.keyStore.type* :
(Java system properties: **zookeeper.ssl.keyStore.type** and **zookeeper.ssl.quorum.keyStore.type**)
**New in 3.5.5:**
- Specifies the file format of client and quorum keystores. Values: JKS, PEM, PKCS12 or null (detect by filename).
+ Specifies the file format of client and quorum keystores. Values: JKS, PEM, PKCS12, BCFKS or null (detect by filename).
+ The format BCFKS was added in ZooKeeper 3.6.3.
Review comment:
yes, maybe this would be a more consistent way... I updated this, please check the new version
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] nkalmar commented on a change in pull request #1480: ZOOKEEPER-3950: Add support for BCFKS key/trust store format
Posted by GitBox <gi...@apache.org>.
nkalmar commented on a change in pull request #1480:
URL: https://github.com/apache/zookeeper/pull/1480#discussion_r499649350
##########
File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
##########
@@ -1531,7 +1531,8 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp
* *ssl.keyStore.type* and *ssl.quorum.keyStore.type* :
(Java system properties: **zookeeper.ssl.keyStore.type** and **zookeeper.ssl.quorum.keyStore.type**)
**New in 3.5.5:**
- Specifies the file format of client and quorum keystores. Values: JKS, PEM, PKCS12 or null (detect by filename).
+ Specifies the file format of client and quorum keystores. Values: JKS, PEM, PKCS12, BCFKS or null (detect by filename).
+ The format BCFKS was added in ZooKeeper 3.6.3.
Review comment:
I meant to say add it the same way the whole feature is introduced, with a separate double asterix header, but theres already a discussion on this.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] nkalmar commented on pull request #1480: ZOOKEEPER-3950: Add support for BCFKS key/trust store format
Posted by GitBox <gi...@apache.org>.
nkalmar commented on pull request #1480:
URL: https://github.com/apache/zookeeper/pull/1480#issuecomment-704107534
LGTM, thanks @symat for the changes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1480: ZOOKEEPER-3950: Add support for BCFKS key/trust store format
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1480:
URL: https://github.com/apache/zookeeper/pull/1480#discussion_r500061178
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/common/BCFKSFileLoader.java
##########
@@ -0,0 +1,46 @@
+/**
+ * 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.zookeeper.common;
+
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+
+/**
+ * Implementation of {@link FileKeyStoreLoader} that loads from BCKFS files.
+ */
+class BCFKSFileLoader extends StandardTypeFileKeyStoreLoader {
+ private BCFKSFileLoader(String keyStorePath,
+ String trustStorePath,
+ String keyStorePassword,
+ String trustStorePassword) {
+ super(keyStorePath, trustStorePath, keyStorePassword, trustStorePassword);
+ }
+
+ @Override
+ protected KeyStore keyStoreInstance() throws KeyStoreException {
+ return KeyStore.getInstance("BCFKS");
Review comment:
this is a good idea, removing some code duplication from the child classes. I updated this part, PTAL
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on pull request #1480: ZOOKEEPER-3950: Add support for BCFKS key/trust store format
Posted by GitBox <gi...@apache.org>.
symat commented on pull request #1480:
URL: https://github.com/apache/zookeeper/pull/1480#issuecomment-703666762
thanks for the quick review!
what about being more specific and saying: "The format VCFKS is supported from ZooKeeper versions 3.6.3 and 3.7.0" ?
(I prefer to state exact version numbers, so that our users don't need to browse older documents)
what do you think?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] nkalmar commented on a change in pull request #1480: ZOOKEEPER-3950: Add support for BCFKS key/trust store format
Posted by GitBox <gi...@apache.org>.
nkalmar commented on a change in pull request #1480:
URL: https://github.com/apache/zookeeper/pull/1480#discussion_r499642231
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/common/BCFKSFileLoader.java
##########
@@ -0,0 +1,46 @@
+/**
+ * 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.zookeeper.common;
+
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+
+/**
+ * Implementation of {@link FileKeyStoreLoader} that loads from BCKFS files.
+ */
+class BCFKSFileLoader extends StandardTypeFileKeyStoreLoader {
+ private BCFKSFileLoader(String keyStorePath,
+ String trustStorePath,
+ String keyStorePassword,
+ String trustStorePassword) {
+ super(keyStorePath, trustStorePath, keyStorePassword, trustStorePassword);
+ }
+
+ @Override
+ protected KeyStore keyStoreInstance() throws KeyStoreException {
+ return KeyStore.getInstance("BCFKS");
Review comment:
nit: make static fields in StandardTypeFileKeyStoreLoader for supported keystore formats? Or add it to KeyStoreFileType as a second enum value, even better I think.
##########
File path: zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md
##########
@@ -1531,7 +1531,8 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp
* *ssl.keyStore.type* and *ssl.quorum.keyStore.type* :
(Java system properties: **zookeeper.ssl.keyStore.type** and **zookeeper.ssl.quorum.keyStore.type**)
**New in 3.5.5:**
- Specifies the file format of client and quorum keystores. Values: JKS, PEM, PKCS12 or null (detect by filename).
+ Specifies the file format of client and quorum keystores. Values: JKS, PEM, PKCS12, BCFKS or null (detect by filename).
+ The format BCFKS was added in ZooKeeper 3.6.3.
Review comment:
Nit/question: For readability add **New in 3.6.3** Support for BCFKS?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on pull request #1480: ZOOKEEPER-3950: Add support for BCFKS key/trust store format
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1480:
URL: https://github.com/apache/zookeeper/pull/1480#issuecomment-703667773
works for me
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on pull request #1480: ZOOKEEPER-3950: Add support for BCFKS key/trust store format
Posted by GitBox <gi...@apache.org>.
symat commented on pull request #1480:
URL: https://github.com/apache/zookeeper/pull/1480#issuecomment-703659689
FYI: I need to create a separate PR for the branch-3.6 (can not be a clean cherry-pick, due to the junit5 upgrade on the master branch)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org