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
>>> <bradford.wetm...@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
>>>>>
>>
