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

[Michael Osipov]

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
sspi.cpp:
* I don't like the inconsistent naming of functions: PascalCase, 
camelCase, snake_case. Is that on purpose?
* 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
* Why do you redefine SEC_SUCCESS macro from sspi.h?
* OID defs: shouldn't the char * be casted to void *?
* 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.
* showTime(): please use a readable format akin to %FT%T
* 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
** If you log the token size you should also log if SecurityStatus isn't 
positive, just in case
* flagSspi2Gss() and complement: please document that ANON_FLAG is not 
supported by SSPI
* 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?
* getTGTEndTime(): Wouldn't it be easier to call 
AcquireCredentialsHandle() and inspect ptsExpiry field?
* get_full_name():
** wcschr() doesn't it expect a L"string" as a second arg?
** 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
* gss_import_name():
** 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
** "    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
* 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.
** CP_ACP: UTF8?
* gss_acquire_cred():
** Why again duplicate code, but only package name differs?
** Doesn't pAuthData accept SEC_WINNT_AUTH_IDENTITY and why do you 
populate it at all if we need default credentials only?!
** 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?
** 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?!
* gss_inquire_cred():
** Why do you ignore cred_usage? You do support all three values in 
gss_acquire_cred()
* gss_init_sec_conent():
** Why don't you probe for GSS_C_NO_CONTEXT instead of zero input_token 
for NewContext?
** Why do you already assign output_token length and value of nothing 
has yet happened and test for a NULL value?
** Same here with SEC_WINNT_AUTH_IDENTITY_EX

Michael