RFR 6722928: Support SSPI as a native GSS-API provider

[Weijun Wang]

Hi Valerie,

Thanks for the feedback. Most accepted. Some comments below inline. I'll read Nico's feedback next and then upload a new webrev.

> On Dec 20, 2018, at 3:51 AM, Valerie Peng <valerie.peng at oracle.com> wrote:
> 
> Hi Max,
> 
> Here are more comments/questions on <sspi.cpp>:
> 
> 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. 
> For all new/malloc calls, check for null return value before proceeding further.
> 
> For gss APIs whose arguments are marked const, can we also do that here? I think it's clearer and helps code readability.
> 
> Below are function-specific comments.
> 
> - SecondsUntil: naming seems inconsistent, should start with lower case?

Yes.

> Based on the MS doc that I found https://msdn.microsoft.com/en-us/library/ms724284(v=VS.85).aspx, 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 FILETIME structure to either a ULARGE_INTEGER* or __int64* value because it can cause alignment faults on 64-bit Windows."

But I haven't casted a pointer like the doc says. Both structs have a QuadPart and I am just using it. In fact, I declare a new variable named uiLocal and assign the fields of nowLocal to it, and not just "uiLocal = (ULARGE_INTEGER*)&nowLocal".

> In addition, the comment about AcquireCredentialsHandle may be incomplete, as gss_context_time also uses this method.

The comment is about when called by AcquireCredentialsHandle the return value is strange (see https://stackoverflow.com/q/11028798/1061575). When called by gss_context_time, it's fine. I decide not to call it at all for AcquireCredentialsHandle and use the end time in the TGT as the expiry time of the credential.

> -1 is returned if the FileTimeToLocalFileTime() call failed.

I should check on it.

> 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?

It's mainly for checking the strangeness of AcquireCredentialsHandle. Since I won't use this method for  AcquireCredentialsHandle, I'll remove the lines.

>  
> - gss_context_time: Shouldn't this method also check the tsExpiry value and return the GSS_S_CONTEXT_EXPIRED error code if applicable? Also the time_rec should be 0 if context has already expired. 

Yes.

> 
> - gss_wrap_size_limit:  based on the MS doc example: https://docs.microsoft.com/en-us/windows/desktop/secauthn/encrypting-a-message, it looks like the value is "cbMaximumMessage" of SecPkgContext_StreamSizes. Where is the documentation for the context_handle and its cbMaxMessage field?

SecPkgContext_StreamSizes is only for Schannel (TLS). Kerberos uses SECPKG_ATTR_SIZES and I found out the SecPkgContext_Sizes cbMaxToken is the same as that in PSecPkgInfo.

But I do realize that it should depend on input size, and I should be able to calculate it using the same logic to calculate output token size in gss_wrap. I tried wrapping 100K bytes and it works but the cbMaxToken is only 48K, so it seems this cbMaxToken value has nothing to with wrap tokens.

> 
> - gss_get_mic: Some places you use SEC _SUCCESS macro and some plain if-else block. What is the criteria?

If I only want to check success and failure, a SEC_SUCCESS is needed. Sometimes I also need to know additional information, like SEC_I_COMPLETE_NEEDED, SEC_E_OUT_OF_SEQUENCE.

> In the SEC_SUCCESS macro, it's clearer to use SEC_E_OK instead of 0?

OK.

> 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?

I tried calling it at SEC_I_CONTINUE_NEEDED (mutual auth 1st call). All sizes are zero. This is enough to avoid a crash.

> - 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? Should we also check qop_state?

Yes.

> 
> - 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. 

Fixed.

> Will continue reviewing the rest.
> Thanks,
> Valerie
> 
> 
> 
> On 12/12/2018 2:20 PM, Valerie Peng wrote:
>> Hi Max,
>> 
>> <sspi.cpp>
>> 
>> - 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.

I've tried to add more comments. The numbers are not used much and defining them seems not worth it.

>> - line 449, I think you mean to use "c" instead of "cred_handle"

Yes.

>> 
>> - 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?

I've discussed with Nico on the const thing. I'll think more about it. Maybe I'll create a copy of gssapi.h for this library that uses the new const typedefs. Do you agree to modify the gssapi.h in the native bridge?

I copied the the variable names from MSDN examples. Modified.

I'll set qop_state. Maybe I thought it was useless.

>> 
>> - 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?

Yes, and yes.

>> - gss_inquire_names_for_mech: why does the PP output has "IMPLEMENTED" wording, other methods do not. Is this intentional?

No. I added IMPLEMENTED and UNIMPLEMENTED at the beginning and modify them one by one. I missed this one.

>> - 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.

I dare not do that. The pointer might not be not been initialized at all. For example, in the newGSSOIDSet() function in NativeUtil.c.

>> - gss_add_oid_set_member: add "const" to the 2nd argument?
>> 
>> - 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?

Yes, and yes.

>> 
>> - 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?

Yes. The SEC_SUCCESS macro always stores the windows error id into minor_status, and gss_display_status  will return a readable message.

Thanks,
Max

>> I am still going through the rest of sspi.cpp, but thought that I will send you what I have first. 
>> Good that you have this targeted to 13 as there is almost no time left for RFEs to get into JDK12.
>> Thanks,
>> Valerie
>> 
>> 
>> On 11/19/2018 5:56 PM, Weijun Wang wrote:
>>> Please take a review at
>>> 
>>>    
>>> https://cr.openjdk.java.net/~weijun/6722928/webrev.01/
>>> 
>>> 
>>> 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] 
>>> https://bugs.openjdk.java.net/browse/JDK-8200468