EC weirdness

[Michael StJohns]

Hi -

Every so often I run into some rather strange things in the way the Sun 
EC classes were built.  Most recently, I was trying to use the SunEC 
provider to do a PACE like protocol.  Basically, the idea was to be able 
to generate public key points on the P-256 curve, but with a different 
base point G (knowledge of G' or having a public key generated from G' 
would allow you to do a valid ECDH operation, keys with disjoint points 
would not).


I was able to generate a normal key pair using ECGenParameterSpec with a 
name, so I grabbed the ECParameterSpec from the public key, made a copy 
of most of the stuff (curve, cofactor), but substituted the public point 
W from the key pair I'd just generated, and passed that in as G to the 
new parameter spec.  I tried to initialize the KPG with my *almost* 
P-256 spec, and it failed with "unsupported curve".


Looking into the code and tracing through 
sun.crypto.ec.ECKeyPairGenerator to the native code, I'm once again 
surprised that all of the curves are hard coded into the C native code, 
rather than being passed in as data from the Java code.


Is there a good security reason for hard coding the curves at the C 
level that I'm missing?


This comes under the heading of unexpected behavior rather than a bug 
per se I think.   Although - I'd *really* like to be able to pass a 
valid ECParameterSpec in to the generation process and get whatever keys 
are appropriate for that curve and base point.


Later, Mike

Re: RFR JDK-8029661: JDK-Support TLS v1.2 algorithm in SunPKCS11 provider

[Valerie Peng]

Thanks for updating the webrev, I will take a look.

Valerie


On 7/10/2018 10:18 AM, Martin Balao wrote:

Hi,

Webrev 04 for JDK-8029661 is ready:

 * 
http://cr.openjdk.java.net/~mbalao/webrevs/8029661/8029661.webrev.04.zip 

 * 
http://cr.openjdk.java.net/~mbalao/webrevs/8029661/8029661.webrev.04/ 



New:

 * Rebased to latest JDK revision (after TLS 1.3 merge)
  * Rev 1acfd2f56d72
 * ProtocolVersion dependencies removed (raw TLS protocol version 
numbers are now used)
 * Code style improvements (tabs, trailing whitespaces, max lines 
length, etc.)


Thanks,
Martin.-

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

[Xuelei Fan]

On 7/13/2018 12:13 PM, Adam Petcher wrote:

On 7/13/2018 1:35 PM, Xuelei Fan wrote:

I think we need to check more aspects, for both the session resumption 
producer and consumer:

1. the local is able to resume the session.
2. the peer is able to resume the session.
3. the change of the security parameters does not impact the resumption.


1 and 3 I agree with, and I believe are pretty well covered (after this 
change), but I'm still not convinced that the implementation should do 
any checking on behalf of the peer.
It is not necessary to be on behalf of the peer.  The local can check if 
the peer request something correctly.  We normally call it input validation.


This includes checking the peer 
supported signature algorithms. Maybe this is a good idea, but I think 
it is not in scope of this ticket, and it should be discussed 
separately. If you want to pursue this, feel free to open another ticket 
for it.


I would treat it as a part of the bug.  I'm fine if you just want to fix 
half of it.  I need a confirmation.  Please let me know if you want 
another half be addressed in a new bug.




So, for the protocol version checking in server side:
1. the client requested to use the negotiated version.
2. the server still supports the negotiated version.

I think #2 is get checked in line 406-414 block. 

Yes.


Did we check #1 in somewhere else?


A lot of the checks for the client side uses existing code, and is 
shared between TLS 1.3 and earlier versions. The way it works is that 
the shared code will decide which session to resume in ClientHello. At 
this point, a lot of things are checked, including the protocol version 
of the session. Only if this code decides that a session should be 
resumed, then the code for TLS 1.3+ in PreSharedKeyExtension will 
perform the additional checks that are specific to TLS 1.3 and then 
produce the extension.


Specifically, the protocol version is checked in ClientHello.java near 
line 458 (in existing code).


I meant to ask whether server side validate the client input.  See what 
we did for TLS v1.2 in line 973 of ClientHello.java.




For the cipher suite checking in server side:
1. the client requested to use the negotiate cipher suite.
2. the server still supports the negotiated cipher suite.

I think #2 is get checked in line 445-453 block. 


Yes.


Did we check #1 in somewhere else?


ClientHello.java near line 445 (in existing code).

I meant to ask whether server side validate the client input.  See what 
we did for TLS v1.2 in line 1003.





For the signature algorithms checking in server side:
1. the client requested to use the algorithms used in full handshake
2. the server still supports the algorithms used in full handshake

The algorithms used in full handshake is one element or a subset of 
the local and peer supported algorithms.   We may not cache the used 
algorithms in the session.  But it is acceptable to me to use 
defensive policy that we don't allow session resumption if the 
supported algorithms get changed (as you did in the webrev to use 
Collection.containsAll() line 435).


We do cache the certificate chain. If we wanted to, we could examine the 
chain and make sure all of the signature algorithms in the chain are 
still supported. I didn't think this was worth the effort, though.



I'm fine to use the defensive policy.  I don't purchase for a perfect check.



As reminded me that we may also want to check for both the 
signature_algorithms and signature_algorithms algorithms.


Did you mean signature_algorithms and signature_algorithms_cert? I think 
that is handled correctly now, because the set of (local) supported 
signature algorithms for certificates and handshake messages will always 
be the same (right?).

Right, the local algorithms checking should be sufficient.

We only have one field for this in 
HandshakeContext: localSupportedSignAlgs. The way it is used in the 
code, this field is used for both handshake messages and certificates.




I think #2 is get checked in line 431-442 block. 



It's for local algorithms.  We should valid the peer input as well.


Yes.


Did we check #1 in somewhere else?


Because this is specific to TLS 1.3+, I put it in the 
CHPreSharedKeyProducer. That is in PreSharedKeyExtension.java, near line 
610 (in the new code in the Webrev).


Yes, it is a good check the generation.  We also need to valid the peer 
input as well, in the consumer.


In the producer, it is call local supported signature algorithms; in the 
consumer, the corresponding one is peer support signature algorithms. 
Peer support signature algorithms should be validated for session 
resumption, I think.


Thanks,
Xuelei



On 7/13/2018 9:10 AM, Adam Petcher wrote:

On 7/13/2018 11:34 AM, Xuelei Fan wrote:


PreSharedKeyExtension.java
--
The local supported signature algorithms are checked in the 
canRejoin() method.  Should the peer supported signature algorithms 
be checked as well?


I don't 

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

[Adam Petcher]

On 7/13/2018 1:35 PM, Xuelei Fan wrote:

I think we need to check more aspects, for both the session resumption 
producer and consumer:

1. the local is able to resume the session.
2. the peer is able to resume the session.
3. the change of the security parameters does not impact the resumption.


1 and 3 I agree with, and I believe are pretty well covered (after this 
change), but I'm still not convinced that the implementation should do 
any checking on behalf of the peer. This includes checking the peer 
supported signature algorithms. Maybe this is a good idea, but I think 
it is not in scope of this ticket, and it should be discussed 
separately. If you want to pursue this, feel free to open another ticket 
for it.




So, for the protocol version checking in server side:
1. the client requested to use the negotiated version.
2. the server still supports the negotiated version.

I think #2 is get checked in line 406-414 block. 

Yes.


Did we check #1 in somewhere else?


A lot of the checks for the client side uses existing code, and is 
shared between TLS 1.3 and earlier versions. The way it works is that 
the shared code will decide which session to resume in ClientHello. At 
this point, a lot of things are checked, including the protocol version 
of the session. Only if this code decides that a session should be 
resumed, then the code for TLS 1.3+ in PreSharedKeyExtension will 
perform the additional checks that are specific to TLS 1.3 and then 
produce the extension.


Specifically, the protocol version is checked in ClientHello.java near 
line 458 (in existing code).




For the cipher suite checking in server side:
1. the client requested to use the negotiate cipher suite.
2. the server still supports the negotiated cipher suite.

I think #2 is get checked in line 445-453 block. 


Yes.


Did we check #1 in somewhere else?


ClientHello.java near line 445 (in existing code).




For the signature algorithms checking in server side:
1. the client requested to use the algorithms used in full handshake
2. the server still supports the algorithms used in full handshake

The algorithms used in full handshake is one element or a subset of 
the local and peer supported algorithms.   We may not cache the used 
algorithms in the session.  But it is acceptable to me to use 
defensive policy that we don't allow session resumption if the 
supported algorithms get changed (as you did in the webrev to use 
Collection.containsAll() line 435).


We do cache the certificate chain. If we wanted to, we could examine the 
chain and make sure all of the signature algorithms in the chain are 
still supported. I didn't think this was worth the effort, though.




As reminded me that we may also want to check for both the 
signature_algorithms and signature_algorithms algorithms.


Did you mean signature_algorithms and signature_algorithms_cert? I think 
that is handled correctly now, because the set of (local) supported 
signature algorithms for certificates and handshake messages will always 
be the same (right?). We only have one field for this in 
HandshakeContext: localSupportedSignAlgs. The way it is used in the 
code, this field is used for both handshake messages and certificates.




I think #2 is get checked in line 431-442 block. 


Yes.


Did we check #1 in somewhere else?


Because this is specific to TLS 1.3+, I put it in the 
CHPreSharedKeyProducer. That is in PreSharedKeyExtension.java, near line 
610 (in the new code in the Webrev).




On 7/13/2018 9:10 AM, Adam Petcher wrote:

On 7/13/2018 11:34 AM, Xuelei Fan wrote:


PreSharedKeyExtension.java
--
The local supported signature algorithms are checked in the 
canRejoin() method.  Should the peer supported signature algorithms 
be checked as well?


I don't think so. When the peer creates its PreSharedKeyExtension, it 
should only offer sessions (i.e. PSK identities) that it is willing 
to resume. This includes checking for its supported signature 
algorithms, or any checks that are required by its policy.
I see your point.  But the peer is not trustworthy so that we normally 
validate the input to make sure the peer is doing the right thing.


Thanks,
Xuelei

If the server gets a PSK identity from the client, then server should 
use that PSK to resume a session as long as it is acceptable 
according to the server's policy. Trying to figure out the peer's 
policy and enforce it is error prone and adds unnecessary complexity.


Though maybe I'm missing some other motivation to add this check.

Re: RFR[11] JDK-8206171: Signature#getParameters for RSASSA-PSS throws ProviderException when not initialized

[Valerie Peng]

Hmm, I like the idea of expanding null to cover both cases.
I will explore it more and see.
Thanks for the feedback,
Valerie

On 7/13/2018 6:56 AM, Sean Mullan wrote:

On 7/12/18 10:23 PM, Weijun Wang wrote:



On Jul 13, 2018, at 10:01 AM, Valerie Peng  
wrote:


Hi Max,

The javadoc is for Signature.getParameters(), so null can be 
returned for signature algorithms which do not use parameters, e.g. 
SHA256withDSA. As Signature class covers all signature algorithms, I 
am not sure about mentioning specific algorithm names as it may be 
lengthy and even misleading unless we list out all applicable 
algorithms.


Sure.



The part of "default and randomly generated" is inherited from 
existing javadoc. I think "default" in the aforementioned sentence 
means "hardcoded values". For example, something like salt length 
will likely have a fixed default value. Since we have no control 
over 3rd party providers, I think we may have to keep this for 
backward compatibility reason. For RSASSA-PSS sig algorithm, it 
errors out if the required parameter is not given. Thus, I added the 
sentence "If there are no provider-specific default values, the 
underlying signature implementation may also fail".



OK, now I understand "a combination of default and randomly 
generated" means some part of the parameter is hardcoded and some is 
random. Anyway, let's keep it unchanged.


Then, how about simply "If there are no provider-specific values" 
which covers both hardcoded and random values?


"the underlying signature implementation may also fail" is still not 
clear to me. If I had not read the CSR I would thought an exception 
will be thrown when update/sign is called.




As for @throws, I debated about it. The main reason for not adding 
one is consistency. Many (or should I say most) security classes do 
not have @throws for ProviderException. If we were to add @throws 
ProviderException here, we should do it across the board. Besides, 
it is recommended to NOT document runtime exceptions which callers 
are not prepared to handle.


I assume other getParameters() had not added it is because it 
happened they can return one.


While people does not catch runtime exceptions but my understanding 
is that if you mentioned "fail" in the spec maybe it's better to add 
a @throws for it.


For example, @throws SecurityException for File/Files operations.


Thinking more about this, I would be more inclined to recommend that 
you change the meaning of null as the return value to cover both cases:


@return the parameters used with this signature, or null if this 
signature does not use any parameters or does not support default or 
randomly generated parameter values


I don't think it is critical to make a distinction between these 2 
cases, because if the programmer doesn't initialize it with parameters 
it will get a SignatureException anyway when it tries to call sign or 
verify.


It's not perfect, but probably the best you can do working within the 
constraints of that API and minimizing compatibility risk.


--Sean



Thanks
Max



Thanks,
Valerie

On 7/10/2018 7:16 PM, Weijun Wang wrote:

Hi Valerie

About "it *may* return", do you mean it could also return null? My 
understanding is no.


Is it better to clarify when the implementation "may also fail"? 
From the CSR, it's this method. Can you add a @throws spec to this 
method then?


Also, I am a little confused by "default and randomly generated". 
Does this actually mean "default (might be randomly generated)"? 
The "it may" sentence mentions "default and randomly generated" but 
the "if there" only says "default", which sounds like the the 
"randomly generated" case could be different.


Thanks
Max


On Jul 11, 2018, at 5:12 AM, Valerie Peng 
 wrote:


Hi Brad,

Would you have time to review the fix for JDK-8206171: 
Signature#getParameters for RSASSA-PSS throws ProviderException 
when not initialized?
No source code changes, but just updating javadoc to mention the 
possible failure case.
Otherwise, JCK team expects a parameter object or null being 
returned.

I filed a CSR to track the javadoc clarification.

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

Thanks,
Valerie

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

[Xuelei Fan]

I think we need to check more aspects, for both the session resumption 
producer and consumer:

1. the local is able to resume the session.
2. the peer is able to resume the session.
3. the change of the security parameters does not impact the resumption.

So, for the protocol version checking in server side:
1. the client requested to use the negotiated version.
2. the server still supports the negotiated version.

I think #2 is get checked in line 406-414 block.  Did we check #1 in 
somewhere else?


For the cipher suite checking in server side:
1. the client requested to use the negotiate cipher suite.
2. the server still supports the negotiated cipher suite.

I think #2 is get checked in line 445-453 block.  Did we check #1 in 
somewhere else?



For the signature algorithms checking in server side:
1. the client requested to use the algorithms used in full handshake
2. the server still supports the algorithms used in full handshake

The algorithms used in full handshake is one element or a subset of the 
local and peer supported algorithms.   We may not cache the used 
algorithms in the session.  But it is acceptable to me to use defensive 
policy that we don't allow session resumption if the supported 
algorithms get changed (as you did in the webrev to use 
Collection.containsAll() line 435).


As reminded me that we may also want to check for both the 
signature_algorithms and signature_algorithms algorithms.


I think #2 is get checked in line 431-442 block.  Did we check #1 in 
somewhere else?


On 7/13/2018 9:10 AM, Adam Petcher wrote:

On 7/13/2018 11:34 AM, Xuelei Fan wrote:


PreSharedKeyExtension.java
--
The local supported signature algorithms are checked in the 
canRejoin() method.  Should the peer supported signature algorithms be 
checked as well?


I don't think so. When the peer creates its PreSharedKeyExtension, it 
should only offer sessions (i.e. PSK identities) that it is willing to 
resume. This includes checking for its supported signature algorithms, 
or any checks that are required by its policy.
I see your point.  But the peer is not trustworthy so that we normally 
validate the input to make sure the peer is doing the right thing.


Thanks,
Xuelei

If the server gets a PSK 
identity from the client, then server should use that PSK to resume a 
session as long as it is acceptable according to the server's policy. 
Trying to figure out the peer's policy and enforce it is error prone and 
adds unnecessary complexity.


Though maybe I'm missing some other motivation to add this check.

Re: SSLEngine weird behavior in 11+21?

[Simone Bordet]

Hi,

On Fri, Jul 13, 2018 at 3:45 PM Xuelei Fan  wrote:
> Thank you very much, Simone.  I find at least two improvements we can
> take.  It's really good!

Great!

Let know when they land in a 11+X release and we'll try them out.

Thanks!
-- 
Simone Bordet
---
Finally, no matter how good the architecture and design are,
to deliver bug-free software with optimal performance and reliability,
the implementation technique must be flawless.   Victoria Livschitz

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

[Adam Petcher]

On 7/13/2018 11:34 AM, Xuelei Fan wrote:


PreSharedKeyExtension.java
--
The local supported signature algorithms are checked in the 
canRejoin() method.  Should the peer supported signature algorithms be 
checked as well?


I don't think so. When the peer creates its PreSharedKeyExtension, it 
should only offer sessions (i.e. PSK identities) that it is willing to 
resume. This includes checking for its supported signature algorithms, 
or any checks that are required by its policy. If the server gets a PSK 
identity from the client, then server should use that PSK to resume a 
session as long as it is acceptable according to the server's policy. 
Trying to figure out the peer's policy and enforce it is error prone and 
adds unnecessary complexity.


Though maybe I'm missing some other motivation to add this check.

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

[Xuelei Fan]

PreSharedKeyExtension.java
--
The local supported signature algorithms are checked in the canRejoin() 
method.  Should the peer supported signature algorithms be checked as well?


Thanks,
Xuelei

On 7/13/2018 8:08 AM, Adam Petcher wrote:
Here's a new Webrev that includes this change: 
http://cr.openjdk.java.net/~apetcher/8206929/webrev.01/



On 7/12/2018 1:02 PM, Xuelei Fan wrote:
Set it in PostHandshakeContext should be fine as the session should 
have been negotiated.


Thanks,
Xuelei

On 7/12/2018 9:57 AM, Adam Petcher wrote:
This value needs to be set when we create a PostHandshakeContext from 
a TransportContext. This only happens after the handshake is 
complete, so we should always have a session (right?).


It may be better to set localSupportedSignAlgs in the constructor of 
PostHandshakeContext instead of where it is now. Would that address 
your concern?



On 7/12/2018 12:45 PM, Xuelei Fan wrote:

A quick question about the update in HandshakeContext.java.

+   this.localSupportedSignAlgs = new ArrayList(
+ conContext.conSession.getLocalSupportedSignatureSchemes());

Why set the value here?  The 'null' value of localSupportedSignAlgs 
has a special meaning that it has not been set.  A few places depend 
on this special value.  The above update may set it to empty if the 
session has not been established, and then prevent the proper 
setting of the value of it later.


Xuelei

On 7/12/2018 8:50 AM, Adam Petcher wrote:
This change adds some checks for session resumption in TLS 1.3 to 
ensure that the resumed session is compatible with what is 
requested. Specifically, I'm adding checks for protocol version, 
cipher suite, client authentication, and signature schemes. There 
are also some minor whitespace formatting changes in 
PreSharedKeyExtension.java.


This is a JDK 11 change, so please review soon.

Webrev: http://cr.openjdk.java.net/~apetcher/8206929/webrev.00/
JBS: https://bugs.openjdk.java.net/browse/JDK-8206929

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

[Adam Petcher]

Here's a new Webrev that includes this change: 
http://cr.openjdk.java.net/~apetcher/8206929/webrev.01/



On 7/12/2018 1:02 PM, Xuelei Fan wrote:
Set it in PostHandshakeContext should be fine as the session should 
have been negotiated.


Thanks,
Xuelei

On 7/12/2018 9:57 AM, Adam Petcher wrote:
This value needs to be set when we create a PostHandshakeContext from 
a TransportContext. This only happens after the handshake is 
complete, so we should always have a session (right?).


It may be better to set localSupportedSignAlgs in the constructor of 
PostHandshakeContext instead of where it is now. Would that address 
your concern?



On 7/12/2018 12:45 PM, Xuelei Fan wrote:

A quick question about the update in HandshakeContext.java.

+   this.localSupportedSignAlgs = new ArrayList(
+ conContext.conSession.getLocalSupportedSignatureSchemes());

Why set the value here?  The 'null' value of localSupportedSignAlgs 
has a special meaning that it has not been set.  A few places depend 
on this special value.  The above update may set it to empty if the 
session has not been established, and then prevent the proper 
setting of the value of it later.


Xuelei

On 7/12/2018 8:50 AM, Adam Petcher wrote:
This change adds some checks for session resumption in TLS 1.3 to 
ensure that the resumed session is compatible with what is 
requested. Specifically, I'm adding checks for protocol version, 
cipher suite, client authentication, and signature schemes. There 
are also some minor whitespace formatting changes in 
PreSharedKeyExtension.java.


This is a JDK 11 change, so please review soon.

Webrev: http://cr.openjdk.java.net/~apetcher/8206929/webrev.00/
JBS: https://bugs.openjdk.java.net/browse/JDK-8206929

Re: RFR[11] JDK-8206171: Signature#getParameters for RSASSA-PSS throws ProviderException when not initialized

[Sean Mullan]

On 7/12/18 10:23 PM, Weijun Wang wrote:




On Jul 13, 2018, at 10:01 AM, Valerie Peng  wrote:

Hi Max,

The javadoc is for Signature.getParameters(), so null can be returned for 
signature algorithms which do not use parameters, e.g. SHA256withDSA. As 
Signature class covers all signature algorithms, I am not sure about mentioning 
specific algorithm names as it may be lengthy and even misleading unless we 
list out all applicable algorithms.


Sure.



The part of "default and randomly generated" is inherited from existing javadoc. I think "default" 
in the aforementioned sentence means "hardcoded values". For example, something like salt length will likely 
have a fixed default value. Since we have no control over 3rd party providers, I think we may have to keep this for 
backward compatibility reason. For RSASSA-PSS sig algorithm, it errors out if the required parameter is not given. 
Thus, I added the sentence "If there are no provider-specific default values, the underlying signature 
implementation may also fail".



OK, now I understand "a combination of default and randomly generated" means 
some part of the parameter is hardcoded and some is random. Anyway, let's keep it 
unchanged.

Then, how about simply "If there are no provider-specific values" which covers 
both hardcoded and random values?

"the underlying signature implementation may also fail" is still not clear to 
me. If I had not read the CSR I would thought an exception will be thrown when 
update/sign is called.



As for @throws, I debated about it. The main reason for not adding one is 
consistency. Many (or should I say most) security classes do not have @throws 
for ProviderException. If we were to add @throws ProviderException here, we 
should do it across the board. Besides, it is recommended to NOT document 
runtime exceptions which callers are not prepared to handle.


I assume other getParameters() had not added it is because it happened they can 
return one.

While people does not catch runtime exceptions but my understanding is that if you 
mentioned "fail" in the spec maybe it's better to add a @throws for it.

For example, @throws SecurityException for File/Files operations.


Thinking more about this, I would be more inclined to recommend that you 
change the meaning of null as the return value to cover both cases:


@return the parameters used with this signature, or null if this 
signature does not use any parameters or does not support default or 
randomly generated parameter values


I don't think it is critical to make a distinction between these 2 
cases, because if the programmer doesn't initialize it with parameters 
it will get a SignatureException anyway when it tries to call sign or 
verify.


It's not perfect, but probably the best you can do working within the 
constraints of that API and minimizing compatibility risk.


--Sean



Thanks
Max



Thanks,
Valerie

On 7/10/2018 7:16 PM, Weijun Wang wrote:

Hi Valerie

About "it *may* return", do you mean it could also return null? My 
understanding is no.

Is it better to clarify when the implementation "may also fail"? From the CSR, 
it's this method. Can you add a @throws spec to this method then?

Also, I am a little confused by "default and randomly generated". Does this actually mean "default (might be randomly 
generated)"? The "it may" sentence mentions "default and randomly generated" but the "if there" only says 
"default", which sounds like the the "randomly generated" case could be different.

Thanks
Max



On Jul 11, 2018, at 5:12 AM, Valerie Peng  wrote:

Hi Brad,

Would you have time to review the fix for JDK-8206171: Signature#getParameters 
for RSASSA-PSS throws ProviderException when not initialized?
No source code changes, but just updating javadoc to mention the possible 
failure case.
Otherwise, JCK team expects a parameter object or null being returned.
I filed a CSR to track the javadoc clarification.

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

Thanks,
Valerie

Re: SSLEngine weird behavior in 11+21?

[Xuelei Fan]

On 7/12/2018 1:17 PM, Simone Bordet wrote:

Hi,

On Thu, Jul 12, 2018 at 9:29 PM Xuelei Fan  wrote:

Per the TLS 1.3 specification:
-  The server sends a dummy change_cipher_spec record immediately
   after its first handshake message.  This may either be after a
   ServerHello or a HelloRetryRequest.

and
-  If not offering early data, the client sends a dummy
   change_cipher_spec record (see the third paragraph of Section 5.1)
   immediately before its second flight.  This may either be before
   its second ClientHello or before its encrypted handshake flight.
   If offering early data, the record is placed immediately after the
   first ClientHello.

My read of the spec is that the dummy change_cipher_spec record is
generated immediately after the ServerHello in server side, and before
the 2nd flight in client side.


The spec is about *sending*, while I'm about *generating*.

Currently:
- Server wraps 160, 6 and 907 in 3 wraps.
- Server *sends* the 1073 bytes in 1 TCP write
- Client unwraps 160, then goes into NEED_WRAP
- Client wraps 6, then goes again into NEED_UNWRAP to finish with the
6 and 907 received by the server.

What I'm suggesting:
- Server wraps 160, 6 and 907 in 3 wraps.
- Server *sends* the 1073 (160+6+907) bytes in 1 TCP write
- Client unwraps 160, stays in NEED_UNWRAP, unwraps the 6, unwraps the
907, then goes into NEED_WRAP
- Client wraps 6 and 58
- Client *sends* the 64 (6+58) bytes in 1 TCP write.

The 6 bytes of the dummy change_cipher_spec are *sent* just after
receiving the ServerHello in both cases, they are just *generated* at
different times.

By having all the unwraps() consecutive and all the wraps()
consecutive you can enable a number of optimizations I described
earlier.

Imagine a client that would perform a TCP *send* every time the state
moves away from NEED_WRAP.
Currently it would:
1. TCP send for ClientHello
2. Reads 1073 from server
3. Unwrap 160, then NEED_WRAP
4. Generate 6, then NEED_UNWRAP
5. TCP send the 6 bytes
6. Unwrap 6 and 907, then NEED_WRAP
7. Generate 58 then FINISHED
8 TCP send the 58 bytes.

You can see that the client issues 3 TCP sends.

With what I am suggesting, the client would only issue 2 TCP sends,
which seems more in line with the efforts in TLS 1.3 to be more
efficient.

It would just be a matter of *generating* those 6 bytes a bit later,
*after* having fully unwrapped the ServerHello.


It's a good idea!


We may consider a option to turn off the middlebox compatibility mode if
it helps Jetty.


No need to.


But the implementation code and the application should
be ready to accept the fact that third party's implementation may be
implemented in middlebox compatibility mode, and the change_cipher_spec
record may still come in.


See above, it's not about processing the dummy change_cipher_spec,
it's just about generating it a little later.


After #7 (FINISHED), if the client only send application data, but never
call read again, it still works, right?  The application don't have to
read something, I think.


Sure, it just can't take advantage of session resumption though.
The client never knows that there are post-handshake messages to read
because SSLEgine did not tell.
SSLEngine just said FINISHED and NOT_HANDSHAKING.

However, I see your point. Handshake messages could come at any time,
just that this one seems more part of the initial handshake,
especially if it's about session resumption.

Yes.  Applications using SSLEngine may want to monitor the network 
incoming, and call unwrap() when there is something comes.  Otherwise, 
if the SSLEngine client does not try to read any more, there is no 
chance to get the post-handshake message.


I know some application might never read again after the full handshake, 
but it may not be able to work for some user cases (session resumption, 
etc), even in the TLS 1.2 context.  As SSLEngine knows nothing about the 
underlying transportation, there is not much it can do here.



in #10, you said, "Client MUST unwrap ...".  Do you mean that the client
cannot send application data without read/unwrap something?


It can send data, but won't be good to leave unprocessed bytes around.
The application would not know that there is stuff to read from the
network and to process as post-handshake messages.
It may never read from the network again.


See above.

Thank you very much, Simone.  I find at least two improvements we can 
take.  It's really good!


Regards,
Xuelei


Half-close means that the server may not send the close_notify when it
receive the client close_notify.  It's a very tricky policy.  The client
close_notify only means the client want to close its writing side.  The
server may not send the close_notify if it doesn't want to close.  If we
have the client goes into NEED_UNWRAP, there is a potential dead waiting.


Good point.

Thanks!

Re: RFR[12] JDK-8206443: Update security libs manual test to cope with removal of javac -source/-target 6

[Xuelei Fan]

Looks fine to me.

Thanks,
Xuelei

On 7/12/2018 10:39 PM, sha.ji...@oracle.com wrote:

Hi,
JDK-8028563 has removed javac support for 6/1.6 source and target, so 
the following tests has to be updated accordingly.

javax/net/ssl/compatibility/Compatibility.java
sun/security/tools/jarsigner/compatibility/Compatibility.java

In addition, this fix also makes 
javax/net/ssl/compatibility/Compatibility.java can work with JDK 12 builds.


Webrev: http://cr.openjdk.java.net/~jjiang/8206443/webrev.00/
JBS: https://bugs.openjdk.java.net/browse/JDK-8206443

Best regards,
John Jiang

Re: RFR: 8199779: Add T-Systems, GlobalSign and Starfield services root certificates

[Sean Mullan]

Looks good.

--Sean

On 7/12/18 2:51 PM, Rajan Halade wrote:
Please review this fix to add T-systems, GlobalSign, and Starfield 
services root certificates to cacerts.


Webrev: http://cr.openjdk.java.net/~rhalade/8199779/webrev.00/

Thanks,
Rajan