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 http-client-openssl instead of http-client-tls #137

Merged
merged 2 commits into from
Dec 27, 2023

Conversation

jfroche
Copy link
Contributor

@jfroche jfroche commented Dec 20, 2023

Using http-client-openssl that also handle the SSL_CERT_FILE environment variable

refs #99

Using http-client-openssl that also handle the SSL_CERT_FILE environment variable

refs channable#99
{ osslSettingsVerifyMode =
if not $ getOptionsValue oValidateCerts opts
then VerifyNone
else VerifyPeer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use record syntax here to make it more clear what these settings do?

package.yaml Outdated
@@ -28,7 +30,7 @@ dependencies:
- utf8-string
- optparse-applicative

ghc-options: -Wall -Werror
ghc-options: -threaded -rtsopts -Wall -Werror
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain why these extra options are necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not an expert in this openssl library but without the -threaded option we face
ConnectionFailure user error (RTS doesn't support multiple OS threads (use ghc -threaded when linking)) at run time when trying to connect to vault. So it seems that openssl requires a threaded runtime.

-rtsopts enable all RTS options processing and can be configured at runtime through command line or environment variable. It enables users to configure the number of threads, the heap size, ... We could remove it until we know that we really need it as it might have security implications.

Copy link
Contributor

@HugoPeters1024 HugoPeters1024 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for taking the time to make a contribution :)

LGTM!

@HugoPeters1024 HugoPeters1024 merged commit 3e4cb41 into channable:master Dec 27, 2023
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