-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8311596: Add separate system properties for TLS server and client for maximum chain length #15163
Conversation
… maximum chain length
/csr |
👋 Welcome back hchao! A progress list of the required criteria for merging this PR into |
@haimaychao has indicated that a compatibility and specification (CSR) request is needed for this pull request. @haimaychao please create a CSR request for issue JDK-8311596 with the correct fix version. This pull request cannot be integrated until the CSR request is approved. |
@haimaychao The following label will be automatically applied to this pull request:
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. |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the other changes look fine to me.
Does this need a test?
@@ -112,6 +112,12 @@ final class SSLConfiguration implements Cloneable { | |||
static final int maxCertificateChainLength = GetIntegerAction.privilegedGetProperty( | |||
"jdk.tls.maxCertificateChainLength", 10); | |||
|
|||
// Limit the maximum certificate chain length accepted from clients |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be moved to after line 89?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remain to stay after the related maxCertificateChainLength.
* system property. | ||
*/ | ||
static { | ||
Integer clientLen = GetIntegerAction.privilegedGetProperty( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could call privilegedGetProperty
with the default value as second argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm looking at the latest version, I don't see @mcpowers suggestion implemented. He's suggesting using the method with this signature:
public static String privilegedGetProperty(String theProp, String defaultVal)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did update the code with his suggestion, and please see it in webrev 01: Incremental. However, I further change the code based on the updated CSR, that the new client and server properties have different default values now from the existing property, and the need to know whether the new property is explicitly set or not. We want to make sure the new properties are not overridden when they are set.
"jdk.tls.maxServerCertificateChainLength"); | ||
maxServerCertificateChainLength = (serverLen != null) ? | ||
serverLen : maxCertificateChainLength; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should take the opportunity here with these new properties as well as jdk.tls.maxCertificateChainLength
to also equate negative numbers (and maybe zero) to be the default. Right now only property values that fail the internal parseInt conversion will evaluate to null
and would be assigned the default I think. But a negative value I think would be taken as-is from the property. Should a negative max cert chain length get set to the default? If so, it might also make sense to give a warning about the offending value and note that it is being set to the default (similar to what GetPropertyAction.privilegedGetTimeoutProp()
does).
If you think this is worthwhile, the CSR should probably be updated to reflect that also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change made to set to the default when a negative value is set for these system properties. Updated CSR for this.
@haimaychao This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. Thanks for making that negative value update.
@haimaychao This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
/open |
@haimaychao This pull request is now open |
// Limit the maximum certificate chain length accepted from clients | ||
static final int maxClientCertificateChainLength; | ||
|
||
// Limit the maximum certificate chain length accepted from servers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would drop "maximum" and just say "Limit the certificate chain length accepted from servers". It's not worth making this change unless you have to make another change to this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
* the values. | ||
*/ | ||
if (maxCertificateChainLength > 0) { | ||
if (clientLen == 8) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user sets "jdk.tls.maxClientCertificateChainLength" precisely to 8 and you will ignore it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since 8 is the default for "jdk.tls.maxClientCertificateChainLength", it is going to be overridden when "jdk.tls.maxCertificateChainLength" is set. Setting "jdk.tls.maxClientCertificateChainLength" to 8 is treated as keeping the original default like no-op.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, "jdk.tls.maxClientCertificateChainLength" is meant to override "jdk.tls.maxClientCertificateChainLength" if both are defined. Then what would happen if user has specified -Djdk.tls.maxClientCertificateChainLength=8 -Djdk.tls.maxCertificateChainLength=4
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jdk.tls.maxCertificateChainLength
will only override jdk.tls.maxClientCertificateChainLength
if jdk.tls.maxCertificateChainLength
is set AND jdk.tls.maxClientCertificateChainLength
is using the default. For the case your provided here, jdk.tls.maxClientCertificateChainLength
will be overridden to be 4 which is set by jdk.tls.maxCertificateChainLength
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not my understanding. Since jdk.tls.maxClientCertificateChainLength
is explicitly set on the command line you should honor it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree that if the application sets jdk.tls.maxClientCertificateChainLength
or jdk.tls.maxServerCertificateChainLength
, it should always take precedence even if the specified value is the same as the default.
You will need to first see if these properties are set before assigning the default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
I was wondering, if it is easier to learn and remember/search by following the naming style "jdk.tls.client.XXX" or "jdk.tls.server.XXX" in SunJSSE provider? |
@XueleiFan The current properties named
Thanks! |
For the name "jdk.tls.maxServerCertificateChainLength", it is not clear to me which side, client or server, the property should be applied to. It could also mean that server can only send out certification with this limitation. For the name Maybe, you can have a try with "jdk.tls.client.maxServerCertificateChainLength", which means for client side, the server certificate chain length (inbound) is limited. Or if you want to simplify the property name, you can have a try for ""jdk.tls.client.maxInboundCertificateChainLength"". |
It sounds good to me that the word "Accepted" is replaced with "Inbound". It should clear out the confusion I think. Thanks for the suggestion. So how about changing the properties names from: |
@haimaychao 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:
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 186 new commits pushed to the
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 |
static final int maxCertificateChainLength = GetIntegerAction.privilegedGetProperty( | ||
"jdk.tls.maxCertificateChainLength", 10); | ||
// Maximum allowed certificate chain length | ||
static final int maxCertificateChainLength; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can just be a local variable now inside the static block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Done.
Integer inboundClientLen = GetIntegerAction.privilegedGetProperty( | ||
"jdk.tls.server.maxInboundCertificateChainLength"); | ||
if (inboundClientLen == null || inboundClientLen < 0) { | ||
inboundClientLen = 8; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic is little too long for me to digest. I wonder if we can just rewrite the line above to
inboundClientLen = globalPropSet ? maxCertificateChainLength : 8;
then there is no need for serverPropSet
and clientPropSet
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, but how does this work? The inbound properties override the global property if both are set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This belongs to the if (inboundClientLen == null || inboundClientLen < 0)
side. The else side stays the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Precisely, it's
if (inboundServerLen == null || inboundServerLen < 0) {
maxInboundClientCertChainLen = globalPropSet ? maxCertificateChainLength : 8;;
} else {
maxInboundClientCertChainLen = inboundServerLen;
}
and nothing else is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated as suggested. Changed code to:
if (inboundServerLen == null || inboundServerLen < 0) {
maxInboundServerCertChainLen = globalPropSet ? maxCertificateChainLength : 10;
} else {
maxInboundServerCertChainLen = inboundServerLen;
}
} else { | ||
globalPropSet = true; | ||
} | ||
maxCertificateChainLength = certLen; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to set certLen
or maxCertificateChainLength
when globalPropSet
is false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed them.
* set. That means if either of those properties have been set, | ||
* the jdk.tls.maxCertificateChainLength property will not override | ||
* the values. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
English is not my native language, but I have some comment on the wording. Normally we don't say maxCertificateChainLength
overrides maxInboundCertificateChainLength
. In fact, it is maxInboundCertificateChainLength
that overrides maxCertificateChainLength
. When maxInboundCertificateChainLength
is not set, it fallbacks to maxCertificateChainLength
(if set) or a default value (8).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that wording is more clear. We should also update the RN with that wording.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section of comments was taken from the CSR. I updated the comments as follows. If it looks fine, I will update the related doc. Thanks!
/*
* If either jdk.tls.server.maxInboundCertificateChainLength or
* jdk.tls.client.maxInboundCertificateChainLength is set, it will
* override jdk.tls.maxCertificateChainLength, regardless of whether
* jdk.tls.maxCertificateChainLength is set or not.
* If neither jdk.tls.server.maxInboundCertificateChainLength nor
* jdk.tls.client.maxInboundCertificateChainLength is set, the behavior
* depends on the setting of jdk.tls.maxCertificateChainLength. If
* jdk.tls.maxCertificateChainLength is set, it falls back to that
* value; otherwise, it defaults to 8 for
* jdk.tls.server.maxInboundCertificateChainLength
* and 10 for jdk.tls.client.maxInboundCertificateChainLength.
* Usesrs can independently set either
* jdk.tls.server.maxInboundCertificateChainLength or
* jdk.tls.client.maxInboundCertificateChainLength.
*/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I did not get time to review this behavior update.
This section of comments was taken from the CSR. I updated the comments as follows. If it looks fine, I will update the related doc. Thanks!
/* * If either jdk.tls.server.maxInboundCertificateChainLength or * jdk.tls.client.maxInboundCertificateChainLength is set, it will * override jdk.tls.maxCertificateChainLength, regardless of whether * jdk.tls.maxCertificateChainLength is set or not.
I'm not sure the statement is clear enough. I think there are two points that need to clarify. The 1st one is that there is a default value of jdk.tls.maxCertificateChainLength, which is 10. The 2nd one is that jdk.tls.maxCertificateChainLength works for both client and server mode. jdk.tls.server.maxInboundCertificateChainLength works on server mode, and it does not work for client mode, and therefore it cannot override client mode behavior for jdk.tls.maxCertificateChainLength. The same for jdk.tls.client.maxInboundCertificateChainLength.
To be clear, the release note or the comment might be placed in different code block like:
/* If jdk.tls.server.maxInboundCertificateChainLength is set, it will override jdk.tls.maxCertificateChainLength behavior for server side.
*/
Integer inboundClientLen = ...
/* If jdk.tls.client.maxInboundCertificateChainLength is set, it will override jdk.tls.maxCertificateChainLength behavior for client side.
*/
Integer inboundServerLen = GetIntegerAction.privilegedGetProperty(
* If neither jdk.tls.server.maxInboundCertificateChainLength nor * jdk.tls.client.maxInboundCertificateChainLength is set, the behavior * depends on the setting of jdk.tls.maxCertificateChainLength. If * jdk.tls.maxCertificateChainLength is set, it falls back to that * value; otherwise, it defaults to 8 for * jdk.tls.server.maxInboundCertificateChainLength
Previously, the jdk.tls.maxCertificateChainLength default value is 10. Now it is changed to 8. This is a behavior change, please document it in release note, or just use 10 in the implementation.
* and 10 for jdk.tls.client.maxInboundCertificateChainLength. * Usesrs can independently set either * jdk.tls.server.maxInboundCertificateChainLength or * jdk.tls.client.maxInboundCertificateChainLength. */
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a behavior change that conflicts with the CSR. I think it is a wording issue, let me suggest some improvements in another comment. There is no longer a default value for jdk.tls.maxCertificateChainLength
. Where is it set to 8 in the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The choice of 8 for the client is mostly based on different processing requirements and use cases for TLS client vs server certificate chains. If we see evidence that 8 is too low, we can always consider adjusting it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if the number 8 or 10 really make a good difference in practice. No matter 8 or 10, if customers need lower value, they can always consider adjusting it. My concern is mainly about compatibility issues. If you want to keep the behavior changes, as there is potential compatibility issue, please feel free to describe the behavior change in release note and CSR if you would like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point - the CSR and RN could have been a bit more specific about the compatibility effect of changing the default from 10 to 8, so we will update that. Note that the CertPathBuilder default max path length is 5 non-self-issued intermediate CA certificates, so even a lower value of 8 should have low risk as rigid TLS cert chains greater than 6 certs (where the 6th cert is the end entity cert which is not affected by the CertPathBuilder limit) on the wire will already be rejected. The additional certs permitted in the wire format is more for including a few more additional certs that might help build a valid path when there is more than one possible chain that a server or client might accept, which sometimes happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point about the setMaxPathLength() limitation. It looks like there might be an issue when the interactivities of setMaxPathLength(and its default value) and the properties defined here are not considered. For example, what if the property is set to 8, while the setMaxPathLength is of value 5? For the "PKIX" trust manager, per your description, if the property is set to 8, but 5 is the limit actually. It looks like a weird behavior to me. If I remember correctly, the "SunX509" trust manager does not use PKIXBuilderParameters, while the "PKIX" trust manager does. It might be not the behavior we'd like to have that property 8 work for "SunX509" but not for "PKIX" trust manager.
Anyway, it might be better to look into the interactive behaviors among the properties and setMaxPathLength/default value.
Good point - the CSR and RN could have been a bit more specific about the compatibility effect of changing the default from 10 to 8, so we will update that. Note that the CertPathBuilder default max path length is 5 non-self-issued intermediate CA certificates, so even a lower value of 8 should have low risk as rigid TLS cert chains greater than 6 certs (where the 6th cert is the end entity cert which is not affected by the CertPathBuilder limit) on the wire will already be rejected. The additional certs permitted in the wire format is more for including a few more additional certs that might help build a valid path when there is more than one possible chain that a server or client might accept, which sometimes happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PKIX has been the default SunJSSE TrustManager since JDK 5 and AFAIK we have not had any reports of compatibility issues. This leads me to believe that valid TLS chains longer than 6 certificates are extremely rare.
* value; otherwise, it defaults to 8 for | ||
* jdk.tls.server.maxInboundCertificateChainLength | ||
* and 10 for jdk.tls.client.maxInboundCertificateChainLength. | ||
* Usesrs can independently set either |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: s/Usesrs/Users
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Thanks all for the review. |
/integrate |
Going to push as commit 0064cf9.
Your commit was automatically rebased without conflicts. |
@haimaychao Pushed as commit 0064cf9. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Please review the enhancement for JDK-8311596 and its CSR JDK-8313236. Thank you.
Progress
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15163/head:pull/15163
$ git checkout pull/15163
Update a local copy of the PR:
$ git checkout pull/15163
$ git pull https://git.openjdk.org/jdk.git pull/15163/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 15163
View PR using the GUI difftool:
$ git pr show -t 15163
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15163.diff
Webrev
Link to Webrev Comment