Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v13]
On Fri, 28 Jan 2022 16:58:55 GMT, Michael McMahon 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: > > spec update from CSR Marked as reviewed by darcy (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v13]
On Fri, 28 Jan 2022 16:58:55 GMT, Michael McMahon 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: > > spec update from CSR Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v13]
> 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: spec update from CSR - Changes: - all: https://git.openjdk.java.net/jdk/pull/7065/files - new: https://git.openjdk.java.net/jdk/pull/7065/files/59f703da..468e5345 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7065=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7065=11-12 Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/7065.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065 PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v12]
On Thu, 27 Jan 2022 18:05:25 GMT, Michael McMahon 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 with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 15 additional > commits since the last revision: > > - test update > - Merge branch 'master' into spnego > - test update > - removed ^M from test > - Added unit test and comment update > - final review update (pre CSR) > - more updates > - fixed failing test issue and update for latest comments > - Merge branch 'master' into spnego > - added root cause to NamingException > - ... and 5 more: > https://git.openjdk.java.net/jdk/compare/ceb44101...59f703da LGTM! - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v10]
On Thu, 27 Jan 2022 16:47:52 GMT, Daniel Fuchs wrote: >> It's `java.net.SocketException: Unexpected end of file from server`. Does >> not include any CBT words so don't know if it's worth parsing. > > Thanks. Then it would be better to catch only `SocketException` here rather > than `Exception`. I'll make it catch `IOException` - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v12]
> 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 15 additional commits since the last revision: - test update - Merge branch 'master' into spnego - test update - removed ^M from test - Added unit test and comment update - final review update (pre CSR) - more updates - fixed failing test issue and update for latest comments - Merge branch 'master' into spnego - added root cause to NamingException - ... and 5 more: https://git.openjdk.java.net/jdk/compare/35ce454c...59f703da - Changes: - all: https://git.openjdk.java.net/jdk/pull/7065/files - new: https://git.openjdk.java.net/jdk/pull/7065/files/d604ee7f..59f703da Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7065=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7065=10-11 Stats: 4735 lines in 368 files changed: 2835 ins; 809 del; 1091 mod Patch: https://git.openjdk.java.net/jdk/pull/7065.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065 PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v10]
On Wed, 26 Jan 2022 19:00:14 GMT, Weijun Wang wrote:
>> 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?
>
> It's `java.net.SocketException: Unexpected end of file from server`. Does not
> include any CBT words so don't know if it's worth parsing.
Thanks. Then it would be better to catch only `SocketException` here rather
than `Exception`.
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v11]
> 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: test update - Changes: - all: https://git.openjdk.java.net/jdk/pull/7065/files - new: https://git.openjdk.java.net/jdk/pull/7065/files/b44184de..d604ee7f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7065=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7065=09-10 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/7065.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065 PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v10]
On Wed, 26 Jan 2022 16:25:24 GMT, Daniel Fuchs wrote: >> Michael McMahon has updated the pull request incrementally with one >> additional commit since the last revision: >> >> removed ^M from test > > 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... Maybe `expectedCbtUrlResult` and `expectedNormalUrlResult`. - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v10]
On Wed, 26 Jan 2022 16:27:29 GMT, Daniel Fuchs wrote:
>> Michael McMahon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> removed ^M from test
>
> 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?
It's `java.net.SocketException: Unexpected end of file from server`. Does not
include any CBT words so don't know if it's worth parsing.
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v10]
On Wed, 26 Jan 2022 16:02:09 GMT, Michael McMahon 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
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v10]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/7065/files - new: https://git.openjdk.java.net/jdk/pull/7065/files/e5a5a79a..b44184de Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7065=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7065=08-09 Stats: 311 lines in 1 file changed: 0 ins; 0 del; 311 mod Patch: https://git.openjdk.java.net/jdk/pull/7065.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065 PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v9]
> 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: Added unit test and comment update - Changes: - all: https://git.openjdk.java.net/jdk/pull/7065/files - new: https://git.openjdk.java.net/jdk/pull/7065/files/004466ea..e5a5a79a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7065=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7065=07-08 Stats: 312 lines in 2 files changed: 311 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7065.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065 PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v8]
On Tue, 25 Jan 2022 12:47:26 GMT, Michael McMahon wrote: >> src/java.base/share/classes/sun/net/www/http/HttpClient.java line 150: >> >>> 148: * "domain:a,c.d,*.e.f" (sent to host a, or c.d or to the domain >>> e.f and any of its subdomains). This is >>> 149: * a comma separated list of arbitrary length with no white-space >>> allowed. >>> 150: * If enabled (for a particular destination) then SPNEGO >>> authentication requests will include >> >> Previously `Negotiate` was used, now `SPNEGO`? > > "Negotiate" is the name of the HTTP authentication scheme which uses the > SPNEGO mechanism. Possibly makes more sense to refer to Negotiate here. Yes, I know. That's the point. Clearly differentiate between GSS-API mech and HTTP auth scheme. - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v8]
On Tue, 25 Jan 2022 11:34:57 GMT, Michael Osipov wrote: >> Michael McMahon has updated the pull request incrementally with one >> additional commit since the last revision: >> >> final review update (pre CSR) > > src/java.base/share/classes/sun/net/www/http/HttpClient.java line 150: > >> 148: * "domain:a,c.d,*.e.f" (sent to host a, or c.d or to the domain >> e.f and any of its subdomains). This is >> 149: * a comma separated list of arbitrary length with no white-space >> allowed. >> 150: * If enabled (for a particular destination) then SPNEGO >> authentication requests will include > > Previously `Negotiate` was used, now `SPNEGO`? "Negotiate" is the name of the HTTP authentication scheme which uses the SPNEGO mechanism. Possibly makes more sense to refer to Negotiate here. - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v8]
On Tue, 25 Jan 2022 10:30:20 GMT, Michael McMahon 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: > > final review update (pre CSR) src/java.base/share/classes/sun/net/www/http/HttpClient.java line 150: > 148: * "domain:a,c.d,*.e.f" (sent to host a, or c.d or to the domain e.f > and any of its subdomains). This is > 149: * a comma separated list of arbitrary length with no white-space > allowed. > 150: * If enabled (for a particular destination) then SPNEGO > authentication requests will include Previously `Negotiate` was used, now `SPNEGO`? - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v8]
On Tue, 25 Jan 2022 10:30:20 GMT, Michael McMahon 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: > > final review update (pre CSR) The new version LGTM. - Marked as reviewed by dfuchs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v8]
> 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: final review update (pre CSR) - Changes: - all: https://git.openjdk.java.net/jdk/pull/7065/files - new: https://git.openjdk.java.net/jdk/pull/7065/files/0d529f9d..004466ea Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7065=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7065=06-07 Stats: 3 lines in 3 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/7065.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065 PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v7]
On Mon, 24 Jan 2022 22:11:51 GMT, Michael McMahon 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:
>
> more updates
Looks good to me. Only several wording and style suggestions.
I know you are asking SQE to create a security infra test, but I'll see if I
can contribute a regression test. Don't wait for me.
src/java.base/share/classes/java/net/doc-files/net-properties.html line 223:
> 221:
> 222: "never". This is also the default value if the property
> is not set. In this case,
> 223: CBT's are never sent.
Typo, "CBTs"?
src/java.base/share/classes/java/net/doc-files/net-properties.html line 224:
> 222: "never". This is also the default value if the property
> is not set. In this case,
> 223: CBT's are never sent.
> 224: "always". CBTs are sent for all Kerberos authentication
> attempts over HTTPS.
Shall we remove "Kerberos"? Or we can use "Kerberos or Negotiate".
src/java.base/share/classes/sun/net/www/protocol/https/AbstractDelegateHttpsURLConnection.java
line 1:
> 1: /**
This is not a doc comment.
src/java.security.jgss/share/classes/sun/net/www/protocol/http/spnego/NegotiatorImpl.java
line 124:
> 122: try {
> 123: init(hci);
> 124: } catch (GSSException | ChannelBindingException e) {
Two spaces before "e".
-
Marked as reviewed by weijun (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v7]
> 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: more updates - Changes: - all: https://git.openjdk.java.net/jdk/pull/7065/files - new: https://git.openjdk.java.net/jdk/pull/7065/files/ad80dfa2..0d529f9d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7065=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7065=05-06 Stats: 38 lines in 2 files changed: 18 ins; 19 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7065.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065 PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v6]
On Mon, 24 Jan 2022 15:54:01 GMT, Michael McMahon wrote: >> src/java.base/share/classes/sun/security/util/TlsChannelBinding.java line >> 100: >> >>> (failed to retrieve contents of file, check the PR for context) >> I think this method should stay here. Suppose one day the CBT type is >> configurable for HTTPS we'll have to get it back. Of course we will need to >> update the message to avoid talking about LDAP. > > So, where should the two constant Strings go? It doesn't feel like they > belong in java.base since they are JNDI/SASL related, and we can't have a > method in `java.base` depending on code in other modules? The 2 strings should be on the LDAP side. This method does not really depend on the strings except for mentioning one in the exception message. We can just rewrite it into `"Illegal channel binding type: " + cbType`. - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v6]
On Mon, 24 Jan 2022 15:23:44 GMT, Weijun Wang wrote: >> Michael McMahon has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains eight additional >> commits since the last revision: >> >> - fixed failing test issue and update for latest comments >> - Merge branch 'master' into spnego >> - added root cause to NamingException >> - more tidy-up >> - removed sasl module dependency and added SaslException cause >> - changes after first review round >> - cleanup but still no test. Will be added in closed repo >> - First version of fix. No test and feature enabled always. > > src/java.base/share/classes/sun/security/util/TlsChannelBinding.java line 100: > >> (failed to retrieve contents of file, check the PR for context) > I think this method should stay here. Suppose one day the CBT type is > configurable for HTTPS we'll have to get it back. Of course we will need to > update the message to avoid talking about LDAP. So, where should the two constant Strings go? It doesn't feel like they belong in java.base since they are JNDI/SASL related, and we can't have a method in `java.base` depending on code in other modules? - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v6]
On Mon, 24 Jan 2022 13:54:12 GMT, Daniel Fuchs wrote:
>> Michael McMahon has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains eight additional
>> commits since the last revision:
>>
>> - fixed failing test issue and update for latest comments
>> - Merge branch 'master' into spnego
>> - added root cause to NamingException
>> - more tidy-up
>> - removed sasl module dependency and added SaslException cause
>> - changes after first review round
>> - cleanup but still no test. Will be added in closed repo
>> - First version of fix. No test and feature enabled always.
>
> src/java.naming/share/classes/com/sun/jndi/ldap/sasl/LdapSasl.java line 260:
>
>> 258: * @throws ChannelBindingException
>> 259: */
>> 260: private static TlsChannelBindingType parseType(String cbType)
>> throws ChannelBindingException {
>
> Maybe this method could throw NamingException directly now? That would avoid
> wrapping CBE into NamingException?
My opinion is this method should be put back.
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v4]
On Fri, 21 Jan 2022 15:40:16 GMT, Daniel Fuchs wrote:
>> Michael McMahon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> more tidy-up
>
> src/java.naming/share/classes/com/sun/jndi/ldap/sasl/LdapSasl.java line 144:
>
>> 142: } catch (ChannelBindingException e) {
>> 143: throw new NamingException(e.getMessage());
>> 144: }
>
> Should we call initCause here and above? I see that we do call initCause in
> NegotiatorImpl.java.
> On the one hand it's better for diagnostic. On the other hand it exposes a
> module-internal exception class which is not great. Or maybe we should set
> the cause of the CBE as the cause of NamingException.
As long as the spec has not dictated what the cause should be, I don't care if
the exception type is internal or not.
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v6]
On Mon, 24 Jan 2022 13:36:47 GMT, Michael McMahon 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 with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains eight additional > commits since the last revision: > > - fixed failing test issue and update for latest comments > - Merge branch 'master' into spnego > - added root cause to NamingException > - more tidy-up > - removed sasl module dependency and added SaslException cause > - changes after first review round > - cleanup but still no test. Will be added in closed repo > - First version of fix. No test and feature enabled always. src/java.base/share/classes/sun/security/util/TlsChannelBinding.java line 100: > (failed to retrieve contents of file, check the PR for context) I think this method should stay here. Suppose one day the CBT type is configurable for HTTPS we'll have to get it back. Of course we will need to update the message to avoid talking about LDAP. - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v6]
On Mon, 24 Jan 2022 13:36:47 GMT, Michael McMahon 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 with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains eight additional
> commits since the last revision:
>
> - fixed failing test issue and update for latest comments
> - Merge branch 'master' into spnego
> - added root cause to NamingException
> - more tidy-up
> - removed sasl module dependency and added SaslException cause
> - changes after first review round
> - cleanup but still no test. Will be added in closed repo
> - First version of fix. No test and feature enabled always.
src/java.naming/share/classes/com/sun/jndi/ldap/sasl/LdapSasl.java line 260:
> 258: * @throws ChannelBindingException
> 259: */
> 260: private static TlsChannelBindingType parseType(String cbType) throws
> ChannelBindingException {
Maybe this method could throw NamingException directly now? That would avoid
wrapping CBE into NamingException?
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v6]
> 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision: - fixed failing test issue and update for latest comments - Merge branch 'master' into spnego - added root cause to NamingException - more tidy-up - removed sasl module dependency and added SaslException cause - changes after first review round - cleanup but still no test. Will be added in closed repo - First version of fix. No test and feature enabled always. - Changes: - all: https://git.openjdk.java.net/jdk/pull/7065/files - new: https://git.openjdk.java.net/jdk/pull/7065/files/058c3830..ad80dfa2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7065=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7065=04-05 Stats: 17855 lines in 614 files changed: 12290 ins; 2870 del; 2695 mod Patch: https://git.openjdk.java.net/jdk/pull/7065.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065 PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v5]
On Fri, 21 Jan 2022 19:48:02 GMT, Weijun Wang wrote:
>> Michael McMahon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> added root cause to NamingException
>
> src/java.base/share/classes/java/net/doc-files/net-properties.html line 220:
>
>> 218: This controls the generation and sending of TLS channel binding
>> tokens (CBT) when Kerberos
>> 219: or the Negotiate authentication scheme using Kerberos are
>> employed over HTTPS with
>> 220: {@code HttpsURLConnection}. There are three possible
>> settings:
>
> You can probably mention here that the 'tls-server-end-point' Channel Binding
> Type defined in RFC 5929 is used.
I've updated this and moved the two properties to LdapSasl where they are used.
Also, the test that was failing before needed some further updates.
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v5]
On Fri, 21 Jan 2022 16:02:29 GMT, Michael McMahon 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: > > added root cause to NamingException Please move // TLS channel binding type property public static final String CHANNEL_BINDING_TYPE = "com.sun.jndi.ldap.tls.cbtype"; // internal TLS channel binding property public static final String CHANNEL_BINDING = "jdk.internal.sasl.tlschannelbinding"; outside of the `TlsChannelBinding` class. - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v5]
On Fri, 21 Jan 2022 16:02:29 GMT, Michael McMahon 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:
>
> added root cause to NamingException
src/java.base/share/classes/java/net/doc-files/net-properties.html line 220:
> 218: This controls the generation and sending of TLS channel binding tokens
> (CBT) when Kerberos
> 219: or the Negotiate authentication scheme using Kerberos are
> employed over HTTPS with
> 220: {@code HttpsURLConnection}. There are three possible
> settings:
You can probably mention here that the 'tls-server-end-point' Channel Binding
Type defined in RFC 5929 is used.
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v5]
On Fri, 21 Jan 2022 16:02:29 GMT, Michael McMahon 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: > > added root cause to NamingException Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v3]
On Fri, 21 Jan 2022 15:51:10 GMT, Michael McMahon wrote: >> `NamingException` has `setRootCause()`. Why not use that? I use that one too >> and full stack is retained. > > Yes, I can do that. Though it will cause the existing LDAP channel binding > test to fail which is checking for an empty root cause. That is checking > unspecified behavior and I can change it to check for a > `ChannelBindingException` as root cause. So long as we are okay having a non > public exception type as the root cause, it's probably helpful to have the > full stack there. Yes please retain as much information as possible. - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v5]
> 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: added root cause to NamingException - Changes: - all: https://git.openjdk.java.net/jdk/pull/7065/files - new: https://git.openjdk.java.net/jdk/pull/7065/files/c9975fd1..058c3830 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7065=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7065=03-04 Stats: 16 lines in 2 files changed: 8 ins; 4 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/7065.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065 PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v3]
On Fri, 21 Jan 2022 13:39:06 GMT, Michael Osipov wrote: >> Actually, it turns out I should be throwing `NamingException` here. That is >> what was being thrown by `TlsChannelBinding.parseType` before and an >> existing test was expecting that. NamingException only takes a String >> message. So, there won't be a root cause exception. > > `NamingException` has `setRootCause()`. Why not use that? I use that one too > and full stack is retained. Yes, I can do that. Though it will cause the existing LDAP channel binding test to fail which is checking for an empty root cause. That is checking unspecified behavior and I can change it to check for a `ChannelBindingException` as root cause. So long as we are okay having a non public exception type as the root cause, it's probably helpful to have the full stack there. - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v4]
On Fri, 21 Jan 2022 15:26:33 GMT, Michael McMahon 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:
>
> more tidy-up
Marked as reviewed by dfuchs (Reviewer).
src/java.naming/share/classes/com/sun/jndi/ldap/sasl/LdapSasl.java line 144:
> 142: } catch (ChannelBindingException e) {
> 143: throw new NamingException(e.getMessage());
> 144: }
Should we call initCause here and above? I see that we do call initCause in
NegotiatorImpl.java.
On the one hand it's better for diagnostic. On the other hand it exposes a
module-internal exception class which is not great. Or maybe we should set the
cause of the CBE as the cause of NamingException.
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v4]
On Fri, 21 Jan 2022 15:26:33 GMT, Michael McMahon 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:
>
> more tidy-up
src/java.base/share/classes/sun/net/www/http/HttpClient.java line 191:
> 189: } else {
> 190: logError("Unexpected value for \"jdk.https.negotiate.cbt\"
> system property");
> 191: return "never";
Much easier to review now :-)
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v3]
On Fri, 21 Jan 2022 15:08:58 GMT, Michael McMahon wrote: >> It was being handled elsewhere as "never". But, I agree it would be clearer >> to normalise it to "never" here. > > Sorry, I should have checked back to the source rather than the snippet > quoted. The problem is that the logError call is in the wrong place. It > should be before line 186. Though some other adjustments are also required Ah! Yes - I was also bitten again by the negation in the `if` too. The presence of `logError` in the body of the `if` will make it clearer :-) - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v4]
> 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: more tidy-up - Changes: - all: https://git.openjdk.java.net/jdk/pull/7065/files - new: https://git.openjdk.java.net/jdk/pull/7065/files/fd56b5e3..c9975fd1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7065=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7065=02-03 Stats: 12 lines in 4 files changed: 5 ins; 1 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/7065.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065 PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v3]
On Fri, 21 Jan 2022 13:38:08 GMT, Michael McMahon wrote:
>> src/java.base/share/classes/sun/net/www/http/HttpClient.java line 189:
>>
>>> 187: } else {
>>> 188: logError("Unexpected value for \"jdk.https.negotiate.cbt\"
>>> system property");
>>> 189: return s;
>>
>> Should this return either "always" or "never" instead? It seems that junk
>> values will be treated as "always". It would be better to make it clear here.
>
> It was being handled elsewhere as "never". But, I agree it would be clearer
> to normalise it to "never" here.
Sorry, I should have checked back to the source rather than the snippet quoted.
The problem is that the logError call is in the wrong place. It should be
before line 186.
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v3]
On Fri, 21 Jan 2022 13:35:53 GMT, Michael McMahon wrote:
>> src/java.naming/share/classes/com/sun/jndi/ldap/sasl/LdapSasl.java line 133:
>>
>>> 131:
>>> (String)env.get(TlsChannelBinding.CHANNEL_BINDING_TYPE));
>>> 132: } catch (ChannelBindingException e) {
>>> 133: throw new SaslException(e.getMessage(), e);
>>
>> Why not ust pass the exception if the API allows? This looks like message
>> duplication.
>
> Actually, it turns out I should be throwing `NamingException` here. That is
> what was being thrown by `TlsChannelBinding.parseType` before and an existing
> test was expecting that. NamingException only takes a String message. So,
> there won't be a root cause exception.
`NamingException` has `setRootCause()`. Why not use that? I use that one too
and full stack is retained.
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v3]
On Thu, 20 Jan 2022 11:16:16 GMT, Daniel Fuchs wrote:
>> Michael McMahon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> removed sasl module dependency and added SaslException cause
>
> src/java.base/share/classes/sun/security/util/ChannelBindingException.java
> line 31:
>
>> 29: * Thrown by TlsChannelBinding if an error occurs
>> 30: */
>> 31: public class ChannelBindingException extends Exception {
>
> Should this extend `GeneralSecurityException` instead? Or should we just
> remove this class and throw plain `GeneralSecurityException` in
> `TlsChannelBinding` ?
I think a distinct exception is necessary. I don't have a strong opinion on
whether it should extend GeneralSecurityException.
> src/java.naming/share/classes/com/sun/jndi/ldap/sasl/LdapSasl.java line 143:
>
>> 141: tlsCB = TlsChannelBinding.create(cert);
>> 142: } catch (ChannelBindingException e) {
>> 143: throw new SaslException(e.getMessage());
>
> Why is there a difference compared to line 133?
Right, that was a mistake.
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v3]
On Thu, 20 Jan 2022 11:14:40 GMT, Michael Osipov wrote:
>> Michael McMahon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> removed sasl module dependency and added SaslException cause
>
> src/java.naming/share/classes/com/sun/jndi/ldap/sasl/LdapSasl.java line 133:
>
>> 131:
>> (String)env.get(TlsChannelBinding.CHANNEL_BINDING_TYPE));
>> 132: } catch (ChannelBindingException e) {
>> 133: throw new SaslException(e.getMessage(), e);
>
> Why not ust pass the exception if the API allows? This looks like message
> duplication.
Actually, it turns out I should be throwing `NamingException` here. That is
what was being thrown by `TlsChannelBinding.parseType` before and an existing
test was expecting that. NamingException only takes a String message. So, there
won't be a root cause exception.
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v3]
On Thu, 20 Jan 2022 11:04:18 GMT, Daniel Fuchs wrote:
>> Michael McMahon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> removed sasl module dependency and added SaslException cause
>
> src/java.base/share/classes/java/net/doc-files/net-properties.html line 220:
>
>> 218: This controls the generation and sending of TLS channel binding
>> tokens (CBT) when Kerberos
>> 219: or the Negotiate authentication scheme using Kerberos are
>> employed over HTTPS with
>> 220: {@code HttpURLConnection}. There are three possible
>> settings:
>
> Should it be `{@code HttpsURLConnection}`?
> (BTW - can we use {@code } here ? Would be worth checking the generated doc)
Right HttpsURLConnection is better. {@code} works here.
> src/java.base/share/classes/sun/net/www/http/HttpClient.java line 189:
>
>> 187: } else {
>> 188: logError("Unexpected value for \"jdk.https.negotiate.cbt\"
>> system property");
>> 189: return s;
>
> Should this return either "always" or "never" instead? It seems that junk
> values will be treated as "always". It would be better to make it clear here.
It was being handled elsewhere as "never". But, I agree it would be clearer to
normalise it to "never" here.
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v3]
On Thu, 20 Jan 2022 10:58:27 GMT, Michael McMahon 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 sasl module dependency and added SaslException cause
src/java.base/share/classes/java/net/doc-files/net-properties.html line 220:
> 218: This controls the generation and sending of TLS channel binding tokens
> (CBT) when Kerberos
> 219: or the Negotiate authentication scheme using Kerberos are
> employed over HTTPS with
> 220: {@code HttpURLConnection}. There are three possible settings:
Should it be `{@code HttpsURLConnection}`?
(BTW - can we use {@code } here ? Would be worth checking the generated doc)
src/java.base/share/classes/sun/net/www/http/HttpClient.java line 189:
> 187: } else {
> 188: logError("Unexpected value for \"jdk.https.negotiate.cbt\"
> system property");
> 189: return s;
Should this return either "always" or "never" instead? It seems that junk
values will be treated as "always". It would be better to make it clear here.
src/java.base/share/classes/sun/security/util/ChannelBindingException.java line
31:
> 29: * Thrown by TlsChannelBinding if an error occurs
> 30: */
> 31: public class ChannelBindingException extends Exception {
Should this extend `GeneralSecurityException` instead? Or should we just remove
this class and throw plain `GeneralSecurityException` in `TlsChannelBinding` ?
src/java.naming/share/classes/com/sun/jndi/ldap/sasl/LdapSasl.java line 143:
> 141: tlsCB = TlsChannelBinding.create(cert);
> 142: } catch (ChannelBindingException e) {
> 143: throw new SaslException(e.getMessage());
Why is there a difference compared to line 133?
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v3]
On Thu, 20 Jan 2022 10:58:27 GMT, Michael McMahon 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 sasl module dependency and added SaslException cause
src/java.naming/share/classes/com/sun/jndi/ldap/sasl/LdapSasl.java line 133:
> 131:
> (String)env.get(TlsChannelBinding.CHANNEL_BINDING_TYPE));
> 132: } catch (ChannelBindingException e) {
> 133: throw new SaslException(e.getMessage(), e);
Why not ust pass the exception if the API allows? This looks like message
duplication.
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v3]
> 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 sasl module dependency and added SaslException cause - Changes: - all: https://git.openjdk.java.net/jdk/pull/7065/files - new: https://git.openjdk.java.net/jdk/pull/7065/files/f2ee58ec..fd56b5e3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7065=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7065=01-02 Stats: 2 lines in 2 files changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7065.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065 PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v2]
On Wed, 19 Jan 2022 22:25:43 GMT, Weijun Wang wrote:
>> Michael McMahon has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> changes after first review round
>
> src/java.naming/share/classes/com/sun/jndi/ldap/sasl/LdapSasl.java line 133:
>
>> 131:
>> (String)env.get(TlsChannelBinding.CHANNEL_BINDING_TYPE));
>> 132: } catch (ChannelBindingException e) {
>> 133: throw new SaslException(e.getMessage());
>
> How about setting `e` as cause of new exception? In `TlsChannelBinding.java`
> the when the original exception was thrown (the 2nd throws) there was a cause.
Agreed.
> src/java.security.jgss/share/classes/module-info.java line 36:
>
>> 34: module java.security.jgss {
>> 35: requires java.naming;
>> 36: requires java.security.sasl;
>
> Can this be removed now?
Yes, well spotted!
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v2]
On Wed, 19 Jan 2022 22:20:47 GMT, Michael McMahon 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:
>
> changes after first review round
src/java.naming/share/classes/com/sun/jndi/ldap/sasl/LdapSasl.java line 133:
> 131:
> (String)env.get(TlsChannelBinding.CHANNEL_BINDING_TYPE));
> 132: } catch (ChannelBindingException e) {
> 133: throw new SaslException(e.getMessage());
How about setting `e` as cause of new exception? In `TlsChannelBinding.java`
the when the original exception was thrown (the 2nd throws) there was a cause.
src/java.security.jgss/share/classes/module-info.java line 36:
> 34: module java.security.jgss {
> 35: requires java.naming;
> 36: requires java.security.sasl;
Can this be removed now?
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v2]
> 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: changes after first review round - Changes: - all: https://git.openjdk.java.net/jdk/pull/7065/files - new: https://git.openjdk.java.net/jdk/pull/7065/files/853ed4db..f2ee58ec Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7065=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7065=00-01 Stats: 111 lines in 7 files changed: 88 ins; 5 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/7065.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065 PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Fri, 14 Jan 2022 15:06:12 GMT, Daniel Fuchs wrote: > Have you been able to test this on a specific setup? Would be good to hear > from @msheppar too. I have tested it with the server setup by Prajwal. Security SQE are looking into configuring a server with a similar setup which can be tested with an infra test. - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Wed, 19 Jan 2022 15:36:16 GMT, Michael McMahon wrote: >>> It's actually a purely system property rather than a Net property at the >>> moment (same as the other spnego ones). Maybe, I should convert them all to >>> net properties, so they can be documented/set in that file? >> >> AFAICS this file documents properties used by the networking stack - not >> necessarily net properties (e.g. java.net.preferIPv6Addresses is documented >> there but AFAICT it is a plain system property) > > Okay, good idea to document it in the properties file. Also, I think > "jdk.https.tls.cbt" is a reasonable name for the property. Sorry, on reflection, something like "jdk.https.negotiate.cbt" might be better. There's no need for tls and https in the name and "negotiate" or "spnego" should be in it, but "negotiate" is probably better - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Sat, 15 Jan 2022 14:02:15 GMT, Michael Osipov wrote: >> I suggest moving the `TlsChannelBinding` class into >> `java.base/sun.security.util` since it's not only used by LDAP anymore. It's >> even not restricted to GSS-API. According to >> https://www.rfc-editor.org/rfc/rfc5056, "Although inspired by and derived >> from the GSS-API, the notion of channel binding described herein is not at >> all limited to use by GSS-API applications". >> >> If so, you might need to modify the types of exceptions thrown in the class, >> and move the 2 final strings to some other class inside `java.security.sasl`. > > Seems like `com.sun.jndi.ldap.sasl.TlsChannelBinding` is not misplaced Okay, I'll look at doing this refactoring. - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Mon, 17 Jan 2022 13:49:35 GMT, Daniel Fuchs wrote: >> I vote for "jdk.https.tls.cbt" > >> It's actually a purely system property rather than a Net property at the >> moment (same as the other spnego ones). Maybe, I should convert them all to >> net properties, so they can be documented/set in that file? > > AFAICS this file documents properties used by the networking task - not > necessarily net properties (e.g. java.net.preferIPv6Addresses is documented > there but AFAICT it is a plain system property) Okay, good idea to document it in the properties file. Also, I think "jdk.https.tls.cbt" is a reasonable name for the property. - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Mon, 17 Jan 2022 13:44:06 GMT, Daniel Fuchs wrote: >> Shall we log a message if the value is not one of the 3 forms? > > Usually malformed values are just ignored - and the property takes its > default value. But yes - s.n.w.h.HttpClient has a logger so it wouldn't be > much effort to log it as a DEBUG trace for better diagnostic. Yes, I will log it using the same debug/logging mechanism already in the same source file.. - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Mon, 17 Jan 2022 06:32:13 GMT, Prasadrao Koppula wrote: >> This system property should only be used for TLS, and the CBT can be used in >> both the SPNEGO mechanism and the Kerberos 5 mechanism. Therefore I suggest >> the name should probably contain "tls" (or maybe "https") and "negotiate". >> >> BTW, will you reuse this system property if we decide to support CBT in NTLM >> as well? > > I vote for "jdk.https.tls.cbt" > It's actually a purely system property rather than a Net property at the > moment (same as the other spnego ones). Maybe, I should convert them all to > net properties, so they can be documented/set in that file? AFAICS this file documents properties used by the networking task - not necessarily net properties (e.g. java.net.preferIPv6Addresses is documented there but AFAICT it is a plain system property) - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Sat, 15 Jan 2022 00:49:05 GMT, Weijun Wang wrote:
>> Argh - you're right I missed the fact that the 3 expressions where included
>> in parenthesis. I read it as
>>
>> ! (s.equals("always")) || ...
>
> Shall we log a message if the value is not one of the 3 forms?
Usually malformed values are just ignored - and the property takes its default
value. But yes - s.n.w.h.HttpClient has a logger so it wouldn't be much effort
to log it as a DEBUG trace for better diagnostic.
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Thu, 13 Jan 2022 12:10:11 GMT, Michael McMahon 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 Add Release note and Doc subtasks to the JBS - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Sat, 15 Jan 2022 00:44:08 GMT, Weijun Wang wrote: >> It's actually a purely system property rather than a Net property at the >> moment (same as the other spnego ones). Maybe, I should convert them all to >> net properties, so they can be documented/set in that file? > > This system property should only be used for TLS, and the CBT can be used in > both the SPNEGO mechanism and the Kerberos 5 mechanism. Therefore I suggest > the name should probably contain "tls" (or maybe "https") and "negotiate". > > BTW, will you reuse this system property if we decide to support CBT in NTLM > as well? I vote for "jdk.https.tls.cbt" - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Sat, 15 Jan 2022 00:44:08 GMT, Weijun Wang wrote: >> It's actually a purely system property rather than a Net property at the >> moment (same as the other spnego ones). Maybe, I should convert them all to >> net properties, so they can be documented/set in that file? > > This system property should only be used for TLS, and the CBT can be used in > both the SPNEGO mechanism and the Kerberos 5 mechanism. Therefore I suggest > the name should probably contain "tls" (or maybe "https") and "negotiate". > > BTW, will you reuse this system property if we decide to support CBT in NTLM > as well? I vote for "jdk.https.tls.cbt" - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Sat, 15 Jan 2022 00:23:31 GMT, Weijun Wang wrote: >> Yes. I would like the security team to validate this. > > I suggest moving the `TlsChannelBinding` class into > `java.base/sun.security.util` since it's not only used by LDAP anymore. It's > even not restricted to GSS-API. According to > https://www.rfc-editor.org/rfc/rfc5056, "Although inspired by and derived > from the GSS-API, the notion of channel binding described herein is not at > all limited to use by GSS-API applications". > > If so, you might need to modify the types of exceptions thrown in the class, > and move the 2 final strings to some other class inside `java.security.sasl`. Seems like `com.sun.jndi.ldap.sasl.TlsChannelBinding` is not misplaced - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Fri, 14 Jan 2022 10:18:50 GMT, Daniel Fuchs wrote:
>> This is what was intended (equivalent)
>>
>> `if (s ==null || (s!="always" && s!="never" && !s.startsWith("domain")))`
>
> Argh - you're right I missed the fact that the 3 expressions where included
> in parenthesis. I read it as
>
> ! (s.equals("always")) || ...
Shall we log a message if the value is not one of the 3 forms?
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Fri, 14 Jan 2022 18:40:41 GMT, Michael McMahon wrote: >> src/java.base/share/classes/sun/net/www/http/HttpClient.java line 152: >> >>> 150: * If enabled (for a particular destination) then SPNEGO >>> authentication requests will include >>> 151: * a channel binding token for the destination server. The default >>> behavior and setting for the >>> 152: * property is "never" >> >> Maybe this description should be added to >> `src/java.base//share/classes/java/net/doc-files/net-properties.html` too? > > It's actually a purely system property rather than a Net property at the > moment (same as the other spnego ones). Maybe, I should convert them all to > net properties, so they can be documented/set in that file? This system property should only be used for TLS, and the CBT can be used in both the SPNEGO mechanism and the Kerberos 5 mechanism. Therefore I suggest the name should probably contain "tls" (or maybe "https") and "negotiate". BTW, will you reuse this system property if we decide to support CBT in NTLM as well? - PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Fri, 14 Jan 2022 18:42:08 GMT, Michael McMahon wrote:
>> src/java.security.jgss/share/classes/module-info.java line 36:
>>
>>> 34: module java.security.jgss {
>>> 35: requires java.naming;
>>> 36: requires java.security.sasl;
>>
>> Someone from security-dev should probably review this and validate that this
>> is OK. I'm also a bit uncomfortable that we require a class from
>> `com.sun.jndi.ldap.sasl` even though `java.naming` is already required by
>> `java.security.jgss` - so maybe this is OK.
>
> Yes. I would like the security team to validate this.
I suggest moving the `TlsChannelBinding` class into
`java.base/sun.security.util` since it's not only used by LDAP anymore. You
might need to modify the types of exceptions thrown in the class and move the 2
final strings to some other class inside `java.security.sasl`.
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Fri, 14 Jan 2022 14:52:13 GMT, Daniel Fuchs 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
>
> src/java.base/share/classes/sun/net/www/http/HttpClient.java line 152:
>
>> 150: * If enabled (for a particular destination) then SPNEGO
>> authentication requests will include
>> 151: * a channel binding token for the destination server. The default
>> behavior and setting for the
>> 152: * property is "never"
>
> Maybe this description should be added to
> `src/java.base//share/classes/java/net/doc-files/net-properties.html` too?
It's actually a purely system property rather than a Net property at the moment
(same as the other spnego ones). Maybe, I should convert them all to net
properties, so they can be documented/set in that file?
> src/java.security.jgss/share/classes/module-info.java line 36:
>
>> 34: module java.security.jgss {
>> 35: requires java.naming;
>> 36: requires java.security.sasl;
>
> Someone from security-dev should probably review this and validate that this
> is OK. I'm also a bit uncomfortable that we require a class from
> `com.sun.jndi.ldap.sasl` even though `java.naming` is already required by
> `java.security.jgss` - so maybe this is OK.
Yes. I would like the security team to validate this.
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Thu, 13 Jan 2022 12:10:11 GMT, Michael McMahon 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
Have you been able to test this on a specific setup?
Would be good to hear from @msheppar too.
src/java.base/share/classes/sun/net/www/http/HttpClient.java line 152:
> 150: * If enabled (for a particular destination) then SPNEGO
> authentication requests will include
> 151: * a channel binding token for the destination server. The default
> behavior and setting for the
> 152: * property is "never"
Maybe this description should be added to
`src/java.base//share/classes/java/net/doc-files/net-properties.html` too?
src/java.security.jgss/share/classes/module-info.java line 36:
> 34: module java.security.jgss {
> 35: requires java.naming;
> 36: requires java.security.sasl;
Someone from security-dev should probably review this and validate that this is
OK. I'm also a bit uncomfortable that we require a class from
`com.sun.jndi.ldap.sasl` even though `java.naming` is already required by
`java.security.jgss` - so maybe this is OK.
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Thu, 13 Jan 2022 18:18:24 GMT, Daniel Fuchs 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
>
> src/java.base/share/classes/sun/net/www/http/HttpClient.java line 180:
>
>> 178: static String normalizeCBT(String s) {
>> 179: if (s == null || ! (s.equals("always") ||
>> 180: s.equals("never") || s.startsWith("domain:"))) {
>
> I guess there's a `!` missing in front of `s.startsWith("domain:")` here?
This is what was intended (equivalent)
`if (s ==null || (s!="always" && s!="never" && !s.startsWith("domain")))`
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Fri, 14 Jan 2022 10:03:37 GMT, Michael McMahon wrote:
>> src/java.base/share/classes/sun/net/www/http/HttpClient.java line 180:
>>
>>> 178: static String normalizeCBT(String s) {
>>> 179: if (s == null || ! (s.equals("always") ||
>>> 180: s.equals("never") || s.startsWith("domain:"))) {
>>
>> I guess there's a `!` missing in front of `s.startsWith("domain:")` here?
>
> This is what was intended (equivalent)
>
> `if (s ==null || (s!="always" && s!="never" && !s.startsWith("domain")))`
Argh - you're right I missed the fact that the 3 expressions where included in
parenthesis. I read it as
! (s.equals("always")) || ...
-
PR: https://git.openjdk.java.net/jdk/pull/7065
Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
On Thu, 13 Jan 2022 12:10:11 GMT, Michael McMahon 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
src/java.base/share/classes/sun/net/www/http/HttpClient.java line 180:
> 178: static String normalizeCBT(String s) {
> 179: if (s == null || ! (s.equals("always") ||
> 180: s.equals("never") || s.startsWith("domain:"))) {
I guess there's a `!` missing in front of `s.startsWith("domain:")` here?
-
PR: https://git.openjdk.java.net/jdk/pull/7065
RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos
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 - Commit messages: - cleanup but still no test. Will be added in closed repo - First version of fix. No test and feature enabled always. Changes: https://git.openjdk.java.net/jdk/pull/7065/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7065=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8279842 Stats: 149 lines in 7 files changed: 143 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/7065.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7065/head:pull/7065 PR: https://git.openjdk.java.net/jdk/pull/7065
