> On May 28, 2019, at 8:55 AM, Weijun Wang <weijun.w...@oracle.com> wrote:
>
> 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....@dev.example.com now. As Michael pointed out, it
> could well be service/host.example....@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?
Or I can always add the current domain name (env var USERDNSDOMAIN) to a
host-based service, and in InitSecurityContext always strip the domain part of
it is the current domain name.
--Max
>
> Thanks,
> Max
>
>> 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
>>
>
