RFR 8225687: Newly added sspi.cpp in JDK-6722928 still contains some small errors

[Weijun Wang]

Please take a review at

   http://cr.openjdk.java.net/~weijun/8225687/webrev.00/

Most changes are around const usage in gssapi.h. Note that this is internal so the change could only prevent any coding error and will not be visible outside JDK.

Other changes are inside gss_import_name(), gss_compare_name(). I also changed some names like get_oid_set_desc -> show_oid_set.

Thanks,
Max

> On Jun 13, 2019, at 10:48 AM, Weijun Wang <weijun.wang at oracle.com> wrote:
> 
> I've filed https://bugs.openjdk.java.net/browse/JDK-8225687.
> 
>> On Jun 11, 2019, at 7:56 AM, Nico Williams <Nico.Williams at twosigma.com> wrote:
>> 
>> On Mon, Jun 10, 2019 at 10:30:50AM +0800, Weijun Wang wrote:
>>> Updated webrev at
>>> 
>>>  http://cr.openjdk.java.net/~weijun/6722928/webrev.08/
>> 
>> - src/java.security.jgss/share/classes/sun/security/jgss/spnego/NegTokenTarg.java
>> 
>>  Ugh, I never noticed GSSUtil.useMSInterop().  This really should not
>>  be needed.  I believe a proper implementation of RFC4178 wouldn't
>>  need it.
> 
> I tried but scared away by MS-SPNG's appendix on which version of Windows supports which style of mechListMic. Now that Java only supports one underlying mechanism this field is looks not so crucial.
> 
>> 
>>  I can't expect you to do that, but can we have a bug opened for this?
> 
> There was one at https://bugs.openjdk.java.net/browse/JDK-6773898.
> 
>> 
>> - src/java.security.jgss/share/classes/sun/security/jgss/wrapper/SunNativeProvider.java
>> 
>>  Shouldn't the sspi_bridge.dll be in the lib directory?
> 
> The DLLs have always been in /bin. Maybe it's more easily loadable by java.exe.
> 
>> 
>> - src/java.security.jgss/share/native/libj2gss/NativeFunc.h
>> - src/java.security.jgss/share/native/libj2gss/gssapi.h
>> 
>>  These continue to have improper constification.
>> 
>>  There should be no function arguments of type "const gss_...".
>> 
>>  I've a PR for Heimdal to fix this in Heimdal.  It was very
>>  mechanical: search the prototypes and function definitions for
>> 
>>      \<const[^I ]*gss_
>> 
>>  then change that to gss_const_... then fix all the warnings and
>>  errors that came up when building the result.
> 
> OK.
> 
>> 
>> - All remaining commentary will be about
>>  src/java.security.jgss/windows/native/libsspi_bridge/sspi.cpp
>> 
>> - CHECK_*() macros
>> 
>>   Macro bodies should not end in a semi-colon.
> 
> OK.
> 
>> 
>>   If these were public and since these macro bodies are all if
>>   statements, you should wrap them in do { } while (0), but since
>>   they're private we can make sure that all uses are correct.
>> 
>> - gss_import_name() doesn't check that the first two bytes of the
>>   input buffer are the expected token ID when the name-type is
>>   GSS_C_NT_EXPORTED_NAME.
> 
> OK.
> 
>> 
>> - gss_import_name() doesn't check that the third byte of the input
>>   buffer is 0 when the name-type is GSS_C_NT_EXPORTED_NAME.
> 
> OK.
> 
>> 
>> - gss_compare_name(), this code will not distinguish a name of the
>>   form 'foo@' from 'foo\@'
>> 
>>      434     if (l1 < l2 && n2[l1] != L'@'
>>      435             || l2 < l1 && n1[l2] != L'@') {
>>      436         return GSS_S_COMPLETE; // different
>>      437     }
>> 
>>   Honestly, this is not the most serious problem because nothing
>>   really should be using gss_compare_name(), but you do use it... and
>>   anyways, it's wrong.
> 
> Yes, it's called in GSSLibStub.c and those are the functions I must support.
> 
>> 
>>   Perhaps the gss_name_struct struct should have a length of realm or
>>   length-of-not-realm-part field to make this check easier.
>> 
>> - gss_compare_name(), do not use NORM_IGNORECASE
> 
> But I do notice that "Administrator" can also be named "administrator", and "HTTP/host" can also be "http/host".
> 
>> 
>> - gss_canonicalize_name() should check that mech_type is Kerberos
> 
> OK.
> 
>> 
>> I'll continue later.  I'm in gss_init_sec_context(), about 58% of the
>> way through.
> 
> Please.
> 
> Thanks,
> Max
> 
>> 
>> Nico
>> -- 
>