Code review of ts jgss jdk12 #1

pburka · 2019-03-04T16:46:24Z

No description provided.

Also improve object size handling in NativeUtil.

Also avoid memory allocation in newGSSOIDSet() renamed to makeGSSOIDset() which now takes a singleton set argument and either assigns the requested OID or with SPNEGO returns a static list of all the supported mechs. With this we no longer need deleteGSSOIDSet().

We must not dispose() of any credential handle passed to the NativeGSSContext() constructor. But we must dispose() of credentials that are acquired in NativeGSSContext. This is very important because the JVM does not know about the size of the JNI credential objects, so it doesn't readily recognize memory pressure from them, leading to memory pressure issues in SASL and GSS server applications.

There is only one token that we can extract an actual mechanism OID from in the SPNEGO case when the native GSS library doesn't provide that (though it should) in the API. If the SPNEGO exchange ends up requiring more than two tokens, then the previous code failed to establish a security context. Also, never raise if we cannot get an actual mech OID from SPNEGO tokens.

Also use empty realm as wildcard for krbtgt names

This module is to be used for GSS applications in preference to Krb5LoginModule, especially when using the native GSS provider.

Also don't force same name for acceptor and initiator.

Add commentary about native in Krb5LoginModule

We were reacquiring the initiator/acceptor credential upon security context full establishment in order to indirectly perform a permission check on the srcName/targName once we find out what they are. But this is just one more way to end up failing, which happens with Heimdal when using SPNEGO because we ask to acquire a Kerberos credentials using a SPNEGO MN and that fails. Also, there was a security bug here: if the permission check fails then we raise, but if the application already has a context handle, then it can use it anyways if it catches the exception! The fix for this is to dispose() when the permission check fails.

Native objects are memory icebergs: they are much larger than the JVM knows, so the GC might not dispose of them soon enough.

pburka · 2019-03-04T16:51:44Z

src/java.security.jgss/share/classes/javax/security/auth/kerberos/ServicePermission.java

+             this.getName().startsWith("krbtgt/")))
+            return true;
+
+        char[] n = this.getName().toCharArray();


Converting this to a char[] is an unnecessary copy. Just operate on the String directly.

pburka · 2019-03-04T16:53:04Z

src/java.security.jgss/share/classes/javax/security/auth/kerberos/ServicePermission.java

+                continue;
+            }
+            if (n[i] == '@') {
+                String s = new String(n, 0, i + 1);


This allocation is unnecessary. Use String#regionMatches instead.

pburka · 2019-03-04T16:58:07Z

src/java.security.jgss/share/classes/sun/security/jgss/GSSUtil.java

-                debug("Skipped name " + name + " due to " + ge);
-            }
-        }
+        Set<GSSName> names = new HashSet<GSSName>();


Use new HashSet<>() (it's no longer necessary to repeat GSSName here).

Ah, this is from having forward-ported from JDK 7.

pburka · 2019-03-04T17:01:31Z

src/java.security.jgss/share/classes/sun/security/jgss/GSSUtil.java

+                            return result;
+                        }
+
+                        result = new Vector<T>();


I know this is pre-existing, but Vectors are very 1998. Consider using an ArrayList.

I might leave this one in the interest of time.

pburka · 2019-03-04T17:03:49Z

src/java.security.jgss/share/classes/sun/security/jgss/GSSUtil.java

+                        Iterator<GSSCredentialImpl> iterator =
+                            accSubj.getPrivateCredentials
+                            (GSSCredentialImpl.class).iterator();
+                        while (iterator.hasNext()) {


Again, pre-existing code, but consider using a for-each here. (for (GSSCredentialImpl cred : accSubj.getPrivateCredentials(...)) ...

pburka · 2019-03-04T19:13:34Z