Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
Bradford Wetmore
bradford.wetmore at oracle.com
Thu Oct 11 04:21:11 UTC 2012
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
> 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