8217633: Configurable extensions with system properties #1752

XueleiFan · 2020-12-11T23:20:34Z

The TLS protocols are designed to tolerant unknown TLS extensions. However, although it is not common, there are a few TLS implementations that cannot handle unknown extensions properly. As results in unexpected interoperability issue when new extensions are introduced in JDK. The interoperability impact could be mitigated If applications can customize the extensions if needed.

With this update, two system properties are added to configure the default extensions in either client or server side of TLS connections. Please note that the impact of blocking TLS extensions is complicated. For example, a TLS connection may not be able to established if a mandatory extension is blocked. Please don't use this feature unless you clearly understand the impact.

Bug: https://bugs.openjdk.java.net/browse/JDK-8217633
CSR: https://bugs.openjdk.java.net/browse/JDK-8217993

Progress

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

Issue

JDK-8217633: Configurable extensions with system properties

Reviewers

Rajan Halade (@rhalade - Reviewer) ⚠️ Review applies to 5bd6e86
Jamil Nimeh (@jnimeh - Reviewer) ⚠️ Review applies to ad5f333

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1752/head:pull/1752
$ git checkout pull/1752

bridgekeeper · 2020-12-11T23:21:34Z

👋 Welcome back xuelei! 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 · 2020-12-11T23:22:56Z

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

mlbridge · 2020-12-11T23:28:08Z

Webrevs

XueleiFan · 2020-12-11T23:29:29Z

/csr needed

openjdk · 2020-12-11T23:29:48Z

@XueleiFan this pull request will not be integrated until the CSR request JDK-8217993 for issue JDK-8217633 has been approved.

bridgekeeper · 2021-01-09T02:38:42Z

@XueleiFan 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!

XueleiFan · 2021-01-22T16:38:22Z

ping ...

RogerRiggs

Where will the new properties be documented?

In the CSR, it may be worth mentioning that the extension names are case sensitive.

XueleiFan · 2021-01-22T20:19:52Z

Where will the new properties be documented?

Yes. The properties will be described in the JSSE Reference Guide. I added a "Documentation" section in the CSR.

In the CSR, it may be worth mentioning that the extension names are case sensitive.
Good point.

Thanks for the review.

rhalade · 2021-01-25T21:30:05Z

test/jdk/sun/security/ssl/SSLSocketImpl/BlockedExtension.java

@@ -0,0 +1,75 @@
+/*
+ * Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.


Suggested change

* Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.

* Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.

rhalade · 2021-01-25T21:32:09Z

test/jdk/sun/security/ssl/SSLSocketImpl/BlockedExtension.java

+            (new BlockedExtension(new String[] {args[1]})).run();
+        } catch (SSLException | IllegalStateException ssle) {
+            if (shouldSuccess) {
+                throw new Exception(


Suggested change

throw new Exception(

throw new RuntimeException(

rhalade · 2021-01-25T21:32:18Z

test/jdk/sun/security/ssl/SSLSocketImpl/BlockedExtension.java

+        }
+
+        if (!shouldSuccess) {
+            throw new Exception(


Suggested change

throw new Exception(

throw new RuntimeException(

Good catches, the years should be 2021 now. Thanks!

jnimeh

Looks good to me.

mlbridge · 2021-01-26T00:12:49Z

Mailing list message from Bernd Eckenfels on security-dev:

Hello,

I wanted to mention again, that all those System property configurations are good, especially to resolve the update pains, but not really useful if you want to make configurations on a per-connection base. If you have to support multiple partners it can be a real pain to setup a common feature set or multiple instances. For this a generic feature setter for the context would be really useful. Most prominent recent example is the ca-extension, which only really makes sense if you also did programmatically configure a small list of trusted CAs.

I also think it would overall clean up the code and give a good place for Javadoc all those options.
Not to mention the default could be tied to a few new context names.

Gruss
Bernd
--
http://bernd.eckenfels.net
________________________________
Von: security-dev <security-dev-retn at openjdk.java.net> im Auftrag von Xue-Lei Andrew Fan <xuelei at openjdk.java.net>
Gesendet: Monday, January 25, 2021 11:17:56 PM
An: security-dev at openjdk.java.net <security-dev at openjdk.java.net>
Betreff: Re: RFR: 8217633: Configurable extensions with system properties [v2]

The TLS protocols are designed to tolerant unknown TLS extensions. However, although it is not common, there are a few TLS implementations that cannot handle unknown extensions properly. As results in unexpected interoperability issue when new extensions are introduced in JDK. The interoperability impact could be mitigated If applications can customize the extensions if needed.

With this update, two system properties are added to configure the default extensions in either client or server side of TLS connections. Please note that the impact of blocking TLS extensions is complicated. For example, a TLS connection may not be able to established if a mandatory extension is blocked. Please don't use this feature unless you clearly understand the impact.

Bug: https://bugs.openjdk.java.net/browse/JDK-8217633
CSR: https://bugs.openjdk.java.net/browse/JDK-8217993

Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision:

Update copyright years to 2021

-------------

Changes:
- all: https://git.openjdk.java.net/jdk/pull/1752/files
- new: https://git.openjdk.java.net/jdk/pull/1752/files/ad5f3330..5bd6e86

Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1752&range=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1752&range=00-01

Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod
Patch: https://git.openjdk.java.net/jdk/pull/1752.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/1752/head:pull/1752

PR: https://git.openjdk.java.net/jdk/pull/1752
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20210126/5f676ee3/attachment.htm>

XueleiFan · 2021-01-26T18:33:04Z

Hi Bernd,

I agree with you that System property is not as useful to configure individual connections. It is mostly used for corner cases that have interoperability or compatibility issues. A general program should use APIs and the default system properties.

Mailing list message from Bernd Eckenfels on security-dev:

Hello,

I wanted to mention again, that all those System property configurations are good, especially to resolve the update pains, but not really useful if you want to make configurations on a per-connection base. If you have to support multiple partners it can be a real pain to setup a common feature set or multiple instances. For this a generic feature setter for the context would be really useful. Most prominent recent example is the ca-extension, which only really makes sense if you also did programmatically configure a small list of trusted CAs.

Yes, ca-extension is an item I was thinking of to support in JDK.

I also think it would overall clean up the code and give a good place for Javadoc all those options.
Not to mention the default could be tied to a few new context names.

Currently, the system properties are documented in the JSSE Reference Guides. But just as you know, it is as easy to follow. I agree with you that it would be nice to have better place to have them all together.

Thank you for the review.

Regards,
Xuelei

Gruss
Bernd

http://bernd.eckenfels.net

openjdk · 2021-01-29T02:51:15Z

@XueleiFan 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:

8217633: Configurable extensions with system properties

Reviewed-by: rhalade, jnimeh

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 23 new commits pushed to the master branch:

1519632: 8251462: Simplify compilation policy
71128cf: 8260586: Shenandoah: simplify "Concurrent Weak References" logging
81e9e6a: 8260461: Modernize jsr166 tck tests
2b166d8: 8260596: Comment cleanup in BigInteger
1de3c55: 8260584: Shenandoah: simplify "Concurrent Thread Roots" logging
bbbfaa5: 8249262: Initialize InstanceKlass::_package_entry during CDS dump time
3aabbd7: 8260432: allocateSpaceForGP in freetypeScaler.c might leak memory
abc4300: 8257746: Regression introduced with JDK-8250984 - memory might be null in some machines
13ca433: 8259628: jdk/net/ExtendedSocketOption/AsynchronousSocketChannelNAPITest.java fails intermittently
baf46ba: 8259801: Enable XML Signature secure validation mode by default
... and 13 more: https://git.openjdk.java.net/jdk/compare/7030d2e026dceab82d795646dff357210aa54061...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.

XueleiFan · 2021-01-30T00:20:09Z

/integrate

openjdk · 2021-01-30T00:21:26Z

@XueleiFan Since your change was applied there have been 37 commits pushed to the master branch:

f5ca838: 8065773: JDI: UOE is not thrown, when redefineClasses changes a class modifier
739bbd0: 8260566: Pattern type X is a subtype of expression type Y message is incorrect
b2e582a: 8259905: Compiler treats 'sealed' keyword as 'var' keyword
6415d3b: 8259584: SuperWord::fix_commutative_inputs checks in_bb(fin1) instead of in_bb(fin2)
b7b391b: 8260625: Rename MetaspaceExpand_lock
22bfa5b: 8255837: Shenandoah: Remove ShenandoahConcurrentRoots class
53f1b93: 8260518: Change default -mmacosx-version-min to 10.12
24a2621: 8260592: jpackage tests fail when Desktop is not supported
d2b0ea1: 8260282: Add option to compress heap dumps created by -XX:+HeapDumpOnOutOfMemoryError
a5fb517: 8259404: Shenandoah: Fix time tracking in parallel_cleaning
... and 27 more: https://git.openjdk.java.net/jdk/compare/7030d2e026dceab82d795646dff357210aa54061...master

Your commit was automatically rebased without conflicts.

Pushed as commit 71bfe96.

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

mdameda · 2023-02-02T14:16:24Z

Hi @XueleiFan , i don't find this patch into latest OpenJDK 1.8 releases . Is there a plan to integrate this patch also into OpenJDK 1.8 ? (i see that OracleJDK 1.8 was updated)

Xuelei Fan added 3 commits December 9, 2020 13:34

8217633: Configurable extensions with system properties

705b6d6

Add regression test

181be98

Remove swp file

ad5f333

openjdk bot added the rfr Pull request is ready for review label Dec 11, 2020

openjdk bot added the security security-dev@openjdk.org label Dec 11, 2020

openjdk bot added the csr Pull request needs approved CSR before integration label Dec 11, 2020

RogerRiggs reviewed Jan 22, 2021

View reviewed changes

rhalade suggested changes Jan 25, 2021

View reviewed changes

jnimeh approved these changes Jan 25, 2021

View reviewed changes

rhalade approved these changes Jan 25, 2021

View reviewed changes

Xuelei Fan added 2 commits January 27, 2021 17:10

rename the system property names

88beb8a

Merge

ed9409c

openjdk bot removed the csr Pull request needs approved CSR before integration label Jan 29, 2021

openjdk bot added the ready Pull request is ready to be integrated label Jan 29, 2021

openjdk bot closed this Jan 30, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated labels Jan 30, 2021

openjdk bot removed the rfr Pull request is ready for review label Jan 30, 2021

XueleiFan deleted the JDK-8217633 branch January 30, 2021 00:26

8217633: Configurable extensions with system properties #1752

8217633: Configurable extensions with system properties #1752

XueleiFan commented Dec 11, 2020 •

edited by openjdk bot

bridgekeeper bot commented Dec 11, 2020

openjdk bot commented Dec 11, 2020

mlbridge bot commented Dec 11, 2020 •

edited

XueleiFan commented Dec 11, 2020

openjdk bot commented Dec 11, 2020

bridgekeeper bot commented Jan 9, 2021

XueleiFan commented Jan 22, 2021

RogerRiggs left a comment

XueleiFan commented Jan 22, 2021

rhalade Jan 25, 2021

rhalade Jan 25, 2021

rhalade Jan 25, 2021

XueleiFan Jan 25, 2021

jnimeh left a comment

mlbridge bot commented Jan 26, 2021

XueleiFan commented Jan 26, 2021

Gruss
Bernd

openjdk bot commented Jan 29, 2021

XueleiFan commented Jan 30, 2021

openjdk bot commented Jan 30, 2021

mdameda commented Feb 2, 2023 •

edited by bridgekeeper bot

		@@ -0,0 +1,75 @@
		/*
		* Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.

	* Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
	* Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.

8217633: Configurable extensions with system properties #1752

8217633: Configurable extensions with system properties #1752

Conversation

XueleiFan commented Dec 11, 2020 • edited by openjdk bot

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Dec 11, 2020

openjdk bot commented Dec 11, 2020

mlbridge bot commented Dec 11, 2020 • edited

Webrevs

XueleiFan commented Dec 11, 2020

openjdk bot commented Dec 11, 2020

bridgekeeper bot commented Jan 9, 2021

XueleiFan commented Jan 22, 2021

RogerRiggs left a comment

Choose a reason for hiding this comment

XueleiFan commented Jan 22, 2021

rhalade Jan 25, 2021

Choose a reason for hiding this comment

rhalade Jan 25, 2021

Choose a reason for hiding this comment

rhalade Jan 25, 2021

Choose a reason for hiding this comment

XueleiFan Jan 25, 2021

Choose a reason for hiding this comment

jnimeh left a comment

Choose a reason for hiding this comment

mlbridge bot commented Jan 26, 2021

XueleiFan commented Jan 26, 2021

Gruss Bernd

openjdk bot commented Jan 29, 2021

XueleiFan commented Jan 30, 2021

openjdk bot commented Jan 30, 2021

mdameda commented Feb 2, 2023 • edited by bridgekeeper bot

XueleiFan commented Dec 11, 2020 •

edited by openjdk bot

mlbridge bot commented Dec 11, 2020 •

edited

Gruss
Bernd

mdameda commented Feb 2, 2023 •

edited by bridgekeeper bot