Support NODE_EXTRA_CA_CERTS
#11724
Support NODE_EXTRA_CA_CERTS
#11724
Conversation
Respects the Node.js API and loads the CA file pointed to by the environment variable NODE_EXTRA_CA_CERTS into the global SSL context.
|
❌ @LudvigHz, your commit has failing tests :( 💪 1 failing tests Darwin AARCH64
💻 1 failing tests Darwin x64 baseline
💻 1 failing tests Darwin x64
🐧💪 1 failing tests Linux AARCH64
🐧🖥 2 failing tests Linux x64 baseline
🐧🖥 1 failing tests Linux x64
🪟💻 2 failing tests Windows x64 baseline
🪟💻 1 failing tests Windows x64
|
| var extra_cas: ?std.ArrayList([*c]const u8) = null; | ||
|
|
||
| blk: { | ||
| if (std.process.getEnvVarOwned(default_allocator, "NODE_EXTRA_CA_CERTS")) |extra_ca_certs_path| { |
There was a problem hiding this comment.
This should use something in DotEnv.Loader and be cached there. We will need to add support for this in several places instead of only in the http client.
There was a problem hiding this comment.
Okey, that was the main thing I was unsure about. I initially looked at going that route, but it ended up being kind of complicated with the current architecture of how/when the HTTPThread is initialized.
But then I'll move this there instead and I will have to change the initialization of the HTTPThread to depend on DotEnv.Loader. I do agree it's also better option devex wise.
If you have any ideas on how to best get the ca file contents into the context of the https context initialization, I'd happily take some pointers as well 😅 (the first/only way I saw how to do this involved passing the dotenv context around alot)
|
|
||
| blk: { | ||
| if (std.process.getEnvVarOwned(default_allocator, "NODE_EXTRA_CA_CERTS")) |extra_ca_certs_path| { | ||
| const file = bun.openFile(extra_ca_certs_path, .{ .mode = .read_only }) catch |err| { |
There was a problem hiding this comment.
There are way too many ways to read files in Bun's codebase which is very confusing (not your fault at all)
Instead of this, let's do:
const contents = bun.sys.File.readFromUserInput(std.fs.cwd(), extra_ca_certs_path, bun.default_allocator)And not in here, instead in DotEnv.Loader
| @@ -0,0 +1,17 @@ | |||
| (async () => { | |||
There was a problem hiding this comment.
IIFEs like this are unnecessary in Bun. Bun will use ESM by default.
| const run = async (done, env, func: (proc: Subprocess<"pipe">) => Promise<void>) => { | ||
| const server = https.createServer( | ||
| { | ||
| key: nodefs.readFileSync(path.join(import.meta.dir, "fixtures", "openssl_localhost.key")), |
There was a problem hiding this comment.
Looks like these files were not committed?
There was a problem hiding this comment.
Whoops, I initially based this off of #11322. I'll rebase now that that's merged and we can reuse the same certs.
There was a problem hiding this comment.
@LudvigHz
Is there any update for the status of this PR?
🙏
There was a problem hiding this comment.
I haven't had the time to work on it lately. And as I'll need to refactor alot of the http initialization it's going to require quite a bit more work than what this PR proposes currently.
But hopefully I'll have some free time soon to look at it some more :)
|
Closing due to inactivity, but feel free to send another if you get the time |
What does this PR do?
Load ca certificates from NODE_EXTRA_CA_CERTS
Respects the Node.js API and loads the CA file pointed to by the environment variable NODE_EXTRA_CA_CERTS into the global SSL context.
This works for fetch, https.request and (should also cover) requests made by the bun CLI, f.ex
bun install.I haven't added tests for
bun installsince there isn't really existing test infra for bun install network activity.How did you verify your code works?
I wrote automated tests
If Zig files changed:
bun-debug test test-file-name.test)A couple notes on this:
HTTPThreadis initialized before the envLoader, and it would require quite a bit of rebasing to get the envMap context into theHTTPContextinitialization. The way it's done now, it requires minimal code changes while still being compatible with Node.js.Fixes #7124
Fixes #7200
Fixes #271