Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
Approved. Brad On 10/16/2012 6:30 PM, Xuelei Fan wrote: On 10/17/2012 9:30 AM, Xuelei Fan wrote: On 10/17/2012 4:57 AM, Brad Wetmore wrote: You could push both together and put everything in just one changeset, but of course up to you. Good. Please review the CCC request, I will fast-track the request. http://ccc.us.oracle.com/8000954 I don't think there's anything else for me to do, let me know if you're waiting on something. I think you are done, ;-) except the above CCC review. I still need to wait for the approval of the CCCs. Thanks, Xuelei
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
On 10/17/2012 9:30 AM, Xuelei Fan wrote: > On 10/17/2012 4:57 AM, Brad Wetmore wrote: >> You could push both together and put everything in just one changeset, >> but of course up to you. >> > Good. > > Please review the CCC request, I will fast-track the request. > http://ccc.us.oracle.com/8000954 >> I don't think there's anything else for me to do, let me know if you're >> waiting on something. > I think you are done, ;-) except the above CCC review. I still need to > wait for the approval of the CCCs. > > Thanks, > Xuelei >
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
On 10/17/2012 4:57 AM, Brad Wetmore wrote: > You could push both together and put everything in just one changeset, > but of course up to you. > Good. Please review the CCC request, I will fast-track the request. > I don't think there's anything else for me to do, let me know if you're > waiting on something. I think you are done, ;-) except the above CCC review. I still need to wait for the approval of the CCCs. Thanks, Xuelei
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
BTW, both 7068321 and 8000954 are in Open, should be "In Progress".
On 10/15/2012 6:27 PM, Xuelei Fan wrote:
On 10/16/2012 7:12 AM, Brad Wetmore wrote:
Looks good.
The main update is to add "final" keyword to the new methods in
SSLParamaters. I prefer to use the final because I don't see a
requirement to override them.
Let me know when the new CCC is ready, I'll approve it.
The CCC for this fix, 7068321 has been finalized. I submitted a new CCC
to add the final keyword:
https://jbs.oracle.com/bugs/browse/JDK-8000954
I will fill the new CCC after the push of 7068321.
Are you going to contact IETF?
Yes, I will.
Almost there.
I think we get all spec review, code review done.
I think so.
> I will push the
changeset after the CCC get approved, and pay more time on CPU bugs.
You could push both together and put everything in just one changeset,
but of course up to you.
I don't think there's anything else for me to do, let me know if you're
waiting on something.
Brad
Thanks,
Xuelei
Brad
On 10/13/2012 6:26 AM, Xuelei Fan wrote:
New webrev: http://cr.openjdk.java.net./~xuelei/7068321/webrev.14/
The main update is to add "final" keyword to the new methods in
SSLParamaters. I prefer to use the final because I don't see a
requirement to override them. But I can go with the version with or
without the "final" keyword. I think you properly don't need to review
other parts of the webrev any more, no other major update.
Thanks,
Xuelei
On 10/13/2012 10:17 AM, Xuelei Fan wrote:
I have no access to office network, just a quick reply. I want add
final keyword for the new methods in SSLParameters. See below.
Sent from my iPad
On Oct 13, 2012, at 8:00 AM, Brad Wetmore
wrote:
I guess you didn't need to have me as reviewer before going final
with
the CCC?
The CCC has been finalized. ;-) I though you have done with spec
review.
Anyway, we still can make updates on specification within new bugs.
That's fine, I just wasn't sure if you needed someone to "approve"
the "final" version of the spec to move it to the next state.
Just a comment:
459/468: Currently you have serverNames and sniMatchers as package
private variables, so the ClientHandshaker/ServerHandshaker *could*
inherit these variables rather than have a separate get methods.
Or you
could make them private and keep the get calls.
Good catch!
I can look at your latest if you like, but probably not necessary.
I will send the new webrev tonight my time.
I'm just not seeing why this implies that it requires *EVERY* name
must
match. This just says we can do one of two things upon receipt of an
unrecognized hostname: continue on, or alert/close. We can be very
restrictive (ALL/AND) or less so (at least one/OR), and still be
within
the RFC, I think.
I agree with your parser.
In our spec, it is required that once a SNIMatcher is defined, it
will be used to recognized the server name in the SNI extension.
Where? We do say in SNIMatcher:
Servers can use Server Name Indication (SNI) information to
decide if
specific {@link SSLSocket} or {@link SSLEngine} instances should
accept a connection.
But I don't think we have ever said that it MUST match or must
match all
or even what the implementation must do if there is a match
failure. Nor
should we specify that in the API, IMHO. That's implementation
behavior.
I may have different options on this point. I think, it must be a
specified behavior in Java. Otherwise, it is very confusing about
how to
use 1+ server names in server side.
I am going suggest we ask for guidance from IETF about this. It is
not clear from RFC 6066 how to handle multiple SNI types, even
reading very generously between the lines. See below.
Let's start from the logic of the design. If the specification is not
clear, I will rewrite the spec according to our agreement.
Let's start from the requirement. I think once a SNIMatcher is defined
in SSL parameters, it MUST be used to perform the match operations if
the corresponding server name appears in the SNI extension.
Otherwise,
what will happens? See the example:
1. SNI extension contains HostName, "www.example.com".
2. Server side defines SNIMatcher for HostName, to accept
"www.example.org". Server does not accept "www.example.com".
3. What's the result of the handshake? Is the SNI extension is
accetable?
www.example.org != www.example.com. I would say fail handshake with
unknown_hostname.
If we do not define the spec about how to use SNIMatcher. The
answer to
#3 is unclear. Because if a SNIMatcher may be used to perform match
operation, and may be not used to perform match operation, then the
server may accept the SNI extension, may ignore the SNI extension, may
reject the SNI extension. It is useless to define SNIMatcher.
I think the requirement is clear that it is a must to use
SNIMatcher to
perform the match operation if the corresponding server name
appears in
the SNI extension.
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
On 10/16/2012 7:12 AM, Brad Wetmore wrote:
> Looks good.
>
>> The main update is to add "final" keyword to the new methods in
>> SSLParamaters. I prefer to use the final because I don't see a
>> requirement to override them.
>
> Let me know when the new CCC is ready, I'll approve it.
>
The CCC for this fix, 7068321 has been finalized. I submitted a new CCC
to add the final keyword:
https://jbs.oracle.com/bugs/browse/JDK-8000954
I will fill the new CCC after the push of 7068321.
> Are you going to contact IETF?
>
Yes, I will.
> Almost there.
>
I think we get all spec review, code review done. I will push the
changeset after the CCC get approved, and pay more time on CPU bugs.
Thanks,
Xuelei
> Brad
>
>
> On 10/13/2012 6:26 AM, Xuelei Fan wrote:
>> New webrev: http://cr.openjdk.java.net./~xuelei/7068321/webrev.14/
>>
>> The main update is to add "final" keyword to the new methods in
>> SSLParamaters. I prefer to use the final because I don't see a
>> requirement to override them. But I can go with the version with or
>> without the "final" keyword. I think you properly don't need to review
>> other parts of the webrev any more, no other major update.
>>
>> Thanks,
>> Xuelei
>>
>>
>> On 10/13/2012 10:17 AM, Xuelei Fan wrote:
>>> I have no access to office network, just a quick reply. I want add
>>> final keyword for the new methods in SSLParameters. See below.
>>>
>>> Sent from my iPad
>>>
>>> On Oct 13, 2012, at 8:00 AM, Brad Wetmore
>>> wrote:
>>>
>> I guess you didn't need to have me as reviewer before going final
>> with
>> the CCC?
> The CCC has been finalized. ;-) I though you have done with spec
> review.
> Anyway, we still can make updates on specification within new bugs.
That's fine, I just wasn't sure if you needed someone to "approve"
the "final" version of the spec to move it to the next state.
>> Just a comment:
>>
>> 459/468: Currently you have serverNames and sniMatchers as package
>> private variables, so the ClientHandshaker/ServerHandshaker *could*
>> inherit these variables rather than have a separate get methods.
>> Or you
>> could make them private and keep the get calls.
> Good catch!
I can look at your latest if you like, but probably not necessary.
>>> I will send the new webrev tonight my time.
>>>
>> I'm just not seeing why this implies that it requires *EVERY* name
>> must
>> match. This just says we can do one of two things upon receipt of an
>> unrecognized hostname: continue on, or alert/close. We can be very
>> restrictive (ALL/AND) or less so (at least one/OR), and still be
>> within
>> the RFC, I think.
> I agree with your parser.
>
>>> In our spec, it is required that once a SNIMatcher is defined, it
>>> will be used to recognized the server name in the SNI extension.
>>
>> Where? We do say in SNIMatcher:
>>
>> Servers can use Server Name Indication (SNI) information to
>> decide if
>> specific {@link SSLSocket} or {@link SSLEngine} instances should
>> accept a connection.
>>
>> But I don't think we have ever said that it MUST match or must
>> match all
>> or even what the implementation must do if there is a match
>> failure. Nor
>> should we specify that in the API, IMHO. That's implementation
>> behavior.
> I may have different options on this point. I think, it must be a
> specified behavior in Java. Otherwise, it is very confusing about
> how to
> use 1+ server names in server side.
I am going suggest we ask for guidance from IETF about this. It is
not clear from RFC 6066 how to handle multiple SNI types, even
reading very generously between the lines. See below.
> Let's start from the logic of the design. If the specification is not
> clear, I will rewrite the spec according to our agreement.
>
> Let's start from the requirement. I think once a SNIMatcher is defined
> in SSL parameters, it MUST be used to perform the match operations if
> the corresponding server name appears in the SNI extension.
> Otherwise,
> what will happens? See the example:
> 1. SNI extension contains HostName, "www.example.com".
> 2. Server side defines SNIMatcher for HostName, to accept
> "www.example.org". Server does not accept "www.example.com".
> 3. What's the result of the handshake? Is the SNI extension is
> accetable?
www.example.org != www.example.com. I would say fail handshake with
unknown_hostname.
> If we do not define the spec about how to use SNIMatcher. The
> answer to
> #3 is unclear. Because if a SNIMatcher may be used to perform match
> operation, and may be not used to perform match operation, then the
> server may accept the SNI extension, may ignore the SNI extension, may
> reject the SNI
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
Looks good.
> The main update is to add "final" keyword to the new methods in
> SSLParamaters. I prefer to use the final because I don't see a
> requirement to override them.
Let me know when the new CCC is ready, I'll approve it.
Are you going to contact IETF?
Almost there.
Brad
On 10/13/2012 6:26 AM, Xuelei Fan wrote:
New webrev: http://cr.openjdk.java.net./~xuelei/7068321/webrev.14/
The main update is to add "final" keyword to the new methods in
SSLParamaters. I prefer to use the final because I don't see a
requirement to override them. But I can go with the version with or
without the "final" keyword. I think you properly don't need to review
other parts of the webrev any more, no other major update.
Thanks,
Xuelei
On 10/13/2012 10:17 AM, Xuelei Fan wrote:
I have no access to office network, just a quick reply. I want add final
keyword for the new methods in SSLParameters. See below.
Sent from my iPad
On Oct 13, 2012, at 8:00 AM, Brad Wetmore wrote:
I guess you didn't need to have me as reviewer before going final with
the CCC?
The CCC has been finalized. ;-) I though you have done with spec review.
Anyway, we still can make updates on specification within new bugs.
That's fine, I just wasn't sure if you needed someone to "approve" the "final"
version of the spec to move it to the next state.
Just a comment:
459/468: Currently you have serverNames and sniMatchers as package
private variables, so the ClientHandshaker/ServerHandshaker *could*
inherit these variables rather than have a separate get methods. Or you
could make them private and keep the get calls.
Good catch!
I can look at your latest if you like, but probably not necessary.
I will send the new webrev tonight my time.
I'm just not seeing why this implies that it requires *EVERY* name must
match. This just says we can do one of two things upon receipt of an
unrecognized hostname: continue on, or alert/close. We can be very
restrictive (ALL/AND) or less so (at least one/OR), and still be within
the RFC, I think.
I agree with your parser.
In our spec, it is required that once a SNIMatcher is defined, it
will be used to recognized the server name in the SNI extension.
Where? We do say in SNIMatcher:
Servers can use Server Name Indication (SNI) information to decide if
specific {@link SSLSocket} or {@link SSLEngine} instances should
accept a connection.
But I don't think we have ever said that it MUST match or must match all
or even what the implementation must do if there is a match failure. Nor
should we specify that in the API, IMHO. That's implementation behavior.
I may have different options on this point. I think, it must be a
specified behavior in Java. Otherwise, it is very confusing about how to
use 1+ server names in server side.
I am going suggest we ask for guidance from IETF about this. It is not clear
from RFC 6066 how to handle multiple SNI types, even reading very generously
between the lines. See below.
Let's start from the logic of the design. If the specification is not
clear, I will rewrite the spec according to our agreement.
Let's start from the requirement. I think once a SNIMatcher is defined
in SSL parameters, it MUST be used to perform the match operations if
the corresponding server name appears in the SNI extension. Otherwise,
what will happens? See the example:
1. SNI extension contains HostName, "www.example.com".
2. Server side defines SNIMatcher for HostName, to accept
"www.example.org". Server does not accept "www.example.com".
3. What's the result of the handshake? Is the SNI extension is accetable?
www.example.org != www.example.com. I would say fail handshake with
unknown_hostname.
If we do not define the spec about how to use SNIMatcher. The answer to
#3 is unclear. Because if a SNIMatcher may be used to perform match
operation, and may be not used to perform match operation, then the
server may accept the SNI extension, may ignore the SNI extension, may
reject the SNI extension. It is useless to define SNIMatcher.
I think the requirement is clear that it is a must to use SNIMatcher to
perform the match operation if the corresponding server name appears in
the SNI extension. I think it is true for the case when there is only
one server name type, at least.
I completely agree.
Let's look more about what happens when there is 1+ server name types in
the SNI extension. Let's use the example in your previous mail.
SNIHostname: "example.com"
SNINickName: "www.example.com"
SNIMatcher: "example.com"
SNINickNameMatcher: "www1.example.com
Suppose that the server is able to understand both HostName and
NickName. In the above example, server is able to recognize HostName,
but not NickName. Then should the server includes an extension of type
"server_name" in the (extended) server hello?
If the server_name extension is included, it is unclear for client side
which one is
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
A server name indication extension will be sent whenever the name is > recognizable by the matches. I did not check for the types of cipher suites. I think it is the proper approach because although for anonymous cipher cuties, there is not certificates, but the ssl context may be different, so it is still can be regarded as the server do something > different related to the specified SNI. True! According to effective java, Item 15? It doesn't specifically talk about final methods, but I think I see where you are coming from since this class was originally non-final. > I would prefer to use final keyword for > the new methods. I did not see clear requirements that customers > need to override the methods. So I would like a stricter restriction > for the new methods in case of any mis-use. Does it make sense to you? Yes. I can't really think of why not, other than for potential extendability. Brad
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
New webrev: http://cr.openjdk.java.net./~xuelei/7068321/webrev.14/
The main update is to add "final" keyword to the new methods in
SSLParamaters. I prefer to use the final because I don't see a
requirement to override them. But I can go with the version with or
without the "final" keyword. I think you properly don't need to review
other parts of the webrev any more, no other major update.
Thanks,
Xuelei
On 10/13/2012 10:17 AM, Xuelei Fan wrote:
> I have no access to office network, just a quick reply. I want add final
> keyword for the new methods in SSLParameters. See below.
>
> Sent from my iPad
>
> On Oct 13, 2012, at 8:00 AM, Brad Wetmore wrote:
>
>>
I guess you didn't need to have me as reviewer before going final with
the CCC?
>>> The CCC has been finalized. ;-) I though you have done with spec review.
>>> Anyway, we still can make updates on specification within new bugs.
>>
>> That's fine, I just wasn't sure if you needed someone to "approve" the
>> "final" version of the spec to move it to the next state.
>>
Just a comment:
459/468: Currently you have serverNames and sniMatchers as package
private variables, so the ClientHandshaker/ServerHandshaker *could*
inherit these variables rather than have a separate get methods. Or you
could make them private and keep the get calls.
>>> Good catch!
>>
>> I can look at your latest if you like, but probably not necessary.
>>
> I will send the new webrev tonight my time.
>
I'm just not seeing why this implies that it requires *EVERY* name must
match. This just says we can do one of two things upon receipt of an
unrecognized hostname: continue on, or alert/close. We can be very
restrictive (ALL/AND) or less so (at least one/OR), and still be within
the RFC, I think.
>>> I agree with your parser.
>>>
> In our spec, it is required that once a SNIMatcher is defined, it
> will be used to recognized the server name in the SNI extension.
Where? We do say in SNIMatcher:
Servers can use Server Name Indication (SNI) information to decide if
specific {@link SSLSocket} or {@link SSLEngine} instances should
accept a connection.
But I don't think we have ever said that it MUST match or must match all
or even what the implementation must do if there is a match failure. Nor
should we specify that in the API, IMHO. That's implementation behavior.
>>> I may have different options on this point. I think, it must be a
>>> specified behavior in Java. Otherwise, it is very confusing about how to
>>> use 1+ server names in server side.
>>
>> I am going suggest we ask for guidance from IETF about this. It is not
>> clear from RFC 6066 how to handle multiple SNI types, even reading very
>> generously between the lines. See below.
>>
>>> Let's start from the logic of the design. If the specification is not
>>> clear, I will rewrite the spec according to our agreement.
>>>
>>> Let's start from the requirement. I think once a SNIMatcher is defined
>>> in SSL parameters, it MUST be used to perform the match operations if
>>> the corresponding server name appears in the SNI extension. Otherwise,
>>> what will happens? See the example:
>>>1. SNI extension contains HostName, "www.example.com".
>>>2. Server side defines SNIMatcher for HostName, to accept
>>> "www.example.org". Server does not accept "www.example.com".
>>>3. What's the result of the handshake? Is the SNI extension is accetable?
>>
>> www.example.org != www.example.com. I would say fail handshake with
>> unknown_hostname.
>>
>>> If we do not define the spec about how to use SNIMatcher. The answer to
>>> #3 is unclear. Because if a SNIMatcher may be used to perform match
>>> operation, and may be not used to perform match operation, then the
>>> server may accept the SNI extension, may ignore the SNI extension, may
>>> reject the SNI extension. It is useless to define SNIMatcher.
>>>
>>> I think the requirement is clear that it is a must to use SNIMatcher to
>>> perform the match operation if the corresponding server name appears in
>>> the SNI extension. I think it is true for the case when there is only
>>> one server name type, at least.
>>
>> I completely agree.
>>
>>> Let's look more about what happens when there is 1+ server name types in
>>> the SNI extension. Let's use the example in your previous mail.
>>>
>>>SNIHostname: "example.com"
>>>SNINickName: "www.example.com"
>>>
>>>SNIMatcher: "example.com"
>>>SNINickNameMatcher: "www1.example.com
>>>
>>> Suppose that the server is able to understand both HostName and
>>> NickName. In the above example, server is able to recognize HostName,
>>> but not NickName. Then should the server includes an extension of type
>>> "server_name" in the (extended) server hello?
>>>
>>> If the server_name extension is included, it is unclear for client side
>>> which one is
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
I have no access to office network, just a quick reply. I want add final
keyword for the new methods in SSLParameters. See below.
Sent from my iPad
On Oct 13, 2012, at 8:00 AM, Brad Wetmore wrote:
>
>>> I guess you didn't need to have me as reviewer before going final with
>>> the CCC?
>> The CCC has been finalized. ;-) I though you have done with spec review.
>> Anyway, we still can make updates on specification within new bugs.
>
> That's fine, I just wasn't sure if you needed someone to "approve" the
> "final" version of the spec to move it to the next state.
>
>>> Just a comment:
>>>
>>> 459/468: Currently you have serverNames and sniMatchers as package
>>> private variables, so the ClientHandshaker/ServerHandshaker *could*
>>> inherit these variables rather than have a separate get methods. Or you
>>> could make them private and keep the get calls.
>> Good catch!
>
> I can look at your latest if you like, but probably not necessary.
>
I will send the new webrev tonight my time.
>>> I'm just not seeing why this implies that it requires *EVERY* name must
>>> match. This just says we can do one of two things upon receipt of an
>>> unrecognized hostname: continue on, or alert/close. We can be very
>>> restrictive (ALL/AND) or less so (at least one/OR), and still be within
>>> the RFC, I think.
>> I agree with your parser.
>>
In our spec, it is required that once a SNIMatcher is defined, it
will be used to recognized the server name in the SNI extension.
>>>
>>> Where? We do say in SNIMatcher:
>>>
>>>Servers can use Server Name Indication (SNI) information to decide if
>>>specific {@link SSLSocket} or {@link SSLEngine} instances should
>>>accept a connection.
>>>
>>> But I don't think we have ever said that it MUST match or must match all
>>> or even what the implementation must do if there is a match failure. Nor
>>> should we specify that in the API, IMHO. That's implementation behavior.
>> I may have different options on this point. I think, it must be a
>> specified behavior in Java. Otherwise, it is very confusing about how to
>> use 1+ server names in server side.
>
> I am going suggest we ask for guidance from IETF about this. It is not clear
> from RFC 6066 how to handle multiple SNI types, even reading very generously
> between the lines. See below.
>
>> Let's start from the logic of the design. If the specification is not
>> clear, I will rewrite the spec according to our agreement.
>>
>> Let's start from the requirement. I think once a SNIMatcher is defined
>> in SSL parameters, it MUST be used to perform the match operations if
>> the corresponding server name appears in the SNI extension. Otherwise,
>> what will happens? See the example:
>>1. SNI extension contains HostName, "www.example.com".
>>2. Server side defines SNIMatcher for HostName, to accept
>> "www.example.org". Server does not accept "www.example.com".
>>3. What's the result of the handshake? Is the SNI extension is accetable?
>
> www.example.org != www.example.com. I would say fail handshake with
> unknown_hostname.
>
>> If we do not define the spec about how to use SNIMatcher. The answer to
>> #3 is unclear. Because if a SNIMatcher may be used to perform match
>> operation, and may be not used to perform match operation, then the
>> server may accept the SNI extension, may ignore the SNI extension, may
>> reject the SNI extension. It is useless to define SNIMatcher.
>>
>> I think the requirement is clear that it is a must to use SNIMatcher to
>> perform the match operation if the corresponding server name appears in
>> the SNI extension. I think it is true for the case when there is only
>> one server name type, at least.
>
> I completely agree.
>
>> Let's look more about what happens when there is 1+ server name types in
>> the SNI extension. Let's use the example in your previous mail.
>>
>>SNIHostname: "example.com"
>>SNINickName: "www.example.com"
>>
>>SNIMatcher: "example.com"
>>SNINickNameMatcher: "www1.example.com
>>
>> Suppose that the server is able to understand both HostName and
>> NickName. In the above example, server is able to recognize HostName,
>> but not NickName. Then should the server includes an extension of type
>> "server_name" in the (extended) server hello?
>>
>> If the server_name extension is included, it is unclear for client side
>> which one is accepted.
>
> The presence of a server's server_name extension indicates that an SNI value
> was used to guide the selection of an appropriate cert. It seems ambiguous
> in RFC 6066 as to whether this extension indicated "ALL" or "AT LEAST ONE"
> SNI value guided the selection. I might lean towards ALL since RFC 6066 says
> "The 'extension_data' field of this extension SHALL be empty". If it AT
> LEAST ONE were meant, there is no way of indicating *which* extension was
> used, or if multiple ones were used.
>
> As an aside, w
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
I guess you didn't need to have me as reviewer before going final with
the CCC?
The CCC has been finalized. ;-) I though you have done with spec review.
Anyway, we still can make updates on specification within new bugs.
That's fine, I just wasn't sure if you needed someone to "approve" the
"final" version of the spec to move it to the next state.
Just a comment:
459/468: Currently you have serverNames and sniMatchers as package
private variables, so the ClientHandshaker/ServerHandshaker *could*
inherit these variables rather than have a separate get methods. Or you
could make them private and keep the get calls.
Good catch!
I can look at your latest if you like, but probably not necessary.
I'm just not seeing why this implies that it requires *EVERY* name must
match. This just says we can do one of two things upon receipt of an
unrecognized hostname: continue on, or alert/close. We can be very
restrictive (ALL/AND) or less so (at least one/OR), and still be within
the RFC, I think.
I agree with your parser.
In our spec, it is required that once a SNIMatcher is defined, it
will be used to recognized the server name in the SNI extension.
Where? We do say in SNIMatcher:
Servers can use Server Name Indication (SNI) information to decide if
specific {@link SSLSocket} or {@link SSLEngine} instances should
accept a connection.
But I don't think we have ever said that it MUST match or must match all
or even what the implementation must do if there is a match failure. Nor
should we specify that in the API, IMHO. That's implementation behavior.
I may have different options on this point. I think, it must be a
specified behavior in Java. Otherwise, it is very confusing about how to
use 1+ server names in server side.
I am going suggest we ask for guidance from IETF about this. It is not
clear from RFC 6066 how to handle multiple SNI types, even reading very
generously between the lines. See below.
Let's start from the logic of the design. If the specification is not
clear, I will rewrite the spec according to our agreement.
Let's start from the requirement. I think once a SNIMatcher is defined
in SSL parameters, it MUST be used to perform the match operations if
the corresponding server name appears in the SNI extension. Otherwise,
what will happens? See the example:
1. SNI extension contains HostName, "www.example.com".
2. Server side defines SNIMatcher for HostName, to accept
"www.example.org". Server does not accept "www.example.com".
3. What's the result of the handshake? Is the SNI extension is accetable?
www.example.org != www.example.com. I would say fail handshake with
unknown_hostname.
If we do not define the spec about how to use SNIMatcher. The answer to
#3 is unclear. Because if a SNIMatcher may be used to perform match
operation, and may be not used to perform match operation, then the
server may accept the SNI extension, may ignore the SNI extension, may
reject the SNI extension. It is useless to define SNIMatcher.
I think the requirement is clear that it is a must to use SNIMatcher to
perform the match operation if the corresponding server name appears in
the SNI extension. I think it is true for the case when there is only
one server name type, at least.
I completely agree.
Let's look more about what happens when there is 1+ server name types in
the SNI extension. Let's use the example in your previous mail.
SNIHostname: "example.com"
SNINickName: "www.example.com"
SNIMatcher: "example.com"
SNINickNameMatcher: "www1.example.com
Suppose that the server is able to understand both HostName and
NickName. In the above example, server is able to recognize HostName,
but not NickName. Then should the server includes an extension of type
"server_name" in the (extended) server hello?
If the server_name extension is included, it is unclear for client side
which one is accepted.
The presence of a server's server_name extension indicates that an SNI
value was used to guide the selection of an appropriate cert. It seems
ambiguous in RFC 6066 as to whether this extension indicated "ALL" or
"AT LEAST ONE" SNI value guided the selection. I might lean towards ALL
since RFC 6066 says "The 'extension_data' field of this extension SHALL
be empty". If it AT LEAST ONE were meant, there is no way of indicating
*which* extension was used, or if multiple ones were used.
As an aside, we have no API for the KeyManagers's to indicate whether
they actually used a SNI extension, so I think you are currently sending
a server server_name extension if any cert was selected. I don't think
this is a major problem given our current APIs.
Because the client side may need to use the
server_name to do endpoint identification, it is not safe to use
"www.example.com" to perform endpoint identification because the server
side does not accept it.
If the server_name extension is not include
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
On 10/11/2012 12:21 PM, Bradford Wetmore wrote: > > > On 10/10/2012 7:54 PM, Xuelei Fan wrote: >> No new webrev. I need a review of how to use SNIMatcher. See bellow >> inline comments. >> >> On 10/11/2012 7:38 AM, Brad Wetmore wrote: >>> >>> >>> On 10/10/2012 5:47 AM, Xuelei Fan wrote: new webrev: http://cr.openjdk.java.net./~xuelei/7068321/webrev.13/ >>> >>> I guess you didn't need to have me as reviewer before going final with >>> the CCC? >>> >> The CCC has been finalized. ;-) I though you have done with spec review. >> Anyway, we still can make updates on specification within new bugs. >> > javax/net/ssl/SSLSocketFactory.java > === > Change look good. > > 216: I think you need a period at end of sentence here. >>> >>> You got the others, but missed this one. >>> >> I think we discussed the style sometimes ago. If the sentence does not >> start with a capital letter, then it does not need a period at the end >> of the sentence. I can see both style in other specs. >> >> Updated to add the period. > > Sorry, I meant to say copyright change. Boy, did I goof on that one. > One of the copyright dates wasn't right, but the rest were. Oops, right, I missed the copyright dates. I double checked the copyright yesterday, but still missed this one. ;-) Xuelei
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
On 10/10/2012 7:54 PM, Xuelei Fan wrote:
No new webrev. I need a review of how to use SNIMatcher. See bellow
inline comments.
On 10/11/2012 7:38 AM, Brad Wetmore wrote:
On 10/10/2012 5:47 AM, Xuelei Fan wrote:
new webrev: http://cr.openjdk.java.net./~xuelei/7068321/webrev.13/
I guess you didn't need to have me as reviewer before going final with
the CCC?
The CCC has been finalized. ;-) I though you have done with spec review.
Anyway, we still can make updates on specification within new bugs.
javax/net/ssl/SSLSocketFactory.java
===
Change look good.
216: I think you need a period at end of sentence here.
You got the others, but missed this one.
I think we discussed the style sometimes ago. If the sentence does not
start with a capital letter, then it does not need a period at the end
of the sentence. I can see both style in other specs.
Updated to add the period.
Sorry, I meant to say copyright change. Boy, did I goof on that one.
One of the copyright dates wasn't right, but the rest were.
I'll respond to the rest tomorrow. I have a sick PC to diagnose. :(
Brad
231: Might suggest some reason text for the
UnsupportedOperationException.
I think the exception name has say the exception clearly. I think it
does not make sense to add additional message for it. I had a search in
the packages of java.lang and java.uitl, most of the codes (39 of 42)
use the default non-parameter constructor of
UnsupportedOperationException. I will prefer to use the current style.
No problem.
sun/security/ssl/Utilities.java
===
46: Minor comment on method name, you might want to use
"addToSNIServerNameList" since you are adding.
84: Just wondering why you added this method here? This added a bit of
overhead in extra methods calls (well, maybe not with hotspot unrolling)
and added a few chars to the source. So given that you have added this,
why not update the remainder also?
EngineOutputRecord/InputRecord/OutputRecord.
Good point!
Or do it the way you did. This just adds a little extra source overhead.
See the below comment in SSLSocketImpl.java. If you decide to accept
it, you will want to remove the unmodifiable collection.
See the reply in SSLSocketImpl.java
Ok.
sun/security/ssl/Handshaker.java
291: The change to allow for setting of SNI information has opened up a
race condition. It's probably not too bad for SSLSocket, but might be
more for SSLEngine. When we create ClientHandshaker/ServerHandshakers,
we normally grab all the parameters necessary for the handshaker, and
any future parameter modifications will apply to the next handshake. In
the past, our SNI information would never change, so it wasn't necessary
to pass it in on creation. That assumption no longer holds.
So you could see something like this:
sslEngine.beginHandshake();
SSLParameters sslp = new SSLParameters();
sslp.setHostnames("example.com");
sslEngine.setSSLParameters(sslp);
// do handshake...
and you'll get the new value instead of old.
Good catch!
Updated to store the local server name indication and matchers in
Handshaker.
Just a comment:
459/468: Currently you have serverNames and sniMatchers as package
private variables, so the ClientHandshaker/ServerHandshaker *could*
inherit these variables rather than have a separate get methods. Or you
could make them private and keep the get calls.
Good catch!
sun/security/ssl/HelloExtensions.java
=
423: This behavior is currently underspecified.
I think the behavior is specified in RFC 6066:
... If the server understood the ClientHello extension but
does not recognize the server name, the server SHOULD take one of two
actions: either abort the handshake by sending a fatal-level
unrecognized_name(112) alert or continue the handshake.
As means that the server will try to recognize every type of server
name.
I'm just not seeing why this implies that it requires *EVERY* name must
match. This just says we can do one of two things upon receipt of an
unrecognized hostname: continue on, or alert/close. We can be very
restrictive (ALL/AND) or less so (at least one/OR), and still be within
the RFC, I think.
I agree with your parser.
In our spec, it is required that once a SNIMatcher is defined, it
will be used to recognized the server name in the SNI extension.
Where? We do say in SNIMatcher:
Servers can use Server Name Indication (SNI) information to decide if
specific {@link SSLSocket} or {@link SSLEngine} instances should
accept a connection.
But I don't think we have ever said that it MUST match or must match all
or even what the implementation must do if there is a match failure. Nor
should we specify that in the API, IMHO. That's implementation behavior.
I may have different options on this point. I think, it must be a
specifi
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
No new webrev. I need a review of how to use SNIMatcher. See bellow
inline comments.
On 10/11/2012 7:38 AM, Brad Wetmore wrote:
>
>
> On 10/10/2012 5:47 AM, Xuelei Fan wrote:
>> new webrev: http://cr.openjdk.java.net./~xuelei/7068321/webrev.13/
>
> I guess you didn't need to have me as reviewer before going final with
> the CCC?
>
The CCC has been finalized. ;-) I though you have done with spec review.
Anyway, we still can make updates on specification within new bugs.
>>> javax/net/ssl/SSLSocketFactory.java
>>> ===
>>> Change look good.
>>>
>>> 216: I think you need a period at end of sentence here.
>
> You got the others, but missed this one.
>
I think we discussed the style sometimes ago. If the sentence does not
start with a capital letter, then it does not need a period at the end
of the sentence. I can see both style in other specs.
Updated to add the period.
>>> 231: Might suggest some reason text for the
>>> UnsupportedOperationException.
>>>
>> I think the exception name has say the exception clearly. I think it
>> does not make sense to add additional message for it. I had a search in
>> the packages of java.lang and java.uitl, most of the codes (39 of 42)
>> use the default non-parameter constructor of
>> UnsupportedOperationException. I will prefer to use the current style.
>
> No problem.
>
>>> sun/security/ssl/Utilities.java
>>> ===
>>> 46: Minor comment on method name, you might want to use
>>> "addToSNIServerNameList" since you are adding.
>>>
>>> 84: Just wondering why you added this method here? This added a bit of
>>> overhead in extra methods calls (well, maybe not with hotspot unrolling)
>>> and added a few chars to the source. So given that you have added this,
>>> why not update the remainder also?
>>> EngineOutputRecord/InputRecord/OutputRecord.
>>>
>> Good point!
>
> Or do it the way you did. This just adds a little extra source overhead.
>
>>> See the below comment in SSLSocketImpl.java. If you decide to accept
>>> it, you will want to remove the unmodifiable collection.
>>>
>> See the reply in SSLSocketImpl.java
>
> Ok.
>
>>> sun/security/ssl/Handshaker.java
>>>
>>> 291: The change to allow for setting of SNI information has opened up a
>>> race condition. It's probably not too bad for SSLSocket, but might be
>>> more for SSLEngine. When we create ClientHandshaker/ServerHandshakers,
>>> we normally grab all the parameters necessary for the handshaker, and
>>> any future parameter modifications will apply to the next handshake. In
>>> the past, our SNI information would never change, so it wasn't necessary
>>> to pass it in on creation. That assumption no longer holds.
>>>
>>> So you could see something like this:
>>>
>>> sslEngine.beginHandshake();
>>> SSLParameters sslp = new SSLParameters();
>>> sslp.setHostnames("example.com");
>>> sslEngine.setSSLParameters(sslp);
>>> // do handshake...
>>>
>>> and you'll get the new value instead of old.
>>>
>> Good catch!
>>
>> Updated to store the local server name indication and matchers in
>> Handshaker.
>
> Just a comment:
>
> 459/468: Currently you have serverNames and sniMatchers as package
> private variables, so the ClientHandshaker/ServerHandshaker *could*
> inherit these variables rather than have a separate get methods. Or you
> could make them private and keep the get calls.
>
Good catch!
>>> sun/security/ssl/HelloExtensions.java
>>> =
>>> 423: This behavior is currently underspecified.
>>
>> I think the behavior is specified in RFC 6066:
>>
>> ... If the server understood the ClientHello extension but
>> does not recognize the server name, the server SHOULD take one of two
>> actions: either abort the handshake by sending a fatal-level
>> unrecognized_name(112) alert or continue the handshake.
>>
>> As means that the server will try to recognize every type of server
>> name.
>
> I'm just not seeing why this implies that it requires *EVERY* name must
> match. This just says we can do one of two things upon receipt of an
> unrecognized hostname: continue on, or alert/close. We can be very
> restrictive (ALL/AND) or less so (at least one/OR), and still be within
> the RFC, I think.
>
I agree with your parser.
>> In our spec, it is required that once a SNIMatcher is defined, it
>> will be used to recognized the server name in the SNI extension.
>
> Where? We do say in SNIMatcher:
>
>Servers can use Server Name Indication (SNI) information to decide if
>specific {@link SSLSocket} or {@link SSLEngine} instances should
>accept a connection.
>
> But I don't think we have ever said that it MUST match or must match all
> or even what the implementation must do if there is a match failure. Nor
> should we specify that in the API, IMHO. That's implementation behavior.
>
I may have different options on this poin
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
On 10/10/2012 5:47 AM, Xuelei Fan wrote:
new webrev: http://cr.openjdk.java.net./~xuelei/7068321/webrev.13/
I guess you didn't need to have me as reviewer before going final with
the CCC?
javax/net/ssl/SSLSocketFactory.java
===
Change look good.
216: I think you need a period at end of sentence here.
You got the others, but missed this one.
231: Might suggest some reason text for the UnsupportedOperationException.
I think the exception name has say the exception clearly. I think it
does not make sense to add additional message for it. I had a search in
the packages of java.lang and java.uitl, most of the codes (39 of 42)
use the default non-parameter constructor of
UnsupportedOperationException. I will prefer to use the current style.
No problem.
sun/security/ssl/Utilities.java
===
46: Minor comment on method name, you might want to use
"addToSNIServerNameList" since you are adding.
84: Just wondering why you added this method here? This added a bit of
overhead in extra methods calls (well, maybe not with hotspot unrolling)
and added a few chars to the source. So given that you have added this,
why not update the remainder also?
EngineOutputRecord/InputRecord/OutputRecord.
Good point!
Or do it the way you did. This just adds a little extra source overhead.
See the below comment in SSLSocketImpl.java. If you decide to accept
it, you will want to remove the unmodifiable collection.
See the reply in SSLSocketImpl.java
Ok.
sun/security/ssl/Handshaker.java
291: The change to allow for setting of SNI information has opened up a
race condition. It's probably not too bad for SSLSocket, but might be
more for SSLEngine. When we create ClientHandshaker/ServerHandshakers,
we normally grab all the parameters necessary for the handshaker, and
any future parameter modifications will apply to the next handshake. In
the past, our SNI information would never change, so it wasn't necessary
to pass it in on creation. That assumption no longer holds.
So you could see something like this:
sslEngine.beginHandshake();
SSLParameters sslp = new SSLParameters();
sslp.setHostnames("example.com");
sslEngine.setSSLParameters(sslp);
// do handshake...
and you'll get the new value instead of old.
Good catch!
Updated to store the local server name indication and matchers in
Handshaker.
Just a comment:
459/468: Currently you have serverNames and sniMatchers as package
private variables, so the ClientHandshaker/ServerHandshaker *could*
inherit these variables rather than have a separate get methods. Or you
could make them private and keep the get calls.
sun/security/ssl/HelloExtensions.java
=
423: This behavior is currently underspecified.
I think the behavior is specified in RFC 6066:
... If the server understood the ClientHello extension but
does not recognize the server name, the server SHOULD take one of two
actions: either abort the handshake by sending a fatal-level
unrecognized_name(112) alert or continue the handshake.
As means that the server will try to recognize every type of server
name.
I'm just not seeing why this implies that it requires *EVERY* name must
match. This just says we can do one of two things upon receipt of an
unrecognized hostname: continue on, or alert/close. We can be very
restrictive (ALL/AND) or less so (at least one/OR), and still be within
the RFC, I think.
In our spec, it is required that once a SNIMatcher is defined, it
will be used to recognized the server name in the SNI extension.
Where? We do say in SNIMatcher:
Servers can use Server Name Indication (SNI) information to decide if
specific {@link SSLSocket} or {@link SSLEngine} instances should
accept a connection.
But I don't think we have ever said that it MUST match or must match all
or even what the implementation must do if there is a match failure.
Nor should we specify that in the API, IMHO. That's implementation
behavior.
That's
to say, every server name should be recognizable if the corresponding
SNIMatcher is defined. I think the current spec is clear that SNIMather
is used to match a particular server name type.
Not seeing it, sorry.
After this discussion, I think we are probably better off starting with
AND anyway, because it will be easier to loosen that condition than
starting with OR and having to tighten to AND. If you agree, can you
add a few words to that effect around 431?
Right now we have one
SNI match type, but eventually we might have more. Your current impl
requires that *ALL* SNI matchers match. What if you have more than one,
and more than one SNI hostnames are sent?
SNIHostname: "example.com"
SNINickName: "www.example.com"
SNIMatcher: "example.com"
SNINickNameMatcher: "www1.example.com
Should this fail or succeed? T
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
new webrev: http://cr.openjdk.java.net./~xuelei/7068321/webrev.13/
On 10/10/2012 8:33 AM, Brad Wetmore wrote:
> Hi again,
>
> On 10/9/2012 8:56 AM, Xuelei Fan wrote:
>> Thanks for the comments. The new comments for
>> SSLEngine/SSLSocket/SSLServerSocket do make the spec much more simple
>> and clear.
>
> Thanks, that looks so much cleaner, and I think developers will
> appreciate it.
>
>> BTW, I removed the restriction on SSLSocketFactory.createSocket(), the
>> socket parameter can be an instance of SSLSocket.
>>
>> The spec review is closed. Please file new bugs if you have other
>> concerns.
>
> Very minor comments in the API which don't need CCC modification.
>
> javax/net/ssl/SNIHostName.java
> ==
> Changes look good. Didn't review everything.
>
> javax/net/ssl/SNIMatcher.java
> =
> Changes look good. Didn't review everything.
>
> javax/net/ssl/SNIServerName.java
>
> Changes look good. Didn't review everything.
>
> javax/net/ssl/StandardConstants.java
>
> Didn't review.
>
> javax/net/ssl/ExtendedSSLSession.java
> =
> Didn't review.
>
> javax/net/ssl/SSLParameters.java
>
> 60: Wow, good catch!
>
> 314: Looks great, thanks!
>
> javax/net/ssl/SSLSocketFactory.java
> ===
> Change look good.
>
> 216: I think you need a period at end of sentence here.
>
> 231: Might suggest some reason text for the UnsupportedOperationException.
>
I think the exception name has say the exception clearly. I think it
does not make sense to add additional message for it. I had a search in
the packages of java.lang and java.uitl, most of the codes (39 of 42)
use the default non-parameter constructor of
UnsupportedOperationException. I will prefer to use the current style.
>
> javax/net/ssl/SSLEngine.java
>
> Minor nit: Copyright.
>
> 1218/1220/1225: You are not closing the properly, I think you are
> effectively adding another empty bullet.
>
> 1227: Copy/paste error. You said socket, but probably meant engine. :)
>
> javax/net/ssl/SSLServerSocket.java
> ==
> Minor nit: Copyright.
>
> 488/490/495: You are not closing the properly.
>
> 499: Similar comment to above. You could say server socket or leave as
> is, since it is a kind of a socket.
>
> javax/net/ssl/SSLSocket.java
>
> Minor nit: Copyright.
>
> sun/security/ssl/Utilities.java
> ===
> 46: Minor comment on method name, you might want to use
> "addToSNIServerNameList" since you are adding.
>
> 84: Just wondering why you added this method here? This added a bit of
> overhead in extra methods calls (well, maybe not with hotspot unrolling)
> and added a few chars to the source. So given that you have added this,
> why not update the remainder also?
> EngineOutputRecord/InputRecord/OutputRecord.
>
Good point!
> See the below comment in SSLSocketImpl.java. If you decide to accept
> it, you will want to remove the unmodifiable collection.
>
See the reply in SSLSocketImpl.java
> sun/security/ssl/BaseSSLSocketImpl.java
> ===
> OK
>
> sun/security/ssl/ClientHandshaker.java
> ==
> OK
>
> sun/security/ssl/HandshakeInStream.java
> ===
> OK
>
> sun/security/ssl/HandshakeMessage.java
> ==
> OK
>
> sun/security/ssl/Handshaker.java
>
> 291: The change to allow for setting of SNI information has opened up a
> race condition. It's probably not too bad for SSLSocket, but might be
> more for SSLEngine. When we create ClientHandshaker/ServerHandshakers,
> we normally grab all the parameters necessary for the handshaker, and
> any future parameter modifications will apply to the next handshake. In
> the past, our SNI information would never change, so it wasn't necessary
> to pass it in on creation. That assumption no longer holds.
>
> So you could see something like this:
>
> sslEngine.beginHandshake();
> SSLParameters sslp = new SSLParameters();
> sslp.setHostnames("example.com");
> sslEngine.setSSLParameters(sslp);
> // do handshake...
>
> and you'll get the new value instead of old.
>
Good catch!
Updated to store the local server name indication and matchers in
Handshaker.
> sun/security/ssl/HelloExtensions.java
> =
> 35: Are these extra imports necessary with the package import at line 31?
>
> 307: Can you add the "backwards compatibility" comment about the size?
>
>For
>backward compatibility, all future data structures associated with
>new NameTypes MUST begin with a 16-bit length field.
>
> I'll forget it otherwise.
>
> 423: This beh
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
Hi again,
On 10/9/2012 8:56 AM, Xuelei Fan wrote:
Thanks for the comments. The new comments for
SSLEngine/SSLSocket/SSLServerSocket do make the spec much more simple
and clear.
Thanks, that looks so much cleaner, and I think developers will
appreciate it.
BTW, I removed the restriction on SSLSocketFactory.createSocket(), the
socket parameter can be an instance of SSLSocket.
The spec review is closed. Please file new bugs if you have other concerns.
Very minor comments in the API which don't need CCC modification.
javax/net/ssl/SNIHostName.java
==
Changes look good. Didn't review everything.
javax/net/ssl/SNIMatcher.java
=
Changes look good. Didn't review everything.
javax/net/ssl/SNIServerName.java
Changes look good. Didn't review everything.
javax/net/ssl/StandardConstants.java
Didn't review.
javax/net/ssl/ExtendedSSLSession.java
=
Didn't review.
javax/net/ssl/SSLParameters.java
60: Wow, good catch!
314: Looks great, thanks!
javax/net/ssl/SSLSocketFactory.java
===
Change look good.
216: I think you need a period at end of sentence here.
231: Might suggest some reason text for the UnsupportedOperationException.
javax/net/ssl/SSLEngine.java
Minor nit: Copyright.
1218/1220/1225: You are not closing the properly, I think you are
effectively adding another empty bullet.
1227: Copy/paste error. You said socket, but probably meant engine. :)
javax/net/ssl/SSLServerSocket.java
==
Minor nit: Copyright.
488/490/495: You are not closing the properly.
499: Similar comment to above. You could say server socket or leave as
is, since it is a kind of a socket.
javax/net/ssl/SSLSocket.java
Minor nit: Copyright.
sun/security/ssl/Utilities.java
===
46: Minor comment on method name, you might want to use
"addToSNIServerNameList" since you are adding.
84: Just wondering why you added this method here? This added a bit of
overhead in extra methods calls (well, maybe not with hotspot unrolling)
and added a few chars to the source. So given that you have added this,
why not update the remainder also?
EngineOutputRecord/InputRecord/OutputRecord.
See the below comment in SSLSocketImpl.java. If you decide to accept
it, you will want to remove the unmodifiable collection.
sun/security/ssl/BaseSSLSocketImpl.java
===
OK
sun/security/ssl/ClientHandshaker.java
==
OK
sun/security/ssl/HandshakeInStream.java
===
OK
sun/security/ssl/HandshakeMessage.java
==
OK
sun/security/ssl/Handshaker.java
291: The change to allow for setting of SNI information has opened up a
race condition. It's probably not too bad for SSLSocket, but might be
more for SSLEngine. When we create ClientHandshaker/ServerHandshakers,
we normally grab all the parameters necessary for the handshaker, and
any future parameter modifications will apply to the next handshake. In
the past, our SNI information would never change, so it wasn't necessary
to pass it in on creation. That assumption no longer holds.
So you could see something like this:
sslEngine.beginHandshake();
SSLParameters sslp = new SSLParameters();
sslp.setHostnames("example.com");
sslEngine.setSSLParameters(sslp);
// do handshake...
and you'll get the new value instead of old.
sun/security/ssl/HelloExtensions.java
=
35: Are these extra imports necessary with the package import at line 31?
307: Can you add the "backwards compatibility" comment about the size?
For
backward compatibility, all future data structures associated with
new NameTypes MUST begin with a 16-bit length field.
I'll forget it otherwise.
423: This behavior is currently underspecified. Right now we have one
SNI match type, but eventually we might have more. Your current impl
requires that *ALL* SNI matchers match. What if you have more than one,
and more than one SNI hostnames are sent?
SNIHostname: "example.com"
SNINickName: "www.example.com"
SNIMatcher: "example.com"
SNINickNameMatcher: "www1.example.com
Should this fail or succeed? That is, should it be an AND or an OR?
Whatever you decide, please document it at least here. Not sure if you
want to make the doc at the API level.
438: Minor nit: snInOther -> sniInOther?
sun/security/ssl/ProtocolList.java
==
OK
sun/security/ssl/SSLEngineImpl.java
===
2084: See comments in SSLSocketImpl.java:2509.
2100/2107: See co
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
The new webrev with the updated spec and implementation:
http://cr.openjdk.java.net./~xuelei/7068321/webrev.12/
Xuelei
On 10/9/2012 11:56 PM, Xuelei Fan wrote:
> Thanks for the comments. The new comments for
> SSLEngine/SSLSocket/SSLServerSocket do make the spec much more simple
> and clear.
>
> BTW, I removed the restriction on SSLSocketFactory.createSocket(), the
> socket parameter can be an instance of SSLSocket.
>
> The spec review is closed. Please file new bugs if you have other concerns.
>
> Thanks,
> Xuelei
>
> On 10/9/2012 9:03 AM, Brad Wetmore wrote:
>>
>> On 9/26/2012 8:05 AM, Xuelei Fan wrote:
>>> Hi,
>>>
>>> Please review the implementation of JEP 114/CR 7068321, Support TLS
>>> Server Name Indication (SNI) Extension in JSSE Server.
>>>
>>> JEP 114: http://openjdk.java.net/jeps/114
>>> BuG: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7068321
>>> webrev : http://cr.openjdk.java.net./~xuelei/7068321/webrev.10/
>>
>> Not a lot of comments since we've already done so much over the last few
>> months. :)
>>
>> If you modified anything else that we hadn't talked about previously,
>> please call it out. I didn't do a complete line/line review, just
>> looking at the main points and previous comments.
>>
>>
>> javax/net/ssl/SSLSocket.java
>> javax/net/ssl/SSLEngine.java
>> javax/net/ssl/SSLServerSocket.java
>> ==
>> (Xuelei and I have been having internal discussions about how to rework
>> some of the lengthy verbage currently in the SSLParameters class, so
>> this comment builds on that discussion.)
>>
>> I just noticed three classes missing from the webrev. We are updating
>> the SSLParameters class with two new data structures (Host
>> names/Matchers), but the actual discussion of what happens to the
>> underlying SSLSockets/SSLEngines/SSLServerSocket is missing. Note we
>> did that for the other values like protocols/cipherSuites.
>>
>> Rather than the current discussion in SSLParameters about "previously
>> configured/default", I would like to suggest we follow the simple
>> discussion already in the javadocs, and it can shorten the verbiage in
>> SSLParameters very significantly! Even though we don't have equivalent
>> methods (setEnabledHostNames()) in SSLSocket/SSLEngine/SSLServer, we
>> should still talk about what happens when called:
>>
>> Add:
>>
>> This means:
>>
>> o ...deleted...
>> o if params.getServerNames() is non-null, the socket will configure
>> its server names with that value.
>> o if params.getSNIMatchers() is non-null, the socket will configure
>> its SNI matchers with that value.
>>
>> This places the behavior explanation better where it should be, and not
>> in the configuration discussion.
>>
>> Now in the SSLParameters we simply talk about the effects these methods
>> have on the SSLParameters class and what the values represent. We
>> already have the link to
>> SSLSocket/SSLServerSocket/SSLEngine.setSSLParameters(). So...
>>
>> setServerNames() remains as is.
>> setSNIHostNames() remains as is.
>>
>> getServerNames():
>> =
>> 316: Add to the constructor
>> ", or null if none has been set."
>>
>> 321-326: We can leave this discussion if desired, I think it sets up
>> the discussion for 327-341 well. I might suggest reordering the points
>> slightly:
>>
>>* It is recommended that providers initialize default Server Name
>>* Indications when creating
>>* SSLSocket/SSLEngines. In the following
>>* examples, the server name could be represented by an instance of
>>* {@link SNIHostName} which has been initialized with the hostname
>>* "www.example.com" and type {@link StandardConstants#SNI_HOST_NAME}.
>>*
>>*
>>* Socket socket =
>>* sslSocketFactory.createSocket("www.example.com", 443);
>>*
>>* or
>>*
>>* SSLEngine engine =
>>* sslContext.createSSLEngine("www.example.com", 443);
>>*
>>*
>>
>> 342-367: Remove these lines. All the necessary info is in the
>> setSSLParameters methods.
>>
>> getSNIHostNames():
>> ==
>> 435: Add to the constructor
>> ", or null if none has been set."
>>
>> 440-471: Remove these lines.
>>
>> Nice and clean. All the discussion about default/current goes away.
>>
>> javax/net/ssl/SNIHostName.java
>> ==
>> 52: Do you want to remind folks here that this class is supposed to be
>> immutable?
>>
>> 64: Not sure if caching this hash value will actually add much to
>> performance vs. volatile adds overhead. There will only a few of these
>> (likely 1).
>>
>> 85-86: Should these fragments be separated "," instead of "." as you
>> did below in 143-149. You probably need an "or" before the last item.
>>
>> 147: You probably need an "or" before the last item.
>>
>> javax/net/ssl/SNIMatcher.java
>> =
>> 32: Do you want to spell out Server Name Indication before you use the
>> ab
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
Thanks for the comments. The new comments for
SSLEngine/SSLSocket/SSLServerSocket do make the spec much more simple
and clear.
BTW, I removed the restriction on SSLSocketFactory.createSocket(), the
socket parameter can be an instance of SSLSocket.
The spec review is closed. Please file new bugs if you have other concerns.
Thanks,
Xuelei
On 10/9/2012 9:03 AM, Brad Wetmore wrote:
>
> On 9/26/2012 8:05 AM, Xuelei Fan wrote:
>> Hi,
>>
>> Please review the implementation of JEP 114/CR 7068321, Support TLS
>> Server Name Indication (SNI) Extension in JSSE Server.
>>
>> JEP 114: http://openjdk.java.net/jeps/114
>> BuG: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7068321
>> webrev : http://cr.openjdk.java.net./~xuelei/7068321/webrev.10/
>
> Not a lot of comments since we've already done so much over the last few
> months. :)
>
> If you modified anything else that we hadn't talked about previously,
> please call it out. I didn't do a complete line/line review, just
> looking at the main points and previous comments.
>
>
> javax/net/ssl/SSLSocket.java
> javax/net/ssl/SSLEngine.java
> javax/net/ssl/SSLServerSocket.java
> ==
> (Xuelei and I have been having internal discussions about how to rework
> some of the lengthy verbage currently in the SSLParameters class, so
> this comment builds on that discussion.)
>
> I just noticed three classes missing from the webrev. We are updating
> the SSLParameters class with two new data structures (Host
> names/Matchers), but the actual discussion of what happens to the
> underlying SSLSockets/SSLEngines/SSLServerSocket is missing. Note we
> did that for the other values like protocols/cipherSuites.
>
> Rather than the current discussion in SSLParameters about "previously
> configured/default", I would like to suggest we follow the simple
> discussion already in the javadocs, and it can shorten the verbiage in
> SSLParameters very significantly! Even though we don't have equivalent
> methods (setEnabledHostNames()) in SSLSocket/SSLEngine/SSLServer, we
> should still talk about what happens when called:
>
> Add:
>
> This means:
>
> o ...deleted...
> o if params.getServerNames() is non-null, the socket will configure
> its server names with that value.
> o if params.getSNIMatchers() is non-null, the socket will configure
> its SNI matchers with that value.
>
> This places the behavior explanation better where it should be, and not
> in the configuration discussion.
>
> Now in the SSLParameters we simply talk about the effects these methods
> have on the SSLParameters class and what the values represent. We
> already have the link to
> SSLSocket/SSLServerSocket/SSLEngine.setSSLParameters(). So...
>
> setServerNames() remains as is.
> setSNIHostNames() remains as is.
>
> getServerNames():
> =
> 316: Add to the constructor
> ", or null if none has been set."
>
> 321-326: We can leave this discussion if desired, I think it sets up
> the discussion for 327-341 well. I might suggest reordering the points
> slightly:
>
>* It is recommended that providers initialize default Server Name
>* Indications when creating
>* SSLSocket/SSLEngines. In the following
>* examples, the server name could be represented by an instance of
>* {@link SNIHostName} which has been initialized with the hostname
>* "www.example.com" and type {@link StandardConstants#SNI_HOST_NAME}.
>*
>*
>* Socket socket =
>* sslSocketFactory.createSocket("www.example.com", 443);
>*
>* or
>*
>* SSLEngine engine =
>* sslContext.createSSLEngine("www.example.com", 443);
>*
>*
>
> 342-367: Remove these lines. All the necessary info is in the
> setSSLParameters methods.
>
> getSNIHostNames():
> ==
> 435: Add to the constructor
> ", or null if none has been set."
>
> 440-471: Remove these lines.
>
> Nice and clean. All the discussion about default/current goes away.
>
> javax/net/ssl/SNIHostName.java
> ==
> 52: Do you want to remind folks here that this class is supposed to be
> immutable?
>
> 64: Not sure if caching this hash value will actually add much to
> performance vs. volatile adds overhead. There will only a few of these
> (likely 1).
>
> 85-86: Should these fragments be separated "," instead of "." as you
> did below in 143-149. You probably need an "or" before the last item.
>
> 147: You probably need an "or" before the last item.
>
> javax/net/ssl/SNIMatcher.java
> =
> 32: Do you want to spell out Server Name Indication before you use the
> abbreviation? Or link to RFC 6066? Just a thought, not strictly
> necessary.
>
> 105: Empty line at end of file.
>
> javax/net/ssl/SNIServerName.java
>
> 56: Same comment about hashCode/volatile.
>
> 136: Might want to add a between sentences.
>
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
On 9/26/2012 8:05 AM, Xuelei Fan wrote:
Hi,
Please review the implementation of JEP 114/CR 7068321, Support TLS
Server Name Indication (SNI) Extension in JSSE Server.
JEP 114: http://openjdk.java.net/jeps/114
BuG: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7068321
webrev : http://cr.openjdk.java.net./~xuelei/7068321/webrev.10/
Not a lot of comments since we've already done so much over the last few
months. :)
If you modified anything else that we hadn't talked about previously,
please call it out. I didn't do a complete line/line review, just
looking at the main points and previous comments.
javax/net/ssl/SSLSocket.java
javax/net/ssl/SSLEngine.java
javax/net/ssl/SSLServerSocket.java
==
(Xuelei and I have been having internal discussions about how to rework
some of the lengthy verbage currently in the SSLParameters class, so
this comment builds on that discussion.)
I just noticed three classes missing from the webrev. We are updating
the SSLParameters class with two new data structures (Host
names/Matchers), but the actual discussion of what happens to the
underlying SSLSockets/SSLEngines/SSLServerSocket is missing. Note we
did that for the other values like protocols/cipherSuites.
Rather than the current discussion in SSLParameters about "previously
configured/default", I would like to suggest we follow the simple
discussion already in the javadocs, and it can shorten the verbiage in
SSLParameters very significantly! Even though we don't have equivalent
methods (setEnabledHostNames()) in SSLSocket/SSLEngine/SSLServer, we
should still talk about what happens when called:
Add:
This means:
o ...deleted...
o if params.getServerNames() is non-null, the socket will configure
its server names with that value.
o if params.getSNIMatchers() is non-null, the socket will configure
its SNI matchers with that value.
This places the behavior explanation better where it should be, and not
in the configuration discussion.
Now in the SSLParameters we simply talk about the effects these methods
have on the SSLParameters class and what the values represent. We
already have the link to
SSLSocket/SSLServerSocket/SSLEngine.setSSLParameters(). So...
setServerNames() remains as is.
setSNIHostNames() remains as is.
getServerNames():
=
316: Add to the constructor
", or null if none has been set."
321-326: We can leave this discussion if desired, I think it sets up
the discussion for 327-341 well. I might suggest reordering the points
slightly:
* It is recommended that providers initialize default Server Name
* Indications when creating
* SSLSocket/SSLEngines. In the following
* examples, the server name could be represented by an instance of
* {@link SNIHostName} which has been initialized with the hostname
* "www.example.com" and type {@link StandardConstants#SNI_HOST_NAME}.
*
*
* Socket socket =
* sslSocketFactory.createSocket("www.example.com", 443);
*
* or
*
* SSLEngine engine =
* sslContext.createSSLEngine("www.example.com", 443);
*
*
342-367: Remove these lines. All the necessary info is in the
setSSLParameters methods.
getSNIHostNames():
==
435: Add to the constructor
", or null if none has been set."
440-471: Remove these lines.
Nice and clean. All the discussion about default/current goes away.
javax/net/ssl/SNIHostName.java
==
52: Do you want to remind folks here that this class is supposed to be
immutable?
64: Not sure if caching this hash value will actually add much to
performance vs. volatile adds overhead. There will only a few of these
(likely 1).
85-86: Should these fragments be separated "," instead of "." as you
did below in 143-149. You probably need an "or" before the last item.
147: You probably need an "or" before the last item.
javax/net/ssl/SNIMatcher.java
=
32: Do you want to spell out Server Name Indication before you use the
abbreviation? Or link to RFC 6066? Just a thought, not strictly necessary.
105: Empty line at end of file.
javax/net/ssl/SNIServerName.java
56: Same comment about hashCode/volatile.
136: Might want to add a between sentences.
javax/net/ssl/StandardConstants.java
OK
javax/net/ssl/ExtendedSSLSession.java
=
OK
javax/net/ssl/SSLParameters.java
See above. No other comments.
javax/net/ssl/SSLSocketFactory.java
===
Ok.
So that you can update and submit to CCC, I'll stop here, but the
implementation review will be continued in next email.
Hope this helps,
Brad
Re: Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
On 9/26/2012 8:05 AM, Xuelei Fan wrote: Hi, Please review the implementation of JEP 114/CR 7068321, Support TLS Server Name Indication (SNI) Extension in JSSE Server. JEP 114: http://openjdk.java.net/jeps/114 BuG: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7068321 webrev : http://cr.openjdk.java.net./~xuelei/7068321/webrev.10/ I would prefer to get any comments by the end of next Monday (Oct 1th), if possible. Minor correction to the URL: http://cr.openjdk.java.net/~xuelei/7068321/webrev.10/ BTW, the openjdk.java.net domain is temporarily down, Ops is working on it. Brad
