JGSS Enhancements (contribution by Two Sigma Open Source)
Nico Williams
Nico.Williams at twosigma.com
Tue May 7 16:53:35 UTC 2019
On Thu, Mar 21, 2019 at 11:01:32AM +0800, Weijun Wang wrote:
> All of them at
>
> https://bugs.openjdk.java.net/issues/?jql=assignee%20%3D%20nwilliams
>
> You might want to add more descriptions.
Below is the commentary I've collated from GitHub, along with my
responses.
- src/java.security.jgss/share/classes/javax/security/auth/kerberos/ServicePermission.java
By: Peter Burka
Status: ACCEPT
Comment:
> + * especially when they are using a keytab.
+ *
+ * If the user is using a password, then the realm matters more. An
+ * untrusted actor could cause KDCs for a realm they control to see
+ * material they could attack offline, but that was already the case
+ * anyways, and the answer is the same in all cases: use stronger
+ * passwords, use randomized keys in a keytab, or let us implement
+ * SPAKE or similar alternatives to the venerable PA-ENC-TIMESTAMP.
+ */
+ if ((this.getName().equals("krbtgt/@") &&
+ p.getName().startsWith("krbtgt/")) ||
+ (p.getName().equals("krbtgt/@") &&
+ 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.
- src/java.security.jgss/share/classes/javax/security/auth/kerberos/ServicePermission.java
By: Peter Burka
Status: ACCEPT
Comment:
> + */
+ if ((this.getName().equals("krbtgt/@") &&
+ p.getName().startsWith("krbtgt/")) ||
+ (p.getName().equals("krbtgt/@") &&
+ this.getName().startsWith("krbtgt/")))
+ return true;
+
+ char[] n = this.getName().toCharArray();
+ int i;
+ for (i = 0; i < n.length; i++) {
+ if (n[i] == '\\') {
+ i++;
+ continue;
+ }
+ if (n[i] == '@') {
+ String s = new String(n, 0, i + 1);
This allocation is unnecessary. Use String#regionMatches instead.
- src/java.security.jgss/share/classes/sun/security/jgss/GSSUtil.java
By: Peter Burka
Status: ACCEPT
Comment:
> - if (name instanceof GSSNameImpl) {
- try {
- GSSNameSpi ne = ((GSSNameImpl) name).getElement
- (GSS_KRB5_MECH_OID);
- String krbName = ne.toString();
- if (ne instanceof Krb5NameElement) {
- krbName =
- ((Krb5NameElement) ne).getKrb5PrincipalName().getName();
- }
- KerberosPrincipal krbPrinc = new KerberosPrincipal(krbName);
- krb5Principals.add(krbPrinc);
- } catch (GSSException ge) {
- 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).
- src/java.security.jgss/share/classes/sun/security/jgss/GSSUtil.java
By: Peter Burka
Status: REJECT
Comment:
> - while (iterator.hasNext()) {
- GSSCredentialImpl cred = iterator.next();
- debug("...Found cred" + cred);
- try {
- GSSCredentialSpi ce =
- cred.getElement(mech, initiate);
- debug("......Found element: " + ce);
- if (ce.getClass().equals(credCls) &&
- (name == null ||
- name.equals((Object) ce.getName()))) {
+ if (accSubj == null) {
+ debug("No Subject");
+ return result;
+ }
+
+ result = new Vector<T>();
I know this is pre-existing, but Vectors are very 1998. Consider using an ArrayList.
- src/java.security.jgss/share/classes/sun/security/jgss/GSSUtil.java
By: Peter Burka
Status: REJECT
Comment:
> - GSSCredentialSpi ce =
- cred.getElement(mech, initiate);
- debug("......Found element: " + ce);
- if (ce.getClass().equals(credCls) &&
- (name == null ||
- name.equals((Object) ce.getName()))) {
+ if (accSubj == null) {
+ debug("No Subject");
+ return result;
+ }
+
+ result = new Vector<T>();
+ 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(...)) ...`
- src/jdk.security.auth/share/classes/com/sun/security/auth/module/GssLoginModule.java
By: Peter Burka
Status: ACCEPT
Comment:
> @@ -65,8 +66,17 @@ public LoginConfigImpl(GSSCaller caller, Oid mech) {
this.caller = caller;
- if (mech.equals(GSSUtil.GSS_KRB5_MECH_OID)) {
+ useNative = "true".equalsIgnoreCase(
+ System.getProperty("sun.security.jgss.native"));
+
Consider `useNative = Boolean.getBoolean("sun.security.jgss.native")`
- src/java.security.jgss/share/classes/sun/security/jgss/krb5/Krb5AcceptCredential.java
- src/java.security.jgss/share/classes/sun/security/jgss/krb5/Krb5InitCredential.java
- src/java.security.jgss/share/classes/sun/security/jgss/wrapper/GSSCredElement.java
By: Peter Burka
Status: ACCEPT
Comment:
> @@ -47,6 +47,7 @@
private final Krb5NameElement name;
private final ServiceCreds screds;
+ private boolean isDefCred = false;
Can you make this final?
- src/java.security.jgss/share/classes/sun/security/jgss/krb5/Krb5InitCredential.java
By: Peter Burka
Status: ACCEPT
Comment:
> @@ -134,7 +137,8 @@ private Krb5InitCredential(Krb5NameElement name,
this.name = name;
// A delegated cred does not have all fields set. So do not try to
- // creat new Credentials out of the delegatedCred.
+ // creat new Credentials out of the delegatedCred. Also, a delegated
"create", unless you're referring to O_CREAT.
- src/java.security.jgss/share/classes/sun/security/jgss/krb5/Krb5AcceptCredential.java
- src/java.security.jgss/share/classes/sun/security/jgss/krb5/Krb5InitCredential.java
- src/java.security.jgss/share/classes/sun/security/jgss/krb5/Krb5NameElement.java
- src/java.security.jgss/share/classes/sun/security/jgss/spi/GSSCredentialSpi.java
- src/java.security.jgss/share/classes/sun/security/jgss/spi/GSSNameSpi.java
- src/java.security.jgss/share/classes/sun/security/jgss/spnego/SpNegoCredElement.java
- src/java.security.jgss/share/classes/sun/security/jgss/wrapper/GSSCredElement.java
- src/java.security.jgss/share/classes/sun/security/jgss/wrapper/GSSNameElement.java
By: Peter Burka
Status: ACCEPT
Comment:
+ /**
+ * Returns true if the credential is a default credential.
+ *
+ * @return true if the credential is a default credential, else false.
+ */
+ public boolean isDefaultCredential() {
`final`, unless you expect subclasses to override.
- src/java.security.jgss/share/classes/sun/security/jgss/krb5/Krb5AcceptCredential.java
- src/java.security.jgss/share/classes/sun/security/jgss/krb5/Krb5InitCredential.java
- src/java.security.jgss/share/classes/sun/security/jgss/wrapper/GSSCredElement.java
By: Peter Burka
Status: ACCEPT
Response: will make private and final
Comment:
> @@ -42,9 +42,14 @@
long pCred; // Pointer to the gss_cred_id_t structure
private GSSNameElement name = null;
private GSSLibStub cStub;
+ public boolean isDefCred;
Are you sure you want this to be public? Especially since it's not
final! What if someone else changes it?
- src/java.security.jgss/share/classes/sun/security/jgss/wrapper/GSSNameElement.java
By: Peter Burka
Status: ACCEPT
Response: huh, might want to do the same for GSSLibStub.java...
Comment:
> @@ -53,6 +53,7 @@
long pName = 0; // Pointer to the gss_name_t structure
private String printableName;
private Oid printableType;
+ private Oid mech;
Can this be made final?
- src/java.security.jgss/share/classes/sun/security/jgss/wrapper/NativeGSSContext.java
By: Peter Burka
Status: ACCEPT
Comment:
> private int flags;
private int lifetime = GSSCredential.DEFAULT_LIFETIME;
private final GSSLibStub cStub;
- private boolean skipDelegPermCheck;
- private boolean skipServicePermCheck;
+ private boolean skipDelegPermCheck = false;
This isn't strictly necessary. `false` is already the default value. (But explicit initialization is clearer, IMO.)
- src/java.security.jgss/share/classes/sun/security/jgss/wrapper/NativeGSSContext.java
By: Peter Burka
Status: REJECT
Comment:
> }
+ } catch (GSSException ge) {
+ dispose();
This should be a finally block. I presume you want to call `dispose()` if _any_ exception occurs, not just `GSSException`.
Reason: in the success case we need `this`. In the failure case we
want to dispose() early because the object is an iceberg to
the Java GC: it's much larger than the GC knows, so if these
accumulate there will be more memory pressure than the GC
knows about.
- src/java.security.jgss/share/classes/sun/security/jgss/wrapper/NativeGSSContext.java
By: Peter Burka
Status: ACCEPT
Comment:
>
// Do Service Permission check when importing SPNEGO context
- // just to be safe
+ // just to be safe. WAT, no. If the caller has an exported sec
Let's not push 'Wat no'
- src/java.security.jgss/share/classes/sun/security/jgss/wrapper/NativeGSSContext.java
By: Peter Burka
Status: HMMM
Comment:
> @@ -346,9 +363,13 @@ public boolean isEstablished() {
}
public void dispose() throws GSSException {
Is this meant to be thread safe? (It doesn't look thread safe.)
Reason: It's not really thread-safe, no, but it's already so before
my changes, and a) we don't dispose() of objects passed by
the caller, only those we create ephemerally. The
application should not call dispose() on objects that might
be used by other threads concurrently. Perhaps we should
document that.
Or we could make synchronized all the native GSS methods that
refer to these C pointers... I guess that's not a big deal.
- src/java.security.jgss/share/native/libj2gss/NativeUtil.c
By: Peter Burka
Status: REJECT
Comment:
> @@ -455,6 +455,14 @@ void throwOutOfMemoryError(JNIEnv *env, const char *message) {
throwByName(env, "java/lang/OutOfMemoryError", message);
}
+static jsize
+safe_jsize(size_t n)
+{
+ jsize res = (jsize)n;
I'm not convinced this function is safe. I suspect it invokes
undefined behavior more than once.
Reason: I believe this function is safe indeed. It's using C casts
to detect whether a size_t value is too large to represent as
a jsize value.
- src/java.security.jgss/share/native/libj2gss/NativeUtil.c
By: Peter Burka
Status: ACCEPT
Comment:
>
- (*env)->SetByteArrayRegion(env, jbytes, 0, len, (jbyte *) bytes->value);
- if ((*env)->ExceptionCheck(env)) {
- goto finish;
- }
+ /* constructs the String object with new String(byte[]) */
This was pre-existing, but it's likely unsafe. This will use
whatever the default Java character encoding is. You probably want
to use an explicit code page.
And if you're not going to do that, it would be simpler to decode
the name yourself and use `(*env)->NewString(...)`
Response: OK, will use NewStringUTF().
- src/java.security.jgss/share/native/libj2gss/NativeUtil.c
By: Peter Burka
Status: ACCEPT
Comment:
> void* value;
- if (jbytes != NULL) {
- len = (*env)->GetArrayLength(env, jbytes);
- value = malloc(len);
- if (value == NULL) {
+ cbytes->length = 0;
+ cbytes->value = NULL;
+
+ if (jbytes == NULL ||
+ (len = (*env)->GetArrayLength(env, jbytes)) == 0)
+ return;
+
+ cbytes->length = len;
+
+ if (wantCopy == JNI_FALSE) {
+ cbytes->value = (*env)->GetByteArrayElements(env, jbytes, &isCopy);
If you don't plan to read `isCopy`, just pass in `NULL`.
- src/java.security.jgss/share/native/libj2gss/NativeUtil.c
By: Peter Burka
Status: IGNORE
Comment:
> void* value;
- if (jbytes != NULL) {
- len = (*env)->GetArrayLength(env, jbytes);
- value = malloc(len);
- if (value == NULL) {
+ cbytes->length = 0;
+ cbytes->value = NULL;
+
+ if (jbytes == NULL ||
+ (len = (*env)->GetArrayLength(env, jbytes)) == 0)
+ return;
+
+ cbytes->length = len;
+
+ if (wantCopy == JNI_FALSE) {
+ cbytes->value = (*env)->GetByteArrayElements(env, jbytes, &isCopy);
... and what will you do if `GetByteArrayElements` doesn't return a copy to you??
Response: If we want a copy, then we malloc() and copy (and later
free()). Otherwise we rely on ReleaseByteArrayElements()
to release the copy.
- src/java.security.jgss/share/native/libj2gss/NativeUtil.c
By: Peter Burka
Status: REJECT
Comment:
> void* value;
- if (jbytes != NULL) {
- len = (*env)->GetArrayLength(env, jbytes);
- value = malloc(len);
- if (value == NULL) {
+ cbytes->length = 0;
+ cbytes->value = NULL;
+
+ if (jbytes == NULL ||
+ (len = (*env)->GetArrayLength(env, jbytes)) == 0)
+ return;
+
+ cbytes->length = len;
+
+ if (wantCopy == JNI_FALSE) {
+ cbytes->value = (*env)->GetByteArrayElements(env, jbytes, &isCopy);
I think this whole function is unnecessarily complicated. In
practice, any modern JVM will return a copy from
GetByteArrayElements, anyway. So what do you gain from having two
codepaths here?
Response: This is an artifact of our support for 7, 8, .., master.
We could rely on the copy behavior, but for now I'll leave
it as-is.
- src/java.security.jgss/share/native/libj2gss/NativeUtil.c
By: Peter Burka
Status:
Comment:
> }
}
+/*
+ * Utility routine for initializing gss_buffer_t structure
+ * with a String.
+ * NOTE: need to call resetGSSBufferString(...) to free up
+ * the resources.
+ */
+void initGSSBufferString(JNIEnv* env, jstring jstr, gss_buffer_t buf)
+{
+ const char *s;
+
+ buf->length = 0;
+ buf->value = NULL;
+ if (jstr != NULL) {
+ s = (*env)->GetStringUTFChars(env, jstr, NULL);
Does GSS expect UTF8 encoded strings?
Response: Yes. GSS is undespecified as to codeset, but UTF-8 is the
only thing that makes sense.
- src/java.security.jgss/share/native/libj2gss/NativeUtil.c
By: Peter Burka
Status: ACCEPT
Comment:
> + } else {
+ buf->length = strlen(s);
+ buf->value = (char *)s; /* Drop const */
+ }
+ }
+}
+
+/*
+ * Utility routine for unpinning/releasing the String
+ * associated with the specified jstring object.
+ * NOTE: used in conjunction with initGSSBufferString(...).
+ */
+void resetGSSBufferString(JNIEnv *env, jstring jstr, gss_buffer_t buf)
+{
+ if (jstr != NULL && buf->value != NULL)
+ (*env)->ReleaseStringUTFChars(env, jstr, buf->value);
`buf->value = NULL`
Response: Ah yes, we're assuming that length == 0 -> no need to
release. That's not necessarily true.
- src/java.security.jgss/share/native/libj2gss/NativeUtil.c
By: Peter Burka
Status: REJECT
Comment:
> @@ -724,66 +786,47 @@ jobject getJavaOID(JNIEnv *env, gss_OID cOid) {
if (jbytes == NULL) {
return NULL;
}
- (*env)->SetByteArrayRegion(env, jbytes, 0, 2, (jbyte *) oidHdr);
- if ((*env)->ExceptionCheck(env)) {
- return NULL;
+ if (!(*env)->ExceptionCheck(env)) {
Consider using `GetPrimitiveArrayCritical()` instead. I think it would result in simpler, faster code.
pburka commented on this pull request.
Reason: This doesn't have to be faster, and it's simple enough.
- src/jdk.security.auth/share/classes/com/sun/security/auth/module/GssLoginModule.java
By: Peter Burka
Status: ACCEPT
Comment:
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+package com.sun.security.auth.module;
+
+import java.io.*;
Generally, wildcard imports are discouraged in modern code.
Commentary: well, it was in the original, in Krb5LoginModule.
- src/jdk.security.auth/share/classes/com/sun/security/auth/module/GssLoginModule.java
By: Peter Burka
Status: REJECT
Comment:
> +
+import org.ietf.jgss.GSSManager;
+import org.ietf.jgss.GSSException;
+import org.ietf.jgss.GSSContext;
+import org.ietf.jgss.GSSCredential;
+import org.ietf.jgss.GSSName;
+import org.ietf.jgss.Oid;
+
+import static sun.security.util.ResourcesMgr.getAuthResourceString;
+
+/**
+ * <p>This <code>LoginModule</code> authenticates users using the
+ * GSS-API.</p>
+ */
+
+public class GssLoginModule implements LoginModule {
Is this class meant to be subclassed? If not, it should be final.
Reason: All the *LoginModule classes are non-final. Perhaps they
should be final, but I won't make that decision.
- src/jdk.security.auth/share/classes/com/sun/security/auth/module/GssLoginModule.java
By: Peter Burka
Status: ACCEPT
Comment:
> + * It stands to reason that when sun.security.jgss.native=false the
+ * login modules corresponding to the actual GSS mechanisms coded in
+ * Java are the ones that should be acquiring their corresponding
+ * credentials.
+ *
+ * A policy like "let the application use GSS credentials but not the
+ * raw, underlying Krb5 credentials" when
+ * sun.security.jgss.native=false" could be expressible by adding a
+ * module option to Krb5LoginModule that causes it to add only GSS
+ * credentials to the Subject, not Krb5 credentials.
+ *
+ * (It has never been possible to express such a policy, so we lose
+ * nothing by punting here when sun.security.jgss.native=false.)
+ */
+ useNative = "true".equalsIgnoreCase(
+ System.getProperty("sun.security.jgss.native"));
Consider `Boolean.getBoolean("sun.security.jgss.native")`
- src/jdk.security.auth/share/classes/com/sun/security/auth/module/GssLoginModule.java
By: Peter Burka
Status: ACCEPT
Comment:
> + * module option to Krb5LoginModule that causes it to add only GSS
+ * credentials to the Subject, not Krb5 credentials.
+ *
+ * (It has never been possible to express such a policy, so we lose
+ * nothing by punting here when sun.security.jgss.native=false.)
+ */
+ useNative = "true".equalsIgnoreCase(
+ System.getProperty("sun.security.jgss.native"));
+ if (!useNative)
+ return;
+
+ manager = GSSManager.getInstance();
+
+ // initialize any configured options
+
+ debug = "true".equalsIgnoreCase((String)options.get("debug"));
Consider `Boolean.parseBoolean((String)options.get("debug"))`
- src/jdk.security.auth/share/classes/com/sun/security/auth/module/GssLoginModule.java
By: Peter Burka
Status: ACCEPT
Comment:
> + *
+ * (It has never been possible to express such a policy, so we lose
+ * nothing by punting here when sun.security.jgss.native=false.)
+ */
+ useNative = "true".equalsIgnoreCase(
+ System.getProperty("sun.security.jgss.native"));
+ if (!useNative)
+ return;
+
+ manager = GSSManager.getInstance();
+
+ // initialize any configured options
+
+ debug = "true".equalsIgnoreCase((String)options.get("debug"));
+ doNotPrompt =
+ "true".equalsIgnoreCase(getWithDefault("doNotPrompt", "true"));
ditto.
- src/jdk.security.auth/share/classes/com/sun/security/auth/module/GssLoginModule.java
By: Peter Burka
Status: REJECT
Comment:
> +
+ debug = "true".equalsIgnoreCase((String)options.get("debug"));
+ doNotPrompt =
+ "true".equalsIgnoreCase(getWithDefault("doNotPrompt", "true"));
+ defName = (String)options.get("name");
+ nametype = (String)options.get("nametype");
+
+ if (defName == null)
+ defName = System.getProperty("sun.security.gss.name");
+ if (nametype == null)
+ nametype = System.getProperty("sun.security.gss.nametype");
+ if (nametype == null || nametype.equals("username")) {
+ nametypeOid = GSSName.NT_USER_NAME;
+ } else if (nametype.equals("hostbased")) {
+ nametypeOid = GSSName.NT_HOSTBASED_SERVICE;
+ } else if (!nametype.equals("")) {
`!nametype.isEmpty()`
Reason: it's good enough as it is.
> + nametypeOid = GSSName.NT_HOSTBASED_SERVICE;
+ } else if (!nametype.equals("")) {
+ try {
+ nametypeOid = new Oid(nametype);
+ } catch (GSSException e) {
+ if (debug)
+ System.out.print("Unknown name type OID " + nametype);
+ nametypeOid = null;
+ }
+ } else {
+ nametype = "<default: username>";
+ nametypeOid = GSSName.NT_USER_NAME;
+ }
+
+ tryFirstPass =
+ "true".equalsIgnoreCase(getWithDefault("tryFirstPass", "true"));
`Boolean.parseBoolean`
- src/jdk.security.auth/share/classes/com/sun/security/auth/module/GssLoginModule.java
By: Peter Burka
Status: ACCEPT
Comment:
> + try {
+ nametypeOid = new Oid(nametype);
+ } catch (GSSException e) {
+ if (debug)
+ System.out.print("Unknown name type OID " + nametype);
+ nametypeOid = null;
+ }
+ } else {
+ nametype = "<default: username>";
+ nametypeOid = GSSName.NT_USER_NAME;
+ }
+
+ tryFirstPass =
+ "true".equalsIgnoreCase(getWithDefault("tryFirstPass", "true"));
+ useFirstPass =
+ "true".equalsIgnoreCase(
`Boolean.parseBoolean`
- src/jdk.security.auth/share/classes/com/sun/security/auth/module/GssLoginModule.java
By: Peter Burka
Status: ACCEPT
Comment:
> + useFirstPass =
+ "true".equalsIgnoreCase(
+ getWithDefault("useFirstPass",
+ doNotPrompt ? "true" : "false"));
+ storePass =
+ "true".equalsIgnoreCase((String)options.get("storePass"));
+ clearPass =
+ "true".equalsIgnoreCase((String)options.get("clearPass"));
+ initiate =
+ "true".equalsIgnoreCase((String)options.get("initiate"));
+ accept =
+ "true".equalsIgnoreCase((String)options.get("accept"));
+ tryDefaultCreds =
+ "true".equalsIgnoreCase(getWithDefault("tryDefaultCreds", "true"));
+ useDefaultCreds =
+ "true".equalsIgnoreCase(
ditto ditto ditto ditto
- src/jdk.security.auth/share/classes/com/sun/security/auth/module/GssLoginModule.java
By: Peter Burka
Status: REJECT
Comment:
> + le.getMessage());
+ }
+ if (useFirstPass)
+ throw le;
+ }
+
+ // The first password didn't work or we didn't try it, try prompting
+ try {
+ attemptAuthentication(false);
+ if (debug)
+ System.out.println("\t\t[GssLoginModule] " +
+ "authentication succeeded");
+ succeeded = true;
+ cleanState();
+ return true;
+ } catch (LoginException le2) {
This looks weirdly like 1e2
Reason: but obviously it cannot be a number...
- src/jdk.security.auth/share/classes/com/sun/security/auth/module/GssLoginModule.java
By: Peter Burka
Status: REJECT
Comment:
> + }
+ if (useFirstPass)
+ throw le;
+ }
+
+ // The first password didn't work or we didn't try it, try prompting
+ try {
+ attemptAuthentication(false);
+ if (debug)
+ System.out.println("\t\t[GssLoginModule] " +
+ "authentication succeeded");
+ succeeded = true;
+ cleanState();
+ return true;
+ } catch (LoginException le2) {
+ cleanState();
This should be in a `finally` block, I think.
Reason: You're right that cleanState() could be called in a `finally`
block, but it wouldn't reduce loc count.
- src/jdk.security.auth/share/classes/com/sun/security/auth/module/GssLoginModule.java
By: Peter Burka
Status: REJECT
Comment:
> + gssName = gssICred.getName();
+ if (gssName == null && gssACred != null)
+ gssName = gssACred.getName();
+ }
+
+ private void attemptAuthentication(boolean getPasswdFromSharedState)
+ throws LoginException {
+
+ // Get a name, maybe
+ if (name == null) {
+ if (useDefaultCreds) {
+ try {
+ getcreds();
+ return;
+ } catch (GSSException e) {
+ throw new LoginException(e.getMessage());
Consider adding `e` as a cause to the `LoginException`
Reason: LoginException is ancient. I'm not going to fix LoginException :/
- src/jdk.security.auth/share/classes/com/sun/security/auth/module/GssLoginModule.java
By: Peter Burka
Status: REJECT
Comment:
> +
+ // Get a name, maybe
+ if (name == null) {
+ if (useDefaultCreds) {
+ try {
+ getcreds();
+ return;
+ } catch (GSSException e) {
+ throw new LoginException(e.getMessage());
+ }
+ }
+ if (tryDefaultCreds) {
+ try {
+ getcreds();
+ return;
+ } catch (GSSException e) { }
Ignoring exceptions is a bad smell. Add a comment explaining why it's ok.
Reason: I think the code is self-explanatory. We're trying one path
and falling back onto the other, and I'm not nesting try
blocks unnecessarily because too much indentation makes code
hard to read.
- src/jdk.security.auth/share/classes/com/sun/security/auth/module/GssLoginModule.java
By: Peter Burka
Status: REJECT
Comment:
> + if (tryDefaultCreds) {
+ try {
+ getcreds();
+ return;
+ } catch (GSSException e) { }
+ }
+
+ promptForName(getPasswdFromSharedState);
+ if (name == null)
+ throw new LoginException ("Unable to determine a GSS name");
+ }
+
+ try {
+ gssName = manager.createName(name, nametypeOid);
+ } catch (GSSException e) {
+ throw new LoginException ("Unable to import GSS name");
Consider reporting `e` as the cause of the `LoginException`
Reason: LoginException is ancient. I'm not going to fix LoginException :/
- src/jdk.security.auth/share/classes/com/sun/security/auth/module/GssLoginModule.java
By: Peter Burka
Status: REJECT
Comment:
> +
+ promptForPass(getPasswdFromSharedState);
+
+ try {
+ getcreds();
+ } catch (GSSException e) {
+ throw new LoginException(e.getMessage());
+ }
+ }
+
+ private void promptForName(boolean getPasswdFromSharedState)
+ throws LoginException {
+ if (getPasswdFromSharedState) {
+ // use the name saved by a module earlier in the stack
+ name = (String)sharedState.get(NAME);
+ if (name == null || name.length() == 0)
use `name.isEmpty()` instead of checking the length.
Reason: it's good enough as it is.
- src/jdk.security.auth/share/classes/com/sun/security/auth/module/GssLoginModule.java
By: Peter Burka
Status: ACCEPT
Comment:
> +
+ if (callbackHandler == null)
+ throw new LoginException("No CallbackHandler "
+ + "available "
+ + "to prompt for authentication "
+ + "information from the user");
+
+ try {
+ String defUsername = System.getProperty("user.name");
+
+ Callback[] callbacks = new Callback[1];
+ MessageFormat form = new MessageFormat(
+ getAuthResourceString(
+ "GSS.name.defName."));
+ Object[] source = {defUsername};
+ callbacks[0] = new NameCallback(form.format(source));
Why not allocate and initialize `callbacks` on one line, like you do for `source` on the previous line?
- src/jdk.security.auth/share/classes/com/sun/security/auth/module/GssLoginModule.java
By: Peter Burka
Status: REJECT
Comment:
> + + "available "
+ + "to prompt for authentication "
+ + "information from the user");
+
+ try {
+ String defUsername = System.getProperty("user.name");
+
+ Callback[] callbacks = new Callback[1];
+ MessageFormat form = new MessageFormat(
+ getAuthResourceString(
+ "GSS.name.defName."));
+ Object[] source = {defUsername};
+ callbacks[0] = new NameCallback(form.format(source));
+ callbackHandler.handle(callbacks);
+ name = ((NameCallback)callbacks[0]).getName();
+ if (name != null && name.length() == 0)
`isEmpty()`
- src/jdk.security.auth/share/classes/com/sun/security/auth/module/GssLoginModule.java
By: Peter Burka
Status: ACCEPT
Comment:
> + throw new LoginException("No CallbackHandler "
+ + "available "
+ + "to prompt for authentication "
+ + "information from the user");
+
+ try {
+ String defUsername = System.getProperty("user.name");
+
+ Callback[] callbacks = new Callback[1];
+ MessageFormat form = new MessageFormat(
+ getAuthResourceString(
+ "GSS.name.defName."));
+ Object[] source = {defUsername};
+ callbacks[0] = new NameCallback(form.format(source));
+ callbackHandler.handle(callbacks);
+ name = ((NameCallback)callbacks[0]).getName();
This feels needlessly roundabout. Just store the callback in an appropriately typed local.
- src/jdk.security.auth/share/classes/com/sun/security/auth/module/GssLoginModule.java
By: Peter Burka
Status: REJECT
Comment:
> +
+ try {
+ String defUsername = System.getProperty("user.name");
+
+ Callback[] callbacks = new Callback[1];
+ MessageFormat form = new MessageFormat(
+ getAuthResourceString(
+ "GSS.name.defName."));
+ Object[] source = {defUsername};
+ callbacks[0] = new NameCallback(form.format(source));
+ callbackHandler.handle(callbacks);
+ name = ((NameCallback)callbacks[0]).getName();
+ if (name != null && name.length() == 0)
+ name = null;
+ if (name == null && defUsername != null &&
+ defUsername.length() != 0)
`isEmpty()`
pburka commented on this pull request.
- src/jdk.security.auth/share/classes/com/sun/security/auth/module/GssLoginModule.java
By: Peter Burka
Status: REJECT
Comment:
> +
+ Callback[] callbacks = new Callback[1];
+ MessageFormat form = new MessageFormat(
+ getAuthResourceString(
+ "GSS.name.defName."));
+ Object[] source = {defUsername};
+ callbacks[0] = new NameCallback(form.format(source));
+ callbackHandler.handle(callbacks);
+ name = ((NameCallback)callbacks[0]).getName();
+ if (name != null && name.length() == 0)
+ name = null;
+ if (name == null && defUsername != null &&
+ defUsername.length() != 0)
+ name = defUsername;
+ } catch (java.io.IOException ioe) {
+ throw new LoginException(ioe.getMessage());
Include `ioe` as cause.
- src/jdk.security.auth/share/classes/com/sun/security/auth/module/GssLoginModule.java
By: Peter Burka
Status: REJECT
Comment:
> + MessageFormat form = new MessageFormat(
+ getAuthResourceString(
+ "GSS.name.defName."));
+ Object[] source = {defUsername};
+ callbacks[0] = new NameCallback(form.format(source));
+ callbackHandler.handle(callbacks);
+ name = ((NameCallback)callbacks[0]).getName();
+ if (name != null && name.length() == 0)
+ name = null;
+ if (name == null && defUsername != null &&
+ defUsername.length() != 0)
+ name = defUsername;
+ } catch (java.io.IOException ioe) {
+ throw new LoginException(ioe.getMessage());
+ } catch (UnsupportedCallbackException uce) {
+ throw new LoginException
Include `uce` as cause.
- src/jdk.security.auth/share/classes/com/sun/security/auth/module/GssLoginModule.java
By: Peter Burka
Status: REJECT
Comment:
> + private void promptForPass(boolean getPasswdFromSharedState)
+ throws LoginException {
+
+ char[] pw;
+
+ if (getPasswdFromSharedState) {
+ // use the password saved by the first module in the stack
+ pw = (char[])sharedState.get(PWD);
+ if (pw == null) {
+ if (debug)
+ System.out.println("\t\t[GssLoginModule] password from" +
+ " shared state is null");
+ throw new LoginException
+ ("Password can not be obtained from sharedstate ");
+ }
+ password = new String(pw);
This uses the default encoding. That's almost certainly not what you want.
Reason: It's what Krb5LoginModule does, and I believe it's correct.
later, in the C JNI code we'll ask for the string's UTF-8
contents, so presumably Java will convert then.
- src/jdk.security.auth/share/classes/com/sun/security/auth/module/GssLoginModule.java
By: Peter Burka
Status: ACCEPT
Comment:
> +
+ if (getPasswdFromSharedState) {
+ // use the password saved by the first module in the stack
+ pw = (char[])sharedState.get(PWD);
+ if (pw == null) {
+ if (debug)
+ System.out.println("\t\t[GssLoginModule] password from" +
+ " shared state is null");
+ throw new LoginException
+ ("Password can not be obtained from sharedstate ");
+ }
+ password = new String(pw);
+ return;
+ }
+ if (doNotPrompt)
+ throw new LoginException("Unable to prompt for password\n");
Remove the `\n`
- src/jdk.security.auth/share/classes/com/sun/security/auth/module/GssLoginModule.java
By: Peter Burka
Status: ACCEPT
Comment:
> + if (doNotPrompt)
+ throw new LoginException("Unable to prompt for password\n");
+
+ if (callbackHandler == null) {
+ throw new LoginException("No CallbackHandler "
+ + "available "
+ + "to garner authentication "
+ + "information from the user");
+ }
+ try {
+ Callback[] callbacks = new Callback[1];
+ MessageFormat form = new MessageFormat(
+ getAuthResourceString(
+ "Kerberos.password.for.username."));
+ Object[] source = {name};
+ callbacks[0] = new PasswordCallback(form.format(source), false);
ditto -- alloc and initialize on one line.
- src/jdk.security.auth/share/classes/com/sun/security/auth/module/GssLoginModule.java
By: Peter Burka
Status: REJECT
Comment:
> + Callback[] callbacks = new Callback[1];
+ MessageFormat form = new MessageFormat(
+ getAuthResourceString(
+ "Kerberos.password.for.username."));
+ Object[] source = {name};
+ callbacks[0] = new PasswordCallback(form.format(source), false);
+ callbackHandler.handle(callbacks);
+ char[] tmpPassword = ((PasswordCallback)
+ callbacks[0]).getPassword();
+ if (tmpPassword == null)
+ throw new LoginException("No password provided");
+ password = new String(tmpPassword);
+ ((PasswordCallback)callbacks[0]).clearPassword();
+
+ // clear tmpPassword
+ for (int i = 0; i < tmpPassword.length; i++)
`Arrays.fill(tmpPassword, ' ')`
Reason: Interesting, but not too compelling. Also, this clearing of
passwords thing (which here I copied from Krb5LoginModule) is
cargo culting. The GC may well have moved the password and
there may well be copies in memory. I'd sooner remove this
code than rewrite it.
- src/jdk.security.auth/share/classes/com/sun/security/auth/module/GssLoginModule.java
By: Peter Burka
Status: REJECT
Comment:
> + "Kerberos.password.for.username."));
+ Object[] source = {name};
+ callbacks[0] = new PasswordCallback(form.format(source), false);
+ callbackHandler.handle(callbacks);
+ char[] tmpPassword = ((PasswordCallback)
+ callbacks[0]).getPassword();
+ if (tmpPassword == null)
+ throw new LoginException("No password provided");
+ password = new String(tmpPassword);
+ ((PasswordCallback)callbacks[0]).clearPassword();
+
+ // clear tmpPassword
+ for (int i = 0; i < tmpPassword.length; i++)
+ tmpPassword[i] = ' ';
+ } catch (java.io.IOException ioe) {
+ throw new LoginException(ioe.getMessage());
Report the cause.
- Regaring various createCredential() methods and credential class
constructors that now have a password argument
By: Michael Osipov
Status: REJECT
Comment: Shouldn't the password be a char array?
To securely wipe the password from memory against immutable
strings in Java. In C you can safely to memset with zero,
you Java you can't.
Reason: Not really. There's no win. The GC can move arrays, leaving
behind copies of the password. There's no easy way to
prevent this.
- src/java.security.jgss/share/classes/sun/security/jgss/LoginConfigImpl.java
By: Michael Osipov
Status: REJECT
Comment:
> }
return new AppConfigurationEntry[] {
new AppConfigurationEntry(
- "com.sun.security.auth.module.Krb5LoginModule",
+ "com.sun.security.auth.module.GssLoginModule",
This is a change in behavior, why?
This is backwards-compatible. We now specify GssLoginModule as
required in the default JAAS config, but it returns ignore if it's
not applicable, and then we have Krb5LoginModule as sufficient, which
means any failure from it will be ignored if GssLoginModule succeeds.
More information about the security-dev
mailing list