Code review request: 8012615: Realm.getRealmsList returns realms list in wrong

[Weijun Wang]

On 8/23/13 10:39 AM, Valerie (Yu-Ching) Peng wrote:
>
> <Config.java>
> 1. Line 255, "returns if keys exists" should be "returns true if key
> exists".
> 2. Line 257, "@see get" should be "@see get0"?

I meant looking at the how IAE is thrown in get. Updated to

      * @throws IllegalArgumentException if any of the keys is illegal
      *         (See {@link #get})

> 3. You may want to add the following to the public getAll(String...
> keys) method.
>
> @throws IllegalArgumentException ...

Yes.

>
> <CredentialsUtil.java> looks fine
>
> Before I looked at Realm.java, I looked at the test first to understand
> the expected realm list result.
>
> Well, judging from the test, I feel the parsing of these CA path
> settings are too relaxed.
> You are allowing all sorts of short cuts and user errors. When has
> capaths been implemented?
>
> I wonder what happens when MIT implementation run into these invalid
> capath settings.
> Is their implementation also interpret them like what you have here?

Below are all differences using the krb5.conf in test

--------- 1 ----------
TIVOLI.COM = {
     IBM.COM = IBM_LDAPCENTRAL.COM MOONLITE.ORG
     IBM_LDAPCENTRAL.COM = LDAPCENTRAL.NET
     LDAPCENTRAL.NET = .
}

TIVOLI.COM -> IBM.COM
     MIT: [TIVOLI.COM, IBM_LDAPCENTRAL.COM]
     java: [TIVOLI.COM, LDAPCENTRAL.NET, IBM_LDAPCENTRAL.COM, MOONLITE.ORG]

Here, the value for IBM.COM is two values on the same line, this is no 
legal in MIT. It simply read the first one. Java reads both and combines 
it with another value
---------- 2 -----------
B1.COM = {
     B2.COM = .
     B3.COM = B2.COM
     B4.COM = B3.COM
}

B1.COM -> B4.COM
     MIT : [B1.COM, B3.COM]
     java: [B1.COM, B2.COM, B3.COM]

Java combines 2 values
----------- 3 -----------
C1.COM = {
     C3.COM = C2.COM
}

C1.COM -> C2.COM
     MIT : [C1.COM, COM]
     java: [C1.COM]

When MIT cannot find the sRealm as key, it goes hierarchy. Java only 
does this when there is no cRealm sub-section
------------ 4 -----------
D1.COM = {
     D2.COM=D1.COM
}

D1.COM -> D2.COM
     MIT : [D1.COM, D1.COM]
     java: [D1.COM]

MIT returns dup realms
----------- 5 -------------
E1.COM = {
     E2.COM = E2.COM
     E3.COM = E4.COM
     E3.COM = .
}

E1.COM -> E2.COM
     MIT : [E1.COM, E2.COM]
     java: [E1.COM]

MIT returns dup realms (please remember path does not include last realm)

E1.COM -> E3.COM
     MIT : [E1.COM, E4.COM, .]
     java: [E1.COM, E4.COM]

"." should only appear as single value for one key. MIT does not notice 
it and blindly returns
----------- 6 ----------------
A9.PRAGUE.XXX.CZ = {
     PRAGUE.XXX.CZ = .
     ROOT.XXX.CZ = PRAGUE.XXX.CZ
     SERVIS.XXX.CZ = ROOT.XXX.CZ
}

A9.PRAGUE.XXX.CZ -> SERVIS.XXX.CZ
     MIT : [A9.PRAGUE.XXX.CZ, ROOT.XXX.CZ]
     java: [A9.PRAGUE.XXX.CZ, PRAGUE.XXX.CZ, ROOT.XXX.CZ]

Java combines 2 values
----------------------------

For 1, 2, 6, Java combines and MIT reads single key. Since we decided to 
do this, it's OK.

4, 5, wrong config, Java manages to return a path as leasts look normal, 
MIT returns ugly path.

3, This is probably something we can learn from MIT. If you agree, I'll 
update webrev.

>
> I think your new comment on line 71 is more confusing.
> Can you just say "D2=D1 is the same as D2=."?

Yes. D1 should not appear as a target when it is the client realm. So 
"D2=D1" is just ignored, which is the same as "D2=.".

As said above, if we decide to choose MIT's way to require a sRealm key 
to use capaths, "D2=D1" will still be regarded the same as "D2=.", but 
it will be different from no D2 line at all.

>
> What happens for the infinite loop case, i.e. G?
> What should (G1, G2) returns? G1 G3?

Yes. I first find G3, and look at G3=G2. Any realm that is either 
client, or target, or has appeared in the path is ignored.

In this case, the config itself has a problem so there is no correct 
answer. Just want to make sure the search does not leads to a real 
infinite loop and then OOME.

Thanks
Max

> Thanks,
> Valerie
>
> On 07/29/13 02:11, Weijun Wang wrote:
>> Hi Valerie
>>
>> Please review the capaths code change at
>>
>>   http://cr.openjdk.java.net/~weijun/8012615/webrev.01/
>>
>> It includes:
>>
>> Config.java
>> ===========
>>
>> Add method to check if a sub-stanza exists.
>>
>> Realm.java
>> ==========
>>
>> Rewrite reading cross-realm path for both [capaths] and hierarchy. The
>> [capaths] part implements the chaining process.
>>
>> CredentialsUtils.java
>> =====================
>>
>> When reading cross-realm TGT for a path A->B->C->D->SERVERREALM, the
>> current impl first gets krbtgt/SERVERREALM at A, and then fallback to
>> krbtgt/D at A, krbtgt/C at A and krbtgt/B at A. In fact, since the capath is
>> already there, krbtgt/B at A should be the first to check. I don't know
>> about the history of this code and dare not change much. But I at
>> least reverse the order of the fallback (what the code calls inner
>> loop) to try krbtgt/B at A first.
>>
>> Tried on a local setup of 5 MIT KDC realms configured with a
>> one-direction cross-auth from K1 to K5. The MIT kvno command starts
>> with kinit in K1 and goes thru krbtgt/K2 at K1, krbtgt/K3 at K2,
>> krbtgt/K4 at K3, krbtgt/K5 at K4 and finally get service ticket to
>> host/host.k5 at K5. Now Java can do the same with the same krb5.conf.
>>
>> Thanks
>> Max
>