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

Thu Aug 29 02:37:03 UTC 2013

Thanks for the clarification on MIT's result on the test cases.

So, for majority of the test cases inside the regression test, we return 
different results from what MIT returns? That's a little unsettling. How 
about for the more complicated valid capaths definition? We for sure 
will return the same results as MIT's, right?

It seems strange/confusing to have all these invalid capaths definition 
as regression test.
I can see the value of the infinite-loop case to ensure that no OOME 
occurs. But why should we care about the rest?
I hope that we won't be bound by these "non-interoperable" (or 
vendor-specific) interpretation in the future.

As for your "learning from MIT" comment, if I understand it correctly, 
it means that we will no longer treating the missing definition as "=." 
but rather go hierarchy, right?
Valerie

On 08/22/13 21:55, Weijun Wang wrote:
>
> 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
Hmm, interesting.

> ---------- 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
>>