Thanks a lot. Some comments inline. > On Nov 29, 2018, at 1:55 AM, Nico Williams <nico.willi...@twosigma.com> wrote: > > On Tue, Nov 20, 2018 at 09:56:36AM +0800, Weijun Wang wrote: >> Please take a review at >> >> https://cr.openjdk.java.net/~weijun/6722928/webrev.01/ >> >> We ported [1] the native GSS bridge to Windows in JDK 11, but user still have >> to download and install a native GSS-API library. This code change provides a >> native GSS-API library inside JDK that can be used without setting the >> "sun.security.jgss.lib" system property. It is based on Windows SSPI and now >> only supports the client side using the default credentials. >> >> No regression tests included. A Windows Active Directory server is needed. > > This is my initial round of comments, and it's mostly high-level: > > - Right now this looks very basic, and you won't get support for any > mechanism other than Kerberos. That's fine though, as NTLM is dead, > you're willing to implement SPNEGO yourself, and there are no other > GSS mechs on Windows at this time (so far as I know). > > - You're constructing SPNEGO tokens yourself. I'll have to review that > very carefully.
I wrapped the outgoing token into NetTokenInit and extracted in incoming NegTokenTarg. This might fail if there are multiple rounds, but I think for kerberos it's OK. > > It would be better to let Windows provide SPNEGO though... But it might promote NTLM. > > - I've checked the DER-encoded OID constants. They are correct. > > - The name type OID for GSS_KRB5_NT_PRINCIPAL_NAME is not implemented but is > used elsewhere (e.g., in one GSSNameElement constructor). Look for > GSSUtil.NT_GSS_KRB5_PRINCIPAL. > > - gss_canonicalize_name() is not fully implemented. This will be noticeable > to callers of GSSNameElement's getKrbName(). In particular, permissions > checks will fail (e.g., in GSSCredElement's doServicePermCheck(); similarly > in NativeGSSContext). > > At minimum you absolutely must parse generic GSS name type names into > Kerberos-style names (e.g., service@hostname -> service/hostname@). OK. > > When an MN is displayed, you need to output the Kerberos-style name. > > (For those following along, "MN" means "mechanism name", which means > a GSSName / gss_name_t instance which is "canonical" and whose > display form will be mechanism-specific. gss_canonicalize_name() is > supposed to output a "canonical" version of its input.) > > You'll also have to determine a realm for the parsed principal names. > > You won't need a realm for services. Use an empty realm name for > services -- that's the Heimdal/MIT (and Solaris) convention for when > you're willing to rely on KDC referrals. > > (I would also urge you to NOT attempt any DNS canonicalization of > hostnames. Let's not further spread that mistake.) I didn't. > > For user principals it's trickier, and I think you might need some notion of > default realm, but a) maybe you can get away with an empty realm here on > Windows, b) I'm not sure where you'd a default realm on Windows. I'm using the environment variable USERDNSDOMAIN now. > > What we do to determine the default realm for user names in our > patched version of Martin Rex's GSS->SSPI bridge is call > AcquireCredentialsHandle() or QueryCredentialsAttributesW() to > get/query the default credential and get its name, and from that the > realm, and we use that as the default realm. > > I'll have to look and see how we handle host-based service names > (Martin's original code has a domain_realm table in the registry, but > we removed all of that and rely on referrals instead.) > > - If you can get Legal approval for including / distributing a fork of > Martin Rex's bridge, you'll get all of the above functionality and > also acceptor functionality. Anyway, one can use his bridge with -Dsun.security.jgss.lib. Thanks Max > > Nico > --
