On Wed, 18 Sep 2024 21:44:08 GMT, Valerie Peng <valer...@openjdk.org> wrote:

>> Kevin Driver has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   refinement of addIKM and addSalt specifications
>
> src/java.base/share/classes/com/sun/crypto/provider/HKDFKeyDerivation.java 
> line 92:
> 
>> 90:         }
>> 91:         this.hmacAlgName = hmacAlgName;
>> 92:         this.hmacLen = hmacLen;
> 
> Instead of doing a binary search whenever an HKDFKeyDerivation object is 
> constructed, it is better to organize the algorithm and output length into an 
> enum, this way, supporting new Hmac algorithms would require adding new enum 
> value. This should be sufficient since these arguments are internally 
> supplied. For example,
> 
> public enum SupportedHmac {
>         SHA256("HmacSHA256", 32),
>         SHA384("HmacSHA384", 48),
>         SHA512("HmacSHA512", 64);
> 
>         public final String algo;
>         public final int outLen;
>         private SupportedHmac(String algo, int outLen) {
>             this.algo = algo;
>             this.outLen = outLen;
>         }
>  };

Then the constructor can specify enum as the argument, e.g. 

    private HKDFKeyDerivation(SupportedHmac h,
                              KDFParameters kdfParameters)
            throws InvalidAlgorithmParameterException {
        super(kdfParameters);
        if (kdfParameters != null) {
            throw new InvalidAlgorithmParameterException(
                    h.algo + " does not support parameters");
        }
        this.hmacAlgName = h.algo;
        this.hmacLen = h.outLen;
    }

> src/java.base/share/classes/com/sun/crypto/provider/HKDFKeyDerivation.java 
> line 396:
> 
>> 394:         public HKDFSHA256(KDFParameters kdfParameters)
>> 395:                 throws InvalidAlgorithmParameterException {
>> 396:             super("HmacSHA256", SHA256_HMAC_SIZE, kdfParameters);
> 
> Using the enum, this line would be:
> `            super(SupportedHmac.SHA256, kdfParameters);`

Similar changes to the other child classes below.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1765767539
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1765769023

Reply via email to