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

[Weijun Wang]

Hi Michael,

Many thanks to your review. Sorry I'm new to the C side of GSS-API and SSPI and I write very little C code these days.

> On Mar 22, 2019, at 5:17 AM, Michael Osipov <1983-01-06 at gmx.net> wrote:
> 
> Hi Max,
> 
> here is my review based on webrev.04. Some of the statements might not be correct due to my little C/C++ knowledge, just correct me if I am wrong.
> 
> I am really looking to test this, especially as a SASL provider for my extended LDAP communication with Active Directory.
> 
> gssapi.h:
> * There are several unnecessary formatting (whitespace) changes in the prototypes

I'll go thru all changes again.

> sspi.cpp:
> * I don't like the inconsistent naming of functions: PascalCase, camelCase, snake_case. Is that on purpose?

The PascalCase should not exist. Now all GSS-API implementations use snake_case and other helper functions use camelCase. I assume that's because I've written too many Java methods.

> * header comment: Why do actually exclude NTLM from SPNEGO? Let SSPI work as it is intended to work. Means less code you have to maintain

Java's GSS codes are not very good at using NTLM as a mechanism (the header, multi-round...).

> * Why do you redefine SEC_SUCCESS macro from sspi.h?

Oops, I didn't realized it's a redefinition. I'll change the name. Or, do you know if my definition is too different from the original one?

> * OID defs: shouldn't the char * be casted to void *?

No compiler warning, so I haven't added any cast.

> * Don't change the semantics of KRB5_TRACE. MIT Kerberos users are used to it, either write to the supplied file path or use a different name.

How about only support "set KRB5_TRACE=/dev/stdout" now and be silent otherwise?

> * showTime(): please use a readable format akin to %FT%T

Sure.

> * NewContext():
> ** why don't you just pass the package name as WCHAR pointer? There is no clear definition what happens if it is not SPNEGO w/o looking into the code

OK I can do that. Only Negotiate and Kerberos are supported now...

> ** If you log the token size you should also log if SecurityStatus isn't positive, just in case

Many logs are actually used by myself while debugging. I don't have a list of what to show and what not. I'll see if this can be embedded into the SEC_SUCCESS macro.

> * flagSspi2Gss() and complement: please document that ANON_FLAG is not supported by SSPI

OK.

> * displayOID(): The function name is misleading. It claims to display an OID, but it doesn't. It displays a custom name It should be somewhat similar to gss_oid_to_str() shouldn't it?

The method is enough for my debugging purpose now. Unless there is a Windows function for that, I don't want to spend many time implementing that. I'm also afraid of C programming.

> * getTGTEndTime(): Wouldn't it be easier to call AcquireCredentialsHandle() and inspect ptsExpiry field?

The field has a strange value. See

   https://stackoverflow.com/questions/11028798/how-do-i-interpret-the-expiry-returned-by-acquirecredentialshandle-kerberos

> * get_full_name():
> ** wcschr() doesn't it expect a L"string" as a second arg?

Will fix it. But currently seems the compiler does a i8 -> i16 promotion.

> ** What is the purpose of this function? It will not work reliably if you have this case (solution 2?): Realm AD001.SIEMENS.NET, SPN HTTP/travel.siemens.com

Java does not support realm-less service name so I'll have to add one to make ServicePermission check working. In your case, is this a stopper? Can windows find out the correct realm even if I provide a false one? Or maybe I can give Java back the (possibly wrong) full name and remove it when passing to the windows API.

> * gss_import_name():

I'll make quite some changes in this function. I'll also reject non-Kerberos exports.

> ** BOOLEAN isNegotiate isn't really readable code
> ** I am a bit confused, Kerberos/GSS names should be UTF-8 encoded, but you do "new SEC_WCHAR[len + 1];" where len denotes the length of bytes and not chars.
>   This might a minor one, but may allocate more memory than necessary
> ** CP_ACP: I'd expect CP_UTF8 here

Yes.

> ** "    value[len] = 0;" rather '\0'?
> ** "if (value[len-1] == '@') {" rather L"@"? This continues in the function
> ** "if (value[len-1] == '@') {" you should comment this block and explain why you are doing this. Is this because of "@ignore_me_rfcXXX"?
> ** SPN conversion, why do you replace the '@' with '/' explicitly for SSPI? A non-mech specific hostbased service is always neutral with '@'
> * gss_canonicalize_name(): something is fishy consider the following case:
>  gss_name HTTP at server.example.com , gss_oid = KRB5. I'd expect to receive HTTP/server.example.com at EXAMPLE.COM (or w/o realm), but get_full_name() doesn't do this

Ideally it should be realm-less, but... See above.

> * gss_export_name(): probably the same issues as with gss_import_name()
> * gss_display_name():
> ** "char* buffer = new char[len+1];" you are changing sematics again. len is length of wide chars. If you have L"Jörg.Björn at EXAMPLE.COM" it won't fit into buffer.

I'll x4 to be safe.

> ** CP_ACP: UTF8?
> * gss_acquire_cred():
> ** Why again duplicate code, but only package name differs?

To produce different tokens I'll need different CredHandle. Since I only want to support 2 mechs here it's easy to just hardcode them.

> ** Doesn't pAuthData accept SEC_WINNT_AUTH_IDENTITY and why do you populate it at all if we need default credentials only?!

Please give me more detail. I only want to exclude NTLM here.

> ** TGT time from CredHandle is huge, true. I believe it is that huge because LSA retains your password in memory so you will ALWAYS have a valid TGT compared to the MIT Kerberos approach.
>   Shall we really care for that value?

What's your suggestion?

> ** Can someone explain me for the stupid why there is pszPrincipal in AcquireCredentialsHandle if a user principal cannot have anything else and machine principals do not retain any local SPN longterm keys?!

Maybe someone will call this function with a non-null name, and then I'll reject the one which is not myself.

> * gss_inquire_cred():
> ** Why do you ignore cred_usage? You do support all three values in gss_acquire_cred()

This first version of sspi.cpp only support the client side.

> * gss_init_sec_conent():
> ** Why don't you probe for GSS_C_NO_CONTEXT instead of zero input_token for NewContext?

Is that more RFC compliant?

> ** Why do you already assign output_token length and value of nothing has yet happened and test for a NULL value?

Is there a way to let windows allocating it?

> ** Same here with SEC_WINNT_AUTH_IDENTITY_EX

To exclude NTLM.

Thanks,
Max

> 
> Michael
>