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

fix WillHaveCertificateForServerName check to be strict match for derived cert name #4167

Merged
merged 2 commits into from May 9, 2023

Conversation

wasaga
Copy link
Contributor

@wasaga wasaga commented May 8, 2023

Summary

An Ingress Controller has the following runtime environment specifics:

  • port-mapped (8443:443) due to non-root container requirements,
  • the authenticate service URL may not be accessible from inside the cluster - one example is performing Quickstart with https://authenticate.localhost.pomerium.io in a popular Docker Desktop environment

The authenticate endpoint is essential as it hosts /.well-known/pomerium/hpke-public-key which is consumed by Proxy and Authorize components.

In order to overcome that limitation, IC sets authenticate_internal_service_url to https://localhost:8443.

There are two issues this PR addresses:

Current implementation of the WillHaveCert assumed that it was sufficient just to check whether autoTLS is enabled, as in that case there would be a wildcard cert, signed by the derived CA.

The wildcard CA is indeed installed, however, if there are other wildcard certs added to the config, they would be presented instead. I think it might be related to full_scan_certs_on_tls_mismatch

If no cert is selected from certs that matches wildcard name, the candidate cert is selected for handshake if it is present. If there is no candidate, check full_scan_certs_on_sni_mismatch, go to full scan all certificates if it is enabled, otherwise pick the first certificate for handshake.

This PR changes the behaviour of the WillHaveCertificateForServerName (which is only currently used by HPKE Key Fetcher) to only promise the.

Couple notes:

  1. Maybe we should return a full set of certificates via cfg.AllCertificates(), moving derived cert generation out of the Envoy Config Builder to Config, and making this function only check the certs returned by the cfg.AllCertificates() so that it would be deterministic.
  2. The cryptutil.MatchesServerName used by tests naturally cannot reflect how the actual Envoy would perform TLS matching. We may want to do something about it as well.
  3. This issue does not seem to be unit testable, requires an integration test.

Related issues

Fixes pomerium/ingress-controller#629

User Explanation

Checklist

  • reference any related issues
  • updated docs
  • updated unit tests
  • updated UPGRADING.md
  • add appropriate tag (improvement / bug / etc)
  • ready for review

@wasaga wasaga added bug Something isn't working backport 0-22-0 labels May 8, 2023
@coveralls
Copy link

coveralls commented May 8, 2023

Coverage Status

Coverage: 63.751% (+0.07%) from 63.68% when pulling 66d4a8c on wasaga/fix-will-have-cert into 794298c on main.

@wasaga wasaga force-pushed the wasaga/fix-will-have-cert branch from 0178203 to 66d4a8c Compare May 9, 2023 01:50
@wasaga wasaga marked this pull request as ready for review May 9, 2023 13:10
@wasaga wasaga requested a review from a team as a code owner May 9, 2023 13:10
@wasaga wasaga requested a review from calebdoxsey May 9, 2023 13:10
@wasaga wasaga merged commit 80ffefe into main May 9, 2023
13 checks passed
@wasaga wasaga deleted the wasaga/fix-will-have-cert branch May 9, 2023 22:54
wasaga added a commit that referenced this pull request May 9, 2023
…ived cert name (#4169)

fix WillHaveCertificateForServerName check to be strict match for derived cert name (#4167)

Co-authored-by: Denis Mishin <dmishin@pomerium.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 0-22-0 bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enabling autoTLS causes authz_error
3 participants