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

Support adding custom CA #454

Merged
merged 4 commits into from
Aug 19, 2023
Merged

Support adding custom CA #454

merged 4 commits into from
Aug 19, 2023

Conversation

zynaxsoft
Copy link
Contributor

Resolve #443

Inspired by Node.js setting, I named the env var to be TAPLO_EXTRA_CA_CERTS (plural here because one file could contain many certs).

I extracted the getting reqwest client part as a function though I don't know exactly where the best location for this function is. I temporarily put it in taplo-common/util.rs.

I tested the build on my local environment described in the issue and it worked perfectly.

to: @ia0
How should we document this?
I was thinking about making the error more obvious and adding a suggestion to let the user know they need to add the environment variable. Though I don't really know how to match the UnknownIssuer error from reqwest.

Custom CA could be specified with env var `TAPLO_EXTRA_CA_CERTS`.
Copy link
Collaborator

@ia0 ia0 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR! It looks good to me.

Regarding where to put the reqwest builder function, I think that's the right place.

Regarding documentation, I would say site/site/configuration/using-schemas.md might be appropriate since it seems this is mostly useful for the schema catalog download. A sentence at the end of the page saying that the root certificate is customizable with the TAPLO_EXTRA_CA_CERTS environment variable for both the LSP and CLI is probably enough.

Regarding the error, I think using tracing should be enough like one of my comment shows. At least in Emacs it's easy to access the stderr of the LSP server. And when using the CLI it's directly visible. We could in theory improve the LSP case by producing a diagnostic or something, but that's probably overkill.

Thanks again for your work!

crates/taplo-common/src/util.rs Outdated Show resolved Hide resolved
crates/taplo-common/src/util.rs Outdated Show resolved Hide resolved
crates/taplo-common/src/util.rs Show resolved Hide resolved
@zynaxsoft
Copy link
Contributor Author

I love your suggestions and I have modified the code according to your comments!

Copy link
Collaborator

@ia0 ia0 left a comment

Choose a reason for hiding this comment

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

Thanks!

@ia0 ia0 requested a review from panekj August 13, 2023 08:44
@panekj panekj merged commit 7f2fba1 into tamasfe:master Aug 19, 2023
4 checks passed
@paholg paholg mentioned this pull request Mar 5, 2024
13 tasks
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.

Add support for custom root certificate
3 participants