HDDS-13961. [STS] Encrypt secretAccessKey in session token#9344
HDDS-13961. [STS] Encrypt secretAccessKey in session token#9344sodonnel merged 5 commits intoapache:HDDS-13323-stsfrom
Conversation
fmorg-git
force-pushed
the
HDDS-13961
branch
2 times, most recently
from
09ab4da to
b6f8f5e
Compare
fmorg-git
force-pushed
the
HDDS-13961
branch
from
b6f8f5e to
ed8ae7f
Compare
|
@yandrey321 do you wanna look at this patch? |
| return sessionPolicy; | ||
| } | ||
|
|
||
| public void setEncryptionKey(byte[] encryptionKey) { |
There was a problem hiding this comment.
how are we protecting key in memory? how long would it be stored in memory? We zeroing out buffers but do nothing with key, which is more sensitive.
There was a problem hiding this comment.
the keys are loaded by OM on startup and cached in memory until restart, and are periodically background refreshed. I updated to remove unnecessary zero filling.
fmorg-git
force-pushed
the
HDDS-13961
branch
from
c8a33d3 to
dbdcc9e
Compare
| // AES-GCM parameters | ||
| private static final int GCM_IV_LENGTH = 12; // 96 bits | ||
|
|
||
| private static final SecureRandom SECURE_RANDOM = new SecureRandom(); |
There was a problem hiding this comment.
should we use non-blocking provider like NativePRNGNonBlocking ?
|
|
||
| // Initialize AES-GCM cipher | ||
| final Cipher cipher = Cipher.getInstance("AES/GCM/NoPadding", BC_PROVIDER); | ||
| final GCMParameterSpec spec = new GCMParameterSpec(128, iv); |
There was a problem hiding this comment.
updated (I updated the transformation and algorithm to be constants also.) Also I updated to use non-blocking provider where available.
| * Encrypt a sensitive field using the configured encryption key. | ||
| */ | ||
| private String encryptSensitiveField(String value) { | ||
| if (value == null || value.isEmpty()) { |
There was a problem hiding this comment.
should it be a part of STSTokenEncryption.encrypt() contract?
in this case we dont need this logic in all the client calls
| * Decrypt a sensitive field using the configured encryption key. | ||
| */ | ||
| private String decryptSensitiveField(String encryptedValue) { | ||
| if (encryptedValue == null || encryptedValue.isEmpty()) { |
There was a problem hiding this comment.
shouldn't it be the part of STSTokenEncryption.decrypt() contract?
| </dependency> | ||
| <dependency> | ||
| <groupId>org.bouncycastle</groupId> | ||
| <artifactId>bcprov-jdk18on</artifactId> |
There was a problem hiding this comment.
how would it work with JDK 11/17/21?
There was a problem hiding this comment.
according to Maven, this dependency would work on Java 1.8 and later
| private static final String AES_CIPHER_TRANSFORMATION = "AES/GCM/NoPadding"; | ||
|
|
||
| private static final SecureRandom SECURE_RANDOM; | ||
| private static final BouncyCastleProvider BC_PROVIDER = new BouncyCastleProvider(); |
There was a problem hiding this comment.
is there a reason to use BC provider vs native JDK implementation?
There was a problem hiding this comment.
my understanding is we need to code for Java 1.8, and native JDK doesn't add support for HKDF until Java 11. HKDF is recommended by NIST and used by TLS 1.3.
| * Includes token type, ownerId, expiry millis, and secretKeyId. | ||
| */ | ||
| private byte[] computeAadBytes() { | ||
| final String aad = "v1|S3_STS_TOKEN|" + getOwnerId() + "|" + getExpiry().toEpochMilli() + "|" + |
sodonnel
left a comment
sodonnel
left a comment
There was a problem hiding this comment.
LGTM. We can commit if we get green CI and @yandrey321 is happy.
5 participants
Please describe your PR in detail:
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-13961
How was this patch tested?
unit tests