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

Thu Aug 29 05:52:43 UTC 2013

On 8/29/13 10:37 AM, Valerie (Yu-Ching) Peng wrote:
>
> 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.

Well that's about the chaining Java does. Since that's the harder part 
to implement, I add more tests for it.

> How about for the more complicated valid capaths definition?

Yes I can. However, since the MIT style just reads one value, I doubt it 
can be more complicated. It can be longer of course.

> We for sure will return the same results as MIT's, right?

I think so. At least for the first 4 cases in the test, which is from 
MIT's own krb5.conf example, the results are the same.

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

Yes, I believe there is no right answer to an invalid capaths. But since 
we had to deal with them anyway, I'd like to keep these test cases to 
make sure the implementation is consistent.

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

Correct.

Thanks
Max

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