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

Implement SNI Fallback in TLS settings #8124

Closed
wants to merge 1 commit into from
Closed

Conversation

akx
Copy link

@akx akx commented May 6, 2021

What does this PR do?

This implements an optional sniFallback field in TLS options, akin to what I requested in #8123.

It can be used in conjunction with SNI-less requests and Let's Encrypt certificates, to blindly assume a request that has no other matching certificate to use a named one.

Motivation

Please see #8123.

More

  • Added/updated tests
    • Not yet! Not sure how to best test this, I'm not an experienced gopher.
  • Added/updated documentation

@akx

This comment has been minimized.

@dtomcej
Copy link
Contributor

dtomcej commented May 6, 2021

Hello @akx,

How does this differ from using the defaultCertificate for non-matching requests? (https://doc.traefik.io/traefik/https/tls/#default-certificate)

@akx
Copy link
Author

akx commented May 6, 2021

How does this differ from using the defaultCertificate for non-matching requests? (https://doc.traefik.io/traefik/https/tls/#default-certificate)

Hi @dtomcej – as discussed in #2829 (comment) defaultCertificates need to be on the file system. This works for certs issued by LE within Traefik.

@akx
Copy link
Author

akx commented May 7, 2021

Could I get e.g. @SantoDE to take a look at this? (I see you'd assigned yourself for #8123)

@akx akx force-pushed the sni-fallback branch 2 times, most recently from 80d6508 to d511785 Compare May 7, 2021 08:33
@stacab
Copy link

stacab commented May 14, 2021

Hi,
I need this feature so much in Traefik. Any chance to have it reviewed soon at least?

@akx
Copy link
Author

akx commented May 27, 2021

Rebased on master.

@akx
Copy link
Author

akx commented Jun 15, 2021

Rebased on master once again.

@jbdoumenjou Is there anything I can do to move this forward?

@ldez ldez requested a review from juliens June 15, 2021 10:42
@jbdoumenjou
Copy link
Member

Hello @akx,

Thanks for your contribution!

This PR is labeled needs-design-review,
as you can see in our contribution guide, this means that we have to take a closer look at the scope of impact for that PR to see, how it would interact with the other parts of Traefik.

We will come back to you soon when we did the first design review iteration.

@akx
Copy link
Author

akx commented Jul 9, 2021

@jbdoumenjou Hi there! Sorry to keep bothering you – how's the design review process coming along? I don't think this is a very large change architecturally, and it's really a last-chance fallback thing...

@shinglerb
Copy link

Hi, would anyone know if/when this will be reviewed? I've encountered this issue a few times now and would be willing to offer help if required.

@akx
Copy link
Author

akx commented Sep 27, 2021

@jbdoumenjou Any news? 🍂 I rebased this branch for your convenience.

This implements an optional `sniFallback` field in TLS options.

It can be used in conjunction with SNI-less requests and Let's Encrypt
certificates, to blindly assume a request that has no other matching
certificate to use a named one.

Refs traefik#8123
@adyanth
Copy link

adyanth commented Jan 18, 2022

Would love to see this pulled in too. Here is another request on community https://community.traefik.io/t/tls-options-for-snistrict-false-in-tcp-router/4626

I have a need for this to implement DNS over TLS since not even modern smartphones send SNI when doing DoT requests.

Thanks @akx for your contribution. Any update from the @traefik team on this? It has been more than 8 months since this was raised!

@rtribotte
Copy link
Member

Hello @akx,

While we definitely see the benefit of your changes to handle SNI-less requests, we were hesitant about the solution and we finally decided to decline the PR, and here's why:

We acknowledge that what we already have (i.e. to let the default cert being selected when no cert matches the SNI) feels a bit uneasy, as it seems to go against the intent of the SNI concept. Which is exactly why we have the sniStrict option by the way. In that regard, we do not like the idea of going further in that "wrong" direction, i.e. to widen the pool of possible certs when SNI does not match. Which is why we'd rather go in a subtly different direction (to achieve a similar goal): we won't change the fact that we get the default cert when there is no SNI match, but we'll expand on the mechanisms that can (dynamically) generate the (unique) default cert.

To that end, we've already started investigating a way to generate the default cert through Let's Encrypt, and we'll keep you posted on how that goes.

@rtribotte rtribotte closed this Feb 10, 2022
v2 automation moved this from To review to Done Feb 10, 2022
@adyanth
Copy link

adyanth commented Feb 10, 2022

@rtribotte

we won't change the fact that we get the default cert when there is no SNI match, but we'll expand on the mechanisms that can (dynamically) generate the (unique) default cert.

Does this mean that if an HTTPS request comes in without SNI, a Let's Encrypt cert can be served?

Also, is there a place we can track these changes?

@rtribotte
Copy link
Member

Hello @adyanth,

Does this mean that if an HTTPS request comes in without SNI, a Let's Encrypt cert can be served?

Yes, Traefik will still have the same behavior as of today, without an SNI value (and when option sniStrict is false), the default certificate will be served.

Since the feature would be to generate the default cert through Let's Encrypt, in that case, the default certificate served will be a Let's Encrypt certificate.

Also, is there a place we can track these changes?

We will link the PR bringing the feature when it will be opened.

@akx
Copy link
Author

akx commented May 25, 2022

@rtribotte Any news on the investigation for a replacement feature? I'd still have a need for this, approximately a year later from me originally opening this PR...

@rtribotte rtribotte mentioned this pull request Jul 13, 2022
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v2
Done
Development

Successfully merging this pull request may close these issues.

None yet

8 participants