RFR: 8261502: ECDHKeyAgreement: Allows alternate ECPrivateKey impl and revised exception handling

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8261502: ECDHKeyAgreement: Allows alternate ECPrivateKey impl and revised exception handling

Anthony Scarpino-2
Hi,

I need a code review of this change to ECDH.  It is a combination of fixing the implementation to not only accept ECPrivateKeyImpl along with a fix to the exception handling.  They started as two fixes, but with the exception handling the underlying code changed significantly that made the ECPrivateKey change in a different place.  The new exception handling is a result of no longer having the native library.  Many of the checks waited until generateSecret() to send the keys to the native library. Now that native is gone, checks can happen when keys are provided to the methods and proper exceptions can be thrown instead of wrapping everything as a ProviderException

thanks,

Tony

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

Commit messages:
 - merge with 8261502
 - initial change

Changes: https://git.openjdk.java.net/jdk/pull/2659/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2659&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261502
  Stats: 205 lines in 2 files changed: 138 ins; 32 del; 35 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2659.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2659/head:pull/2659

PR: https://git.openjdk.java.net/jdk/pull/2659
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261502: ECDHKeyAgreement: Allows alternate ECPrivateKey impl and revised exception handling [v2]

Anthony Scarpino-2
> Hi,
>
> I need a code review of this change to ECDH.  It is a combination of fixing the implementation to not only accept ECPrivateKeyImpl along with a fix to the exception handling.  They started as two fixes, but with the exception handling the underlying code changed significantly that made the ECPrivateKey change in a different place.  The new exception handling is a result of no longer having the native library.  Many of the checks waited until generateSecret() to send the keys to the native library. Now that native is gone, checks can happen when keys are provided to the methods and proper exceptions can be thrown instead of wrapping everything as a ProviderException
>
> thanks,
>
> Tony

Anthony Scarpino has updated the pull request incrementally with one additional commit since the last revision:

  Simpler fix for ECPrivateKey

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2659/files
  - new: https://git.openjdk.java.net/jdk/pull/2659/files/3451055a..aec24cc1

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2659&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2659&range=00-01

  Stats: 5 lines in 1 file changed: 0 ins; 1 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2659.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2659/head:pull/2659

PR: https://git.openjdk.java.net/jdk/pull/2659
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261502: ECDHKeyAgreement: Allows alternate ECPrivateKey impl and revised exception handling [v2]

Jamil Nimeh-2
On Tue, 23 Feb 2021 20:40:05 GMT, Anthony Scarpino <[hidden email]> wrote:

>> Hi,
>>
>> I need a code review of this change to ECDH.  It is a combination of fixing the implementation to not only accept ECPrivateKeyImpl along with a fix to the exception handling.  They started as two fixes, but with the exception handling the underlying code changed significantly that made the ECPrivateKey change in a different place.  The new exception handling is a result of no longer having the native library.  Many of the checks waited until generateSecret() to send the keys to the native library. Now that native is gone, checks can happen when keys are provided to the methods and proper exceptions can be thrown instead of wrapping everything as a ProviderException
>>
>> thanks,
>>
>> Tony
>
> Anthony Scarpino has updated the pull request incrementally with one additional commit since the last revision:
>
>   Simpler fix for ECPrivateKey

src/jdk.crypto.ec/share/classes/sun/security/ec/ECDHKeyAgreement.java line 211:

> 209:         try {
> 210:             result = deriveKeyImpl(privateKey, privateKeyOps, publicKey);
> 211:         } catch (Exception e) {

Why such a broad exception catch here?  It looks like deriveKeyImpl is only explicitly throwing IKE.  Are there other unchecked exceptions that you're trying to snag here that I'm missing in the deriveKeyImpl below?

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

PR: https://git.openjdk.java.net/jdk/pull/2659
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261502: ECDHKeyAgreement: Allows alternate ECPrivateKey impl and revised exception handling [v2]

Jamil Nimeh-2
In reply to this post by Anthony Scarpino-2
On Tue, 23 Feb 2021 20:40:05 GMT, Anthony Scarpino <[hidden email]> wrote:

>> Hi,
>>
>> I need a code review of this change to ECDH.  It is a combination of fixing the implementation to not only accept ECPrivateKeyImpl along with a fix to the exception handling.  They started as two fixes, but with the exception handling the underlying code changed significantly that made the ECPrivateKey change in a different place.  The new exception handling is a result of no longer having the native library.  Many of the checks waited until generateSecret() to send the keys to the native library. Now that native is gone, checks can happen when keys are provided to the methods and proper exceptions can be thrown instead of wrapping everything as a ProviderException
>>
>> thanks,
>>
>> Tony
>
> Anthony Scarpino has updated the pull request incrementally with one additional commit since the last revision:
>
>   Simpler fix for ECPrivateKey

test/jdk/com/sun/crypto/provider/KeyAgreement/ECKeyCheck.java line 27:

> 25:  * @test
> 26:  * @bug 8238911
> 27:  * @summary Check that ECPrivateKey's that are not ECPrivateKeyImpl can use

Do the bug and summary tags need to be updated to 8261502 and "ECDHKeyAgreement: Allows alternate ECPrivateKey impl and revised exception handling", respectively?

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

PR: https://git.openjdk.java.net/jdk/pull/2659
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261502: ECDHKeyAgreement: Allows alternate ECPrivateKey impl and revised exception handling [v2]

Anthony Scarpino-2
In reply to this post by Jamil Nimeh-2
On Thu, 18 Mar 2021 17:06:21 GMT, Jamil Nimeh <[hidden email]> wrote:

>> Anthony Scarpino has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Simpler fix for ECPrivateKey
>
> src/jdk.crypto.ec/share/classes/sun/security/ec/ECDHKeyAgreement.java line 211:
>
>> 209:         try {
>> 210:             result = deriveKeyImpl(privateKey, privateKeyOps, publicKey);
>> 211:         } catch (Exception e) {
>
> Why such a broad exception catch here?  It looks like deriveKeyImpl is only explicitly throwing IKE.  Are there other unchecked exceptions that you're trying to snag here that I'm missing in the deriveKeyImpl below?

Just being cautious and wrapping anything.  Maybe there will be some exceptions in the math methods that throw could exceptions.

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

PR: https://git.openjdk.java.net/jdk/pull/2659
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261502: ECDHKeyAgreement: Allows alternate ECPrivateKey impl and revised exception handling [v2]

Jamil Nimeh-2
In reply to this post by Anthony Scarpino-2
On Tue, 23 Feb 2021 20:40:05 GMT, Anthony Scarpino <[hidden email]> wrote:

>> Hi,
>>
>> I need a code review of this change to ECDH.  It is a combination of fixing the implementation to not only accept ECPrivateKeyImpl along with a fix to the exception handling.  They started as two fixes, but with the exception handling the underlying code changed significantly that made the ECPrivateKey change in a different place.  The new exception handling is a result of no longer having the native library.  Many of the checks waited until generateSecret() to send the keys to the native library. Now that native is gone, checks can happen when keys are provided to the methods and proper exceptions can be thrown instead of wrapping everything as a ProviderException
>>
>> thanks,
>>
>> Tony
>
> Anthony Scarpino has updated the pull request incrementally with one additional commit since the last revision:
>
>   Simpler fix for ECPrivateKey

Marked as reviewed by jnimeh (Reviewer).

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

PR: https://git.openjdk.java.net/jdk/pull/2659
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261502: ECDHKeyAgreement: Allows alternate ECPrivateKey impl and revised exception handling [v2]

Jamil Nimeh-2
In reply to this post by Anthony Scarpino-2
On Fri, 19 Mar 2021 20:29:48 GMT, Anthony Scarpino <[hidden email]> wrote:

>> src/jdk.crypto.ec/share/classes/sun/security/ec/ECDHKeyAgreement.java line 211:
>>
>>> 209:         try {
>>> 210:             result = deriveKeyImpl(privateKey, privateKeyOps, publicKey);
>>> 211:         } catch (Exception e) {
>>
>> Why such a broad exception catch here?  It looks like deriveKeyImpl is only explicitly throwing IKE.  Are there other unchecked exceptions that you're trying to snag here that I'm missing in the deriveKeyImpl below?
>
> Just being cautious and wrapping anything.  Maybe there will be some exceptions in the math methods that throw could exceptions.

Fair enough.  I'm OK to leave this as-is.

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

PR: https://git.openjdk.java.net/jdk/pull/2659
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261502: ECDHKeyAgreement: Allows alternate ECPrivateKey impl and revised exception handling [v3]

Anthony Scarpino-2
In reply to this post by Anthony Scarpino-2
> Hi,
>
> I need a code review of this change to ECDH.  It is a combination of fixing the implementation to not only accept ECPrivateKeyImpl along with a fix to the exception handling.  They started as two fixes, but with the exception handling the underlying code changed significantly that made the ECPrivateKey change in a different place.  The new exception handling is a result of no longer having the native library.  Many of the checks waited until generateSecret() to send the keys to the native library. Now that native is gone, checks can happen when keys are provided to the methods and proper exceptions can be thrown instead of wrapping everything as a ProviderException
>
> thanks,
>
> Tony

Anthony Scarpino has updated the pull request incrementally with one additional commit since the last revision:

  update test bug id

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2659/files
  - new: https://git.openjdk.java.net/jdk/pull/2659/files/aec24cc1..3342e3b2

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2659&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2659&range=01-02

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2659.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2659/head:pull/2659

PR: https://git.openjdk.java.net/jdk/pull/2659
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261502: ECDHKeyAgreement: Allows alternate ECPrivateKey impl and revised exception handling [v4]

Anthony Scarpino-2
In reply to this post by Anthony Scarpino-2
> Hi,
>
> I need a code review of this change to ECDH.  It is a combination of fixing the implementation to not only accept ECPrivateKeyImpl along with a fix to the exception handling.  They started as two fixes, but with the exception handling the underlying code changed significantly that made the ECPrivateKey change in a different place.  The new exception handling is a result of no longer having the native library.  Many of the checks waited until generateSecret() to send the keys to the native library. Now that native is gone, checks can happen when keys are provided to the methods and proper exceptions can be thrown instead of wrapping everything as a ProviderException
>
> thanks,
>
> Tony

Anthony Scarpino has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits:

 - update test bug id
 - Simpler fix for ECPrivateKey
 - merge with 8261502
 - initial change

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

Changes: https://git.openjdk.java.net/jdk/pull/2659/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2659&range=03
  Stats: 203 lines in 2 files changed: 137 ins; 32 del; 34 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2659.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2659/head:pull/2659

PR: https://git.openjdk.java.net/jdk/pull/2659
Reply | Threaded
Open this post in threaded view
|

Integrated: 8261502: ECDHKeyAgreement: Allows alternate ECPrivateKey impl and revised exception handling

Anthony Scarpino-2
In reply to this post by Anthony Scarpino-2
On Sat, 20 Feb 2021 04:25:50 GMT, Anthony Scarpino <[hidden email]> wrote:

> Hi,
>
> I need a code review of this change to ECDH.  It is a combination of fixing the implementation to not only accept ECPrivateKeyImpl along with a fix to the exception handling.  They started as two fixes, but with the exception handling the underlying code changed significantly that made the ECPrivateKey change in a different place.  The new exception handling is a result of no longer having the native library.  Many of the checks waited until generateSecret() to send the keys to the native library. Now that native is gone, checks can happen when keys are provided to the methods and proper exceptions can be thrown instead of wrapping everything as a ProviderException
>
> thanks,
>
> Tony

This pull request has now been integrated.

Changeset: 374272fd
Author:    Anthony Scarpino <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/374272fd
Stats:     203 lines in 2 files changed: 137 ins; 32 del; 34 mod

8261502: ECDHKeyAgreement: Allows alternate ECPrivateKey impl and revised exception handling

Reviewed-by: jnimeh

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

PR: https://git.openjdk.java.net/jdk/pull/2659