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.

Sign up for GitHub

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

Jump to bottom

HTTP client does not reject untrusted certificates unless trust store is configured #4116

Closed
myles-anderson-ox opened this issue Sep 16, 2020 · 1 comment
Closed

HTTP client does not reject untrusted certificates unless trust store is configured #4116

myles-anderson-ox opened this issue Sep 16, 2020 · 1 comment
Assignees
@yawkat
Labels
type: improvement A minor improvement to an existing feature
Milestone
3.2.0

Comments

@myles-anderson-ox
Copy link

Steps to Reproduce

  1. Create a Micronaut application with feature http-client
  2. Create a controller
  3. Configure the server to enable ssl and use a self signed certificate
  4. Create a test that calls the controller with an http client that points to the controller's https endpoint

Expected Behaviour

An SSLException is thrown due to an unknown certificate

Actual Behaviour

The client accepts the certificate and gets a valid response

Environment Information

  • Operating System: macOS
  • Micronaut Version: tested with 1.3.5 and 2.0.2
  • JDK Version: 8

Example Application
@graemerocher graemerocher added the status: awaiting validation Waiting to be validated as a real issue label Nov 21, 2021
@yawkat
Copy link
Member

yawkat commented Nov 22, 2021

Code in question:

micronaut-core/http-client/src/main/java/io/micronaut/http/client/netty/ssl/NettyClientSslBuilder.java

Line 132 in a6c0bae

return InsecureTrustManagerFactory.INSTANCE;

While it seems sensible not to use the InsecureTrustManagerFactory by default (it even has a warning in its javadoc: "Never use this TrustManagerFactory in production. It is purely for testing purposes, and thus it is very insecure."), we can't really change this now. Maybe for 4.0?

@yawkat yawkat added status: next major version The issue will be considered for the next major version type: improvement A minor improvement to an existing feature and removed status: awaiting validation Waiting to be validated as a real issue labels Nov 22, 2021
yawkat added a commit that referenced this issue Nov 23, 2021
@yawkat
Enable client SSL certificate checking by default
28f37bb 
This patch enables SSL cert checking by default. The old behavior without checking can be enabled using the `insecureTrustAllCertificates` option.
Fixes #4116
@yawkat yawkat mentioned this issue Nov 23, 2021
Merged
@jameskleeh jameskleeh removed the status: next major version The issue will be considered for the next major version label Nov 23, 2021
@jameskleeh jameskleeh added this to the 3.2.0 milestone Nov 23, 2021
jameskleeh added a commit that referenced this issue Nov 23, 2021
@yawkat @jameskleeh
Enable client SSL certificate checking by default (#6562)
6def4e8 
* Enable client SSL certificate checking by default
This patch enables SSL cert checking by default. The old behavior without checking can be enabled using the `insecureTrustAllCertificates` option.
Fixes #4116

* note on trust store config

* use netty TMF setup

* fix some tests, move config

* more test fixes

* more test fixes

* breaks.adoc

* spotless

* Use dash case

Co-authored-by: jameskleeh <[email protected]>
@yawkat yawkat closed this as completed Dec 2, 2021
yawkat added a commit that referenced this issue Mar 15, 2023
@yawkat
More mTLS test cases
4c02eb0 
This PR adds more test cases for the server side of mTLS. These came from an internal user that reported expired certs being accepted.

The test cases check a normal cert, an expired cert, and an untrusted cert. The previous RequestCertificateSpec only tests the "happy path" with the valid cert. These tests will prevent issues similar to #4116.

It turns out that the behavior for expired certs is correct. When a cert is directly added to the trust store (not just its CA), the JDK does not check expiry. I think we should match that behavior.

Also contains a small change to SelfSignedSslBuilder to make it actually use the configured trust store. This has no security implications, it just makes the tests work.
@yawkat yawkat mentioned this issue Mar 15, 2023
Merged
sdelamo pushed a commit that referenced this issue Mar 15, 2023
@yawkat
test: More mTLS test cases (#8944)
a18658a 
This PR adds more test cases for the server side of mTLS. These came from an internal user that reported expired certs being accepted.

The test cases check a normal cert, an expired cert, and an untrusted cert. The previous RequestCertificateSpec only tests the "happy path" with the valid cert. These tests will prevent issues similar to #4116.

It turns out that the behavior for expired certs is correct. When a cert is directly added to the trust store (not just its CA), the JDK does not check expiry. I think we should match that behavior.

Also contains a small change to SelfSignedSslBuilder to make it actually use the configured trust store. This has no security implications, it just makes the tests work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@yawkat yawkat

Labels
type: improvement A minor improvement to an existing feature
Projects
None yet
Milestone
3.2.0
Development

No branches or pull requests
4 participants
@graemerocher @jameskleeh @yawkat @myles-anderson-ox