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
>>>
