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

fix: Add TLS support, and allow changing TLS backend. #9

Merged
merged 1 commit into from
Jun 9, 2024

Conversation

airblast-dev
Copy link
Contributor

With the default features being disabled for the reqwest dependency in this commit 398e498, a scheme error is raised by the request client when sending any request with ClientOptions.secure set to true.

This enables TLS support by default and allows changing the backend via features.

@airblast-dev airblast-dev marked this pull request as draft June 8, 2024 07:38
@airblast-dev airblast-dev force-pushed the tls-support branch 4 times, most recently from 65b72a6 to 671ec40 Compare June 8, 2024 08:22
@airblast-dev
Copy link
Contributor Author

The workflow is failing due to pkg-conf, and libssl-dev being missing.

We can either use reqwest/rustls-tls, and disable reqwest/native-tls since rustls-tls doesn't require the packages mentioned above. Or we can simply install said packages.

I will take a look into how .woodpecker.yaml works, and install said packages. This is likely the better solution as the default reqwest features are likely what people would expect. And where that isn't what they want they can simply change the features and use reqwest/rustls-tls package instead.

@SleeplessOne1917
Copy link
Member

Thanks for opening this. I've been using this library exclusively for local development, so I hadn't been in a situation where I could encounter this issue.

@airblast-dev
Copy link
Contributor Author

airblast-dev commented Jun 8, 2024

Simplest solution is going to be to just install the packages before testing. If anyone has the interest they can use a workspace and do a few tricks to optimize it.

I have spent quite a bit of time trying to set up local instances to test around with the workflow but to no success. Meaning I can only test stuff in a debian docker container locally.

@SleeplessOne1917
Copy link
Member

It could be worth just sticking to 1 way of doing TLS so it doesn't rely on having a specific package installed. If I understand correctly, rustls lets us do that?

@airblast-dev
Copy link
Contributor Author

Thats correct and definitely an option. However I think keeping the default reqwest TLS backend is a better idea as it is likely to get better testing. The native solution also is generally a battle hardened backend which is used widely in many applications.

Either way its your choice. 👍

@airblast-dev airblast-dev marked this pull request as ready for review June 8, 2024 19:30
@SleeplessOne1917
Copy link
Member

Default should do the trick, though I'm having trouble finding a list of required system dependencies for it across distros/operating systems.

@airblast-dev
Copy link
Contributor Author

I agree that is a bit problematic. I would recommend reading seanmonstar/reqwest#2025 as they mention similar issues on the rustls side as well. When it comes to system dependencies the failed build does give out an appropriate error message for both dependencies in the case they are missing. It is also worth mentioning that most systems will not need an extra step anyway. This includes the older test image with Alpine.

In short I think sticking with the default and providing options is the way to go considering both have various reasons to prefer one over the other.

@SleeplessOne1917 SleeplessOne1917 merged commit 83fe72d into LemmyNet:main Jun 9, 2024
1 check passed
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