Re: RFR: 8245527: LDAP Channel Binding support for Java GSS/Kerberos [v2]
On Tue, 22 Sep 2020 20:19:21 GMT, Alexey Bakhtin wrote: >> Hi, >> >> Plaese review JDK-8245527 fix which implements LDAP Channel Binding support >> for Java GSS/Kerberos. >> Initial review is available at core-devs: >> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-August/068197.html >> This version removes "tls-unique" CB type from the list of possible channel >> binding types. The only supported type is >> "tls-server-end-point" >> CSR is also updated : https://bugs.openjdk.java.net/browse/JDK-8247311 >> >> Thank you >> Alexey > > Alexey Bakhtin has updated the pull request incrementally with one additional > commit since the last revision: > > 8245527: version.01 Hi Alexey, The latest changes looks good to me. - Marked as reviewed by aefimov (Committer). PR: https://git.openjdk.java.net/jdk/pull/278
Re: RFR: 8245527: LDAP Channel Binding support for Java GSS/Kerberos [v2]
On Tue, 22 Sep 2020 20:19:21 GMT, Alexey Bakhtin wrote: >> Hi, >> >> Plaese review JDK-8245527 fix which implements LDAP Channel Binding support >> for Java GSS/Kerberos. >> Initial review is available at core-devs: >> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-August/068197.html >> This version removes "tls-unique" CB type from the list of possible channel >> binding types. The only supported type is >> "tls-server-end-point" >> CSR is also updated : https://bugs.openjdk.java.net/browse/JDK-8247311 >> >> Thank you >> Alexey > > Alexey Bakhtin has updated the pull request incrementally with one additional > commit since the last revision: > > 8245527: version.01 Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/278
Re: RFR: 8245527: LDAP Channel Binding support for Java GSS/Kerberos [v2]
On Tue, 22 Sep 2020 15:36:24 GMT, Daniel Fuchs wrote:
>> No, It is not used.
>> However, I'd like to leave it as is (it is mentioned in the documentation as
>> unsupported value).
>> Otherwise, TlsChannelBindingType enum will have one element only and should
>> be simplified/removed in all places. In
>> this case, it would be double work to add TlsChannelBindingType enum back in
>> the future if "tls-unique" required. If
>> required I can remove TLS_UNIQUE item, but not remove TlsChannelBindingType
>> enum
>
> I was suggesting to keep TlsChannelBindingType but remove TLS_UNIQUE;
> However, I'm OK to keep things as is: this is an
> internal API. I wonder if it would deserve a comment:
> /**
> * Channel binding on the basis of TLS Finished message
> */
> // TLS_UNIQUE is defined by RFC 5929 but is not supported by the
> current LDAP stack.
> TLS_UNIQUE("tls-unique"),
Thank you. Added suggested comment.
-
PR: https://git.openjdk.java.net/jdk/pull/278
Re: RFR: 8245527: LDAP Channel Binding support for Java GSS/Kerberos [v2]
On Tue, 22 Sep 2020 15:11:57 GMT, Weijun Wang wrote:
>> Alexey Bakhtin has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8245527: version.01
>
> src/jdk.security.jgss/share/classes/com/sun/security/sasl/gsskerb/GssKrb5Client.java
> line 156:
>
>> 154: if (props != null) {
>> 155: // TLS Channel Binding
>> 156: byte[] tlsCB =
>> (byte[])props.get("jdk.internal.sasl.tlschannelbinding");
>
> You can say the name is defined in another class in another module. If we
> really want to rename it one day we will know
> where it's from.
Thank you. Comment is added
> src/java.security.jgss/share/classes/sun/security/jgss/krb5/InitialToken.java
> line 389:
>
>> 387: int acceptorAddressType = getAddrType(acceptorAddress,
>> 388: (channelBinding instanceof TlsChannelBindingImpl)?
>> 389:
>> CHANNEL_BINDING_AF_UNSPEC:CHANNEL_BINDING_AF_NULL_ADDR);
>
> Normally we put a white space around "?" and ":".
Thank you. Fixed.
> src/java.naming/share/classes/com/sun/jndi/ldap/sasl/TlsChannelBinding.java
> line 82:
>
>> 80: /**
>> 81: * Parse value of "com.sun.jndi.ldap.tls.cbtype" property
>> 82: * @param cbType
>
> Please add a `@return` here, esp, about null.
Added @return with comments
> src/java.naming/share/classes/com/sun/jndi/ldap/sasl/TlsChannelBinding.java
> line 137:
>
>> 135: public TlsChannelBindingType getType() {
>> 136: return cbType;
>> 137: }
>
> Add a new line here.
Fixed
-
PR: https://git.openjdk.java.net/jdk/pull/278
Re: RFR: 8245527: LDAP Channel Binding support for Java GSS/Kerberos [v2]
> Hi, > > Plaese review JDK-8245527 fix which implements LDAP Channel Binding support > for Java GSS/Kerberos. > Initial review is available at core-devs: > https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-August/068197.html > This version removes "tls-unique" CB type from the list of possible channel > binding types. The only supported type is > "tls-server-end-point" > CSR is also updated : https://bugs.openjdk.java.net/browse/JDK-8247311 > > Thank you > Alexey Alexey Bakhtin has updated the pull request incrementally with one additional commit since the last revision: 8245527: version.01 - Changes: - all: https://git.openjdk.java.net/jdk/pull/278/files - new: https://git.openjdk.java.net/jdk/pull/278/files/3f4ae08c..8b135f48 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=278&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=278&range=00-01 Stats: 15 lines in 4 files changed: 7 ins; 0 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/278.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/278/head:pull/278 PR: https://git.openjdk.java.net/jdk/pull/278
Re: RFR: 8245527: LDAP Channel Binding support for Java GSS/Kerberos
On Mon, 21 Sep 2020 08:19:28 GMT, Alexey Bakhtin wrote:
> Hi,
>
> Plaese review JDK-8245527 fix which implements LDAP Channel Binding support
> for Java GSS/Kerberos.
> Initial review is available at core-devs:
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-August/068197.html
> This version removes "tls-unique" CB type from the list of possible channel
> binding types. The only supported type is
> "tls-server-end-point"
> CSR is also updated : https://bugs.openjdk.java.net/browse/JDK-8247311
>
> Thank you
> Alexey
I'm mostly OK with the SASL/JGSS part, except for the small nits in this
comment. I'm not an expert on
HandshakeCompletedListener.
src/jdk.security.jgss/share/classes/com/sun/security/sasl/gsskerb/GssKrb5Client.java
line 156:
> 154: if (props != null) {
> 155: // TLS Channel Binding
> 156: byte[] tlsCB =
> (byte[])props.get("jdk.internal.sasl.tlschannelbinding");
You can say the name is defined in another class in another module. If we
really want to rename it one day we will know
where it's from.
src/java.security.jgss/share/classes/sun/security/jgss/krb5/InitialToken.java
line 389:
> 387: int acceptorAddressType = getAddrType(acceptorAddress,
> 388: (channelBinding instanceof TlsChannelBindingImpl)?
> 389:
> CHANNEL_BINDING_AF_UNSPEC:CHANNEL_BINDING_AF_NULL_ADDR);
Normally we put a white space around "?" and ":".
src/java.naming/share/classes/com/sun/jndi/ldap/sasl/TlsChannelBinding.java
line 82:
> 80: /**
> 81: * Parse value of "com.sun.jndi.ldap.tls.cbtype" property
> 82: * @param cbType
Please add a `@return` here, esp, about null.
src/java.naming/share/classes/com/sun/jndi/ldap/sasl/TlsChannelBinding.java
line 137:
> 135: public TlsChannelBindingType getType() {
> 136: return cbType;
> 137: }
Add a new line here.
-
PR: https://git.openjdk.java.net/jdk/pull/278
Re: RFR: 8245527: LDAP Channel Binding support for Java GSS/Kerberos
On Tue, 22 Sep 2020 15:17:23 GMT, Alexey Bakhtin wrote:
>> src/java.naming/share/classes/com/sun/jndi/ldap/sasl/TlsChannelBinding.java
>> line 63:
>>
>>> 61: * Channel binding on the basis of TLS Finished message
>>> 62: */
>>> 63: TLS_UNIQUE("tls-unique"),
>>
>> Is that still used? If not maybe it should be removed?
>
> No, It is not used.
> However, I'd like to leave it as is (it is mentioned in the documentation as
> unsupported value).
> Otherwise, TlsChannelBindingType enum will have one element only and should
> be simplified/removed in all places. In
> this case, it would be double work to add TlsChannelBindingType enum back in
> the future if "tls-unique" required. If
> required I can remove TLS_UNIQUE item, but not remove TlsChannelBindingType
> enum
OK
-
PR: https://git.openjdk.java.net/jdk/pull/278
Re: RFR: 8245527: LDAP Channel Binding support for Java GSS/Kerberos
On Tue, 22 Sep 2020 14:47:35 GMT, Daniel Fuchs wrote:
>> Hi,
>>
>> Plaese review JDK-8245527 fix which implements LDAP Channel Binding support
>> for Java GSS/Kerberos.
>> Initial review is available at core-devs:
>> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-August/068197.html
>> This version removes "tls-unique" CB type from the list of possible channel
>> binding types. The only supported type is
>> "tls-server-end-point"
>> CSR is also updated : https://bugs.openjdk.java.net/browse/JDK-8247311
>>
>> Thank you
>> Alexey
>
> src/java.naming/share/classes/com/sun/jndi/ldap/sasl/TlsChannelBinding.java
> line 63:
>
>> 61: * Channel binding on the basis of TLS Finished message
>> 62: */
>> 63: TLS_UNIQUE("tls-unique"),
>
> Is that still used? If not maybe it should be removed?
No, It is not used.
However, I'd like to leave it as is (it is mentioned in the documentation as
unsupported value).
Otherwise, TlsChannelBindingType enum will have one element only and should be
simplified/removed in all places. In
this case, it would be double work to add TlsChannelBindingType enum back in
the future if "tls-unique" required. If
required I can remove TLS_UNIQUE item, but not remove TlsChannelBindingType enum
-
PR: https://git.openjdk.java.net/jdk/pull/278
Re: RFR: 8245527: LDAP Channel Binding support for Java GSS/Kerberos
On Tue, 22 Sep 2020 14:41:57 GMT, Daniel Fuchs wrote: >> Hi, >> >> Plaese review JDK-8245527 fix which implements LDAP Channel Binding support >> for Java GSS/Kerberos. >> Initial review is available at core-devs: >> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-August/068197.html >> This version removes "tls-unique" CB type from the list of possible channel >> binding types. The only supported type is >> "tls-server-end-point" >> CSR is also updated : https://bugs.openjdk.java.net/browse/JDK-8247311 >> >> Thank you >> Alexey > > src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 994: > >> 992: } >> 993: >> 994: private CompletableFuture tlsHandshakeCompleted = > > Should be `final`? Thank you. Agree. It should be final. - PR: https://git.openjdk.java.net/jdk/pull/278
Re: RFR: 8245527: LDAP Channel Binding support for Java GSS/Kerberos
On Mon, 21 Sep 2020 08:19:28 GMT, Alexey Bakhtin wrote:
> Hi,
>
> Plaese review JDK-8245527 fix which implements LDAP Channel Binding support
> for Java GSS/Kerberos.
> Initial review is available at core-devs:
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-August/068197.html
> This version removes "tls-unique" CB type from the list of possible channel
> binding types. The only supported type is
> "tls-server-end-point"
> CSR is also updated : https://bugs.openjdk.java.net/browse/JDK-8247311
>
> Thank you
> Alexey
src/java.naming/share/classes/com/sun/jndi/ldap/Connection.java line 994:
> 992: }
> 993:
> 994: private CompletableFuture tlsHandshakeCompleted =
Should be `final`?
src/java.naming/share/classes/com/sun/jndi/ldap/sasl/TlsChannelBinding.java
line 63:
> 61: * Channel binding on the basis of TLS Finished message
> 62: */
> 63: TLS_UNIQUE("tls-unique"),
Is that still used? If not maybe it should be removed?
-
PR: https://git.openjdk.java.net/jdk/pull/278
Re: RFR: 8245527: LDAP Channel Binding support for Java GSS/Kerberos
On Mon, 21 Sep 2020 08:19:28 GMT, Alexey Bakhtin wrote: > Hi, > > Plaese review JDK-8245527 fix which implements LDAP Channel Binding support > for Java GSS/Kerberos. > Initial review is available at core-devs: > https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-August/068197.html > This version removes "tls-unique" CB type from the list of possible channel > binding types. The only supported type is > "tls-server-end-point" > CSR is also updated : https://bugs.openjdk.java.net/browse/JDK-8247311 > > Thank you > Alexey Thanks for the PR Alexey! Let me run a last round of testing - and if that comes back clean I'll approve. Please don't integrate until you get a reviewer from security-libs too. best regards, -- daniel - PR: https://git.openjdk.java.net/jdk/pull/278
