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/09 19:10:00 UTC
[GitHub] [ozone] kerneltime opened a new pull request #2817: HDDS-5910 Add additional verification for S3 Auth.
kerneltime opened a new pull request #2817:
URL: https://github.com/apache/ozone/pull/2817
## What changes were proposed in this pull request?
Ensure that if the client has asked for S3 style auth, each request
sent has S3 Auth information set.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-5910
## How was this patch tested?
Set the validation to trip always and insure it catches the missing auth.
--
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] kerneltime commented on a change in pull request #2817: HDDS-5910 Add additional verification for S3 Auth.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #2817:
URL: https://github.com/apache/ozone/pull/2817#discussion_r746254698
##########
File path: hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/OzoneClientProducer.java
##########
@@ -112,6 +112,9 @@ public S3Auth getSignature() {
@NotNull
@VisibleForTesting
OzoneClient createOzoneClient() throws IOException {
+ // S3 Gateway should always set the S3 Auth. OM can choose to ignore it
+ // based on the security configuration.
Review comment:
Essentially do not skip setting S3 auth on the client side even if security is not 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] bharatviswa504 commented on pull request #2817: HDDS-5910 Add additional verification for S3 Auth.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #2817:
URL: https://github.com/apache/ozone/pull/2817#issuecomment-965841700
Thank You @kerneltime for the contribution
--
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] bharatviswa504 commented on pull request #2817: HDDS-5910 Add additional verification for S3 Auth.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #2817:
URL: https://github.com/apache/ozone/pull/2817#issuecomment-965734056
> > > > > Adding annotations will not insure that the insertion will occur at the time of invocation of the endpoint without the need to modify each invocation. For now not adding that change. No additional unit tests are needed for signature parsing for now.
> > > >
> > > >
> > > > I think our discussion is to add method level annotation. But this fix will catch issue, we are safe guarded in a way. But I still believe lets have a Jira open to add during method invocation, instead of Object Creation.
> > >
> > >
> > > I do not see an easy way without intercepting the invocation of all the HTTP operations for the annotations to take effect. We can discuss offline but it seems a bit redundant with this change.
> >
> >
> > I see one way is adding annotation to each method which handles HTTP requests or use aspectj library which we are discussing offline with @avijayanhwx. But with the proposed fix it is good, we can catch errors.
>
> Yeah. I don't think bringing in a whole new library to deal with this is prudent at the moment. The current check with protocol check PR #2822 should be sufficient and straightforward
Yes, as said for now we should be good. Lets create a Jira if any one interested can pickup or we can work on when needed if we see any issue.
--
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] bharatviswa504 commented on pull request #2817: HDDS-5910 Add additional verification for S3 Auth.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #2817:
URL: https://github.com/apache/ozone/pull/2817#issuecomment-965674887
> > > Adding annotations will not insure that the insertion will occur at the time of invocation of the endpoint without the need to modify each invocation. For now not adding that change. No additional unit tests are needed for signature parsing for now.
> >
> >
> > I think our discussion is to add method level annotation. But this fix will catch issue, we are safe guarded in a way. But I still believe lets have a Jira open to add during method invocation, instead of Object Creation.
>
> I do not see an easy way without intercepting the invocation of all the HTTP operations for the annotations to take effect. We can discuss offline but it seems a bit redundant with this change.
I see one way is adding annotation to each method which handles HTTP requests or use aspectj library which we are discussing offline with @avijayanhwx. But with the proposed fix it is good, we can catch 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] bharatviswa504 commented on a change in pull request #2817: HDDS-5910 Add additional verification for S3 Auth.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2817:
URL: https://github.com/apache/ozone/pull/2817#discussion_r746925211
##########
File path: hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/OzoneClientProducer.java
##########
@@ -112,6 +112,9 @@ public S3Auth getSignature() {
@NotNull
@VisibleForTesting
OzoneClient createOzoneClient() throws IOException {
+ // S3 Gateway should always set the S3 Auth. OM can choose to ignore it
+ // based on the security configuration.
Review comment:
Got it. But the 2nd part in comment is confusing for readers.
--
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] bharatviswa504 merged pull request #2817: HDDS-5910 Add additional verification for S3 Auth.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 merged pull request #2817:
URL: https://github.com/apache/ozone/pull/2817
--
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] kerneltime commented on a change in pull request #2817: HDDS-5910 Add additional verification for S3 Auth.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #2817:
URL: https://github.com/apache/ozone/pull/2817#discussion_r746159060
##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java
##########
@@ -185,15 +185,21 @@ public RpcClient(ConfigurationSource conf, String omServiceId)
this.clientConfig = conf.getObject(OzoneClientConfig.class);
OmTransport omTransport = createOmTransport(omServiceId);
- this.ozoneManagerClient = TracingUtil.createProxy(
+ OzoneManagerProtocolClientSideTranslatorPB
+ ozoneManagerProtocolClientSideTranslatorPB =
new OzoneManagerProtocolClientSideTranslatorPB(omTransport,
- clientId.toString()),
- OzoneManagerClientProtocol.class, conf
- );
+ clientId.toString());
+ this.ozoneManagerClient = TracingUtil.createProxy(
+ ozoneManagerProtocolClientSideTranslatorPB,
+ OzoneManagerClientProtocol.class, conf);
dtService = omTransport.getDelegationTokenService();
- ServiceInfoEx serviceInfoEx = ozoneManagerClient.getServiceInfo();
List<X509Certificate> x509Certificates = null;
if (OzoneSecurityUtil.isSecurityEnabled(conf)) {
+ ServiceInfoEx serviceInfoEx = ozoneManagerClient.getServiceInfo();
+ // If the client is authenticating using S3 style aut, all future
Review comment:
Thanks! Fixing 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@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] kerneltime commented on a change in pull request #2817: HDDS-5910 Add additional verification for S3 Auth.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #2817:
URL: https://github.com/apache/ozone/pull/2817#discussion_r746158459
##########
File path: hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/OzoneClientProducer.java
##########
@@ -112,6 +112,9 @@ public S3Auth getSignature() {
@NotNull
@VisibleForTesting
OzoneClient createOzoneClient() throws IOException {
+ // S3 Gateway should always set the S3 Auth. OM can choose to ignore it
+ // based on the security configuration.
Review comment:
S3 Clients will always set the authentication information as S3 APIs do not really have a contract for unauthenticated communication. Thus, it is best for S3G to forward the signature to OM. OM can choose to validate the signature or not based on how OM is configured.
--
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] kerneltime commented on a change in pull request #2817: HDDS-5910 Add additional verification for S3 Auth.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #2817:
URL: https://github.com/apache/ozone/pull/2817#discussion_r746942975
##########
File path: hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/OzoneClientProducer.java
##########
@@ -112,6 +112,9 @@ public S3Auth getSignature() {
@NotNull
@VisibleForTesting
OzoneClient createOzoneClient() throws IOException {
+ // S3 Gateway should always set the S3 Auth. OM can choose to ignore it
+ // based on the security configuration.
Review comment:
Addressed, dropped the OM handling bit.
--
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] kerneltime commented on pull request #2817: HDDS-5910 Add additional verification for S3 Auth.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on pull request #2817:
URL: https://github.com/apache/ozone/pull/2817#issuecomment-965699297
> > > > Adding annotations will not insure that the insertion will occur at the time of invocation of the endpoint without the need to modify each invocation. For now not adding that change. No additional unit tests are needed for signature parsing for now.
> > >
> > >
> > > I think our discussion is to add method level annotation. But this fix will catch issue, we are safe guarded in a way. But I still believe lets have a Jira open to add during method invocation, instead of Object Creation.
> >
> >
> > I do not see an easy way without intercepting the invocation of all the HTTP operations for the annotations to take effect. We can discuss offline but it seems a bit redundant with this change.
>
> I see one way is adding annotation to each method which handles HTTP requests or use aspectj library which we are discussing offline with @avijayanhwx. But with the proposed fix it is good, we can catch errors.
Yeah. I don't think bringing in a whole new library to deal with this is prudent at the moment. The current check with protocol check PR https://github.com/apache/ozone/pull/2822 should be sufficient and straightforward
--
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] kerneltime commented on a change in pull request #2817: HDDS-5910 Add additional verification for S3 Auth.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #2817:
URL: https://github.com/apache/ozone/pull/2817#discussion_r746157445
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocol/S3Auth.java
##########
@@ -24,6 +24,7 @@
private String stringToSign;
private String signature;
private String accessID;
+ public static final String S3_AUTH_CHECK = "ozone.s3.auth.check";
Review comment:
Yes that is correct. It is not exposed as a end user config.
--
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] kerneltime commented on pull request #2817: HDDS-5910 Add additional verification for S3 Auth.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on pull request #2817:
URL: https://github.com/apache/ozone/pull/2817#issuecomment-964674899
> > Adding annotations will not insure that the insertion will occur at the time of invocation of the endpoint without the need to modify each invocation. For now not adding that change. No additional unit tests are needed for signature parsing for now.
>
> I think our discussion is to add method level annotation. But this fix will catch issue, we are safe guarded in a way. But I still believe lets have a Jira open to add during method invocation, instead of Object Creation.
I do not see an easy way without intercepting the invocation of all the HTTP operations for the annotations to take effect. We can discuss offline but it seems a bit redundant with this change.
--
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] bharatviswa504 commented on a change in pull request #2817: HDDS-5910 Add additional verification for S3 Auth.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2817:
URL: https://github.com/apache/ozone/pull/2817#discussion_r746153098
##########
File path: hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/OzoneClientProducer.java
##########
@@ -112,6 +112,9 @@ public S3Auth getSignature() {
@NotNull
@VisibleForTesting
OzoneClient createOzoneClient() throws IOException {
+ // S3 Gateway should always set the S3 Auth. OM can choose to ignore it
+ // based on the security configuration.
Review comment:
Can you explain what this comment means?
OM can choose to ignore it
// based on the security configuration.
##########
File path: hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocol/S3Auth.java
##########
@@ -24,6 +24,7 @@
private String stringToSign;
private String signature;
private String accessID;
+ public static final String S3_AUTH_CHECK = "ozone.s3.auth.check";
Review comment:
This will be S3Gateway config, but it is internal should not be exposed to end users if i understand correctly.
##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java
##########
@@ -185,15 +185,21 @@ public RpcClient(ConfigurationSource conf, String omServiceId)
this.clientConfig = conf.getObject(OzoneClientConfig.class);
OmTransport omTransport = createOmTransport(omServiceId);
- this.ozoneManagerClient = TracingUtil.createProxy(
+ OzoneManagerProtocolClientSideTranslatorPB
+ ozoneManagerProtocolClientSideTranslatorPB =
new OzoneManagerProtocolClientSideTranslatorPB(omTransport,
- clientId.toString()),
- OzoneManagerClientProtocol.class, conf
- );
+ clientId.toString());
+ this.ozoneManagerClient = TracingUtil.createProxy(
+ ozoneManagerProtocolClientSideTranslatorPB,
+ OzoneManagerClientProtocol.class, conf);
dtService = omTransport.getDelegationTokenService();
- ServiceInfoEx serviceInfoEx = ozoneManagerClient.getServiceInfo();
List<X509Certificate> x509Certificates = null;
if (OzoneSecurityUtil.isSecurityEnabled(conf)) {
+ ServiceInfoEx serviceInfoEx = ozoneManagerClient.getServiceInfo();
+ // If the client is authenticating using S3 style aut, all future
Review comment:
aut -> auth
--
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