<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>Hi Max,</p>
<p>Here are more comments/questions on <sspi.cpp>:</p>
<p>I am a bit unclear on the handling of the gss qop and MS qop.
There are a few places where you seem to treat them as the same
thing. But there are also cases where they seem unrelated as you
didn't set one using the other and vice versa. <br>
</p>
<p>For all new/malloc calls, check for null return value before
proceeding further.</p>
<p>For gss APIs whose arguments are marked const, can we also do
that here? I think it's clearer and helps code readability.</p>
<p>Below are function-specific comments.</p>
<p>- SecondsUntil: naming seems inconsistent, should start with
lower case? Based on the MS doc that I found
<a class="moz-txt-link-freetext" href="https://msdn.microsoft.com/en-us/library/ms724284(v=VS.85).aspx">https://msdn.microsoft.com/en-us/library/ms724284(v=VS.85).aspx</a>,
TimeStamp is same as FILETIME which you then copy the two fields
into a ULARGE_INTEGER to do the 64-bit arithmetic, but through out
the code, you seems to treat ULARGE_INTEGER and FILETIME as
interchangeable and directly access the QuadPart field. MS doc has
"Do not cast a pointer to a <strong>FILETIME</strong> structure
to either a <strong>ULARGE_INTEGER*</strong> or <strong>__int64*</strong>
value because it can cause alignment faults on 64-bit Windows." In
addition, the comment about AcquireCredentialsHandle may be
incomplete, as gss_context_time also uses this method. -1 is
returned if the FileTimeToLocalFileTime() call failed. Also it
seems that this method assumes that the passed-in "time" is always
beyond the current time (line 291 - 296) which may not always be
true? <br>
</p>
<p>- gss_context_time: Shouldn't this method also check the tsExpiry
value and return the <code>GSS_S_CONTEXT_EXPIRED error code if
applicable? Also the time_rec should be 0 if context has already
expired. <br>
</code></p>
<p>- gss_wrap_size_limit: based on the MS doc example:
<a class="moz-txt-link-freetext" href="https://docs.microsoft.com/en-us/windows/desktop/secauthn/encrypting-a-message">https://docs.microsoft.com/en-us/windows/desktop/secauthn/encrypting-a-message</a>,
<code class="lang-C++ x-hidden-focus">it looks like the value is
"cbMaximumMessage" of</code><code class="lang-C++
x-hidden-focus"> SecPkgContext_StreamSizes. Where is the
documentation for the</code><code></code> context_handle and its
cbMaxMessage field?<br>
</p>
<p><code></code></p>
<p>- gss_get_mic: Some places you use SEC _SUCCESS macro and some
plain if-else block. What is the criteria? In the SEC_SUCCESS
macro, it's clearer to use SEC_E_OK instead of 0? Are we sure that
the context handle already has the cbMaxSignature value? I saw
that you make this call inside gss_init_sec_context() when the
context is established. But do we need to handle the case that a
half-established context is passed in? Should not crash even if
user does that, right?<br>
</p>
<p>- gss_verify_mic: qop_state is optional and may be null. You
should check the value before dereferencing it. Line 1262, should
be SECBUFFER_VERSION instead of 0?<span class="x-hidden-focus"><code>
Should we also check qop_state? </code></span></p>
<p>- gss_wrap: Line 1347, since the order of data is 0-1-2, why the
length addition is ordered 1-0-2? Seems more natural to use the
same order. Move the assignment of conf_state (line 1329) after
the if-block (line 1331). In addition, conf_state is optional and
may be null. You should check the value before dereferencing it. <br>
</p>
<span class="x-hidden-focus"><code>Will continue reviewing the rest.</code></span><br>
<span class="x-hidden-focus"><code>Thanks,</code></span><br>
<span class="x-hidden-focus"><code>Valerie</code></span>
<p><span class="x-hidden-focus"><code><br>
</code></span></p>
<p><span class="x-hidden-focus"><code></code></span></p>
<div class="moz-cite-prefix">On 12/12/2018 2:20 PM, Valerie Peng
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:fe74a752-a23c-aead-5d6b-21b793c7d905@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
<p>Hi Max,</p>
<p><sspi.cpp></p>
<p>- the DER related code is very hard to read... Would be nice to
use constants/enum for commonly used tag or use some method to
construct them.<br>
</p>
<p>- line 449, I think you mean to use "c" instead of
"cred_handle"</p>
<p>- gss_unwrap: add "const" to the 2nd and 3rd arguments? Isn't
variable naming convention starts with lower case? the argument
qop_state may be non-null but is not set?<code class="lang-C++
x-hidden-focus"></code></p>
<p>- gss_indicate_mechs: the SSPI docs that I found mentioned that
you need to call FreeContextBuffer on pkgInfo after calling
QuerySecurityPackageInfo(). Local variable "minor" not used and
can be removed?<br>
</p>
<p>- gss_inquire_names_for_mech: why does the PP output has
"IMPLEMENTED" wording, other methods do not. Is this
intentional?<br>
</p>
<p>- gss_create_empty_oid_set: do we need to check the specified
oid_set for existing content and free if not-empty before wiping
it out? This is called by a few other gss api methods also, it
may be better to defend against user errors.<br>
</p>
<p>- gss_add_oid_set_member: add "const" to the 2nd argument? </p>
<p>- gss_release_buffer: maybe set buffer->length = 0 outside
the if-block. Do we need to check for GSS_C_NO_BUFFER in
addition to null?</p>
<p>- gss_display_status: add "const" to the 4th argument? As for
the impl, I have a question, this particular method is for
displaying text output for gssapi error codes, but the
FormatMessage() call takes window specific message id. Are they
the same?<br>
</p>
<p>I am still going through the rest of sspi.cpp, but thought that
I will send you what I have first. <br>
</p>
<p>Good that you have this targeted to 13 as there is almost no
time left for RFEs to get into JDK12.<br>
</p>
Thanks,<br>
Valerie<br>
<p> </p>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">On 11/19/2018 5:56 PM, Weijun Wang
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:AB7943C6-51CD-46B1-9C54-1F26BD885F74@oracle.com">
<pre class="moz-quote-pre" wrap="">Please take a review at
<a class="moz-txt-link-freetext" href="https://cr.openjdk.java.net/~weijun/6722928/webrev.01/" moz-do-not-send="true">https://cr.openjdk.java.net/~weijun/6722928/webrev.01/</a>
We ported [1] the native GSS bridge to Windows in JDK 11, but user still have to download and install a native GSS-API library. This code change provides a native GSS-API library inside JDK that can be used without setting the "sun.security.jgss.lib" system property. It is based on Windows SSPI and now only supports the client side using the default credentials.
No regression tests included. A Windows Active Directory server is needed.
Thanks
Max
[1] <a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8200468" moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8200468</a></pre>
</blockquote>
</blockquote>
</body>
</html>