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/02/01 18:12:52 UTC
[GitHub] [ozone] elek opened a new pull request #1871: HDDS-4763. Owner field of S3AUTHINFO type delegation token should be validated
elek opened a new pull request #1871:
URL: https://github.com/apache/ozone/pull/1871
## What changes were proposed in this pull request?
Adding a check for S3AUTH token validation.
Please see the jira for the motivation.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4763
## How was this patch tested?
Test method is explained in the Jira (with the code to test 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.
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] xiaoyuyao merged pull request #1871: HDDS-4763. Owner field of S3AUTHINFO type delegation token should be validated
Posted by GitBox <gi...@apache.org>.
xiaoyuyao merged pull request #1871:
URL: https://github.com/apache/ozone/pull/1871
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xiaoyuyao commented on a change in pull request #1871: HDDS-4763. Owner field of S3AUTHINFO type delegation token should be validated
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1871:
URL: https://github.com/apache/ozone/pull/1871#discussion_r568126597
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/security/OzoneDelegationTokenSecretManager.java
##########
@@ -472,6 +472,19 @@ public boolean verifySignature(OzoneTokenIdentifier identifier,
private byte[] validateS3AuthInfo(OzoneTokenIdentifier identifier)
throws InvalidToken {
LOG.trace("Validating S3AuthInfo for identifier:{}", identifier);
+ if (identifier.getOwner() == null) {
+ throw new InvalidToken(
+ "Owner is missing from the S3 auth token");
+ }
+ if (!identifier.getOwner().toString().equals(identifier.getAwsAccessId())) {
Review comment:
Thanks @elek for working on this. I have one question about the awsaccessId from identifier or owner, they can all be modified by the attacker as well. What we need to check is compare awsaccessId from the identifier.stringtosign using s3 signatureparser (after verify the secret) with the one from the owner/awsid from the identifier.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] elek commented on a change in pull request #1871: HDDS-4763. Owner field of S3AUTHINFO type delegation token should be validated
Posted by GitBox <gi...@apache.org>.
elek commented on a change in pull request #1871:
URL: https://github.com/apache/ozone/pull/1871#discussion_r568416516
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/security/OzoneDelegationTokenSecretManager.java
##########
@@ -472,6 +472,19 @@ public boolean verifySignature(OzoneTokenIdentifier identifier,
private byte[] validateS3AuthInfo(OzoneTokenIdentifier identifier)
throws InvalidToken {
LOG.trace("Validating S3AuthInfo for identifier:{}", identifier);
+ if (identifier.getOwner() == null) {
+ throw new InvalidToken(
+ "Owner is missing from the S3 auth token");
+ }
+ if (!identifier.getOwner().toString().equals(identifier.getAwsAccessId())) {
Review comment:
_awsaccessId_ is used as a primary key to find the stored secret in the rockdb. If you use any wrong accessId the signature validation will be failed. If the string2sign contains reference to a different accessId, the validation will be failed as the signature won't be matched.
(At least this is my understanding, but let me know if I missed something...)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xiaoyuyao commented on a change in pull request #1871: HDDS-4763. Owner field of S3AUTHINFO type delegation token should be validated
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1871:
URL: https://github.com/apache/ozone/pull/1871#discussion_r568849894
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/security/OzoneDelegationTokenSecretManager.java
##########
@@ -472,6 +472,19 @@ public boolean verifySignature(OzoneTokenIdentifier identifier,
private byte[] validateS3AuthInfo(OzoneTokenIdentifier identifier)
throws InvalidToken {
LOG.trace("Validating S3AuthInfo for identifier:{}", identifier);
+ if (identifier.getOwner() == null) {
+ throw new InvalidToken(
+ "Owner is missing from the S3 auth token");
+ }
+ if (!identifier.getOwner().toString().equals(identifier.getAwsAccessId())) {
Review comment:
Make sense to me. I further check the hadoop rpc code and the UGI of token user is taken from the owner field of the identifier. Because S3 token identifier is not protected with a token signature like Hadoop delegation token, the proposed check of owner against awsaccessid is the right fix.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xiaoyuyao merged pull request #1871: HDDS-4763. Owner field of S3AUTHINFO type delegation token should be validated
Posted by GitBox <gi...@apache.org>.
xiaoyuyao merged pull request #1871:
URL: https://github.com/apache/ozone/pull/1871
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] elek commented on a change in pull request #1871: HDDS-4763. Owner field of S3AUTHINFO type delegation token should be validated
Posted by GitBox <gi...@apache.org>.
elek commented on a change in pull request #1871:
URL: https://github.com/apache/ozone/pull/1871#discussion_r568416516
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/security/OzoneDelegationTokenSecretManager.java
##########
@@ -472,6 +472,19 @@ public boolean verifySignature(OzoneTokenIdentifier identifier,
private byte[] validateS3AuthInfo(OzoneTokenIdentifier identifier)
throws InvalidToken {
LOG.trace("Validating S3AuthInfo for identifier:{}", identifier);
+ if (identifier.getOwner() == null) {
+ throw new InvalidToken(
+ "Owner is missing from the S3 auth token");
+ }
+ if (!identifier.getOwner().toString().equals(identifier.getAwsAccessId())) {
Review comment:
_awsaccessId_ is used as a primary key to find the stored secret in the rockdb. If you use any wrong accessId the signature validation will be failed. If the string2sign contains reference to a different accessId, the validation will be failed as the signature won't be matched.
(At least this is my understanding, but let me know if I missed something...)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xiaoyuyao commented on a change in pull request #1871: HDDS-4763. Owner field of S3AUTHINFO type delegation token should be validated
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1871:
URL: https://github.com/apache/ozone/pull/1871#discussion_r568849894
##########
File path: hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/security/OzoneDelegationTokenSecretManager.java
##########
@@ -472,6 +472,19 @@ public boolean verifySignature(OzoneTokenIdentifier identifier,
private byte[] validateS3AuthInfo(OzoneTokenIdentifier identifier)
throws InvalidToken {
LOG.trace("Validating S3AuthInfo for identifier:{}", identifier);
+ if (identifier.getOwner() == null) {
+ throw new InvalidToken(
+ "Owner is missing from the S3 auth token");
+ }
+ if (!identifier.getOwner().toString().equals(identifier.getAwsAccessId())) {
Review comment:
Make sense to me. I further check the hadoop rpc code and the UGI of token user is taken from the owner field of the identifier. Because S3 token identifier is not protected with a token signature like Hadoop delegation token, the proposed check of owner against awsaccessid is the right fix.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org