RFR: 8343232: PKCS#12 KeyStore support for RFC 9879: Use of Password-Based Message Authentication Code 1 (PBMAC1) [v13]
Weijun Wang
weijun at openjdk.org
Wed Oct 15 14:46:29 UTC 2025
On Tue, 14 Oct 2025 23:43:39 GMT, Mark Powers <mpowers at openjdk.org> wrote:
>> [JDK-8343232](https://bugs.openjdk.org/browse/JDK-8343232)
>
> Mark Powers has updated the pull request incrementally with one additional commit since the last revision:
>
> remaining comments
These are mostly style-related comments.
Please add javadoc comments to new methods, especially in `MacData`.
src/java.base/share/classes/com/sun/crypto/provider/PBES2Parameters.java line 251:
> 249: keysize = kdfParams.getKeyLength();
> 250:
> 251: if (pBES2_params.tag != DerValue.tag_Sequence) {
You can probably move this check before line 228. Note that the same check has been performed on line 214 and the only reason to do it again is if `pBES2_params` has been reassigned on line 227.
src/java.base/share/classes/sun/security/pkcs12/MacData.java line 65:
> 63: * }
> 64: *
> 65: * </pre>
Please move the `@author` line to the end of the javadoc comment.
src/java.base/share/classes/sun/security/pkcs12/MacData.java line 135:
> 133: }
> 134:
> 135: private static Mac getMac(String macAlgorithm, char[] password,
I would rather let this method return `mac.doFinal(data)`. The method cleans up `pbeKey` in a finally block and I somehow cannot guarantee it would not have any negative impact when the `Mac` object is still alive.
There are two places this method is called and the `Mac` object is used for other purposes. For one, its `getAlgorithm` is called and used in a debug log. I think the `macAlgorithm` can be used too. The other called `getMacLength` but that is the same of the length of the `doFinal` output.
src/java.base/share/classes/sun/security/pkcs12/MacData.java line 147:
> 145: if (macAlgorithm.startsWith("pbewith")) {
> 146: m = Mac.getInstance(hmac);
> 147: int len = keyLength == 0 ? m.getMacLength()*8 : keyLength;
Somehow I prefer to use `-1` for the default case. This is consistent with `PBKDF2Parameters`.
src/java.base/share/classes/sun/security/pkcs12/MacData.java line 155:
> 153: } else {
> 154: hmac = macAlgorithm;
> 155: m = Mac.getInstance(hmac);
Don't assign to `hmac` and just call `Mac.getInstance(macAlgorithm)`. Mutating `hmac` makes people wondering if it has a purpose.
src/java.base/share/classes/sun/security/pkcs12/MacData.java line 160:
> 158: pbeKey = skf.generateSecret(keySpec);
> 159: }
> 160: keySpec.clearPassword();
Put the line above into the `finally` block at the end. Exceptions could be thrown in the lines above.
This will also help merging the two `if (macAlgorithm.startsWith("pbewith"))` checks into one.
src/java.base/share/classes/sun/security/pkcs12/MacData.java line 208:
> 206: String macAlgorithm, int macIterationCount, byte[] salt)
> 207: throws IOException, NoSuchAlgorithmException {
> 208: final byte[] mData;
No need for `mData`. Just `return bytes.toByteArray()` at the end.
src/java.base/share/classes/sun/security/pkcs12/MacData.java line 210:
> 208: final byte[] mData;
> 209: final PBEParameterSpec params;
> 210: String algName = "PBMAC1";
No need to assign a value now. Assign it in the `if (macAlgorithm.startsWith("pbewith"))` block.
src/java.base/share/classes/sun/security/pkcs12/MacData.java line 214:
> 212: String hmac;
> 213: Mac m;
> 214: int keyLength;
Useless.
src/java.base/share/classes/sun/security/pkcs12/MacData.java line 225:
> 223: } else if (macAlgorithm.startsWith("hmacpbe")) {
> 224: algName = macAlgorithm.substring(7);
> 225: kdfHmac = macAlgorithm;
Assign null to `kdfHmac`. People might wonder it's useful in this case.
src/java.base/share/classes/sun/security/pkcs12/MacData.java line 240:
> 238: DerOutputStream bytes = new DerOutputStream();
> 239: bytes.write(encode(algName, macResult, params, kdfHmac, hmac,
> 240: m.getMacLength()));
`m.getLength()` is the same as `macResult`.
src/java.base/share/classes/sun/security/pkcs12/PBMAC1Parameters.java line 99:
> 97: this.hmacAlgo = o.stdName();
> 98:
> 99: DerValue kdf = pBMAC1_params.data.getDerValue();
This is just `info[0]`. You have already read all sub-components on line 77.
src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line 2074:
> 2072: MacData macData = new MacData(s);
> 2073: int ic = macData.getIterations();
> 2074: byte[] salt = macData.getSalt();
This is useless.
src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 80:
> 78: private final int keyLength;
> 79:
> 80: private String prfAlgo;
This can be final.
src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 86:
> 84: * parameter block.
> 85: *
> 86: * @param keyDerivationFunc the DER encoding of the parameter block
The `@param` name should be `pBKDF2_params`.
src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 152:
> 150: .putOctetString(salt)
> 151: .putInteger(iterationCount)
> 152: .putInteger(keyLength)
When this method is called, `keyLength` can never be -1. Please add a comment or `assert` statement.
-------------
PR Review: https://git.openjdk.org/jdk/pull/24429#pullrequestreview-3340688324
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432766065
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432772229
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432791173
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432797416
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432800846
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432803553
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432821945
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432810417
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432823775
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432818517
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432827317
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432835982
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432837929
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432839534
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432841221
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432845455
More information about the security-dev
mailing list