add support for overriding the TLS clientSupported member in TLSSettingsSimple

[bfrk]

This is so we can manipulate these TLS settings even when using a connection manager like the http-client package does. See
#2 for details.

[bfrk]

I tested this and it seems to work fine with http-client when I set a suitably configure global connection manager.

[kazu-yamamoto]

I'm not sure why Maybe is used. I think that Supported is simply good enough.

[bfrk]

Hm, now that you say it, I think you are right.

[bfrk]

(I can squash the commits when you are done with reviewing)

[kazu-yamamoto]

LGTM.
Squash is not necessary.
Since the constructor is historically exported, this is a breaking change.
So, we need to bump the version to v0.4.0.
Is this OK for you?

[bfrk]

Yes, perfectly fine. Thanks!

[kazu-yamamoto]

Merged. Thanks!
A new version has been released.

[mrkkrp]

I am trying to update req to use the new version of the library with this change (I pass def as the new fourth argument of the TLSSettingsSimple data constructor), and I observe this:

  1) Network.HTTP.Req.redirects follows redirects
       uncaught exception: HttpException
       VanillaHttpException (HttpExceptionRequest Request {
         host                 = "localhost"
         port                 = 1234
         secure               = False
         requestHeaders       = []
         path                 = "/redirect-to"
         queryString          = "?url=https%3A%2F%2Fhttpbin.org"
         method               = "GET"
         proxy                = Nothing
         rawBody              = False
         redirectCount        = 10
         responseTimeout      = ResponseTimeoutDefault
         requestVersion       = HTTP/1.1
         proxySecureMode      = ProxySecureWithConnect
       }
        (InternalException (HandshakeFailed Error_EOF)))

https://github.com/mrkkrp/req/actions/runs/9155853746/job/25169229880?pr=169#step:9:217

This is a test that used to pass. Do you think it is related? If so, am I doing something wrong? Here is the change I'm testing:

https://github.com/mrkkrp/req/pull/169/files

[bfrk]

I think this is indeed related. Passing def to TLSSettingsSimple does not work as you intend it to, since this will be the value that the tls package provides as default and that has an empty list of supported ciphers. You can either use the def that crypton-connection suplies for TLSSettings and modify that, which is a bit of a pain because of the nested record update needed. Or you can manually override supportedCiphers with Network.TLS.Extra.ciphersuite_default.

[bfrk]

See e.g. https://hub.darcs.net/darcs/darcs-screened/browse/src/Darcs/Util/HTTP.hs#168 for the painful way.

[kazu-yamamoto]

def of Supported in tls is defaultSupported:

defaultSupported :: Supported
defaultSupported =
    Supported
        { supportedVersions = [TLS13, TLS12]
        , supportedCiphers = []
        , supportedCompressions = [nullCompression]
        , supportedHashSignatures = Struct.supportedSignatureSchemes
        , supportedSecureRenegotiation = True
        , supportedClientInitiatedRenegotiation = False
        , supportedExtendedMainSecret = RequireEMS
        , supportedSession = True
        , supportedFallbackScsv = True
        , supportedEmptyPacket = True
        , supportedGroups = supportedNamedGroups
        }

Unfortunately, supportedCiphers is [].
So, you need to set it by yourself.

You might ask why supportedCiphers is not ciphersuite_default.
The answer is that the original author of tls designed that Cipher is extensible.
No Cipher is available when defaultSupported is defined.

But I guess this design can be changed.
That is, we probably implement that ciphersuite_default is available when defaultSupported is defined while Cipher is extensible.

If users want this, I will take time to try to implement it.

[kazu-yamamoto]

It appeared that this change is quite easy.
See the PR above.
@mrkkrp If you agree, I will release a new version of tls which automatically fixes your issue.

[mrkkrp]

Hi @kazu-yamamoto I have no option on the design of the library since I am not familiar with it. That said, of course it would be nice if the problem went away "automatically" :-)