8378500: Restrict IP literals from being allowed in javax.net.ssl.SNIHostName

[vy]

Per RFC 6066 "3. Server Name Indication", disallow IP literals in SNIHostName::new.

While the following two call-sites could be simplified by removing IP literal checks, I've refrained from doing so because delegating some of the checks to an exception catching mechanism would impact the performance:

sun.security.ssl.Utilities::rawToSNIHostName
sun.net.www.protocol.https.HttpsClient::afterConnect

---

• I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change requires CSR request JDK-8382318 to be approved

Issues

• JDK-8378500: Restrict IP literals from being allowed in javax.net.ssl.SNIHostName (Bug - P3)
• JDK-8382318: Restrict IP literals from being allowed in javax.net.ssl.SNIHostName (CSR)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/30747/head:pull/30747
$ git checkout pull/30747

Update a local copy of the PR:
$ git checkout pull/30747
$ git pull https://git.openjdk.org/jdk.git pull/30747/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 30747

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/30747.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back vyazici! 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]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@vy The following labels will be automatically applied to this pull request:

• net
• security

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

[mlbridge]

Webrevs

• 01: Full - Incremental (b2c40b25)
• 00: Full (bd7aea4f)

[AlanBateman]

/csr

[AlanBateman]

I've added the "csr" label as I think (and correct me if I have this wrong) that javax.net.ssl.SNIHostName can now fail for cases that it didn't previously fail.

[openjdk]

@AlanBateman has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@vy please create a CSR request for issue JDK-8378500 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

[dfuch]

Yes - a CSR is needed for this change. Thanks for adding the label @AlanBateman.

[vy]

We've decided to leave SNIHostName::new untouched, and introduce a new static factory method with stricter checks. See the CSR discussion for details. I will update the PR in this direction.

[vy]

In b2c40b2,

• I've added the ofString() static factory method
• I've updated the SNIHostName(String) ctor specification:
  • I've placed the interesting bits (i.e., what qualifies as an illegal host name) to the top
  • I've moved ASCII/IDN validation story to a dedication subsection
  • I've added an @apiNote to advice users to prefer ofString() instead (this was suggested by @dfuch, IIRC)
• In the ofString() specification, I've added an @apiNote to advice users to have a catch (IAE) safety net

An alternative direction could be making the new SNIHostName(String hostname, boolean strict) ctor public instead of adding a new static factory method.

Note that this is a draft to have a feeling on the end result — SNIHostName(byte[])/tests/etc. are not adapted yet.

@AlanBateman, @dfuch, @djelinski, would you mind reviewing these changes, please? How shall I proceed?

[AlanBateman]

I don't know if this will be seen, which makes me wonder if this should go a step further and deprecate the constructors.

[vy]

I'm glad you said this, I completely agree with your concern and reasoning. I will do it.

Though this reminded me another question I need some help with: Shall I migrate existing usages of SNIHostName::new to the new static factory methods in this PR? Slim PRs are ideal, but leaving the correction of call-sites to a 2nd PR bears the risk that we might end up having a 3rd PR if the 1st (this) one has any problems.

[AlanBateman]

There may be compatibility issues with change the existing usages (as they might fail with IAE) so I would keep that separate.

[AlanBateman]

ofHostName(String) and ofEncoded(byte) is another choice that would make it clear at the use sites.

[vy]

I had considered how it would look like at the call-site:

SNIHostName.ofHostName(hostName);  // Too verbose/noisy with repetition?

and then thought of:

SNIHostName.ofString(hostName);
SNIHostName.ofBytes(hostName);

But I will leave the decision on naming to those more experienced in this field.

[AlanBateman]

SNIHostName.of(String) and SNIHostName.ofEncoded(byte[]) are other candidates.

[Michael-Mc-Mahon]

This list seems to imply that an SNI hostname must be an IDN. I wonder if it might be better to first state what a valid SNI hostname is (ie a DNS hostname), mentioning IDNs and then later specify the exclusions, including (I presume) a string that starts with "xn--" but which doesn't represent a valid IDN?