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

[Nico Williams]

On Wed, Mar 20, 2019 at 10:20:28AM +0800, Weijun Wang wrote:
> New webrev at https://cr.openjdk.java.net/~weijun/6722928/webrev.05, and interdiff at
> 
>    https://cr.openjdk.java.net/~weijun/6722928/webrev.05/interdiff.patch.html

Thanks.  I'll take a look.

> I'll think more about secondsUntil() and getTGTEndTime(). Other comments below.
> 
> > - sspi.cpp:61
> > 
> >   61 #define SEC_SUCCESS(status) (*minor_status = status, (status) >= SEC_E_OK)
> >                                   ^^^^^^^^^^^^^^^^^^^^^^
> > 
> >   Please add parenthesis around the whole assignment expression.
> 
> OK, but what kind of bad things could happen?

First, there's a matter of hygiene: you need to parenthesize `status`,
and you do in the second part but not the first.  Second, assignments in
if statements need to be parenthesized to avoid compiler warnings -- and
this one is parenthesized, but who knows if a compiler might be confused
by the comma operator and emit a warning anyways.

> > - sspi.cpp:NewContext()
> > 
> >   Please fully-initialize all fields of the allocated context structure before
> >   returning it.
> 
> Some of them are of struct types, do I need to go inside and set each to zero? Or ZeroMemory the whole struct?

C does not guarantee that (void*)0 is an all-bits-zero pattern, but I
imagine all the supported Windows platforms do (certainly x86 and
x86_64).  So ZeroMemory will do.  For more portable code you should
ZeroMemory the struct then manually assign all the pointer values.

> > - sspi.cpp:flagSspi2Gss() and flagGss2Sspi()
> > 
> >   I realize that this would be a change to the Java bindings, so this is just
> >   a note, but, we should add support for GSS_C_DELEG_POLICY_FLAG.
> 
> How? Always set it with GSS_C_DELEG_FLAG?

No, we'd have to add the Java bindings too.  Forget this request.

> > - sspi.cpp:getTGTEndTime()
> > 
> >   Please immediately write to the `endTime' output parameter upon entry.
> 
> Are you afraid of garbages for "return 1"s? But I won't touch the parameter at all in this case.

Yes.  It's just defensive programming.

> > - sspi.cpp:315
> > 
> >   314     // input has realm, no need to add one
> >   315     if (wcschr(input, '@')) {
> > 
> >   This is not going to work with UPNs.  
> 
> Why not? I just want to find out if there is already a realm.

UPNs are of the form name\@domain at REALM.  You're not checking for `\@`.

You can't search for `@` backwards either because realm names can
actually have `\@` as well in them.

> > You should probably implement proper
> >   RFC1864 name format support, including backslash-quoting.
> 
> Ahh...

Right :)

> > - sspi.cpp:315
> > 
> >   315     if (wcschr(input, '@')) {
> >                             ^
> > 
> >   I don't know if it is necessary that this be L'@' or if the compiler will do
> >   the right thing because the `wc' argument of `wcschr()' is a `wchar_t'.
> > 
> >   Elsewhere you do use wide character literals, so I assume even if type
> >   promotion works correctly for char->WCHAR (does it?) you might want to be
> >   consistent.
> 
> I'll made them all L'@'.

Or stop using wchar_t...  :)

> >   wchar_t/WCHAR is cancer.
> > 
> >   I worry that adding any more wchar_t-using code makes worse the legacy
> >   cleanup as Windows starts to prefer UTF-8.  But whatever.
> > 
> > - sspi.cpp:351
> > 
> >   351     wcscpy_s(fullname + oldlen + 1, wcslen(realm) + 1, realm);
> > 
> >   It would be safer to use wcscat_s().
> 
> How is this unsafe? If using wcscat_s(), I'll need to add a '\0' after the '@' first.

You wouldn't have to compute the offset to copy at.  I call that safer.

> > - sspi.cpp:362
> > 
> >   362 #define CHECK_OUTPUT(x)  if (!x) return GSS_S_CALL_INACCESSIBLE_WRITE;
> > 
> >   I would much prefer the more standard and safer do { ... } while (0)
> >   construction:
> > 
> >    362 #define CHECK_OUTPUT(x)  \
> >        do { if (!x) return GSS_S_CALL_INACCESSIBLE_WRITE; } while (0)
> 
> I can do that, but why is this safer?

You don't have to worry about this

    if (something)
        CHECK_OUTPUT(out_arg);
--->else
        other stuff

That `else` will bind to the `if` that `CHECK_OUTPUT()` expands into.

> You mean there is actually no need to check the arguments?

No, I mean that they don't save that much work.

> > - sspi.cpp:527
> > 
> >   527     int len = (int)wcslen(fullname);
> > 
> >   You don't need that cast.  
> 
> There was a warning.

Yes, but see the comments that followed:

> >   But you should check first that `wcslen()' did
> >   not return a value larger than `INT_MAX'.
> > 
> >   You only even need an `int len' because `WideCharToMultiByte()' returns
> >   `int'...
> > 
> >   I'd write this like:
> > 
> >    527     int len;
> >            size_t namelen = wcslen(fullname);
> >            if (namelen > 255)
> >                goto err;
> >            // 04 01 00 ** 06 ** OID len:int32 name
> >            // ...
> >            //

> > - sspi.cpp:532,574
> > 
> >   You can't assume the length of a multi-byte string will be the same number
> >   of `char' as the number of `wchar_t' in the wide-char version of the string.
> > 
> >   Determinining the correct length is a bit tricky, though since we should be
> >   converting to UTF-8 (see above), you can assume that 4x is enough, use
> >   4*len for the buffer allocation, then take the correct length from
> >   WideCharToMultiByte().
> 
> OK.

(And even this assumes that we Windows won't do normalization on the
string, though if they normalize to NFC, I think the number of
codepoints can't go up.)

> > - sspi.cpp:607
> > 
> >   606     TimeStamp ts;
> >   607     ts.QuadPart = 0;
> > 
> >   Huh?  TimeStamp is a typedef of SECURITY_INTEGER, and SECURITY_INTEGER is a
> >   struct of the same shape and size as FILETIME, and has no QuadPart field.
> > 
> >   What am I missing?
> 
> It compiles. I think it's a union of one Quad or two DWORD.

Ok.

> > - sspi.cpp:954
> > 
> >   Should you check if there's an output token before doing this:
> > 
> >   954         ss = CompleteAuthToken(&pc->hCtxt, &outBuffDesc);
> > 
> >   ?
> 
> Does this matter? Do you think CompleteAuthToken cannot deal with no token?

Hmm, probably not.

> > - sspi.cpp:874 and others
> > 
> >   delete `output_token->value' and set it to NULL (and the length to 0) before
> >   returning errors after 865.
> 
> OK. Is this just a good habit or do you know someone using it even at a failure?

It's perfectly safe to gss_release_buffer() after a failure.  After all,
the failure might involve an error token.

> > - sspi.cpp:878,962-963
> > 
> >   The local variable `pfDone' is set but not used.  Drop it.
> 
> OK,
> 
> > 
> >   (The compiler should have warned about this, no?)
> 
> No.

Why not.

> > - sspi.cpp:964-966
> > 
> >   Change this:
> > 
> >   964     outFlag = flagSspi2Gss(outFlag);
> >   965 
> >   966     *ret_flags = (OM_uint32)outFlag;
> > 
> >   to:
> > 
> >   964     *ret_flags = flagSspi2Gss(outFlag);
> > 
> >   You don't use outFlag again.  Setting it to a different kind of set of flags
> >   seems like a trap for someone to fall into in the future.
> 
> Good.

Huh?

> > - sspi.cpp:948
> > 
> >   948     if (!SEC_SUCCESS(ss)) {
> >   949         return GSS_S_FAILURE;
> >   950     }
> >   951 
> >   952     if ((SEC_I_COMPLETE_NEEDED == ss)
> >   953             || (SEC_I_COMPLETE_AND_CONTINUE == ss)) {
> > 
> >   I can't find documentation on SEC_SUCCES().  I don't know if it treats
> >   SEC_I_COMPLETE_NEEDED as an error or not...  I gues it must not.  Can you
> >   confirm?
> 
> Not an error. I defined SEC_SUCCESS in this file.

Oy, sorry!

> > - sspi.cpp:1002
> > 
> >   It should be simple now to implement this...
> > 
> >   1002     PP(">>>> Calling UNIMPLEMENTED gss_accept_sec_context...");
> >   1003     return GSS_S_FAILURE;
> 
> Not our goal this time, and I don't know how to test. Does server on Windows only run as services?

Do we have servers that run on Windows?  I think we do.  Our shim
(forked from SAP's) works for us.

> > - gssapi.h:gss_acquire_cred()
> > 
> >        @@ -387,13 +399,13 @@
> > 
> >         /* Function Prototypes */
> > 
> >         GSS_DLLIMP OM_uint32 gss_acquire_cred(
> >                 OM_uint32 *,            /* minor_status */
> >        -        gss_name_t,             /* desired_name */
> >        +        gss_const_name_t,       /* desired_name */
> >                 OM_uint32,              /* time_req */
> >        -        gss_OID_set,            /* desired_mechs */
> >   ---->+        const gss_OID_set,      /* desired_mechs */
> >                 gss_cred_usage_t,       /* cred_usage */
> >                 gss_cred_id_t *,        /* output_cred_handle */
> >                 gss_OID_set *,          /* actual_mechs */
> >                 OM_uint32 *             /* time_rec */
> >         );
> > 
> >   That needs to be `gss_const_OID_set', not `const gss_OID_set'.
> > 
> >   There's a number of these.  Just search for "const gss_" -- any time you see
> >   that in a function prototype, it's wrong.
> 
> I think I copied all of them from Heimdal. Do you mean Heimdal is not complete at using the types?

Yes.  Thanks for noticing.  I'll fix it.

Nico
--