On Thu, 16 Oct 2025 13:29:41 GMT, Weijun Wang <[email protected]> wrote:
>> src/java.base/share/classes/sun/security/pkcs12/PBMAC1Parameters.java line
>> 121:
>>
>>> 119: String kdfHmac, String hmac, byte[] digest) throws
>>> NoSuchAlgorithmException {
>>> 120: if (algName.equals("PBMAC1")) {
>>> 121: return new DerOutputStream().write(DerValue.tag_Sequence,
>>> new DerOutputStream()
>>
>> This use of method chaining is compact, but I find it much harder to review.
>> Consider breaking it up into something more readable. Also, some of the
>> lines are quite long.
>
> Oh, I suggested this format so it maps almost line to line with the actual
> encoding:
>
> SEQUENCE
> SEQUENCE
> SEQUENCE
> OID 1.2.840.113549.1.5.14 (PBMAC1)
> SEQUENCE
> SEQUENCE
> OID 1.2.840.113549.1.5.12 (PBKDF2WithHmacSHA1)
> SEQUENCE
> OCTET STRING (20 bytes)
> INTEGER 10000
> INTEGER 20
> SEQUENCE
> OID 1.2.840.113549.2.7 (HmacSHA1)
> NULL
> SEQUENCE
> OID 1.2.840.113549.2.7 (HmacSHA1)
> NULL
> OCTET STRING (20 bytes)
> OCTET STRING (523127819, 8 bytes)
> INTEGER 1
>
> But yes, the single large expression does not align well with the multi-level
> ASN.1 grammar at the top of this file. I'm fine with breaking it up to better
> align with that structure.
>
> BTW, the else block is not about PBMAC1. Maybe we should move it back into
> `MacData`.
I believe it is fixed now. The "else block" not about PBMAC1 has been moved
back into MacData.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2453793320
