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

[Michael Osipov]

I will have a look at the outstanding emails today.

Am 2019-05-28 um 02:55 schrieb Weijun Wang:
> Hi Everyone,
>
> Do you have any new comments?
>
> My major concern now is the canonicalization of service/host.dev.example.com to service/host.example.com at DEV.EXAMPLE.COM now. As Michael pointed out, it could well be service/host.example.com at EXAMPLE.COM.
>
> My suggestion now is to strip the realm part when InitSecurityContext is called. But when? If always, some information is lost if the realm is provided by the caller. So, how about we add "@WELLKNOWN:ORG.H5L.REFERALS-REALM" when it's a host-based service name?
>
> Thanks,
> Max
>
>> On May 5, 2019, at 4:33 PM, Weijun Wang <weijun.wang at oracle.com> wrote:
>>
>> Hi Michael and Nico,
>>
>>>
>>> sspi.cpp:
>>> * KRB5_TRACE
>>
>> If you think a different name is better I'll change it. And then I'd like to revert to my old code that it always print to stderr. I don't have a need to print it to any file.
>>
>>> * showTime(): please use a readable format akin to %FT%T
>>
>> What is %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
>>
>> What I really want to express here is that I am only supporting 2 mechanisms now, and using a WCHAR might lead to an unsupported one. I also don't want to do OID<->string translation a lot. If you are only unsatisfied with the name, is negOrKrb5 better?
>>
>>> ** If you log the token size you should also log if SecurityStatus isn't
>>> positive, just in case
>>
>> I didn't use cbMaxMessage. Will remove it.
>>
>>> ** new_context minor_status is never written, remove it?
>>
>> It was used by the SEC_SUCCESS macro. Now that I won't call QuerySecurityPackageInfo it's useless. Will remove it.
>>
>>>
>>> * get_full_name()!!!:
>>> ** 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
>>
>> It's only used by gss_export and gss_canonicalize. A service must have a realm in the output of these 2 functions. The realm is not used in init_sec_context.
>>
>>> ** I don't like the idea using Heimdal-internal identifiers. Shouldn't
>>> we define JGSS specific ones? At least create a define for.
>>
>> Well MIT krb5 is already using it (I see it in the exported byte array) so I think it's fine. I don't want to invent a new one. However I cannot use it now because of the permission check. I would think about supporting realm-less name in ServicePermission.
>>
>>> ** your concat fails if USERDNSDOMAIN is empty, you end up ith
>>> service/instance@
>>
>> That's sad, but I think it should never be empty if the client machine is in a domain and that's what this library wants to support.
>>
>>> ** Why do you check for '\\' what can be escaped here? Requires a better
>>> comment
>>
>> I think Nico answered this in full detail.
>>
>>> * gss_import_name():
>>> ** BOOLEAN isNegotiate isn't really readable code
>>> ** "    value[len] = 0;" rather '\0'? This idiom repeats over and over
>>
>> Sometimes it's '\0' and sometimes it could even be L'\0'. I really don't want to make it too precise here.
>>
>>> ** "if (value[len-1] == '@') {" rather L"@"? This continues in the function
>>
>> Yes.
>>
>>> ** "if (value[len-1] == '@') {" you should comment this block and
>>> explain why you are doing this. Is this because of "@ignore_me_rfcXXX"?
>>
>> I suspect there will be empty/ignorable realms.
>>
>>> ** SPN conversion, why do you replace the '@' with '/' explicitly for
>>> SSPI? A non-mech specific hostbased service is always neutral with '@'
>>
>> I just want to support Kerberos, and I don't want to replace it before calling InitSecContext.
>>
>>>
>>> * 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
>>
>> The realm part is a problem because I cannot get the real one. The '@' should not be changed to '/' if name type is not HOSTBASED-SERVICE.
>>
>>> * gss_export_name(): probably the same issues as with gss_import_name()
>>>
>>>
>>> * gss_init_sec_context():
>>> ** Line 909, wrong case for package name
>>
>> The one in PackageList? Fixed. It did work.
>>
>>>
>>> I will try to setup a compile env after the next webrev and see how far
>>> I get. I have enough cross-realm stuff around here.
>>
>> Great. If an app calls canonicalize/export and feed back the result into InitSecContext then there might be a problem. Maybe I should always strip the realm part before calling it? That sound like a part of the information is lost. Or maybe it's really useless for Windows? I know giving a wrong realm will fail.
>>
>> Or maybe I should use "WELLKNOWN:ORG.H5L.REFERALS-REALM". I'll do some experiment.
>>
>> Thanks,
>> Max
>>
>>>
>>> Michael
>>
>
>