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 h...@server.example.com , gss_oid = KRB5. I'd expect to
receive HTTP/server.example....@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ör...@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
