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

Cipher Suite and Start up fixes #2772

Closed
wants to merge 16 commits into from

Conversation

ACEx86
Copy link

@ACEx86 ACEx86 commented Jan 26, 2025

Please verify and merge this pull request.

I made some changes that will fix the selected things:

Correctly use on xtrasport dnscrypt > fallback\bootstrat > system as it described on toml.

From commit ca0f353 and after if the cipher_suite is set and there is no source files the dnscrypt will fail to download it

On start up the cipher suite will not use the selected cipher suite when source is present. I fixed that by changing the config to rebuildtrasnport with the selected cipher_suite when it finishes to fix the download on startup if there is no source and we have handshake issue.

Some times on certificate refresh and other places that the rebuildtrasport may be called it will drop the cipher_suite to 4865. Now it will keep the selected cipher_suite

Changed some messages and fixed a wrong error message

This error's created by setting the cipher suite to null.
Right now if the cipher suite is incorrect it won't connect but the other implementation dropped the cipher suite on refreshes to no PFS 4865. I think by default you can try to set for TLS 1.2 a more secure communication.

52393 = TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305

52392 = TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305

49249 = TLS_ECDHE_RSA_WITH_ARIA_256_GCM_SHA384

49200 = TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384

49199 = TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256

49196 = TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384

49195 = TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256

There is one more thing that i haven't fixed. Gonna create an issue.
When no bootstarp resolver is specified in the file and you drop to deafult bootstrap, i think it is because of panic, it will panic before trying to create a connection with the second protocol if the first is blocked. 9.9.9.9 UDP no TCP panic but TCP 9.9.9.9 may be allowed. Double request on same protocol.

ACEx86 added 10 commits January 25, 2025 17:42
downgrade test fix
Testing keep cipher suite to fix cipher suite downgrade and tls 1.3 upgrade on error/dos.
Trying to keep the same specified cipher suite if we had a successfull connection.
Testing fix the ResolveAndUpdate to go with the correct dnscrypt order and dont add null IP
fix cipher suite on startup error
fix on startup and cipher suite downgrade on refresh
fix cipher suite downgrade on startup if source don't exist
change message
Fix false error message when only 1 resolver on second protocol
@jedisct1
Copy link
Member

I think we should remove support for setting the cipher suite.

The justification for this was to use RSA instead of ECDSA since it is faster on some old devices. That was relevant 5 years ago, but today, not so much. And if resource usage is a concern, use dnscrypt, not doh.

Go also doesn't support setting the cipher suite without a downgrade to TLS 1.2, which is not ideal and may even prevent connecting to some servers.

It's also not easy to use, with all these cipher suite numbers. Messing with the cipher suites can only reduce, never improve security.

Your changes are probably good but add a lot of complexity that I don't understand but that will have to be maintained, for a feature that should be deprecated.

Things may be different if Go didn't remove the ability to do that, but eventually, even forcing TLS 1.2 is likely to become impossible.

So I think we should make the cipher suite setting a no-op rather than trying to maintain that monstrosity.

@ACEx86
Copy link
Author

ACEx86 commented Jan 26, 2025

TLS 1.3 is 5 algos with no PFS and Authentication and all the secure algos of TLS 1.2 and this is a HTTPS TLS proxy.
https://ciphersuite.info/cs/?tls=xtls13
I don't believe you should drop support for TLS1.2 for as long as it is supported the TLS1.2 is the most secure option.
52393 49196 works perfectly for me on cloudflare and they also support RSA not sure for DNS.
https://developers.cloudflare.com/ssl/edge-certificates/additional-options/cipher-suites/supported-cipher-suites/

@ACEx86
Copy link
Author

ACEx86 commented Jan 26, 2025

I believe for now we missing the ECH support.
https://developers.cloudflare.com/ssl/edge-certificates/ech/

@jedisct1
Copy link
Member

This is not about dropping TLS 1.2 support, but always using Go's default strategy, which already includes using TLS 1.2 if TLS 1.3 is not available. TLS 1.3 always provides PFS by design.

And this is a problem with allowing people to specify a cipher list. They think changing this is going to improve something while the defaults are safer.

That option was there just for one thing: devices on which ECDSA was too slow compared to RSA. But having that specified as a cipher list was a poor choice, especially as new algorithms are being added to TLS.

Maybe it should be changed to a only_use_legacy_tls_configurations_because_my_device_is_slow boolean option, that would also disable anything else that would be CPU intensive on these devices.

The RFC for ECH hasn't been published yet, and support in Go is not going to happen before Go 1.24. But then it should be pretty much transparent.

Fix on error decoding drop cipher suite at begin and change tls
Fix TLS 1.3 cipher configured
Follow overrideCipherSuite commit
revert follow up change
Only scan the suites if we have a suites and we also keep the cipher suite and don't drop to tls 1.2 if it is an tls 1.3 suite.
@ACEx86
Copy link
Author

ACEx86 commented Jan 27, 2025

TLS_ECDHE?_AES_128_GCM_SHA256 is this even 4865?
i dont believe its better than
TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 or TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
but that doesn't even matter. Let people configure whatever from the existing client cipher suite list.

ECH is there.
https://pkg.go.dev/crypto/tls#Config

@jedisct1
Copy link
Member

You’ve demonstrated how confusing this is. 😊

To be honest, I don’t understand what you’re trying to achieve, either from the description or the code. It seems like you might be a bit unsure about your own code as well. 😊

I’m hesitant to maintain code when I don’t fully understand what it’s supposed to do, what it actually does, or what problem it solves. I’m sorry, but I find this code particularly hard to follow. While you might have ideas for improving the software, these would ideally be discussed in the discussion section first.

The one thing I did understand is that the documentation previously stated that, to use the default configuration, the cipher suite parameter should be empty. This was incorrect—what the documentation should have said is that the parameter should be undefined (i.e., the line should remain commented), not empty.

Thank you for pointing this out! The documentation has been updated accordingly, and leaving the parameter empty will now behave the same way. An actual empty list of ciphers wouldn’t make sense anyway.

@jedisct1
Copy link
Member

https://00f.net/2025/01/27/why-your-pull-request-might-not-be-merged/

@jedisct1 jedisct1 closed this Jan 27, 2025
@DNSCrypt DNSCrypt locked and limited conversation to collaborators Feb 27, 2025
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.

2 participants