RFR 8215032: Support Kerberos cross-realm referrals (RFC 6806)

[Weijun Wang]

> On May 25, 2019, at 4:43 AM, Martin Balao <mbalao at redhat.com> wrote:
> 
> Hi Max,
> 
> Thanks for your review.
> 
> 1)
> src/java.security.jgss/share/classes/sun/security/krb5/KrbAsReqBuilder.java:
> 
> * When NT-ENTERPRISE names are used, a "@" char can be part of the name
> and we should not interpret it as a realm separator. If we don't escape,
> we may be missing part of the name when building a new PrincipalName.
> The exact place where this happens is in PrincipalName.parseName function.

Is the character already escaped in the oldName? How about just changing

  new PrincipalName(oldName.getNameString().replace..., newRealm)

to

  new PrincipalName(oldName.getNameStrings(), newRealm)


> 
> 2)
> src/java.security.jgss/share/classes/sun/security/krb5/internal/EncKDCRepPart.java:
> 
> * My understanding is that there won't be confusion between caddr and
> padata because the tag for each one is different (0x0B and 0x0C). When
> the tag does not match, "parse" functions return null and execution
> continues. See for example HostAddress.parse or PAData.parseSequence
> methods. All the previous "optionals" work the same way.

Oops, my wrong. I thought getData() would consume some bytes but actually it just "peekByte".

> 
> 3)
> src/java.security.jgss/share/classes/sun/security/krb5/internal/EncKDCRepPart.java:
> 
> * This was a trivial renaming because I found more clear to have "out"
> for the final output and "temp / bytes" for intermediate outputs. I can
> revert this change and use a different name scheme that does not modify
> the original "bytes" variable if you wish.

Not really. Keep your change.

> 
> 4)
> src/java.security.jgss/share/classes/sun/security/krb5/internal/KDCRep.java:
> 
> * msgType being public is no longer needed (it was needed in a previous
> changeset). I'll fix it for Webrev.03.
> 
> 5) KrbTgsReqBuilder
> 
> * Yes, probably.
> 
> 6)
> src/java.security.jgss/share/classes/sun/security/krb5/internal/CredentialsUtil.java
> 
> * Perhaps "serviceCredsSingle" is not the best name. What I meant with
> single was in respect to cross-realm referrals (which are not handled in
> that method). getTGTforRealm won't be called when we get a cross-realm
> referral because there will be a match between the sname realm and the
> realm we will ask for a ticket (both elements are part of the
> cross-realm TGT).
> 
> * In regards to the check being possibly false, you are probably
> correct. Let me do some checking and I'll fix it for Webrev.03.

I was just thinking serviceCredsSingle() is called everywhere and for each one I would need to find out what the result of those "if" checks (if same realm, if krbtgt) are. It would be clearer to me that for some calls the request is really single. Maybe you can add some comments.

> 
> * In regards to cross-realm referrals for S4U2self and S4U2proxy, this
> will be my next step when proposing a patch for resource-based
> constrained delegation.

Great.

It is also called by getTGTforRealm(). Is this for RFC 6806 "9.  Cross-Realm Routing"?

Thanks,
Max

> 
> * "PrincipalName cSname = (PrincipalName) sname.clone();" Right, I'll
> fix this for Webrev.03.
> 
> Kind regards,
> Martin.-
>