Am 2019-04-16 um 09:56 schrieb Weijun Wang:
There is been some time but I've uploaded a new webrev at
https://cr.openjdk.java.net/~weijun/6722928/webrev.06
Major changes:
1. No more get expiration time from TGT, accept the one from
AcquireCredentialsHandle, which makes GSS_C_INDEFINITE.
2. KRB5_TRACE can be set to any file name. /dev/stderr and stderr are
recognized.
Hi Max,
here is my static review of the code. Some spots are still fishy to me
and not resolved by my previous comments:
gssapi.h:
* Still has indentation issues
sspi.cpp:
* KRB5_TRACE moved to gss_indicate_mechs which is no where called in
this file.
** How is this supposed to work then?
** FILE *trace seems to be global, can this lead to race conditions
leaving open file descriptors?
* showTime(): please use a readable format akin to %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
** If you log the token size you should also log if SecurityStatus isn't
positive, just in case
** new_context minor_status is never written, 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
** I don't like the idea using Heimdal-internal identifiers. Shouldn't
we define JGSS specific ones? At least create a define for.
** your concat fails if USERDNSDOMAIN is empty, you end up ith
service/instance@
** Why do you check for '\\' what can be escaped here? Requires a better
comment
** Why do you do "!input[i]"?
I know that we don't have host to realm mappings like in MIT Kerberos
* gss_import_name():
** BOOLEAN isNegotiate isn't really readable code
** " value[len] = 0;" rather '\0'? This idiom repeats over and over
** "if (value[len-1] == '@') {" rather L"@"? This continues in the function
** "if (value[len-1] == '@') {" you should comment this block and
explain why you are doing this. Is this because of "@ignore_me_rfcXXX"?
** SPN conversion, why do you replace the '@' with '/' explicitly for
SSPI? A non-mech specific hostbased service is always neutral with '@'
* 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
* gss_export_name(): probably the same issues as with gss_import_name()
* gss_init_sec_context():
** Line 909, wrong case for package name
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.
Michael
