Re: RFR 6722928: Support SSPI as a native GSS-API provider

2019-06-12 Thread Weijun Wang
I've filed https://bugs.openjdk.java.net/browse/JDK-8225687.

> On Jun 11, 2019, at 7:56 AM, Nico Williams  wrote:
> 
> On Mon, Jun 10, 2019 at 10:30:50AM +0800, Weijun Wang wrote:
>> Updated webrev at
>> 
>>   http://cr.openjdk.java.net/~weijun/6722928/webrev.08/
> 
> - 
> src/java.security.jgss/share/classes/sun/security/jgss/spnego/NegTokenTarg.java
> 
>   Ugh, I never noticed GSSUtil.useMSInterop().  This really should not
>   be needed.  I believe a proper implementation of RFC4178 wouldn't
>   need it.

I tried but scared away by MS-SPNG's appendix on which version of Windows 
supports which style of mechListMic. Now that Java only supports one underlying 
mechanism this field is looks not so crucial.

> 
>   I can't expect you to do that, but can we have a bug opened for this?

There was one at https://bugs.openjdk.java.net/browse/JDK-6773898.

> 
> - 
> src/java.security.jgss/share/classes/sun/security/jgss/wrapper/SunNativeProvider.java
> 
>   Shouldn't the sspi_bridge.dll be in the lib directory?

The DLLs have always been in /bin. Maybe it's more easily loadable by java.exe.

> 
> - src/java.security.jgss/share/native/libj2gss/NativeFunc.h
> - src/java.security.jgss/share/native/libj2gss/gssapi.h
> 
>   These continue to have improper constification.
> 
>   There should be no function arguments of type "const gss_...".
> 
>   I've a PR for Heimdal to fix this in Heimdal.  It was very
>   mechanical: search the prototypes and function definitions for
> 
>   \ 
>   then change that to gss_const_... then fix all the warnings and
>   errors that came up when building the result.

OK.

> 
> - All remaining commentary will be about
>   src/java.security.jgss/windows/native/libsspi_bridge/sspi.cpp
> 
>  - CHECK_*() macros
> 
>Macro bodies should not end in a semi-colon.

OK.

> 
>If these were public and since these macro bodies are all if
>statements, you should wrap them in do { } while (0), but since
>they're private we can make sure that all uses are correct.
> 
>  - gss_import_name() doesn't check that the first two bytes of the
>input buffer are the expected token ID when the name-type is
>GSS_C_NT_EXPORTED_NAME.

OK.

> 
>  - gss_import_name() doesn't check that the third byte of the input
>buffer is 0 when the name-type is GSS_C_NT_EXPORTED_NAME.

OK.

> 
>  - gss_compare_name(), this code will not distinguish a name of the
>form 'foo@' from 'foo\@'
> 
>   434 if (l1 < l2 && n2[l1] != L'@'
>   435 || l2 < l1 && n1[l2] != L'@') {
>   436 return GSS_S_COMPLETE; // different
>   437 }
> 
>Honestly, this is not the most serious problem because nothing
>really should be using gss_compare_name(), but you do use it... and
>anyways, it's wrong.

Yes, it's called in GSSLibStub.c and those are the functions I must support.

> 
>Perhaps the gss_name_struct struct should have a length of realm or
>length-of-not-realm-part field to make this check easier.
> 
>  - gss_compare_name(), do not use NORM_IGNORECASE

But I do notice that "Administrator" can also be named "administrator", and 
"HTTP/host" can also be "http/host".

> 
>  - gss_canonicalize_name() should check that mech_type is Kerberos

OK.

> 
> I'll continue later.  I'm in gss_init_sec_context(), about 58% of the
> way through.

Please.

Thanks,
Max

> 
> Nico
> -- 



Re: RFR 8225180: SignedObject with invalid Key not throwing the InvalidKeyException in Windows

2019-06-12 Thread Weijun Wang
Like this?

   http://cr.openjdk.java.net/~weijun/8225180/webrev.01/

Thanks,
Max

> On Jun 12, 2019, at 5:00 AM, Sean Mullan  wrote:
> 
> On 6/11/19 12:00 AM, Weijun Wang wrote:
>> Oops, forgot to add the bug title in the subject.
>>> On Jun 11, 2019, at 11:56 AM, Weijun Wang  wrote:
>>> 
>>> Please take a review at
>>> 
>>>   https://cr.openjdk.java.net/~weijun/8225180/webrev.00
>>> 
>>> I added some extra info into the exception message not realizing it will be 
>>> a bug. Revert back to previous code.
> 
> It is fine I guess, but you could workaround the null key if the additional 
> info in the exception message is useful.
> 
> --Sean



Re: RFR: JDK-8225392: Comparison builds are failing due to cacerts file

2019-06-12 Thread Weijun Wang
Hi Sean,

The previous fix for JDK-8193255 already made the creation date useless. Before 
that, each time cacerts was regenerated the date changed. I compared cacerts of 
different releases and the same cert have difference creation dates.

The only other solution I can think of is to look at 
make/autoconf/version-numbers and use the DEFAULT_VERSION_DATE=2019-09-17 there.

Have you reviewed the code changes? You can see I stored the hash of the whole 
file into the test. This means anyone updating the CA certs will have to create 
cacerts and calculate the correct hash and update this test. I suppose this 
won't be a hassle.

Thanks,
Max

> On Jun 13, 2019, at 4:15 AM, Sean Mullan  wrote:
> 
> On 6/12/19 4:01 PM, Erik Joelsson wrote:
>> Hello,
>> We cannot rely on querying mercurial at build time. The source must be 
>> buildable from a source distribution.
> 
> I had a feeling it wouldn't work but thought I would ask anyway.
> 
> Well, offhand I can't think of any better solution than notBefore then, 
> unless we included it as a comment in the PEM file, and then extracted it. 
> With notBefore, someone might be curious about why some keystore entries were 
> created so long ago (ex: the earliest notBefore date is 1996). But the 
> creationDate doesn't really have any usefulness for cacerts, so it's probably 
> ok.
> 
> --Sean
> 
>> /Erik
>> On 2019-06-12 11:39, Sean Mullan wrote:
>>> Using the certificate's notBefore date as the KeyStore entry creation date 
>>> is misleading since many of these root certs were not integrated into the 
>>> JDK until after they were created by the CA. Can we somehow extract the 
>>> last revision time of each PEM file instead? That is more aligned with the 
>>> previous creation date that we used.
>>> 
>>> --Sean
>>> 
>>> On 6/12/19 12:38 PM, Erik Joelsson wrote:
 Hello Max,
 
 Much appreciated! I will need to have this fixed one way or other in JDK 
 13, so depending on if you get your fix there in time, I will retract my 
 proposal. If your fix only hits 14, I will push mine to 13.
 
 /Erik
 
 On 2019-06-12 08:41, Weijun Wang wrote:
> This is my version of the fix:
> 
> http://cr.openjdk.java.net/~weijun/8225392/webrev.00/
> 
> Now you can still compare cacerts bit by bit.
> 
> Thanks,
> Max
> 
>> On Jun 12, 2019, at 10:50 PM, Weijun Wang  wrote:
>> 
>> Hi Erik,
>> 
>> Are you going to fix this bug soon?
>> 
>> I am inspired by Martin's words and would like to update 
>> GenerateCacerts.java so that as long as the certs and their aliases are 
>> unchanged, the output cacerts will always be the same. I can send out a 
>> code review today.
>> 
>> Thanks,
>> Max
>> 
>>> On Jun 12, 2019, at 10:59 AM, Weijun Wang  
>>> wrote:
>>> 
>>> Good idea about the creation time.
>>> 
>>> --Max
>>> 
 On Jun 12, 2019, at 10:53 AM, Martin Buchholz  
 wrote:
 
 Google culture really likes build output determinism, and we recently 
 built our own cacerts generator.
 
 To get determinism, we are using cert digest as alias (must have a 
 unique alias, but value doesn't seem to matter much), and using cert 
 notBefore instead of current (build) timestamp.
 
 On Mon, Jun 10, 2019 at 12:40 PM Erik Joelsson 
  wrote:
 Since JDK-8193255, when we started generating the cacerts file in the
 build, the build compare baseline builds have started failing. It seems
 the cacerts binary file has some non determinism built in so it doesn't
 get generated exactly the same given the same input. This patch adds
 special handling when comparing that file by comparing the output of
 "keytool -list" on the files instead.
 
 Bug: https://bugs.openjdk.java.net/browse/JDK-8225392
 
 Webrev: http://cr.openjdk.java.net/~erikj/8225392/webrev.01/
 
 /Erik
 



Re: RFR: JDK-8225392: Comparison builds are failing due to cacerts file

2019-06-12 Thread Weijun Wang



> On Jun 13, 2019, at 12:16 AM, Martin Buchholz  wrote:
> 
> I'm not a security engineer, but:
> - consider creating static finals for e.g. "Mighty Aphrodite" just to give it 
> a symbolic name.

That method is copied from JavaKeyStore.java. Keeping it 100% unchanged gives 
me more confidence I'm write the file correctly.

> - VerifyCACerts probably fails when the jdk is configured with a different 
> cacerts file (but the JDK doesn't preserve configuration information - how 
> could one fix it?)
> Many downstream organizations will configure a different cacerts.

I don't have a solution. Maybe in your repo you can @ignore this test.

Thanks,
Max

> 
> On Wed, Jun 12, 2019 at 8:42 AM Weijun Wang  wrote:
> This is my version of the fix:
> 
>http://cr.openjdk.java.net/~weijun/8225392/webrev.00/
> 
> Now you can still compare cacerts bit by bit.
> 
> Thanks,
> Max
> 
> > On Jun 12, 2019, at 10:50 PM, Weijun Wang  wrote:
> > 
> > Hi Erik,
> > 
> > Are you going to fix this bug soon?
> > 
> > I am inspired by Martin's words and would like to update 
> > GenerateCacerts.java so that as long as the certs and their aliases are 
> > unchanged, the output cacerts will always be the same. I can send out a 
> > code review today.
> > 
> > Thanks,
> > Max
> > 
> >> On Jun 12, 2019, at 10:59 AM, Weijun Wang  wrote:
> >> 
> >> Good idea about the creation time.
> >> 
> >> --Max
> >> 
> >>> On Jun 12, 2019, at 10:53 AM, Martin Buchholz  wrote:
> >>> 
> >>> Google culture really likes build output determinism, and we recently 
> >>> built our own cacerts generator.
> >>> 
> >>> To get determinism, we are using cert digest as alias (must have a unique 
> >>> alias, but value doesn't seem to matter much), and using cert notBefore 
> >>> instead of current (build) timestamp.
> >>> 
> >>> On Mon, Jun 10, 2019 at 12:40 PM Erik Joelsson  
> >>> wrote:
> >>> Since JDK-8193255, when we started generating the cacerts file in the 
> >>> build, the build compare baseline builds have started failing. It seems 
> >>> the cacerts binary file has some non determinism built in so it doesn't 
> >>> get generated exactly the same given the same input. This patch adds 
> >>> special handling when comparing that file by comparing the output of 
> >>> "keytool -list" on the files instead.
> >>> 
> >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8225392
> >>> 
> >>> Webrev: http://cr.openjdk.java.net/~erikj/8225392/webrev.01/
> >>> 
> >>> /Erik
> >>> 
> >> 
> > 
> 



Re: RFR: JDK-8225392: Comparison builds are failing due to cacerts file

2019-06-12 Thread Weijun Wang
I plan to fix it in JDK 13. --Max

> On Jun 13, 2019, at 12:38 AM, Erik Joelsson  wrote:
> 
> Hello Max,
> 
> Much appreciated! I will need to have this fixed one way or other in JDK 13, 
> so depending on if you get your fix there in time, I will retract my 
> proposal. If your fix only hits 14, I will push mine to 13.
> 
> /Erik
> 
> On 2019-06-12 08:41, Weijun Wang wrote:
>> This is my version of the fix:
>> 
>>http://cr.openjdk.java.net/~weijun/8225392/webrev.00/
>> 
>> Now you can still compare cacerts bit by bit.
>> 
>> Thanks,
>> Max



Re: RFR: JDK-8225392: Comparison builds are failing due to cacerts file

2019-06-12 Thread Sean Mullan

On 6/12/19 4:01 PM, Erik Joelsson wrote:

Hello,

We cannot rely on querying mercurial at build time. The source must be 
buildable from a source distribution.


I had a feeling it wouldn't work but thought I would ask anyway.

Well, offhand I can't think of any better solution than notBefore then, 
unless we included it as a comment in the PEM file, and then extracted 
it. With notBefore, someone might be curious about why some keystore 
entries were created so long ago (ex: the earliest notBefore date is 
1996). But the creationDate doesn't really have any usefulness for 
cacerts, so it's probably ok.


--Sean



/Erik

On 2019-06-12 11:39, Sean Mullan wrote:
Using the certificate's notBefore date as the KeyStore entry creation 
date is misleading since many of these root certs were not integrated 
into the JDK until after they were created by the CA. Can we somehow 
extract the last revision time of each PEM file instead? That is more 
aligned with the previous creation date that we used.


--Sean

On 6/12/19 12:38 PM, Erik Joelsson wrote:

Hello Max,

Much appreciated! I will need to have this fixed one way or other in 
JDK 13, so depending on if you get your fix there in time, I will 
retract my proposal. If your fix only hits 14, I will push mine to 13.


/Erik

On 2019-06-12 08:41, Weijun Wang wrote:

This is my version of the fix:

    http://cr.openjdk.java.net/~weijun/8225392/webrev.00/

Now you can still compare cacerts bit by bit.

Thanks,
Max

On Jun 12, 2019, at 10:50 PM, Weijun Wang  
wrote:


Hi Erik,

Are you going to fix this bug soon?

I am inspired by Martin's words and would like to update 
GenerateCacerts.java so that as long as the certs and their aliases 
are unchanged, the output cacerts will always be the same. I can 
send out a code review today.


Thanks,
Max

On Jun 12, 2019, at 10:59 AM, Weijun Wang  
wrote:


Good idea about the creation time.

--Max

On Jun 12, 2019, at 10:53 AM, Martin Buchholz 
 wrote:


Google culture really likes build output determinism, and we 
recently built our own cacerts generator.


To get determinism, we are using cert digest as alias (must have 
a unique alias, but value doesn't seem to matter much), and using 
cert notBefore instead of current (build) timestamp.


On Mon, Jun 10, 2019 at 12:40 PM Erik Joelsson 
 wrote:
Since JDK-8193255, when we started generating the cacerts file in 
the
build, the build compare baseline builds have started failing. It 
seems
the cacerts binary file has some non determinism built in so it 
doesn't

get generated exactly the same given the same input. This patch adds
special handling when comparing that file by comparing the output of
"keytool -list" on the files instead.

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

Webrev: http://cr.openjdk.java.net/~erikj/8225392/webrev.01/

/Erik



Re: RFR: JDK-8225392: Comparison builds are failing due to cacerts file

2019-06-12 Thread Erik Joelsson

Hello,

We cannot rely on querying mercurial at build time. The source must be 
buildable from a source distribution.


/Erik

On 2019-06-12 11:39, Sean Mullan wrote:
Using the certificate's notBefore date as the KeyStore entry creation 
date is misleading since many of these root certs were not integrated 
into the JDK until after they were created by the CA. Can we somehow 
extract the last revision time of each PEM file instead? That is more 
aligned with the previous creation date that we used.


--Sean

On 6/12/19 12:38 PM, Erik Joelsson wrote:

Hello Max,

Much appreciated! I will need to have this fixed one way or other in 
JDK 13, so depending on if you get your fix there in time, I will 
retract my proposal. If your fix only hits 14, I will push mine to 13.


/Erik

On 2019-06-12 08:41, Weijun Wang wrote:

This is my version of the fix:

    http://cr.openjdk.java.net/~weijun/8225392/webrev.00/

Now you can still compare cacerts bit by bit.

Thanks,
Max

On Jun 12, 2019, at 10:50 PM, Weijun Wang  
wrote:


Hi Erik,

Are you going to fix this bug soon?

I am inspired by Martin's words and would like to update 
GenerateCacerts.java so that as long as the certs and their aliases 
are unchanged, the output cacerts will always be the same. I can 
send out a code review today.


Thanks,
Max

On Jun 12, 2019, at 10:59 AM, Weijun Wang  
wrote:


Good idea about the creation time.

--Max

On Jun 12, 2019, at 10:53 AM, Martin Buchholz 
 wrote:


Google culture really likes build output determinism, and we 
recently built our own cacerts generator.


To get determinism, we are using cert digest as alias (must have 
a unique alias, but value doesn't seem to matter much), and using 
cert notBefore instead of current (build) timestamp.


On Mon, Jun 10, 2019 at 12:40 PM Erik Joelsson 
 wrote:
Since JDK-8193255, when we started generating the cacerts file in 
the
build, the build compare baseline builds have started failing. It 
seems
the cacerts binary file has some non determinism built in so it 
doesn't

get generated exactly the same given the same input. This patch adds
special handling when comparing that file by comparing the output of
"keytool -list" on the files instead.

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

Webrev: http://cr.openjdk.java.net/~erikj/8225392/webrev.01/

/Erik



Re: RFR: JDK-8225392: Comparison builds are failing due to cacerts file

2019-06-12 Thread Sean Mullan
Using the certificate's notBefore date as the KeyStore entry creation 
date is misleading since many of these root certs were not integrated 
into the JDK until after they were created by the CA. Can we somehow 
extract the last revision time of each PEM file instead? That is more 
aligned with the previous creation date that we used.


--Sean

On 6/12/19 12:38 PM, Erik Joelsson wrote:

Hello Max,

Much appreciated! I will need to have this fixed one way or other in JDK 
13, so depending on if you get your fix there in time, I will retract my 
proposal. If your fix only hits 14, I will push mine to 13.


/Erik

On 2019-06-12 08:41, Weijun Wang wrote:

This is my version of the fix:

    http://cr.openjdk.java.net/~weijun/8225392/webrev.00/

Now you can still compare cacerts bit by bit.

Thanks,
Max

On Jun 12, 2019, at 10:50 PM, Weijun Wang  
wrote:


Hi Erik,

Are you going to fix this bug soon?

I am inspired by Martin's words and would like to update 
GenerateCacerts.java so that as long as the certs and their aliases 
are unchanged, the output cacerts will always be the same. I can send 
out a code review today.


Thanks,
Max

On Jun 12, 2019, at 10:59 AM, Weijun Wang  
wrote:


Good idea about the creation time.

--Max

On Jun 12, 2019, at 10:53 AM, Martin Buchholz  
wrote:


Google culture really likes build output determinism, and we 
recently built our own cacerts generator.


To get determinism, we are using cert digest as alias (must have a 
unique alias, but value doesn't seem to matter much), and using 
cert notBefore instead of current (build) timestamp.


On Mon, Jun 10, 2019 at 12:40 PM Erik Joelsson 
 wrote:

Since JDK-8193255, when we started generating the cacerts file in the
build, the build compare baseline builds have started failing. It 
seems
the cacerts binary file has some non determinism built in so it 
doesn't

get generated exactly the same given the same input. This patch adds
special handling when comparing that file by comparing the output of
"keytool -list" on the files instead.

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

Webrev: http://cr.openjdk.java.net/~erikj/8225392/webrev.01/

/Erik



Re: RFR: JDK-8225392: Comparison builds are failing due to cacerts file

2019-06-12 Thread Erik Joelsson

Hello Max,

Much appreciated! I will need to have this fixed one way or other in JDK 
13, so depending on if you get your fix there in time, I will retract my 
proposal. If your fix only hits 14, I will push mine to 13.


/Erik

On 2019-06-12 08:41, Weijun Wang wrote:

This is my version of the fix:

http://cr.openjdk.java.net/~weijun/8225392/webrev.00/

Now you can still compare cacerts bit by bit.

Thanks,
Max


On Jun 12, 2019, at 10:50 PM, Weijun Wang  wrote:

Hi Erik,

Are you going to fix this bug soon?

I am inspired by Martin's words and would like to update GenerateCacerts.java 
so that as long as the certs and their aliases are unchanged, the output 
cacerts will always be the same. I can send out a code review today.

Thanks,
Max


On Jun 12, 2019, at 10:59 AM, Weijun Wang  wrote:

Good idea about the creation time.

--Max


On Jun 12, 2019, at 10:53 AM, Martin Buchholz  wrote:

Google culture really likes build output determinism, and we recently built our 
own cacerts generator.

To get determinism, we are using cert digest as alias (must have a unique 
alias, but value doesn't seem to matter much), and using cert notBefore instead 
of current (build) timestamp.

On Mon, Jun 10, 2019 at 12:40 PM Erik Joelsson  wrote:
Since JDK-8193255, when we started generating the cacerts file in the
build, the build compare baseline builds have started failing. It seems
the cacerts binary file has some non determinism built in so it doesn't
get generated exactly the same given the same input. This patch adds
special handling when comparing that file by comparing the output of
"keytool -list" on the files instead.

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

Webrev: http://cr.openjdk.java.net/~erikj/8225392/webrev.01/

/Erik



Re: RFR: JDK-8225392: Comparison builds are failing due to cacerts file

2019-06-12 Thread Martin Buchholz
I'm not a security engineer, but:
- consider creating static finals for e.g. "Mighty Aphrodite" just to give
it a symbolic name.
- VerifyCACerts probably fails when the jdk is configured with a different
cacerts file (but the JDK doesn't preserve configuration information - how
could one fix it?)
Many downstream organizations will configure a different cacerts.

On Wed, Jun 12, 2019 at 8:42 AM Weijun Wang  wrote:

> This is my version of the fix:
>
>http://cr.openjdk.java.net/~weijun/8225392/webrev.00/
>
> Now you can still compare cacerts bit by bit.
>
> Thanks,
> Max
>
> > On Jun 12, 2019, at 10:50 PM, Weijun Wang 
> wrote:
> >
> > Hi Erik,
> >
> > Are you going to fix this bug soon?
> >
> > I am inspired by Martin's words and would like to update
> GenerateCacerts.java so that as long as the certs and their aliases are
> unchanged, the output cacerts will always be the same. I can send out a
> code review today.
> >
> > Thanks,
> > Max
> >
> >> On Jun 12, 2019, at 10:59 AM, Weijun Wang 
> wrote:
> >>
> >> Good idea about the creation time.
> >>
> >> --Max
> >>
> >>> On Jun 12, 2019, at 10:53 AM, Martin Buchholz 
> wrote:
> >>>
> >>> Google culture really likes build output determinism, and we recently
> built our own cacerts generator.
> >>>
> >>> To get determinism, we are using cert digest as alias (must have a
> unique alias, but value doesn't seem to matter much), and using cert
> notBefore instead of current (build) timestamp.
> >>>
> >>> On Mon, Jun 10, 2019 at 12:40 PM Erik Joelsson <
> erik.joels...@oracle.com> wrote:
> >>> Since JDK-8193255, when we started generating the cacerts file in the
> >>> build, the build compare baseline builds have started failing. It
> seems
> >>> the cacerts binary file has some non determinism built in so it
> doesn't
> >>> get generated exactly the same given the same input. This patch adds
> >>> special handling when comparing that file by comparing the output of
> >>> "keytool -list" on the files instead.
> >>>
> >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8225392
> >>>
> >>> Webrev: http://cr.openjdk.java.net/~erikj/8225392/webrev.01/
> >>>
> >>> /Erik
> >>>
> >>
> >
>
>


Re: RFR: JDK-8225392: Comparison builds are failing due to cacerts file

2019-06-12 Thread Weijun Wang
This is my version of the fix:

   http://cr.openjdk.java.net/~weijun/8225392/webrev.00/

Now you can still compare cacerts bit by bit.

Thanks,
Max

> On Jun 12, 2019, at 10:50 PM, Weijun Wang  wrote:
> 
> Hi Erik,
> 
> Are you going to fix this bug soon?
> 
> I am inspired by Martin's words and would like to update GenerateCacerts.java 
> so that as long as the certs and their aliases are unchanged, the output 
> cacerts will always be the same. I can send out a code review today.
> 
> Thanks,
> Max
> 
>> On Jun 12, 2019, at 10:59 AM, Weijun Wang  wrote:
>> 
>> Good idea about the creation time.
>> 
>> --Max
>> 
>>> On Jun 12, 2019, at 10:53 AM, Martin Buchholz  wrote:
>>> 
>>> Google culture really likes build output determinism, and we recently built 
>>> our own cacerts generator.
>>> 
>>> To get determinism, we are using cert digest as alias (must have a unique 
>>> alias, but value doesn't seem to matter much), and using cert notBefore 
>>> instead of current (build) timestamp.
>>> 
>>> On Mon, Jun 10, 2019 at 12:40 PM Erik Joelsson  
>>> wrote:
>>> Since JDK-8193255, when we started generating the cacerts file in the 
>>> build, the build compare baseline builds have started failing. It seems 
>>> the cacerts binary file has some non determinism built in so it doesn't 
>>> get generated exactly the same given the same input. This patch adds 
>>> special handling when comparing that file by comparing the output of 
>>> "keytool -list" on the files instead.
>>> 
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8225392
>>> 
>>> Webrev: http://cr.openjdk.java.net/~erikj/8225392/webrev.01/
>>> 
>>> /Erik
>>> 
>> 
> 



Re: RFR [13] JDK-8224829 : AsyncSSLSocketClose.java has timing issue

2019-06-12 Thread Daniel Fuchs

Hi Xuelei,

I am not an expert of the field so please get another
reviewer for this change:

 656 if (!super.isOutputShutdown() &&
 ...
 661 } else if (!super.isOutputShutdown()) {

I think it might be clearer with a nested if:

   if (!super.isOutputShutdown()) {
   if (isLayered() && !autoClose) {
   throw exception ...
   } else {
   super.shutdownOutput();
   log ...
   }
   }

Could lines 630 .. 647 be moved to a private method?
Something like:

 private void handleClosedNotifyAlert() {
 630 try {
 // send a user_canceled alert if needed.
 if (useUserCanceled) {
 conContext.warning(Alert.USER_CANCELED);
 }

 // send a close_notify alert
 conContext.warning(Alert.CLOSE_NOTIFY);
 } finally {
 if (!conContext.isOutboundClosed()) {
 conContext.outputRecord.close();
 }

 if (!super.isOutputShutdown() &&
 (autoClose || !isLayered())) {
 super.shutdownOutput();
 647 }
  }
  }

because if I'm not mistaken this exact same piece of code
appears at lines 700 - 717.

I believe this would simplify this long method a little bit,
and possibly makes what is happening here clearer:

 623 if (linger >= 0) {
// don't wait more than SO_LINGER for obtaining the
// the lock...
 ...
 626 try {
 627 if (conContext.outputRecord.recordLock.tryLock() ||
 628 conContext.outputRecord.recordLock.tryLock(
 629 linger, TimeUnit.SECONDS)) {
 try {
 handleClosedNotifyAlert();
 } finally {
 conContext.outputRecord.recordLock.unlock();
 }
 652 } else {
 653 // For layered, non-autoclose sockets, we are not
 654 // able to bring them into a usable state, so we
 655 // treat it as fatal error.
 ...
 688 }
 689 } catch (InterruptedException ex) {
   ...
 692 }
 693
 694 // restore the interrupted status
 ...
 698 } else {
 699 conContext.outputRecord.recordLock.lock();
 700 try {
 handleClosedNotifyAlert();
 } finally {
 conContext.outputRecord.recordLock.unlock();
 }
  }
  ...


BlockedAsyncClose.java:

   Can you bind the server to the loopback instead of the
   wildcard? My experience is that it make tests more reliable.

 InetAddress loopback = InetAddress.getLoopbackAddress();
  74 ss = (SSLServerSocket) sslssf.createServerSocket();
 ss.bind(new InetSosketAddress(loopback, 0));
  75
  76 SSLSocketFactory sslsf =
  77 (SSLSocketFactory)SSLSocketFactory.getDefault();
 socket = (SSLSocket)sslsf.createSocket(loopback, 
ss.getLocalPort());



best regards,

-- daniel

On 12/06/2019 14:51, Xuelei Fan wrote:

ping ...

On 6/4/2019 11:10 AM, Xuelei Fan wrote:

Hi,

Could I get the following update reviewed?
    http://cr.openjdk.java.net/~xuelei/8224829/webrev.00/

If using one thread for closing, one thread for writing.  Closing and 
writing threads are synchronized in order to delivery close_notify TLS 
record.  There are could be race between the two threads.  If the 
output stream buffer reach the limit, the write will be blocked, and 
then the close thread may be pending.


The SO_LINGER is used to resolve the issue for general socket.  With 
the above update, the SSLSocket implementation will check the 
SO_LINGER as well.  If SO_LINGER is on, the SSLSocket close will try 
to get the synchronization lock in the time of SO_LINGER value.  If 
timeout, the close process will move on, and close the underlying 
socket by force.


Thanks,
Xuelei




Re: RFR (RFE-13): JDK-8224520: Support X25519 and X448 in TLS

2019-06-12 Thread Sean Mullan

On 6/11/19 7:19 PM, Bradford Wetmore wrote:

Just noticed a couple things so far:


Is there more coming?  I plan to putback tomorrow to make RDP1.  We can 
do an RFE during RDP1->RDP2 if you find something.


I'm done. Could you add a release note to the issue? I just skimmed thru 
the rest and had one question:


KAKeyDerivation.java

 100 AlgorithmParameterSpec params) throws IOException {

This parameter is never used. Is that intentional?

--Sean


Re: RFR [13] JDK-8224829 : AsyncSSLSocketClose.java has timing issue

2019-06-12 Thread Xuelei Fan

ping ...

On 6/4/2019 11:10 AM, Xuelei Fan wrote:

Hi,

Could I get the following update reviewed?
    http://cr.openjdk.java.net/~xuelei/8224829/webrev.00/

If using one thread for closing, one thread for writing.  Closing and 
writing threads are synchronized in order to delivery close_notify TLS 
record.  There are could be race between the two threads.  If the output 
stream buffer reach the limit, the write will be blocked, and then the 
close thread may be pending.


The SO_LINGER is used to resolve the issue for general socket.  With the 
above update, the SSLSocket implementation will check the SO_LINGER as 
well.  If SO_LINGER is on, the SSLSocket close will try to get the 
synchronization lock in the time of SO_LINGER value.  If timeout, the 
close process will move on, and close the underlying socket by force.


Thanks,
Xuelei


Re: RFR (RFE-13): JDK-8224520: Support X25519 and X448 in TLS

2019-06-12 Thread Xuelei Fan

Thanks!  I have no more comments.

Xuelei

On 6/11/2019 4:19 PM, Bradford Wetmore wrote:

Updated webrev:

http://cr.openjdk.java.net/~wetmore/8171279/webrev.01/

Xuelei wrote:

 > ECDHClientKeyExchange.java
 > --
 > The enum builtin method valueOf(String) is used.  There is not problem
 > here. But as requires the enum name in NamedGroup is exactly the same as
 > the name defined in NamedParameterSpec.  It might be a potential risk
 > for future update of the names.

Unfortunately, all I have available is the key.getAlgorithm() and the 
key.getParams() String from the NamedParameterSpec at this point, so 
I've switched to NamedGroup.nameOf(String) which is what other places in 
the code do.