feat: Support a NEXTCLADE_EXTRA_CA_CERTS environment variable and --extra-ca-certs option

[tsibley]

Updated to split into two parts:

1. feat: Switch to platform-specific TLS/SSL certificate verification #1529 to use the platform's certification verification, including system-level trust store
2. This PR to add support for configuration of extra CA certificates to trust.

---

Previously

See commit messages: 8816fe2 and 4005505.

Motivated by having to write:

There is currently no way to configure or modify the trust store without modifying the Nextclade source code.

in the recent CA certificates doc I wrote.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
nextclade	✅ Ready (Inspect)	Visit Preview	Oct 15, 2024 6:43am

[tsibley]

The macOS and Windows build is failing because rustls_platform_verifier::Verifier::new_with_extra_roots() seems to be available on a platform-specific basis or something? Hmm, I'll dig. This might have been docs.rs misleading me with Linux-specific docs rather than cross-platform/universal docs. (The Linux builds work fine for me and seemingly in CI too.)

The clippy linting fails due to issues in wasm-bindgen, which was fixed with 0.2.93. I was hesitant to upgrade wasm-bindgen more than necessary (i.e. beyond 0.2.87 → 0.2.89) because it's a ZeroVer package without any breakage guarantees and I didn't feel well-placed to evaluate the impact of many of the changes. But I guess if we want Clippy to be happy we gotta?

[tsibley]

The macOS and Windows build is failing because rustls_platform_verifier::Verifier::new_with_extra_roots() seems to be available on a platform-specific basis or something?

Ah ha. It is indeed Linux-only as of rustls-platform-verifier 0.3.4 (the latest release) but is being extended to other platforms too: rustls/rustls-platform-verifier#58. Implementations for macOS and Windows are landed but not yet released.

[tsibley]

If we don't want to wait for a new release of rustls-platform-verifier (nor build from GitHub), we can take just the first commit in this PR (8816fe2) and land the second commit later.

[ivan-aksamentov]

Thank you very much Tom!

I just pushed a few commits, mostly cosmetic/pedantic stuff.

[ivan-aksamentov]

I cross compile all things on Linux, including versions for Windows (through mingw) and mac (through osxcross). So any platform-specifics might be pretty difficult to handle, even if they are supported. Let's hope that their planned implementations will work in cross environments too.

For the time being we could extract the Linux code to a function and slap conditional compilation to it. What do you think?

90% of CLI downloads are for Linux. I guess there might be some Windows and Mac users, but having this at least for Linux is already a big win, I think!

[ivan-aksamentov]

Does it make sense to also add a CLI flag, just in case? #1528
Please merge-in if you like it.

This also allows us to make the feature more visible and document it right in the help screen.

What is the usual convention: flag has priority over env var or other way around?

[ivan-aksamentov]

I think we are doing good already. But the following is just a few ideas to harmonize with Nextstrain.

Should we also check for REQUESTS_CA_BUNDLE (mentioned in Nextstrain docs) and perhaps CURL_CA_BUNDLE?

Explicit Nextclade-specific NEXTCLADE_EXTRA_CA_CERTS var makes sense for direct users, but from Nextstrain user's perpective where Nextclade might not even appear directly, having to track a whole zoo of env vars might not be be perfect.

Should we try to cover the most widespread ones? Or is it looking for troubles?

[ivan-aksamentov]

When considering a CLI flag in #1528, what are the possible good names for the flag to harmonize it across Nextstrain and perhaps also with curl? Looks like curl accepts --with-ca-path which accepts a directory path.

[ivan-aksamentov]

It seems that there could be multiple certs in the file, and when the libraries process it, some certs could fail. Currently the entire program fails if one cert fails to load.

We could consider skipping failing certs, if that makes sense and is safe, though I cannot immediately evaluate either.

One disadvantage of silently failing certs is that it will be unclear for the user that e.g. a single cert in a file fails. It will probably only manifest in connection errors later. To remediate, we could emit a warning for each failing cert.

[ivan-aksamentov]

We could also consider to bubble the check for presence for env var (and/or cli arg) up and only run the new code path conditionally if the CA certs are provided. Just to be extra safe that this does not affect the old code path, which most users use (especially considering the 0.x nature of all of these libs).

I might do this and other things I mentioned above in the followup PRs.

[tsibley]

Let's hope that their planned implementations will work in cross environments too.

As far as I saw, implementing Verifier::new_with_extra_roots() for the other platforms we care about doesn't mean new libraries or new platform-specific integrations, but just some more Rust code to call existing libraries and use existing integrations. Point is, if you can cross-build rustls-platform-verifier 0.3.4 now, then I think you'll be fine.

For the time being we could extract the Linux code to a function and slap conditional compilation to it. What do you think?

I guess, but I'd be surprised if NEXTCLADE_EXTRA_CA_CERTS only worked on Linux?

I'd probably just drop it for now, leaving the new system-level trust store support in place, as that's a big enough win by itself. Linux users will still have other ways of adding extra CA certs (e.g. via OpenSSL's SSL_CERT_FILE).

Does it make sense to also add a CLI flag, just in case? #1528

Sure.

This also allows us to make the feature more visible and document it right in the help screen.

+1 for doc there. I was gonna ask where this should got doc'd.

Relatedly: should I add tests for this (or at least the env var)? Where would those go? The few Cram tests I saw look unused, but that's the kind of thing I'd reach for with this particular feature.

What is the usual convention: flag has priority over env var or other way around?

Yes, usually argument overrides env var, as its more specific to a single invocation. Env vars can be thought of overriding or setting the defaults.

Should we also check for REQUESTS_CA_BUNDLE (mentioned in Nextstrain docs) and perhaps CURL_CA_BUNDLE?

Explicit Nextclade-specific NEXTCLADE_EXTRA_CA_CERTS var makes sense for direct users, but from Nextstrain user's perpective where Nextclade might not even appear directly, having to track a whole zoo of env vars might not be be perfect.

Should we try to cover the most widespread ones? Or is it looking for troubles?

Agreed a zoo of env vars is not perfect, but that's reality. I think it's looking for troubles. It's surprising to me to have software which goes looking for env vars expected by other software (e.g. REQUESTS_CA_BUNDLE). The best way to avoid a zoo is to add certs to the system trust store and expect/configure everything to use that.

If we want to insulate Nextstrain users specifically (though I don't think we need to here), I'd instead advocate for a specific NEXTSTRAIN_EXTRA_CA_CERTS and adapting all our software to DTRT with it.

So any platform-specifics might be pretty difficult to handle, even if they are supported. Let's hope that their planned implementations will work in cross environments too.

For the time being we could extract the Linux code to a function and slap conditional compilation to it. What do you think?

90% of CLI downloads are for Linux. I guess there might be some Windows and Mac users, but having this at least for Linux is already a big win, I think!

When considering a CLI flag in #1528, what are the possible good names for the flag to harmonize it across Nextstrain and perhaps also with curl? Looks like curl accepts --with-ca-path which accepts a directory path.

Curl is basing those on OpenSSL's conventions. When given a directory, OpenSSL in particular requires a specific content-addressed filename scheme that I don't think we want to explicitly support. And when Nextclade is using OpenSSL with this branch, then the normal SSL_CERT_DIR will still be available.

In general, I wouldn't go searching for functionality we could add here but focus on the minimum needed to solve concrete problems (e.g. no way to configure CA certs) and meet basic expectations (e.g. system CAs are used).

It seems that there could be multiple certs in the file, and when the libraries process it, some certs could fail. Currently the entire program fails if one cert fails to load.

We could consider skipping failing certs, if that makes sense and is safe, though I cannot immediately evaluate either.

One disadvantage of silently failing certs is that it will be unclear for the user that e.g. a single cert in a file fails. It will probably only manifest in connection errors later. To remediate, we could emit a warning for each failing cert.

I intentionally chose to fail hard early if anything doesn't succeed in the extra cert adding steps. I don't think there's any advantage to allowing invalid certs in the explicitly-configured file. Especially with security stuff, I'd rather know there's a problem or deviation from what I expect earlier than later.

We could also consider to bubble the check for presence for env var (and/or cli arg) up and only run the new code path conditionally if the CA certs are provided. Just to be extra safe that this does not affect the old code path, which most users use (especially considering the 0.x nature of all of these libs).

Hmm. I think you're conflating two things here. Using the new code path only when extra CA certs are provided sort of defeats the point. The extra CA certs are merely a convenience. The real useful change is using the system CA trust store, which should be the default and expected behaviour for any software (and it seems that reqwest is moving that way eventually as well). It means that for many correctly-configured systems there would be no additional configuration required for Nextclade to Just Work.

If you didn't want to use the system CA trust store and instead only support configuration of extra CA certs explicitly, then that can be done without rustls-platform-verifier entirely. But again, that'd be throwing away the change that I think is most useful.

[ivan-aksamentov]

Relatedly: should I add tests for this (or at least the env var)? Where would those go? The few Cram tests I saw look unused, but that's the kind of thing I'd reach for with this particular feature.

I am not sure. To be honest Nextclade is severely under-tested. But I would not test an env var, as it's just a single call to the std library. It will probably take too much time to test anything else in the PR.

drop it for now, leaving the new system-level trust store support in place

Sounds like a plan!

If I understand correctly we cherry pick the first commit (8816fe2), and also bump wasm-bindgen to 0.2.93 to avoid the clippy warnings. And we will revisit this PR once they release fixes for other platforms. Correct?

If so, and if you still have time and forces, feel free to do and merge to master. And I'll release tomorrow. If not, I'll do tomorrow.

[tsibley]

But I would not test an env var, as it's just a single call to the std library. It will probably take too much time to test anything else in the PR.

Ah, I meant test that setting the env var correctly gets a cert into the trust store and that it allows the TLS connection to succeed. An end-to-end integration test, exactly like what I've been doing manually to verify the functionality on my local machine. The actual specific test set up for this on Linux and macOS would be very straightforward for me. Windows would be a little more unknown, but I think I know how I'd do it. But if there's existing scaffolding for testing the final nextclade binaries isn't there… then yeah, maybe too much work to have to build that out first too? Also, if you're cross compiling to macOS and Windows, is there native test infra for those platforms available?

Sounds like a plan! […] Correct?

Correct! I'll do it now on a new PR and leave this one with the extended conversation for revisiting.

[tsibley]

I'll do it now on a new PR and leave this one with the extended conversation for revisiting.

Done. I've also gone ahead and reworked this PR to use the unreleased rustls-platform-verifier changes from GitHub (pinned to a specific revision) so that we can a) verify it works as expected and b) we're ready for when those changes are released.

I've verified the functionality still works as expected, at least on x86_64-unknown-linux-gnu.

[tsibley]

Main commit from me now reads:

feat: Support a NEXTCLADE_EXTRA_CA_CERTS environment variable

Allows adding additional CA certificates to the trust store specifically
for Nextclade, for when modifying the system's trust store isn't
desirable/possible.  Works across all platforms.

Currently requires using landed but unreleased changes to
rustls-platform-verifier so that macOS and Windows have the needed
Verifier::new_with_extra_certs() API.¹  That API also has signature
changes proposed² which, while unmerged, seem likely to land, so use
those in anticipation.  They actually bring the signature back closer to
the latest release (0.3.4).

¹ <https://github.com/rustls/rustls-platform-verifier/issues/58>
² <https://github.com/rustls/rustls-platform-verifier/pull/145>

Related-to: <https://github.com/nextstrain/nextclade/pull/1529>
Related-to: <https://github.com/nextstrain/nextclade/issues/726>

[tsibley]

Closing this as I expect it to be superseded by #1536.