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