Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
Xuelei Fan
xuelei.fan at oracle.com
Thu Oct 11 02:54:43 UTC 2012
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 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.
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?
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.
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. 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 included, it does not follow the
spec when server side is able to recognize the server name.
So, I will prefer that once a SNIMatcher is defined, it must be used to
perform match operation if the corresponding server name type appears in
the SNI extension.
I think we need to adjust the spec to make the behavior more clear. I
will adjust the spec of SNIMatcher a little:
* the exact server that the client wants to access. Instances of this
- * class can be used by a server to verify the acceptable server names
+ * class is used by a server to verify the acceptable server names
* of a particular type, such as host names.
And SSLParameters.getSNIMatchers()
* <P>
* This method is only useful to {@link SSLSocket}s or
* {@link SSLEngine}s operating in server mode.
+ * <P>
+ * Every {@code SNIMatcher} in the returned {@link Collection} must
+ * be used to perform match operation if the corresponding name
+ * type appears in the SNI extension.
* <P>
* For better interoperability, providers generally will not define
* default matchers so that by default servers will ignore the SNI
* extension and continue the handshake.
What do you think? I may not need a new bug or CCC request because the
update is minor.
>> 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?
>
Agree! See above.
>>> 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.
>>>
>> If it is a "OR", we need to doc the special behaviors in API level. But
>> it is a "AND", I think the current API is clear that a defined
>> SNIMatcher is used to match a particular server name type in SNI
>> extension. I think the current spec is OK.
>
> I think we can leave it unspecified for now, but please add a quick note
> here. Thanks.
>
See above.
>>> sun/security/ssl/SSLSocketImpl.java
>>> ===================================
>>> 561: If I'm remembering and understanding this code right:
>>>
>>> this.host = sock.getLocalAddress().getHostName(); // in server
>>> mode
>>>
>>> Don't ServerSockets generally creates Sockets with just local IP
>>> addresses (no hostnames), so this getHostname() will trigger a reverse
>>> hostname lookup? Is that right?
>>>
>> Right.
>>
>> It is not really necessary to set the "host" and "serverNames"
>> variables, because in server mode the two variables are useless. I will
>> remove the setting.
>
> Ok.
>
> 561: // In server mode, it is not necessary to set host and serverNames.
> Can you add "...and would require a reverse DNS lookup" or something
> similar?
>
Good.
// In server mode, it is not necessary to set host and serverNames.
// Otherwise, would require a reverse DNS lookup to get the hostname.
>>> 2509: Something to investigate: you are depending on SSLParameters
>>> returning an unmodifiable list. But a subclass might not do that. Can
>>> you think of any situations where this might be a bad idea? If so, then
>>> we might want to change the unmodifiable language in SSLParameters and
>>> do regular cloning. This will have repercussions in other areas, like
>>> around 2527/2532, where you'll need to pass a copy to the Handshaker.
>>>
>> Well, I understand that something bad would happen if the subclass does
>> not follow the method spec while doing method implementation overriding.
>
> Yes, it was just something to think about. Of course, I'm trying to
> think of exploit situations, since you're really just hurting yourself
> if tweak this in the middle of a handshake.
>
> If I see Joe Darcy around, I'll check in with him.
>
Thanks! I really think we need to think about it more and carefully. The
style (immutable returned value in none-final method) does impact the
reliability of the APIs.
>> Let's have the spec and implementation as it is. We can update the spec
>> and implementation if we have strong concerns or a common solution for
>> the issue before JDK 8 officially release.
>
> I'm fine with this.
>
>>> sun/security/ssl/X509TrustManagerImpl.java
>>> ==========================================
>>> OK
>>>
>> All of other comments are accepted.
>>
>> I think there is no major concerns so far.
>
> None still outstanding. I looked over the previous comments I've made
> and what you've done to address them, and all looks good minus the
> little points above.
>
Good! It seems that we only have one question, how to use SNIMatachers?
See above.
Thanks,
Xuelei
>> Thank you so much for the
>> detailed evaluation and comments on both spec and implementation. TLS
>> and its implementation is very complicated, your support is the critical
>> success factor to deliver quality product.
>
> Thanks, and be sure to give yourself a lot of that credit. With all the
> back/forth and development this feature has seen, I think you've done a
> great job.
>
> Brad
>
More information about the security-dev
mailing list