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

Add support for custom root certificate #443

Closed
zynaxsoft opened this issue Jul 27, 2023 · 5 comments · Fixed by #454
Closed

Add support for custom root certificate #443

zynaxsoft opened this issue Jul 27, 2023 · 5 comments · Fixed by #454

Comments

@zynaxsoft
Copy link
Contributor

Request

Add an ability to define a custom root certificate for querying a schema from the Internet.

Motivation

In my environment, it's necessary to use a custom root certificate.
I am using Taplo as a language server with Neovim. Here is what I would get when I opened a Cargo.toml file.
An error from Neovim :LspLog

taplo:configuration_change:initialize: failed to fetch catalog error=error sending request for URL 
(https://www.schemastore.org/api/json/catalog.json): error trying to connect: invalid peer certificate contents: invalid peer certificate: UnknownIssuer\n\nCaused by:
    0: error trying to connect: invalid peer certificate contents: invalid peer certificate: UnknownIssuer
    1: invalid peer certificate contents: invalid peer certificate: UnknownIssuer 
     self.root=file:///...
    OR taplo:configuration_change:initialize: failed to add schemas from catalog error=error sending request for URL 
(https://www.schemastore.org/api/json/catalog.json): error trying to connect: invalid peer certificate contents: invalid peer certificate: UnknownIssuer self.root=file:///...
"

I have already setup my OS to trust the custom certificate. Also it seems that this crate uses reqwest to do the query.
I believe reqwest doesn't get the list of root certificates from the OS.

@ia0
Copy link
Collaborator

ia0 commented Aug 8, 2023

Thanks for the diagnosis. But if I understand correctly, shouldn't this be reported to reqwest instead?

@zynaxsoft
Copy link
Contributor Author

Thank you for picking this up!

I believe the current trend of implementing HTTPS client is to not obey what an OS says about the root CA they trust, but let the application/user side specifically adds a custom root CA by themselves.

reqwest already supports this with add_root_certificate

I am not sure if this is the correct place for the implementation but I see in this line, this uses the default built-in root CAs which means it doesn't use the OS setting.

@ia0
Copy link
Collaborator

ia0 commented Aug 9, 2023

I see, so you essentially want to add .add_root_certificate(certificate) here:

client = reqwest::Client::builder()
.timeout(Duration::from_secs(10))
.build()

The question is where do you get certificate. I can see at least 2 sources: a flag or an environment variable (or a flag that defaults to an environment variable). In the case of a flag, the question is how this would be made available in WorkspaceState::new(). I see at least 2 options: a function parameter or a global variable.

I think the simplest solution would be to just use an environment variable (or use a flag with a global variable). Would you be able to write a PR? If not, I could take a look this weekend, but I'll need you to be able to test it or provide instructions on how to test it.

@zynaxsoft
Copy link
Contributor Author

Yes, that's what I wanted to do essentially. To confirm my understanding, does the flag you mentioned mean something that is passed from the LSP configuration? (I am thinking about nvim-lspconfig#taplo).

Environment variable could work too and I agree it would be the simplest way to do it. Though I am not really sure about the design. I guess we could do both.

On the other hand, do we need to also implement this for taplo-cli for consistency?

I am willing to help and write a PR, but I would need some pointers on where to look.

@ia0
Copy link
Collaborator

ia0 commented Aug 10, 2023

Does the flag you mentioned mean something that is passed from the LSP configuration?

Actually I had it wrong, we need 2 things:

And we also need to update the reqwest::Client in taplo-cli too:

let http = reqwest::Client::builder()
.timeout(std::time::Duration::from_secs(5))
.build()

I guess we could do both.

Yes, but I'm not sure it's worth the effort.

On the other hand, do we need to also implement this for taplo-cli for consistency?

Yes, I think we need to do that.

I am willing to help and write a PR, but I would need some pointers on where to look.

Great! Thanks a lot! Let's only do the environment variable. You essentially need to:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants