Code review request, JEP 114, 7068321 Support TLS Server Name Indication (SNI) Extension in JSSE Server
Brad Wetmore
bradford.wetmore at oracle.com
Tue Oct 16 20:57:13 UTC 2012
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
>>>> <bradford.wetmore at oracle.com> 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, 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.
>>>>>
>>>> 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.
>>>>
>>>>> 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
>>>>>
>>>>> I think I still prefer "can be used" because some servers won't
>>>>> accept SNI info, and this indicates to me that it absolutely will be
>>>>> used in all situations. Unless you said something like "are used by
>>>>> servers which recognize the extension..." But that gets wordy.
>>>>>
>>>>>> * 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 think what you had is fine, and I suggest we go with it for now.
>>>>> I think we can and should ask for clarification at IETF as to the
>>>>> intent, and we add this new paragraph as an enhancement if they are
>>>>> in agreement. If there is no agreement there, then we could just
>>>>> call it a implementation detail and continue using AND.
>>>>>
>>>> Ok. Let's go with the current spec.
>>>>
>>>>>> I may not need a new bug or CCC request because the
>>>>>> update is minor.
>>>>>
>>>>> Actually, I think this is a little more than a minor change.
>>>>>
>>>>> Which reminds me, is your server code sending SNI extensions for
>>>>> anonymous suites?
>>>>>
>>>> Yes. See above.
>>>>
>>>>>>>>> 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.
>>>>>
>>>>> I talked to Joe, and it's one of those grey areas. It's ultimately
>>>>> your own fault if you misuse the API like this. However, if there
>>>>> were potential vulnerability issues, that would be different. Given
>>>>> what this API does, I don't see anything like that here. But you're
>>>>> right, it's not reliable which is why I brought it up.
>>>>>
>>>> According to effective java, 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?
>>>>
>>>> Xuelei
>>>>
>>>>> Brad
>>>>>
>>>>>
>>>>>
>>>>>>>> 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