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

feature: use native-tls in ureq requests to trust system TLS certs #2988

Merged
merged 1 commit into from
Dec 21, 2024

Conversation

adtac
Copy link
Contributor

@adtac adtac commented Dec 20, 2024

@skeptrunedev @cdxker

This PR makes ureq use the native-tls feature so that the distro-supplied TLS certificates are trusted.

The following change captures the gist of this PR:

image

I believe all instances of ureq::get, ureq::post, etc. have been changed, but if there's anything missing / if this PR doesn't match your code style, feel free to close it and incorporate the changes in a new PR :)

@cdxker
Copy link
Member

cdxker commented Dec 20, 2024

Great, going to build and test to see if this works with subtrace enabled now! Should we also up our subtrace version in this pr?

@adtac
Copy link
Contributor Author

adtac commented Dec 20, 2024

Nope, everything should work with the old version of subtrace. You can remove the -tls=false, of course ;)

@adtac
Copy link
Contributor Author

adtac commented Dec 20, 2024

(forgot a couple of use std::sync::Arc statements, amended the commit)

@cdxker
Copy link
Member

cdxker commented Dec 20, 2024

@adtac In this project we use both ureq and reqwests do we need to do something similar here for reqwests?

@adtac
Copy link
Contributor Author

adtac commented Dec 20, 2024

reqwest works fine with subtrace, it's just ureq that causes problems with its bundled TLS certs (from rustls)

@cdxker
Copy link
Member

cdxker commented Dec 20, 2024

Great, just made a few modifications to fix all the rust errors, can you cherry-pick this commit. From 505ef43

505ef43

@adtac
Copy link
Contributor Author

adtac commented Dec 20, 2024

cherry picked, rebased onto latest main

@adtac
Copy link
Contributor Author

adtac commented Dec 20, 2024

fixed the unused std::sync::Arc error

@cdxker
Copy link
Member

cdxker commented Dec 20, 2024

Just built a docker image @ trieve/server:native-tls-subtrace . I don't have time to test right now, but noting the image tag here for when I have time to evaluate.

Also note that the default run command is not subtrace. You will need set your run command to this.

{{- if .Values.config.trieve.useSubtrace }}
securityContext:
capabilities:
add: ["SYS_PTRACE"]
command:
- "./subtrace-linux-amd64"
- "run"
- "-tls=false"
- "--"
- "/app/trieve-server"
{{- end }}

@adtac adtac force-pushed the adtac/native-tls branch 2 times, most recently from 434524e to f7802e4 Compare December 20, 2024 02:13
skeptrunedev
skeptrunedev previously approved these changes Dec 20, 2024
@adtac
Copy link
Contributor Author

adtac commented Dec 20, 2024

@cdxker
Copy link
Member

cdxker commented Dec 20, 2024

I believe d9172d7 will fix > https://github.com/devflowinc/trieve/actions/runs/12424254684/job/34689358734?pr=2988

Yes, it should can you add that into this PR. Testing environment works with Subtrace!!!

@adtac adtac force-pushed the adtac/native-tls branch 4 times, most recently from 46351de to 573c46b Compare December 21, 2024 00:14
@cdxker cdxker requested review from cdxker and removed request for skeptrunedev December 21, 2024 00:18
@cdxker cdxker merged commit dd1183c into devflowinc:main Dec 21, 2024
3 checks 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.

3 participants