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

Fri Mar 22 09:28:24 UTC 2019

Hi Max,

Am 2019-03-22 um 02:10 schrieb 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.
>

>> * 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...).

Can you please elaborate why? The token content is opaque to the API and
isEstablished() will fail unless type 3 token has been received. I'd use
this as a change to improve JGSS regardless of NTLM.

>> * 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?

Here is sample code from Microsoft:
https://docs.microsoft.com/en-us/windows/desktop/secauthn/using-sspi-with-a-windows-sockets-server
I would not even bother adding it myself. What is the reason for this?

>> * OID defs: shouldn't the char * be casted to void *?
>
> No compiler warning, so I haven't added any cast.

Don't expect cl be a decent compiler. clang will give you one. Have a
look at this const array:
https://github.com/krb5/krb5/blob/master/src/lib/gssapi/generic/gssapi_generic.c#L38

Code hygiene.

>> * 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?

Hmm, I'd still insist on somthing else, it just doesn't feel right.
Maybe JGSS_KRB5_TRACE with fprintf(stderr,...)? And later one you can
imrpove/replace it?

>> * 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...

That is ok, if you signal unsupported mech.

>> * 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.

I see. I am not aware of any Windows function because SSPI is at a
higher level than GSS-API. The user does not interact with OIDs. Why not
statically reserve a char array with OID strings?

>> * 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

Please see the third comment, it is from me. I have exprienced the same.
To be frank, I would accept it as-is because that is the way LSA works.
the way you do it now is as if you would use the internal KRB5 functions
from MIT Kerberos. It doesn't feel right.

>> * 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.

I wouldn't really rely on that. Otherwise you should always work with
_("string").

>> ** 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.

Wait a second, don't you deduce the realm from the krb5.conf? I have
been using JGSS for the past 10 years now and since referrals don't
really work, I have used the domain_realm setion to solve that problem,
e.g, blnn719x.ww004.siemens.net = AD001.SIEMENS.NET. If you are talking
about domainbased services ldap at dc1.ad001.siemens.net@ad001.siemens.net
Windows does support them, but there is no OID for, it uses UKNOWN or
hostbased with three components.

Windows does *not* require the realm with the SPN, unless for the
usecase about specified, it will apply proper routing by itself if the
namespaces are properly configured. We moved two years ago from one
forest to another with a bidirectional trust and everything just work
with referrals with MIT Kerberos and MS Kerberos.

Maybe you need to provide an example to me to understand the JGSS
shortcomings.

>> * gss_import_name():
>
> I'll make quite some changes in this function. I'll also reject non-Kerberos exports.

OK, I will wait for the change.

>> ** "    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.

Same as above, it probably needs some example for me.

>> ** 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.

I concur, all you need is the package name, the code remains the same.
The out handle will have the appropriate dwLower, dwUpper to LSA to the
mech.

>> ** 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.

Strike that, I just reread the API. Everything is fine. Though, I think
this shouldn't be necessary.

>> ** 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?

I'd accept it as-is or truncate to year 9999 for ISO 8601 compliance. A
note in the docs is sufficient about that fact. This binding has other
semantics.

>> ** 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.

So you are only thinking about a negative case?

>> * 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.

You should then log at least that if usage is not initiator.

>> * 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?

This is my understanding of the NO_ macros in MIT Kerbros. See also the
first hit:
https://github.com/krb5/krb5/search?q=GSS_C_NO_CONTEXT&unscoped_q=GSS_C_NO_CONTEXT

int initialContextToken = (*context_handle == GSS_C_NO_CONTEXT);

I'd always use the NO_ macros for cleaner code.

>> ** 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?

Consider to evaluate: ISC_REQ_ALLOCATE_MEMORY. Frees your from the
malloc() with the maxTokenSize from registry.

Michael