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

feat: Expose cURL's --cacert and --capath, along with --insecure #60

Merged
merged 8 commits into from
Oct 31, 2023

Conversation

Eta0
Copy link
Contributor

@Eta0 Eta0 commented Oct 31, 2023

Customizing cURL Certificate Verification

This PR builds off of #59 by @spmkone and closes #58.

Additions

  • stream_io.open_stream and stream_io.CURLStreamFile now accept an additional, optional certificate_handling argument to customize the verification of SSL certificates
    • This corresponds to the flags --cacert, --capath, and -k/--insecure in curl
    • Customization is achieved by passing an instance of stream_io.CAInfo to open_stream or the CURLStreamFile constructor
    • Example usages:
      • open_stream("https://localhost/model.tensors", certificate_handling=CAInfo(cacert="./localhost.pem")
      • open_stream("https://127.0.0.1/model.tensors", certificate_handling=CAInfo(allow_untrusted=True)
    • Pass certificate_handling=None (the default) to use default certificate verification as compiled into cURL

Changes from #59

  • This adds more secure alternatives to --insecure when using self-signed certificates, to encourage better security practices
  • allow_insecure has been removed as a direct parameter to open_stream and friends, and is instead a CAInfo option
  • Test cases cover using customized CAInfo configurations with self-signed certificates in moto
  • Fixed a bug where only S3 and not raw HTTPS downloads were using open_stream's certificate verification configuration

Bonus

Error messages in the CURLStreamFile constructor have been improved in the case where cURL terminates at startup with no other output (e.g. when SSL certificate verification fails) by querying the subprocess exit code and attaching it as supplementary information, if it is nonzero.

@Eta0 Eta0 added the enhancement New feature or request label Oct 31, 2023
@Eta0 Eta0 requested a review from wbrown October 31, 2023 00:48
@Eta0 Eta0 self-assigned this Oct 31, 2023
Copy link
Collaborator

@wbrown wbrown left a comment

Choose a reason for hiding this comment

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

Stray print() -- other than that, looks pretty good except ...

We are a Kubernetes shop -- if someone wants to pass in a certificate via Kubernetes secrets, which may be an env var, or such, we may want to:

  • allow passing a cert as a handle or a string
  • allow passing in a cert where the contents are stored in an env var

I'll approve this, as I don't want any further work on this at the moment, but we should write this up as an issue.

tensorizer/stream_io.py Outdated Show resolved Hide resolved
@Eta0 Eta0 merged commit 860482e into main Oct 31, 2023
4 checks passed
@Eta0 Eta0 deleted the eta/curl-ca branch October 31, 2023 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support self-signed certificates in CURLStreamFile
3 participants