-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Code review of ts jgss jdk12 #1
base: jdk12-master
Are you sure you want to change the base?
Changes from all commits
0b47709
e7b31c2
003d82b
0191c79
f8d26b3
2a9e744
e2ad2c5
63dcbe5
a742c8e
d975a3d
561e7f4
5840bfd
ac1f706
837a750
173c04b
b7cd095
89341f8
98f952f
78a7cac
5a494b0
ac27524
90d3097
5a541ee
192cc47
16f33b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -214,10 +214,48 @@ public boolean implies(Permission p) { | |
|
||
|
||
boolean impliesIgnoreMask(ServicePermission p) { | ||
return ((this.getName().equals("*")) || | ||
this.getName().equals(p.getName()) || | ||
(p.getName().startsWith("@") && | ||
this.getName().endsWith(p.getName()))); | ||
if ((this.getName().equals("*")) || | ||
this.getName().equals(p.getName()) || | ||
(p.getName().startsWith("@") && | ||
this.getName().endsWith(p.getName()))) | ||
return true; | ||
|
||
/* | ||
* Empty realm in this or p is a wild-card. This is needed to support | ||
* non-Kerberos ServicePermissions for GSS (a band-aid until we can | ||
* implement a proper GssAcceptorPermission class), but also because | ||
* users may not know and might not care what realm the service is in, | ||
* 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(); | ||
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This allocation is unnecessary. Use String#regionMatches instead. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. |
||
return (p.getName().startsWith(s) && | ||
(p.getName().equals(s) || this.getName().equals(s))); | ||
} | ||
} | ||
|
||
// No realm, not even empty -> fail | ||
return false; | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -423,6 +423,58 @@ public abstract GSSCredential createCredential (GSSName name, | |
int lifetime, Oid mech, int usage) | ||
throws GSSException; | ||
|
||
/** | ||
* Factory method for acquiring a single mechanism credential with a | ||
* password.<p> | ||
* | ||
* GSS-API mechanism providers must impose a local access-control | ||
* policy on callers to prevent unauthorized callers from acquiring | ||
* credentials to which they are not entitled. The kinds of permissions | ||
* needed by different mechanism providers will be documented on a | ||
* per-mechanism basis. A failed permission check might cause a {@link | ||
* java.lang.SecurityException SecurityException} to be thrown from | ||
* this method. <p> | ||
* | ||
* Non-default values for lifetime cannot always be honored by the | ||
* underlying mechanisms, thus applications should be prepared to call | ||
* {@link GSSCredential#getRemainingLifetime() getRemainingLifetime} | ||
* on the returned credential.<p> | ||
* | ||
* @param name the name of the principal for whom this credential is to be | ||
* acquired. Use <code>null</code> to specify the default principal. | ||
* @param lifetime The number of seconds that credentials should remain | ||
* valid. Use {@link GSSCredential#INDEFINITE_LIFETIME | ||
* GSSCredential.INDEFINITE_LIFETIME} to request that the credentials | ||
* have the maximum permitted lifetime. Use {@link | ||
* GSSCredential#DEFAULT_LIFETIME GSSCredential.DEFAULT_LIFETIME} to | ||
* request default credential lifetime. | ||
* @param mech the Oid of the desired mechanism. Use <code>(Oid) null | ||
* </code> to request the default mechanism. | ||
* @param usage The intended usage for this credential object. The value | ||
* of this parameter must be one of: | ||
* {@link GSSCredential#INITIATE_AND_ACCEPT | ||
* GSSCredential.INITIATE_AND_ACCEPT}, | ||
* {@link GSSCredential#ACCEPT_ONLY GSSCredential.ACCEPT_ONLY}, and | ||
* {@link GSSCredential#INITIATE_ONLY GSSCredential.INITIATE_ONLY}. | ||
* @return a GSSCredential of the requested type. | ||
* | ||
* @see GSSCredential | ||
* | ||
* @throws GSSException containing the following | ||
* major error codes: | ||
* {@link GSSException#BAD_MECH GSSException.BAD_MECH}, | ||
* {@link GSSException#BAD_NAMETYPE GSSException.BAD_NAMETYPE}, | ||
* {@link GSSException#BAD_NAME GSSException.BAD_NAME}, | ||
* {@link GSSException#CREDENTIALS_EXPIRED | ||
* GSSException.CREDENTIALS_EXPIRED}, | ||
* {@link GSSException#NO_CRED GSSException.NO_CRED}, | ||
* {@link GSSException#FAILURE GSSException.FAILURE} | ||
*/ | ||
public abstract GSSCredential createCredential (GSSName name, | ||
String password, int lifetime, Oid mech, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't the password be a char array? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why should it be a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh, ok. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that reliable? @pburka suggests that a copying GC can easily leave copies of a password lying about. And in the JAAS case the password would come from a file that is read in and parsed, so there will be a number of copies. I'm not keen on reading the whole file as a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What file are you talking about? With JAAS you have callback handlers and they use char arrays too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, maybe I was confused. Krb5LoginModule let's you specify a raw secret key -- that's a bit crazy, IMO. Still, other login modules use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How if I look at this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I see, that's ugly. You'll see a lot of such things in the JDK. Old Code (TM). |
||
int usage) | ||
throws GSSException; | ||
|
||
/** | ||
* Factory method for acquiring credentials over a set of | ||
* mechanisms. This method attempts to acquire credentials for | ||
|
@@ -480,6 +532,64 @@ public abstract GSSCredential createCredential(GSSName name, | |
int lifetime, Oid mechs[], int usage) | ||
throws GSSException; | ||
|
||
/** | ||
* Factory method for acquiring credentials with a password over a set of | ||
* mechanisms. This method attempts to acquire credentials for | ||
* each of the mechanisms specified in the array called mechs. To | ||
* determine the list of mechanisms for which the acquisition of | ||
* credentials succeeded, the caller should use the {@link | ||
* GSSCredential#getMechs() GSSCredential.getMechs} method.<p> | ||
* | ||
* GSS-API mechanism providers must impose a local access-control | ||
* policy on callers to prevent unauthorized callers from acquiring | ||
* credentials to which they are not entitled. The kinds of permissions | ||
* needed by different mechanism providers will be documented on a | ||
* per-mechanism basis. A failed permission check might cause a {@link | ||
* java.lang.SecurityException SecurityException} to be thrown from | ||
* this method.<p> | ||
* | ||
* Non-default values for lifetime cannot always be honored by the | ||
* underlying mechanisms, thus applications should be prepared to call | ||
* {@link GSSCredential#getRemainingLifetime() getRemainingLifetime} | ||
* on the returned credential.<p> | ||
* | ||
* @param name the name of the principal for whom this credential is to | ||
* be acquired. Use <code>null</code> to specify the default | ||
* principal. | ||
* @param lifetime The number of seconds that credentials should remain | ||
* valid. Use {@link GSSCredential#INDEFINITE_LIFETIME | ||
* GSSCredential.INDEFINITE_LIFETIME} to request that the credentials | ||
* have the maximum permitted lifetime. Use {@link | ||
* GSSCredential#DEFAULT_LIFETIME GSSCredential.DEFAULT_LIFETIME} to | ||
* request default credential lifetime. | ||
* @param mechs an array of Oid's indicating the mechanisms over which | ||
* the credential is to be acquired. Use <code>(Oid[]) null</code> for | ||
* requesting a system specific default set of mechanisms. | ||
* @param usage The intended usage for this credential object. The value | ||
* of this parameter must be one of: | ||
* {@link GSSCredential#INITIATE_AND_ACCEPT | ||
* GSSCredential.INITIATE_AND_ACCEPT}, | ||
* {@link GSSCredential#ACCEPT_ONLY GSSCredential.ACCEPT_ONLY}, and | ||
* {@link GSSCredential#INITIATE_ONLY GSSCredential.INITIATE_ONLY}. | ||
* @return a GSSCredential of the requested type. | ||
* | ||
* @see GSSCredential | ||
* | ||
* @throws GSSException containing the following | ||
* major error codes: | ||
* {@link GSSException#BAD_MECH GSSException.BAD_MECH}, | ||
* {@link GSSException#BAD_NAMETYPE GSSException.BAD_NAMETYPE}, | ||
* {@link GSSException#BAD_NAME GSSException.BAD_NAME}, | ||
* {@link GSSException#CREDENTIALS_EXPIRED | ||
* GSSException.CREDENTIALS_EXPIRED}, | ||
* {@link GSSException#NO_CRED GSSException.NO_CRED}, | ||
* {@link GSSException#FAILURE GSSException.FAILURE} | ||
*/ | ||
public abstract GSSCredential createCredential(GSSName name, | ||
String password, int lifetime, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. |
||
Oid mechs[], int usage) | ||
throws GSSException; | ||
|
||
/** | ||
* Factory method for creating a context on the initiator's | ||
* side. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -135,12 +135,26 @@ public GSSCredential createCredential(GSSName aName, | |
return wrap(new GSSCredentialImpl(this, aName, lifetime, mech, usage)); | ||
} | ||
|
||
public GSSCredential createCredential(GSSName aName, String password, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. |
||
int lifetime, Oid mech, int usage) | ||
throws GSSException { | ||
return new GSSCredentialImpl(this, aName, password, | ||
lifetime, mech, usage); | ||
} | ||
|
||
public GSSCredential createCredential(GSSName aName, | ||
int lifetime, Oid[] mechs, int usage) | ||
throws GSSException { | ||
return wrap(new GSSCredentialImpl(this, aName, lifetime, mechs, usage)); | ||
} | ||
|
||
public GSSCredential createCredential(GSSName aName, String password, | ||
int lifetime, Oid mechs[], int usage) | ||
throws GSSException { | ||
return new GSSCredentialImpl(this, aName, password, | ||
lifetime, mechs, usage); | ||
} | ||
|
||
public GSSContext createContext(GSSName peer, Oid mech, | ||
GSSCredential myCred, int lifetime) | ||
throws GSSException { | ||
|
@@ -175,6 +189,17 @@ public GSSCredentialSpi getCredentialElement(GSSNameSpi name, int initLifetime, | |
acceptLifetime, usage); | ||
} | ||
|
||
public GSSCredentialSpi getCredentialElement(GSSNameSpi name, | ||
String password, | ||
int initLifetime, | ||
int acceptLifetime, | ||
Oid mech, int usage) | ||
throws GSSException { | ||
MechanismFactory factory = list.getMechFactory(mech); | ||
return factory.getCredentialElement(name, password, initLifetime, | ||
acceptLifetime, usage); | ||
} | ||
|
||
// Used by java SPNEGO impl | ||
public GSSNameSpi getNameElement(String name, Oid nameType, Oid mech) | ||
throws GSSException { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converting this to a char[] is an unnecessary copy. Just operate on the String directly.