quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.82k stars 2.69k forks source link

OIDC client should support TLSv1.3 #29635

Closed andrm closed 1 year ago

andrm commented 1 year ago

Description

It seems to me that the oidc client (just configured with https://mykeycloak-com/realms/myrealm ) will connect only with TLSv1.2. If the keycloak server (or the reverse proxy in front of keycloak) will be configured with a modern config, it will fail with "protocol version" TLS error.

Implementation ideas

Make TLSv1.3 configurable in the http client options.

quarkus-bot[bot] commented 1 year ago

/cc @pedroigor, @sberyozkin

sberyozkin commented 1 year ago

@cescoffier Hi Clement, how does Vert.x client handle it, it should auto-upgrade to TLS v1.3, right ?

cescoffier commented 1 year ago

Yes, but better ask @pmlopes

pmlopes commented 1 year ago

If I'm not mistaken all you need is to ensure it is supported by the netty version and maybe call:

clientOptions.addEnabledSecureTransportProtocol("TLSv1.3")
sberyozkin commented 1 year ago

@andrm Which version of Quarkus do you work with ? If not the latest, then can you please try for ex 2.15.0.CR1 ? Let's check if the latest Netty can handle it as per Paulo's suggestion. I can explicitly enable it if it won't work.

andrm commented 1 year ago

I'm using 2.14.2-FINAL. Testing on latest today.

andrm commented 1 year ago

Still happening with 2.15.0.CR1 2022-12-03 10:51:07,263 WARN [io.qua.oid.com.run.OidcCommonUtils] (vert.x-eventloop-thread-1) OIDC Server is not available:: javax.net.ssl.SSLHandshakeException: Received fatal alert: protocol_version at java.base/sun.security.ssl.Alert.createSSLException(Alert.java:131) at java.base/sun.security.ssl.Alert.createSSLException(Alert.java:117) at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:358) at java.base/sun.security.ssl.Alert$AlertConsumer.consume(Alert.java:293) at java.base/sun.security.ssl.TransportContext.dispatch(TransportContext.java:204) at java.base/sun.security.ssl.SSLTransport.decode(SSLTransport.java:172) at java.base/sun.security.ssl.SSLEngineImpl.decode(SSLEngineImpl.java:736) at java.base/sun.security.ssl.SSLEngineImpl.readRecord(SSLEngineImpl.java:691) at java.base/sun.security.ssl.SSLEngineImpl.unwrap(SSLEngineImpl.java:506) at java.base/sun.security.ssl.SSLEngineImpl.unwrap(SSLEngineImpl.java:482) at java.base/javax.net.ssl.SSLEngine.unwrap(SSLEngine.java:679) at io.netty.handler.ssl.SslHandler$SslEngineType$3.unwrap(SslHandler.java:296) at io.netty.handler.ssl.SslHandler.unwrap(SslHandler.java:1343) at io.netty.handler.ssl.SslHandler.decodeJdkCompatible(SslHandler.java:1236) at io.netty.handler.ssl.SslHandler.decode(SslHandler.java:1285) at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:529) at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:468) at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:290) at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:444) at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420) at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412) at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410) at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:440) at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420) at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919) at io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:166) at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:788) at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:724) at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:650) at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:562) at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997) at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74) at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) at java.base/java.lang.Thread.run(Thread.java:833) 2022-12-03 10:51:07,779 INFO [io.quarkus] (Quarkus Main Thread) rmserver 1.2-SNAPSHOT on JVM (powered by Quarkus 2.15.0.CR1) started in 4.992s. Listening on: http://localhost:8080

sberyozkin commented 1 year ago

Thanks @andrm

cescoffier commented 1 year ago

Which version of Java and OS are you using? Can you provide a reproducer?

andrm commented 1 year ago

Java 17, temurin 17 17.0.5, Fedora 37 for quarkus. Nginx side: 1.22.1. Reproducer: quarkus side: just add quarkus.http.auth.permission.authenticated.policy=authenticated quarkus.oidc.auth-server-url=https://mykeycloak/realms/myrealm

to application.properties. gradle file has implementation 'io.quarkus:quarkus-oidc'

The interesting thing is on the nginx side:

#    ssl_protocols TLSv1.3;
    ssl_protocols TLSv1.2 TLSv1.3;

First line will not work, second line does work.

I'm not sure how to provide a producer. I can set up a publicly available keycloak + nginx configuration on a vhost. DM me if you need that.

sberyozkin commented 1 year ago

@andrm Can you access this Keycloak site from the browser ? I'm OK with adding a configuration property, but just would like to double check it is not restricted to the OIDC/Vert.x Client

andrm commented 1 year ago

Yes, no problem with browsers. Basically I'm using the "Modern" configuration from https://ssl-config.mozilla.org/ I'm fine with any solution you deem the right one. I just think TLSv1.3 should be supported (by quarkus and vertex)


# generated 2022-12-05, Mozilla Guideline v5.6, nginx 1.17.7, OpenSSL 1.1.1k, modern configuration
# https://ssl-config.mozilla.org/#server=nginx&version=1.17.7&config=modern&openssl=1.1.1k&guideline=5.6
server {
    listen 80 default_server;
    listen [::]:80 default_server;

    location / {
        return 301 https://$host$request_uri;
    }
}

server {
    listen 443 ssl http2;
    listen [::]:443 ssl http2;

    ssl_certificate /path/to/signed_cert_plus_intermediates;
    ssl_certificate_key /path/to/private_key;
    ssl_session_timeout 1d;
    ssl_session_cache shared:MozSSL:10m;  # about 40000 sessions
    ssl_session_tickets off;

    # modern configuration
    ssl_protocols TLSv1.3;
    ssl_prefer_server_ciphers off;

    # HSTS (ngx_http_headers_module is required) (63072000 seconds)
    add_header Strict-Transport-Security "max-age=63072000" always;

    # OCSP stapling
    ssl_stapling on;
    ssl_stapling_verify on;

    # verify chain of trust of OCSP response using Root CA and Intermediate certs
    ssl_trusted_certificate /path/to/root_CA_cert_plus_intermediates;

    # replace with the IP address of your resolver
    resolver 127.0.0.1;
}
pmlopes commented 1 year ago

@andrm can you capture a wireshark session with the application connecting to keycloak? It's hard to especulate what could be happening without some reproducer/network dump.

You can also start a keycloak server, apply the required configuration and export the config, so we can start a container again and import it to test a simple client credentials flow from vertx/quarkus

pmlopes commented 1 year ago

@sberyozkin @cescoffier I've no knowledge on the internals, but from the stack trace I saw:

https://github.com/quarkusio/quarkus/blob/main/extensions/oidc-common/runtime/src/main/java/io/quarkus/oidc/common/runtime/OidcCommonUtils.java#L121-L175

And in there I don't see any reference to:

clientOptions.addEnabledSecureTransportProtocol("TLSv1.3")

Maybe that is what is missing? @vietj just confirmed to me that current vert.x does work with TLSv1.3 (a simple HttpClient can connect without issues). I'm wondering if the method above is missing a few more ways to configure the whole object?

vietj commented 1 year ago

as @pmlopes if you can capture a TCP dump of the SSL handshake (client hello, server hello) that would help understand if that's a TLS version issue as we can determine which supported versions the client sent and what the server replied

andrm commented 1 year ago

kc2.zip

Capture. You can use quarkus.oidc.auth-server-url=https://authtest.obersielmingen.de/realms/tlstest to test.

andrm commented 1 year ago

If needed I can give you ssh access to this server.

sberyozkin commented 1 year ago

@pmlopes Sure, I'll be on it quite soon.

I suppose it will have to be done optionally if TLSv1.3 is supported at the configuration level ? Otherwise it might cause problems communicating with the old TLSv1.2 only systems ?

andrm commented 1 year ago

Configuration of nginx:

server {
    listen 443 ssl http2;
    listen [::]:443 ssl http2;
    server_name authtest.obersielmingen.de;

    ssl_certificate /etc/letsencrypt/live/authtest.obersielmingen.de/fullchain.pem;
    ssl_certificate_key /etc/letsencrypt/live/authtest.obersielmingen.de/privkey.pem;
    ssl_prefer_server_ciphers on;
    #ssl_ciphers "EECDH+AESGCM:EDH+AESGCM:AES256+EECDH:AES256+EDH";
    ssl_ciphers ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:DHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES256-GCM-SHA384;
    ssl_dhparam /etc/ssl/certs/dhparam.pem;
    ssl_ecdh_curve secp384r1;
    ssl_session_cache shared:SSL:10m;
    ssl_session_tickets off;
    ssl_stapling on;
    ssl_stapling_verify on;

    add_header X-Content-Type-Options nosniff;
    add_header X-XSS-Protection "1; mode=block";
    add_header X-Robots-Tag none;
    add_header Referrer-Policy no-referrer;
    add_header Strict-Transport-Security "max-age=15552000; includeSubDomains; preload";

    ssl_protocols TLSv1.3;
    error_log /var/log/nginx/authtest.obersielmingen.de-ssl-error.log debug;
    access_log /var/log/nginx/authtest.obersielmingen.de-ssl-access.log main; 

  location / {
    proxy_set_header Host $host;
    proxy_set_header X-Real-IP $remote_addr;
    #proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
    proxy_set_header X-Forwarded-Proto $scheme;
    proxy_set_header X-Forwarded-Host $host;
    proxy_set_header X-Forwarded-For $proxy_protocol_addr;
      proxy_pass http://localhost:8080/;
  }
  #   location /realms/ {
  #      proxy_pass http://keycloak/realms/;
  #    }
  #  location /resources/ {
  #      proxy_pass http://keycloak/resources/;
  #    } 
  #      location /robots.txt {
  #      proxy_pass http://keycloak/robots.txt;
  #    }
  #  location /js/ {
  #      proxy_pass http://keycloak/js/;
  #    } 
  #  location /admin/ {
  #       proxy_pass http://keycloak/admin/;
  #  }

}
cescoffier commented 1 year ago

I would enable TLS 1.3 by default no? The handshake should be able to pick the right version.

pmlopes commented 1 year ago

I suppose it will have to be done optionally if TLSv1.3 is supported at the configuration level ? Otherwise it might cause problems communicating with the old TLSv1.2 only systems ?

I'd rather ask @vietj as he's the expert in that area :)

sberyozkin commented 1 year ago

@cescoffier Sure, that would be ideal :-). Lets also see what Julien says

vietj commented 1 year ago

I think we should indeed allow TLSv1.3 among default protocols, I'll take care of that

vietj commented 1 year ago

the list of protocols is currently hardcoded because of https://fr.wikipedia.org/wiki/POODLE

vietj commented 1 year ago

https://github.com/eclipse-vertx/vert.x/issues/4557

andrm commented 1 year ago

I'll shutdown the test instance. Please let me know if you need it for testing

sberyozkin commented 1 year ago

@andrm Can you please check Quarkus 2.16.0.CR1 ?

andrm commented 1 year ago

Still happening with 2.16.0.CR1:

 > Task :rmserver:quarkusDev
Listening for transport dt_socket at address: 5005
Press [h] for more options>
Tests paused
Press [r] to resume testing, [h] for more options>
Press [r] to resume testing, [o] Toggle test output, [h] for more options>
__  ____  __  _____   ___  __ ____  ______ 
 --/ __ \/ / / / _ | / _ \/ //_/ / / / __/ 
 -/ /_/ / /_/ / __ |/ , _/ ,< / /_/ /\ \   
--\___\_\____/_/ |_/_/|_/_/|_|\____/___/   
2023-01-17 08:11:45,370 WARN  [io.qua.oid.com.run.OidcCommonUtils] (vert.x-eventloop-thread-1) OIDC Server is not available:: javax.net.ssl.SSLHandshakeException: Received fatal alert: protocol_version
    at java.base/sun.security.ssl.Alert.createSSLException(Alert.java:131)
    at java.base/sun.security.ssl.Alert.createSSLException(Alert.java:117)
    at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:358)
    at java.base/sun.security.ssl.Alert$AlertConsumer.consume(Alert.java:293)
    at java.base/sun.security.ssl.TransportContext.dispatch(TransportContext.java:204)
    at java.base/sun.security.ssl.SSLTransport.decode(SSLTransport.java:172)
    at java.base/sun.security.ssl.SSLEngineImpl.decode(SSLEngineImpl.java:736)
    at java.base/sun.security.ssl.SSLEngineImpl.readRecord(SSLEngineImpl.java:691)
    at java.base/sun.security.ssl.SSLEngineImpl.unwrap(SSLEngineImpl.java:506)
    at java.base/sun.security.ssl.SSLEngineImpl.unwrap(SSLEngineImpl.java:482)
    at java.base/javax.net.ssl.SSLEngine.unwrap(SSLEngine.java:679)
    at io.netty.handler.ssl.SslHandler$SslEngineType$3.unwrap(SslHandler.java:296)
    at io.netty.handler.ssl.SslHandler.unwrap(SslHandler.java:1343)
    at io.netty.handler.ssl.SslHandler.decodeJdkCompatible(SslHandler.java:1236)
    at io.netty.handler.ssl.SslHandler.decode(SslHandler.java:1285)
    at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:529)
    at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:468)
    at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:290)
    at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:444)
    at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420)
    at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412)
    at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410)
    at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:440)
    at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420)
    at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919)
    at io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:166)
    at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:788)
    at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:724)
    at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:650)
    at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:562)
    at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997)
    at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
    at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
    at java.base/java.lang.Thread.run(Thread.java:833)

2023-01-17 08:11:45,527 INFO  [io.quarkus] (Quarkus Main Thread) rmserver 1.2-SNAPSHOT on JVM (powered by Quarkus 2.16.0.CR1) started in 1.428s. Listening on: http://localhost:8080
sberyozkin commented 1 year ago

Thanks @andrm

Vert.x 4.3.6 milestone is set in https://github.com/eclipse-vertx/vert.x/issues/4557, but I don't see the related commit in https://github.com/eclipse-vertx/vert.x/commits/4.3.6, @vietj Hi, do you plan to have it included in 4.4.0 only ?

juangon commented 1 year ago

I am reproducing the same error, using Quarkus 2.16.3.Final. Are there any workaround at least?

andrm commented 1 year ago

@juangon The server needs to allow TLSv1.2.

juangon commented 1 year ago

And what if I set quarkus.oidc-client.tls.verification=none? We have strong requirements for using just TLSv1.3

andrm commented 1 year ago

I understand. Switching off verification does not help, it's about verifying the certificate of the server, TLS version is negotiated before.

I think the problem will be addressed soon: vertx 4.4 which fixes this issue will be out by end of the week, and hopefully quarkus will be rebased to vertx 4.4 in the next few weeks.

andrm commented 1 year ago

Vertx 4.4.0 including the TLSv1.3 default. has been released today. I'm wondering how fast quarkus can include it.

sberyozkin commented 1 year ago

@andrm Hi, please see the linked PR in progress

andrm commented 1 year ago

Great, is there already a version that I can test?

cescoffier commented 1 year ago

No, we need to wait for Vert.x 4.4.1

andrm commented 1 year ago

Will this be in 3.0.x? How long vertx 4.4.1 out

cescoffier commented 1 year ago

Vert.x 4.4.1 will be released next week.

andrm commented 1 year ago

Anything new here?

cescoffier commented 1 year ago

The integration of vertx 4.4.1 requires a bit more work than anticipated, but we are finalizing the PR.

sberyozkin commented 1 year ago

@andrm Can you please try with Quarkus 3.0.3.Final ?

andrm commented 1 year ago

Looks good to me.

sberyozkin commented 1 year ago

Thanks @andrm