On Wed, 26 Jan 2022 16:02:09 GMT, Michael McMahon <micha...@openjdk.org> wrote:

>> Hi,
>> 
>> This change adds Channel Binding Token (CBT) support to HTTPS 
>> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, 
>> Kerberos) authentication scheme. When enabled, the implementation 
>> preemptively includes a CBT with authentication requests over Kerberos. The 
>> feature is enabled as follows:
>> 
>> A system property "jdk.spnego.cbt" is defined which can have the values 
>> "never" (default), which means the feature is disabled, "always", which 
>> means the CBT is included for all https Negotiate authentications, or it can 
>> take the form "domain:a,b.c,*.d.com" which is a comma separated list of 
>> domains/hosts where the feature is enabled, and disabled everywhere else. In 
>> the given example, the CBT would be included in authentication requests for 
>> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its 
>> sub-domains.
>> 
>> A test will be added separately to the implementation.
>> 
>> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
>> 
>> Thanks,
>> Michael
>
> Michael McMahon has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   removed ^M from test

Looks mostly good. Some doubts about catching just any exception 
indiscriminately though.

test/jdk/sun/security/krb5/auto/HttpsCB.java line 120:

> 118: 
> 119:         boolean expected1 = Boolean.parseBoolean(args[0]);
> 120:         boolean expected2 = Boolean.parseBoolean(args[1]);

It might be better for future maintainers and readability if these two 
variables could have better names, and possibly a comment to explain their 
purpose. AFAIU it's the expected result of running with/without CBT - where 
`true` means that the operation should succeed and `false` that it's expected 
to fail with some exception...

test/jdk/sun/security/krb5/auto/HttpsCB.java line 201:

> 199:             return reader.readLine().equals(CONTENT);
> 200:         } catch (Exception e) {
> 201:             return false;

Should we log that we have received the excepted exception here? Shouldn't the 
catch clause only list the exceptions that we are expecting?

-------------

PR: https://git.openjdk.java.net/jdk/pull/7065

Reply via email to