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...@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 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 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ör...@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 >
