Skip to content
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

8296820: Add implementation note to SSLContext.getInstance noting subsequent behavior if protocol is disabled #11172

Closed
wants to merge 2 commits into from

Conversation

seanjmullan
Copy link
Member

@seanjmullan seanjmullan commented Nov 15, 2022

Please review this PR to add an implementation note to theSSLContext.getInstance methods to document the behavior when a protocol is disabled.


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

Issue

  • JDK-8296820: Add implementation note to SSLContext.getInstance noting subsequent behavior if protocol is disabled

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11172

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 15, 2022

👋 Welcome back mullan! 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 openjdk bot added the rfr Pull request is ready for review label Nov 15, 2022
@openjdk
Copy link

openjdk bot commented Nov 15, 2022

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

@openjdk openjdk bot added security security-dev@openjdk.org net net-dev@openjdk.org labels Nov 15, 2022
@openjdk
Copy link

openjdk bot commented Nov 15, 2022

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

8296820: Add implementation note to SSLContext.getInstance noting subsequent behavior if protocol is disabled

Reviewed-by: darcy

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

  • fafe682: 8295861: get rid of list argument in debug agent's removeNode() API
  • 216c6f6: 8294881: test/hotspot/jtreg/vmTestbase/nsk/jdi/VirtualMachine/dispose/dispose003/TestDescription.java fails
  • 6aef3a4: 8262435: Clarify the behavior of a few inherited ZipInputStream methods
  • c042b8e: 8294731: Improve multiplicative inverse for secp256r1 implementation
  • d3051a7: 8296736: Some PKCS9Attribute can be created but cannot be encoded
  • decb1b7: 8286800: Assert in PhaseIdealLoop::dump_real_LCA is too strong
  • c49e484: 8294739: jdk/jshell/ToolShiftTabTest.java timed out
  • a45c9af: 8295814: jdk/jshell/CommandCompletionTest.java fails with "lists don't have the same size expected [2] but found [1]"
  • d0fae43: 8294947: Use 64bit atomics in patch_verified_entry on x86_64
  • 6f467cd: 8295934: IGV: keep node selection when changing view or graph
  • ... and 10 more: https://git.openjdk.org/jdk/compare/749335d34ac570760279ac81308d5d323aba4067...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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 15, 2022
@mlbridge
Copy link

mlbridge bot commented Nov 15, 2022

Webrevs

@XueleiFan
Copy link
Member

As you are already there, it may be nice to cover more options that the protocol may not be used for the handshaking. Here are some cases I'm aware of:

  1. the protocol is disabled with Security property
  2. the protocol is not set while setting the SSLParameters
  3. the protocol is not set while set the enabled protocol
  4. the protocol is not supported by the peer
  5. the protocol is not supported by any cipher suites enabled.

@seanjmullan
Copy link
Member Author

As you are already there, it may be nice to cover more options that the protocol may not be used for the handshaking. Here are some cases I'm aware of:

  1. the protocol is disabled with Security property
  2. the protocol is not set while setting the SSLParameters
  3. the protocol is not set while set the enabled protocol
  4. the protocol is not supported by the peer
  5. the protocol is not supported by any cipher suites enabled.

Good point, but I feel that should be handled in a separate issue, if necessary. Also, most of those other cases above (except for #4) are influenced by additional methods subsequently invoked by the application. For this one, it may be a bit surprising for the SSLContext to be returned when the protocol is disabled (although it is still compliant with the API as specified), and then later it doesn't work, so the implementation note is useful. The JCK team requested this clarification.

@XueleiFan
Copy link
Member

As you are already there, it may be nice to cover more options that the protocol may not be used for the handshaking. Here are some cases I'm aware of:

  1. the protocol is disabled with Security property
  2. the protocol is not set while setting the SSLParameters
  3. the protocol is not set while set the enabled protocol
  4. the protocol is not supported by the peer
  5. the protocol is not supported by any cipher suites enabled.

Good point, but I feel that should be handled in a separate issue, if necessary.

If it will be addressed in the future issue, the current specification may need to weight the impact in. Otherwise, the specification added here might need to re-work to cover more cases.

Also, most of those other cases above (except for #4) are influenced by additional methods subsequently invoked by the application. For this one, it may be a bit surprising for the SSLContext to be returned when the protocol is disabled (although it is still compliant with the API as specified), and then later it doesn't work, so the implementation note is useful. The JCK team requested this clarification.

I understand the concerns. There are many cases in security components that an instance could return, but will not work in the following process if not set or configured properly. The impact of disabled properties is just one case of many. Maybe, a simple description is sufficient, "the instance may not work if not configured or set properly, for example ... ".

It may be not an option to stop at SSLContext.getInstance() if the protocol is disabled rather than delay to handshaking, as an application still can have the protocol back by overriding the default security properties.

BTW, the protocol for SSLContext.getInstance() method is not a TLS protocol. It is more of a context-algorithm. For example, SSLContext.getInstance("TLSv1.3") could support TLS 1.0/1.1/1.2/1.3. It may be not usual, but if TLS 1.3 is disabled, and TLS 1.2 not, the connection should be able to established with TLS 1.2. In the description, it would be nice to make it clean what the 'protocol" means in different circumstances.

@XueleiFan
Copy link
Member

It may be not an option to stop at SSLContext.getInstance() if the protocol is disabled rather than delay to handshaking, as an application still can have the protocol back by overriding the default security properties.

I may be wrong. The security property may be just loaded one time, and the follow-on update will not take effect. If it is the case, is it an option to stop at SSLContext.getInstance()?

@seanjmullan
Copy link
Member Author

It may be not an option to stop at SSLContext.getInstance() if the protocol is disabled rather than delay to handshaking, as an application still can have the protocol back by overriding the default security properties.

I may be wrong. The security property may be just loaded one time, and the follow-on update will not take effect. If it is the case, is it an option to stop at SSLContext.getInstance()?

It is not specified if the property is read only once or multiple times. However, the JDK implementation reads it once and also when it creates an SSLContext, so there is no chance to modify it later.

@seanjmullan
Copy link
Member Author

If it will be addressed in the future issue, the current specification may need to weight the impact in. Otherwise, the specification added here might need to re-work to cover more cases.

Which is fine. But we can still cover the behavior of jdk.tls.disabledAlgorithms in this one.

Also, most of those other cases above (except for #4) are influenced by additional methods subsequently invoked by the application. For this one, it may be a bit surprising for the SSLContext to be returned when the protocol is disabled (although it is still compliant with the API as specified), and then later it doesn't work, so the implementation note is useful. The JCK team requested this clarification.

I understand the concerns. There are many cases in security components that an instance could return, but will not work in the following process if not set or configured properly. The impact of disabled properties is just one case of many. Maybe, a simple description is sufficient, "the instance may not work if not configured or set properly, for example ... ".

But I don't think there is general confusion over those other scenarios, so I am wary about going down that path where we feel obligated to document all possible scenarios. People using TLS should already know that a connection may not succeed for various reasons and the SSLException should provide a good reason for that. However, this one is not very visible and could be somewhat surprising. An application may want to query the jdk.tls.disabledAlgorithms property and make sure the protocol is not disabled before creating the SSLContext. Although, in practice, we recommend calling SSLContext.getDefault.

It may be not an option to stop at SSLContext.getInstance() if the protocol is disabled rather than delay to handshaking, as an application still can have the protocol back by overriding the default security properties.

It won't work - see my other reply.

BTW, the protocol for SSLContext.getInstance() method is not a TLS protocol. It is more of a context-algorithm. For example, SSLContext.getInstance("TLSv1.3") could support TLS 1.0/1.1/1.2/1.3. It may be not usual, but if TLS 1.3 is disabled, and TLS 1.2 not, the connection should be able to established with TLS 1.2. In the description, it would be nice to make it clean what the 'protocol" means in different circumstances.

Yes, it might be useful to add something like "The returned SSLContext implements the specified protocol version, and may also implement other protocol versions." But I think it could be covered in a separate issue.

The wording in this PR specifically refers to the protocol version that was specified. It isn't covering other optional protocols that may be supported.

I would like to hear from others if they feel this note is useful, or if it is opening up more questions that not.

@XueleiFan
Copy link
Member

As you are already there, it may be nice to cover more options that the protocol may not be used for the handshaking. Here are some cases I'm aware of:

  1. the protocol is disabled with Security property
  2. the protocol is not set while setting the SSLParameters
  3. the protocol is not set while set the enabled protocol
  4. the protocol is not supported by the peer
  5. the protocol is not supported by any cipher suites enabled.

Good point, but I feel that should be handled in a separate issue, if necessary. Also, most of those other cases above (except for #4) are influenced by additional methods subsequently invoked by the application.

I think it twice. It may be not necessary to invoke additional methods subsequently by the application. The impact could be configurations (security properties, key store, etc) other than jdk.tls.disabledAlgorithms.

For this one, it may be a bit surprising for the SSLContext to be returned when the protocol is disabled (although it is still compliant with the API as specified), and then later it doesn't work, so the implementation note is useful.

If the purpose is to make sure an SSLContext instance work later, more options may be considered. IMO, I did not see the value if we cannot make the description in a general and accuracy way. The "jdk.tls.disabledAlgorithms" is just one of many impacts. All jdk.xxx.disabledAlgorithms properties could play a role to break the connection. It may be not enough to check the "jdk.tls.disabledAlgorithms" and than can make sure an SSLContext instance will work for sure.

@XueleiFan
Copy link
Member

BTW, the protocol for SSLContext.getInstance() method is not a TLS protocol. It is more of a context-algorithm. For example, SSLContext.getInstance("TLSv1.3") could support TLS 1.0/1.1/1.2/1.3. It may be not usual, but if TLS 1.3 is disabled, and TLS 1.2 not, the connection should be able to established with TLS 1.2. In the description, it would be nice to make it clean what the 'protocol" means in different circumstances.

Yes, it might be useful to add something like "The returned SSLContext implements the specified protocol version, and may also implement other protocol versions." But I think it could be covered in a separate issue.

I was not meant to add this kind of description. I meant that the "specified protocol" in the description is not a TLS protocol version, and hence it could not be referred as TLS protocol version.

The wording in this PR specifically refers to the protocol version that was specified. It isn't covering other optional protocols that may be supported.

I may be wrong. But let me check. For example, the code looks like:
SSLContext context = SSLContext.getInstance("TLSv1.3");

Per "The wording in this PR specifically refers to the protocol version that was specified", I guess "the protocol version that was specified" is "TLSv1.3". And for code like SSLContext context = SSLContext.getInstance("TLSv1.2"); I guess the "the protocol version that was specified" is "TLSv1.2".

If the property looks like jdk.tls.disabledAlgorithms=TLSv1.3, the "TLSv1.3" specified in the security property is not the one specified in SSLContext.getInstance("TLSv1.3");. They are two different concepts. The one in SSLContext.getInstance("TLSv1.3"); refers to SSLContext protocol, while the one in jdk.tls.disabledAlgorithms=TLSv1.3 refers to TLS versions.

However, in the following description:

     * ... However, if the specified provider is "SunJSSE",
     * subsequent operations that attempt to use the specified protocol will
     * fail with an {@code SSLHandshakeException}."

if "the specified protocol" is referring to TLS version, it is not correct.

If I get it right, the context in the example above can be used to establish connections for TLS 1.0/1.1/1.2. The description in the PR , "will fail with ...", may be not true.

@mlbridge
Copy link

mlbridge bot commented Nov 16, 2022

Mailing list message from Xuelei Fan on security-dev:

The wording in this PR specifically refers to the protocol version that

was specified. It isn't covering other optional protocols that may be
supported.

Sorry, I may not make it clear. The protocol specified in
SSLContext.getInstance is not TLS protocol version. I think the protocol
disabled in security properties refers to protocol version. The new added
specification in this PR would better avoid the confusing. If you want the
specified protocol referring to TLS version, the handshake may still
success even the TLS version is disabled. If you means the specified
protocol as SSLContext algorithm, I am not sure if the current security
properties supporting disabling of SSLContext algorithm.

Hope it helps!

Xuelei
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20221115/6698b475/attachment-0001.htm>

@seanjmullan
Copy link
Member Author

Mailing list message from Xuelei Fan on security-dev:

The wording in this PR specifically refers to the protocol version that

was specified. It isn't covering other optional protocols that may be supported.

Sorry, I may not make it clear. The protocol specified in SSLContext.getInstance is not TLS protocol version. I think the protocol disabled in security properties refers to protocol version.

Where in the javadoc APIs does it say that? I think the only assumption you can make is that the SSLContext that is returned supports the protocol version that was specified. Whether or not it supports other versions is completely implementation-specific AFAICT.

@XueleiFan
Copy link
Member

Mailing list message from Xuelei Fan on security-dev:

The wording in this PR specifically refers to the protocol version that

was specified. It isn't covering other optional protocols that may be supported.
Sorry, I may not make it clear. The protocol specified in SSLContext.getInstance is not TLS protocol version. I think the protocol disabled in security properties refers to protocol version.

Where in the javadoc APIs does it say that?

The Javadoc APIs specification does not say it is referring to TLS protocol version. In the standard algorithm documentation, the word "SSLContext Algorithms" is used and here is an example:

Algorithm Name Description
TLSv1.2 Supports RFC 5246: TLS version 1.2; may support other SSL/TLS versions

I think the only assumption you can make is that the SSLContext that is returned supports the protocol version that was specified. Whether or not it supports other versions is completely implementation-specific AFAICT.

Yes. So we cannot assume that the literal SSLContext algorithm is the only supported TLS version for an JSSE provider, including the SunJSSE provider.

@seanjmullan
Copy link
Member Author

I plan to withdraw this PR for now. @XueleiFan you make some good points that the implementation note is too specific (there are other failure scenarios) and also that a connection may still be successfully established using other protocols if the implementation supports other protocols than what was requested.

I really think that what is needed is a better class description in the javadoc of the SSLContext class about what the protocol passed to getInstance means. Something similar to what is documented in the JSSE Developer's Guide: https://docs.oracle.com/en/java/javase/19/security/java-secure-socket-extension-jsse-reference-guide.html#GUID-9BAC1902-A203-4422-8163-61D64ADD2FF7
I will open an RFE for that.

After that we can add more information on specific implementation specific scenarios, such as what is being proposed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net net-dev@openjdk.org ready Pull request is ready to be integrated rfr Pull request is ready for review security security-dev@openjdk.org
3 participants