On Fri, 22 Oct 2021 16:31:02 GMT, Weijun Wang <wei...@openjdk.org> wrote:
> The S4U2proxy extension requires that the service ticket to the first service > has the forwardable flag set, but some versions of Windows Server do not set > the forwardable flag in a S4U2self response and accept it in a S4U2proxy > request. > > There are 2 commits now. The 1st is a refactoring that sends more info into > the methods (Ex: `KdcComm::send(byte[])` -> `KdcComm::send(KrbKdcReq)`, and > `Ticket` -> `Credentials` in multiple places) so that inside `KdcComm::send` > there is enough info to decide how to deal with various errors. The 2nd is > the actual fix to this issue, i.e. ignore the flag and retry another KDC. Hi Max, Thanks for making this contribution. I see the problem that we are addressing here, and the trade-off between the proposed approach and the complexity of locating a DS_BEHAVIOR_WIN2012 DC. I'm okay overall with the decision made. A few minor comments or suggestions: * 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. * 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? * 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'? * 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. * 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? * 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)? Otherwise, looks good to me. Thanks, Martin.- ------------- PR: https://git.openjdk.java.net/jdk/pull/6082
