You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by GitBox <gi...@apache.org> on 2022/11/16 12:22:42 UTC
[GitHub] [ratis] ChenSammi opened a new pull request, #785: RATIS-1747. Support keyManager and trustManager in tlsConfig.
ChenSammi opened a new pull request, #785:
URL: https://github.com/apache/ratis/pull/785
https://issues.apache.org/jira/browse/RATIS-1747
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #785: RATIS-1747. Support keyManager and trustManager in tlsConfig.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on PR #785:
URL: https://github.com/apache/ratis/pull/785#issuecomment-1321430939
@ChenSammi , please see https://github.com/apache/ratis/blob/master/DEPLOY.md
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] ChenSammi commented on pull request #785: RATIS-1747. Support keyManager and trustManager in tlsConfig.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #785:
URL: https://github.com/apache/ratis/pull/785#issuecomment-1321647656
I see. I don't have the committer privilege yet. @szetszwo , could you do me a favor and help to publish a 3.0 SNAPSHOT release at your convenient time?
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #785: RATIS-1747. Support keyManager and trustManager in tlsConfig.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on PR #785:
URL: https://github.com/apache/ratis/pull/785#issuecomment-1319176003
@ChenSammi , thanks a lot for the update! There is conflict. Could you resolve it?
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #785: RATIS-1747. Support keyManager and trustManager in tlsConfig.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on PR #785:
URL: https://github.com/apache/ratis/pull/785#issuecomment-1317573641
> ... Will we keep on maintaining both branch-2 and master(3.0) for ratis?
@ChenSammi , we should move Ozone to Ratis 3.0 soon.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] ChenSammi commented on pull request #785: RATIS-1747. Support keyManager and trustManager in tlsConfig.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #785:
URL: https://github.com/apache/ratis/pull/785#issuecomment-1319931724
Hi @szetszwo , could you help to merge it? I have re-based the patch against the master because there is some file conflict. There is one unit test failure which is irrelevant.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] ChenSammi commented on pull request #785: RATIS-1747. Support keyManager and trustManager in tlsConfig.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #785:
URL: https://github.com/apache/ratis/pull/785#issuecomment-1321370102
Thanks @szetszwo . Could you tell me how to publish a ratis 3.0 snapshot? so it can be used in Ozone 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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] ChenSammi commented on pull request #785: RATIS-1747. Support keyManager and trustManager in tlsConfig.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #785:
URL: https://github.com/apache/ratis/pull/785#issuecomment-1316936729
Hi @szetszwo , I see currently Ozone is using ratis 2.4.0 release. And it looks like ratis master(3.0) is not compatible with branch-2 in some API. Will we keep on maintaining both branch-2 and master(3.0) for ratis?
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #785: RATIS-1747. Support keyManager and trustManager in tlsConfig.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on PR #785:
URL: https://github.com/apache/ratis/pull/785#issuecomment-1325527670
@ChenSammi , just have deployed 3.0.0-729d3dc-SNAPSHOT .
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on pull request #785: RATIS-1747. Support keyManager and trustManager in tlsConfig.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on PR #785:
URL: https://github.com/apache/ratis/pull/785#issuecomment-1332852843
@ChenSammi , also deployed 2.4.2-8b8bdda-SNAPSHOT.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] ChenSammi commented on pull request #785: RATIS-1747. Support keyManager and trustManager in tlsConfig.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #785:
URL: https://github.com/apache/ratis/pull/785#issuecomment-1326955224
@szetszwo , thanks a lot.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo commented on a diff in pull request #785: RATIS-1747. Support keyManager and trustManager in tlsConfig.
Posted by GitBox <gi...@apache.org>.
szetszwo commented on code in PR #785:
URL: https://github.com/apache/ratis/pull/785#discussion_r1024349594
##########
ratis-common/pom.xml:
##########
@@ -49,6 +49,17 @@
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.bouncycastle</groupId>
+ <artifactId>bcprov-jdk15on</artifactId>
+ <version>${bouncycastle.version}</version>
+ </dependency>
+ <dependency>
+ <groupId>org.bouncycastle</groupId>
+ <artifactId>bcpkix-jdk15on</artifactId>
+ <version>${bouncycastle.version}</version>
+ </dependency>
Review Comment:
Move org.bouncycastle dependencies to `ratis-test` and add
```xml
<scope>test</scope>
```
##########
ratis-common/src/main/java/org/apache/ratis/security/TlsConf.java:
##########
@@ -96,24 +98,34 @@ public PrivateKeyConf(File privateKeyFile) {
/** Configurations for a trust manager. */
public static final class TrustManagerConf {
/** Trust certificates. */
- private final CertificatesConf trustCertificates;
+ private CertificatesConf trustCertificates;
+ private TrustManager trustManager;
Review Comment:
Keep both fields `final`.
##########
ratis-common/src/test/java/org/apache/ratis/security/SecurityTestUtils.java:
##########
@@ -20,12 +20,33 @@
import org.apache.ratis.security.TlsConf.Builder;
import org.apache.ratis.security.TlsConf.CertificatesConf;
import org.apache.ratis.security.TlsConf.PrivateKeyConf;
+import org.bouncycastle.util.io.pem.PemObject;
+import org.bouncycastle.util.io.pem.PemReader;
Review Comment:
Let's move `SecurityTestUtils` to `ratis-test`. Then, we don't have to add org.bouncycastle dependencies to `ratis-common`.
##########
pom.xml:
##########
@@ -218,6 +218,8 @@
<test.exclude.pattern>_</test.exclude.pattern>
<!-- number of threads/forks to use when running tests in parallel, see parallel-tests profile -->
<testsThreadCount>4</testsThreadCount>
+
+ <bouncycastle.version>1.67</bouncycastle.version>
Review Comment:
Use 1.70. IntelliJ reported some security vulnerabilities on 1.67.
##########
ratis-common/src/main/java/org/apache/ratis/security/TlsConf.java:
##########
@@ -96,24 +98,34 @@ public PrivateKeyConf(File privateKeyFile) {
/** Configurations for a trust manager. */
public static final class TrustManagerConf {
/** Trust certificates. */
- private final CertificatesConf trustCertificates;
+ private CertificatesConf trustCertificates;
+ private TrustManager trustManager;
private TrustManagerConf(CertificatesConf trustCertificates) {
this.trustCertificates = trustCertificates;
}
+ private TrustManagerConf(TrustManager trustManager) {
+ this.trustManager = trustManager;
+ }
+
/** @return the trust certificates. */
public CertificatesConf getTrustCertificates() {
return trustCertificates;
}
+
+ public TrustManager getTrustManager() {
+ return trustManager;
+ }
}
/** Configurations for a key manager. */
public static final class KeyManagerConf {
/** A {@link PrivateKey}. */
- private final PrivateKeyConf privateKey;
+ private PrivateKeyConf privateKey;
/** Certificates for the private key. */
- private final CertificatesConf keyCertificates;
+ private CertificatesConf keyCertificates;
+ private KeyManager keyManager;
Review Comment:
Keep all the fields `final`.
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/GrpcTlsConfig.java:
##########
@@ -81,6 +88,18 @@ public File getTrustStoreFile() {
.orElse(null);
}
+ public TrustManager getSslTrustManager() {
+ return Optional.ofNullable(getTrustManager())
+ .map(TrustManagerConf::getTrustManager)
+ .orElse(null);
+ }
+
+ public KeyManager getSslKeyManager() {
+ return Optional.ofNullable(getKeyManager())
+ .map(KeyManagerConf::getKeyManager)
+ .orElse(null);
+ }
Review Comment:
Let's don't add new API to `GrpcTlsConfig`, which will be deprecated soon. We should use the API from `TlsConf`.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] ChenSammi commented on pull request #785: RATIS-1747. Support keyManager and trustManager in tlsConfig.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #785:
URL: https://github.com/apache/ratis/pull/785#issuecomment-1316977133
The failed UT TestWatchRequestWithGrpc is irrelevant. It passed in local.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] ChenSammi commented on pull request #785: RATIS-1747. Support keyManager and trustManager in tlsConfig.
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on PR #785:
URL: https://github.com/apache/ratis/pull/785#issuecomment-1318001495
Thanks @szetszwo . A new patch is uploaded to address the comments.
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ratis] szetszwo merged pull request #785: RATIS-1747. Support keyManager and trustManager in tlsConfig.
Posted by GitBox <gi...@apache.org>.
szetszwo merged PR #785:
URL: https://github.com/apache/ratis/pull/785
--
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@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org