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

[improvement] Use OS CA certs when possible #1432

Open
captn3m0 opened this Issue Apr 12, 2019 · 7 comments

Comments

Projects
None yet
2 participants
@captn3m0
Copy link

commented Apr 12, 2019

  • Insomnia Version: 6.3.2.1
  • Operating System: Linux 5.0.7-arch1-1-ARC

Details

Ref:

if (process.platform !== 'darwin') {
const baseCAPath = getTempDir();
const fullCAPath = pathJoin(baseCAPath, CACerts.filename);
try {
fs.statSync(fullCAPath);
} catch (err) {
// Doesn't exist yet, so write it
mkdirp.sync(baseCAPath);
fs.writeFileSync(fullCAPath, CACerts.blob);
console.log('[net] Set CA to', fullCAPath);
}
setOpt(Curl.option.CAINFO, fullCAPath);
}

libcurl on linux gives the same functionality, and there should be no need to rely on a insomnia-provided CA bundle.

@welcome

This comment has been minimized.

Copy link

commented Apr 12, 2019

👋 Thanks for opening your first issue! If you're reporting a 🐞 bug, please make sure
you include steps to reproduce it. If you're requesting a feature 🎁, please provide real
use cases that would benefit. 👪

To help make this a smooth process, please be sure you have first read the
contributing guidelines.

@gschier

This comment has been minimized.

Copy link
Collaborator

commented Apr 12, 2019

Do you know if this is this new curl behavior? A few years ago this logic was definitely required and was the suggested way of doing in by the libcurl docs. It's the reason they provide an up-to-date CA bundle https://curl.haxx.se/docs/caextract.html

I can't seem to find any mention of this requirement in the docs anymore but I also cannot find any information on the change.

Also remember, Insomnia does not use the system's libcurl on Linux and Windows. It instead bundles its own to prevent dependency issues.

@gschier gschier changed the title Don't write root CA certs for linux [question] Are bundled CA certificates still needed? Apr 12, 2019

@captn3m0

This comment has been minimized.

Copy link
Author

commented Apr 12, 2019

I meant that it comes bundled with every linux distro these days, and insomnia on linux should set up the ca-certificates package as a dependency instead of bundling its own.

I thought that this must be standardized in some way, but it doesn't look like it.

Node seems to come with it hardcoded though: https://github.com/nodejs/node/blob/master/src/node_root_certs.h

Wondering if that could be used instead? Not present on all node installations.

(I'm sure there is a NPM package somewhere that finds the correct path for system root CA certs, but I can't find it)

@gschier

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2019

Based on that it seems like the best idea might be to check those common directories and fallback to the bundled CAs if they don't exist. We could also make the path configurable in the app preferences.

Let me know if you want to take this one on and I can point you in the right direction (if needed).

@gschier gschier added Accepted and removed Needs Discussion labels Apr 16, 2019

@gschier gschier changed the title [question] Are bundled CA certificates still needed? [improvement] Use OS CA certs when possible Apr 16, 2019

@captn3m0

This comment has been minimized.

Copy link
Author

commented Apr 16, 2019

Sounds good. I'll wrap it into a separate NPM package if that makes sense (or if I can't find one).

Just checking, but this wouldn't affect #675 right?

@gschier

This comment has been minimized.

Copy link
Collaborator

commented Apr 18, 2019

Yes, it should not affect #675. Unless you are also planning on making it configurable too. In that case, you'd be able to close #675.

@captn3m0

This comment has been minimized.

Copy link
Author

commented Apr 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.