Looks fine to me. Thanks, Xuelei
On 5/4/2016 9:38 PM, Ivan Gerasimov wrote: > Thank you Xuelei for looking into this! > > > On 26.04.2016 17:33, Xuelei Fan wrote: >> jdk.tls.handleCertReqAuthoritesOverflow is a little bit long. >> jdk.tls.server.overFlowAuthorites? > Yes, I agree. > Changed to a shorter property name. > >> May not need to define the "none" property value. >> >> Using enum for HAO_NONE, HAO_EMPTY and HAO_TRUNC? > Changed to enum. > Have to keep NONE, though, as we need to represent it in the enum somehow. > >> line 1920: Overflow should be rare, what about print only when overflow? > It the indication of overflow is printed only when it has happened. > > Please find the updated version of webrev: > http://cr.openjdk.java.net/~igerasim/8154947/02/webrev/ > > With kind regards, > Ivan > >> Xuelei >> >> On 4/26/2016 9:56 PM, Seán Coffey wrote: >>> Looks like a fair approach to solving this issue Ivan. A few comments >>> from me : >>> >>> typo : authoririesOverflow --> authoritiesOverflow >>> typo : handleAuthoritesOverflow --> handleAuthoritiesOverflow >>> typo : jdk.tls.handleCertReqAuthoritesOverflow --> >>> jdk.tls.handleCertReqAuthoritiesOverflow >>> >>> + throw new RuntimeException("Value of " + prop >>> + + " must be one of '" + HAO_NONE + "', '" >>> + + HAO_EMPTY + "', '" + HAO_TRUNC + "'"); >>> >>> I think it would be good to print the value of s in above exception >>> also. something like + ". Received: \"" + s + "\""); >>> == >>> >>> s.println("Cert Authorities:" + (authoririesOverflow ? " (overflow)" : >>> "")); >>> >>> I would also be good to indicate the handleAuthoritiesOverflow string >>> value in above printing *if* authoritiesOverflow turns out to be true. >>> We should be able to determine from the next message printed - but no >>> harm to future proof. >>> Maybe : >>> >>> s.println("Cert Authorities:" + (authoritiesOverflow ? " (overflow" + >>> "[" + handleAuthoritiesOverflow + "])" : "")); >>> >>> Regards, >>> Sean. >>> >>> On 26/04/2016 11:57, Ivan Gerasimov wrote: >>>> Here's a modified version of the fix. >>>> >>>> Instead of a boolean-type property, a string-type property is >>>> introduced. >>>> It is used to specify the strategy to use, if we encounter the >>>> overflow during filling the list of authorities. >>>> >>>> The default strategy is to throw an exception (just like the currently >>>> implemented behavior.) >>>> >>>> It can also be set to the values 'empty' or 'truncate', which will >>>> make the server to send an empty or truncated list upon overflow. >>>> >>>> Would you please help review it? >>>> >>>> http://cr.openjdk.java.net/~igerasim/8154947/01/webrev/ >>>> >>>> With kind regards, >>>> Ivan >>>> >>>> >>>> On 22.04.2016 20:09, Ivan Gerasimov wrote: >>>>> Hello everyone! >>>>> >>>>> During TLS handshake, a server may be required to send a >>>>> CertificateRequest, which contains a list of authorities. >>>>> If the list happens to be too long, the server is throwing an >>>>> exception, indicating an overflow. >>>>> >>>>> It may be convenient to be able to just drop the list altogether, and >>>>> let the client to choose a certificate randomly. >>>>> In certain situation this may be more preferable that just block >>>>> communication. >>>>> >>>>> Would you please help review a patch, which introduces an >>>>> command-line option that controls this behavior of the server? >>>>> If the approach is approved, I'll file a CCC request for that option. >>>>> >>>>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8154947 >>>>> WEBREV: http://cr.openjdk.java.net/~igerasim/8154947/00/webrev/ >>>>> >>>>> With the proposed fix all the security-related regression tests, >>>>> including the modified one, passed on all supported platforms. >>>>> >>>>> With kind regards, >>>>> Ivan >>>>> >>>>> >> >
