On Mon, Apr 29, 2019 at 02:35:28PM +0200, Michael Osipov wrote:
> 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.
KRB5_TRACE is also used by MIT Kerberos. I don't know if the KfW
(Kerberos for Windows) builds of MIT Kerberos use this, but just in case
I'd call it something else.
> sspi.cpp:
> * KRB5_TRACE moved to gss_indicate_mechs which is no where called in
> this file.
> ** How is this supposed to work then?
See src/java.security.jgss/share/native/libj2gss/{NativeFunc.c,
GSSLibStub.c}, and
src/java.security.jgss/share/classes/sun/security/jgss/wrapper/GSSLibStub.java
where gss_indicate_mechs() is known as indicateMechs().
> ** FILE *trace seems to be global, can this lead to race conditions
> leaving open file descriptors?
Yes. There should be a lock or else this should be installed with an
atomic CAS and never disabled.
(I'd prefer for tracing to be possible to enable and disable at
run-time, but here I think it's acceptable to have it not be possible to
disable at run-time.)
> * 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 for "name canonicalization".
> ** I don't like the idea using Heimdal-internal identifiers. Shouldn't
> we define JGSS specific ones? At least create a define for.
Well, that's commented out, and anyways, it's a "well-known" realm name,
which the RFCs permit. Using a well-known realm name from Heimdal is
safe, but Weijun could just allocate one from an OpenJDK namespace.
> ** your concat fails if USERDNSDOMAIN is empty, you end up ith
> service/instance@
That's solution #2, and intended.
> ** Why do you check for '\\' what can be escaped here? Requires a better
> comment
Basically, Kerberos principal names consist of N "components" and a
realm name. In RFC4120 (successor to RFC1510) there is no text
representation given for principal names -- it's just ASN.1 types.
RFC1964 (updated by RFC4121) defines the GSS-API mechanism based on
Kerberos, and defines a textual representation of principal names using
'/' to join/separate components, and '@ to join/separate the realm.
Because '/' and '@' (and '\\') are not forbidden in principal name
components and realm names, RFC1964 requires backslash-escaping of them.
Thus, the principal name joe\@foo.example@FOO.EXAMPLE has 1 component
("joe@foo.example") and a realm name "FOO.EXAMPLE". joe\@@FOO.EXAMPLE
is also a valid principal name.
> ** Why do you do "!input[i]"?
> I know that we don't have host to realm mappings like in MIT Kerberos
These are C strings, thus NUL-terminated.
In principle Kerberos principal names allow embedded NULs(!). I'm not
sure it's wise to support that here. Passing in "foo\\" might cause
this code to read past the end of the string!
> * gss_import_name():
> ** BOOLEAN isNegotiate isn't really readable code
It's BOOLEAN isSPNEGO in webrev.06. I think that's readable.
> ** " value[len] = 0;" rather '\0'? This idiom repeats over and over
I'm OK with that. I dunno what the OpenJDK style guides have to say
about this.
> ** "if (value[len-1] == '@') {" rather L"@"? This continues in the function
Good catch! Does the compiler coerce this automatically?
> ** "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()
+1
Nico
--
