On Sat, 18 Oct 2025 20:23:41 GMT, Koushik Muthukrishnan Thirupattur
<[email protected]> wrote:
>> Looking at RFC 9879 on PBES2 and PBMAC1 in PKCS12, algorithm identifiers for
>> HmacSHA*** (like SHA***) should always contain NULL as params. We can update
>> the list at AlgorithmId.encode(DOS) to enforce this rule.
>
> Koushik Muthukrishnan Thirupattur has updated the pull request incrementally
> with one additional commit since the last revision:
>
> 8367008: Algorithm identifiers for HmacSHA* should always have NULL as
> params
Looks good, just a few questions about the test
test/jdk/sun/security/x509/AlgorithmId/AlgorithmIdEqualsHashCode.java line 32:
> 30: */
> 31:
> 32: import java.io.*;
Nit: as you're touching this tests, if there is another commit, could you
please change all wildcard imports in the test to the explicit ones?
test/jdk/sun/security/x509/AlgorithmId/AlgorithmIdEqualsHashCode.java line 42:
> 40: public class AlgorithmIdEqualsHashCode {
> 41:
> 42: static boolean failed = false;
As far as I can see, this is only used as a part of the test. Could you please
make it local variable if it stays at all?
test/jdk/sun/security/x509/AlgorithmId/AlgorithmIdEqualsHashCode.java line 103:
> 101: }
> 102:
> 103: try {
I have 2 questions here:
1. Wouldn't it be easier if it was fail by default and pass only if conditions
are met? In case of the future changes this seems to me to be a bit clearer and
easier to maintain. But i'm ok with how it is now as well.
2. I strongly feel you should throw an exception when this is failed, otherwise
this will never indicate a fail, only log. This way it is also consistent with
all the other code above. However, if you want to keep it as it is, what do you
think about checking the failed status underneath and throwing an exception if
it failed?
-------------
PR Review: https://git.openjdk.org/jdk/pull/27700#pullrequestreview-3353766572
PR Review Comment: https://git.openjdk.org/jdk/pull/27700#discussion_r2442593187
PR Review Comment: https://git.openjdk.org/jdk/pull/27700#discussion_r2442592276
PR Review Comment: https://git.openjdk.org/jdk/pull/27700#discussion_r2442590144
