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 Host support choosing cleartext or TLS #2279

Merged
merged 12 commits into from
Feb 7, 2020
Merged

Conversation

kflynn
Copy link
Member

@kflynn kflynn commented Feb 6, 2020

CHANGELOG: Properly support using a Host to specify cleartext or non-ACME TLS.
CHANGELOG: Be more forgiving about mismatched SNI connections.

This is probably best reviewable commit-by-commit. There are several small changes here that add up to substantial effects:

  • (commit abf295f) Edge Stack was creating the fallback TLSContext whenever no termination contexts were present, irrespective of what any Hosts said. If you think this through you'll realize that it means that there would always be a termination context present, preventing running a cleartext-only Edge Stack.

    We now only create the fallback context if there are no termination contexts and no Hosts, which is to say, when Edge Stack is running basically unconfigured. If a Host is present that says to run cleartext, it will be honored.

  • (commit 2182353) Don't look at the acmeProvider when deciding whether or not to enable TLS termination for a Host -- instead, only look at whether a tlsSecret is present. The effect here is that if you say acmeProvider.authority: none but manually provide a tlsSecret, you'll still get TLS termination.

    (When Edge Stack ACME is in use, the ACME client will fill in the tlsSecret for you if you don't set it, so there will always be a tlsSecret if termination should happen. If you do set a tlsSecret with Edge Stack ACME in use, the ACME client will happily use the secret name you provide.)

  • (commits cd50960 and a63b529) Rationalize Envoy VHost entries such that we'll always have a VHost for *. If the configuration specifies a Host with hostname: "*", we'll use that -- otherwise, grab the first VHost entry and make it have hostname: "*".

    This seems odd, but is probably the least surprising thing to do: if you connect to a port and offer a strange SNI domain, it's probably less surprising to people to get back a certificate for something that should be listening there, rather than having Envoy simply drop the connection with a TLS error. If need be, we can create a setting that allows disabling this more forgiving behavior later.

Still to come: doc tweaks, and the port-override mechanism suggested by @nbkrause to fully support L4 split LBs. Those will be separate PRs.

Testing: KAT passes for both API Gateway and Edge Stack, including dropping the XFail for HostCleartext.

@kflynn kflynn requested review from LukeShu and concaf February 6, 2020 17:25
if not os.environ.get('AMBASSADOR_NO_TLS_REDIRECT', None):
new_ctx['redirect_cleartext_from'] = 8080
# if not os.environ.get('AMBASSADOR_NO_TLS_REDIRECT', None):
# new_ctx['redirect_cleartext_from'] = 8080
Copy link
Contributor

Choose a reason for hiding this comment

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

What was that ever for? Just as a hack for KAT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Many of the KAT tests did a poor job of handling the automagic redirect on 8080 that used to be there. This was a crutch that I expect to be deleting shortly (I have to do a full test run to make sure it's really no longer used).

@LukeShu
Copy link
Contributor

LukeShu commented Feb 6, 2020

I'm having a hard time understanding what's going on in a63b529. Perhaps it will make more sense after I put some food in me.

@kflynn
Copy link
Member Author

kflynn commented Feb 6, 2020

An important note for a63b529 is that enable_sni used to keep track of whether we would use a VHost of "*" (if only one unique domain was present) or a VHost with an actual domain name in it (if more than one unique domain was present). So much of that commit is ripping out the now-dead enable_sni logic, and then there's the small chunk of "if we don't have a VHost "*", then overwrite the first VHost's hostname that takes its place.

@LukeShu LukeShu self-requested a review February 7, 2020 18:06
@kflynn kflynn changed the base branch from master to rel/v1.1.1 February 7, 2020 18:35
@kflynn kflynn merged commit fd2c0e4 into rel/v1.1.1 Feb 7, 2020
@kflynn kflynn deleted the flynn/dev/cleartext branch February 7, 2020 18:37
LukeShu pushed a commit that referenced this pull request Feb 7, 2020
Fix Host support choosing cleartext or TLS
LukeShu pushed a commit that referenced this pull request Feb 7, 2020
Fix Host support choosing cleartext or TLS

(cherry picked from commit fd2c0e4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants