On May 5, 2019, at 4:33 PM, Weijun Wang <weijun.w...@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 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
