Re: RFR: 8258833: Cancel multi-part cipher operations in SunPKCS11 after failures [v6]
> When a multi-part cipher operation fails in SunPKCS11 (i.e. because of an > invalid block size), we now cancel the operation before returning the > underlying Session to the Session Manager. This allows to use the returned > Session for a different purpose. Otherwise, an CKR_OPERATION_ACTIVE error > would be raised from the PKCS#11 library. > > The jdk/sun/security/pkcs11/Cipher/CancelMultipart.java regression test is > introduced as part of this PR. > > No regressions found in jdk/sun/security/pkcs11. Martin Balao has updated the pull request incrementally with one additional commit since the last revision: Removing the encryption-update path in CancelMultipart test as it depends on a know bug to cause a PKCS#11 error. - Changes: - all: https://git.openjdk.java.net/jdk/pull/1901/files - new: https://git.openjdk.java.net/jdk/pull/1901/files/0f96ddf1..4c892a44 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1901&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1901&range=04-05 Stats: 22 lines in 1 file changed: 0 ins; 20 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/1901.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1901/head:pull/1901 PR: https://git.openjdk.java.net/jdk/pull/1901
Re: RFR: 8258833: Cancel multi-part cipher operations in SunPKCS11 after failures [v6]
On Thu, 14 Jan 2021 20:29:54 GMT, Valerie Peng wrote: >> The update fails because the native mechanism (CKM_AES_ECB) has no padding >> and OpenJDK does not buffer data in the Java side for encryption [1] (this >> is a bug that I'll address soon). As a result, there is a PKCS#11 call with >> an invalid length and we get the error that ends up returning the session to >> the Session Manager. I just realized that when we fix the previous >> padding-bug, this test path won't work anymore. CKR_BUFFER_TOO_SMALL errors >> on updates do not lead to a reset call in the OpenJDK side (contrary to >> doFinal), so they wouldn't be useful for the test. I'll investigate if there >> is a way to trigger the path. Otherwise we should keep the doFinal path >> only. I'd still force a reset if there is an error other than >> CKR_BUFFER_TOO_SMALL in the update. >> >> -- >> [1] - >> https://github.com/openjdk/jdk/blob/1cc09ccaef9a3695dd2862e3ee121e141e0a8a13/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java#L583 > > It's an update call, isn't padding occur when doFinal() is called for > encryption? > In any case, it's best for the test case to not have this bug dependency. I > am ok if you can only test doFinal path only. Yes, makes sense to remove the bug dependency and the whole encrypt-update path. I'll keep the test extensible, though; so we can include new paths eventually. - PR: https://git.openjdk.java.net/jdk/pull/1901
Re: RFR: 8258833: Cancel multi-part cipher operations in SunPKCS11 after failures [v6]
On Fri, 15 Jan 2021 20:28:28 GMT, Martin Balao wrote:
>> When a multi-part cipher operation fails in SunPKCS11 (i.e. because of an
>> invalid block size), we now cancel the operation before returning the
>> underlying Session to the Session Manager. This allows to use the returned
>> Session for a different purpose. Otherwise, an CKR_OPERATION_ACTIVE error
>> would be raised from the PKCS#11 library.
>>
>> The jdk/sun/security/pkcs11/Cipher/CancelMultipart.java regression test is
>> introduced as part of this PR.
>>
>> No regressions found in jdk/sun/security/pkcs11.
>
> Martin Balao has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Removing the encryption-update path in CancelMultipart test as it depends
> on a know bug to cause a PKCS#11 error.
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11AEADCipher.java
line 353:
> 351: }
> 352: } catch (PKCS11Exception e) {
> 353: if (e.getErrorCode() == CKR_OPERATION_NOT_INITIALIZED) {
nit: update copyright year to 2021
-
PR: https://git.openjdk.java.net/jdk/pull/1901
Re: RFR: 8258833: Cancel multi-part cipher operations in SunPKCS11 after failures [v6]
On Fri, 15 Jan 2021 20:28:28 GMT, Martin Balao wrote: >> When a multi-part cipher operation fails in SunPKCS11 (i.e. because of an >> invalid block size), we now cancel the operation before returning the >> underlying Session to the Session Manager. This allows to use the returned >> Session for a different purpose. Otherwise, an CKR_OPERATION_ACTIVE error >> would be raised from the PKCS#11 library. >> >> The jdk/sun/security/pkcs11/Cipher/CancelMultipart.java regression test is >> introduced as part of this PR. >> >> No regressions found in jdk/sun/security/pkcs11. > > Martin Balao has updated the pull request incrementally with one additional > commit since the last revision: > > Removing the encryption-update path in CancelMultipart test as it depends > on a know bug to cause a PKCS#11 error. test/jdk/sun/security/pkcs11/Cipher/CancelMultipart.java line 2: > 1: /* > 2: * Copyright (c) 2020, Red Hat, Inc. 2021? - PR: https://git.openjdk.java.net/jdk/pull/1901
Re: RFR: 8258833: Cancel multi-part cipher operations in SunPKCS11 after failures [v6]
On Fri, 15 Jan 2021 20:28:28 GMT, Martin Balao wrote: >> When a multi-part cipher operation fails in SunPKCS11 (i.e. because of an >> invalid block size), we now cancel the operation before returning the >> underlying Session to the Session Manager. This allows to use the returned >> Session for a different purpose. Otherwise, an CKR_OPERATION_ACTIVE error >> would be raised from the PKCS#11 library. >> >> The jdk/sun/security/pkcs11/Cipher/CancelMultipart.java regression test is >> introduced as part of this PR. >> >> No regressions found in jdk/sun/security/pkcs11. > > Martin Balao has updated the pull request incrementally with one additional > commit since the last revision: > > Removing the encryption-update path in CancelMultipart test as it depends > on a know bug to cause a PKCS#11 error. src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11AEADCipher.java line 631: > 629: // these cases are not expected here because the output > length > 630: // is checked in the OpenJDK side before making the PKCS#11 > call. > 631: // Thus, doCancel can safely be 'false'. Since the code is following the spec, I am not sure if this comment provides additional info? Fine to leave it if you prefer to have it. Just a thought. This goes for the same comments for other classes where we are not changing the behavior. - PR: https://git.openjdk.java.net/jdk/pull/1901
Re: RFR: 8258833: Cancel multi-part cipher operations in SunPKCS11 after failures [v6]
On Fri, 15 Jan 2021 20:28:28 GMT, Martin Balao wrote: >> When a multi-part cipher operation fails in SunPKCS11 (i.e. because of an >> invalid block size), we now cancel the operation before returning the >> underlying Session to the Session Manager. This allows to use the returned >> Session for a different purpose. Otherwise, an CKR_OPERATION_ACTIVE error >> would be raised from the PKCS#11 library. >> >> The jdk/sun/security/pkcs11/Cipher/CancelMultipart.java regression test is >> introduced as part of this PR. >> >> No regressions found in jdk/sun/security/pkcs11. > > Martin Balao has updated the pull request incrementally with one additional > commit since the last revision: > > Removing the encryption-update path in CancelMultipart test as it depends > on a know bug to cause a PKCS#11 error. Just made some feedback regarding comments, the actual changes look good. src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java line 638: > 636: (new ShortBufferException().initCause(e)); > 637: } > 638: reset(true); Per PKCS#11 spec, "A call to C_EncryptUpdate which results in an error other than CKR_BUFFER_TOO_SMALL terminates the current encryption operation.", so I'd expect comment here to explain why we are doing reset(true). If not mentioning the known NSS behavior which triggered this change, at least comment the bug id so we don't lost track of the reason for the switch. src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java line 756: > 754: (new ShortBufferException().initCause(e)); > 755: } > 756: reset(true); Comment for line 638 apply here as well. - Marked as reviewed by valeriep (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1901
Re: RFR: 8258833: Cancel multi-part cipher operations in SunPKCS11 after failures [v6]
On Wed, 20 Jan 2021 03:16:32 GMT, Valerie Peng wrote:
>> Martin Balao has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Removing the encryption-update path in CancelMultipart test as it depends
>> on a know bug to cause a PKCS#11 error.
>
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11AEADCipher.java
> line 353:
>
>> 351: }
>> 352: } catch (PKCS11Exception e) {
>> 353: if (e.getErrorCode() == CKR_OPERATION_NOT_INITIALIZED) {
>
> nit: update copyright year to 2021. Same goes for other sources.
right, thanks
> test/jdk/sun/security/pkcs11/Cipher/CancelMultipart.java line 2:
>
>> 1: /*
>> 2: * Copyright (c) 2020, Red Hat, Inc.
>
> 2021?
right, thanks
-
PR: https://git.openjdk.java.net/jdk/pull/1901
Re: RFR: 8258833: Cancel multi-part cipher operations in SunPKCS11 after failures [v6]
On Wed, 20 Jan 2021 05:55:26 GMT, Valerie Peng wrote: >> Martin Balao has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Removing the encryption-update path in CancelMultipart test as it depends >> on a know bug to cause a PKCS#11 error. > > src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11AEADCipher.java > line 631: > >> 629: // these cases are not expected here because the output >> length >> 630: // is checked in the OpenJDK side before making the PKCS#11 >> call. >> 631: // Thus, doCancel can safely be 'false'. > > Since the code is following the spec, I am not sure if this comment provides > additional info? Fine to leave it if you prefer to have it. Just a thought. > This goes for the same comments for other classes where we are not changing > the behavior. I wish we could keep the comment and make the previous assumption more explicit, even when someone can read the code above and arrive to the same conclusion. If the code around ever changes, this comment is something we must consider and, eventually, take action. - PR: https://git.openjdk.java.net/jdk/pull/1901
Re: RFR: 8258833: Cancel multi-part cipher operations in SunPKCS11 after failures [v6]
On Wed, 20 Jan 2021 05:58:49 GMT, Valerie Peng wrote: >> Martin Balao has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Removing the encryption-update path in CancelMultipart test as it depends >> on a know bug to cause a PKCS#11 error. > > src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java line > 638: > >> 636: (new ShortBufferException().initCause(e)); >> 637: } >> 638: reset(true); > > Per PKCS#11 spec, "A call to C_EncryptUpdate which results in an error other > than CKR_BUFFER_TOO_SMALL terminates the current encryption operation.", so > I'd expect comment here to explain why we are doing reset(true). If not > mentioning the known NSS behavior which triggered this change, at least > comment the bug id so we don't lost track of the reason for the switch. Yes, makes sense. Thanks - PR: https://git.openjdk.java.net/jdk/pull/1901
