feat: TLS config option availability using the client options

[ssi-anik]

Description

This PR allows developers to provide the TLS config during the client creation via the client options.

Issues

• extends: Make SSL Certificate Verification Optional #2

[aldy505]

Curious, wouldn't it possible to set the *tls.Config from ClientOptions.HTTPClient?

[ssi-anik]

@aldy505 yes, it could be done. But setting the value using that HttpClient will require more lines. Also, allowing the package to choose the defaults rather than having the task done by the developer.

[giortzisg]

I'm a bit doubtful with the actual usefulness of this and don't really want to populate the clientOptions with so many conflicting options. Currently TlsConfig completely overrides CaCerts, which might be confusing to users, so even if we want to add the option we need to:

• either merge those two
• deprecate CaCerts and only use TlsConfig

In general the plan at some point is to deprecate the current transport and use the new one, when it would make sense to simplify the httpConfig to actually make sense and make all the breaking changes then.

[ssi-anik]

@giortzisg Thanks for your concern.

Firstly, in my case, the use case of TlsConfig is that I can provide the value of InsecureSkipVerify with minimal effort. There are scenarios where fixing the SSL of the domain might take longer than expected. Without merging this change, someone will have to use HTTPTransport. Rather, I want to let the module decide the instantiation of HTTPTransport during the init process.

As the CaCerts config is a part of the TlsConfig, after merging this PR, the existing implementation will continue to work. As TlsConfig's default is nil, it will fall back to the CaCerts section and will use that value. Otherwise, whoever needs to set the TlsConfig, just like I need, will set the value, and TlsConfig will take precedence over the CaCerts.

In general the plan at some point is to deprecate the current transport and use the new one, when it would make sense to simplify the httpConfig to actually make sense and make all the breaking changes then.

Yes, when trying to include breaking changes, you might do things like that. But IMO, it was the least change someone has to add to get their application running as soon as they figure it out, just like I did.

[giortzisg]

TlsConfig will take precedence over the CaCerts.

Let's at least try and merge the two in case that TlsConfig exists without any CaCerts

[ssi-anik]

Let's at least try and merge the two in case that TlsConfig exists without any CaCerts

Did you mean to say that if the developer provides both the TlsConfig and CaCerts, then CaCerts will be injected into the TlsConfig even if the developer intentionally sets any value in CaCerts inside the TlsConfig? Did I get your point right?

However, my thought was that if the developer is adding TlsConfig, they already know that they can skip the value of CaCerts in the root config, and add it directly to the TlsConfig. For me, it made more sense.

[giortzisg]

then CaCerts will be injected into the TlsConfig

if TlsConfig already specifies CaCerts then the other option gets ignored, but if TlsConfig doesn't and CaCerts does, we need to merge them. So the precedence would go from:
Client -> Transport -> TlsConfig -> CaCerts

[ssi-anik]

then CaCerts will be injected into the TlsConfig

if TlsConfig already specifies CaCerts then the other option gets ignored, but if TlsConfig doesn't and CaCerts does, we need to merge them. So the precedence would go from: Client -> Transport -> TlsConfig -> CaCerts

Added those changes mention in your comment.

[sentry]

Bug: The getTLSConfig function mutates the user-provided TlsConfig object by directly assigning to its RootCAs field, which can cause unexpected side effects and race conditions.
_{Severity: HIGH}

Suggested Fix

Instead of mutating the input options.TlsConfig, create a deep copy of it. Then, merge the CaCerts into the RootCAs field of the new, copied tls.Config object. This avoids modifying the user's original configuration object.

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: transport.go#L64-L66

Potential issue: The `getTLSConfig` function modifies the input `ClientOptions` by
assigning `options.CaCerts` to `options.TlsConfig.RootCAs`. This mutation of a
user-provided object is an unexpected side effect. If a user reuses the same
`ClientOptions` or `TlsConfig` object to configure multiple clients, the configuration
will be silently changed. Furthermore, if multiple clients are configured concurrently
with the same options object, this could introduce a race condition on the `RootCAs`
field, leading to an inconsistent state.

_{Did we get this right? 👍 / 👎 to inform future reviews.}

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.}

[cursor]

User-provided TlsConfig unexpectedly mutated during merge

Medium Severity

The getTLSConfig function directly mutates the user-provided TlsConfig.RootCAs field when merging with CaCerts. Since TlsConfig is a pointer, this unexpectedly modifies the caller's original tls.Config object. If users reuse their TLS config elsewhere, this side effect could cause subtle and confusing behavior. The function could use options.TlsConfig.Clone() to create a copy before modifying it.