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: Add tls-native, tls-rustls features to allow building w/o Rustls #110

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jirutka
Copy link

@jirutka jirutka commented Dec 30, 2023

No description provided.

@simonsan simonsan added C-enhancement Category: New feature or request S-blocked Status: Blocked from merging/working on due to some issue A-dependencies Area: Related to updating dependencies labels Dec 31, 2023
@simonsan
Copy link
Contributor

Hey there, we are currently working on a relatively big refactor to an own rustic_backend crate, see #73.

We need to focus first on that one, before we are being able to merge this.

@simonsan
Copy link
Contributor

Also regarding the topic, as there has been no issue, which needs to be fixed, and we haven't discussed it, could you elaborate a bit on the background:

  • What's your use case?
  • Why is this change needed?
  • What does it improve/solve?

@aawsome
Copy link
Member

aawsome commented Dec 31, 2023

Agree to @simonsan - when the backend refactor is done, the TLS things will be all in the crate rustic_backends, so we better wait for the refactoring #73 to land!

Besides this, if there are use cases where you need other TLS options, I would very much agree to adding the possibilites as features. It might be that the depending crate(s) (e.g. opendal) need to support those features, too...

@jirutka
Copy link
Author

jirutka commented Dec 31, 2023

What's your use case?

Packaging rustic for Alpine Linux.

Why is this change needed and what does it improve/solve?

Bundling (static linking) TLS library is a very bad idea from a security perspective, and Linux distributions try to avoid it. Also, rustls/ring doesn’t support all platforms supported by Rust, e.g. ppc64le and s390x.

@simonsan
Copy link
Contributor

simonsan commented Jan 5, 2024

@jirutka if the features are mutually exclusive, could you throw a compile error in lib.rs, in case both are turned on?

#[cfg(all(feature = "tls-native", feature = "tls-rustls "))]
compile_error!("feature \"tls-native\" and feature \"tls-rustls \" cannot be enabled at the same time. Please disable one of them.");

@jirutka
Copy link
Author

jirutka commented Jan 5, 2024

if the features are mutually exclusive, could you throw a compile error in lib.rs, in case both are turned on?

Sure, done.

src/lib.rs Outdated Show resolved Hide resolved
algitbot pushed a commit to alpinelinux/aports that referenced this pull request Jan 7, 2024
@aawsome
Copy link
Member

aawsome commented Jan 17, 2024

Now that #73 is merged, this PR can continue...
@jirutka: Your changes should now go to crates/backend. Can you move this or do you need support?

Also, now that we have added opendal, this should be also treated - however, I don't know if this is already supported by opendal; this may need a bit of research.

@aawsome aawsome removed the S-blocked Status: Blocked from merging/working on due to some issue label Jan 17, 2024
@simonsan simonsan marked this pull request as draft September 30, 2024 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependencies Area: Related to updating dependencies C-enhancement Category: New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants