On Thu, 28 Oct 2021 19:21:02 GMT, Martin Balao <mba...@openjdk.org> wrote:
> * The names 'second' and 'secondTicket' -that were used before- don't look > ideal to me. I've not seen them used neither in RFC 4120 nor in MS-SFU > (v.20.0). In the case of 'additionalTickets', it's defined in RFC 4120 but > more from a message-format perspective than with any specific semantics. Many > of us are familiar to these names at this point but perhaps we can take this > opportunity to find a better replacement. These are service tickets used for > impersonation, from a user principal probably. I used 'userSvcTicket' when > working on the Referrals feature. It could be that or something different. > I'm okay if you want to postpone this consideration, but I wanted to raise it > just in case. I see what you mean. I'll go through them. > * While I see the need of introducing a new class to the hierarchy > (KrbKdcReq), I'd rearrange that a bit if possible. In particular, I'd make > ::getMessage part of the interface, instead of ::encoding, and then delegate > to the message (KDCReq --> ASReq | TGSReq) the encoding. In fact, the message > already does the encoding; it's just caching it in the same place where it's > done -which should be fine as the message is non-mutable-. This would > simplify things a bit and we can have only one 'obuf' field in KDCReq, > instead of caching it both in KrbAsReq and KrbTgsReq. We are not loosing > encapsulation because the message is already accessible. What do you think? Good idea. > * In CredentialsUtil.java, I see how the checks 'additionalTickets == null || > additionalTickets.length == 0' were replaced by 'second == null', but what > about the check 'credsInOut[1] == null'? Sorry, missed it. > * I want to notice that 'additionalTickets' was technically an 'in/out' > parameter in CredentialsUtil::serviceCredsReferrals. I couldn't find any > consequence of making it an 'in' parameter, but just in case I want to > mention this. I believe the only function that could have used that > information is CredentialsUtil::acquireS4U2proxyCreds but it's an r-value > there so it should be fine. That's right. Although the content of the original array argument could be modified, the caller has not used it. In fact, changing it from array to a normal object makes me feel relieved. I always needed to remind myself that this was not meant to be an '[out]' parameter. > * A KDC_ERR_BADOPTION error meaning that the KDC requires the 'FORWARDABLE' > option in the ticket is what the code suggests. Is it possible that this is > not what really happened and there is something else wrong? In other words, > is it possible that a DS_BEHAVIOR_WIN2012 DC returns this error? I don't know. > * The FORWARDABLE check removed is the one in S4U2Self. Apparently, for > S4U2Proxy with non-S4U2Self second-tickets there were no checks. Now we check > at S4U2Proxy level (for all 'second' tickets, S4U2Self and non-S4U2Self > ones). Is that okay? Or do we need to be more specific and check for S4U2Self > second-tickets only (in a S4U2Proxy communication)? That's what I asked you about a more precise way to ensure a cred is a S4U2self one. I thought about stuff the `S4U2Type` value as a "type" field into the credentials returned by `serviceCreds()` but it looks a little ugly. > > Otherwise, looks good to me. Thanks. > > Thanks, Martin.- Hi @martinuy, I looked at the variable names. Some can be named `clientCreds` or `proxyCreds`. Some are for a general `additionalTickets`. I can name it to `additionalCreds` but this "creds" is only one object and not an array. ------------- PR: https://git.openjdk.java.net/jdk/pull/6082
