On Mon, 1 Nov 2021 14:42:32 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.- > >> >> > * 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
