HDDS-14091. [STS] Deny access if STS token is found in revoked table#9445
HDDS-14091. [STS] Deny access if STS token is found in revoked table#9445ChenSammi merged 6 commits intoapache:HDDS-13323-stsfrom
Conversation
Tejaskriya
left a comment
Tejaskriya
left a comment
There was a problem hiding this comment.
Thanks @fmorg-git, the changes mostly look good, I just had a suggestion for the tests (below).
|
|
||
| final String tempAccessKeyId = stsTokenIdentifier.getTempAccessKeyId(); | ||
| if (tempAccessKeyId == null || tempAccessKeyId.isEmpty()) { | ||
| return false; |
There was a problem hiding this comment.
Although the revoke hit chance is very low, but If metadataManager, revokedStsTokenTable or tempAccessKeyId is null, we'd better throw exception to fail the request, instead of silently ignore these failures and assuming that temp access key ID is not revoked, for none of above cases are normal as expected.
|
|
||
| return revokedStsTokenTable.getIfExist(tempAccessKeyId) != null; | ||
| } catch (Exception e) { | ||
| // Any DB or codec problem is treated as best-effort failure. |
There was a problem hiding this comment.
Whether the revocation check is a best-effort, or strictly enforced, it's an important behavior of revocation, we should highlight it clearly in the design doc. Current doc doesn't have this info.
There was a problem hiding this comment.
updated the design with this commit to clarify that revocation is strictly enforced: 398c9f6
| return false; | ||
| } | ||
|
|
||
| return revokedStsTokenTable.getIfExist(tempAccessKeyId) != null; |
There was a problem hiding this comment.
I think there will not be many rovoked IDs, so we can consider make s3RevokedStsTokenTable table FULL_CACHE, then all revoked items will be kept in memory.
fmorg-git
force-pushed
the
HDDS-14091
branch
from
28e0de8 to
716f750
Compare
| if (tempAccessKeyId == null || tempAccessKeyId.isEmpty()) { | ||
| final String msg = "Could not determine STS revocation: tempAccessKeyId in STSTokenIdentifier is null/empty"; | ||
| LOG.warn(msg); | ||
| throw new OMException(msg, INTERNAL_ERROR); |
There was a problem hiding this comment.
If tempAccessKeyId is null or empty, it's an invalid tempAccessKeyId, INTERNAL_ERROR is not a proper error code. It's better to move this check to verifyAndDecryptToken().
The rest looks good to me.
| public void testEnsureEssentialFieldsArePresentInTokenMissingExpiry() { | ||
| final byte[] encryptionBytes = new byte[5]; | ||
| ThreadLocalRandom.current().nextBytes(encryptionBytes); | ||
| final STSTokenIdentifier tokenIdentifier = new STSTokenIdentifier( |
There was a problem hiding this comment.
Fabian, do you want to replace this line with createTokenWithMissingField too?
There was a problem hiding this comment.
I couldn't use createTokenWithMissingField for expiration since the expiration is set internally via clock.instant(). However, I see how it can be confusing, so I updated the testEnsureEssentialFieldsArePresentInToken* tests so they are consistent.
Tejaskriya
left a comment
Tejaskriya
left a comment
There was a problem hiding this comment.
Thanks for updating this @fmorg-git , LGTM.
ChenSammi
left a comment
ChenSammi
left a comment
There was a problem hiding this comment.
The last commit LGTM, +1.
|
CI consistently failed for last three runs on the same test |
|
I downloaded fmorg-git:HDDS-14091 and run the |
3 participants
Please describe your PR in detail:
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-14091
How was this patch tested?
unit tests (and prototype)