-
Notifications
You must be signed in to change notification settings - Fork 492
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HDDS-7831. Use symmetric secret key to sign and verify token #4417
HDDS-7831. Use symmetric secret key to sign and verify token #4417
Conversation
56d7d79
to
77bd133
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @duongkame, thank you for working on the patch, overall it looks good, but please find a few inline comments below that I would like to either discuss or to be addressed before we get this in.
...ds/common/src/main/java/org/apache/hadoop/hdds/security/token/ShortLivedTokenIdentifier.java
Outdated
Show resolved
Hide resolved
@@ -51,6 +53,20 @@ public class OzoneBlockTokenIdentifier extends ShortLivedTokenIdentifier { | |||
private EnumSet<AccessModeProto> modes; | |||
private long maxLength; | |||
|
|||
public OzoneBlockTokenIdentifier( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering if we really need both of these constructor... I understand that we have two from the older version as well, but the only difference is how we get the blockId parameter, as a BlockID or as a String.
The String representation is used in the protocol, I believe we should not use it elsewhere, but apparently we are doing it right now. I think this we might address along with the series of changes that we are doing to go from asymmetric to symmetric keys. What do you think?
I don't want to address this right now, but should we go ahead and have a task for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got the concern. I think the internal logic of OzoneBlockTokenId always favor a string presentation. The BlockId if passed is converted to a "service id".
public static String getTokenService(BlockID blockID) {
return String.valueOf(blockID.getContainerBlockID());
}
and this "service id" is used to verify the token .
// check token service (blockID or containerID)
String service = String.valueOf(getService(cmd));
if (!Objects.equals(service, tokenId.getService())) {
throw new BlockTokenException("ID mismatch. Token for ID: " +
tokenId.getService() + " can't be used to access: " + service +
" by user: " + tokenUser);
}
hadoop-hdds/framework/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker
Outdated
Show resolved
Hide resolved
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneSecurityUtil.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
Outdated
Show resolved
Hide resolved
...ework/src/test/java/org/apache/hadoop/hdds/security/token/TestOzoneBlockTokenIdentifier.java
Outdated
Show resolved
Hide resolved
hadoop-hdds/interface-admin/src/main/proto/ScmAdminProtocol.proto
Outdated
Show resolved
Hide resolved
@@ -955,8 +953,8 @@ private ContainerTokenSecretManager createContainerTokenSecretManager( | |||
scmCertificateClient = new SCMCertificateClient(securityConfig, | |||
certSerialNumber, SCM_ROOT_CA_COMPONENT_NAME); | |||
} | |||
return new ContainerTokenSecretManager(securityConfig, | |||
expiryTime); | |||
return new ContainerTokenSecretManager(expiryTime, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this code piece, there is the regular checks for certificate lifetime, and existence. Do we still need those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the check you mention is in the DefaultCertificateClient, and it's independent from the implementation of ContainerTokenSecretManager
.
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/ozone/container/common/TestEndPoint.java
Outdated
Show resolved
Hide resolved
@duongkame , thanks for working on this. It's really quite a big patch. Can we separate it into small ones? It's hard to find the exact piece of code line to add comments. |
Thanks for the helpful review @fapifta. I've looked at the inline comments. |
Thanks for looking at this @ChenSammi. At the beginning I tried to deliver in smaller pieces, e.g. separate token generator and verifier, or block and container tokens... But I couldn't figure out a good way to do that because the logic parts are very coherent. E.g. the token verifier depends on the generator, block token and container token share the same root... Anyway, the number of changed files are huge because of many adapted test cases. The number of core changes are limited in the diagram in the PR description. |
Yeah it's a large patch. But looks like it may be hard to separate as small pieces as lot of the files touched due to passing over the variables etc. Just wondering whether we can just provide implementation for some of the new classes like SecretKey*Client etc as this is in branch. That may not reduce much I guess. Please take care of avoid touching with some unrelated changes here as this patch is already big ex: OzoneSecurityUtil.java |
@fapifta and @ChenSammi do you have further comments on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went over this PR once more, there are clean up tasks that need to be done and renames for classes, mergint this in as the change looks good and there will be follow up tasks for this project.
* master: (73 commits) HDDS-8587. Test that CertificateClient can store multiple rootCA certificates (apache#4852) HDDS-8801. ReplicationManager: Add metric to count how often replication is throttled (apache#4864) HDDS-8477. Unit test for Snapdiff using tombstone entries (apache#4678) HDDS-7507. [Snapshot] Implement List Snapshot API Pagination (apache#4065) (apache#4861) HDDS-8373. Document that setquota doesn't accept decimals (apache#4856) HDDS-8779. Recon - Expose flag for enable/disable of heatmap. (apache#4845) HDDS-8677. Ozone admin OM CLI command for block tokens (apache#4760) HDDS-8164. Authorize secret key APIs (apache#4597) HDDS-7945. Integrate secret keys to SCM snapshot (apache#4549) HDDS-8003. E2E integration test cases for block tokens (apache#4547) HDDS-7831. Use symmetric secret key to sign and verify token (apache#4417) HDDS-7830. SCM API for OM and Datanode to get secret keys (apache#4345) HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM (apache#4194) HDDS-8679. Add dedicated, configurable thread pool for OM gRPC server (apache#4771) HDDS-8790. Split EC acceptance tests (apache#4855) HDDS-8714. TestScmHAFinalization: mark testFinalizationWithRestart as flaky, enable other test cases HDDS-8787. Reduce ozone sh calls in robot tests (apache#4854) HDDS-8774. Log allocation stack trace for leaked CodecBuffer (apache#4840) HDDS-8729. Add metric for count of blocks pending deletion on datanode (apache#4800) HDDS-8780. Leak of ManagedChannel in HASecurityUtils (apache#4850) ...
* tmp-dir-refactor: (73 commits) HDDS-8587. Test that CertificateClient can store multiple rootCA certificates (apache#4852) HDDS-8801. ReplicationManager: Add metric to count how often replication is throttled (apache#4864) HDDS-8477. Unit test for Snapdiff using tombstone entries (apache#4678) HDDS-7507. [Snapshot] Implement List Snapshot API Pagination (apache#4065) (apache#4861) HDDS-8373. Document that setquota doesn't accept decimals (apache#4856) HDDS-8779. Recon - Expose flag for enable/disable of heatmap. (apache#4845) HDDS-8677. Ozone admin OM CLI command for block tokens (apache#4760) HDDS-8164. Authorize secret key APIs (apache#4597) HDDS-7945. Integrate secret keys to SCM snapshot (apache#4549) HDDS-8003. E2E integration test cases for block tokens (apache#4547) HDDS-7831. Use symmetric secret key to sign and verify token (apache#4417) HDDS-7830. SCM API for OM and Datanode to get secret keys (apache#4345) HDDS-7734. Implement symmetric SecretKeys lifescycle management in SCM (apache#4194) HDDS-8679. Add dedicated, configurable thread pool for OM gRPC server (apache#4771) HDDS-8790. Split EC acceptance tests (apache#4855) HDDS-8714. TestScmHAFinalization: mark testFinalizationWithRestart as flaky, enable other test cases HDDS-8787. Reduce ozone sh calls in robot tests (apache#4854) HDDS-8774. Log allocation stack trace for leaked CodecBuffer (apache#4840) HDDS-8729. Add metric for count of blocks pending deletion on datanode (apache#4800) HDDS-8780. Leak of ManagedChannel in HASecurityUtils (apache#4850) ...
What changes were proposed in this pull request?
Use symmetric secret keys to sign and verify (container and block) tokens.
This implements the token signing and verifying following Proposal #2 (secret key pulling model).
The detailed structure of the components looks like below:
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-7831
How was this patch tested?