8272162: S4U2Self ticket without forwardable flag

[wangweij]

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.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issues

• JDK-8272162: S4U2Self ticket without forwardable flag
• JDK-8277308: S4U2Self ticket without forwardable flag (CSR) ⚠️ Issue is not open.

Reviewers

• Valerie Peng (@valeriepeng - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6082/head:pull/6082
$ git checkout pull/6082

Update a local copy of the PR:
$ git checkout pull/6082
$ git pull https://git.openjdk.java.net/jdk pull/6082/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6082

View PR using the GUI difftool:
$ git pr show -t 6082

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6082.diff

[bridgekeeper]

👋 Welcome back weijun! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@wangweij The following label will be automatically applied to this pull request:

• security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[martinuy]

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

[wangweij]

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

[wangweij]

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.

[martinuy]

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

[martinuy]

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)

[wangweij]

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.

Alexey said their customer has at least 50 KDCs. It will be quite a waste of time if we go through each of them and get a KDC_ERR_BADOPTION all the time. Therefore I would like this retry to be as restricted as possible.

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)

Unfortunately we cannot call them additionalTickets anymore, first it's no longer just a message, second it's not plural. Yes, userCreds is better. One place proxyCreds is used is because it's a Krb5ProxyCredential. As for second, I think it might be from the "second ticket" inside a ccache.

I've pushed a new commit for everything I've tried on.

[wangweij]

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.

[martinuy]

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

[wangweij]

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.

[martinuy]

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.

[wangweij]

That ccreds in acquireS4U2selfCreds? I'll fix it.

Other comments accepted. I'll add a system property.

[wangweij]

/csr

[openjdk]

@wangweij has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@wangweij please create a CSR request for issue JDK-8272162. This pull request cannot be integrated until the CSR request is approved.

[martinuy]

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

[wangweij]

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.

[martinuy]

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

[valeriepeng]

nit: can just use sname on line 64?

[wangweij]

Sure.

[mlbridge]

Webrevs

• 01: Full - Incremental (1f93a881)
• 00: Full (c07e6f64)

[valeriepeng]

nit: swap to use "static final"

[wangweij]

Done.

[valeriepeng]

Looks good. Thanks, Valerie

[openjdk]

@wangweij This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8272162: S4U2Self ticket without forwardable flag

Reviewed-by: valeriep

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 175 new commits pushed to the master branch:

• 2942646: 8276683: Malformed Javadoc inline tags in JDK source in com/sun/tools/javac/util/RawDiagnosticFormatter.java
• e30e676: 8277606: String(String) constructor could copy hashIsZero
• 5a4a9bb: 8278019: ProblemList java/awt/dnd/BadSerializationTest/BadSerializationTest.java on linux and windows
• 15a6806: 8277434: tests fail with "assert(is_forwarded()) failed: only decode when actually forwarded"
• 21d9ca6: 8274983: C1 optimizes the invocation of private interface methods
• 98a9f03: 8277602: Deopt code does not extend the stack enough if the caller is an optimize entry blob
• 9150840: 8277899: Parallel: Simplify PSVirtualSpace::initialize logic
• 01cefc9: 8277977: Incorrect references to --enable-reproducible-builds in docs
• 69f56a0: 8264485: build.tools.depend.Depend.toString(byte[]) creates malformed hex strings
• fecf906: 8267928: Loop predicate gets inexact loop limit before PhaseIdealLoop::rc_predicate
• ... and 165 more: https://git.openjdk.java.net/jdk/compare/b0a463fa59a1c3c554f48267525729bf89a2c5be...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[wangweij]

/integrate

[openjdk]

Going to push as commit ab867f6.
Since your change was applied there have been 178 commits pushed to the master branch:

• dd73e3c: 8277814: ConcurrentRefineThread should report rate when deactivating
• 65251f7: 8151594: Move concurrent refinement thread activation logging out of GC pause
• f1c20e9: 8190748: java/text/Format/DateFormat/DateFormatTest.java and NonGregorianFormatTest fail intermittently
• 2942646: 8276683: Malformed Javadoc inline tags in JDK source in com/sun/tools/javac/util/RawDiagnosticFormatter.java
• e30e676: 8277606: String(String) constructor could copy hashIsZero
• 5a4a9bb: 8278019: ProblemList java/awt/dnd/BadSerializationTest/BadSerializationTest.java on linux and windows
• 15a6806: 8277434: tests fail with "assert(is_forwarded()) failed: only decode when actually forwarded"
• 21d9ca6: 8274983: C1 optimizes the invocation of private interface methods
• 98a9f03: 8277602: Deopt code does not extend the stack enough if the caller is an optimize entry blob
• 9150840: 8277899: Parallel: Simplify PSVirtualSpace::initialize logic
• ... and 168 more: https://git.openjdk.java.net/jdk/compare/b0a463fa59a1c3c554f48267525729bf89a2c5be...master

Your commit was automatically rebased without conflicts.

[openjdk]

@wangweij Pushed as commit ab867f6.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.