Allow `useOpenSsl` -> `tlsEngineType` to be configured by `ClientFactoryBuilder` #4962

seonWKim · 2023-06-15T16:26:09Z

Motivation:

Let users configure tlsEngineType client option when using ClientFactoryBuilder or ServerBuilder

Example
Client

ClientFactory
  .builder()
  .tlsEngineType(TlsEngineType.OPENSSL)
  .build();

Server

Server.builder()
           .tlsSelfSigned()
           .service("/example", (ctx, req) -> HttpResponse.of(HttpStatus.OK))
           .tlsEngineType(TlsEngineType.JDK)
           .virtualHost("*.example1.com")
           .service("/example", (ctx, req) -> HttpResponse.of(HttpStatus.OK))
           .tlsSelfSigned()
           .tlsEngineType(TlsEngineType.OPENSSL)
           .and()
           .virtualHost("*.example2.com")
           .service("/example", (ctx, req) -> HttpResponse.of(HttpStatus.OK))
           .tlsSelfSigned()
           .tlsEngineType(TlsEngineType.JDK)
           .and()
           .virtualHost("*.example3.com")
           .service("/example", (ctx, req) -> HttpResponse.of(HttpStatus.OK))
           .and()
           .build();

Modifications:

Add tlsEngineType method in ClientFactoryBuilder to allow users choose whether to use openSsl.

Result:

Closes #<4949>. (If this resolves the issue.)
Users will be able to configure tlsEngineType using ClientFactoryBuilder or ServerBuilder

jrhee17

Looks good! Left some minor comments on testing 🙇

jrhee17 · 2023-06-16T06:59:12Z

core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java

+     * Allow the use of JNI-based TLS support with OpenSSL.
+     * @param useOpenSsl Whether to allow the use of JNI-based TLS support with OpenSSL
+     */
+    public ClientFactoryBuilder useOpenSsl(boolean useOpenSsl) {


Note that currently, we already have the ability to set this per server/clientFactory via tlsCustomizer

I have no problem with adding this API for now, but I slightly worry that this API will become obsolete if a different SslProvider is added in the future.
What do you think other maintainers?

Good point. It is a possible situation but we may need to provide users a workaround to selectively use JDK SSL over Open SSL.

When SslProvider is made, I think, we have them mutually exclusive.

What do you think about adding a new enum first, then? @seonwoo960000, how about creating a new PR that deprecates Flags.useOpenSsl() and adding Flags.tlsEngineType() and enum TlsEngineType { JDK, OPENSSL }? You might want to check Flags.useEpoll() and Flags.transportType()?

@trustin thank you for the detailed comment. So by adding a enum TlsEngineType, armeria should be able to allow users select tls engine not only JDK and OPENSSL but also other engines which might be added in the future. Did I understand correctly 🤔 ?

I believe you understand it correctly. If a new TLS engine is added, Flags.useOpenSsl() == false could be vague.

jrhee17 · 2023-06-16T07:00:56Z

core/src/test/java/com/linecorp/armeria/client/HttpClientFactoryTest.java

+            final AggregatedHttpResponse response3 = WebClient.builder(SessionProtocol.HTTP,
+                                                                       server.httpEndpoint())
+                                                              .factory(sslDisabledClient).build()
+                                                              .get("/hello").aggregate().join();
+            assertThat(response3.status()).isEqualTo(HttpStatus.OK);
+            assertThat(response3.contentUtf8()).isEqualTo("hello");
+
+            assertThatThrownBy(() -> WebClient.builder(SessionProtocol.HTTPS, server.httpsEndpoint())
+                                              .factory(sslDisabledClient).build()
+                                              .get("/hello").aggregate().join())
+                    .isInstanceOfSatisfying(CompletionException.class, e -> {
+                        assertThat(e).hasCauseInstanceOf(UnprocessedRequestException.class);
+                    });


I'm not sure what these lines are testing 😅 Can we just test once with tlsVerify disabled for each ClientFactory?

ClientFactory sslDisabledClient = ClientFactory.builder().useOpenSsl(false).tlsNoVerify().build(); ... final AggregatedHttpResponse response3 = WebClient.builder(SessionProtocol.HTTPS, server.httpsEndpoint()) .factory(sslDisabledClient).build() .get("/hello").aggregate().join(); assertThat(response3.status()).isEqualTo(HttpStatus.OK); assertThat(response3.contentUtf8()).isEqualTo("hello");

Please correct me if my understanding is wrong.

If client doesn't use openSsl, request sent using https protocol will fail(because https require ssl/tls)

The purpose of useOpenSsl() was to test above scenario🤔

If OpenSSL is disabled, the JDK SSL engine is used instead.

Oh, then it seems that above mentioned scenario isn't valid. I'll remove the test 👍

core/src/test/java/com/linecorp/armeria/client/HttpClientFactoryTest.java

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

Motivation: Add `TlsEngineType` to let users choose the type of tls engine. (Using `Flags`) Modifications: - Add `TlsEngineType` enum - Deprecate `useOpenSsl` Result: - Related to #<[4949](#4949)>. - Related to #4962 (comment) - `TlsEngineType` is added - `useOpenSsl` is deprecated  --------- Co-authored-by: Ikhun Um <ih.pert@gmail.com> Co-authored-by: Ikhun Um <ikhun.um@linecorp.com> Co-authored-by: jrhee17 <guins_j@guins.org> Co-authored-by: minwoox <songmw725@gmail.com> Co-authored-by: minux <minu.song@linecorp.com>

seonWKim · 2024-04-09T00:03:58Z

I'll start working on this issue again 🏃

github-actions · 2024-05-19T04:05:32Z

🔍 Build Scan® (commit: `e6d3fcb`)

Job name	Status	Build Scan®
build-windows-latest-jdk-21	✅	https://ge.armeria.dev/s/2cxjy6rlznbtq
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/dteuqgveic74c
build-self-hosted-unsafe-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/pb4rip5rznbxo
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/tjewlmyhjprzs
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/gmihrjmb4rn6u
build-self-hosted-unsafe-jdk-17-leak	✅	https://ge.armeria.dev/s/x6qlgdfj4xgfe
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/jgahrj5conkwy
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/ceumqamekkz2s

… `useOpenSsl`

seonWKim · 2024-05-19T06:49:18Z

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

@@ -94,13 +98,26 @@ static SSLSession validateSslContext(SslContext sslContext) {
        return sslSession;
    }

+    private static TlsEngineType determineTlsEngineType(SslContext sslContext) {


Determining which TlsEngineType should be matched is based on io.netty.handler.ssl.SslContext#newServerContextInternal.

switch (provider) { case JDK: if (enableOcsp) { throw new IllegalArgumentException("OCSP is not supported with this SslProvider: " + provider); } return new JdkSslServerContext(sslContextProvider, trustCertCollection, trustManagerFactory, keyCertChain, key, keyPassword, keyManagerFactory, ciphers, cipherFilter, apn, sessionCacheSize, sessionTimeout, clientAuth, protocols, startTls, keyStoreType); case OPENSSL: verifyNullSslContextProvider(provider, sslContextProvider); return new OpenSslServerContext( trustCertCollection, trustManagerFactory, keyCertChain, key, keyPassword, keyManagerFactory, ciphers, cipherFilter, apn, sessionCacheSize, sessionTimeout, clientAuth, protocols, startTls, enableOcsp, keyStoreType, ctxOptions); case OPENSSL_REFCNT: verifyNullSslContextProvider(provider, sslContextProvider); return new ReferenceCountedOpenSslServerContext( trustCertCollection, trustManagerFactory, keyCertChain, key, keyPassword, keyManagerFactory, ciphers, cipherFilter, apn, sessionCacheSize, sessionTimeout, clientAuth, protocols, startTls, enableOcsp, keyStoreType, ctxOptions); default: throw new Error(provider.toString()); }

seonWKim · 2024-05-19T06:53:12Z

I've not added useOpenSsl because tlsEngineType should be a better alternative to useOpenSsl.

Please let me know if we should add support for both tlsEngineType and useOpenSsl in the ClientFactoryBuilder.

codecov · 2024-05-19T07:42:51Z

Codecov Report

Attention: Patch coverage is 83.33333% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 74.10%. Comparing base (14c5566) to head (6438c82).
Report is 14 commits behind head on main.

❗ Current head 6438c82 differs from pull request most recent head e6d3fcb

Please upload reports for the commit e6d3fcb to get more accurate results.

Files	Patch %	Lines
.../linecorp/armeria/client/ClientFactoryBuilder.java	0.00%	2 Missing ⚠️
.../linecorp/armeria/server/ServerSslContextUtil.java	75.00%	1 Missing and 1 partial ⚠️
...p/armeria/internal/common/util/SslContextUtil.java	50.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #4962      +/-   ##
============================================
+ Coverage     74.05%   74.10%   +0.04%     
- Complexity    21253    21313      +60     
============================================
  Files          1850     1853       +3     
  Lines         78600    78775     +175     
  Branches      10032    10067      +35     
============================================
+ Hits          58209    58376     +167     
+ Misses        15686    15682       -4     
- Partials       4705     4717      +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

minwoox

Just left nits. Thanks! 👍

core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java

core/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.java

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

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

- Add @UnstableApi - set `Flags.tlsEngineType()` as default virtualHostTemplate's tlsEngineType

seonWKim marked this pull request as ready for review June 15, 2023 16:28

seonWKim requested review from ikhoon, jrhee17, minwoox and trustin as code owners June 15, 2023 16:28

jrhee17 reviewed Jun 16, 2023

View reviewed changes

ikhoon reviewed Jun 16, 2023

View reviewed changes

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

ikhoon added the new feature label Jun 16, 2023

seonWKim force-pushed the enable-disable-openssl-4949 branch 3 times, most recently from b396271 to d378831 Compare June 23, 2023 16:59

seonWKim changed the title ~~Allow useOpenSsl to be configured by ClientFactoryBuilder~~ Allow useOpenSsl to be configured by ClientFactoryBuilder Jun 30, 2023

seonWKim force-pushed the enable-disable-openssl-4949 branch from d378831 to 5b6a2d1 Compare July 2, 2023 09:21

This was referenced Jul 12, 2023

Add TlsEngineType enum #5028

Closed

Add TlsEngineType enum #5029

Merged

github-actions bot added the Stale label May 16, 2024

Add support for TlsEngineType for ClientFactoryBuilder instead of…

1800360

… `useOpenSsl`

seonWKim force-pushed the enable-disable-openssl-4949 branch from c2a3d34 to 1800360 Compare May 19, 2024 06:35

seonWKim changed the title ~~Allow useOpenSsl to be configured by ClientFactoryBuilder~~ Allow useOpenSsl -> tlsEngineType to be configured by ClientFactoryBuilder May 19, 2024

seonWKim commented May 19, 2024

View reviewed changes

Nit

8b1ddf0

seonWKim force-pushed the enable-disable-openssl-4949 branch from efc86ba to 8b1ddf0 Compare May 19, 2024 06:51

Add more test cases

6438c82

minwoox approved these changes May 20, 2024

View reviewed changes

Apply comments

18d7325

- Add @UnstableApi - set `Flags.tlsEngineType()` as default virtualHostTemplate's tlsEngineType

github-actions bot removed the Stale label May 24, 2024

Merge branch 'main' into enable-disable-openssl-4949

e6d3fcb

minwoox added this to the 1.29.0 milestone Jun 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow `useOpenSsl` -> `tlsEngineType` to be configured by `ClientFactoryBuilder` #4962

Allow `useOpenSsl` -> `tlsEngineType` to be configured by `ClientFactoryBuilder` #4962

seonWKim commented Jun 15, 2023 •

edited

jrhee17 left a comment

jrhee17 Jun 16, 2023

ikhoon Jun 16, 2023

trustin Jul 5, 2023

seonWKim Jul 5, 2023

ikhoon Jul 12, 2023

jrhee17 Jun 16, 2023

seonWKim Jun 16, 2023 •

edited

ikhoon Jun 16, 2023

seonWKim Jun 16, 2023

seonWKim commented Apr 9, 2024

github-actions bot commented May 19, 2024 •

edited

seonWKim May 19, 2024 •

edited

seonWKim commented May 19, 2024 •

edited

codecov bot commented May 19, 2024 •

edited

minwoox left a comment

Allow useOpenSsl -> tlsEngineType to be configured by ClientFactoryBuilder #4962

Are you sure you want to change the base?

Allow useOpenSsl -> tlsEngineType to be configured by ClientFactoryBuilder #4962

Conversation

seonWKim commented Jun 15, 2023 • edited

Motivation:

Modifications:

Result:

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seonWKim Jun 16, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seonWKim commented Apr 9, 2024

github-actions bot commented May 19, 2024 • edited

🔍 Build Scan® (commit: e6d3fcb)

seonWKim May 19, 2024 • edited

Choose a reason for hiding this comment

seonWKim commented May 19, 2024 • edited

codecov bot commented May 19, 2024 • edited

Codecov Report

minwoox left a comment

Choose a reason for hiding this comment

Allow `useOpenSsl` -> `tlsEngineType` to be configured by `ClientFactoryBuilder` #4962

Allow `useOpenSsl` -> `tlsEngineType` to be configured by `ClientFactoryBuilder` #4962

seonWKim commented Jun 15, 2023 •

edited

seonWKim Jun 16, 2023 •

edited

github-actions bot commented May 19, 2024 •

edited

🔍 Build Scan® (commit: `e6d3fcb`)

seonWKim May 19, 2024 •

edited

seonWKim commented May 19, 2024 •

edited

codecov bot commented May 19, 2024 •

edited