Tiny comments: - java.security typos:
492,497: ovewritten 496: infite - CredentialsUtils.java: 36: unused import - KDCRep.java: no need to move the position - ReferralsCache.java: Red Hat has different copyright lines. For example, in java.base. Maybe any one is OK. IANAL. Same with ReferralsTest.java. share/classes/sun/security/ssl/ExtendedMasterSecretExtension.java:2: * Copyright (c) 2017, Red Hat, Inc. and/or its affiliates. share/classes/jdk/internal/misc/UnsafeConstants.java:2: * Copyright (c) 2019, Red Hat Inc. All rights reserved. share/classes/com/sun/crypto/provider/GHASH.java:3: * Copyright (c) 2015 Red Hat, Inc. BTW, is it worth adding some comments here? - TicketFlags.java: 55: enc-pa-rep (15) Remove the whitespace before "(15)" to be consistent with others. Everything else looks fine. You are free to push the change now. Thanks, Max > On May 29, 2019, at 2:20 AM, Martin Balao <mba...@redhat.com> wrote: > > Hi Max, > > Thanks for your feedback. > > Here it's Webrev.03: > > * http://cr.openjdk.java.net/~mbalao/webrevs/8215032/8215032.webrev.03/ > > Changes: > > * msgType is now private > > * Check in CredentialsUtil.java removed (always true) > > * "PrincipalName cSname = (PrincipalName) sname.clone();" not necessary > > * In CredentialsUtils.java, I added comments to methods "serviceCreds", > "serviceCredsReferrals" and "serviceCredsSingle". I'll keep the "single" > suffix but if there is a better one, we can change it. Hopefully this is > clearer with the comments now and removing the unnecessary "if". > > * "new PrincipalName(oldName.getNameString().replace..., newRealm)" > replaced > > Regression testing in "jdk/sun/security/krb5" passed. > > On 5/24/19 9:23 PM, Weijun Wang wrote: >>> 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) >> >> > > Yes, this should be fine. Addressed in Webrev.03. > >> >> It is also called by getTGTforRealm(). Is this for RFC 6806 "9. Cross-Realm >> Routing"? >> > > No, getTGTforRealm is part of the cross-realm operation described by RFC > 4120. If we need to get a TGS for a service which is in a different > realm than the TGT we have, we first ask for a TGT on the service's > realm (using the original TGT) and then we ask for the TGS. The service > realm is determined by previous configurations and heuristics. > > My understanding is that getTGTforRealm is never needed when following > RFC 6806 cross-realm referrals because the referral TGT we use for the > next query is always for the next realm we are going to ask (the > "service's realm"). That's how I see both things working together. A > comment was added to "serviceCredsSingle" method explaining this. > > Kind regards, > Martin.-
