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

[Sean Mullan]

On Thu, 2 Oct 2025 18:49:53 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:
> 
>   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`.

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 anymore, only the one that takes a `byte[]`.

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/

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`.

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.

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.

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.

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.

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.

src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 89:

> 87:     private String prfAlgo = "HmacSHA1";
> 88: 
> 89:     public PBKDF2Parameters(DerValue keyDerivationFunc) throws IOException {

Add some comments describing this ctor and its parameter.

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.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2408034141
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2408043209
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2408046719
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2408070382
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2408067504
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2408174703
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2408165609
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2408206621
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2407354430
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2407440344
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2407364493