On Mon, 1 Nov 2021 17:24:48 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. > > Oops, I introduced a bug. At the end of the constructor of `KrbTgsReq`, > `options` could be changed. Since I'm now calculating the encoding on-demand, > the encoding will also change. I'll fix this in another commit. Hi @wangweij , Yes, I see the concern about the 50 KDCs scenario. But one nuance here: having different AD behaviors with issued tickets that won't be accepted in other services afterwards looks to me like a configuration problem that goes beyond OpenJDK. Now, if we cannot distinguish between S4U2Self and non-S4U2Self second tickets based on their actual data, it's likely that the KDC behaves in the same way when it comes to the forwardable flag -both in 2012 DC and after-. That would be a reason not to establish any difference in OpenJDK and behave in the same way. This official documentation here [1] makes no distinction between the two types: "The S4U2proxy extension requires that the service ticket to the first service has the forwardable flag set (see Service 1 in the figure specifying Kerberos delegation with forwarded TGT, section 1.3.3). This ticket can be obtained through an S4U2self protocol exchange." If we are concerned about this, why don't we let the user decide the behavior by means of a security/system property? That would also mitigate the risk of assuming that a KDC_ERR_BADOPTION error is always because of a non-forwardable ticket on a 2012 DC -which looks quite an assumption to me-. Thanks, Martin.- -- [1] - https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-sfu/bde93b0e-f3c9-4ddf-9f44-e1453be7af5a > First, my latest commit contains a mechanism to tell if a ticket is from > S4U2self. Is it significant enough to change your mind? > > Even if there is a system property for this purpose (suppose it's named > `jdk.security.krb5.accept.non.forwardble.s4u2self.response.and.retry`), I > wonder if it's worth turning it off. As you said, a legitimate S4U2self > should always be FORWARDABLE, and in this case the system property is > useless. But, if a service really receives such a ticket, I assume it would > prefer to try it in a S4U2proxy request instead of just failing early. After > all, when it impersonates someone, its purpose should be accessing a backend > service. > > We usually introduce a system property for a compatibility reason so that > existing normal cases will not behave differently, but here, we are simply > trying to resurrect a former failure. > > The main problem I see with the current approach is still about whether a > "tolerant" KDC can be accessed in time. If this can be optimized by adjusting > the orders of KDC either in krb5.conf or in the DNS response, then I would be > greatly relieved. Hi @wangweij , I liked the renaming that you proposed, including the 'ccreds -> middleTGT' change. I find it easier to reason about now. There is still a 'ccreds' in CredentialsUtil::acquireS4U2proxyCreds that you might want to check. In regards to 'type' -and assuming that we want to keep that check-, can we re-use the CredentialsUtil::S4U2Type enum type? I'm fine if you want to move the enum location to Credentials.java. I'm not sure that we are resurrecting an old failure. My understanding is that the new KrbException exception (in KdcComm::sendIfPossible) is causing a behavioral change in the sense that it's now allowing KDC iteration, and there is no way to go back to the old fail-immediately behavior. The property would be to avoid iterating over the 50 KDCs after the first failure; or it could be to not even try if the ticket is non-forwardable. In that sense, the property's name would be something like 'jdk.security.krb5.s4u2proxy.acceptNonForwardableServiceTicket'. My second concern is that the condition that we use to iterate might be beyond the forwardable flag scope, because we are now based on a KDC_ERR_BADOPTION error. I personally wouldn't make any distinction between S4U2Self and non-S4U2Self second tickets when it comes to the second ticket forwardable flag. > That `ccreds` in `acquireS4U2selfCreds`? I'll fix it. > > Other comments accepted. I'll add a system property. Hi @wangweij , Thanks for taking the backward-compatibility property suggestion. I went through all the changes again and have some comments / questions: * I realized that the field 'Krb5Context::serviceCreds' in Krb5Context.java is now overloaded and might have two different value depending on whether it's an Initiator or an Acceptor context. A comment next to the field declaration might be helpful if you want to consider: "// On the Initiator side, contains the final TGS to a service on both delegation and no-delegation scenarios. On the Acceptor side, contains a user TGS usable for delegation.". Note that previous to this change, the field was meaningful for the Initiator side only, as the Acceptor side was using 'Krb5Context::serviceTicket'. * In Credentials.java, can we replace 'ccreds' with 'middleTGT' in acquireS4U2proxyCreds? This would align the names with CredentialsUtil::acquireS4U2proxyCreds and, in my opinion, would be more clear. Additionally, you might want to consider replacing 'ccreds' with 'initCreds' in acquireServiceCreds, so it's aligned with CredentialsUtil::acquireServiceCreds. * In KrbTgsReq.java, you said 'At the end of the constructor of KrbTgsReq, options could be changed. Since I'm now calculating the encoding on-demand, the encoding will also change. I'll fix this in another commit.'. Have you made that change? I still see 'obuf = ...' before 'options.set(KDCOptions.FORWARDED, true);'. * I'm not entirely sure of the reason why 'KrbKdcReq::getMessage' was reverted to 'KrbKdcReq::encoding'. It does not look wrong but wish I could understand the reason. * In CredentialsUtil.java, new line missing in 'return creds;' (CredentialsUtil::acquireS4U2selfCreds) * In java.security, I'd reword the text a bit (and fix a typo). One suggestion for you to consider: "The S4U2proxy Kerberos extension enables a middle service to obtain a service ticket to another service on behalf of a user. It requires that the user service ticket to the middle service has the forwardable flag set [1]. However, some implementations ignore this requirement and accept service tickets with the flag unset. If this security property is set to "true", then 1) The user service ticket, when obtained by the middle service after a S4U2self impersonation, is not required to have the forwardable flag set; and, 2) If a S4U2proxy request receives a KRB_ERROR of the KDC_ERR_BADOPTION error code and the ticket to the middle service is not forwardable, OpenJDK will try the same request in another KDC instead of treating it as a fatal failure. The default value is "false". If a system property of the same name is also specified, it supersedes the security property value defined here. [1] https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-sfu/bde93b0e-f3c9-4ddf-9f44-e1453be7af5a jdk.security.krb5.s4u2proxy.acceptNonForwardableServiceTicket=false " Thanks, Martin.- > All suggestions accepted, except that I still write out the full name for > S4U2proxy in `java.security`. > > For the `KrbKdcReq` method name. It's now `encoding` because it's returning a > byte array. `getMessage` was used to return a message type. Hi @wangweij Thanks for considering my suggestions. I agree with removing the 'forwardable middle service TGT' requirement from S4U2Self. This looks good to me. Note: I'm not an official JDK Reviewer so you'll need someone else to have a look. Martin.- ------------- PR: https://git.openjdk.java.net/jdk/pull/6082
