date:20161102

Everything is fine now.

Thanks
Max

On 11/3/2016 9:38 AM, Artem Smotrakov wrote:

My bad, I missed that.

http://cr.openjdk.java.net/~asmotrak/8168882/webrev.02/

Artem

On 11/02/2016 06:30 PM, Wang Weijun wrote:

On 11/01/2016 11:59 PM, Wang Weijun wrote:

>> Main.java:
>>
>> The warning (and the subsequent empty line) should be printed into 
System.err.

This one?

Re: [9] RFR: 8168882: keytool doesn't print certificate info if disabled algorithm was used for signing a jar


My bad, I missed that.

http://cr.openjdk.java.net/~asmotrak/8168882/webrev.02/

Artem


On 11/02/2016 06:30 PM, Wang Weijun wrote:

On 11/01/2016 11:59 PM, Wang Weijun wrote:

>>Main.java:
>>
>>The warning (and the subsequent empty line) should be printed into System.err.

This one?

Re: [9] RFR: 8168882: keytool doesn't print certificate info if disabled algorithm was used for signing a jar


> On Nov 3, 2016, at 8:27 AM, Artem Smotrakov  
> wrote:
> 
> Hi Max,
> 
> Please see inline.
> 
> 
> On 11/01/2016 11:59 PM, Wang Weijun wrote:
>> Main.java:
>> 
>> The warning (and the subsequent empty line) should be printed into 
>> System.err.

This one?

> 
> Thank you for review Max, please take a look at updated webrev:
> 
> http://cr.openjdk.java.net/~asmotrak/8168882/webrev.01/

Fine except for the System.out one above.

> 
> By the way, I just noticed that we have another version of JarUtils.java 
> which was added by Alan 7 month ago
> 
> http://hg.openjdk.java.net/jdk9/dev/jdk/log/1396fb6d0279/test/lib/testlibrary/JarUtils.java

Looks like it's only used by himself, and has 2 lines of history.

Also, my understanding is that we are switching to test utils in the root repo. 
A lot of same name utils in jdk/test were deprecated.

Thanks
Max

> 
> Artem

Re: [9] RFR: 8168969: Merge SSLSocketSample and SSLSocketTemplate


Hi John,

On 11/02/2016 05:36 PM, John Jiang wrote:
Correct, AnonCipherWithWantClientAuth can work fine without extending 
SSLSocketTemplate.


I make those classes to extend SSLSocketTemplate to make lines 
shorter (we keep lines shorter than 80 symbols).
In practice, you could divide one line to two lines if the line length 
is greater than 80.
Correct. Extending SSLSocketTemplate allows not to mention class name 
when you call a static method which makes lines shorter.




The original SSLSocketSample.java defines the core methods, such as 
doServerSide(), runServerApplication(), doClientSide() and 
runClientApplication(), as non-static. In my eyes, this style may be 
better. The child class can simply override the methods if necessary.
I think it may be better to keep them static which also means 
stateless here. It would be easier to re-use in my opinion.


In addition, some tests have to configure SSLServerSocket. Why not 
provide one more extension point in doServerSide()? Then, it 
unnecessary to re-write the whole doServerSide() (or, set a new 
server peer).
I see your point, and agree with that. I would prefer to add this 
extension when we update a test which really needs it. If you have 
such a bug, please feel free to do that.
The test AnonCipherWithWantClientAuth.java in your patch looks need 
this extension.
This update is about merging files in javax/net/ssl/template to avoid 
confusions Brad mentioned recently.


I would prefer to implement other enhancements separately. This may also 
eliminate some confusions.




We have more than one hundred SSL/TLS tests, and they may need some 
other extensions. I am not sure that it's possible to provide a 
universal SSLSocketTemplate which fits all test's needs.
I see. It's unnecessary to design too much before getting more 
concrete requirements.

Agree. Please see above.

Artem



Best regards,
John Jiang



Artem
The code talks more clearly. Please take a look at my example: 
http://cr.openjdk.java.net/~jjiang/8168969/example/


Best regards,
John Jiang

On 2016/11/2 8:54, Artem Smotrakov wrote:

Hello,

Please review the following patch which merges a couple of classes 
in javax/net/ssl/templates.


SSLTest class contains re-usable parts of SSLSocketSample. 
SSLSocketTemplate class is buggy (tests which follows it may fail 
intermittently). I basically replaced SSLSocketTemplate with 
SSLTest, and removed SSLSocketSample.


SSL/TLS tests should use SSLSocketTemplate class. I updated test 
which use SSLTest to use SSLSocketTemplate.


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

Artem

Re: [9] RFR: 8168969: Merge SSLSocketSample and SSLSocketTemplate

2016-11-02 Thread John Jiang


Hi Artem,

On 2016/11/2 23:54, Artem Smotrakov wrote:

Hi John,

Please see inline.


On 11/01/2016 11:48 PM, John Jiang wrote:

Hi Artem,
Thanks for making the template to be used easily.

The tests in your patch extend class SSLSocketTemplate, but 
SSLSocketTemplate looks like an utility class, but not a parent class.

For example,
test/sun/security/ssl/ServerHandshaker/AnonCipherWithWantClientAuth.java
  67 new SSLSocketTemplate()
AnonCipherWithWantClientAuth still creates a SSLSocketTemplate 
instance, but not itself.
In fact, if replacing "print()" with "SSLSocketTemplate.print()", 
AnonCipherWithWantClientAuth can work fine without extending 
SSLSocketTemplate.
Correct, AnonCipherWithWantClientAuth can work fine without extending 
SSLSocketTemplate.


I make those classes to extend SSLSocketTemplate to make lines shorter 
(we keep lines shorter than 80 symbols).
In practice, you could divide one line to two lines if the line length 
is greater than 80.




The original SSLSocketSample.java defines the core methods, such as 
doServerSide(), runServerApplication(), doClientSide() and 
runClientApplication(), as non-static. In my eyes, this style may be 
better. The child class can simply override the methods if necessary.
I think it may be better to keep them static which also means 
stateless here. It would be easier to re-use in my opinion.


In addition, some tests have to configure SSLServerSocket. Why not 
provide one more extension point in doServerSide()? Then, it 
unnecessary to re-write the whole doServerSide() (or, set a new 
server peer).
I see your point, and agree with that. I would prefer to add this 
extension when we update a test which really needs it. If you have 
such a bug, please feel free to do that.
The test AnonCipherWithWantClientAuth.java in your patch looks need this 
extension.




We have more than one hundred SSL/TLS tests, and they may need some 
other extensions. I am not sure that it's possible to provide a 
universal SSLSocketTemplate which fits all test's needs.
I see. It's unnecessary to design too much before getting more concrete 
requirements.



Best regards,
John Jiang



Artem
The code talks more clearly. Please take a look at my example: 
http://cr.openjdk.java.net/~jjiang/8168969/example/


Best regards,
John Jiang

On 2016/11/2 8:54, Artem Smotrakov wrote:

Hello,

Please review the following patch which merges a couple of classes 
in javax/net/ssl/templates.


SSLTest class contains re-usable parts of SSLSocketSample. 
SSLSocketTemplate class is buggy (tests which follows it may fail 
intermittently). I basically replaced SSLSocketTemplate with 
SSLTest, and removed SSLSocketSample.


SSL/TLS tests should use SSLSocketTemplate class. I updated test 
which use SSLTest to use SSLSocketTemplate.


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

Artem

Re: RFR 8133632: javax.net.ssl.SSLEngine does not properly handle received SSL fatal alerts


Looks fine to me.

Thanks,
Xuelei

On 11/3/2016 8:13 AM, Jamil Nimeh wrote:

Good suggestion.  Updated webrev below:

http://cr.openjdk.java.net/~jnimeh/reviews/8133632/webrev.02/

On 11/2/2016 1:56 AM, Xuelei Fan wrote:

Looks fine to me exception that you may also want to consider the case:

1850   if (description == -1) { // check for short message
1851   fatal(Alerts.alert_illegal_parameter, "Short alert message");
1852   }

If the level is not warning, please don't sent the alert any more at
line 1851 (via fatal()).

Xuelei

On 11/2/2016 3:30 PM, Jamil Nimeh wrote:

Hello folks,

This fixes an issue in SSLEngine that happens when an engine unwraps a
TLS fatal alert record.  The resulting engine state still leaves both
input and output queues in an open state, and in NEED_UNWRAP. Unwrapping
just causes the exception thrown as a result of processing the exception
to be thrown again.

This fix updates the resulting state of the engine in this particular
case to have both I/O queues closed and updates the state of the engine
to NOT_HANDSHAKING.

Bug: https://bugs.openjdk.java.net/browse/JDK-8133632
Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8133632/webrev.01/

Thanks,
--Jamil

Re: [9] RFR: 8168882: keytool doesn't print certificate info if disabled algorithm was used for signing a jar

Hi Max,

Please see inline.

On 11/01/2016 11:59 PM, Wang Weijun wrote:

Main.java:

The warning (and the subsequent empty line) should be printed into System.err.

Resources.java:

"This tool accepts any algorithm" is a little confusing (sorry that I originally suggested it).
Maybe "This tool does not attempt to verify a signed jar file, please run \"jarsigner
-verify\" if you want to."

Sure, I'll update the message.

Also, ever since the 1st time hard coded strings are changed into dot-connected
resource keys, newly added keys do not necessarily use the exact same string.
Make it simple so next time if the value needs to be updated you don't need to
change the key.
Agree, I was thinking about shorter key, but then noticed that most of
keys look like messages. I'll make the key shorter.

Test:

- You can also add -Duser.language=en and -Duser.country=US to keytool.

Makes sense to me, will do.

- With my recent update to JarUtils.createJar(), there is no need to create the
"test" file.
Right. I'll remove it, however creating a file would make it a bit
clearer to me.

Thank you for review Max, please take a look at updated webrev:

http://cr.openjdk.java.net/~asmotrak/8168882/webrev.01/

By the way, I just noticed that we have another version of JarUtils.java
which was added by Alan 7 month ago

http://hg.openjdk.java.net/jdk9/dev/jdk/log/1396fb6d0279/test/lib/testlibrary/JarUtils.java

Artem

Everything else looks fine.

Thanks
Max

On Nov 2, 2016, at 7:35 AM, Artem Smotrakov wrote:

Hello,

Please review this small update for keytool.

"keytool -printcert -jarfile" doesn't work with jars which were signed with algorithms
listed in "jdk.jar.disabledAlgorithms" security property.

The patch below resets "jdk.jar.disabledAlgorithms" security property before
reading a jar file, and prints a warning.

I also re-wrote readjar.sh test, and added SecurityTools class with a couple of
re-usable methods for jarsigner and keytool (those methods are based on methods
from TimestampCheck.java).

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

Artem

Re: RFR 8133632: javax.net.ssl.SSLEngine does not properly handle received SSL fatal alerts

2016-11-02 Thread Jamil Nimeh


Good suggestion.  Updated webrev below:

http://cr.openjdk.java.net/~jnimeh/reviews/8133632/webrev.02/

On 11/2/2016 1:56 AM, Xuelei Fan wrote:

Looks fine to me exception that you may also want to consider the case:

1850   if (description == -1) { // check for short message
1851   fatal(Alerts.alert_illegal_parameter, "Short alert message");
1852   }

If the level is not warning, please don't sent the alert any more at 
line 1851 (via fatal()).


Xuelei

On 11/2/2016 3:30 PM, Jamil Nimeh wrote:

Hello folks,

This fixes an issue in SSLEngine that happens when an engine unwraps a
TLS fatal alert record.  The resulting engine state still leaves both
input and output queues in an open state, and in NEED_UNWRAP. Unwrapping
just causes the exception thrown as a result of processing the exception
to be thrown again.

This fix updates the resulting state of the engine in this particular
case to have both I/O queues closed and updates the state of the engine
to NOT_HANDSHAKING.

Bug: https://bugs.openjdk.java.net/browse/JDK-8133632
Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8133632/webrev.01/

Thanks,
--Jamil

Re: [9] RFR: 8168969: Merge SSLSocketSample and SSLSocketTemplate


Hi Xuelei,

This totally makes sense. But in my opinion we should use new features 
and APIs because:

- they are here to make code readable (okay, not to much lambdas)
- they help to avoid bugs (e.g., forgetting to close resources)
- we implicitly provide test coverage for new APIs
- we give examples how these features can be used
- finally, we encourage people to move to new Java versions

Not sure if I got correction what you mean by removing the part to 
declare the store files in each sub-classes. Do you mean getting rid of 
setting keystore/truststore in actual test files?


If so, I would prefer to do it in a separate enhancement. Most of 
SSL/TLS tests use keystores from "etc" directory. The test use 
"test.src" system property (set by jtreg) to build a path to "etc", and 
it depends on actual directory where a test stays. Also if I recall 
correctly, some tests use their own keystore. This update may be quite 
big. Would you mind if we did it separately?


8168969 is about merging helper classes to remove possible confusions 
Brad mentioned recently.


Artem

On 11/02/2016 03:57 AM, Xuelei Fan wrote:
Please use JDK 6 only features (no Lambda, try-with-resource, 
multi-catch, java.util.Objects, etc.) so as to expedite porting to 
previous releases.


It would be nice if removing the part to declare the store files in 
each sub-classes.


Xuelei

On 11/2/2016 2:48 PM, John Jiang wrote:

Hi Artem,
Thanks for making the template to be used easily.

The tests in your patch extend class SSLSocketTemplate, but
SSLSocketTemplate looks like an utility class, but not a parent class.
For example,
test/sun/security/ssl/ServerHandshaker/AnonCipherWithWantClientAuth.java
  67 new SSLSocketTemplate()
AnonCipherWithWantClientAuth still creates a SSLSocketTemplate instance,
but not itself.
In fact, if replacing "print()" with "SSLSocketTemplate.print()",
AnonCipherWithWantClientAuth can work fine without extending
SSLSocketTemplate.

The original SSLSocketSample.java defines the core methods, such as
doServerSide(), runServerApplication(), doClientSide() and
runClientApplication(), as non-static. In my eyes, this style may be
better. The child class can simply override the methods if necessary.

In addition, some tests have to configure SSLServerSocket. Why not
provide one more extension point in doServerSide()? Then, it unnecessary
to re-write the whole doServerSide() (or, set a new server peer).
The code talks more clearly. Please take a look at my example:
http://cr.openjdk.java.net/~jjiang/8168969/example/

Best regards,
John Jiang

On 2016/11/2 8:54, Artem Smotrakov wrote:

Hello,

Please review the following patch which merges a couple of classes in
javax/net/ssl/templates.

SSLTest class contains re-usable parts of SSLSocketSample.
SSLSocketTemplate class is buggy (tests which follows it may fail
intermittently). I basically replaced SSLSocketTemplate with SSLTest,
and removed SSLSocketSample.

SSL/TLS tests should use SSLSocketTemplate class. I updated test which
use SSLTest to use SSLSocketTemplate.

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

Artem

Re: RFR 7004967: SecureRandom should be more explicit about threading



> 在 2016年11月2日，21:50，Xuelei Fan  写道：
> 
> I may change "the service provider attribute" to "the service attribute".

I didn't invent that name, I remember it's named exactly like this in one of 
the security guides. Will check tomorrow, not near a big screen now. 

Thanks
Max

Re: RFR 7004967: SecureRandom should be more explicit about threading


Not a big concern of mine.  Please go ahead if you would like.

Xuelei

On 11/2/2016 9:58 PM, Wang Weijun wrote:




在 2016年11月2日，21:50，Xuelei Fan  写道：

I may change "the service provider attribute" to "the service attribute".


I didn't invent that name, I remember it's named exactly like this in one of 
the security guides. Will check tomorrow, not near a big screen now.

Thanks
Max

Re: RFR 7004967: SecureRandom should be more explicit about threading

On 11/2/2016 9:33 PM, Wang Weijun wrote:

>>>
>>> 1.  More specific
>>>
>>>  "A SecureRandom service provider can advertise that it is
>>>   thread-safe by setting the service provider attribute
>>>   "ThreadSafe" to "true" when registering the provider."
>>>
>>> A service provider may contains many services implementations.  May need to be more 
specific to set "ThreadSafe" for SecureRandom only, rather the full provider is thread 
safe.  For example:
>>>
>>>   map.put("SecureRandom.SHA1PRNG ThreadSafe", "true");
>>>
>>> Otherwise, a service provider need to make sure all services are thread 
safe, or all services implementation are not thread safe.

How about changing "A SecureRandom service provider" to "A SecureRandom 
implementation"?

I may change "the service provider attribute" to "the service attribute".

   A SecureRandom service provider can advertise that the
   implementation is thread-safe by setting the service attribute
   "ThreadSafe" to "true" when registering the provider.

Xuelei

Re: RFR 7004967: SecureRandom should be more explicit about threading


Oops, I was mis-reading with the following code.

 990  super(token.provider, type, algorithm, className, toList(al),
 991  type.equals(SR) ? Map.of("ThreadSafe", "true") : null);

OK, it's fine.

Xuelei

On 11/2/2016 9:33 PM, Wang Weijun wrote:

 I may use the property similar to:

  map.put("SecureRandom.SHA1PRNG ThreadSafe", "true");

Re: RFR 7004967: SecureRandom should be more explicit about threading


> On Nov 2, 2016, at 9:18 PM, Xuelei Fan  wrote:
> 
> On 11/2/2016 8:47 PM, Wang Weijun wrote:
>> 
>>> On Nov 2, 2016, at 5:34 PM, Xuelei Fan  wrote:
>>> 
>>> 1.  More specific
>>> 
>>>  "A SecureRandom service provider can advertise that it is
>>>   thread-safe by setting the service provider attribute
>>>   "ThreadSafe" to "true" when registering the provider."
>>> 
>>> A service provider may contains many services implementations.  May need to 
>>> be more specific to set "ThreadSafe" for SecureRandom only, rather the full 
>>> provider is thread safe.  For example:
>>> 
>>>   map.put("SecureRandom.SHA1PRNG ThreadSafe", "true");
>>> 
>>> Otherwise, a service provider need to make sure all services are thread 
>>> safe, or all services implementation are not thread safe.
>> 
>> How about changing "A SecureRandom service provider" to "A SecureRandom 
>> implementation"?
>> 
> I don't think it is sufficient.  See bellow.
> 
>>> 
>>> 2. "true" is the only true property value.
>>>   "If this attribute is not set or is "false", this class will
>>>instead ..."
>>> 
>>> If the attribute is set to "yes" or "hello, world!", I think it is the same 
>>> as set to "false" per your current implementation.
>>> 
>>>   "Otherwise, this class will ... "
>> 
>> OK.
>> 
>>> 
>>> May need to update the implementation accordingly if you accept the 
>>> comments.
>> 
>> Why update the implementation?
>> 
> If I'm reading correct, you are setting the "ThreadSafe" for provider, but 
> not for SecureRandom implementation.

Sorry but I don't understand what you mean.

On read side, the threadSafe field is for each "SecureRandom." + algorithm, so 
it is for an implementation.

On write side, every call looks like the line you write below.

>  I may use the property similar to:
> 
>   map.put("SecureRandom.SHA1PRNG ThreadSafe", "true");
> 
> That's why I don't think above update is not sufficient.

Can you point out on exactly which line in webrev I'm doing wrong?

Thanks
Max


> 
> Xuelei
> 
>> Thanks
>> Max
>> 
>>> 
>>> Xuelei
>>> 
>>> 
>>> On 11/2/2016 3:27 PM, Wang Weijun wrote:
 Ping again.
 
 There is an updated version at 
 http://cr.openjdk.java.net/~weijun/7004967/webrev.01/ with doc-only 
 changes.
 
 Thanks
 Max
 
> On Aug 25, 2016, at 10:00 AM, Weijun Wang  wrote:
> 
> Please review the enhancement at
> 
> http://cr.openjdk.java.net/~weijun/7004967/webrev.00/
> 
> Basically, we want SecureRandom to be more efficient by removing all 
> synchronized keywords from its public methods and let an implementation 
> to take care of thread-safety (We already did some in JDK-8098581). On 
> the other hand, we need to make sure that existing implementations that 
> have not synchronized correctly to behave just as good as before.
> 
> Therefore a new Service Attribute "ThreadSafe" is introduced. If you 
> think your implementation is already thread-safe, set it to "true" and 
> SecureRandom will be happy. Otherwise, don't set it and SecureRandom will 
> continuously call your SecureRandomSpi engine methods in synchronized 
> blocks.
> 
> Thanks
> Max

Re: RFR 7004967: SecureRandom should be more explicit about threading


On 11/2/2016 8:47 PM, Wang Weijun wrote:



On Nov 2, 2016, at 5:34 PM, Xuelei Fan  wrote:

1.  More specific

  "A SecureRandom service provider can advertise that it is
   thread-safe by setting the service provider attribute
   "ThreadSafe" to "true" when registering the provider."

A service provider may contains many services implementations.  May need to be more 
specific to set "ThreadSafe" for SecureRandom only, rather the full provider is 
thread safe.  For example:

   map.put("SecureRandom.SHA1PRNG ThreadSafe", "true");

Otherwise, a service provider need to make sure all services are thread safe, 
or all services implementation are not thread safe.


How about changing "A SecureRandom service provider" to "A SecureRandom 
implementation"?


I don't think it is sufficient.  See bellow.



2. "true" is the only true property value.
   "If this attribute is not set or is "false", this class will
instead ..."

If the attribute is set to "yes" or "hello, world!", I think it is the same as set to 
"false" per your current implementation.

   "Otherwise, this class will ... "


OK.



May need to update the implementation accordingly if you accept the comments.


Why update the implementation?

If I'm reading correct, you are setting the "ThreadSafe" for provider, 
but not for SecureRandom implementation.  I may use the property similar to:


   map.put("SecureRandom.SHA1PRNG ThreadSafe", "true");

That's why I don't think above update is not sufficient.

Xuelei


Thanks
Max



Xuelei


On 11/2/2016 3:27 PM, Wang Weijun wrote:

Ping again.

There is an updated version at 
http://cr.openjdk.java.net/~weijun/7004967/webrev.01/ with doc-only changes.

Thanks
Max


On Aug 25, 2016, at 10:00 AM, Weijun Wang  wrote:

Please review the enhancement at

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

Basically, we want SecureRandom to be more efficient by removing all 
synchronized keywords from its public methods and let an implementation to take 
care of thread-safety (We already did some in JDK-8098581). On the other hand, 
we need to make sure that existing implementations that have not synchronized 
correctly to behave just as good as before.

Therefore a new Service Attribute "ThreadSafe" is introduced. If you think your 
implementation is already thread-safe, set it to "true" and SecureRandom will be happy. 
Otherwise, don't set it and SecureRandom will continuously call your SecureRandomSpi engine methods 
in synchronized blocks.

Thanks
Max

Re: RFR 7004967: SecureRandom should be more explicit about threading


> On Nov 2, 2016, at 5:34 PM, Xuelei Fan  wrote:
> 
> 1.  More specific
> 
>   "A SecureRandom service provider can advertise that it is
>thread-safe by setting the service provider attribute
>"ThreadSafe" to "true" when registering the provider."
> 
> A service provider may contains many services implementations.  May need to 
> be more specific to set "ThreadSafe" for SecureRandom only, rather the full 
> provider is thread safe.  For example:
> 
>map.put("SecureRandom.SHA1PRNG ThreadSafe", "true");
> 
> Otherwise, a service provider need to make sure all services are thread safe, 
> or all services implementation are not thread safe.

How about changing "A SecureRandom service provider" to "A SecureRandom 
implementation"?

> 
> 2. "true" is the only true property value.
>"If this attribute is not set or is "false", this class will
> instead ..."
> 
> If the attribute is set to "yes" or "hello, world!", I think it is the same 
> as set to "false" per your current implementation.
> 
>"Otherwise, this class will ... "

OK.

> 
> May need to update the implementation accordingly if you accept the comments.

Why update the implementation?

Thanks
Max

> 
> Xuelei
> 
> 
> On 11/2/2016 3:27 PM, Wang Weijun wrote:
>> Ping again.
>> 
>> There is an updated version at 
>> http://cr.openjdk.java.net/~weijun/7004967/webrev.01/ with doc-only changes.
>> 
>> Thanks
>> Max
>> 
>>> On Aug 25, 2016, at 10:00 AM, Weijun Wang  wrote:
>>> 
>>> Please review the enhancement at
>>> 
>>> http://cr.openjdk.java.net/~weijun/7004967/webrev.00/
>>> 
>>> Basically, we want SecureRandom to be more efficient by removing all 
>>> synchronized keywords from its public methods and let an implementation to 
>>> take care of thread-safety (We already did some in JDK-8098581). On the 
>>> other hand, we need to make sure that existing implementations that have 
>>> not synchronized correctly to behave just as good as before.
>>> 
>>> Therefore a new Service Attribute "ThreadSafe" is introduced. If you think 
>>> your implementation is already thread-safe, set it to "true" and 
>>> SecureRandom will be happy. Otherwise, don't set it and SecureRandom will 
>>> continuously call your SecureRandomSpi engine methods in synchronized 
>>> blocks.
>>> 
>>> Thanks
>>> Max
>>

Re: [9] RFR: 8168969: Merge SSLSocketSample and SSLSocketTemplate

Please use JDK 6 only features (no Lambda, try-with-resource, 
multi-catch, java.util.Objects, etc.) so as to expedite porting to 
previous releases.


It would be nice if removing the part to declare the store files in each 
sub-classes.


Xuelei

On 11/2/2016 2:48 PM, John Jiang wrote:

Hi Artem,
Thanks for making the template to be used easily.

The tests in your patch extend class SSLSocketTemplate, but
SSLSocketTemplate looks like an utility class, but not a parent class.
For example,
test/sun/security/ssl/ServerHandshaker/AnonCipherWithWantClientAuth.java
  67 new SSLSocketTemplate()
AnonCipherWithWantClientAuth still creates a SSLSocketTemplate instance,
but not itself.
In fact, if replacing "print()" with "SSLSocketTemplate.print()",
AnonCipherWithWantClientAuth can work fine without extending
SSLSocketTemplate.

The original SSLSocketSample.java defines the core methods, such as
doServerSide(), runServerApplication(), doClientSide() and
runClientApplication(), as non-static. In my eyes, this style may be
better. The child class can simply override the methods if necessary.

In addition, some tests have to configure SSLServerSocket. Why not
provide one more extension point in doServerSide()? Then, it unnecessary
to re-write the whole doServerSide() (or, set a new server peer).
The code talks more clearly. Please take a look at my example:
http://cr.openjdk.java.net/~jjiang/8168969/example/

Best regards,
John Jiang

On 2016/11/2 8:54, Artem Smotrakov wrote:

Hello,

Please review the following patch which merges a couple of classes in
javax/net/ssl/templates.

SSLTest class contains re-usable parts of SSLSocketSample.
SSLSocketTemplate class is buggy (tests which follows it may fail
intermittently). I basically replaced SSLSocketTemplate with SSLTest,
and removed SSLSocketSample.

SSL/TLS tests should use SSLSocketTemplate class. I updated test which
use SSLTest to use SSLSocketTemplate.

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

Artem

Re: RFR 7004967: SecureRandom should be more explicit about threading


1.  More specific

   "A SecureRandom service provider can advertise that it is
thread-safe by setting the service provider attribute
"ThreadSafe" to "true" when registering the provider."

A service provider may contains many services implementations.  May need 
to be more specific to set "ThreadSafe" for SecureRandom only, rather 
the full provider is thread safe.  For example:


map.put("SecureRandom.SHA1PRNG ThreadSafe", "true");

Otherwise, a service provider need to make sure all services are thread 
safe, or all services implementation are not thread safe.


2. "true" is the only true property value.
"If this attribute is not set or is "false", this class will
 instead ..."

If the attribute is set to "yes" or "hello, world!", I think it is the 
same as set to "false" per your current implementation.


"Otherwise, this class will ... "

May need to update the implementation accordingly if you accept the 
comments.


Xuelei


On 11/2/2016 3:27 PM, Wang Weijun wrote:

Ping again.

There is an updated version at 
http://cr.openjdk.java.net/~weijun/7004967/webrev.01/ with doc-only changes.

Thanks
Max


On Aug 25, 2016, at 10:00 AM, Weijun Wang  wrote:

Please review the enhancement at

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

Basically, we want SecureRandom to be more efficient by removing all 
synchronized keywords from its public methods and let an implementation to take 
care of thread-safety (We already did some in JDK-8098581). On the other hand, 
we need to make sure that existing implementations that have not synchronized 
correctly to behave just as good as before.

Therefore a new Service Attribute "ThreadSafe" is introduced. If you think your 
implementation is already thread-safe, set it to "true" and SecureRandom will be happy. 
Otherwise, don't set it and SecureRandom will continuously call your SecureRandomSpi engine methods 
in synchronized blocks.

Thanks
Max

Re: RFR 8133632: javax.net.ssl.SSLEngine does not properly handle received SSL fatal alerts


Looks fine to me exception that you may also want to consider the case:

1850   if (description == -1) { // check for short message
1851   fatal(Alerts.alert_illegal_parameter, "Short alert message");
1852   }

If the level is not warning, please don't sent the alert any more at 
line 1851 (via fatal()).


Xuelei

On 11/2/2016 3:30 PM, Jamil Nimeh wrote:

Hello folks,

This fixes an issue in SSLEngine that happens when an engine unwraps a
TLS fatal alert record.  The resulting engine state still leaves both
input and output queues in an open state, and in NEED_UNWRAP. Unwrapping
just causes the exception thrown as a result of processing the exception
to be thrown again.

This fix updates the resulting state of the engine in this particular
case to have both I/O queues closed and updates the state of the engine
to NOT_HANDSHAKING.

Bug: https://bugs.openjdk.java.net/browse/JDK-8133632
Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8133632/webrev.01/

Thanks,
--Jamil

RFR 8133632: javax.net.ssl.SSLEngine does not properly handle received SSL fatal alerts

2016-11-02 Thread Jamil Nimeh


Hello folks,

This fixes an issue in SSLEngine that happens when an engine unwraps a 
TLS fatal alert record.  The resulting engine state still leaves both 
input and output queues in an open state, and in NEED_UNWRAP. Unwrapping 
just causes the exception thrown as a result of processing the exception 
to be thrown again.


This fix updates the resulting state of the engine in this particular 
case to have both I/O queues closed and updates the state of the engine 
to NOT_HANDSHAKING.


Bug: https://bugs.openjdk.java.net/browse/JDK-8133632
Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8133632/webrev.01/

Thanks,
--Jamil

Re: RFR 7004967: SecureRandom should be more explicit about threading

Ping again.

There is an updated version at 
http://cr.openjdk.java.net/~weijun/7004967/webrev.01/ with doc-only changes.

Thanks
Max

> On Aug 25, 2016, at 10:00 AM, Weijun Wang  wrote:
> 
> Please review the enhancement at
> 
>  http://cr.openjdk.java.net/~weijun/7004967/webrev.00/
> 
> Basically, we want SecureRandom to be more efficient by removing all 
> synchronized keywords from its public methods and let an implementation to 
> take care of thread-safety (We already did some in JDK-8098581). On the other 
> hand, we need to make sure that existing implementations that have not 
> synchronized correctly to behave just as good as before.
> 
> Therefore a new Service Attribute "ThreadSafe" is introduced. If you think 
> your implementation is already thread-safe, set it to "true" and SecureRandom 
> will be happy. Otherwise, don't set it and SecureRandom will continuously 
> call your SecureRandomSpi engine methods in synchronized blocks.
> 
> Thanks
> Max

Re: [9] RFR: 8168882: keytool doesn't print certificate info if disabled algorithm was used for signing a jar