Skip to content
This repository has been archived by the owner on Nov 16, 2022. It is now read-only.

KEYCLOAK-12307 TLS Termination configuration #186

Conversation

slaskawi
Copy link
Contributor

@slaskawi slaskawi commented May 8, 2020

JIRA ID

KEYCLOAK-14095
KEYCLOAK-12307

Additional Information

This Pull Request changes the TLS configuration from Passthrough to Re-encrypt. In addition to that, it introduces a new tlsTermination parameter (in the Keycloak CR) that can be set to passthrough and fall back to previous behavior.

This change improves user experience and sacrifices a bit of security. As we agreed with @stianst, the safest way of running Keycloak is a Passthrough Route that sends all encrypted traffic to Keycloak. However, this option is quite hard to configure as the Signing Operator would need to create a valid, publicly accepted certificate to Keycloak. Much easier option is to configure this on an Ingress or Route. Moreover, OpenShift Installer has an option to install OpenShift with Let's Encrypt support out of the box. However, it's worth to mention, that OpenShift Router (HAProxy) will be performing re-encryption and if some malicious code gets there, all secret codes might be dumped somewhere. This is a security concern every administrator needs to consider.

Finally, this Pull Request doesn't touch Ingress. Ingress Controller uses, so called backside re-encryption (basically, it is configured to use HTTPS Backend) (see link1 link2). The Frontend TLS configuration is up to the user. The Operator doesn't reconcile it.

Verification Steps

Checklist:

  • Verified by team member
  • Comments where necessary
  • Automated Tests
  • Documentation changes if necessary

Additional Notes

@slaskawi
Copy link
Contributor Author

slaskawi commented May 8, 2020

@andymunro @pb82 @davidffrench You might be interested in this PR.

@slaskawi slaskawi requested review from mhajas and stianst May 8, 2020 09:07
@slaskawi slaskawi added this to the 9.0.3 milestone May 8, 2020
@coveralls
Copy link

coveralls commented May 8, 2020

Coverage Status

Coverage increased (+1.5%) to 42.181% when pulling fbcea8c on slaskawi:KEYCLOAK-12307-TLS-configuration-for-external-access into 3ed671b on keycloak:master.

@davidffrench
Copy link
Contributor

davidffrench commented May 8, 2020

Thanks @slaskawi . @pb82 this means we should be able to remove our additional route for keycloak in the integreatly-operator 🥳

cc @philbrookes

@slaskawi
Copy link
Contributor Author

slaskawi commented May 8, 2020

@davidffrench Yes, it took @stianst and myself a loooong time to decide on this.

@pb82
Copy link
Contributor

pb82 commented May 8, 2020

@slaskawi @davidffrench looks like it only can be passthrough or reencrypt: https://github.com/keycloak/keycloak-operator/pull/186/files#diff-5ddfce748972d078ca691ae71e4d08ffR56

We currently create a custom edge route.

@slaskawi
Copy link
Contributor Author

slaskawi commented May 8, 2020

@pb82 Why do you use an edge-terminated route? If that's the Let's Encrypt case, the re-encrypt should do the job and it's much safer (as the traffic between HAProxy and the Pod is encrypted).

@pb82
Copy link
Contributor

pb82 commented May 8, 2020

@slaskawi I was wrong, we actually use re-encrypt. So this should be fine.

@slaskawi
Copy link
Contributor Author

I was wrong, we actually use re-encrypt. So this should be fine.

@pb82 Ok, that's great! Thanks Peter!

@slaskawi slaskawi force-pushed the KEYCLOAK-12307-TLS-configuration-for-external-access branch from 2068558 to f62297e Compare May 11, 2020 07:25
@slaskawi
Copy link
Contributor Author

@stianst Comments addressed and re-pushed.

@slaskawi slaskawi force-pushed the KEYCLOAK-12307-TLS-configuration-for-external-access branch 2 times, most recently from 9c9d6a0 to 3e0d2e4 Compare May 11, 2020 07:47
mhajas
mhajas previously approved these changes May 11, 2020
Copy link
Contributor

@mhajas mhajas left a comment

Choose a reason for hiding this comment

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

After fixing the issue @stianst mentioned, this looks good to me. I was able to test it with both passthrough and re-encrypt and both worked as expected.

@slaskawi slaskawi force-pushed the KEYCLOAK-12307-TLS-configuration-for-external-access branch from 3e0d2e4 to a35212a Compare May 12, 2020 10:38
pkg/model/keycloak_route.go Outdated Show resolved Hide resolved
@slaskawi slaskawi force-pushed the KEYCLOAK-12307-TLS-configuration-for-external-access branch from a35212a to fbcea8c Compare May 12, 2020 14:03
@slaskawi
Copy link
Contributor Author

@mhajas Should be fine now.

Copy link
Contributor

@mhajas mhajas left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, @slaskawi.

@stianst stianst assigned slaskawi and unassigned stianst May 18, 2020
@slaskawi slaskawi merged commit 204a6a8 into keycloak:master May 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants