RFR: 8343232: PKCS#12 KeyStore support for RFC 9879: Use of Password-Based Message Authentication Code 1 (PBMAC1) [v11]

[Mark Powers]

On Mon, 6 Oct 2025 19:04:07 GMT, Sean Mullan <mullan at openjdk.org> wrote:

>> Mark Powers has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   more review comments from Weijun and Sean
>
> src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java line 75:
> 
>> 73: 
>> 74:     // the key derivation function (default is HmacSHA1)
>> 75:     private final ObjectIdentifier kdfAlgo_OID =
> 
> This can just be a local variable in `getEncoded`.

fixed

> src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java line 81:
> 
>> 79:     private int keyLength = -1;
>> 80: 
>> 81:     protected void Init(AlgorithmParameterSpec paramSpec)
> 
> You don't need this method/ctor. You should add one that takes the different components as objects and then generates the bytes. Move the code that is currently doing this out of `MacData`.

It's long gone.

> src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java line 97:
> 
>> 95:                     + "not an ASN.1 SEQUENCE tag");
>> 96:         }
>> 97:         DerValue[] Info = (new DerInputStream(pBMAC1_params.toByteArray()))
> 
> No need for parens around `new DerInputStream`.
> 
> Also, s/Info/info/

fixed

> src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java line 138:
> 
>> 136:     }
>> 137: 
>> 138:     protected byte[] getEncoded() throws IOException {
> 
> The methods don't need to be `protected`. Make them package-private after you move the class to `sun.security.pkcs12`.

fixed

> src/java.base/share/classes/com/sun/crypto/provider/PBMAC1Parameters.java line 172:
> 
>> 170:     protected byte[] getEncoded(String encodingMethod) throws IOException {
>> 171:         return getEncoded();
>> 172:     }
> 
> This method doesn't seem useful or needed.

gone

> src/java.base/share/classes/sun/security/pkcs12/MacData.java line 179:
> 
>> 177:     }
>> 178: 
>> 179:     static Mac getMac(String macAlgorithm, char[] password,
> 
> This method can be private.

done

> src/java.base/share/classes/sun/security/pkcs12/MacData.java line 204:
> 
>> 202:         try {
>> 203:             if (macAlgorithm.startsWith("PBEWith")) {
>> 204:                  m.init(pbeKey);
> 
> should be 4 spaces indent instead of 5 spaces, also on line 206.

Fixed. You must have special eyes to catch this.

> src/java.base/share/classes/sun/security/pkcs12/MacData.java line 244:
> 
>> 242:      * create a message authentication code (MAC)
>> 243:      */
>> 244:     public static byte[] calculateMac(char[] passwd, byte[] data,
> 
> Method should be package-private.

fixed

> src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 84:
> 
>> 82: 
>> 83:     // the pseudorandom function (default is HmacSHA1)
>> 84:     private ObjectIdentifier kdfAlgo_OID =
> 
> I agree with Valerie that it looks like it can be a local variable inside the constructor on line 125.

fixed

> src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 148:
> 
>> 146:      *
>> 147:      * @return the salt. Returns a new array
>> 148:      * each time this method is called.
> 
> Does not return a new array each time called so remove this comment.

fixed

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2409138459
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2409138117
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2409137896
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2409137013
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2409137544
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2409136445
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2409136625
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2409135754
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2409138788
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2409138659