Re: RFR 8246640: @systemproperty should be @systemProperty in java.security.jgss

[Xuelei Fan]

Looks good to me.

Xuelei

On 6/4/2020 8:18 PM, Weijun Wang wrote:

Please review the patch below. The tag name should be camelCased.

*diff --git 
a/src/java.security.jgss/share/classes/javax/security/auth/kerberos/KerberosPrincipal.java 
b/src/java.security.jgss/share/classes/javax/security/auth/kerberos/KerberosPrincipal.java*
*--- 
a/src/java.security.jgss/share/classes/javax/security/auth/kerberos/KerberosPrincipal.java*
*+++ 
b/src/java.security.jgss/share/classes/javax/security/auth/kerberos/KerberosPrincipal.java*

@@ -106,7 +106,7 @@
       *
       * If the input name does not contain a realm, the default realm
       * is used. The default realm can be specified either in a Kerberos
-     * configuration file or via the {@systemproperty 
java.security.krb5.realm}
+     * configuration file or via the {@systemProperty 
java.security.krb5.realm}

       * system property. For more information, see the
       * {@extLink security_guide_jgss_tutorial Kerberos Requirements}.
       *
@@ -155,7 +155,7 @@
       *
       * If the input name does not contain a realm, the default realm
       * is used. The default realm can be specified either in a Kerberos
-     * configuration file or via the {@systemproperty 
java.security.krb5.realm}
+     * configuration file or via the {@systemProperty 
java.security.krb5.realm}

       * system property. For more information, see the
       * {@extLink security_guide_jgss_tutorial Kerberos Requirements}.
       *

Thanks,
Max

RFR JDK-8239950: Update PKCS9 Attributes to PKCS#9 v2.0 Encodings

[Jamil Nimeh]

Hello all,

This brings a few PKCS#9 attributes (unstructuredName, 
unstructuredAddress, signingTime) into line with v2.0 of the spec (RFC 
2985).  It mostly expands the allowed string or date types for these 
attributes.  I also came across a corner-case bug where toString calls 
on PKCS9Attribute objects were throwing NPE if the attribute type was a 
UniversalString, so that is addressed in this webrev too.


Webrev: https://cr.openjdk.java.net/~jnimeh/reviews/8239950/webrev.01

JBS: https://bugs.openjdk.java.net/browse/JDK-8239950

--Jamil

RE: [15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering

[Prasadrao Koppula]

Hi,

Looks good to me, one question
 
If first registered SecureRandom algo gets removed, 
getDefaultSecureRandomAlgorithm return stale data, a refresh required in remove?

Thanks,
Prasad.K  

>-Original Message-
>From: Valerie Peng
>Sent: Friday, June 5, 2020 2:52 AM
>To: security-dev@openjdk.java.net
>Subject: Re: [15] RFR JDK-8246613: Choose the default SecureRandom algo
>based on registration ordering
>
>Hi, Sean,
>
>Thanks for the review and feedback. Webrev updated:
>http://cr.openjdk.java.net/~valeriep/8246613/webrev.01/
>
>getTypeAndAlgorithm(...) was not static due to an instance variable used by
>debugging output. I have removed it and made both method static.
>
>I will wait for others' review comments also.
>
>Thanks,
>Valerie
>On 6/4/2020 2:01 PM, Sean Mullan wrote:
>> On 6/4/20 3:34 PM, Valerie Peng wrote:
>>> Hi,
>>>
>>> Could someone help reviewing this fix? This change keep tracks of the
>>> first registered SecureRandom algorithm and returns it upon the
>>> request of SecureRandom class.
>>
>> This looks good to me. I would recommend that Max or someone else
>> review it as well.
>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8246613
>>>
>>> Webrev: http://cr.openjdk.java.net/~valeriep/8246613/webrev.00/
>>
>> A couple of minor comments, feel free to fix or ignore.
>>
>> * SecureRandom.java
>>
>> 879 // For SUN provider, we use
>> SunEntries.DEFF_SECURE_RANDOM_ALGO
>>
>> Might as well fix the typo while you are in there: s/DEFF/DEF/
>>
>> * Provider.java
>>
>> 1156 private String parseSecureRandomPut(String name, String
>> value) {
>>
>> Could be static if you also make getTypeAndAlgorithm static, I think.
>>
>> --Sean

Re: RFR[15] JDK-8243114: Implement montgomery{Multiply, Square}intrinsics on Windows

[David Holmes]

Hi Simon,

On 4/06/2020 11:35 pm, Simon Tooke wrote:

Hello, David, and thanks for the review!

I've responded to your comments below, and intend to post a new patch 
for review today or tomorrow.


Thanks again,

-Simon

On 2020-06-03 2:06 a.m., David Holmes wrote:

Hi Simon,

On 23/05/2020 12:04 am, Sean Mullan wrote:
Cross-posting to hotspot-dev for additional review since the code 
changes are in hotspot.


--Sean

On 5/21/20 1:24 PM, Simon Tooke wrote:

Hello,

I'd like to request a review for:

JBS: https://bugs.openjdk.java.net/browse/JDK-8243114

Webrev: 
http://cr.openjdk.java.net/~stooke/webrevs/jdk-8243114-jdk/00/00/


This change implements the given intrinsics on Windows.

The Windows toolchain has no 128 bit integer types, and no inline 
asm (on x64 and Arm).  In addition, 'long' is 4 bytes, not 8, as it 
is with gcc.  This patch had to change some of the linux 
implementation to account for these limitations.


I can't really comment on the intrinsics themselves but a couple of 
suggestions:


- use explicitly sized types e.g. uint64_t instead of unsigned long long


Yes, this hurt to type.  A previous review suggested using julong, is 
that acceptable to you?


j* types should only be used when dealing with values that come from or 
go to Java. Obviously julong is a misnomer as Java doesn't have an 
unsigned type, but in general this is something we are trying to fix up 
in the codebase. If these arrays are extracted from Java arrays then 
using a j* type would be appropriate, but then I would expect jlong, 
unless the algorithm explicitly requires unsigned types? If so julong 
would be acceptable.


(an aside: I'm now wondering if there is other code in the JDK that 
assumes long is 64bits - which is not true on all platforms...)


There has been such code, but hopefully there is no remaining actively 
used code with that bug. There are using of "long" that should be 
eradicated (there's an open JBS issue for that as I recall) but the 
remaining uses don't seem to require the long be 64-bit.



- use the existing NOINLINE macro for the _declspec(noinline)

Thanks, will do.


The conditional compilation in this code has me quite confused. 
Looking at the existing code we have:


3680 #ifndef _WINDOWS
3681
3682 #define ASM_SUBTRACT
3683
3684 #ifdef ASM_SUBTRACT
...
3702 #else // ASM_SUBTRACT
...
3719 #endif // ! ASM_SUBTRACT

So all the above code is only built on not-Windows, and we 
unconditionally define ASM_SUBTRACT, so lines 3702 to 3719 appear to 
be dead code! I'm not at all sure whether that is actually a bug and 
the windows ifndef should have had an endif after line 3682; or 
whether we can just simplify the code.
The dead code existed prior to this patch, so I wasn't proposing 
removing it.  I'm happy to do so if that's for the best.  As far as I 
can tell, it's for implementing these intrinsics on gcc platforms 
without assembler.


AFAICS Andrew originally had the ASM_SUBTRACT parts, with the 
unconditional #define, but there was no explanation in the review thread 
as to why the unused code remained present. Then before integration all 
the code was wrapped by the ifndef Windows to exclude it from Windows.


I think it needs to be fixed as you are making changes to the Windows 
part and it is very hard to establish how the dead code should look in 
that case.


Thanks,
David



Thanks,
David



My gratitude for Andrew Haley for doing the heavy lifting at the 
core of this patch.


The patch, if accepted, will be offered to 11u as a potential 
backport. The changes apply cleanly modulo some line number changes.


As for the speedup, this test case:

BigInteger base = BigInteger.ONE.shiftLeft(1024);
long count = LongStream.rangeClosed(2, 100_000)
 .mapToObj(n -> BigInteger.valueOf(n).add(base))
 .filter(i -> i.isProbablePrime(50))
 .count();

goes from 1 minute 20 seconds down to about 35 seconds om my VM, 
over multiple runs.  As is the case on other platforms where the 
intrinsics are supported, they will be enabled by default on Windows.


Thank you for your time,

-Simon Tooke


Principal Software Engineer,

Red Hat Canada

Re: RFR:8246330:Add TLS Tests for Legacy ECDSA curves

[shivangi . g . gupta]

Corrected the links.

On 04/06/20 10:59 pm, shivangi.g.gu...@oracle.com wrote:


Hi,

May I please find a sponsor for this patch?

Bug: https://bugs.openjdk.java.net/browse/JDK-8246330

Description:Many of EC named curves has been disabled with security 
property "jdk.disabled.namedCurves". The purpose of this Test is to 
verify the behavior of any EC named curve from the disabled list.

This Test will address a single name from the disabled list as sect283r1.


Webrev: http://cr.openjdk.java.net/~sshivang/reviews/8246330/webrev.00/

The patch has been tested on mach5, and the individual test was passed.

Thanks

Shivangi

Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos

[Weijun Wang]

Hi Alexey,

It's so unfortunate that different addressType must be used. I'm OK with the 
new TlsChannelBindingImpl class.

One thing I'm not comfortable is the 
java.security.sasl/share/classes/module-info.java change. We've struggled hard 
to avoid these kind of secret tunnels. Is it possible to pass the 
tlsCB.getData() directly to the SASL mechanism? The property name is clear 
enough to avoid any misuse.

Another question: can an application user set the 
"com.sun.security.sasl.tlschannelbinding" property? and can they read it after 
it's set internally? I cannot think of a good attack now but at least it seems 
they have no need to access that property value. If we keep it internal then we 
also have the chance to modify the approach later.

Thanks,
Max


> On Jun 5, 2020, at 2:15 AM, Alexey Bakhtin  wrote:
> 
> Hello,
> 
> Could you please review new version of the patch:
> http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v3/
> 
> I’ve moved all logic for creating TLS Channel Binding data out of  
> GssKrb5Client.java file.
> All data is prepared inside TlsChannelBinding class.
> Internal property name is renamed to 
> “com.sun.security.sasl.tlschannelbinding”.
> TlsChannelBinding object is still used to pass channel binding data from 
> LdapClient to GssKrb5Client.
> I do not change it to byte[] because of TlsChannelBinding class is still used 
> for internal property name.
> Also, I’ve updated TlsChannelBinding class to select SHA-256 hash algorithm 
> if it can not be derived
> from the certificate signature name.
> 
> Regards
> Alexey
> 
> 
>> On 26 May 2020, at 17:50, Weijun Wang  wrote:
>> 
>> I have a question on GssKrb5Client.java:
>> 
>> Do you think it's a good idea to let the SASL mechanism understand what TLS 
>> binding is? Instead of passing the whole TlsChannelBinding object through a 
>> SASL property, is it possible to only pass the actual cbData? After all, the 
>> name "com.sun.security.sasl.channelbinding" suggests it's just a general 
>> ChannelBinding which is independent with any application level info.
>> 
>> From my reading of the code change, it looks like this cbData can be 
>> calculated on the LDAP side.
>> 
>> Thanks,
>> Max
>> 
>>> On May 21, 2020, at 3:35 PM, Alexey Bakhtin  wrote:
>>> 
>>> Hello,
>>> 
>>> Could you please review the following patch:
>>> 
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8245527
>>> Webrev: http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v0/
>>> 
>>> The Windows LDAP server with LdapEnforceChannelBinding=2 uses the 
>>> tls-server-end-point channel binding
>>> (based on the TLS server certificate). The channel binding data is 
>>> calculated as following :
>>> • The client calculates a hash of the TLS server certificate.
>>> The hash algorithm is selected on the base of the certificate 
>>> signature algorithm.
>>> Also, the client should use SHA-256 algorithm, in case of the 
>>> certificate signature algorithm is SHA1 or MD5 based
>>> • The channel binding information is the same as defined in rfc4121 [1] 
>>> with small corrections:
>>> • initiator and acceptor addresses should be set to NULL
>>> • initiator and acceptor address types should be zero.
>>>It contradicts to the “Using Channel Bindings in GSS-API” 
>>> document [2] that say that
>>>the address type should be set to GSS_C_AF_NULLADDR=0xFF,
>>>instead of GSS_C_AF_UNSPEC=0x00.
>>> 
>>> This patch introduces changes in the LDAP, SASL and JGSS modules
>>> to generate channel binding data automatically if requested by an 
>>> application.
>>> This patch reuses existing org.ietf.jgss.ChannelBinding class 
>>> implementation but changes
>>> initial unspecified address type from CHANNEL_BINDING_AF_NULL_ADDR to 
>>> CHANNEL_BINDING_AF_UNSPEC.
>>> The patch introduces new environment property 
>>> “com.sun.jndi.ldap.tls.cbtype” that indicates
>>> Channel Binding type that should be used in the LDAPS connection over 
>>> JGSS/Kerberos.
>>> Right now "tls-server-end-point" Channel Binding type is supported only.
>>> The client extracts server certificate from the underlying TLS connection 
>>> and creates
>>> Channel Binding data for JGSS/Kerberos authentication if application 
>>> indicates
>>> com.sun.jndi.ldap.tls.cbtype=tls-server-end-point property.
>>> Client application should also specify existing 
>>> "com.sun.jndi.ldap.connect.timeout” property
>>> to force and wait TLS handshake completion before JGSS/Kerberos 
>>> authentication data is generated.
>>> 
>>> [1] - https://tools.ietf.org/html/rfc4121#section-4.1.1.2
>>> 
>>> [2] -
>>> https://docs.oracle.com/cd/E19120-01/open.solaris/819-2145/overview-52/index.html
>>> 
>>> Initial discussion of this issue is available at security-dev list:
>>> https://mail.openjdk.java.net/pipermail/security-dev/2019-December/021052.html
>>> https://mail.openjdk.java.net/pipermail/security-dev/2020-January/021140.html
>>> https://mail.op

RFR 8246640: @systemproperty should be @systemProperty in java.security.jgss

[Weijun Wang]

Please review the patch below. The tag name should be camelCased.

diff --git 
a/src/java.security.jgss/share/classes/javax/security/auth/kerberos/KerberosPrincipal.java
 
b/src/java.security.jgss/share/classes/javax/security/auth/kerberos/KerberosPrincipal.java
--- 
a/src/java.security.jgss/share/classes/javax/security/auth/kerberos/KerberosPrincipal.java
+++ 
b/src/java.security.jgss/share/classes/javax/security/auth/kerberos/KerberosPrincipal.java
@@ -106,7 +106,7 @@
  *
  * If the input name does not contain a realm, the default realm
  * is used. The default realm can be specified either in a Kerberos
- * configuration file or via the {@systemproperty java.security.krb5.realm}
+ * configuration file or via the {@systemProperty java.security.krb5.realm}
  * system property. For more information, see the
  * {@extLink security_guide_jgss_tutorial Kerberos Requirements}.
  *
@@ -155,7 +155,7 @@
  *
  * If the input name does not contain a realm, the default realm
  * is used. The default realm can be specified either in a Kerberos
- * configuration file or via the {@systemproperty java.security.krb5.realm}
+ * configuration file or via the {@systemProperty java.security.krb5.realm}
  * system property. For more information, see the
  * {@extLink security_guide_jgss_tutorial Kerberos Requirements}.
  *

Thanks,
Max

Re: RFR[15] JDK-8244683: A TSA server used by tests

[sha . jiang]

Hi Max
Please review this webrev:
http://cr.openjdk.java.net/~jjiang/8244683/webrev.05/

- TimestampCheck.java
The codes in those private methods in Interceptor are merged into
getRespParam().

- TsaParam.java
Group the fields as signing internals and TSA fields.

- TsaSigner.java
parseRequestParam() and createResponse() are private now.

Best regards,
John Jiang

On 2020/6/4 22:18, Weijun Wang wrote:

OK. Please go on. No other comment.


On Jun 4, 2020, at 12:02 AM, sha.ji...@oracle.com wrote:


Can we make them internal at the moment until someone really need to extend it?

Yes, we can.
But as a test lib, is it bad to think about a bit further?

But think about it carefully when there are new requirements. It's probably not 
a good idea to make every method overridable at the beginning and say do 
whatever you want.

Thanks,
Max

Re: [15] RFR JDK-8246077: Cloneable test in HmacCore seems questionable

[Weijun Wang]

> 在 2020年6月5日，03:19，Valerie Peng  写道：
> 
>> Can you give an example when these 2 rules have different results? Is this 
>> only true for those implementation that directly subclass MessageDigest?
> 
> Before this fix, even the Spi impl implements Cloneable the instanceof check 
> will always fail because the wrapper class, i.e. MessageDigest.Delegate does 
> not. However, if you call the clone() (made public by the MessageDigest 
> class), it will succeed because Delegate.clone() checks to see if the spi 
> object implements the Cloneable interface, if yes, it will proceed to call 
> the spi clone(). So, for this scenario, the results are different, e.g. 
> instanceof returns false, but clone() succeeds. This is just one example. Is 
> this what you are asking?

No. 

I understand this case, but this has already been fixed. Is there any other 
example? Or are you only follow the words in the spec? i.e. try clone() to see 
if it’s cloneable. 

I am worried that try clone() is much heavier than just check instanof 
Cloneable. 

Thanks,
Max

Re: [15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering

[Valerie Peng]

Hi, Sean,

Thanks for the review and feedback. Webrev updated: 
http://cr.openjdk.java.net/~valeriep/8246613/webrev.01/


getTypeAndAlgorithm(...) was not static due to an instance variable used 
by debugging output. I have removed it and made both method static.


I will wait for others' review comments also.

Thanks,
Valerie
On 6/4/2020 2:01 PM, Sean Mullan wrote:

On 6/4/20 3:34 PM, Valerie Peng wrote:

Hi,

Could someone help reviewing this fix? This change keep tracks of the 
first registered SecureRandom algorithm and returns it upon the 
request of SecureRandom class.


This looks good to me. I would recommend that Max or someone else 
review it as well.



Bug: https://bugs.openjdk.java.net/browse/JDK-8246613

Webrev: http://cr.openjdk.java.net/~valeriep/8246613/webrev.00/


A couple of minor comments, feel free to fix or ignore.

* SecureRandom.java

879 // For SUN provider, we use 
SunEntries.DEFF_SECURE_RANDOM_ALGO


Might as well fix the typo while you are in there: s/DEFF/DEF/

* Provider.java

1156 private String parseSecureRandomPut(String name, String value) {

Could be static if you also make getTypeAndAlgorithm static, I think.

--Sean

Re: [15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering

[Sean Mullan]

On 6/4/20 3:34 PM, Valerie Peng wrote:

Hi,

Could someone help reviewing this fix? This change keep tracks of the 
first registered SecureRandom algorithm and returns it upon the request 
of SecureRandom class.


This looks good to me. I would recommend that Max or someone else 
review it as well.



Bug: https://bugs.openjdk.java.net/browse/JDK-8246613

Webrev: http://cr.openjdk.java.net/~valeriep/8246613/webrev.00/


A couple of minor comments, feel free to fix or ignore.

* SecureRandom.java

879 // For SUN provider, we use 
SunEntries.DEFF_SECURE_RANDOM_ALGO


Might as well fix the typo while you are in there: s/DEFF/DEF/

* Provider.java

1156 private String parseSecureRandomPut(String name, String value) {

Could be static if you also make getTypeAndAlgorithm static, I think.

--Sean

Re: RFR 8244565: Accept PKCS #8 with version number 1

[Valerie Peng]

Sure, I have no more comments.

Thanks,
Valerie
On 6/3/2020 7:48 PM, Weijun Wang wrote:

RSAPrivateKeyImpl and RSAPrivateCrtKeyImpl

- throws InvalidKeyException when RSAUtil.createAlgorithmId(type, keyParams) 
fails. I'll keep it.

EdDSAPrivateKeyImpl, XDHPrivateKeyImpl and ECPrivateKeyImpl

- check the input ECParameterSpec params and might throw an 
InvalidKeyException. I'll keep it.

Some of these constructors now throws ProviderException or InvalidKeyException 
when DER encoding goes wrong. This should never happen, and I'll throw 
AssertionError for it.

In short, the only possible exception is now InvalidKeyException. I also throw 
AssertionError but it's not possible and definitely means a programming error 
somewhere (Ex: DerValue::putInteger goes wrong).

So here is the updated webrev

http://cr.openjdk.java.net/~weijun/8244565/webrev.04/

Thanks,
Max




On Jun 4, 2020, at 7:37 AM, Valerie Peng  wrote:

Hi, Max,

Overall looks very good. Just one more thing:

Different key classes seems to differ in their handling of IOException in their 
constructor which produces the DER bytes.

DSAPrivateKey changed to use AssertionError, XDHPrivateKeyImpl and 
EdDSAPrivateKeyImpl throws ProviderException. It seems that all other 
PrivateKey classes use InvalidKeyException. Perhaps it'd be nice to use 
InvalidKeyException for consistency sake?

Thanks,
Valerie
On 6/1/2020 12:35 AM, Weijun Wang wrote:

Updated webrev at

https://cr.openjdk.java.net/~weijun/8244565/webrev.03.

I've inlined more methods that is only called once and inside the same method.

Thanks,
Max



On May 28, 2020, at 9:16 AM, Weijun Wang  wrote:




On May 28, 2020, at 8:46 AM, Valerie Peng  wrote:

Hi Max,

I like this new structure better. Much easier to understand. Most of the 
changes are technical debt that's accumulated inside PKCS8Key class.

A few notable differences which I am not so sure about are

- the encodedKey is returned by getEncoded() directly w/o cloning, and

Too bad. I'll fix.


- the protected parseKeyBits() method being made private. I wonder if the 
parseKeyBits() method should be made abstract so it's more obvious that the 
subclasses needs to parse the key bytes into algorithm-specific components.

Or how about I just inline parseKeyBits in those child classes into their 
constructors? I don't think the child class will forget it. Otherwise, why 
write a child class?

Thanks,
Max


Thanks,
Valerie
On 5/26/2020 5:54 PM, Weijun Wang wrote:

Can you please take a look (not a review) at an updated webrev at 
http://cr.openjdk.java.net/~weijun/8244565/webrev.02/? It contains the code to inspect 
the extra fields. I haven't deal with the "..." here yet.

However, when I tried to consolidate the DER parsing into one place, I've made 
more and more changes everywhere. The major change now is a base constructor 
PKCS8Key(byte[]) and subclass constructors that call it and no more protected 
parseKeyBits(). Although I don't think there is any technical error here but at 
the end of the day I'm asking myself if this is too much code change for such a 
simple bug.

Do you like the overall structure? If yes, I'll continue this way. Otherwise, I 
can restrict the change only inside PKCS8Key.

Thanks,
Max



On May 27, 2020, at 7:14 AM, Valerie Peng  wrote:

I suppose we can allow trailing data to conform to "..." then.

But the "[[2: publicKey [1] PublicKey OPTIONAL ]]," line mean that the 
publicKey should not be present for V1? This should be checked?

Valerie

On 5/25/2020 9:02 PM, Weijun Wang wrote:

The new definition at https://tools.ietf.org/html/rfc5958 shows,

 OneAsymmetricKey ::= SEQUENCE {
   version   Version,
   privateKeyAlgorithm   PrivateKeyAlgorithmIdentifier,
   privateKeyPrivateKey,
   attributes[0] Attributes OPTIONAL,
   ...,
   [[2: publicKey[1] PublicKey OPTIONAL ]],
   ...
 }

According to 
https://www.ibm.com/support/pages/what-does-string-three-elipses-mean-asn1:

The string "..." in ASN.1 is called an extension marker. The extension 
marker,
ellipsis, is an indication that extension additions are expected. It makes 
no
statement as to how such additions should be handled. However they shall 
not be
treated as an error during the decoding process.

So there might be more fields in the future. Do you still insist we need more 
validation?

Thanks,
Max



On May 20, 2020, at 1:37 PM, Valerie Peng  wrote:


True, the current handling of the extra bytes in parseKey() and decode() are 
kind of opposite, one does not allow any extra bytes but the other ignores all. 
The trailing bytes may look harmless but simply ignores it may open up 
something unexpected.

Given that this is key related class, I think it's important to do reasonable 
validation even though we currently do not use these trailing bytes. It'd also 
be good if the handling can be consistent regardless of the call path.

Thanks

[15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering

[Valerie Peng]

Hi,

Could someone help reviewing this fix? This change keep tracks of the 
first registered SecureRandom algorithm and returns it upon the request 
of SecureRandom class.


Bug: https://bugs.openjdk.java.net/browse/JDK-8246613

Webrev: http://cr.openjdk.java.net/~valeriep/8246613/webrev.00/

Thanks,

Valerie

Re: [15] RFR JDK-8246077: Cloneable test in HmacCore seems questionable

[Valerie Peng]

Hi Max,

Please find replies in line.

On 6/4/2020 3:54 AM, Weijun Wang wrote:

HmacCore.java:

   78 md = null;
   79 String noCloneProv = md.getProvider().getName();

NPE on line 79. Should reverse.

Good catch, fixed.

On Jun 4, 2020, at 8:09 AM, Valerie Peng  wrote:

Hi,

Anyone can help review this fix? I changed com.sun.crypto.provider.HmacCore 
class and sun.security.ssl.HandshakeHash class to check for cloneability based 
on the clone() call instead of the Cloneable interface.

Can you give an example when these 2 rules have different results? Is this only 
true for those implementation that directly subclass MessageDigest?


Before this fix, even the Spi impl implements Cloneable the instanceof 
check will always fail because the wrapper class, i.e. 
MessageDigest.Delegate does not. However, if you call the clone() (made 
public by the MessageDigest class), it will succeed because 
Delegate.clone() checks to see if the spi object implements the 
Cloneable interface, if yes, it will proceed to call the spi clone(). 
So, for this scenario, the results are different, e.g. instanceof 
returns false, but clone() succeeds. This is just one example. Is this 
what you are asking?



The test is not complete. There should be non-cloneable hash. Or even better, a 
hash which is not cloneable from the preferred provider but cloneable from 
another. Hopefully you can simply reuse an existing implementation with a 
different algorithm name and override its clone() to throw CNSE.


Right, I will expand the tests. Just want to gauge on people's 
preference of matching the Cloneable interface (the last part of changes 
in my earlier email) first, while I explore the existing tests.


Thanks,

Valerie



Thanks,
Max



Noticed a bug in sun.security.provider.DigestBase class which misses to reset 
the temporary buffer when cloning and fixed it here as well.

Lastly, I also made changes to java.security.MessageDigest and 
java.security.Signature classes and attempt to match the Delegate wrapper with 
the underlying spi object, i.e. if the spi implements Cloneable and then 
Delegate wrapper also implements Cloneable. This part is mostly for non-JDK 
callers which rely on the instanceof Cloneable check for cloneability. However, 
for Signature class, if the object is requested using 
Signature.getInstance(String), then we can't do matching here since the 
underlying spi is not yet determined at this time. The 3  TestCloneable.java 
tests are for testing this last part and is NOT for the HmacCore and 
HandshakeHash changes. I am on the fence about this part and am open to leave 
this out if minimum fix is preferred.

Bug: https://bugs.openjdk.java.net/browse/JDK-8246077
Webrev: http://cr.openjdk.java.net/~valeriep/8246077/webrev.00/

Thanks,

Valerie

Re: RFR: 8245527: LDAP Cnannel Binding support for Java GSS/Kerberos

[Alexey Bakhtin]

Hello,

Could you please review new version of the patch:
http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v3/

I’ve moved all logic for creating TLS Channel Binding data out of  
GssKrb5Client.java file.
All data is prepared inside TlsChannelBinding class.
Internal property name is renamed to “com.sun.security.sasl.tlschannelbinding”.
TlsChannelBinding object is still used to pass channel binding data from 
LdapClient to GssKrb5Client.
I do not change it to byte[] because of TlsChannelBinding class is still used 
for internal property name.
Also, I’ve updated TlsChannelBinding class to select SHA-256 hash algorithm if 
it can not be derived
from the certificate signature name.

Regards
Alexey


> On 26 May 2020, at 17:50, Weijun Wang  wrote:
> 
> I have a question on GssKrb5Client.java:
> 
> Do you think it's a good idea to let the SASL mechanism understand what TLS 
> binding is? Instead of passing the whole TlsChannelBinding object through a 
> SASL property, is it possible to only pass the actual cbData? After all, the 
> name "com.sun.security.sasl.channelbinding" suggests it's just a general 
> ChannelBinding which is independent with any application level info.
> 
> From my reading of the code change, it looks like this cbData can be 
> calculated on the LDAP side.
> 
> Thanks,
> Max
> 
>> On May 21, 2020, at 3:35 PM, Alexey Bakhtin  wrote:
>> 
>> Hello,
>> 
>> Could you please review the following patch:
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8245527
>> Webrev: http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v0/
>> 
>> The Windows LDAP server with LdapEnforceChannelBinding=2 uses the 
>> tls-server-end-point channel binding
>> (based on the TLS server certificate). The channel binding data is 
>> calculated as following :
>>  • The client calculates a hash of the TLS server certificate.
>>  The hash algorithm is selected on the base of the certificate 
>> signature algorithm.
>>  Also, the client should use SHA-256 algorithm, in case of the 
>> certificate signature algorithm is SHA1 or MD5 based
>>  • The channel binding information is the same as defined in rfc4121 [1] 
>> with small corrections:
>>  • initiator and acceptor addresses should be set to NULL
>>  • initiator and acceptor address types should be zero.
>> It contradicts to the “Using Channel Bindings in GSS-API” 
>> document [2] that say that
>> the address type should be set to GSS_C_AF_NULLADDR=0xFF,
>> instead of GSS_C_AF_UNSPEC=0x00.
>> 
>> This patch introduces changes in the LDAP, SASL and JGSS modules
>> to generate channel binding data automatically if requested by an 
>> application.
>> This patch reuses existing org.ietf.jgss.ChannelBinding class implementation 
>> but changes
>> initial unspecified address type from CHANNEL_BINDING_AF_NULL_ADDR to 
>> CHANNEL_BINDING_AF_UNSPEC.
>> The patch introduces new environment property “com.sun.jndi.ldap.tls.cbtype” 
>> that indicates
>> Channel Binding type that should be used in the LDAPS connection over 
>> JGSS/Kerberos.
>> Right now "tls-server-end-point" Channel Binding type is supported only.
>> The client extracts server certificate from the underlying TLS connection 
>> and creates
>> Channel Binding data for JGSS/Kerberos authentication if application 
>> indicates
>> com.sun.jndi.ldap.tls.cbtype=tls-server-end-point property.
>> Client application should also specify existing 
>> "com.sun.jndi.ldap.connect.timeout” property
>> to force and wait TLS handshake completion before JGSS/Kerberos 
>> authentication data is generated.
>> 
>> [1] - https://tools.ietf.org/html/rfc4121#section-4.1.1.2
>> 
>> [2] -
>> https://docs.oracle.com/cd/E19120-01/open.solaris/819-2145/overview-52/index.html
>> 
>> Initial discussion of this issue is available at security-dev list:
>> https://mail.openjdk.java.net/pipermail/security-dev/2019-December/021052.html
>> https://mail.openjdk.java.net/pipermail/security-dev/2020-January/021140.html
>> https://mail.openjdk.java.net/pipermail/security-dev/2020-February/021278.html
>> https://mail.openjdk.java.net/pipermail/security-dev/2020-May/021864.html



signature.asc
Description: Message signed with OpenPGP

RFR:8246330:Add TLS Tests for Legacy ECDSA curves

[shivangi . g . gupta]

Hi,

May I please find a sponsor for this patch?

Bug: https://bugs.openjdk.java.net/browse/JDK-8246330

Description:Many of EC named curves has been disabled with security 
property "jdk.disabled.namedCurves". The purpose of this Test is to 
verify the behavior of any EC named curve from the disabled list.

This Test will address a single name from the disabled list as sect283r1.


Webrev: http://cr.openjdk.java.net/~sshivang/reviews/ 
8246330/webrev.00/ 



The patch has been tested on mach5, and the individual test was passed.

Thanks

Shivangi

[15] RFR JDK-8246031: Hang observed with coherence SSLNIOServer test

[Prasadrao Koppula]

Hi,

 

Could you please review this patch. For timeout/interrupts, JDK11u+ releases, 
SSLSocket:getSession behavior is different, compare to JDK8u. i.e, connection 
is in open state for timeout/interrupts exception. For comparability reasons, 
this fix will close connection for getSession timeout/ interrupts. 

 

Bug: https://bugs.openjdk.java.net/browse/JDK-8246031

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

 

Thanks,

Prasad.K

Re: RFR[15] JDK-8244683: A TSA server used by tests

[Weijun Wang]

OK. Please go on. No other comment.

> On Jun 4, 2020, at 12:02 AM, sha.ji...@oracle.com wrote:
> 
>> Can we make them internal at the moment until someone really need to extend 
>> it?
> Yes, we can.
> But as a test lib, is it bad to think about a bit further?

But think about it carefully when there are new requirements. It's probably not 
a good idea to make every method overridable at the beginning and say do 
whatever you want.

Thanks,
Max

Re: RFR[15] JDK-8243114: Implement montgomery{Multiply, Square}intrinsics on Windows

[Andrew Haley]

On 04/06/2020 14:35, Simon Tooke wrote:
> Yes, this hurt to type.  A previous review suggested using julong, is 
> that acceptable to you?
> 
> (an aside: I'm now wondering if there is other code in the JDK that 
> assumes long is 64bits - which is not true on all platforms...)

That was just me, I think. I knew that this code would take some
fixing up on Windows so I postponed portability issues until then.

-- 
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. 
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671

Re: RFR[15] JDK-8243114: Implement montgomery{Multiply, Square}intrinsics on Windows

[Simon Tooke]

Hello, David, and thanks for the review!

I've responded to your comments below, and intend to post a new patch 
for review today or tomorrow.


Thanks again,

-Simon

On 2020-06-03 2:06 a.m., David Holmes wrote:

Hi Simon,

On 23/05/2020 12:04 am, Sean Mullan wrote:
Cross-posting to hotspot-dev for additional review since the code 
changes are in hotspot.


--Sean

On 5/21/20 1:24 PM, Simon Tooke wrote:

Hello,

I'd like to request a review for:

JBS: https://bugs.openjdk.java.net/browse/JDK-8243114

Webrev: 
http://cr.openjdk.java.net/~stooke/webrevs/jdk-8243114-jdk/00/00/


This change implements the given intrinsics on Windows.

The Windows toolchain has no 128 bit integer types, and no inline 
asm (on x64 and Arm).  In addition, 'long' is 4 bytes, not 8, as it 
is with gcc.  This patch had to change some of the linux 
implementation to account for these limitations.


I can't really comment on the intrinsics themselves but a couple of 
suggestions:


- use explicitly sized types e.g. uint64_t instead of unsigned long long


Yes, this hurt to type.  A previous review suggested using julong, is 
that acceptable to you?


(an aside: I'm now wondering if there is other code in the JDK that 
assumes long is 64bits - which is not true on all platforms...)



- use the existing NOINLINE macro for the _declspec(noinline)

Thanks, will do.


The conditional compilation in this code has me quite confused. 
Looking at the existing code we have:


3680 #ifndef _WINDOWS
3681
3682 #define ASM_SUBTRACT
3683
3684 #ifdef ASM_SUBTRACT
...
3702 #else // ASM_SUBTRACT
...
3719 #endif // ! ASM_SUBTRACT

So all the above code is only built on not-Windows, and we 
unconditionally define ASM_SUBTRACT, so lines 3702 to 3719 appear to 
be dead code! I'm not at all sure whether that is actually a bug and 
the windows ifndef should have had an endif after line 3682; or 
whether we can just simplify the code.
The dead code existed prior to this patch, so I wasn't proposing 
removing it.  I'm happy to do so if that's for the best.  As far as I 
can tell, it's for implementing these intrinsics on gcc platforms 
without assembler.


Thanks,
David



My gratitude for Andrew Haley for doing the heavy lifting at the 
core of this patch.


The patch, if accepted, will be offered to 11u as a potential 
backport. The changes apply cleanly modulo some line number changes.


As for the speedup, this test case:

BigInteger base = BigInteger.ONE.shiftLeft(1024);
long count = LongStream.rangeClosed(2, 100_000)
 .mapToObj(n -> BigInteger.valueOf(n).add(base))
 .filter(i -> i.isProbablePrime(50))
 .count();

goes from 1 minute 20 seconds down to about 35 seconds om my VM, 
over multiple runs.  As is the case on other platforms where the 
intrinsics are supported, they will be enabled by default on Windows.


Thank you for your time,

-Simon Tooke


Principal Software Engineer,

Red Hat Canada

Re: RFR 8244148: keytool -printcert and -printcrl should support the -trustcacerts and -keystore options

[Weijun Wang]

> On Jun 4, 2020, at 7:29 PM, Hai-May Chao  wrote:
> 
> Hi Max,
> 
>> On Jun 3, 2020, at 12:59 AM, Weijun Wang  wrote:
>> 
>> The source change looks fine to me.
>> 
>> In TrustedCert.java:
>> 
>> - You can use FileOutputStream and Files.copy(Path,OutputStream) in cat().
> 
> This cat() is taken from WealAlg.java.
> 
>> 
>> - There is no need to recreate root.jks and root.pem.
> 
> The sequences of the commands used in this test scenario allows me to test 
> -printcert for the -trustcacerts and -keytsore options. We had discussion 
> offline about it. The test uses trusted certificates and checks no warnings 
> on the weak algorithms to address the requirement described in the bug. I 
> believe it does serve that purpose, and looks legitimate to me. There could 
> be different ways of testing a functionality, and please let me know if there 
> is a problem with the current approach.

I just meant that the keytool commands generating root.jks and root.pem are 
exactly the same and there is no need to recreate it.

> 
> Please also elaborate your comment about no need to recreate root.jks and 
> root.pem.
> 
>> 
>> - Why not use -trustcacerts below?
>> 
>> 160 kt("-importcert -file server.pem -noprompt", "server.jks”);
> 
> 
> Because here is to import the server (end-entity) cert, and it will not make 
> a difference for the test result whether to use the -trustcacerts or not. 
> It’s the ca (intermediate) cert needs to have it in this test scenario. I 
> intended to leave it out in #160 to distinguish between server and ca certs.

OK.

Then how about we add a new command before line 155?

kt("-importcert -file ca.pem", "ca.jks").shouldNotHaveExitValue(0);

This would prove the "-trustcacerts" on line 155 is really useful.

> 
>> 
>> - It's probably better to add a " " between cmd and options in patchcmd(). 
>> Same in TrustedCRL.java.
> 
> Ok, will change it.
> 
>> 
>> In TrustedCRL.java:
>> 
>> - No need to recreate ks and ca.crl. Just call "-printcrl" with different 
>> options.
> 
> Same reply as above.

Same question as above.

> 
>> 
>> - Why create using MD5withRSA? Do you meant to warn about the weak algorithm?
> 
> Yes, exactly, and it differentiates from the weak algorithm SHA1withRSA used 
> in root CA where no warning will be emitted. There is another -gencrl in #119 
> without using MD5withRSA so I’d have two test cases.
> 
>> 
>> Also I would suggest you create a dedicate method (maybe in 
>> SecurityTools.java) to create your own cacerts. There is no need to copy 
>> over the system cacerts, just make sure the file is created with the JKS 
>> storetype. We are thinking of upgrading the storetype of cacerts and it's 
>> nice to do this at a single place so we can modify it easily later.
> 
> I created a method in SecurityTools.java to create the own cacerts. With this 
> keystore, the subsequent importing a certificate reply would not work. It 
> turns out that its caks.size() is zero detected at establishCertChain() in 
> keytool/Main.java after root cert has been imported to that cacerts. At this 
> point I’d like to suggest a separate bug be filed to cover the cacerts 
> enhancement that you suggested.

I meant creating the cacerts in one method, something like

   void createCacerts(String ks, String... crt);

and you can call createCacerts("mycacerts", "root.crt") to create it. The 
method can call KeyStore APIs and not keytool commands.

BTW, what does caks.size() == 0 matter here?

Thanks,
Max


> 
> Thanks,
> Hai-May
> 
> 
>> Thanks,
>> Max
>> 
>> 
>>> On Jun 2, 2020, at 2:37 AM, Hai-May Chao  wrote:
>>> 
>>> Hi,
>>> 
>>> I’d like to request a review for:
>>> 
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8244148
>>> CSR: https://bugs.openjdk.java.net/browse/JDK-8246269
>>> Webrev: http://cr.openjdk.java.net/~hchao/8244148/webrev.00/
>>> 
>>> The change is to add the support of -trustcacerts and -keystore options to 
>>> -printcert and -princrl command for keytool. This enables keytool to use 
>>> the trusted certificates when verifying untrusted artifacts that are signed 
>>> by CAs. It also incorporates Max’s change that consolidates the code to get 
>>> the default location of cacerts keystore.
>>> 
>>> Thanks,
>>> Hai-May
>>> 
>> 
> 

Re: RFR 8244148: keytool -printcert and -printcrl should support the -trustcacerts and -keystore options

[Hai-May Chao]

Hi Max,

> On Jun 3, 2020, at 12:59 AM, Weijun Wang  wrote:
> 
> The source change looks fine to me.
> 
> In TrustedCert.java:
> 
> - You can use FileOutputStream and Files.copy(Path,OutputStream) in cat().

This cat() is taken from WealAlg.java.

> 
> - There is no need to recreate root.jks and root.pem.

The sequences of the commands used in this test scenario allows me to test 
-printcert for the -trustcacerts and -keytsore options. We had discussion 
offline about it. The test uses trusted certificates and checks no warnings on 
the weak algorithms to address the requirement described in the bug. I believe 
it does serve that purpose, and looks legitimate to me. There could be 
different ways of testing a functionality, and please let me know if there is a 
problem with the current approach.

Please also elaborate your comment about no need to recreate root.jks and 
root.pem.

> 
> - Why not use -trustcacerts below?
> 
> 160 kt("-importcert -file server.pem -noprompt", "server.jks”);


Because here is to import the server (end-entity) cert, and it will not make a 
difference for the test result whether to use the -trustcacerts or not. It’s 
the ca (intermediate) cert needs to have it in this test scenario. I intended 
to leave it out in #160 to distinguish between server and ca certs.

> 
> - It's probably better to add a " " between cmd and options in patchcmd(). 
> Same in TrustedCRL.java.

Ok, will change it.

> 
> In TrustedCRL.java:
> 
> - No need to recreate ks and ca.crl. Just call "-printcrl" with different 
> options.

Same reply as above.

> 
> - Why create using MD5withRSA? Do you meant to warn about the weak algorithm?

Yes, exactly, and it differentiates from the weak algorithm SHA1withRSA used in 
root CA where no warning will be emitted. There is another -gencrl in #119 
without using MD5withRSA so I’d have two test cases.

> 
> Also I would suggest you create a dedicate method (maybe in 
> SecurityTools.java) to create your own cacerts. There is no need to copy over 
> the system cacerts, just make sure the file is created with the JKS 
> storetype. We are thinking of upgrading the storetype of cacerts and it's 
> nice to do this at a single place so we can modify it easily later.

I created a method in SecurityTools.java to create the own cacerts. With this 
keystore, the subsequent importing a certificate reply would not work. It turns 
out that its caks.size() is zero detected at establishCertChain() in 
keytool/Main.java after root cert has been imported to that cacerts. At this 
point I’d like to suggest a separate bug be filed to cover the cacerts 
enhancement that you suggested.

Thanks,
Hai-May


> Thanks,
> Max
> 
> 
>> On Jun 2, 2020, at 2:37 AM, Hai-May Chao  wrote:
>> 
>> Hi,
>> 
>> I’d like to request a review for:
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8244148
>> CSR: https://bugs.openjdk.java.net/browse/JDK-8246269
>> Webrev: http://cr.openjdk.java.net/~hchao/8244148/webrev.00/
>> 
>> The change is to add the support of -trustcacerts and -keystore options to 
>> -printcert and -princrl command for keytool. This enables keytool to use the 
>> trusted certificates when verifying untrusted artifacts that are signed by 
>> CAs. It also incorporates Max’s change that consolidates the code to get the 
>> default location of cacerts keystore.
>> 
>> Thanks,
>> Hai-May
>> 
> 

Re: [15] RFR JDK-8246077: Cloneable test in HmacCore seems questionable

[Weijun Wang]

HmacCore.java:

  78 md = null;
  79 String noCloneProv = md.getProvider().getName();

NPE on line 79. Should reverse.


> On Jun 4, 2020, at 8:09 AM, Valerie Peng  wrote:
> 
> Hi,
> 
> Anyone can help review this fix? I changed com.sun.crypto.provider.HmacCore 
> class and sun.security.ssl.HandshakeHash class to check for cloneability 
> based on the clone() call instead of the Cloneable interface.

Can you give an example when these 2 rules have different results? Is this only 
true for those implementation that directly subclass MessageDigest?

The test is not complete. There should be non-cloneable hash. Or even better, a 
hash which is not cloneable from the preferred provider but cloneable from 
another. Hopefully you can simply reuse an existing implementation with a 
different algorithm name and override its clone() to throw CNSE.

Thanks,
Max


> Noticed a bug in sun.security.provider.DigestBase class which misses to reset 
> the temporary buffer when cloning and fixed it here as well.
> 
> Lastly, I also made changes to java.security.MessageDigest and 
> java.security.Signature classes and attempt to match the Delegate wrapper 
> with the underlying spi object, i.e. if the spi implements Cloneable and then 
> Delegate wrapper also implements Cloneable. This part is mostly for non-JDK 
> callers which rely on the instanceof Cloneable check for cloneability. 
> However, for Signature class, if the object is requested using 
> Signature.getInstance(String), then we can't do matching here since the 
> underlying spi is not yet determined at this time. The 3  TestCloneable.java 
> tests are for testing this last part and is NOT for the HmacCore and 
> HandshakeHash changes. I am on the fence about this part and am open to leave 
> this out if minimum fix is preferred.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8246077
> Webrev: http://cr.openjdk.java.net/~valeriep/8246077/webrev.00/
> 
> Thanks,
> 
> Valerie
> 

Re: [15] RFR JDK-8087327: CipherStream produces new byte array on every update or doFinal operation

[Weijun Wang]

Looks fine to me.

Thanks,
Max

> On Jun 4, 2020, at 10:40 AM, Valerie Peng  wrote:
> 
> Hi,
> 
> Anyone can help review this straightforward enhancement? The changes are 
> mostly based on the submitted patch with some minor polishing. Essentially 
> re-using existing buffer instead of relying on the Cipher object to allocate 
> new buffers.
> 
> RFE: https://bugs.openjdk.java.net/browse/JDK-8087327
> 
> Webrev: http://cr.openjdk.java.net/~valeriep/8087327/webrev.00/
> 
> Thanks,
> 
> Valerie
>