<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>I am also concerned about the changes in GSSLibStub.c about the
default value being <span class="changed">GSS_C_AF_UNSPEC</span><span
class="changed"> instead of GSS_C_AF_NULLADDR (</span><span
class="changed">line 194-195).</span></p>
<p><span class="changed">Can you try and see if Window works with </span><span
class="changed"><span class="changed">GSS_C_AF_NULLADDR? If yes,
I'd prefer to not changing the default value for addresstype
for the same reason which Michael has already stated.</span></span></p>
<span class="changed"><span class="changed">Thanks,</span></span><br>
<span class="changed"><span class="changed">Valerie</span></span><span
class="changed"><br>
</span>
<pre><span class="changed">
</span></pre>
<p><span class="changed"></span></p>
<div class="moz-cite-prefix">On 5/25/2020 8:33 AM, Alexey Bakhtin
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:90F01C9D-9756-4BFB-97EE-7354050F1AF3@azul.com">
<pre class="moz-quote-pre" wrap="">Hello Michael, Thomas,
Thank you a lot for review and suggestions.
I’ve fixed most of the issues except of fundamental one
I need more time to evaluate suggested usage of UnspecEmptyInetAddress subtype.
Updated webrev is available at: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v1/">http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v1/</a>
Also, please see my comments below.
Regards
Alexey
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">On 24 May 2020, at 02:38, Michael Osipov <a class="moz-txt-link-rfc2396E" href="mailto:1983-01-06@gmx.net"><1983-01-06@gmx.net></a> wrote:
Am 2020-05-21 um 09:35 schrieb Alexey Bakhtin:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Hello,
Could you please review the following patch:
JBS: <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8245527">https://bugs.openjdk.java.net/browse/JDK-8245527</a>
Webrev: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v0/">http://cr.openjdk.java.net/~abakhtin/8245527/webrev.v0/</a>
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Let's go through your changes statically:
* The JIRA issue title has a typo
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Thank you. Fixed in Jira
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">* The word "cannot" is incorrectly spelled throughout all exception messages
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Fixed from “can not” to “cannot"
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ if (cbTypeProp.equals(TlsChannelBindingType.TLS_UNIQUE.getName())) {
+ throw new UnsupportedOperationException("LdapCtx: " +
+ TlsChannelBindingType.TLS_UNIQUE.getName() + " type is not supported");
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
"LdapCtx: " is redundant because the stacktrace will contain the class
name already. A better message would be: "Channel binding type '%s' is
not supported". Not just the plain value.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Exception message is corrected
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ } else if (cbTypeProp.equals(TlsChannelBindingType.TLS_SERVER_END_POINT.getName())) {
+ if (connectTimeout == -1)
+ throw new IllegalArgumentException(CHANNEL_BINDING_TYPE + " property requires " +
+ CONNECT_TIMEOUT + " property is set.");
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
* Same here with the message
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">Not sure, What’s wrong with the message ?
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">* The IAE is wrong because passed value is correct, but leads to an
invalid state because connection timeout is -1. You need an
IllegalStateException here.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Thank you. You are right again. Changed to IllegalStateException
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
Stupid question: how can one create a GSS security context when the TLS
security context has not been established yet?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
This logic already existed here. It could be a reason for it and I don’t want change it without strong purpose.
The only changes here is to prevent double setting of channel binding data.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">--- old/src/java.security.jgss/share/classes/sun/security/jgss/GSSContextImpl.java 2020-05-18 19:39:46.000000000 +0300
+++ new/src/java.security.jgss/share/classes/sun/security/jgss/GSSContextImpl.java 2020-05-18 19:39:46.000000000 +0300
@@ -531,9 +531,12 @@
public void setChannelBinding(ChannelBinding channelBindings)
throws GSSException {
- if (mechCtxt == null)
+ if (mechCtxt == null) {
+ if (this.channelBindings != null) {
+ throw new GSSException(GSSException.BAD_BINDINGS);
+ }
this.channelBindings = channelBindings;
-
+ }
}
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I don't understand the purpose of this hunk. Is this safeguard to set
bindings only once?
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> private static final int CHANNEL_BINDING_AF_INET = 2;
private static final int CHANNEL_BINDING_AF_INET6 = 24;
private static final int CHANNEL_BINDING_AF_NULL_ADDR = 255;
+ private static final int CHANNEL_BINDING_AF_UNSPEC = 0;
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
This should sort from 0 to 255 and not at the end.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
OK. Moved to the top.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> private int getAddrType(InetAddress addr) {
- int addressType = CHANNEL_BINDING_AF_NULL_ADDR;
+ int addressType = CHANNEL_BINDING_AF_UNSPEC;
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> // initialize addrtype in CB first
- cb->initiator_addrtype = GSS_C_AF_NULLADDR;
- cb->acceptor_addrtype = GSS_C_AF_NULLADDR;
+ cb->initiator_addrtype = GSS_C_AF_UNSPEC;
+ cb->acceptor_addrtype = GSS_C_AF_UNSPEC;
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
This looks wrong to me -- as you already mentioned -- this violates RFC
2744, section 3.11, last sentence:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">or omit addressing information, specifying
GSS_C_AF_NULLADDR as the address-types.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> /* release initiator address */
- if (cb->initiator_addrtype != GSS_C_AF_NULLADDR) {
+ if (cb->initiator_addrtype != GSS_C_AF_NULLADDR &&
+ cb->initiator_addrtype != GSS_C_AF_UNSPEC) {
resetGSSBuffer(&(cb->initiator_address));
}
/* release acceptor address */
- if (cb->acceptor_addrtype != GSS_C_AF_NULLADDR) {
+ if (cb->acceptor_addrtype != GSS_C_AF_NULLADDR &&
+ cb->acceptor_addrtype != GSS_C_AF_UNSPEC) {
resetGSSBuffer(&(cb->acceptor_address));
}
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Unspecified does not mean that it is null.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ final byte[] prefix = (TlsChannelBindingType.TLS_SERVER_END_POINT.getName() + ":").getBytes();
+ byte[] cbData = Arrays.copyOf(prefix,
+ prefix.length + tlsCB.getData().length );
+ System.arraycopy(tlsCB.getData(), 0, cbData, prefix.length, tlsCB.getData().length);
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Since you are calling "tlsCB.getData()" multiple times, this should go
into a variable.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Temporary variable is added
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ secCtx.setChannelBinding(new
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">ChannelBinding(null, null, cbData));
Why not use new ChannelBinding(cbData)?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Right. updated
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ String hashAlg = serverCertificate.getSigAlgName().
+ replace("SHA", "SHA-").toUpperCase();
+ int ind = hashAlg.indexOf("WITH");
+ if (ind > 0) {
+ hashAlg = hashAlg.substring(0, ind);
+ if (hashAlg.equals("MD5") || hashAlg.equals("SHA-1")) {
+ hashAlg = "SHA-256";
+ }
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I have several problems with that, some might be hypothetical:
* toUpperCase() should be qualified with Locale.ROOT or Locate.ENGLISH
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
As you mentioned below AlgorithmId.getName() uses nameTable to return algorithm name.
Looking at implementation I do not think it is realistic to get name in the non-English locale.
I do not think it should be fixed
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">* Looking at <a class="moz-txt-link-freetext" href="https://tools.ietf.org/html/rfc5280#section-4.1.1.2">https://tools.ietf.org/html/rfc5280#section-4.1.1.2</a>, then
at sun.security.x509.AlgorithmId.getName() it uses nameTable to
translate OIDs to readible names.
With indexOf("WITH") you are implying that the cert's sig alg may not
use a cryptographic function, but this would mean that if the OID is
1.3.14.3.2.26 you'd turn "SHA-X" into "SHA--X" according to nameTable,
wouldn't you?
I also don't know how realistic "PBEWith..." is.
This likely needs more thought or I am missing something.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I do not think it is realistic scenario to have certificate signature algorithm without crypto function.
indexOf(“WITH”) just used to find end out hash algorithm name.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">* Exception messages are inconsistent. Sometimes "TLS channel binding",
sometimes just "channel binding". To avoid misunderstandings it should
always read "TLS channel binding..".
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Messages are updated.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Generally, I have two fundemental issues:
* How do you know that address type must be UNSPEC and not NULLADDR?
Trial and error?
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I did not find any strict specification about TLS Channel Binding format in Windows server.
So, it was a lot of experiments, reading related forums and docs.
One of the doc, that turns me to try UNSPEC type is the following:
<a class="moz-txt-link-freetext" href="https://docs.microsoft.com/en-us/archive/blogs/openspecification/ntlm-and-channel-binding-hash-aka-extended-protection-for-authentication">https://docs.microsoft.com/en-us/archive/blogs/openspecification/ntlm-and-channel-binding-hash-aka-extended-protection-for-authentication</a>
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">* Changing the default address type against the RFC will break every
installation using channel bindings relying on it with cross GSS-API
implementations.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I do not like this conflict between UNSPEC and NULLADDR types too, but I do not know How to better solve it.
The usage of UnspecEmptyInetAddress subtype is not quite clear to me yet. Who set this value and will it change org.ietf.jgss.ChannelBinding public api ? As I understand, Implementation cannot decide (without pplication request), What channel binding type is enabled on the server.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
There must be another way to solve this. A system property would also be
problematic because if you have a product which connects to MS and
non-MS backends at the same time, channel bindings would be broken again.
What about introducing a UnspecEmptyInetAddress() which gives the proper
AF type, but #getAddress() would return null? This should satisfy the
requirements, InitialToken as well as the RFC. this of course needs to
be properly passed to native providers too. GssKrb5Client would also
need to know that this channel binding is explicitly for Active
Directory and not some other, spec-compliant, SASL peer (property on
LdapCtx?)
One could easily use this for custom code with a SSLServerSocket paired
with Java SASL or in C, OpenSSL and Cyrus SASL.
Michael
PS: Yes, I am a nitpicker.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
</pre>
</blockquote>
</body>
</html>