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

feat: Add "if-same-cert" sni consistency mode #216

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

xkr47
Copy link
Contributor

@xkr47 xkr47 commented Nov 4, 2024

So yeah, I have a wildcard certificate that I use for multiple separate apps. And my browser is indeed reusing a connection to site1.example.com for service site2.example.com. I don't yet know whether it is doing it because it notices the same certificate is valid for both, or because they are served by the same IP:port combination. But I am hopeful the latter is not the case because that honestly feels a bit insane.

So yeah, by default rpxy sends 421 responses to such un-rfc behaviour. But unfortunately my browser does not either automatically re-issue the request on a different connection (MDN docs also says it is indeed optional operation). So at least one app that I have stops polling the server when this happens.

So I tested setting ignore_sni_consistency = true in the config. And that solved the problem. But the comments in front of the configuration option left me restless and I thought "so will it serve any configured app then, regardless of certificate?".. So I tested it and found out that yes, I could even access sites that don't have a tls certificate configured by using e.g. curl 'https://site1.example.com/' -H 'Host: site2.example.com' where site2 does not have a tls cert configured.

So I wanted a middle ground that allows this un-rfc behaviour ONLY when both the app served by the servername from sni AND the app requested by the Host header have the SAME certificate configured. More specifically, I defined "SAME" as "has the same tls_cert_path".

It works. I have yet to configure apps using two different certificates, will probably try that later using the built-in acme support. But even if my beloved browser decided to re-use a connection for to a site with a different certificate, I would be very angry at said browser and perhaps even file a ticket for it. So it would probably not change my stance that I want to stick to this "same-cert" middle-ground.

Anyway, this PR is my first take on adding this functionality. I initially wanted to make ignore_sni_consistency a three-state variable still supporting true and false so that existing configurations would not be broken, but alas true and false always seem to get treated as booleans so that didn't work. Instead I added a new samecert_sni_consistency boolean next to it to avoid breaking existing configurations. Please give your opinion on this. How it works now is:

ignore_sni_consistency samecert_sni_consistency result
false false "Full" SNI consistency = sni hostname must match Host hostname
false true "Same certificate" SNI consistency = app configured for sni hostname must use same certificate as app configured for Host hostname
true ignored "Ignore" SNI constistency = any app can be accessed, even ones without tls configured

I'll be happy to receive any feedback about this. Documentation changes not yet done. I introduced a LazyCell<> in handle_request_inner() to avoid the hashmap lookup until it is needed in one of the two places. I don't know if it makes sense performance-wise. Also if you want to support older rust versions then LazyCell is not usable. And yeah, I had to add the certificate path to the Backend object so I could compare it. But in retrospect I could also perhaps replace that with some certificate id so the comparison would be faster in the request path. Perhaps I'll do that.

@junkurihara
Copy link
Owner

Hi @xkr47

Thanks for your contribution. This is really related to the security design principle of rpxy, which must prevent the domain fronting https://en.wikipedia.org/wiki/Domain_fronting. So we should carefully handle this. I will review this carefully later.

@xkr47
Copy link
Contributor Author

xkr47 commented Nov 5, 2024

Agreed. I forgot to think about rpxy-acme-managed apps, which don't have the certificate file configured. But it might still work, just need to think that through as well. I will return after I have checked that.

@xkr47 xkr47 marked this pull request as draft November 5, 2024 11:38
@Gamerboy59
Copy link
Contributor

Agreed. I forgot to think about rpxy-acme-managed apps, which don't have the certificate file configured. But it might still work, just need to think that through as well. I will return after I have checked that.

Have you checked this, @xkr47 ?

@xkr47
Copy link
Contributor Author

xkr47 commented Jan 13, 2025

Sorry not yet. My setup is in kindof "if it ain't broken don't fix it" state but I'll try to look into this when I can.

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.

3 participants