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

Use better default TLS settings #175

Merged
merged 2 commits into from
Sep 29, 2024

Conversation

bmillwood
Copy link
Contributor

@bmillwood bmillwood commented Sep 21, 2024

Commit message:

On versions of tls pre-2.0.6, the default Supported had an empty list of supportedCiphers. Per discussion on their issue, it seems like everyone was overriding it to ciphersuite_default anyway, hence the change at version 2.0.6 to make that default. But we're not doing that here! Luckily, the default for TLSSettings does do that override, so we can use that to get the right behaviour regardless of tls version.

I noticed this because all my https requests were failing with this message:

(InternalException (HandshakeFailed (Error_Protocol ("expecting server hello, got alert : [(AlertLevel_Fatal,IllegalParameter)]",True,HandshakeFailure)))))

With this change, they succeed. (But they also succeed with the latest version of tls, which makes this difficult to write a regression test for.)

On versions of tls pre-2.0.6, the default `Supported` had an empty list
of `supportedCiphers`. Per discussion on [their issue][1], it seems like
everyone was overriding it to `ciphersuite_default` anyway, hence the
change at version 2.0.6 to make that default. But we're not doing that
here!  Luckily, the [default for `TLSSettings`][2] does do that
override, so we can use that to get the right behaviour regardless of
tls version.

[1]: haskell-tls/hs-tls#471
[2]: https://hackage.haskell.org/package/crypton-connection-0.4.0/docs/src/Network.Connection.Types.html#line-86
@bmillwood
Copy link
Contributor Author

I didn't think of this while I was creating this PR, but I guess this probably also means you can put the crypton-connection constraint lower bound back down to 0.3, since this code will also work before that field was added.

(Originally it was >= 0.2.2, but that version doesn't exist.)

@mrkkrp
Copy link
Owner

mrkkrp commented Sep 28, 2024

Thanks! Could you add the corresponding change in the lower bound of crypton-connection and a changelog entry?

@bmillwood
Copy link
Contributor Author

Thanks! Could you add the corresponding change in the lower bound of crypton-connection and a changelog entry?

Done. I think the PVP only requires this to be 3.14.3.1, but it looks like you don't use four-component versions? Happy to tweak it to that if you want.

@mrkkrp mrkkrp merged commit 947fa38 into mrkkrp:master Sep 29, 2024
4 checks passed
@mrkkrp
Copy link
Owner

mrkkrp commented Sep 29, 2024

Thanks!

@bmillwood bmillwood deleted the better-tls-setting-defaults branch September 29, 2024 19:24
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.

2 participants