On 06/14/2012 05:26 PM, Xuelei Fan wrote:
It's really confusing to mix the name of a principal and realm of a
principal in the same PrincipalName class.
For example, when printing a debug log for "sname", from the name of
"sname", I think it a name of the principal, but the output also include
the realm of the principal.
When I read the spec, I think a message should contains both Realm and
PrincipalName. But when read the code, only PrincipalName class appears
in a call of a method. Feels very strange. I have to read the
implementation carefully in order to understand the debug log and logic
of the code.
I understand your feelings. But in JDK, PrincipalName contains the realm
info from the very beginning. Removing it from PrincipalName makes me
confusing. Yes, maybe it's a bad name.
Shall we wait for Valerie's opinion?
Can we make PrincipalName a pure name of a principal, without the realm
field in it?
We can, but for what? Anyway, I don't think we need to maintain both
PrincipalNameWithoutRealm and PrincipalNameWithRealm. The 1st one is
completely useless. The 2nd one is the existing PrincipalName. If you
don't like it, how about simply call it Principal?
How hard to make the changes?
Not hard at all. If you agree with me on my last paragraph, it's just a
simple "s/PrincipalName/Principal/g" on all src/test codes.
I noticed most of the code
have the name and realm parameters at the same time, just as the krb5 v5
specification.
Yes, that's why I say PrincipalNameWithoutRealm is useless.
I can accept your current design if you won't like to make more changes.
I may be able to complete the review tomorrow.
Please go on. I think we agree on the basic idea that name and realm
should be bound in a single class. The only disagreement is on the name
of the class. Am I right?
Thanks
Max
Thanks,
Xuelei
On 6/13/2012 9:12 AM, Weijun Wang wrote:
On 06/12/2012 10:23 PM, Xuelei Fan wrote:
I have not reviewed too much about the fix. But before looking more
updates, I was wondering whether it is good to have PrincipalName to be
paired with Realm in the PrincipalName class.
In RFC4120, realm names and principal names are two different name
constraints for different purpose. The PrincipalName is defined as:
PrincipalName ::= SEQUENCE {
name-type [0] Int32,
name-string [1] SEQUENCE OF KerberosString
}
There is no realm field in the structure. When I read Java code, I think
the PrincipalName class is a one-to-one map to above PrincipalName
specification. Otherwise, the class name may be confusing to me.
Yes, it's true that the PrincipalName defined in RFC has no realm, and
therefore not equivalent to the PrincipalName class in Java, but I think
there are several reasons we keep the current design:
1. In Kerberos, PrincipalName and Realm always appear together. In this
sense, they should be modeled as a single data entity.
2. In Java, we had this model from the very beginning, i.e.
PrincipalName includes the realm info inside. (We should document this).
The current problem is that whether/when the realm field is filled is
not consistent.
I was wondering, can we just make the PrincipalName independent from
Realm, and create a new class, such as XXXPrincipal to pair the
PrincipalName and Realm?
XXXPrincipal ::= SEQUENCE {
principal PrincipalName,
realm Realm
}
Suppose you agree with me on the model design, this is just a name
change, and after this name change, the plain PrincipalName class will
be useless. I doubt this makes any real enhancement, but it really
touches all the source files.
Thanks
Max
Xuelei
On 6/11/2012 1:29 PM, Weijun Wang wrote:
Hi Valerie
Here is the webrev:
http://cr.openjdk.java.net/~weijun/6966259/webrev.00/
The patch is quite long, but most of the real changes are in a few
classes:
PrincipalName.java:
. All fields are final and non-null non-empty now
. All constructors have a realm argument, including those from DER
. Add a new static method tgsService(r1, r2) to get AS/TGS name
. Remove ServiceName class
. New reg test: Constructors.java
Realm.java:
. Field is now final
. New getDefault() method
KDCReqBody.java:
. cname and sname share the same realm field. This is also the only
message format that realm comes after name.
KrbKdcRep.java:
. Related to the class above. The check() method now has a new argument
isAsReq to deal with AS-REQ and TGS-REQ differently. The old code does
not check crealm equality, now both name and realm are checked, but only
for AS-REQ. For TGS-REQ, no more check for cname equality, this will be
useful for S4U2proxy where the cname of KDCRep comes from the additional
ticket in request. The old code checks if req.crealm is rep.srealm,
since KDCReqBody has only one realm field, this is the same as checking
req.srealm and rep.srealm.
CCacheInputStream.java:
. readPrincipal returns null when there is no realm field. This has the
benefit that a ccache is readable even if there is no valid krb5
setting. A normal ccache entry's principal should have its own realm
field. but when an entry is used to store non-ticket, returning null
won't trigger an exception. (See readCred about "X-CACHECONF:" style
entries)
Other trivial code changes:
. Methods with the realm/name argument pair now has only name
. In parsing DER, read realm and then merge the info into name
. In encoding DER, encode the realm from name.getRealm()
. No need to check realm == null for name
. No need to print realm in debug output
. No need to call setRealm()
Thanks
Max
On 06/09/2012 08:23 AM, Valerie (Yu-Ching) Peng wrote:
Max,
Yes, I think the current model that you described sounds error prone. I
don't know the history of the current design.
But I do also prefer the described changes that you have.
I'd expect the refactoring would make the code clearer and more robust.
Valerie
On 06/06/12 17:55, Weijun Wang wrote:
Hi Valerie
The krb5 PrincipalName class has a realm field and the class says
If null, means the default realm
Ideally this means if the realm of a name is null then this field can
be null. Otherwise, it must be filled when created.
In fact, inside our codes, the field is often filled (using
setRealm()) after it's created. This leads to several strange coding
styles that make the codes confusing and error-prone.
1. a lot of setRealm() calls that's far from the creation of the
principal name only when the field needs to be used
2. a lot of if (realm == null) checks
3. a lot of "unresolved" names that never has a realm but is
definitely not in the default realm (just because the realm field is
not used inside JDK)
I am planning to fix this to make the PrincipalName immutable and
always with a non-null non-empty realm. I also plan to make Realm
immutable and remove the ServiceName class (it's quite useless).
A brief look into the code and protocol suggests this is quite
feasible. In every krb5 message and serialized data (I mean ccache and
keytab) defined, there is always a realm beside name. This is also
true for most Java methods. And I don't think a name with an
"unresolved" realm should exists at all. If we have to deal with
something like this, I'd rather invent a new class for it.
The code change will be mostly refactoring, removing a lot of realm
arguments/fields and merging it into name. One behavior change is that
there will be no name with "unresolved" realm anymore, but I think
this should never be true in a real production environment. In fact,
the public API KerberosPrincipal has
* @throws IllegalArgumentException if name is improperly
* formatted, if name is null, or if name does not contain
* the realm to use and the default realm is not specified
* in either a Kerberos configuration file or via the
* java.security.krb5.realm system property.
What's your suggestion? I've been haunted by this several times,
mostly because a setRealm() is not called.
Thanks
Max
