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

[Weijun Wang]

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