Validate `SslContext` thoroughly #2124

jyblue · 2019-09-28T17:51:20Z

Motivation:

In some case, a server can start with a misconfigured SslContext and
fail when the serving its first request. If a server refuses to start
in such a case, users could fix their configuration problem at an earlier
stage with much less confusion.

Modifications:

Move VirtualHost.validateSslContext() to VirtualHostBuilder
Make an actual SslEngine with the given SslContext and perform
an initial handshake to trigger most configuration issues.
Add throws SSLException to tls() builder methods.
Remove the deprecated sslContext() methods.

Result:

Fixes Cryptic error message when TLS key store password is incorrect. #1844
(Breaking) tls() now throws a checked SSLException.
(Breaking) sslContext() methods, previously deprecated, have been
removed.

Motivation: In some case, servers can start with miss-configured `SslContext` and fail when users request ssl/tls connections. If servers refuse to start in that case, users can fix their configuration before severs start. Modifications: Add `VirtualHostBuilder.validateSslContext()` Result: Fixes line#1844

jyblue · 2019-09-28T18:45:05Z

Hello, here is my draft.
And I'm not sure I'm on the right way to go. Please see my questions.

If SSLEngine is created without exceptions, is it guaranteed that SslContext is well-configured?
If not guaranteed, what configurations do I have to validate manually?
I added some test cases and it seems that most of configurations are already validated by other classes before validateSslContext() method is hit.
I cannot reproduce initial bug report in my test code. Because when I try to load KeyStore without proper key, it throws a exception, which means the server fails to start. How do I create KeyStore with invalid key?

Thank you in advance 🙇‍♂️

jyblue · 2019-09-28T18:48:55Z

One more question,
I added dummy JKS file to resources directory for test purpose and source code contains its password.
Is it okay with any code security issue?

trustin · 2019-09-29T03:31:39Z

@jyblue wrote:

If SSLEngine is created without exceptions, is it guaranteed that SslContext is well-configured?

I think so. If not, we can add more checks later.

I cannot reproduce initial bug report in my test code. Because when I try to load KeyStore without proper key, it throws a exception, which means the server fails to start. How do I create KeyStore with invalid key?

You have to try to reproduce the problem with 1) a PEM format private key and 2) OpenSSL enabled (which is enabled by default, but you'll have to check using Flags.useOpenSsl()).

I added dummy JKS file to resources directory for test purpose and source code contains its password. Is it okay with any code security issue?

I don't think it has any security implication if you used a self-signed certificate and if it's not encrypted with your personal password. 😄

anuraaga · 2019-09-29T07:06:23Z

We have validation of created contexts here

https://github.com/line/armeria/blob/master/core/src/main/java/com/linecorp/armeria/internal/SslContextUtil.java

I think the new checks here look the same as what we do so maybe they don't help with the issue. I guess we need to reproduce the error case we first to know for sure.

jyblue · 2019-09-29T12:01:01Z

@anuraaga Thanks for good hint.

I think I found the reason.
In this case, null was given to KeyStore.load() as a password parameter and KeyStore.load() didn't perform password integrity check.

armeria/spring/boot-autoconfigure/src/main/java/com/linecorp/armeria/internal/spring/ArmeriaConfigurationUtil.java

Line 500 in 45c1633

store.load(url.openStream(), password != null ? password.toCharArray()

https://docs.oracle.com/javase/7/docs/api/java/security/KeyStore.html
A password may be given to unlock the keystore , or to check the integrity of the keystore data. If a password is not given for integrity checking, then integrity checking is not performed.

If steps reproduced,

KeyStore.load(keyStore, null);  // not performed password check
...
// server starts
// wait for ssl/tls request
...
KeyStore.getEntry(password); // password check, exception occurs and server fails

I will write a test case to reproduce it.

trustin · 2019-09-30T03:02:18Z

Thanks @jyblue! Looks like we are going into the right direction. Please keep us posted.

codecov · 2019-09-30T22:52:58Z

Codecov Report

Merging #2124 into master will decrease coverage by 0.02%.
The diff coverage is 91.66%.

@@             Coverage Diff              @@
##             master    #2124      +/-   ##
============================================
- Coverage     73.59%   73.56%   -0.03%     
+ Complexity     9571     9567       -4     
============================================
  Files           837      837              
  Lines         36836    36863      +27     
  Branches       4543     4547       +4     
============================================
+ Hits          27108    27119      +11     
- Misses         7402     7421      +19     
+ Partials       2326     2323       -3

Impacted Files	Coverage Δ	Complexity Δ
...ava/com/linecorp/armeria/server/ServerBuilder.java	`74.53% <ø> (+1.97%)`	`104 <0> (+1)`	⬆️
.../java/com/linecorp/armeria/server/VirtualHost.java	`56% <100%> (+0.53%)`	`32 <0> (-2)`	⬇️
...om/linecorp/armeria/server/VirtualHostBuilder.java	`61.15% <91.3%> (+4.51%)`	`64 <2> (+2)`	⬆️
...rp/armeria/server/tomcat/ManagedTomcatService.java	`50.84% <0%> (-22.04%)`	`6% <0%> (-1%)`
...armeria/server/tomcat/Tomcat90ProtocolHandler.java	`48.14% <0%> (-14.82%)`	`11% <0%> (-4%)`
...corp/armeria/client/DefaultEventLoopScheduler.java	`79.27% <0%> (-9.91%)`	`30% <0%> (-5%)`
...inecorp/armeria/client/AbstractEventLoopState.java	`92.3% <0%> (-7.7%)`	`6% <0%> (-1%)`
...com/linecorp/armeria/client/OneEventLoopState.java	`66.66% <0%> (-4.77%)`	`6% <0%> (-1%)`
...om/linecorp/armeria/server/jetty/JettyService.java	`63.05% <0%> (-2.96%)`	`23% <0%> (-1%)`
.../linecorp/armeria/server/tomcat/TomcatService.java	`63.82% <0%> (-2.66%)`	`24% <0%> (-1%)`
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 326877a...09c488f. Read the comment docs.

jyblue · 2019-10-02T19:57:00Z

Hello, here is my research and code updated. PTAL 😀

Test conclusion
If key store password is not given to PKCS12 key store,
a server is built, running without exceptions, but fails when ssl/tls request come.

Internal cause
When netty.SslHandler is trying to decode client's first packet, a exception occurs.
I think SslHandler doesn't have a valid access to key store (not for sure).
https://github.com/netty/netty/blob/d8b1a2d93f556a08270e6549bf7f91b3b09f24bb/handler/src/main/java/io/netty/handler/ssl/SslHandler.java#L1329

Validation
I added validation method which tries dummy ssl/tls request,
and it produces same exception as original error.

failed to validate SSL/TLS configuration : javax.net.ssl.SSLHandshakeException: error:100000ae:SSL routines:OPENSSL_internal:NO_CERTIFICATE_SET
java.lang.RuntimeException: failed to validate SSL/TLS configuration : javax.net.ssl.SSLHandshakeException: error:100000ae:SSL routines:OPENSSL_internal:NO_CERTIFICATE_SET
	at com.linecorp.armeria.server.VirtualHostBuilder.build(VirtualHostBuilder.java:843)
	at com.linecorp.armeria.server.ServerBuilder.build(ServerBuilder.java:1432)

Note
JKS key store doesn't produce any exception when key store password is not given.

FYI
Bug reproduction and detail report
https://github.com/jyblue/ArmeriaIssueReproduction1844

trustin · 2019-10-03T06:46:27Z

Please remove the WIP status if it's ready for reviews. 😉

core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java

core/src/test/java/com/linecorp/armeria/server/ServerTlsTest.java

trustin · 2019-10-04T03:19:16Z

You also have to release the two SSLEngines you created during validation:

try {
    ...
} finally {
    ReferenceCountUtil.release(serverEngine);
    ReferenceCountUtil.release(clientEngine);
}

…lidation

jyblue · 2019-10-05T15:36:36Z

Here is my update.

Rename validateSslContext to validateKeyStoreNullPassword
Because it is confused with VirtualHost.validateSslContext
Make validateKeyStoreNullPassword to be called inside of tls(KeyManagerFactory, TlsCoustomizer)
Making the validation more close to user-side, so users can easily find their miss configured key store password

jyblue · 2019-10-05T15:41:14Z

It seems @Ignore annotation on testJksKeyStoreWithNullPassword() doesn't work.
I will take a look and fix it.

core/src/test/java/com/linecorp/armeria/server/ServerTlsValidationTest.java

`VirtualHost.validateSslContext()` * Remove the `validateSslContext()` call from VirtualHost's constructor. * Add `SSLException` exception to method signature `ServerBuilder.tls(SslContext)` `VirtualHostBuilder.tls(SslContext)` * Update javadoc on `testJksKeyStoreWithNullPassword()` test case * Remove deprecated method `ServerBuilder.sslContext(sslContext)` `ServerBuilder.sslContext(protocol, keyCertChainFile, keyFile)` `ServerBuilder.sslContext(protocol, keyCertChainFile, keyFile, keyPassword)` `VirtualHostBuilder.sslContext(sslContext)` `VirtualHostBuilder.sslContext(protocol, keyCertChainFile, keyFile)` `VirtualHostBuilder.sslContext(protocol, keyCertChainFile, keyFile, keyPassword)`

jyblue · 2019-10-07T14:26:29Z

Thank you for review, again. Here is my update.

I fixed items from previous review
and removed 6 deprecated method (had a chat with @trustin for this)
Please see below removed method list.

ServerBuilder.sslContext(sslContext)
ServerBuilder.sslContext(protocol, keyCertChainFile, keyFile)
ServerBuilder.sslContext(protocol, keyCertChainFile, keyFile, keyPassword)
VirtualHostBuilder.sslContext(sslContext)
VirtualHostBuilder.sslContext(protocol, keyCertChainFile, keyFile)
VirtualHostBuilder.sslContext(protocol, keyCertChainFile, keyFile, keyPassword)

minwoox

Thanks a lot for your work! 🙇

core/src/main/java/com/linecorp/armeria/server/VirtualHost.java

trustin

Just one nit. Nice work, @jyblue! 👍

core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java

ikhoon

Thanks, Great job!

core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java

minwoox

Thanks a lot, @jyblue!

trustin · 2019-10-08T13:53:52Z

Nice work, @jyblue! I've updated the PR description and commit message for you. 😉

Motivation: In some case, a server can start with a misconfigured `SslContext` and fail when the serving its first request. If a server refuses to start in such a case, users could fix their configuration problem at an earlier stage with much less confusion. Modifications: - Move `VirtualHost.validateSslContext()` to `VirtualHostBuilder` - Make an actual `SslEngine` with the given `SslContext` and perform an initial handshake to trigger most configuration issues. - Add `throws SSLException` to `tls()` builder methods. - Remove the deprecated `sslContext()` methods. Result: - Fixes line#1844 - (Breaking) `tls()` now throws a checked `SSLException`. - (Breaking) `sslContext()` methods, previously deprecated, have been removed.

trustin added the defect label Sep 29, 2019

jyblue added 2 commits October 1, 2019 07:49

Add more test cases

0962532

Add more test cases

1d62ffe

Modify SslContext Validation

4d56cea

jyblue marked this pull request as ready for review October 3, 2019 10:47

jyblue requested review from ikhoon, minwoox and trustin as code owners October 3, 2019 10:47

jyblue changed the title ~~[WIP] Add SslContext validation~~ Add SslContext validation Oct 3, 2019

trustin requested changes Oct 4, 2019

View reviewed changes

jyblue added 6 commits October 4, 2019 23:09

Update validation method and test case

043a6d8

Merge remote-tracking branch 'upstream/master' into add_sslcontext_va…

90965c2

…lidation

Update test case

5257c56

Fix line break

25c861d

Merge remote-tracking branch 'upstream/master' into add_sslcontext_va…

d941479

…lidation

Move validation method

df89f90

Fix Ignore annotation on test case

d9dec0b

ikhoon reviewed Oct 7, 2019

View reviewed changes

core/src/test/java/com/linecorp/armeria/server/ServerTlsValidationTest.java Show resolved Hide resolved

jyblue added 2 commits October 7, 2019 20:14

Reduce test area to narrow down target location

7cbba79

minwoox reviewed Oct 8, 2019

View reviewed changes

core/src/main/java/com/linecorp/armeria/server/VirtualHost.java Outdated Show resolved Hide resolved

core/src/main/java/com/linecorp/armeria/server/VirtualHost.java Outdated Show resolved Hide resolved

ikhoon reviewed Oct 8, 2019

View reviewed changes

core/src/main/java/com/linecorp/armeria/server/VirtualHost.java Outdated Show resolved Hide resolved

ikhoon added the breaking change label Oct 8, 2019

ikhoon added this to the 0.94.1 milestone Oct 8, 2019

jyblue added 7 commits October 8, 2019 11:58

Modify exception message

eb573a1

Move validateSslContext() to VirtualHostBuilder

024790c

Organize method order

016875e

Make SslContext parameter non-null

f34aa41

Add comments on validateSslContext()

99b0ce7

Make buildSslContext() private

7c5e7e4

Remove redundant beginHandshake() in validateSslContext()

6e8df28

trustin approved these changes Oct 8, 2019

View reviewed changes

core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java Outdated Show resolved Hide resolved

Add exception cause to SSLException in validateSslContext()

7c34807

ikhoon approved these changes Oct 8, 2019

View reviewed changes

core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java Outdated Show resolved Hide resolved

jyblue added 3 commits October 8, 2019 20:32

Update javadoc

58ef5ae

Fix typo

6df9164

Fix typo

4aadaa2

trustin reviewed Oct 8, 2019

View reviewed changes

core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java Outdated Show resolved Hide resolved

Fix typo

09c488f

minwoox approved these changes Oct 8, 2019

View reviewed changes

trustin merged commit 26c8fae into line:master Oct 8, 2019

trustin changed the title ~~Add SslContext validation~~ Validate SslContext thoroughly Oct 8, 2019

jyblue deleted the add_sslcontext_validation branch October 8, 2019 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate `SslContext` thoroughly #2124

Validate `SslContext` thoroughly #2124

jyblue commented Sep 28, 2019 •

edited by trustin

jyblue commented Sep 28, 2019 •

edited

jyblue commented Sep 28, 2019 •

edited

trustin commented Sep 29, 2019

anuraaga commented Sep 29, 2019

jyblue commented Sep 29, 2019 •

edited

trustin commented Sep 30, 2019

codecov bot commented Sep 30, 2019 •

edited

jyblue commented Oct 2, 2019

trustin commented Oct 3, 2019

trustin commented Oct 4, 2019

jyblue commented Oct 5, 2019

jyblue commented Oct 5, 2019

jyblue commented Oct 7, 2019

minwoox left a comment

trustin left a comment

ikhoon left a comment

minwoox left a comment

trustin commented Oct 8, 2019

Validate SslContext thoroughly #2124

Validate SslContext thoroughly #2124

Conversation

jyblue commented Sep 28, 2019 • edited by trustin

jyblue commented Sep 28, 2019 • edited

jyblue commented Sep 28, 2019 • edited

trustin commented Sep 29, 2019

anuraaga commented Sep 29, 2019

jyblue commented Sep 29, 2019 • edited

trustin commented Sep 30, 2019

codecov bot commented Sep 30, 2019 • edited

Codecov Report

jyblue commented Oct 2, 2019

trustin commented Oct 3, 2019

trustin commented Oct 4, 2019

jyblue commented Oct 5, 2019

jyblue commented Oct 5, 2019

jyblue commented Oct 7, 2019

minwoox left a comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

trustin commented Oct 8, 2019

Validate `SslContext` thoroughly #2124

Validate `SslContext` thoroughly #2124

jyblue commented Sep 28, 2019 •

edited by trustin

jyblue commented Sep 28, 2019 •

edited

jyblue commented Sep 28, 2019 •

edited

jyblue commented Sep 29, 2019 •

edited

codecov bot commented Sep 30, 2019 •

edited