Re: [RFR] 8166597: Crypto support for the EdDSA Signature Algorithm (JEP 339)

[Weijun Wang]

Another question:

Why does getAlgorithm() of PublicKey and PrivateKey return "EdDSA" instead of 
"Ed25519" and "Ed448"?

Do we suggest people using "EdDSA" or "Ed25519"/"Ed448" when calling 
KeyFactory.getInstance() and KeyPairGenerator.getInstance()?

Thanks,
Max

> On Mar 24, 2020, at 2:53 AM, Anthony Scarpino  
> wrote:
> 
> On 2/25/20 12:49 PM, Anthony Scarpino wrote:
>> Hi
>> I need a code review for the EdDSA support in JEP 339.  The code builds on 
>> the existing java implemented constant time classes used for XDH and the 
>> NIST curves.  The change also adds classes to the public API to support 
>> EdDSA operations.
>> All information about the JEP is located at:
>> JEP 339: https://bugs.openjdk.java.net/browse/JDK-8199231
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8190219
>> webrev: https://cr.openjdk.java.net/~ascarpino/8166597/webrev/
>> thanks
>> Tony
> 
> 
> I updated the webrev with some minor updates that were commented previously.
> 
> https://cr.openjdk.java.net/~ascarpino/8166597/webrev.01/
> 
> Tony

Re: RFR 8241960: The SHA3 message digests are not thread safe when cloned

[Valerie Peng]

Hi Alexey,

In general looks pretty good, just some comments on the regression test:

- line 28: The duration value 10 may be lowered to shorten the execution 
time. On a Linux machine, with threadFactor=5, each digest algo takes 
about (2xduration+2) sec and overall takes ~283sec. Cutting the duration 
value drastically to 2, I still can reproduce the bug. Maybe 4 would be 
a better default value considering the execution time.


- line 43: add "SHA-512/224", "SHA-512/256" to the ALGORITHM_ARRAY?

- line 50: instead of hardcoding 5 here, why not just use threadsFactor 
(assigned with default value 5 on line 48)?


- line 52-55: I think you meant to use 5 (instead of the value 180) as 
the default min duration on line 52. Then, use duration variable instead 
of the hardcoded  5 on line54. For testing purpose, why not just use the 
supplied value? You already have default values if none are supplied.


- Consider iterating through existing providers and remove 
isSHA3supported() method. This way all providers which supports the 
digest algorithm are tested and no provider-specific knowledge is 
needed. For example:


    Provider[] provs = Security.getProviders();

    for (Provider p : provs) {

    System.out.println("Testing provider " + p.getName() + "...");

    for (String alg: ALGORITHM_ARRAY) {

    try {

    MessageDigest md = MessageDigest.getInstance(alg, p);

    testThreadSafe(md, input, nTasks, duration, false);

                

- line 76: missing space char between } and catch.

Thanks,
Valerie
On 3/31/2020 1:01 PM, Valerie Peng wrote:

Hi Alexey,

Good catch, thanks for the report, I will review it.

On a first look, it seems that this is more about the clone() method 
of the SHA-3 impl missed copying/cloning an internal field.


Given that this is about SUN provider, I've modified the synopsis 
accordingly and move the priority up to P3.


It may not take multi-thread to reproduce this? If so, we can simplify 
the regression test.


Regards,
Valerie
On 3/31/2020 11:27 AM, Alexey Bakhtin wrote:

Hi All,

Please review fix for SHA3 message digests thread safety.
Issue reproduced on the JDK11, JDK13 and JDK14
JTREG test is provided in the patch

JBS: https://bugs.openjdk.java.net/browse/JDK-8241960
Webrev: https://cr.openjdk.java.net/~abakhtin/8241960/webrev.v0/

Regards
Alexey

Re: RFR 8241888: Mirror jdk.security.allowNonCaAnchor system property with a security one

[Martin Balao]

Hi,

Webrev.02:

 * http://cr.openjdk.java.net/~mbalao/webrevs/8241888/8241888.webrev.02

On 4/2/20 5:02 PM, Sean Mullan wrote:
> 
> In the java.security file might add the sentence "The default value of
> the property is "false"" just to avoid any confusion.
> 

Added.

Thanks,
Martin.-

Re: RFR 8241888: Mirror jdk.security.allowNonCaAnchor system property with a security one

[Sean Mullan]

On 4/1/20 5:53 PM, Martin Balao wrote:

Fixed.

Webrev.01:

  *http://cr.openjdk.java.net/~mbalao/webrevs/8241888/8241888.webrev.01/


In the java.security file might add the sentence "The default value of 
the property is "false"" just to avoid any confusion.


--Sean

RE: RFR[jdk] 8237474: Default SSLEngine should create in server role

[Prasadrao Koppula]

Thanks for review Xuelei, I will incorporate your suggestions. 

Thanks,
Prasad.K

> -Original Message-
> From: Xuelei Fan
> Sent: Thursday, April 2, 2020 9:12 PM
> To: security-dev@openjdk.java.net
> Subject: Re: RFR[jdk] 8237474: Default SSLEngine should create in server role
> 
> Please update copyright year to 2020.
> 
> SSLEngine.java
> --
> @@ -1109,10 +1115,14 @@
> + * @implNote
> + * The JDK SunJSSE provider implementation returns false unless
> {@link setUseClientMode}
> + *  is used to change the mode to true.
> 
> For the link, I may add parameter, and limit the line under 80 characters, and
> don't indent the lines.
> 
> + * @implNote
> - * The JDK SunJSSE provider implementation returns false unless
> {@link setUseClientMode}
> - *  is used to change the mode to true.
> + * The JDK SunJSSE provider implementation returns false unless
> + * {@link setUseClientMode(boolean)} is used to change the mode
> +  * to true.
> 
> It's fine to leave the CSR  as it is.
> 
> Otherwise, looks fine to me.
> 
> Xuelei
> 
> On 3/30/2020 6:50 AM, Prasadrao Koppula wrote:
> > Hi,
> >
> > Added @implnote and updated test changes, here is the new webrev,
> > please review it.
> >
> > Webrev: http://cr.openjdk.java.net/~pkoppula/8237474/webrev.01/
> >
> > issue: https://bugs.openjdk.java.net/browse/JDK-8237474
> >
> > CSR: https://bugs.openjdk.java.net/browse/JDK-8238593
> >
> > Thanks,
> >
> > Prasad.K
> >
> > *From:* Prasadrao Koppula
> > *Sent:* Friday, February 7, 2020 5:03 PM
> > *To:* security-dev@openjdk.java.net
> > *Subject:* RFR[jdk] 8237474: Default SSLEngine should create in server
> > role
> >
> > Hi,
> >
> > Could you please review this patch. Default server role mode was
> > flipped in SSLEngine, to client role mode as part of SSL package code
> > refactoring for TLSv1.3, this patch flips back default client role to
> > server role in SSLEngine.
> >
> > webrev: http://cr.openjdk.java.net/~pkoppula/8237474/webrev.00/
> >
> > issue: https://bugs.openjdk.java.net/browse/JDK-8237474
> >
> > CSR: https://bugs.openjdk.java.net/browse/JDK-8238593
> >
> > Thanks,
> >
> > Prasad.K
> >

Re: RFR[jdk] 8237474: Default SSLEngine should create in server role

[Xuelei Fan]

Please update copyright year to 2020.

SSLEngine.java
--
@@ -1109,10 +1115,14 @@
+ * @implNote
+ * The JDK SunJSSE provider implementation returns false unless 
{@link setUseClientMode}

+ *  is used to change the mode to true.

For the link, I may add parameter, and limit the line under 80 
characters, and don't indent the lines.


+ * @implNote
- * The JDK SunJSSE provider implementation returns false unless 
{@link setUseClientMode}

- *  is used to change the mode to true.
+ * The JDK SunJSSE provider implementation returns false unless
+ * {@link setUseClientMode(boolean)} is used to change the mode
+  * to true.

It's fine to leave the CSR  as it is.

Otherwise, looks fine to me.

Xuelei

On 3/30/2020 6:50 AM, Prasadrao Koppula wrote:

Hi,

Added @implnote and updated test changes, here is the new webrev, please 
review it.


Webrev: http://cr.openjdk.java.net/~pkoppula/8237474/webrev.01/

issue: https://bugs.openjdk.java.net/browse/JDK-8237474

CSR: https://bugs.openjdk.java.net/browse/JDK-8238593

Thanks,

Prasad.K

*From:* Prasadrao Koppula
*Sent:* Friday, February 7, 2020 5:03 PM
*To:* security-dev@openjdk.java.net
*Subject:* RFR[jdk] 8237474: Default SSLEngine should create in server role

Hi,

Could you please review this patch. Default server role mode was flipped 
in SSLEngine, to client role mode as part of SSL package code 
refactoring for TLSv1.3, this patch flips back default client role to 
server role in SSLEngine.


webrev: http://cr.openjdk.java.net/~pkoppula/8237474/webrev.00/

issue: https://bugs.openjdk.java.net/browse/JDK-8237474

CSR: https://bugs.openjdk.java.net/browse/JDK-8238593

Thanks,

Prasad.K

Re: Possible regression in JDK 14 related to SSLSessionContext / SSLSession on the server side

[Norman Maurer]

Thanks a lot… 

Happy to be able to help here.

Once you have a fix ready let me know and I can verify it with the netty 
testsuite.

Bye
Norman


> On 1. Apr 2020, at 23:37, Jamil Nimeh  wrote:
> 
> Hi Norman, session context issue is here:
> 
> https://bugs.openjdk.java.net/browse/JDK-8242008 
> 
> --Jamil
> 
> On 4/1/2020 12:32 AM, Norman Maurer wrote:
>> Please add a link to the bug once it is created.
>> 
>> Bye
>> Norman
>> 
>> 
>>> On 31. Mar 2020, at 17:35, Jamil Nimeh >> > wrote:
>>> 
>>> Thanks Norman, I'm going to file a bug on this one.  After playing with it 
>>> a bit more I found cases where even SSLServerSockets do run into the issue 
>>> but it doesn't always happen.  Still working on characterizing it.
>>> 
>>> --Jamil
>>> 
>>> On 3/31/2020 7:11 AM, Norman Maurer wrote:
 Yes thats about right… if setting to false it works as expected.
 
 
 Bye
 Norman
 
 
> On 31. Mar 2020, at 01:50, Jamil Nimeh  > wrote:
> 
> Hi Norman,
> 
> I've been able to run your test code and I can reproduce it.  
> Interestingly enough, it appears to happen when 
> -Djdk.tls.server.enableSessionTicketExtension=true, which is the default 
> position.  With session tickets enabled, I would see the issue in TLS 1.3 
> and 1.2 connections just as you did.  Setting the above property to false 
> however allowed me to make successful connections.  Would you mind 
> setting that property to false, just to make sure you and I see the same 
> thing?
> 
> I did go back and run SSLServerSocket-based connections just to see if 
> the session ticket settings had any impact on things, but they don't.  I 
> can make connections to a socket based SSL server regardless of the 
> property value on the server side.
> 
> Thanks,
> 
> --Jamil
> 
> On 3/30/2020 5:31 AM, Norman Maurer wrote:
>> Hey Sean,
>> 
>> There is not much to share as its just a simple handshake :)
>> 
>> Anyway here is a reproducer:
>> 
>> https://github.com/normanmaurer/jdk_ssl_session_context_reproducer 
>> 
>> 
>> It basically does nothing more then complete the handshake and then 
>> calling engine.getSession().getSessionContext() which will return null 
>> on the server side since JDK 14 (earlier versions work).
>> I tested it with TLSv1.2 and TLSv1.3 and both times it produced the 
>> error on JDK 14.
>> 
>> 
>> Bye
>> Norman
>> 
>> 
>>> On 30. Mar 2020, at 13:22, Seán Coffey >> > wrote:
>>> 
>>> Looks interesting Norman. Do you want to share some more details about 
>>> the peculiarities of this handshake before considering a fully fledged 
>>> testcase ?
>>> 
>>> regards,
>>> Sean.
>>> 
>>> On 27/03/2020 12:48, Norman Maurer wrote:
 Hi there,
 
 I am just about to add JDK14 to the test matrix for netty and think I 
 found a regression. Before I will invest time to write a standalone 
 reproducer please let me know if you think this is a regression or not.
 Basically after the handshake is complete 
 SSLEngine.getSession().getSessionContext() returns null on the 
 serverside when using JDK14. Running the same test with any previous 
 version (JDK13 and earlier) doesn’t show the same result.
 
 Does this sounds like a regression and if so should I provide a 
 standalone reproducer here ?
 
 Bye
 Norman
 
>> 
 
>> 

Re: [RFR] 8166597: Crypto support for the EdDSA Signature Algorithm (JEP 339)

[Weijun Wang]

One more thing:

https://tools.ietf.org/html/rfc8410#section-1

   In [RFC8032] the elliptic curve signature system Edwards-curve
   Digital Signature Algorithm (EdDSA) is described along with a
   recommendation for the use of the curve25519 and curve448.  EdDSA has
   defined two modes: the PureEdDSA mode without prehashing and the
   HashEdDSA mode with prehashing.  The convention used for identifying
   the algorithm/curve combinations is to use "Ed25519" and "Ed448" for
   the PureEdDSA mode.

Does this mean we should reject prehash=true for "Ed25519" and "Ed448"?

Thanks,
Max

Re: [15] RFR 8241761 : Typos: empty lines in javadoc, inconsistent indents, etc. (security-libs only)

[Ivan Gerasimov]

Thank you Max!

I've made the suggested adjustment and pushed the fix.

With kind regards,

Ivan

On 4/1/20 2:59 AM, Weijun Wang wrote:

Looks fine to me.

If I have to pick one tiny problem, you can remove a blank in line 58 below to 
make the indentation more precise:

src/java.security.sasl/share/classes/javax/security/sasl/SaslServer.java:

   57  * status = ldap.sendBindResponse(mechanism, challenge,
   58  *  SASL_BIND_IN_PROGRESS);

Thanks,
Max



On Mar 31, 2020, at 5:39 AM, Ivan Gerasimov  wrote:

Hello!

The fix follows up on JDK-8241727 [1].

This is a javadoc/comments only fix in the security-libs area.

The changes are to remove redundant empty lines, correct indentation, or 
otherwise restore harmony.

Would you please help review this rather technical fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8241761
WEBREV: http://cr.openjdk.java.net/~igerasim/8241761/00/webrev/

Thank in advance!

[1] https://bugs.openjdk.java.net/browse/JDK-8241727

--
With kind regards,
Ivan Gerasimov


--
With kind regards,
Ivan Gerasimov