RFR 8221172: SunEC specific test is not limited to SunEC

2019-03-20 Thread Adam Petcher
The regression test I added for JDK-8147502[1] has a minor bug. The test only works for SunEC, so the calls to getInstance should specify SunEC as the provider. Please review the fix for this test bug---it should only take a moment. Webrev:

Re: RFR 8147502: Digest is incorrectly truncated for ECDSA signatures...

2019-03-19 Thread Adam Petcher
normally be non-deterministic.  Just one really tiny nit, update the copyright on ec.c. --Jamil On 3/4/19 11:40 AM, Adam Petcher wrote: webrev: https://cr.openjdk.java.net/~apetcher/8147502/webrev.00/ JBS: https://bugs.openjdk.java.net/browse/JDK-8147502 Please review this fix to a bug

Re: RFR: release note for JDK-8218723

2019-03-18 Thread Adam Petcher
A couple of comments: *) I think it would be helpful to also describe the change using API terms, so it is more clear to programmers who may not recognize all the crypto terminology. Perhaps: "... will now exclusively use the SunJCE Mac service for the underlying pseudorandom function (PRF)."

Re: RFR 8218723: SecretKeyFactory.getInstance( algo_, provider_ ) ignores the provider argument.

2019-03-18 Thread Adam Petcher
codepaths are a bit different between Oracle and Open JDKs.  Also the original bug came in against Oracle JDK so it would be nice to have the test run there. http://cr.openjdk.java.net/~jnimeh/reviews/8218723/webrev.02 Thanks, --Jamil On 3/14/19 7:53 AM, Adam Petcher wrote: The change

Re: RFR 8218723: SecretKeyFactory.getInstance( algo_, provider_ ) ignores the provider argument.

2019-03-14 Thread Adam Petcher
The change to PBKDF2KeyImpl.java looks fine. About the test: *) Is it necessary to put the provider in a separate jar? It seems unnecessary because you are adding it with Security.insertProviderAt. *) Line 54 of the test compares the result of a constructor to null. Unless I'm missing

Re: CSR Review Request: JDK-8220531, SecretKeyFactory.getInstance( algo_, provider_ ) ignores the provider argument.

2019-03-13 Thread Adam Petcher
Adam, thanks for the feedback.  I have some comments below: On 3/13/2019 6:44 AM, Adam Petcher wrote: On 3/12/2019 2:33 PM, Jamil Nimeh wrote: Hello all, Please review the CSR for the behavioral change to SunJCE's PBKDF2 implementaion.  This change will make the underlying Mac also come from

Re: CSR Review Request: JDK-8220531, SecretKeyFactory.getInstance( algo_, provider_ ) ignores the provider argument.

2019-03-13 Thread Adam Petcher
On 3/12/2019 2:33 PM, Jamil Nimeh wrote: Hello all, Please review the CSR for the behavioral change to SunJCE's PBKDF2 implementaion.  This change will make the underlying Mac also come from SunJCE.  This change only affects the SunJCE implementation of PBKDF2, not any other implementation

RFR 8147502: Digest is incorrectly truncated for ECDSA signatures...

2019-03-04 Thread Adam Petcher
webrev: https://cr.openjdk.java.net/~apetcher/8147502/webrev.00/ JBS: https://bugs.openjdk.java.net/browse/JDK-8147502 Please review this fix to a bug that causes ECDSA signatures to be incorrect in some cases. The fix is simple, but testing this issue is difficult because the API doesn't give

Re: RFR[s]: 8201633 Problems with AES-GCM native acceleration

2019-02-06 Thread Adam Petcher
I'm still not sold on this---it seems like a specific solution to a single instance of a more general problem. Have you investigated any tweaks to the JVM or compiler settings? Andrew prototyped something[1], but it looks like this is also specific to GCM. A more general solution would be

RFR 8217518: Crypto benchmarks not warming up in time

2019-01-22 Thread Adam Petcher
Claes, Please review this very small change that adds the "+AlwaysPreTouch" option to the crypto benchmarks to allow them to warm up in time. This fix is in response to Eric's discovery (when reviewing the new benchmarks for KeyAgreement and Cipher) that the crypto benchmarks were not

Re: RFR (XS) 8217344 : Make comparison overflow-aware in ECDHKeyAgreement.engineGenerateSecret()

2019-01-18 Thread Adam Petcher
Looks good to me. On 1/17/2019 4:32 PM, Ivan Gerasimov wrote: Hello! Would you please help review a trivial fix to avoid a possible arithmetic overflow in comparison? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8217344 WEBREV: http://cr.openjdk.java.net/~igerasim/8217344/00/webrev/

Re: New cryptographic primitives

2019-01-07 Thread Adam Petcher
Hi Eric, I don't immediately see anything in your list of implementations that we don't already have, and that we clearly need. But I would be interested in hearing from you or others about whether there is demand for these algorithms. The ones that are used in TLS 1.3 are valuable, but we

Re: RFR 8215643: Microbenchmarks for KeyAgreement and Cipher

2018-12-20 Thread Adam Petcher
updated webrev: http://cr.openjdk.java.net/~apetcher/8215643/webrev.01/ On 12/19/2018 4:49 PM, Jamil Nimeh wrote: Hi Adam.  On the whole the benchmarks look good to me.  Can I ask why those ciphers and key agreement schemes that support multiple key lengths aren't called out in the @Param

RFR 8215643: Microbenchmarks for KeyAgreement and Cipher

2018-12-19 Thread Adam Petcher
Webrev: http://cr.openjdk.java.net/~apetcher/8215643/webrev.00/ JBS: https://bugs.openjdk.java.net/browse/JDK-8215643 Please review this enhancement that adds two new crypto microbenchmarks. See the JBS ticket for the motivation behind these new benchmarks. The Cipher benchmark nearly

Re: RFR 8208698: Improved ECC Implementation

2018-12-07 Thread Adam Petcher
On 12/7/2018 1:12 AM, Anthony Scarpino wrote: I don't have any code comments to add to your code. It's pretty clean and mostly algorithm code which the known answer tests will do a better job of judging correctness. One comment I did have was if there were any implications from using little

Re: RFR 8208648: ECC Field Arithmetic Enhancements

2018-12-07 Thread Adam Petcher
Updated webrev: http://cr.openjdk.java.net/~apetcher/8208648/webrev.01/ Thanks for looking at this. See below. On 12/6/2018 8:03 PM, Jamil Nimeh wrote: Hi Adam, comments/questions below (mostly simple stuff, nothing major): * IntegerPolynomial.java o The comment block for multByInt

RFR 8214688: TLS 1.3 session resumption with hello retry request failed with "illegal_parameter"

2018-12-06 Thread Adam Petcher
Webrev: http://cr.openjdk.java.net/~apetcher/8214688/webrev.00/ JBS: https://bugs.openjdk.java.net/browse/JDK-8214688 The last session resumption bug fix[1] I made introduced a new issue when the server sends a HelloRetryRequest message. The proposed fix is pretty simple: when the client

Re: AES ctr benchmark performance

2018-12-03 Thread Adam Petcher
I'm seeing JDK 11 performance around 40% of OpenSSL performance, too. This is on a Core i7-4980HQ (Haswell, turbo at 4 GHz). In addition to the "Java stuff" (e.g. garbage collection, array bounds checking) that Tony suggested, there is also the issue that the JDK native implementation does

RFR 8208698: Improved ECC Implementation

2018-11-30 Thread Adam Petcher
JBS: https://bugs.openjdk.java.net/browse/JDK-8208698 webrev: http://cr.openjdk.java.net/~apetcher/8208698/webrev.00/ This is a re-implementation of ECDH and ECDSA that is designed to be resilient against side-channel attacks. The implementation only supports the 256-bit, 384-bit, and 521-bit

RFR 8208648: ECC Field Arithmetic Enhancements

2018-11-30 Thread Adam Petcher
JBS: https://bugs.openjdk.java.net/browse/JDK-8208648 webrev: http://cr.openjdk.java.net/~apetcher/8208648/webrev.00/ This changeset includes the enhancements to the finite field arithmetic library that are necessary for the new implementation of ECDSA and ECDH[1]. The actual ECDH and ECDSA

Re: RFR 8213363: X25519 private key PKCS#8 encoding/decoding is incorrect

2018-11-15 Thread Adam Petcher
Done[1]. Please take a look when you have a chance. [1] https://bugs.openjdk.java.net/browse/JDK-8213946 On 11/14/2018 4:29 PM, Sean Mullan wrote: The fix and the CSR look good. Please also add a release note. --Sean On 11/8/18 11:51 AM, Adam Petcher wrote: Oops. And the JBS ticket: https

Re: Problems with AES-GCM native acceleration

2018-11-14 Thread Adam Petcher
I'm adding back in hotspot-dev, because this is a somewhat tricky topic related to intrinsics and JIT. Hopefully, a Hotspot expert can correct anything that I say below that is wrong, and suggest any solutions that I missed. The AES acceleration is implemented in a HotSpot intrinsic. In order

Re: RFR 8213400: Support choosing curve name in keytool keypair generation

2018-11-13 Thread Adam Petcher
(). This is implementation dependent but it works within OpenJDK. No change on other files. I filed https://bugs.openjdk.java.net/browse/JDK-8213719 for the double BD issue. Thanks Max On Nov 9, 2018, at 11:33 PM, Adam Petcher wrote: On 11/8/2018 10:30 PM, Weijun Wang wrote: On Nov 9, 2018, at 12

RFR 8213202: Possible race condition in TLS 1.3 session resumption

2018-11-09 Thread Adam Petcher
JBS: https://bugs.openjdk.java.net/browse/JDK-8213202 webrev: http://cr.openjdk.java.net/~apetcher/8213202/webrev.00/ This change fixes a bug that allows multiple clients thread to offer the same PSK to the server, even though only one thread may actually use the PSK to resume the session. The

Re: RFR 8213400: Support choosing curve name in keytool keypair generation

2018-11-09 Thread Adam Petcher
On 11/8/2018 10:30 PM, Weijun Wang wrote: On Nov 9, 2018, at 12:28 AM, Adam Petcher wrote: On 11/8/2018 8:10 AM, Weijun Wang wrote: - CurveDB.java: -add("sect163r2 [NIST B-163]", "1.3.132.0.15", BD, +add("sect163r2 [NIST B-163]", "

Re: RFR 8213363: X25519 private key PKCS#8 encoding/decoding is incorrect

2018-11-08 Thread Adam Petcher
Oops. And the JBS ticket: https://bugs.openjdk.java.net/browse/JDK-8213363 On 11/8/2018 11:43 AM, Adam Petcher wrote: webrev: http://cr.openjdk.java.net/~apetcher/8213363/webrev.00/ CSR: https://bugs.openjdk.java.net/browse/JDK-8213493 This change fixes a bug related to the encoded format

RFR 8213363: X25519 private key PKCS#8 encoding/decoding is incorrect

2018-11-08 Thread Adam Petcher
webrev: http://cr.openjdk.java.net/~apetcher/8213363/webrev.00/ CSR: https://bugs.openjdk.java.net/browse/JDK-8213493 This change fixes a bug related to the encoded format of X25519/X448 private keys. The new code will not preserve compatibility with the improperly-formatted keys produced by

Re: RFR 8213400: Support choosing curve name in keytool keypair generation

2018-11-08 Thread Adam Petcher
On 11/8/2018 8:10 AM, Weijun Wang wrote: - CurveDB.java: -add("sect163r2 [NIST B-163]", "1.3.132.0.15", BD, +add("sect163r2 [NIST B-163]", "1.3.132.0.15", B, All other NIST B-*** curves do not have BD. This should have been a typo. I think this will change the default

Re: RFR CSR for 8213400: Support choosing curve name in keytool keypair generation

2018-11-08 Thread Adam Petcher
I'm missing the motivation behind this question. Is the current set of aliases causing some problem? Are they incomplete? Why is it bad that "X9.62 prime256v1" works but "prime256v1" doesn't? On 11/7/2018 10:05 PM, Weijun Wang wrote: In CurveDB.java, we have add("secp256r1 [NIST P-256, X9.62

Re: RFR CSR for 8213400: Support choosing curve name in keytool keypair generation

2018-11-08 Thread Adam Petcher
On 11/7/2018 8:53 PM, Weijun Wang wrote: Oh, I didn't know that. To make sure -keyalg matches KeyPairGenerator.getInstance(), I'd like to support it. If I read the impl correctly, you don't need to initialize it anymore and if you really want to initialize it the params must be the same.

Re: RFR CSR for 8213400: Support choosing curve name in keytool keypair generation

2018-11-07 Thread Adam Petcher
One issue that just came to me: How will this work for EdDSA? I think the CSR could be generalized a bit: 1) Make the first item in the "Solution" more general. Instead of limiting it to "EC" allow any valid algorithm/curve combination. 2) (Optional) Use -groupname instead of -curvename and

Re: RFR CSR for 8213400: Support choosing curve name in keytool keypair generation

2018-11-06 Thread Adam Petcher
On 11/6/2018 2:18 AM, Weijun Wang wrote: On Nov 6, 2018, at 1:06 PM, Xuelei Fan wrote: If the option "-keysize 256 -curvename sect163k1" work, I may think that the key size if 256 bits. I want to create a 256 bits sect163k1 EC key, and the tool allows this behavior, so I should get a 256

Re: Hashing in Java and Java Cryptography Architecture (JCA) design

2018-10-29 Thread Adam Petcher
There are several ways in which JCA is deficient when viewed from the standpoint of modern API design. At some point, we will probably want to develop a new API that doesn't have these issues. I expect this to require significant effort, though, and this sort of work gets lower priority

RFR 8205476: KeyAgreement#generateSecret is not reset for ECDH based algorithm

2018-10-17 Thread Adam Petcher
Webrev: http://cr.openjdk.java.net/~apetcher/8205476/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8205476 CSR: https://bugs.openjdk.java.net/browse/JDK-8212051 Please review the following change for a conformance bug in the ECDH service. The KeyAgreement is supposed to reset itself

Re: DSA default algorithm for keytool -genkeypair. Bad choice?

2018-10-11 Thread Adam Petcher
On 10/10/2018 5:05 PM, Anthony Scarpino wrote: On 10/10/2018 07:42 AM, Weijun Wang wrote: If not DSA, should RSA be the new default? Or maybe RSASSA-PSS (I wonder if RSASSA-PSS signature can always use legacy RSA keys) or EC? We don't have an option to specify ECCurve in keytool yet (a

Re: RFR: 8209862:CipherCore performance improvement

2018-10-08 Thread Adam Petcher
/~coffeys/webrev.8209862.v2/webrev/ regards, Sean. On 01/10/2018 15:32, Adam Petcher wrote: Looks like a nice improvement, but is it possible to do this without duplicating code? For example, code almost identical to this also appears starting on line 860: 971 } else { // encrypting 972 try { 973

Re: Conceptual feedback on new ECC JEP

2018-10-03 Thread Adam Petcher
here will be no CSR for this change. [1] https://bugs.openjdk.java.net/browse/JDK-8208698 On 9/25/2018 10:40 AM, Adam Petcher wrote: Thanks, everyone for your feedback on this JEP. I have incorporated this feedback (received on this mailing list and elsewhere) into the draft JEP[1]. Here is a summar

Re: RFR: 8209862:CipherCore performance improvement

2018-10-01 Thread Adam Petcher
Looks like a nice improvement, but is it possible to do this without duplicating code? For example, code almost identical to this also appears starting on line 860: 971 } else { // encrypting 972 try { 973 outLen = finalNoPadding(finalBuf, finalOffset, output, 974 outputOffset, finalBufLen);

Re: Conceptual feedback on new ECC JEP

2018-09-26 Thread Adam Petcher
On 9/25/2018 11:57 AM, Xuelei Fan wrote: On 9/25/2018 8:34 AM, Adam Petcher wrote: There will be nothing provider-dependent in the TLS implementation. The point of #3 is to say that we should test the TLS implementation to ensure that it will work with either "EC" provider

Re: Conceptual feedback on new ECC JEP

2018-09-25 Thread Adam Petcher
On 9/25/2018 11:15 AM, Xuelei Fan wrote: I did not follow the discussion.  But it does not sound right to me to have an application to be provider dependent (#3). There will be nothing provider-dependent in the TLS implementation. The point of #3 is to say that we should test the TLS

Re: Conceptual feedback on new ECC JEP

2018-09-25 Thread Adam Petcher
://bugs.openjdk.java.net/browse/JDK-8204574 On 8/23/2018 1:50 PM, Adam Petcher wrote: I'm starting work on yet another ECC JEP[1], this time with the goal of developing improved implementations of existing algorithms, rather than implementing new ones. The JEP will re-implement ECDH and ECDSA for the 256

Re: Conceptual feedback on new ECC JEP

2018-09-19 Thread Adam Petcher
On 9/19/2018 1:37 PM, Bernd Eckenfels wrote: Hello, I think I missed it, but where is the conversion on BigInteger branching on key material? Isn’t this only branching on effective constant values? Or are you concerned about Spectre-type problems? This is not for Spectre (etc.) issues,

Re: Conceptual feedback on new ECC JEP

2018-09-19 Thread Adam Petcher
On 9/19/2018 12:51 PM, Michael StJohns wrote: On 9/19/2018 11:45 AM, Adam Petcher wrote: My goal is for the new provider to be at least as interoperable as PKCS11 providers with non-exportable keys. Do all the PKCS11 providers that you have used allow importing private keys over JCA, or over

Re: Conceptual feedback on new ECC JEP

2018-09-19 Thread Adam Petcher
On 9/18/2018 4:24 PM, Michael StJohns wrote: Adam - Basically, the JCE is all about plugging in not about the implementations.  If this is truly an EC library, I should be able to get the benefit of your library with very minimal changes - e.g. specifying your provider in the various

Re: Conceptual feedback on new ECC JEP

2018-09-18 Thread Adam Petcher
On 9/11/2018 11:07 AM, Adam Petcher wrote: I still haven't been convinced that this lack of interoperability is a significant problem. In the proposed design, the new KeyFactory will not support ECPrivateKeySpec, and the implementation will produce private keys that inherit from PrivateKey

Re: Conceptual feedback on new ECC JEP

2018-09-11 Thread Adam Petcher
On 9/10/2018 7:49 PM, Xuelei Fan wrote: The motivation of the JEP is that some formulas may be more easier to expose attacks.  It's true that the formulas impact the security level of the implementation.  I was just wondering if the JEP proposed formulas have been well analyze.  A standard or

Re: Conceptual feedback on new ECC JEP

2018-09-10 Thread Adam Petcher
/23/2018 10:50 AM, Adam Petcher wrote: I'm starting work on yet another ECC JEP[1], this time with the goal of developing improved implementations of existing algorithms, rather than implementing new ones. The JEP will re-implement ECDH and ECDSA for the 256-, 384-, and 521-bit NIST prime curves

Re: Conceptual feedback on new ECC JEP

2018-09-10 Thread Adam Petcher
it is starting to get away from the intended use of these properties. On 9/7/2018 7:07 PM, Anthony Scarpino wrote: On Sep 7, 2018, at 12:08 PM, Adam Petcher wrote: This is a good suggestion. I don't have particularly strong feelings about using separate providers vs a property in a single prov

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

2018-09-10 Thread Adam Petcher
Fan wrote: On 9/7/2018 9:03 AM, Adam Petcher wrote: This is more clear, thanks. See below. On 9/7/2018 11:34 AM, Xuelei Fan wrote: EncodedKeySpec keySpec = ... // find a way to construct the keySpec // at least, we can use: //    new

Re: Conceptual feedback on new ECC JEP

2018-09-07 Thread Adam Petcher
ion. I am concerned the desire for a purest provider will result in it being unused. Documentation can be clear about the import/export situation, the preference toward PKCS8EncodedKeySpec, and the property to lock it down. Tony On Aug 23, 2018, at 10:50 AM, Adam Petcher wrote: I'm starting w

Re: Conceptual feedback on new ECC JEP

2018-09-07 Thread Adam Petcher
I'm only going to respond to one of your points in detail, see inline below. For the other points, I don't think we are going to agree. You want the implementation to be more interoperable, and I have stated that the level of interoperability that you want is not a goal of the JEP. This is

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

2018-09-07 Thread Adam Petcher
This is more clear, thanks. See below. On 9/7/2018 11:34 AM, Xuelei Fan wrote: EncodedKeySpec keySpec = ... // find a way to construct the keySpec // at least, we can use: //    new EncodedKeySpec(byte[]);

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

2018-09-07 Thread Adam Petcher
On 9/7/2018 9:34 AM, Xuelei Fan wrote: JSSE should use the 'x25519' name (via NamedParameterSpec object) only. This is the part that I don't know how to do. In JSSE, we convert between the array containing the encoded public key and the BigInteger representation of the public key used in

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

2018-09-07 Thread Adam Petcher
On 9/6/2018 4:49 PM, Xuelei Fan wrote: I asked the question in a previous email.  The key size for x25529 is fixed, right? Right. If it is not right, stop here and tell me that it is not right. Keep reading if it is right. OK, as the key size for x25519 is fixed, when you know the

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

2018-09-06 Thread Adam Petcher
On 9/6/2018 3:19 PM, Xuelei Fan wrote: I think I suggested to use NamedParameterSpec, which is a public API.    NamedParameterSpec -> name "x25519"    -> key size is the key size of x25519. Please let me know if the logic is wrong. It's that last arrow that I still don't get. How does

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

2018-09-06 Thread Adam Petcher
On 9/6/2018 1:55 PM, Xuelei Fan wrote: Yes, the key sizes are fixed. All we need in ECUtil is a mapping from curve name to this (fixed) size. Are you suggesting some other solution, other than using the XECParameters to map curve names to key sizes? Using name only (NamedParameterSpec?) and

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

2018-09-06 Thread Adam Petcher
Minor correction below. On 9/6/2018 1:04 PM, Adam Petcher wrote: In the latest webrev, I changed it so there is a single static NamedGroupFunctions of each type, and the NamedGroup is passed in as the first argument to each method that requires it (rather than being a member

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

2018-09-06 Thread Adam Petcher
On 9/6/2018 12:10 PM, Xuelei Fan wrote: The algorithm name is not quite sufficient. See the new methods that were added to ECUtil that encode/decode public keys. We also need to know the key length (which is in XECParameters) in order to encode/decode public keys. I did not get your point. 

Re: Conceptual feedback on new ECC JEP

2018-09-06 Thread Adam Petcher
On 9/5/2018 5:53 PM, Michael StJohns wrote: BigInteger is not required to encode a PKCS8 key, but its trivial to translate from BigInteger to PKCS8 and vice versa.  Note that internally, the current BigInteger stores the magnitude as an array of int in big endian order and stores the sign

Re: RFR 8171279: Support X25519 and X448 in TLS 1.3

2018-09-05 Thread Adam Petcher
Updated webrev: http://cr.openjdk.java.net/~apetcher/8171279/webrev.01/ On 9/4/2018 3:25 PM, Xuelei Fan wrote: I have no finished the full code review.  So far, I have a few question about the struct of the code. 1. XECParameters I can see the reason to dynamic parameters for something other

Re: Conceptual feedback on new ECC JEP

2018-09-04 Thread Adam Petcher
On 9/4/2018 2:01 PM, Michael StJohns wrote: Below *buzz* wrong answer.   Sorry.   The internal storage of the key can be anything you want it to be if you want to prevent a non-constant-time issue for normal calculation.  But the import/export of the key really isn't subject to the cargo

Re: Conceptual feedback on new ECC JEP

2018-09-04 Thread Adam Petcher
On 9/1/2018 2:03 PM, Michael StJohns wrote: On 8/23/2018 1:50 PM, Adam Petcher wrote: It will only support a subset of the API that is supported by the implementation in SunEC. In particular, it will reject any private keys with scalar values specified using BigInteger

RFR 8171279: Support X25519 and X448 in TLS 1.3

2018-08-30 Thread Adam Petcher
Webrev: http://cr.openjdk.java.net/~apetcher/8171279/webrev.00/ JBS: https://bugs.openjdk.java.net/browse/JDK-8171279 Please review the following change to add support for X25519 and X448 (XDH) to TLS 1.3. This change includes some refactoring to remove code that was duplicated for DH and

Re: RFR 8201317: X25519/X448 code improvements (xs)

2018-08-28 Thread Adam Petcher
/28/2018 9:58 AM, Adam Petcher wrote: I received some suggestions for improvements to the X25519/X448 code after the completion of the code review back in March. The improvements are some additional comments on methods and some rearranged expressions to prevent integer overflow. The change is

RFR 8201317: X25519/X448 code improvements (xs)

2018-08-28 Thread Adam Petcher
I received some suggestions for improvements to the X25519/X448 code after the completion of the code review back in March. The improvements are some additional comments on methods and some rearranged expressions to prevent integer overflow. The change is very small, please review if you have

Re: SSLSocket session resumption not working with TLS 1.3 and 11+27

2018-08-27 Thread Adam Petcher
On 8/24/2018 4:08 PM, Simone Bordet wrote: However, on the server-side the session ID is the same, so I am confused by the fact that on the client the session has been resumed, but it has a different ID. When the client receives a NewSessionTicket message, it copies the current SSLSession

Re: SSLSocket session resumption not working with TLS 1.3 and 11+27

2018-08-24 Thread Adam Petcher
Thanks for testing this. I developed most of the NewSessionTicket/PSK code. See below for my comments. On 8/23/2018 3:28 PM, Simone Bordet wrote: Hi, The code below reproduces a bug where session resumption is tested. Turns out that session resumption does happen on the client and on the

Conceptual feedback on new ECC JEP

2018-08-23 Thread Adam Petcher
I'm starting work on yet another ECC JEP[1], this time with the goal of developing improved implementations of existing algorithms, rather than implementing new ones. The JEP will re-implement ECDH and ECDSA for the 256-, 384-, and 521-bit NIST prime curves. The new implementation will be all

Re: RFR[11]: release note for JDK-7007966 "Add Brainpool ECC support (RFC 5639)"

2018-08-17 Thread Adam Petcher
No more suggestions. Looks good to me. On 8/16/2018 9:27 PM, Valerie Peng wrote: Sure, I've updated the release note. Please let me know if you have other suggestions/feedbacks. https://bugs.openjdk.java.net/browse/JDK-8208580 Thanks, Valerie On 8/16/2018 8:10 AM, Adam Petcher wrote

Re: RFR[11]: release note for JDK-7007966 "Add Brainpool ECC support (RFC 5639)"

2018-08-16 Thread Adam Petcher
This is in JCA only, and not yet integrated into SunJSSE, right? Either way, I think it would be helpful to add a sentence saying whether these curves are supported in SunJSSE. I expect that people will wonder about this when reading the release note. On 8/15/2018 7:02 PM, Valerie Peng

Re: Feedback on EdDSA API

2018-08-09 Thread Adam Petcher
On 8/9/2018 1:17 PM, Sean Mullan wrote: A few (mostly minor) comments on the Solution section. I'll continue my review of the rest of the CSR later. First sentence, "from the existing API ECDSA ..." should that be "API for ECDSA"? // example: use KeyFactory to contruct a public key typo:

Re: Please review EdDSA API

2018-07-26 Thread Adam Petcher
On 7/26/2018 5:05 PM, Michael StJohns wrote: The test vectors will not pass, because they are calling the byte array from which the private key and the signing value are derived as the private key. However, each and every signature generated by the above approach (e.g. using a *real*

Re: Please review EdDSA API

2018-07-26 Thread Adam Petcher
On 7/26/2018 3:58 PM, Michael StJohns wrote: On 7/25/2018 2:05 PM, Adam Petcher wrote: Did you mean PrivateKey ::= OctetToInteger(random)? Setting/clearing bits here destroys information. If we don't prune here, then we can reverse this operation later to get the byte array back to give

Re: Please review EdDSA API

2018-07-25 Thread Adam Petcher
On 7/25/2018 11:24 AM, Michael StJohns wrote: *sigh* Private keys are big integers.  There's an associated parameter used in signing that the implementation described in the RFC (*not a standard please note*) generates from a common random byte array - that byte array is NOT a (or the)

Re: Please review EdDSA API

2018-07-25 Thread Adam Petcher
+core-libs-dev for additional API expertise. On 7/25/2018 10:29 AM, Adam Petcher wrote: The draft CSR[1] for the EdDSA API[2] is ready for review. Please take a look and send me any feedback you may have. Here are a few high-level notes to explain the API: 1) Where possible, this API

Re: EC weirdness

2018-07-23 Thread Adam Petcher
On 7/19/2018 3:36 PM, Michael StJohns wrote: On 7/16/2018 4:42 PM, Adam Petcher wrote: Though it has the additional benefit... Actually... The implementation may also need... Nope... I think that you interpreted my statements a bit more specifically that I intended. I was speaking

Re: RFR 8206929: Check session context for TLS session resumption

2018-07-17 Thread Adam Petcher
On 7/16/2018 8:05 PM, Xuelei Fan wrote: On 7/16/2018 10:38 AM, Adam Petcher wrote: New webrev: http://cr.openjdk.java.net/~apetcher/8206929/webrev.02/ PreSharedKeyExtension.java  447 if (result && !shc.isNegotiable(s.getSuite()) ||  448 !clientHello.cipherSuites.

Re: RFR 8206929: Check session context for TLS session resumption

2018-07-16 Thread Adam Petcher
On 7/16/2018 3:04 PM, Xuelei Fan wrote: On 7/16/2018 10:38 AM, Adam Petcher wrote: Note that the relationship between sessions/PSKs and cipher suites is different in TLS 1.2 vs 1.3. In TLS 1.3, the cipher suite doesn't need to match---only the hash algorithm needs to match. I did not get

Re: RFR 8206929: Check session context for TLS session resumption

2018-07-16 Thread Adam Petcher
New webrev: http://cr.openjdk.java.net/~apetcher/8206929/webrev.02/ On 7/13/2018 4:55 PM, Xuelei Fan wrote: On 7/13/2018 12:13 PM, Adam Petcher wrote: This includes checking the peer supported signature algorithms. Maybe this is a good idea, but I think it is not in scope of this ticket

Re: RFR 8206929: Check session context for TLS session resumption

2018-07-13 Thread Adam Petcher
. Did we check #1 in somewhere else? Because this is specific to TLS 1.3+, I put it in the CHPreSharedKeyProducer. That is in PreSharedKeyExtension.java, near line 610 (in the new code in the Webrev). On 7/13/2018 9:10 AM, Adam Petcher wrote: On 7/13/2018 11:34 AM, Xuelei Fan wrote

Re: RFR 8206929: Check session context for TLS session resumption

2018-07-13 Thread Adam Petcher
On 7/13/2018 11:34 AM, Xuelei Fan wrote: PreSharedKeyExtension.java -- The local supported signature algorithms are checked in the canRejoin() method.  Should the peer supported signature algorithms be checked as well? I don't think so. When the peer creates its

Re: RFR 8206929: Check session context for TLS session resumption

2018-07-13 Thread Adam Petcher
Here's a new Webrev that includes this change: http://cr.openjdk.java.net/~apetcher/8206929/webrev.01/ On 7/12/2018 1:02 PM, Xuelei Fan wrote: Set it in PostHandshakeContext should be fine as the session should have been negotiated. Thanks, Xuelei On 7/12/2018 9:57 AM, Adam Petcher wrote

Re: RFR 8206929: Check session context for TLS session resumption

2018-07-12 Thread Adam Petcher
, Adam Petcher wrote: This change adds some checks for session resumption in TLS 1.3 to ensure that the resumed session is compatible with what is requested. Specifically, I'm adding checks for protocol version, cipher suite, client authentication, and signature schemes. There are also some minor

RFR 8206929: Check session context for TLS session resumption

2018-07-12 Thread Adam Petcher
This change adds some checks for session resumption in TLS 1.3 to ensure that the resumed session is compatible with what is requested. Specifically, I'm adding checks for protocol version, cipher suite, client authentication, and signature schemes. There are also some minor whitespace

Re: RFR 8206915: XDH TCK issues

2018-07-11 Thread Adam Petcher
On 7/11/2018 12:02 PM, Xuelei Fan wrote: Does it make sense if secret is not temporarily stored as a class filed? I agree that it's a bit strange, but it is organized this way because of the zero result check described in the RFC. If the result of the key agreement is zero, then that means

Re: RFR 8206915: XDH TCK issues

2018-07-11 Thread Adam Petcher
On 7/11/2018 10:41 AM, Sean Mullan wrote: XDHKeyAgreement.java 176 byte[] result = secret; Shouldn't this be: 176 byte[] result = secret.clone(); since engineGenerateSecret() says it is returned in a new buffer. I don't think cloning is necessary. The new array is created

RFR 8206915: XDH TCK issues

2018-07-09 Thread Adam Petcher
A couple of conformance issues were found during TCK development for XDH. Both required very small modifications to fix, so I put them under the same ticket/review in order to save time. The actual information about the issues can be found in the sub-tasks of the ticket. This fix needs to go

Feedback on EdDSA API

2018-06-27 Thread Adam Petcher
I'm looking for some initial feedback on the proposed JCA API for EdDSA[2], which I have documented in a draft CSR ticket[1]. Any comments, concerns, suggestions, etc are appreciated. To summarize, the API for EdDSA looks a lot like the API for X25519/X448. Like X25519/X448, it does not allow

Re: RFR 8203228: Branch-free output conversion for X25519 and X448

2018-06-25 Thread Adam Petcher
It would be nice to get this X25519/X448 enhancement into JDK 11. If anyone has some time to review this in the next day or so, I would appreciate it. On 5/15/2018 2:42 PM, Adam Petcher wrote: Webrev: http://cr.openjdk.java.net/~apetcher/8203228/webrev.00/ Please review the change

Re: Code Review Request: TLS 1.3 Implementation

2018-06-07 Thread Adam Petcher
Updates to address these comments: http://hg.openjdk.java.net/jdk/sandbox/rev/2dc6efcdeb11 http://hg.openjdk.java.net/jdk/sandbox/rev/97447478b7da On 6/3/2018 8:24 PM, Xuelei Fan wrote: > http://cr.openjdk.java.net/~xuelei/8196584/webrev-full.00 PskKeyExchangeModesExtension.java

Re: Code Review Request: TLS 1.3 Implementation

2018-06-06 Thread Adam Petcher
On 6/4/2018 9:43 AM, Adam Petcher wrote: This webrev also includes a small change I made to enable TLS 1.3 by default in the HTTP client: http://hg.openjdk.java.net/jdk/sandbox/rev/2a396fdb3afb This change has been backed out. The default TLS version in the HTTP client will be addressed

Re: Code Review Request: TLS 1.3 Implementation

2018-06-04 Thread Adam Petcher
This webrev also includes a small change I made to enable TLS 1.3 by default in the HTTP client: http://hg.openjdk.java.net/jdk/sandbox/rev/2a396fdb3afb On 6/4/2018 12:43 AM, Xuelei Fan wrote: Hi, Here it the 2nd full webrev:   http://cr.openjdk.java.net/~xuelei/8196584/webrev-full.01 and

RFR 8203228: Branch-free output conversion for X25519 and X448

2018-05-15 Thread Adam Petcher
Webrev: http://cr.openjdk.java.net/~apetcher/8203228/webrev.00/ Please review the change for this leftover task from the X25519/X448 JEP. The current code uses BigInteger to convert the final result from a field element to a byte array that can be used to derive a key. Using branch-free

JEP Proposal: EdDSA Signatures

2018-05-01 Thread Adam Petcher
The work for X25519/X448 is wrapping up[1], so I have started thinking about EdDSA. Please review the draft JEP[2] for EdDSA, and let me know what you think. I'm not really sure what priority to give the EdDSA work, and it is optional in TLS 1.3. So if you have a particular need or desire for

Re: SHAKE XOFs

2018-04-11 Thread Adam Petcher
On 4/11/2018 5:37 AM, Bernd Eckenfels wrote: Hello, I noticed that the OASIS draft for extending PKCS#11 with SHA-3 also specifies new Mechanisms for SHAKE128/256. They introduce them as Key Derivation functions. Interesting. Though to be pedantic, it looks like they introduce key

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

2018-03-28 Thread Adam Petcher
On 3/28/2018 2:48 AM, sha.ji...@oracle.com wrote: Would you like to move this method to a test lib class, like test/lib/jdk/test/lib/Utils.java? In fact, this class has a method, named toHexString, for converting bin to hex. This method appears to be the same as Convert.hexStringToByteArray

RFR 8171277: Elliptic Curves for Security in Crypto (part 2)

2018-03-27 Thread Adam Petcher
After the last code review[1] on this topic completed, it was suggested that I add some more "spec enforcement" to the XDH service. The code hasn't been integrated yet, so I'm doing this as a follow-on review under the same ticket. The latest webrev contains only the diff from the end of the

Re: RFR: 8200219: Develop new tests for using new elliptic curves: curve25519 and curve448

2018-03-27 Thread Adam Petcher
I have a couple of minor comments. I am not a Reviewer, so someone else will still need to look at this. KeySizeTest: You can use the byteArrayToHexString that is in Convert in the test lib. See TestXDH.java for an example of how this method is imported and used. MultiThreadTest: In

Re: RFR 8171277: Elliptic Curves for Security in Crypto

2018-03-20 Thread Adam Petcher
always do it later. --Sean On 3/12/18 3:03 PM, Sean Mullan wrote: On 3/9/18 8:25 AM, Adam Petcher wrote: New webrev: http://cr.openjdk.java.net/~apetcher/8171277/webrev.01/ I think somewhere there should be a sentence or two on the difference between XECKeys and ECKeys and when you would want

Re: RFR 8181594: Efficient and constant-time modular arithmetic

2018-03-20 Thread Adam Petcher
Latest webrev: http://cr.openjdk.java.net/~apetcher/8181594/webrev.02/ Comments inline below. In addition, I also changed the name of IntegerModuloP_Base to IntegerModuloP, and IntegerModuloP to ImmutableIntegerModuloP. On 3/11/2018 12:04 PM, Xuelei Fan wrote: On 2/26/2018 10:39 AM, Adam

  1   2   >