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/12/23 20:01:41 UTC
[GitHub] [ozone] neils-dev opened a new pull request #2945: HDDS-5545. Enable TLS for GRPC OmTransport implementation
neils-dev opened a new pull request #2945:
URL: https://github.com/apache/ozone/pull/2945
…sync with Ozone master containing s3gateway perisistent connection hadoop rpc from HDDS-5881. This commit also includes functionality to configure omtransport choosiing either hrpc (default) or Grpc OmTransport s3gateway om channel - HDDS-5813.
## What changes were proposed in this pull request?
Support for TLS secured Grpc OmTransport channel between s3gateway, s3g, and the ozone manager, om. TLS is enabled for the s3g persistent connection between the s3g and the om through the configuration "_**hdds.grpc.tls.enabled**_" boolean.
Included is selection of OmTransport used between s3g and om through configuration "_**ozone.om.transport.class**_", `org.apache.hadoop.ozone.om.protocolPB.Hadoop3OmTransportFactory` (default) or `org.apache.hadoop.ozone.om.protocolPB.GrpcOmTransportFactory`. Allowing use of Hadoop rpc, hrpc, (HDDS-5581) while migrating to the full HDDS-4440 feature using Grpc persistent OmTransport s3g.
The TLS Grpc client connection is established by initially using a temporary secured (kerberos) hrpc connection between the s3g and the om to obtain the CA Certificates `(serviceInfo` `OmRequest`). The certificates are then used to authenticate the Grpc TLS connection established between the s3g and the om through the `GrpcOmTransport`.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5545
## How was this patch tested?
Patch was tested through both unit tests and manually with the secureozone cluster.
1. unit test `TestGrpcOmTransport.testGrpcOmTransportFactory`, `TestGrpcOmTransport.testHrpcOmTransportFactory`
`hadoop-ozone/s3gateway$ mvn -Dtest=TestGrpcOmTransport#testGrpcOmTransportFactory test`
[INFO] Running org.apache.hadoop.ozone.protocolPB.TestGrpcOmTransport
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.97 s - in org.apache.hadoop.ozone.protocolPB.TestGrpcOmTransport
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0
`hadoop-ozone/s3gateway$ mvn -Dtest=TestGrpcOmTransport#testHrpcOmTransportFactory test`
[INFO] Running org.apache.hadoop.ozone.protocolPB.TestGrpcOmTransport
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.647 s - in org.apache.hadoop.ozone.protocolPB.TestGrpcOmTransport
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0
2. manual testing with ozonesecure cluster:
`hadoop-ozone/dist/target/ozone-1.2.0-SNAPSHOT/compose/ozonesecure$ docker-compose up -d --scale datanode=3`
setup kdc and get user secret:
```
hadoop-ozone/dist/target/ozone-1.2.0-SNAPSHOT/compose/ozonesecure$ docker-compose exec scm bash
$ kinit -kt /etc/security/keytabs/testuser.keytab testuser/scm@EXAMPLE.COM
$ ozone s3 getsecret -u=testuser/scm@EXAMPLE.COM
```
With user credentials set in profile 'ozone' (aws secret & username testuser/scm@EXAMPLE.COM) through aws cli:
```
$ aws s3api --profile ozone --endpoint http://localhost:9878 list-buckets
{
"Buckets": []
}
```
Runs s3g with hrpc omtransport.
Now for TLS Grpc rerun with same but with ozone configuration for Grpc om transport through "_**ozone.om.transport.class**_"=`org.apache.hadoop.ozone.om.protocolPB.GrpcOmTransportFactory` ie.
**set** in _hadoop-ozone/dist/target/ozone-1.2.0-SNAPSHOT/compose/ozonesecure/docker-config_ : ` OZONE-SITE.XML_ozone.om.transport.class=org.apache.hadoop.ozone.om.protocolPB.GrpcOmTransportFactory`
```
$ aws s3api --profile ozone --endpoint http://localhost:9878 list-buckets
{
"Buckets": []
}
```
--
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] neils-dev commented on a change in pull request #2945: HDDS-5545. Enable TLS for GRPC OmTransport implementation
Posted by GitBox <gi...@apache.org>.
neils-dev commented on a change in pull request #2945:
URL: https://github.com/apache/ozone/pull/2945#discussion_r825235324
##########
File path: hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/OzoneClientCache.java
##########
@@ -75,8 +99,66 @@ public static OzoneClient getOzoneClientInstance(OzoneConfiguration
return instance.client;
}
+ public static void closeClient() throws IOException {
+ if (instance != null) {
+ instance.client.close();
+ instance = null;
+ }
+ }
+
+ private void setCertificate(String omServiceID,
+ OzoneConfiguration conf)
+ throws IOException {
+
+ // create local copy of config incase exception occurs
+ // with certificate OmRequest
+ OzoneConfiguration config = new OzoneConfiguration(conf);
+ OzoneClient certClient;
+
+ if (secConfig.isGrpcTlsEnabled()) {
+ // set OmTransport to hadoop rpc to securely,
+ // get certificates with service list request
+ config.set(OZONE_OM_TRANSPORT_CLASS,
+ OZONE_OM_TRANSPORT_CLASS_DEFAULT);
+
+ if (omServiceID == null) {
+ certClient = OzoneClientFactory.getRpcClient(config);
+ } else {
+ // As in HA case, we need to pass om service ID.
+ certClient = OzoneClientFactory.getRpcClient(omServiceID,
+ config);
+ }
+ try {
+ ServiceInfoEx serviceInfoEx = certClient
+ .getObjectStore()
+ .getClientProxy()
+ .getOzoneManagerClient()
+ .getServiceInfo();
+
+ if (OzoneSecurityUtil.isSecurityEnabled(conf)) {
+ String caCertPem = null;
+ List<String> caCertPems = null;
+ caCertPem = serviceInfoEx.getCaCertificate();
+ caCertPems = serviceInfoEx.getCaCertPemList();
+ if (caCertPems == null || caCertPems.isEmpty()) {
+ caCertPems = Collections.singletonList(caCertPem);
Review comment:
The same conditions exist in the `RpcClient` constructor (RpcClient.java) where `caCertPem` and `caCertPems` are used in a similar check to see if `caCertPems` exists to set the `caCertPems` to `caCertPem` if necessary. Not sure if we need to address this. Are we guaranteed to have at least one of them valid and non null (`caCertPem`)?
--
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] neils-dev commented on a change in pull request #2945: HDDS-5545. Enable TLS for GRPC OmTransport implementation
Posted by GitBox <gi...@apache.org>.
neils-dev commented on a change in pull request #2945:
URL: https://github.com/apache/ozone/pull/2945#discussion_r827488331
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/GrpcOmTransport.java
##########
@@ -93,6 +104,22 @@ public void start() {
.usePlaintext()
.maxInboundMessageSize(maxSize);
+ if (secConfig.isGrpcTlsEnabled()) {
+ try {
+ SslContextBuilder sslContextBuilder = GrpcSslContexts.forClient();
+ if (caCerts != null) {
+ sslContextBuilder.trustManager(caCerts);
+ } else {
+ LOG.error("x509Certicates empty");
Review comment:
Errors on s3g client creation appear the same to the user I'm afraid. The s3 gateway uses jax context and dependency injections that fail if the s3 gateway fails on initialization. That is what happens when the s3g TLS channel is, for any given reason, unable to be created when _ozone.security.enabled_ and _hdds.grpc.tls.enabled_ are set. The error in s3g client creation is propagated up through the jax injection failure which results in retries and the 500 http erro response.
> would it not be better to fail the client request here itself instead of when it reaches the max retries?
In this case all errors that occur during the TLS / Grpc channel creation appear the same to the client with a 500 http response and max retires. So if we fail it another way, it will still propagate back the same. We do provide additional valuable information on the error through the log. Here we have the additional information that we log indicating that the `"x509Certificates empty"`.
##########
File path: hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/OzoneClientCache.java
##########
@@ -75,8 +99,66 @@ public static OzoneClient getOzoneClientInstance(OzoneConfiguration
return instance.client;
}
+ public static void closeClient() throws IOException {
+ if (instance != null) {
+ instance.client.close();
+ instance = null;
+ }
+ }
+
+ private void setCertificate(String omServiceID,
+ OzoneConfiguration conf)
+ throws IOException {
+
+ // create local copy of config incase exception occurs
+ // with certificate OmRequest
+ OzoneConfiguration config = new OzoneConfiguration(conf);
+ OzoneClient certClient;
+
+ if (secConfig.isGrpcTlsEnabled()) {
+ // set OmTransport to hadoop rpc to securely,
+ // get certificates with service list request
+ config.set(OZONE_OM_TRANSPORT_CLASS,
+ OZONE_OM_TRANSPORT_CLASS_DEFAULT);
+
+ if (omServiceID == null) {
+ certClient = OzoneClientFactory.getRpcClient(config);
+ } else {
+ // As in HA case, we need to pass om service ID.
+ certClient = OzoneClientFactory.getRpcClient(omServiceID,
+ config);
+ }
+ try {
+ ServiceInfoEx serviceInfoEx = certClient
+ .getObjectStore()
+ .getClientProxy()
+ .getOzoneManagerClient()
+ .getServiceInfo();
+
+ if (OzoneSecurityUtil.isSecurityEnabled(conf)) {
+ String caCertPem = null;
+ List<String> caCertPems = null;
+ caCertPem = serviceInfoEx.getCaCertificate();
+ caCertPems = serviceInfoEx.getCaCertPemList();
+ if (caCertPems == null || caCertPems.isEmpty()) {
+ caCertPems = Collections.singletonList(caCertPem);
Review comment:
Thanks @hanishakoneru for following up. Yes, I agree - looking at the code for the certificates, it may not be set in neither the `CaCertPemList` nor the `CaCertificate`. I've added an additional check for null CaCertificate and throws if the not null precondition is not met in the `OzoneClientCache` prior to setCaCerts for the transport.
As a result we handle this with the client creation jax injection error 500 http response (max retries) with an exception trace indicating the precondition failure of not null check on the certificate:
_java.lang.NullPointerException
at com.google.common.base.Preconditions.checkNotNull(Preconditions.java:880)
,org.apache.hadoop.ozone.s3.OzoneClientCache.setCertificate(OzoneClientCache.java:146)_
Do we need to do anything in addition?
The same conditional check of `caCertPems` for null or empty then setting the `caCertPerms` with the `caCertPer` exists in the `RpcClient` constructor code. I can followup on this with new jira to patch the `RpcClient` if you think it is needed(?).
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/GrpcOzoneManagerServer.java
##########
@@ -46,24 +58,42 @@ public GrpcOzoneManagerServer(OzoneConfiguration config,
OzoneManagerProtocolServerSideTranslatorPB
omTranslator,
OzoneDelegationTokenSecretManager
- delegationTokenMgr) {
+ delegationTokenMgr,
+ CertificateClient caClient) {
this.port = config.getObject(
GrpcOzoneManagerServerConfig.class).
getPort();
init(omTranslator,
delegationTokenMgr,
- config);
+ config,
+ caClient);
}
public void init(OzoneManagerProtocolServerSideTranslatorPB omTranslator,
OzoneDelegationTokenSecretManager delegationTokenMgr,
- OzoneConfiguration omServerConfig) {
+ OzoneConfiguration omServerConfig,
+ CertificateClient caClient) {
NettyServerBuilder nettyServerBuilder = NettyServerBuilder.forPort(port)
.maxInboundMessageSize(OzoneConsts.OZONE_SCM_CHUNK_MAX_SIZE)
.addService(new OzoneManagerServiceGrpc(omTranslator,
delegationTokenMgr,
omServerConfig));
+ SecurityConfig secConf = new SecurityConfig(omServerConfig);
+ if (secConf.isGrpcTlsEnabled()) {
Review comment:
Yes, logging a separate error for misconfigured ozone.security while _grpc.tls.enabled_ is `true` for the s3g Grpc TLS channel is much better.
I have modified the code for this case on both the s3g Grpc client and the s3g Grpc server. For the client, `GrpcOmTransport`, should `grpc.tls.enabled` be set when _ozone.security_ = `false`, we log the error "...security not enabled when TLS specified.". Also, we don't set the sslContextBuilder for tls and default to plaintext channel. Similarly, for the server, `GrpcOzoneManagerServer`, should `grpc.tls.enabled` be set when _ozone.security_ = `false`, we log the error ("... not enabled when TLS..., using plaintext.") and default to a plaintext channel.
--
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] neils-dev commented on a change in pull request #2945: HDDS-5545. Enable TLS for GRPC OmTransport implementation
Posted by GitBox <gi...@apache.org>.
neils-dev commented on a change in pull request #2945:
URL: https://github.com/apache/ozone/pull/2945#discussion_r827488949
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/GrpcOzoneManagerServer.java
##########
@@ -46,24 +58,42 @@ public GrpcOzoneManagerServer(OzoneConfiguration config,
OzoneManagerProtocolServerSideTranslatorPB
omTranslator,
OzoneDelegationTokenSecretManager
- delegationTokenMgr) {
+ delegationTokenMgr,
+ CertificateClient caClient) {
this.port = config.getObject(
GrpcOzoneManagerServerConfig.class).
getPort();
init(omTranslator,
delegationTokenMgr,
- config);
+ config,
+ caClient);
}
public void init(OzoneManagerProtocolServerSideTranslatorPB omTranslator,
OzoneDelegationTokenSecretManager delegationTokenMgr,
- OzoneConfiguration omServerConfig) {
+ OzoneConfiguration omServerConfig,
+ CertificateClient caClient) {
NettyServerBuilder nettyServerBuilder = NettyServerBuilder.forPort(port)
.maxInboundMessageSize(OzoneConsts.OZONE_SCM_CHUNK_MAX_SIZE)
.addService(new OzoneManagerServiceGrpc(omTranslator,
delegationTokenMgr,
omServerConfig));
+ SecurityConfig secConf = new SecurityConfig(omServerConfig);
+ if (secConf.isGrpcTlsEnabled()) {
Review comment:
Latest commit contains changes to handle ozone-site misconfigurations for TLS enabled s3g Grpc and ozone.security.enabled.
--
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 #2945: HDDS-5545. Enable TLS for GRPC OmTransport implementation
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on pull request #2945:
URL: https://github.com/apache/ozone/pull/2945#issuecomment-1081203718
Thanks @neils-dev for addressing all the comments.
LGTM. +1 pending CI.
--
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 edited a comment on pull request #2945: HDDS-5545. Enable TLS for GRPC OmTransport implementation
Posted by GitBox <gi...@apache.org>.
hanishakoneru edited a comment on pull request #2945:
URL: https://github.com/apache/ozone/pull/2945#issuecomment-1074478984
Thanks for updating the patch @neils-dev.
I would request you to avoid force-pushing to PRs. It makes it hard for the reviewer as they cannot see what updates have been done since the last review. Force-push can be avoided by merging from master/ base instead of rebasing.
--
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 #2945: HDDS-5545. Enable TLS for GRPC OmTransport implementation
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #2945:
URL: https://github.com/apache/ozone/pull/2945#discussion_r834537269
##########
File path: hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/OzoneClientCache.java
##########
@@ -75,8 +99,66 @@ public static OzoneClient getOzoneClientInstance(OzoneConfiguration
return instance.client;
}
+ public static void closeClient() throws IOException {
+ if (instance != null) {
+ instance.client.close();
+ instance = null;
+ }
+ }
+
+ private void setCertificate(String omServiceID,
+ OzoneConfiguration conf)
+ throws IOException {
+
+ // create local copy of config incase exception occurs
+ // with certificate OmRequest
+ OzoneConfiguration config = new OzoneConfiguration(conf);
+ OzoneClient certClient;
+
+ if (secConfig.isGrpcTlsEnabled()) {
+ // set OmTransport to hadoop rpc to securely,
+ // get certificates with service list request
+ config.set(OZONE_OM_TRANSPORT_CLASS,
+ OZONE_OM_TRANSPORT_CLASS_DEFAULT);
+
+ if (omServiceID == null) {
+ certClient = OzoneClientFactory.getRpcClient(config);
+ } else {
+ // As in HA case, we need to pass om service ID.
+ certClient = OzoneClientFactory.getRpcClient(omServiceID,
+ config);
+ }
+ try {
+ ServiceInfoEx serviceInfoEx = certClient
+ .getObjectStore()
+ .getClientProxy()
+ .getOzoneManagerClient()
+ .getServiceInfo();
+
+ if (OzoneSecurityUtil.isSecurityEnabled(conf)) {
+ String caCertPem = null;
+ List<String> caCertPems = null;
+ caCertPem = serviceInfoEx.getCaCertificate();
+ caCertPems = serviceInfoEx.getCaCertPemList();
+ if (caCertPems == null || caCertPems.isEmpty()) {
+ caCertPems = Collections.singletonList(caCertPem);
Review comment:
I think logging and throwing an exception would be better than throwing a NullPointerException.
--
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 #2945: HDDS-5545. Enable TLS for GRPC OmTransport implementation
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #2945:
URL: https://github.com/apache/ozone/pull/2945#discussion_r834752393
##########
File path: hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/OzoneClientCache.java
##########
@@ -75,8 +99,66 @@ public static OzoneClient getOzoneClientInstance(OzoneConfiguration
return instance.client;
}
+ public static void closeClient() throws IOException {
+ if (instance != null) {
+ instance.client.close();
+ instance = null;
+ }
+ }
+
+ private void setCertificate(String omServiceID,
+ OzoneConfiguration conf)
+ throws IOException {
+
+ // create local copy of config incase exception occurs
+ // with certificate OmRequest
+ OzoneConfiguration config = new OzoneConfiguration(conf);
+ OzoneClient certClient;
+
+ if (secConfig.isGrpcTlsEnabled()) {
+ // set OmTransport to hadoop rpc to securely,
+ // get certificates with service list request
+ config.set(OZONE_OM_TRANSPORT_CLASS,
+ OZONE_OM_TRANSPORT_CLASS_DEFAULT);
+
+ if (omServiceID == null) {
+ certClient = OzoneClientFactory.getRpcClient(config);
+ } else {
+ // As in HA case, we need to pass om service ID.
+ certClient = OzoneClientFactory.getRpcClient(omServiceID,
+ config);
+ }
+ try {
+ ServiceInfoEx serviceInfoEx = certClient
+ .getObjectStore()
+ .getClientProxy()
+ .getOzoneManagerClient()
+ .getServiceInfo();
+
+ if (OzoneSecurityUtil.isSecurityEnabled(conf)) {
+ String caCertPem = null;
+ List<String> caCertPems = null;
+ caCertPem = serviceInfoEx.getCaCertificate();
+ caCertPems = serviceInfoEx.getCaCertPemList();
+ if (caCertPems == null || caCertPems.isEmpty()) {
+ caCertPems = Collections.singletonList(caCertPem);
Review comment:
Or I am ok with leaving it as before (no extra check, similar to how it is in RpcClient) and we can file a separate Jira to address this at both the places.
--
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] neils-dev commented on pull request #2945: HDDS-5545. Enable TLS for GRPC OmTransport implementation
Posted by GitBox <gi...@apache.org>.
neils-dev commented on pull request #2945:
URL: https://github.com/apache/ozone/pull/2945#issuecomment-1064393011
@kerneltime - following up on what we discussed earlier, it would be great for you look at this and get your comments. 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 a change in pull request #2945: HDDS-5545. Enable TLS for GRPC OmTransport implementation
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #2945:
URL: https://github.com/apache/ozone/pull/2945#discussion_r826262703
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/GrpcOzoneManagerServer.java
##########
@@ -46,24 +58,42 @@ public GrpcOzoneManagerServer(OzoneConfiguration config,
OzoneManagerProtocolServerSideTranslatorPB
omTranslator,
OzoneDelegationTokenSecretManager
- delegationTokenMgr) {
+ delegationTokenMgr,
+ CertificateClient caClient) {
this.port = config.getObject(
GrpcOzoneManagerServerConfig.class).
getPort();
init(omTranslator,
delegationTokenMgr,
- config);
+ config,
+ caClient);
}
public void init(OzoneManagerProtocolServerSideTranslatorPB omTranslator,
OzoneDelegationTokenSecretManager delegationTokenMgr,
- OzoneConfiguration omServerConfig) {
+ OzoneConfiguration omServerConfig,
+ CertificateClient caClient) {
NettyServerBuilder nettyServerBuilder = NettyServerBuilder.forPort(port)
.maxInboundMessageSize(OzoneConsts.OZONE_SCM_CHUNK_MAX_SIZE)
.addService(new OzoneManagerServiceGrpc(omTranslator,
delegationTokenMgr,
omServerConfig));
+ SecurityConfig secConf = new SecurityConfig(omServerConfig);
+ if (secConf.isGrpcTlsEnabled()) {
Review comment:
It would be the same error ("x509Certicates empty") for different cases.
Would it be better to add a check that security is enabled when TLS is enabled so that the error log can state clearly what the issue is?
--
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] neils-dev commented on a change in pull request #2945: HDDS-5545. Enable TLS for GRPC OmTransport implementation
Posted by GitBox <gi...@apache.org>.
neils-dev commented on a change in pull request #2945:
URL: https://github.com/apache/ozone/pull/2945#discussion_r813191619
##########
File path: hadoop-ozone/common/pom.xml
##########
@@ -48,6 +48,17 @@ https://maven.apache.org/xsd/maven-4.0.0.xsd">
<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-handler-proxy</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-tcnative-boringssl-static</artifactId>
+ <version>2.0.38.Final</version> <!-- See table for correct version -->
Review comment:
Thanks @hanishakoneru for reviewing this PR and for your comments. I've added the pom property for the netty-tcnative version to the central main pom.xml. Thanks.
`<tcnative.version>2.0.38.Final</tcnative.version>`
I also brought this up with @adoroszlai offline and he kindly added a comment to this PR on this.
##########
File path: hadoop-ozone/common/pom.xml
##########
@@ -48,6 +48,17 @@ https://maven.apache.org/xsd/maven-4.0.0.xsd">
<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-handler-proxy</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-tcnative-boringssl-static</artifactId>
+ <version>2.0.38.Final</version> <!-- See table for correct version -->
+ <scope>runtime</scope>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-tcnative</artifactId>
+ <version>2.0.38.Final</version> <!-- See table for correct version -->
Review comment:
The table comment indeed refers to the grpc tls jar version combinations (`grpc-netty`, `netty-handler`, `netty-tcnative-boringssl`) that work together. This is posted on the grpc github in the above link.
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OmTransportFactory.java
##########
@@ -34,26 +37,37 @@ OmTransport createOmTransport(ConfigurationSource source,
static OmTransport create(ConfigurationSource conf,
UserGroupInformation ugi, String omServiceId) throws IOException {
- OmTransportFactory factory = createFactory();
+ OmTransportFactory factory = createFactory(conf);
return factory.createOmTransport(conf, ugi, omServiceId);
}
- static OmTransportFactory createFactory() throws IOException {
- ServiceLoader<OmTransportFactory> transportFactoryServiceLoader =
- ServiceLoader.load(OmTransportFactory.class);
- Iterator<OmTransportFactory> iterator =
- transportFactoryServiceLoader.iterator();
- if (iterator.hasNext()) {
- return iterator.next();
- }
+ static OmTransportFactory createFactory(ConfigurationSource conf)
+ throws IOException {
try {
+ // if configured transport class is different than the default
+ // Hadoop3OmTransportFactory, then check service loader for
+ // transport class and instantiate it
+ if (conf
+ .get(OZONE_OM_TRANSPORT_CLASS,
+ OZONE_OM_TRANSPORT_CLASS_DEFAULT) !=
+ OZONE_OM_TRANSPORT_CLASS_DEFAULT) {
Review comment:
Right, the OmTransportFactory was hardcoded to instantiate the Hadoop3OmTransportFactory implementation when no other implementation was provided. I've updated this to using the default, `OZONE_OM_TRANSPORT_CLASS_DEFAULT`, when no other as suggested. Thanks @hanishakoneru, all changes pushed in latest commit.
--
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 #2945: HDDS-5545. Enable TLS for GRPC OmTransport implementation
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #2945:
URL: https://github.com/apache/ozone/pull/2945#discussion_r831588780
##########
File path: hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/OzoneClientCache.java
##########
@@ -75,8 +99,66 @@ public static OzoneClient getOzoneClientInstance(OzoneConfiguration
return instance.client;
}
+ public static void closeClient() throws IOException {
+ if (instance != null) {
+ instance.client.close();
+ instance = null;
+ }
+ }
+
+ private void setCertificate(String omServiceID,
+ OzoneConfiguration conf)
+ throws IOException {
+
+ // create local copy of config incase exception occurs
+ // with certificate OmRequest
+ OzoneConfiguration config = new OzoneConfiguration(conf);
+ OzoneClient certClient;
+
+ if (secConfig.isGrpcTlsEnabled()) {
+ // set OmTransport to hadoop rpc to securely,
+ // get certificates with service list request
+ config.set(OZONE_OM_TRANSPORT_CLASS,
+ OZONE_OM_TRANSPORT_CLASS_DEFAULT);
+
+ if (omServiceID == null) {
+ certClient = OzoneClientFactory.getRpcClient(config);
+ } else {
+ // As in HA case, we need to pass om service ID.
+ certClient = OzoneClientFactory.getRpcClient(omServiceID,
+ config);
+ }
+ try {
+ ServiceInfoEx serviceInfoEx = certClient
+ .getObjectStore()
+ .getClientProxy()
+ .getOzoneManagerClient()
+ .getServiceInfo();
+
+ if (OzoneSecurityUtil.isSecurityEnabled(conf)) {
+ String caCertPem = null;
+ List<String> caCertPems = null;
+ caCertPem = serviceInfoEx.getCaCertificate();
+ caCertPems = serviceInfoEx.getCaCertPemList();
+ if (caCertPems == null || caCertPems.isEmpty()) {
+ caCertPems = Collections.singletonList(caCertPem);
Review comment:
I don't think the Precondtion check is the right over here as there could be a case where caCertPems is null.
When caCertPems is null, the request would fail in authentication step anyway, right? If so, we should just log an error here. Throwing a NullPointerException does not look right.
--
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] neils-dev commented on a change in pull request #2945: HDDS-5545. Enable TLS for GRPC OmTransport implementation
Posted by GitBox <gi...@apache.org>.
neils-dev commented on a change in pull request #2945:
URL: https://github.com/apache/ozone/pull/2945#discussion_r825235324
##########
File path: hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/OzoneClientCache.java
##########
@@ -75,8 +99,66 @@ public static OzoneClient getOzoneClientInstance(OzoneConfiguration
return instance.client;
}
+ public static void closeClient() throws IOException {
+ if (instance != null) {
+ instance.client.close();
+ instance = null;
+ }
+ }
+
+ private void setCertificate(String omServiceID,
+ OzoneConfiguration conf)
+ throws IOException {
+
+ // create local copy of config incase exception occurs
+ // with certificate OmRequest
+ OzoneConfiguration config = new OzoneConfiguration(conf);
+ OzoneClient certClient;
+
+ if (secConfig.isGrpcTlsEnabled()) {
+ // set OmTransport to hadoop rpc to securely,
+ // get certificates with service list request
+ config.set(OZONE_OM_TRANSPORT_CLASS,
+ OZONE_OM_TRANSPORT_CLASS_DEFAULT);
+
+ if (omServiceID == null) {
+ certClient = OzoneClientFactory.getRpcClient(config);
+ } else {
+ // As in HA case, we need to pass om service ID.
+ certClient = OzoneClientFactory.getRpcClient(omServiceID,
+ config);
+ }
+ try {
+ ServiceInfoEx serviceInfoEx = certClient
+ .getObjectStore()
+ .getClientProxy()
+ .getOzoneManagerClient()
+ .getServiceInfo();
+
+ if (OzoneSecurityUtil.isSecurityEnabled(conf)) {
+ String caCertPem = null;
+ List<String> caCertPems = null;
+ caCertPem = serviceInfoEx.getCaCertificate();
+ caCertPems = serviceInfoEx.getCaCertPemList();
+ if (caCertPems == null || caCertPems.isEmpty()) {
+ caCertPems = Collections.singletonList(caCertPem);
Review comment:
The same conditions exist in the `RpcClient` constructor (RpcClient.java) where `caCertPem` and `caCertPems` are used in a similar check to see if `caCertPems` exists to set the `caCertPems` to `caCertPem` if necessary. Not sure if we need to address this. Are we guaranteed to have at least one of them valid and non null (caCertPem)?
--
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] adoroszlai commented on a change in pull request #2945: HDDS-5545. Enable TLS for GRPC OmTransport implementation
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2945:
URL: https://github.com/apache/ozone/pull/2945#discussion_r811622506
##########
File path: hadoop-ozone/common/pom.xml
##########
@@ -48,6 +48,17 @@ https://maven.apache.org/xsd/maven-4.0.0.xsd">
<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-handler-proxy</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-tcnative-boringssl-static</artifactId>
+ <version>2.0.38.Final</version> <!-- See table for correct version -->
Review comment:
Netty provides a BOM (bill of materials) module. We use it to ensure version consistency of various Netty modules (see HDDS-5298). In most recent versions this BOM also includes tcnative modules. We will be able to avoid tracking tcnative version after upgrade to latest Netty (HDDS-6097).
--
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] neils-dev commented on a change in pull request #2945: HDDS-5545. Enable TLS for GRPC OmTransport implementation
Posted by GitBox <gi...@apache.org>.
neils-dev commented on a change in pull request #2945:
URL: https://github.com/apache/ozone/pull/2945#discussion_r833600467
##########
File path: hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/OzoneClientCache.java
##########
@@ -75,8 +99,66 @@ public static OzoneClient getOzoneClientInstance(OzoneConfiguration
return instance.client;
}
+ public static void closeClient() throws IOException {
+ if (instance != null) {
+ instance.client.close();
+ instance = null;
+ }
+ }
+
+ private void setCertificate(String omServiceID,
+ OzoneConfiguration conf)
+ throws IOException {
+
+ // create local copy of config incase exception occurs
+ // with certificate OmRequest
+ OzoneConfiguration config = new OzoneConfiguration(conf);
+ OzoneClient certClient;
+
+ if (secConfig.isGrpcTlsEnabled()) {
+ // set OmTransport to hadoop rpc to securely,
+ // get certificates with service list request
+ config.set(OZONE_OM_TRANSPORT_CLASS,
+ OZONE_OM_TRANSPORT_CLASS_DEFAULT);
+
+ if (omServiceID == null) {
+ certClient = OzoneClientFactory.getRpcClient(config);
+ } else {
+ // As in HA case, we need to pass om service ID.
+ certClient = OzoneClientFactory.getRpcClient(omServiceID,
+ config);
+ }
+ try {
+ ServiceInfoEx serviceInfoEx = certClient
+ .getObjectStore()
+ .getClientProxy()
+ .getOzoneManagerClient()
+ .getServiceInfo();
+
+ if (OzoneSecurityUtil.isSecurityEnabled(conf)) {
+ String caCertPem = null;
+ List<String> caCertPems = null;
+ caCertPem = serviceInfoEx.getCaCertificate();
+ caCertPems = serviceInfoEx.getCaCertPemList();
+ if (caCertPems == null || caCertPems.isEmpty()) {
+ caCertPems = Collections.singletonList(caCertPem);
Review comment:
Thanks @hanishakoneru for following up on this null check. Yes, we were discussing the case where `caCertPems` is null and if it would be guaranteed to have at least one of `caCertPems` or `caCertPem` to be non null. We came up with that we can't make the assumption so to handle that case here ourselves.
In this case with the Precondition check, it is after we determine that `caCertPems` is empty or null (`caCertPem` can't be null here). In this case we need the `caCertPem` to be non-null or else it causes a NPE in the following `OzoneSecurityUtil.convertToX509 `call. I put in the precondition check to catch this null violation to intentionally throw an exception so the caller knows that in this case the `caCertPem` must be non null.
What do think if we,
i.) update the code to check the precondition that `caCertPem` is not null in the case that `caCertPems` is either empty or null with a message such as,
`Preconditions.checkNotNull(caCertPem, "caCertPem can not be null when caCertPems is empty or null");`
_OR_,
ii.) log the error and throw an exception.
However if we just log the error (`caCertPem` is null) and continue, unfortunately the call that follows to `OzoneSecurityUtil.convertToX509` will throw a NPE.
--
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 #2945: HDDS-5545. Enable TLS for GRPC OmTransport implementation
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #2945:
URL: https://github.com/apache/ozone/pull/2945#discussion_r824936278
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/GrpcOmTransport.java
##########
@@ -93,6 +104,22 @@ public void start() {
.usePlaintext()
.maxInboundMessageSize(maxSize);
+ if (secConfig.isGrpcTlsEnabled()) {
+ try {
+ SslContextBuilder sslContextBuilder = GrpcSslContexts.forClient();
+ if (caCerts != null) {
+ sslContextBuilder.trustManager(caCerts);
+ } else {
+ LOG.error("x509Certicates empty");
Review comment:
If TLS is enabled and caCerts is empty, wouldn't authentication fail?
##########
File path: hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/OzoneClientCache.java
##########
@@ -75,8 +99,66 @@ public static OzoneClient getOzoneClientInstance(OzoneConfiguration
return instance.client;
}
+ public static void closeClient() throws IOException {
+ if (instance != null) {
+ instance.client.close();
+ instance = null;
+ }
+ }
+
+ private void setCertificate(String omServiceID,
+ OzoneConfiguration conf)
+ throws IOException {
+
+ // create local copy of config incase exception occurs
+ // with certificate OmRequest
+ OzoneConfiguration config = new OzoneConfiguration(conf);
+ OzoneClient certClient;
+
+ if (secConfig.isGrpcTlsEnabled()) {
+ // set OmTransport to hadoop rpc to securely,
+ // get certificates with service list request
+ config.set(OZONE_OM_TRANSPORT_CLASS,
+ OZONE_OM_TRANSPORT_CLASS_DEFAULT);
+
+ if (omServiceID == null) {
+ certClient = OzoneClientFactory.getRpcClient(config);
+ } else {
+ // As in HA case, we need to pass om service ID.
+ certClient = OzoneClientFactory.getRpcClient(omServiceID,
+ config);
+ }
+ try {
+ ServiceInfoEx serviceInfoEx = certClient
+ .getObjectStore()
+ .getClientProxy()
+ .getOzoneManagerClient()
+ .getServiceInfo();
+
+ if (OzoneSecurityUtil.isSecurityEnabled(conf)) {
+ String caCertPem = null;
+ List<String> caCertPems = null;
+ caCertPem = serviceInfoEx.getCaCertificate();
+ caCertPems = serviceInfoEx.getCaCertPemList();
+ if (caCertPems == null || caCertPems.isEmpty()) {
+ caCertPems = Collections.singletonList(caCertPem);
Review comment:
It is possible that caCertPem could also be null.
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/GrpcOzoneManagerServer.java
##########
@@ -46,24 +58,42 @@ public GrpcOzoneManagerServer(OzoneConfiguration config,
OzoneManagerProtocolServerSideTranslatorPB
omTranslator,
OzoneDelegationTokenSecretManager
- delegationTokenMgr) {
+ delegationTokenMgr,
+ CertificateClient caClient) {
this.port = config.getObject(
GrpcOzoneManagerServerConfig.class).
getPort();
init(omTranslator,
delegationTokenMgr,
- config);
+ config,
+ caClient);
}
public void init(OzoneManagerProtocolServerSideTranslatorPB omTranslator,
OzoneDelegationTokenSecretManager delegationTokenMgr,
- OzoneConfiguration omServerConfig) {
+ OzoneConfiguration omServerConfig,
+ CertificateClient caClient) {
NettyServerBuilder nettyServerBuilder = NettyServerBuilder.forPort(port)
.maxInboundMessageSize(OzoneConsts.OZONE_SCM_CHUNK_MAX_SIZE)
.addService(new OzoneManagerServiceGrpc(omTranslator,
delegationTokenMgr,
omServerConfig));
+ SecurityConfig secConf = new SecurityConfig(omServerConfig);
+ if (secConf.isGrpcTlsEnabled()) {
Review comment:
Is is possible that GRPC TLS is enabled but Ozone security is not. What would happen in that case?
--
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 #2945: HDDS-5545. Enable TLS for GRPC OmTransport implementation
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #2945:
URL: https://github.com/apache/ozone/pull/2945#discussion_r826237044
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/GrpcOmTransport.java
##########
@@ -93,6 +104,22 @@ public void start() {
.usePlaintext()
.maxInboundMessageSize(maxSize);
+ if (secConfig.isGrpcTlsEnabled()) {
+ try {
+ SslContextBuilder sslContextBuilder = GrpcSslContexts.forClient();
+ if (caCerts != null) {
+ sslContextBuilder.trustManager(caCerts);
+ } else {
+ LOG.error("x509Certicates empty");
Review comment:
So if the client request cannot be authenticated, would it not be better to fail the client request here itself instead of when it reaches the max retries?
--
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 #2945: HDDS-5545. Enable TLS for GRPC OmTransport implementation
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on pull request #2945:
URL: https://github.com/apache/ozone/pull/2945#issuecomment-1082215533
Thanks @neils-dev. I will merge the PR.
--
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] neils-dev commented on a change in pull request #2945: HDDS-5545. Enable TLS for GRPC OmTransport implementation
Posted by GitBox <gi...@apache.org>.
neils-dev commented on a change in pull request #2945:
URL: https://github.com/apache/ozone/pull/2945#discussion_r825234575
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/GrpcOzoneManagerServer.java
##########
@@ -46,24 +58,42 @@ public GrpcOzoneManagerServer(OzoneConfiguration config,
OzoneManagerProtocolServerSideTranslatorPB
omTranslator,
OzoneDelegationTokenSecretManager
- delegationTokenMgr) {
+ delegationTokenMgr,
+ CertificateClient caClient) {
this.port = config.getObject(
GrpcOzoneManagerServerConfig.class).
getPort();
init(omTranslator,
delegationTokenMgr,
- config);
+ config,
+ caClient);
}
public void init(OzoneManagerProtocolServerSideTranslatorPB omTranslator,
OzoneDelegationTokenSecretManager delegationTokenMgr,
- OzoneConfiguration omServerConfig) {
+ OzoneConfiguration omServerConfig,
+ CertificateClient caClient) {
NettyServerBuilder nettyServerBuilder = NettyServerBuilder.forPort(port)
.maxInboundMessageSize(OzoneConsts.OZONE_SCM_CHUNK_MAX_SIZE)
.addService(new OzoneManagerServiceGrpc(omTranslator,
delegationTokenMgr,
omServerConfig));
+ SecurityConfig secConf = new SecurityConfig(omServerConfig);
+ if (secConf.isGrpcTlsEnabled()) {
Review comment:
Normal configuration would be for _ozone.security.enabled_ **and** _hdds.grpc.tls.enabled_ set to _'true'_ as is the case for our docker secure cluster configurations.
In the event GRPC TLS is enabled but Ozone security is not, the certificate list (TLS authenication) used by the client (`GrpcOmTransport`) will not be available (set to null) and the client will not be able to authenticate the server certificate to establish a secure channel between client and server. On the server side (`GrpcOzoneManagerServer`) receives a connection error. The error is logged on the client side as `"x509Certicates empty"` and the client receives a 500 http response. The log and errors should provide useful info to the admin to correct configuration errors.
--
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] neils-dev commented on a change in pull request #2945: HDDS-5545. Enable TLS for GRPC OmTransport implementation
Posted by GitBox <gi...@apache.org>.
neils-dev commented on a change in pull request #2945:
URL: https://github.com/apache/ozone/pull/2945#discussion_r825235685
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/GrpcOmTransport.java
##########
@@ -93,6 +104,22 @@ public void start() {
.usePlaintext()
.maxInboundMessageSize(maxSize);
+ if (secConfig.isGrpcTlsEnabled()) {
+ try {
+ SslContextBuilder sslContextBuilder = GrpcSslContexts.forClient();
+ if (caCerts != null) {
+ sslContextBuilder.trustManager(caCerts);
+ } else {
+ LOG.error("x509Certicates empty");
Review comment:
Thanks @hanishakoneru for looking at this and for your comments.
If `grpcTlsEnabled` and the caCerts is empty, then the channel connection between the s3g client (`GrpcOmTransport`) and the om will not be authenticated by the client. In this case, the error will be logged and the connection will fail. The client receives a 500 http response "An error occurred (500) (reached max retries)".
The logged error as a result of TLS enabled with empty caCerts logs that the `"x509Certicates empty"`. That and the client request failure should provide /alert the necessary info for the admin.
--
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] neils-dev commented on a change in pull request #2945: HDDS-5545. Enable TLS for GRPC OmTransport implementation
Posted by GitBox <gi...@apache.org>.
neils-dev commented on a change in pull request #2945:
URL: https://github.com/apache/ozone/pull/2945#discussion_r835584607
##########
File path: hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/OzoneClientCache.java
##########
@@ -75,8 +99,66 @@ public static OzoneClient getOzoneClientInstance(OzoneConfiguration
return instance.client;
}
+ public static void closeClient() throws IOException {
+ if (instance != null) {
+ instance.client.close();
+ instance = null;
+ }
+ }
+
+ private void setCertificate(String omServiceID,
+ OzoneConfiguration conf)
+ throws IOException {
+
+ // create local copy of config incase exception occurs
+ // with certificate OmRequest
+ OzoneConfiguration config = new OzoneConfiguration(conf);
+ OzoneClient certClient;
+
+ if (secConfig.isGrpcTlsEnabled()) {
+ // set OmTransport to hadoop rpc to securely,
+ // get certificates with service list request
+ config.set(OZONE_OM_TRANSPORT_CLASS,
+ OZONE_OM_TRANSPORT_CLASS_DEFAULT);
+
+ if (omServiceID == null) {
+ certClient = OzoneClientFactory.getRpcClient(config);
+ } else {
+ // As in HA case, we need to pass om service ID.
+ certClient = OzoneClientFactory.getRpcClient(omServiceID,
+ config);
+ }
+ try {
+ ServiceInfoEx serviceInfoEx = certClient
+ .getObjectStore()
+ .getClientProxy()
+ .getOzoneManagerClient()
+ .getServiceInfo();
+
+ if (OzoneSecurityUtil.isSecurityEnabled(conf)) {
+ String caCertPem = null;
+ List<String> caCertPems = null;
+ caCertPem = serviceInfoEx.getCaCertificate();
+ caCertPems = serviceInfoEx.getCaCertPemList();
+ if (caCertPems == null || caCertPems.isEmpty()) {
+ caCertPems = Collections.singletonList(caCertPem);
Review comment:
Thanks @hanishakoneru for your comments on handling the NPE should both the `caCertPems` (list) be empty / null and the `caCertPem` be null.
I've updated the patch to log the error and throw a `CertificateException` should we have this case.
We can also add this type of exception handling in the RpcClient, or bud it where the NPE is found: `OzoneSecurityUtil.java` `convertToX509` method. We can file that separate jira for that. Let me know your thoughts 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.
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 #2945: HDDS-5545. Enable TLS for GRPC OmTransport implementation
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #2945:
URL: https://github.com/apache/ozone/pull/2945#discussion_r808483443
##########
File path: hadoop-ozone/common/pom.xml
##########
@@ -48,6 +48,17 @@ https://maven.apache.org/xsd/maven-4.0.0.xsd">
<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-handler-proxy</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-tcnative-boringssl-static</artifactId>
+ <version>2.0.38.Final</version> <!-- See table for correct version -->
Review comment:
Can you please add the version to variable in the main pom.xml and use that variable here. (For example - io.grpc.version is defined in ozone/pom.xml and reused in ozone-common/pom.xml). It makes it easier to track and update versions.
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OmTransportFactory.java
##########
@@ -34,26 +37,37 @@ OmTransport createOmTransport(ConfigurationSource source,
static OmTransport create(ConfigurationSource conf,
UserGroupInformation ugi, String omServiceId) throws IOException {
- OmTransportFactory factory = createFactory();
+ OmTransportFactory factory = createFactory(conf);
return factory.createOmTransport(conf, ugi, omServiceId);
}
- static OmTransportFactory createFactory() throws IOException {
- ServiceLoader<OmTransportFactory> transportFactoryServiceLoader =
- ServiceLoader.load(OmTransportFactory.class);
- Iterator<OmTransportFactory> iterator =
- transportFactoryServiceLoader.iterator();
- if (iterator.hasNext()) {
- return iterator.next();
- }
+ static OmTransportFactory createFactory(ConfigurationSource conf)
+ throws IOException {
try {
+ // if configured transport class is different than the default
+ // Hadoop3OmTransportFactory, then check service loader for
+ // transport class and instantiate it
+ if (conf
+ .get(OZONE_OM_TRANSPORT_CLASS,
+ OZONE_OM_TRANSPORT_CLASS_DEFAULT) !=
+ OZONE_OM_TRANSPORT_CLASS_DEFAULT) {
Review comment:
This check is not very robust. If the default transport factory is changed, then it would still end up loading Hadoop3OmTransportFactory instead of the new default.
##########
File path: hadoop-ozone/common/pom.xml
##########
@@ -48,6 +48,17 @@ https://maven.apache.org/xsd/maven-4.0.0.xsd">
<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-handler-proxy</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-tcnative-boringssl-static</artifactId>
+ <version>2.0.38.Final</version> <!-- See table for correct version -->
+ <scope>runtime</scope>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-tcnative</artifactId>
+ <version>2.0.38.Final</version> <!-- See table for correct version -->
Review comment:
Which table is being referred to here? I didn't understand this comment here. Is 2.0.38 not the final version which needs to be used here?
--
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] adoroszlai commented on a change in pull request #2945: HDDS-5545. Enable TLS for GRPC OmTransport implementation
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2945:
URL: https://github.com/apache/ozone/pull/2945#discussion_r811620164
##########
File path: hadoop-ozone/common/pom.xml
##########
@@ -48,6 +48,17 @@ https://maven.apache.org/xsd/maven-4.0.0.xsd">
<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-handler-proxy</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-tcnative-boringssl-static</artifactId>
+ <version>2.0.38.Final</version> <!-- See table for correct version -->
+ <scope>runtime</scope>
+ </dependency>
+ <dependency>
+ <groupId>io.netty</groupId>
+ <artifactId>netty-tcnative</artifactId>
+ <version>2.0.38.Final</version> <!-- See table for correct version -->
Review comment:
I think it's this one: https://github.com/grpc/grpc-java/blob/master/SECURITY.md#netty
There is a compatibility table, describing which netty and tcnative versions to use for each gRPC version. Netty and tcnative usually need to be bumped together.
--
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 merged pull request #2945: HDDS-5545. Enable TLS for GRPC OmTransport implementation
Posted by GitBox <gi...@apache.org>.
hanishakoneru merged pull request #2945:
URL: https://github.com/apache/ozone/pull/2945
--
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 #2945: HDDS-5545. Enable TLS for GRPC OmTransport implementation
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on a change in pull request #2945:
URL: https://github.com/apache/ozone/pull/2945#discussion_r826245840
##########
File path: hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/OzoneClientCache.java
##########
@@ -75,8 +99,66 @@ public static OzoneClient getOzoneClientInstance(OzoneConfiguration
return instance.client;
}
+ public static void closeClient() throws IOException {
+ if (instance != null) {
+ instance.client.close();
+ instance = null;
+ }
+ }
+
+ private void setCertificate(String omServiceID,
+ OzoneConfiguration conf)
+ throws IOException {
+
+ // create local copy of config incase exception occurs
+ // with certificate OmRequest
+ OzoneConfiguration config = new OzoneConfiguration(conf);
+ OzoneClient certClient;
+
+ if (secConfig.isGrpcTlsEnabled()) {
+ // set OmTransport to hadoop rpc to securely,
+ // get certificates with service list request
+ config.set(OZONE_OM_TRANSPORT_CLASS,
+ OZONE_OM_TRANSPORT_CLASS_DEFAULT);
+
+ if (omServiceID == null) {
+ certClient = OzoneClientFactory.getRpcClient(config);
+ } else {
+ // As in HA case, we need to pass om service ID.
+ certClient = OzoneClientFactory.getRpcClient(omServiceID,
+ config);
+ }
+ try {
+ ServiceInfoEx serviceInfoEx = certClient
+ .getObjectStore()
+ .getClientProxy()
+ .getOzoneManagerClient()
+ .getServiceInfo();
+
+ if (OzoneSecurityUtil.isSecurityEnabled(conf)) {
+ String caCertPem = null;
+ List<String> caCertPems = null;
+ caCertPem = serviceInfoEx.getCaCertificate();
+ caCertPems = serviceInfoEx.getCaCertPemList();
+ if (caCertPems == null || caCertPems.isEmpty()) {
+ caCertPems = Collections.singletonList(caCertPem);
Review comment:
I don't think it is guaranteed. I commented about it as I am not sure how SslContextBuilder handles a list with null value and if we need to handle that case from our end.
--
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 #2945: HDDS-5545. Enable TLS for GRPC OmTransport implementation
Posted by GitBox <gi...@apache.org>.
hanishakoneru commented on pull request #2945:
URL: https://github.com/apache/ozone/pull/2945#issuecomment-1074478984
Thanks for updating the patch @neils-dev.
I would request you to avoid force-pushing to PRs. It makes it hard for the reviewer as they cannot see what updates have been done since the last review.
--
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