Re: RFR: 8255409: Support the new APIs in PKCS#11 v3.0
On Mon, 6 Dec 2021 18:50:08 GMT, Anthony Scarpino wrote:
> Should the NSS supporting 3.0 get added to the changeset for testing?
Recall there was a bug filed for updating the artifactory NSS version. There
was some window build issue, will follow up with the SQE RE again.
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11AEADCipher.java
> line 406:
>
>> 404: token.ensureValid();
>> 405: if (token.p11.getVersion().major == 3) {
>> 406: long flags = (encrypt? CKF_ENCRYPT : CKF_DECRYPT);
>
> I think this is a syntax nit with no space between "encrypt" and '?'
Ok.
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java line
> 449:
>
>> 447: token.ensureValid();
>> 448: if (token.p11.getVersion().major == 3) {
>> 449: long flags = (encrypt? CKF_ENCRYPT : CKF_DECRYPT);
>
> I think this is a syntax nit with no space between "encrypt" and '?'
Ok.
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyWrapCipher.java
> line 291:
>
>> 289:
>> 290: if (token.p11.getVersion().major == 3) {
>> 291: long flags = (opmode == Cipher.ENCRYPT_MODE? CKF_ENCRYPT :
>
> I think this is a syntax nit with no space between MODE and '?'
Ok.
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java
> line 285:
>
>> 283:
>> 284: if (token.p11.getVersion().major == 3) {
>> 285: long flags = (mode == M_SIGN? CKF_SIGN : CKF_VERIFY);
>
> M_SIGN ? ...
Ok. BTW, is there a central place for these syntax? I usually just pick up the
convention used in the same file.
-
PR: https://git.openjdk.java.net/jdk/pull/6655
Re: RFR: 8255409: Support the new APIs in PKCS#11 v3.0
On Mon, 6 Dec 2021 17:24:44 GMT, Anthony Scarpino wrote:
>> PKCS#11 v3.0 adds the support for several new APIs. For this particular RFE,
>> it enhances SunPKCS11 provider to load PKCS#11 provider by first trying the
>> C_GetInterface (new in 3.0) before the C_GetFunctionList assuming not
>> explicitly specified in config. In addition, PKCS#11 v3.0 defines a new API
>> for cancelling session operations, so I've also updated various classes to
>> call this new API if the PKCS#11 library version is 3.0. Otherwise, these
>> classes will try to cancel by finishing off current operations as before.
>> The support for the new C_LoginUser() has not been tested, so I commented it
>> out for now. Given the current release schedule, support for other new
>> PKCS#11 APIs (such as message-based ones and parameters structure) and
>> options for C_GetInterface (if needed) will be handled later.
>>
>> I validated the current changes against different NSS releases (supports
>> PKCS#11 v2.40 and v3..0 respectively) with existing regression tests.
>>
>> Thanks,
>> Valerie
>
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11PSSSignature.java
> line 275:
>
>> 273:
>> 274: if (token.p11.getVersion().major == 3) {
>> 275: long flags = (mode == M_SIGN? CKF_SIGN : CKF_VERIFY);
>
> I think this is a syntax nit with no space between M_SIGN and '?'
Ok, will add the space.
-
PR: https://git.openjdk.java.net/jdk/pull/6655
Re: RFR: 8255409: Support the new APIs in PKCS#11 v3.0
On Tue, 7 Dec 2021 00:08:03 GMT, Valerie Peng wrote:
>> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11AEADCipher.java
>> line 405:
>>
>>> 403: private void cancelOperation() {
>>> 404: // cancel operation by finishing it; avoid killSession as some
>>> 405: // hardware vendors may require re-login
>>
>> The new `cancelOperation()` methods seems identical everywhere. Is it
>> possible to consolidate it to a helper method like `trySessionCancel(token,
>> session, flags)`? It can return true if canceled successfully, false if
>> needs a fallback, and can still throw a `ProviderException`.
>
> I assume you mean the if-() block of code? I can move the code into a helper
> method inside the P11Util class.
Yes, just keep duplicated lines as few as possible.
-
PR: https://git.openjdk.java.net/jdk/pull/6655
Re: RFR: 8255409: Support the new APIs in PKCS#11 v3.0
On Sun, 5 Dec 2021 05:00:51 GMT, Weijun Wang wrote:
>> PKCS#11 v3.0 adds the support for several new APIs. For this particular RFE,
>> it enhances SunPKCS11 provider to load PKCS#11 provider by first trying the
>> C_GetInterface (new in 3.0) before the C_GetFunctionList assuming not
>> explicitly specified in config. In addition, PKCS#11 v3.0 defines a new API
>> for cancelling session operations, so I've also updated various classes to
>> call this new API if the PKCS#11 library version is 3.0. Otherwise, these
>> classes will try to cancel by finishing off current operations as before.
>> The support for the new C_LoginUser() has not been tested, so I commented it
>> out for now. Given the current release schedule, support for other new
>> PKCS#11 APIs (such as message-based ones and parameters structure) and
>> options for C_GetInterface (if needed) will be handled later.
>>
>> I validated the current changes against different NSS releases (supports
>> PKCS#11 v2.40 and v3..0 respectively) with existing regression tests.
>>
>> Thanks,
>> Valerie
>
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11AEADCipher.java
> line 405:
>
>> 403: private void cancelOperation() {
>> 404: // cancel operation by finishing it; avoid killSession as some
>> 405: // hardware vendors may require re-login
>
> The new `cancelOperation()` methods seems identical everywhere. Is it
> possible to consolidate it to a helper method like `trySessionCancel(token,
> session, flags)`? It can return true if canceled successfully, false if needs
> a fallback, and can still throw a `ProviderException`.
I assume you mean the if-() block of code? I can move the code into a helper
method inside the P11Util class.
-
PR: https://git.openjdk.java.net/jdk/pull/6655
Re: RFR: 8255409: Support the new APIs in PKCS#11 v3.0
On Sun, 5 Dec 2021 04:51:51 GMT, Weijun Wang wrote:
>> PKCS#11 v3.0 adds the support for several new APIs. For this particular RFE,
>> it enhances SunPKCS11 provider to load PKCS#11 provider by first trying the
>> C_GetInterface (new in 3.0) before the C_GetFunctionList assuming not
>> explicitly specified in config. In addition, PKCS#11 v3.0 defines a new API
>> for cancelling session operations, so I've also updated various classes to
>> call this new API if the PKCS#11 library version is 3.0. Otherwise, these
>> classes will try to cancel by finishing off current operations as before.
>> The support for the new C_LoginUser() has not been tested, so I commented it
>> out for now. Given the current release schedule, support for other new
>> PKCS#11 APIs (such as message-based ones and parameters structure) and
>> options for C_GetInterface (if needed) will be handled later.
>>
>> I validated the current changes against different NSS releases (supports
>> PKCS#11 v2.40 and v3..0 respectively) with existing regression tests.
>>
>> Thanks,
>> Valerie
>
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Config.java line
> 418:
>
>> 416: }
>> 417: String word = st.sval;
>> 418: switch (word) {
>
> Since every case has a break it's probably better to use the enhanced switch
> (`case "x" -> ...;`). It's safer and also saves quite some lines. An IDE can
> help you with the conversion.
Sure, I can do that. I tend to use the "->" for switch with less items.
-
PR: https://git.openjdk.java.net/jdk/pull/6655
Re: RFR: 8255409: Support the new APIs in PKCS#11 v3.0
On Wed, 1 Dec 2021 21:42:51 GMT, Valerie Peng wrote: > PKCS#11 v3.0 adds the support for several new APIs. For this particular RFE, > it enhances SunPKCS11 provider to load PKCS#11 provider by first trying the > C_GetInterface (new in 3.0) before the C_GetFunctionList assuming not > explicitly specified in config. In addition, PKCS#11 v3.0 defines a new API > for cancelling session operations, so I've also updated various classes to > call this new API if the PKCS#11 library version is 3.0. Otherwise, these > classes will try to cancel by finishing off current operations as before. The > support for the new C_LoginUser() has not been tested, so I commented it out > for now. Given the current release schedule, support for other new PKCS#11 > APIs (such as message-based ones and parameters structure) and options for > C_GetInterface (if needed) will be handled later. > > I validated the current changes against different NSS releases (supports > PKCS#11 v2.40 and v3..0 respectively) with existing regression tests. > > Thanks, > Valerie I updated my comments because I neglected to read your initial message and went straight to the code review - PR: https://git.openjdk.java.net/jdk/pull/6655
Re: RFR: 8255409: Support the new APIs in PKCS#11 v3.0
On Wed, 1 Dec 2021 21:42:51 GMT, Valerie Peng wrote:
> PKCS#11 v3.0 adds the support for several new APIs. For this particular RFE,
> it enhances SunPKCS11 provider to load PKCS#11 provider by first trying the
> C_GetInterface (new in 3.0) before the C_GetFunctionList assuming not
> explicitly specified in config. In addition, PKCS#11 v3.0 defines a new API
> for cancelling session operations, so I've also updated various classes to
> call this new API if the PKCS#11 library version is 3.0. Otherwise, these
> classes will try to cancel by finishing off current operations as before. The
> support for the new C_LoginUser() has not been tested, so I commented it out
> for now. Given the current release schedule, support for other new PKCS#11
> APIs (such as message-based ones and parameters structure) and options for
> C_GetInterface (if needed) will be handled later.
>
> I validated the current changes against different NSS releases (supports
> PKCS#11 v2.40 and v3..0 respectively) with existing regression tests.
>
> Thanks,
> Valerie
Are there no tests because there is no support for 3.0 in nss?
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11AEADCipher.java
line 406:
> 404: token.ensureValid();
> 405: if (token.p11.getVersion().major == 3) {
> 406: long flags = (encrypt? CKF_ENCRYPT : CKF_DECRYPT);
I think this is a syntax nit with no space between "encrypt" and '?'
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java line
449:
> 447: token.ensureValid();
> 448: if (token.p11.getVersion().major == 3) {
> 449: long flags = (encrypt? CKF_ENCRYPT : CKF_DECRYPT);
I think this is a syntax nit with no space between "encrypt" and '?'
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyWrapCipher.java
line 291:
> 289:
> 290: if (token.p11.getVersion().major == 3) {
> 291: long flags = (opmode == Cipher.ENCRYPT_MODE? CKF_ENCRYPT :
I think this is a syntax nit with no space between MODE and '?'
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11PSSSignature.java
line 275:
> 273:
> 274: if (token.p11.getVersion().major == 3) {
> 275: long flags = (mode == M_SIGN? CKF_SIGN : CKF_VERIFY);
I think this is a syntax nit with no space between M_SIGN and '?'
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java
line 285:
> 283:
> 284: if (token.p11.getVersion().major == 3) {
> 285: long flags = (mode == M_SIGN? CKF_SIGN : CKF_VERIFY);
M_SIGN ? ...
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/wrapper/PKCS11.java
line 558:
> 556: throws PKCS11Exception;
> 557:
> 558: ///**
Was it intentional to have the below commented out?
src/jdk.crypto.cryptoki/windows/native/libj2pkcs11/p11_md.c line 128:
> 126: }
> 127:
> 128: #ifdef DEBUG
Is C_GetInterfaceList in DEBUG only because it's a new 3.0 function we are not
supporting yet?
-
PR: https://git.openjdk.java.net/jdk/pull/6655
Re: RFR: 8255409: Support the new APIs in PKCS#11 v3.0
On Wed, 1 Dec 2021 21:42:51 GMT, Valerie Peng wrote:
> PKCS#11 v3.0 adds the support for several new APIs. For this particular RFE,
> it enhances SunPKCS11 provider to load PKCS#11 provider by first trying the
> C_GetInterface (new in 3.0) before the C_GetFunctionList assuming not
> explicitly specified in config. In addition, PKCS#11 v3.0 defines a new API
> for cancelling session operations, so I've also updated various classes to
> call this new API if the PKCS#11 library version is 3.0. Otherwise, these
> classes will try to cancel by finishing off current operations as before. The
> support for the new C_LoginUser() has not been tested, so I commented it out
> for now. Given the current release schedule, support for other new PKCS#11
> APIs (such as message-based ones and parameters structure) and options for
> C_GetInterface (if needed) will be handled later.
>
> I validated the current changes against different NSS releases (supports
> PKCS#11 v2.40 and v3..0 respectively) with existing regression tests.
>
> Thanks,
> Valerie
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Config.java line 418:
> 416: }
> 417: String word = st.sval;
> 418: switch (word) {
Since every case has a break it's probably better to use the enhanced switch
(`case "x" -> ...;`). It's safer and also saves quite some lines. An IDE can
help you with the conversion.
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11AEADCipher.java
line 405:
> 403: private void cancelOperation() {
> 404: // cancel operation by finishing it; avoid killSession as some
> 405: // hardware vendors may require re-login
The new `cancelOperation()` methods seems identical everywhere. Is it possible
to consolidate it to a helper method like `trySessionCancel(token, session,
flags)`? It can return true if canceled successfully, false if needs a
fallback, and can still throw a `ProviderException`.
-
PR: https://git.openjdk.java.net/jdk/pull/6655
