On Wed, 17 Jul 2024 00:48:20 GMT, Valerie Peng <[email protected]> wrote:
> Can someone help review this fix? Changed the required-mechanism check by
> checking if the particular mechanism is inside the list of enabled supported
> mechanisms. This should be more reliable than calling C_GetMechanismInfo(..)
> on the required mechanism given vendors may return various sorts of error
> codes.
>
> Thanks,
> Valerie
Overall, I like the change because in addition to fixing the bug we will save
some `getMechanismInfo` calls for mechanisms that are supported by the token
but disabled in the configuration. I made a couple of minor comments, though.
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line
1265:
> 1263: (Collectors.toCollection(HashSet::new));
> 1264: if (config.getDisabledMechanisms() != null) {
> 1265: enabledMechSet.removeAll(config.getDisabledMechanisms());
Should `enabledMechSet` be further restricted to `Config::enabledMechanisms`
(if set)? `Config::disabledMechanisms` looks like a fallback for when
`Config::enabledMechanisms` is not set, according to `Config::isEnabled`. I'd
keep the logic that makes `Config::enabledMechanisms` work in pair with
`Config::disabledMechanisms` in a single place, as duplicating it into two
different places may lead to misalignment.
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java line
1297:
> 1295: new HashMap<Descriptor,Integer>();
> 1296:
> 1297: for (long longMech : supportedMechanisms) {
Is the code under `if (!config.isEnabled(longMech)) {` still needed? Looks to
me that we will be iterating over enabled mechanisms now.
-------------
Changes requested by mbalao (Committer).
PR Review: https://git.openjdk.org/jdk/pull/20207#pullrequestreview-2222333684
PR Review Comment: https://git.openjdk.org/jdk/pull/20207#discussion_r1706174632
PR Review Comment: https://git.openjdk.org/jdk/pull/20207#discussion_r1706176318
