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

TrustStore support for OIDC and OIDCClient #18012

Merged
merged 1 commit into from
Jul 26, 2021

Conversation

sberyozkin
Copy link
Member

@sberyozkin sberyozkin commented Jun 18, 2021

Fixes #18002.

This draft PR adds support for configuring OIDC, OIDC client and also Keycloak authorization with the trust store file/password - property names are the same as in Vert.x HTTP, we can add more related properties as needed (alias, etc).

CC @rvansa

@sberyozkin sberyozkin requested a review from pedroigor June 18, 2021 16:50
@sberyozkin sberyozkin force-pushed the oidc_trust_certificate branch 2 times, most recently from 9690f29 to 7eb1648 Compare June 18, 2021 17:25
keycloak = new GenericContainer<>(keycloakDockerImage)
.withExposedPorts(8080, 8443)
.withEnv("DB_VENDOR", "H2")
.withEnv("KEYCLOAK_USER", "admin")
.withEnv("KEYCLOAK_PASSWORD", "admin")
.waitingFor(Wait.forHttp("/auth").forPort(8080));

if (KEYCLOAK_USE_HTTPS && KEYCLOAK_TRUSTSTORE_REQUIRED) {
keycloak = keycloak.withClasspathResourceMapping(KEYCLOAK_TLS_KEY, KEYCLOAK_TLS_KEY_MOUNTED_PATH,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pedroigor is it the right way to mount a key ? I've read in in a rather old Stackoverflow that this is how it should be done, but I'm not sure it is still the case, the test does not work

Copy link
Member Author

@sberyozkin sberyozkin Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pedroigor I have not seen in the latest docs any references to a private key having to be mounted to /etc/x509/http/tls.key, it is only mentioned in the non up to date docs. I wonder what the current approach is - mounting a private key should lead to WildFly converting it to a keystore, but perhaps now an actual keystore file should be mapped ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it should be /etc/x509/https/tls.key?

Copy link
Member Author

@sberyozkin sberyozkin Jun 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pedroigor thanks, the docs at https://hub.docker.com/r/jboss/keycloak/ do refer to /etc/x509/https.

I'm pretty sure some progress has been made as a result since now I'm seeing in the logs:

Creating HTTPS keystore via OpenShift's service serving x509 certificate secrets..
HTTPS keystore not created at: /opt/jboss/keycloak/standalone/configuration/keystores/https-keystore.jks (check permissions?)

I've checked, this error is not logged if the path is wrong (http in /etc/x509/http/tls.key).

The section at https://hub.docker.com/r/jboss/keycloak/ points out at the possible problem.

The permissions for both tls.crt and tls.key are r for the world on my system.
Host mounting them with withClasspathResourceMapping or copying to the docker (.withCopyFileToContainer) makes no difference though....
But it is nearly there, we just need to see how it is done in some working deployments

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pedroigor @rvansa the example here shows how these files are mapped with the specific user/group ids and also the mode. TestContainers does now allow to set it when mounting the files.
I've also seen somewhere setting the mode to 655 on the local system before mounting the files should work - but I could not make it work.

@sberyozkin sberyozkin force-pushed the oidc_trust_certificate branch from 7eb1648 to 053647b Compare June 22, 2021 13:43
@sberyozkin
Copy link
Member Author

sberyozkin commented Jun 23, 2021

@rvansa Would you like to try this PR against your deployment ? If we'll have problems verifying it with the test then I guess we can just merge it without it if you confirm it works for you, though the test should really be done :-)

@rvansa
Copy link
Contributor

rvansa commented Jun 23, 2021

@sberyozkin Hi, thanks a lot for the fix. I'll try to verify it soon but I have pretty full hands this week and the next one I won't be available, so have some patience please.

@sberyozkin
Copy link
Member Author

@rvansa Sounds good :-), I'll be off next week myself, thanks

@famod
Copy link
Member

famod commented Jun 24, 2021

I'll try to put that on my list as well since we also have to use TLS to talk to our Keycloak container and in dev env we also use custom certs.

@sberyozkin sberyozkin force-pushed the oidc_trust_certificate branch 2 times, most recently from 0b721fd to e0c17dd Compare June 28, 2021 12:35
@famod
Copy link
Member

famod commented Jul 5, 2021

I haven't yet gotten around to testing this but I have it on my list for this week. Would be great to have this end up in Quarkus 2.1

Btw, main is now on vert.x 4.1.1 which offers aliasPassword (and 4.1 offers alias). #18387 is adding aliasPassword to quarkus-vertx-http.

@sberyozkin
Copy link
Member Author

@famod Thanks, I believe you meant trustStoreKeyAlias, I'll add this property as well

@famod
Copy link
Member

famod commented Jul 6, 2021

I was actually thinking of the next step that would be client cert auth (for mutual TLS).

@sberyozkin
Copy link
Member Author

@famod

I was actually thinking of the next step that would be client cert auth (for mutual TLS).

OK

@sberyozkin sberyozkin force-pushed the oidc_trust_certificate branch from e0c17dd to 1c5a9e1 Compare July 7, 2021 16:17
@famod
Copy link
Member

famod commented Jul 15, 2021

@sberyozkin Sorry, didn't have the time yet to look at it. Do you think its still realistic to get this into 2.1?

@sberyozkin
Copy link
Member Author

@famod, np at all - we need at least a single confirmation this PR actually helps - I can try to experiment as well, haven't had time yet; so not sure if it can make it, I believe it would be safe to backport though

@sberyozkin sberyozkin requested a review from famod July 26, 2021 15:29
@sberyozkin sberyozkin marked this pull request as ready for review July 26, 2021 15:29
@sberyozkin sberyozkin marked this pull request as draft July 26, 2021 15:30
@sberyozkin
Copy link
Member Author

sberyozkin commented Jul 26, 2021

@famod Hi, I've managed to verify this code against a standalone Keycloak instance, by following Server installation Guide and also updating a custom realm configuration to require SSL for all the requests as described here. I copied the JKS file used in this PR, here are the relevant fragment:

 <subsystem xmlns="urn:jboss:domain:undertow:12.0" default-server="default-server" default-virtual-host="default-host" default-servlet-container="default" default-security-domain="other" statistics-enabled="${wildfly.undertow.statistics-enabled:${wildfly.statistics-enabled:false}}">
            <buffer-cache name="default"/>
            <server name="default-server">
<!--
                <https-listener name="https" socket-binding="https" security-realm="ApplicationRealm" enable-http2="true"/>
-->
                <https-listener name="https" socket-binding="https" security-realm="UndertowRealm"/>

        </subsystem>

and

<management>
        <security-realms>
<security-realm name="UndertowRealm">
    <server-identities>
        <ssl>
            <keystore path="keycloak.jks" relative-to="jboss.server.config.dir" keystore-password="secret" />
        </ssl>
    </server-identities>
</security-realm>
        </security-realms>
    </management>

I've also had to add a new enum, CERTIFICATE_VALIDATION to support a case where only the certificate validation but no hostname verification is required. I also imported the existing realm from quarkus-quickstarts/security-openid-connect-quickstart

Finally I've tweaked BearerTokenAuthorizationTest:

  1. Commented //@QuarkusTestResource(KeycloakTestResourceLifecycleManager.class)
  2. Copied getAccessToken from KeycloakTestResourceLifecycleManager to BearerTokenAuthorizationTest and updated to set the truststore path and secret exactly as it is done in KeycloakTestResourceLifecycleManager (and also use backend-client client id which is created from the existing realm)
  3. Updated application.properties to have auth-server-url to point to https://localhost:8543/auth/realms/quarkus
  4. Removed a ref to the email claim from application.properties as it is not avail in that existing realm
  5. Finally run a slightly updated test BearerTokenAuthorizationTest#testSecureAccessSuccessCustomPrincipal
@Test
    public void testSecureAccessSuccessCustomPrincipal() {
            RestAssured.given().auth().oauth2(getLocalAccessToken("alice"))
                    .when().get("/api/users/me")
                    .then()
                    .statusCode(200)
                    .body("userName", equalTo("alice"));
    }

and it worked.

I've temporarily disabled though the test resource factory code which sets up the trust store, I'll open an issue afterwards to try and get the test running, I've found another resource:
https://strimzi.io/blog/2019/10/25/kafka-authentication-using-oauth-2.0/

which suggests that the test factory does exactly the same thing by mounting the key and cert, but something just does not click - all these docs refer to the old image, so it needs some support from the KC guys.

Have a look please and I'll merge once you approve

@sberyozkin sberyozkin marked this pull request as ready for review July 26, 2021 16:04
@sberyozkin sberyozkin force-pushed the oidc_trust_certificate branch from 1c5a9e1 to 90243f7 Compare July 26, 2021 16:04
@sberyozkin sberyozkin force-pushed the oidc_trust_certificate branch from 90243f7 to 715ccdb Compare July 26, 2021 16:07
@famod
Copy link
Member

famod commented Jul 26, 2021

I have just started testing this PR with my current Quarkus project (that incorporates KC).

@famod
Copy link
Member

famod commented Jul 26, 2021

@sberyozkin Hum...it seems like I'm hitting a wall here as I keep getting:

io.quarkus.oidc.OIDCException: OIDC server is not available at the 'https://idp:8443/auth/realms/egbr' URL
[...]
 Caused by: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target

I seems as if the entry in my store is not taken into account. It does find the store though because settings some invalid value for quarkus.oidc.tls.trust-store-file does yield an exception (as expected).

I even tried setting the alias.

@famod
Copy link
Member

famod commented Jul 26, 2021

Nevermind, the store was missing our custom CA cert (it only contained the actual server cert). I will conclude my review tomorrow.
But looking good so far! 🎉

Copy link
Member

@famod famod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works in my project/setup and code LGTM.

I'll trust you on that test factory part @sberyozkin 🙂, didn't have a look at it myself.

I'll mark this for backport and see how it goes.

@famod famod merged commit 24829c7 into quarkusio:main Jul 26, 2021
@quarkus-bot quarkus-bot bot added this to the 2.2 - main milestone Jul 26, 2021
@famod
Copy link
Member

famod commented Jul 26, 2021

Have a look please and I'll merge once you approve

I read that a bit wrong and merged it already but I don't think it matters who merges in the end. 😉

@sberyozkin
Copy link
Member Author

@famod cool, thanks for checking it (I just came here to mention that 8543 port was used because I started with a 100 port offset :-) )

@sberyozkin
Copy link
Member Author

@famod in the next iteration we can wire in a mutual tls authentication, will have a look later on

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom CA for OIDC connection
5 participants