RFR: 8272162: S4U2Self ticket without forwardable flag

[Martin Balao]

On Mon, 1 Nov 2021 14:42:32 GMT, Martin Balao <mbalao at 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.-
>
>> 
>> > * 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.
>> 
> 
> This would be tricky. The problem is that the 'cname' and 'crealm' in the S4U2Self ticket are the user's ones; so indistinguishable from the non-S4U2Self. The 'sname' and 'srealm' are also equal: the middle service principal. I'm not sure if there are any differences in the PAC. Even when it's a bit 'ugly', storing the 'type' looks like a reliable scheme in my opinion. But the question that concerns me most is if we really want to make such a tight check, or we are willing to forward everything. I'd suggest to keep your proposal as it is now in this regard. Meanwhile, I'll check what the MIT client does and let you know if there is anything that we to consider.

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

`additionalTickets` is a term introduced in the RFC. Even when it does not have defined semantics (i.e.: what are these attached/additional tickets for?), I'd keep it for everything related to message formatting. My comment was more about 'second', which is undefined in RFC/docs and not a very meaningful name. I prefer `clientCreds` over `proxyCreds` because 'proxy' makes me think about the middle-service. What about `userCreds`? (the reason I like 'user' is because it's more about the actor, while client might be a role that the middle-service is playing in a communication with a KDC or a back-end)

-------------

PR: https://git.openjdk.java.net/jdk/pull/6082