On Fri, 6 Aug 2021 19:27:30 GMT, Martin Balao <mba...@openjdk.org> wrote:
> I'd like to propose a fix for JDK-8270137 [1]. > > This bug is triggered when using a previously stored referral ticket (in the > Referrals Cache) at the moment of following a S4U2Proxy cross-realm referral. > The mistakenly-used referral ticket matched the client and service names but > it was obtained as a result of a non-S4U2Proxy request. In fact, it was the > middle service that got it while trying to determine the backend service > realm in a previous S4U2Proxy communication. The mistakenly-used referral > ticket was not bind to the impersonated user (in other words, it was not > obtained attaching the user's TGS as part of a S4U2Proxy request) and, thus, > must not be used. > > Even when one possible approach to fix this issue could be to be more > selective at the moment of getting referral tickets from the Cache (that is: > do not get anything from the Cache if it's for a S4U2Proxy request), I > decided to go one step further and enhance the Referrals Cache. With this > enhancement, we add more information to the stored referral tickets such as a > footprint of the TGS (in the case of S4U2Proxy requests) or the user > principal (in the case of S4U2Self requests). We now allow to store S4U2Proxy > and S4U2Self referrals tickets but those will be re-used only if there is a > perfect match of the TGS or user principal. As an example, if a middle > service tries to replicate the exact S4U2Self communication for exactly the > same user, cached referral tickets should be okay. With this enhancement, we > increase the use of the Cache and the performance (time, network resources, > etc.). > > The ReferralsTest is enhanced to reflect these new scenarios and now uses > cached S4U2Proxy/S4U2Self referral tickets. > > No regressions observed in jdk/sun/security/krb5. > > -- > [1] - https://bugs.openjdk.java.net/browse/JDK-8270137 Looks fine. Some small comments. src/java.security.jgss/share/classes/sun/security/krb5/internal/CredentialsUtil.java line 90: > 88: Credentials creds = serviceCreds( > 89: KDCOptions.with(KDCOptions.FORWARDABLE), > 90: ccreds, ccreds.getClient(), sname, client, How about we rename `client` to `user` here? src/java.security.jgss/share/classes/sun/security/krb5/internal/CredentialsUtil.java line 496: > 494: */ > 495: private static void handleS4U2SelfReferral(PAData[] pas, > 496: PrincipalName user, Credentials oldCeds, Credentials > newCreds) `oldCreds` is useless now. src/java.security.jgss/share/classes/sun/security/krb5/internal/ReferralsCache.java line 59: > 57: private byte[] clientSvcTicketEnc; // S4U2Proxy only > 58: ReferralCacheKey (PrincipalName cname, PrincipalName sname, > 59: PrincipalName user, Ticket clientSvcTicket) { It's probably not necessary, but I somehow feel it will be clearer to add S4U2Type into the key. In fact, with all these info it almost look like the key contains everything in a TGS-REQ (except for the timestamp maybe). ------------- Marked as reviewed by weijun (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5036
