fix(tls): Optionally support loading native certs #11491

justinmchase · 2021-07-22T15:28:31Z

Overview

This changes Deno to support optionally loading certificates from the users local certificate store. This will allow them to successfully connect via tls with corporate and self signed certs provided they have them installed in their keystore. It also allows them to deal with revoked certs by simply updating their keystore without having to upgrade Deno.

Potentially in the future Deno could consider modifying its default behavior to load from the certificate store but for now it is only optionally enabled by specifying it in the new flag described below.

This change also adds a new flag DENO_TLS_CA_STORE which can have the values system,mozilla and will load them in the order they are specified and defaults to mozilla. system controls loading certs from the users certificate store and mozilla controls loading the bundled mozilla certs.

Issues

Fixes #11482
Related To #10312

Testing

I have access through VPN to an internal website which is using a certificate issued by my companies internal CA. With latest Deno I am unable to access that site because the only certificate I have is installed in my system keychain by my IT department.

The app code:

const { ok, status } = await fetch('http://example-app.acme.com')
console.log(`${ok ? 'OK' : 'FAIL'} ${status}`)

To see this error I am able to run:

$ deno run --unstable -A main.ts
Check file:///Users/jchase24/code/example/main.ts
Sending fatal alert BadCertificate
error: Uncaught (in promise) TypeError: error sending request for url (https://example-app.acme.com/#undefined): error trying to connect: invalid certificate: UnknownIssuer
    at deno:core/core.js:86:46
    at unwrapOpResult (deno:core/core.js:106:13)
    at async mainFetch (deno:extensions/fetch/26_fetch.js:229:14)

Now with these changes I've run

$ cd ~/code/deno
$ cargo build

$ cd ~/code/example
$ DENO_TLS_CA_STORE=system ../deno/target/debug/deno run -A main.ts
Check file:///Users/jchase24/code/example/main.ts
OK 200

Using the flag to remove the certificate store, results in the error again (as expected)

DENO_TLS_CA_STORE=mozilla ../deno/target/debug/deno run -A main.ts
Sending fatal alert BadCertificate
error: Uncaught (in promise) TypeError: error sending request for url (https://example-app.acme.com/#undefined): error trying to connect: invalid certificate: UnknownIssuer
    at deno:core/01_core.js:106:46
    at unwrapOpResult (deno:core/01_core.js:126:13)
    at async mainFetch (deno:extensions/fetch/26_fetch.js:265:14)

CLAassistant · 2021-07-22T15:28:35Z

All committers have signed the CLA.

extensions/fetch/Cargo.toml

extensions/fetch/lib.rs

extensions/net/Cargo.toml

justinmchase · 2021-07-23T03:50:31Z

Outstanding Issues

There should probably be one function in all of Deno for initializing the ClientBuilder and tls config
There should be one cache for tls persistence
Unit tests.

lucacasonato

There is now a lot of duplicated logic. I would suggest the following:

On ProgramState creation, create a single rustls::RootCertStore should be created containing certificates based on the user requested CA stores, and the certificate provided by --cert. This rustls::RootCertStore should then be injected into create_http_client (and deno_fetch), and all other places where we use TLS (deno_net etc).

This would also allow you to remove all the places we inject ca_data, because that would already be part of the rustls::RootCertStore.

cli/flags.rs

cli/http_util.rs

cli/standalone.rs

justinmchase · 2021-07-26T16:06:35Z

Ok this was refactored and updated to:

Consolidate duplicate code up into ~~deno_core~~ deno_tls
Replace passing ca_data,ca_file with root_cert_store.
Default behavior is to only load the mozilla certs as it does today, the system certs are only loaded if specified via the flag.
Some places in code such as op_* functions can have arguments which pass certs directly in, that behavior should not have been changed and they get added to a clone of the base store per function call.
Some modules such as rustls were bumped up to ~~deno_core~~ deno_tls and are referenced directly there rather than having each other module have a duplicate dependency.

bnoordhuis

Drive-by review.

cli/program_state.rs

core/Cargo.toml

bartlomieju

@justinmchase nice work! I pointed out some nitpicks and how to solve problem with adding multiple TLS related crates to deno_core crate.

cli/file_fetcher.rs

cli/Cargo.toml

cli/tests/integration/mod.rs

core/Cargo.toml

justinmchase · 2021-08-03T16:44:06Z

Ok thanks @bartlomieju I know how to proceed.

justinmchase · 2021-08-03T20:26:26Z

Ok merged with main and all PR comments addressed and two unit tests were added, which need to be reviewed.

The new tests call https://deno.land since its signed with a valid root CA covered by the mozilla certs. Arguably this is a forbidden action but I'm not sure how else to test it specifically.

These tests positively test that the mozilla certs connect to deno.land and positively test that not loading any ca certs (an empty root_cert_store) fails to connect to deno.land.

The only other test vector that isn't covered by existing tests would be to then put the right cert into the system keystore and test loading that... I don't know how to do that consistently across systems without some really complicated automation and frankly... it feels a bit tautological to test as that entire functionality exists in a different dependency we are adding which has its own testing.

I've been testing manually and can confirm that adding the new flag allows me to connect to my internal company sites successfully.

I'll leave it up to the maintainers to decide which tests are needed.

justinmchase · 2021-08-03T21:17:15Z

Curious... on Windows it seems to successfully make a request even without certs. Does it not have tls support by default? Or maybe its loading certs from the certificate store regardless? I'm not sure.

bnoordhuis

Left some comments but your PR mostly looks (very!) good.

Node supports all these use-cases via esoteric OpenSSL env vars. We can centralize and clean this up.

We'll be doing even better. Node can only use the system's openssl certificate bundle, whereas we will be able to use the native macos and windows stores.

It's an often-requested feature that never happened because openssl can't do it. Take nodejs/node#39657 - a feature request from < 24 hours ago.

cli/program_state.rs

cli/standalone.rs

bnoordhuis · 2021-08-05T08:51:25Z

extensions/net/ops_tls.rs

+  let root_cert_store = state
+    .borrow()
+    .borrow::<DefaultTlsOptions>()
+    .root_cert_store
+    .clone();


Cloning the store every time is somewhat inefficient. On my system it copies ~200 kB of data.

It's not really a regression though because we currently copy webpki_roots::TLS_SERVER_ROOTS and that's about the same size.

Ideally, we create and cache the Arc<rustls::ClientConfig> so it can be reused instead of created anew for every connection...

...but that's something that can wait for a follow-up PR, it doesn't have to block this one.

That is why I did that, the roots were cloned before and this ended up following that pattern for better or worse.

Given how this already feels to be max size for this PR I'm happy to leave this for a future refactor.

That's fine, but please add a TODO here so we don't forget about it :)

extensions/tls/lib.rs

ry · 2021-08-05T11:00:19Z

If Ben and Luca are ok with this, then I have no objections. We just need to make sure this is properly documented after this patch lands.

justinmchase · 2021-08-06T04:37:30Z

@ry If Ben and Luca are ok with this, then I have no objections

Just to be clear by "this" I assume you mean keeping the functionality as it is in this pr right now? To have the DENO_TLS_CA_STORE with string[] arguments, not the DENO_SYSTEM_CA_STORE=1 boolean flag?

If so then this PR is ready I believe. I linted, formatted and squashed the changes in the branch as well as manually tested one more time.

bnoordhuis · 2021-08-06T05:52:47Z

For posterity: I don't have opinions on how this feature should be activated. Bartek or Luca are more qualified to make that call.

extensions/tls/lib.rs

bartlomieju · 2021-08-06T12:50:49Z

@ry If Ben and Luca are ok with this, then I have no objections

Just to be clear by "this" I assume you mean keeping the functionality as it is in this pr right now? To have the DENO_TLS_CA_STORE with string[] arguments, not the DENO_SYSTEM_CA_STORE=1 boolean flag?

If so then this PR is ready I believe. I linted, formatted and squashed the changes in the branch as well as manually tested one more time.

That's right, I'll give this PR another review tonight.

Could you please open a PR in https://github.com/denoland/manual that describes this functionality? Turning Luca's comment into some kind of doc would be awesome.

Fix typo Co-authored-by: Ben Noordhuis <[email protected]>

bartlomieju

@justinmchase this PR is really well implemented, very nice work! I have only a couple minor nitpicks left and we're good to merge.

bartlomieju · 2021-08-06T17:45:47Z

Cargo.lock

+[[package]]
+name = "openssl-probe"
+version = "0.1.4"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "28988d872ab76095a6e6ac88d99b54fd267702734fd7ffe610ca27f533ddb95a"
+


Yikes 😬 it comes from rustls-native-certs and it doesn't seem it could be in any way skipped (no feature flags in that crate). I looked up that crate's code and it doesn't do any interaction with OpenSSL so I guess we can live with that.

Interesting, I think it does say something about it in the docs now that you mention it:

On Linux and other UNIX-like operating systems, the openssl-probe crate is used to discover the filename of the system CA bundle.

cli/program_state.rs

cli/http_util.rs

cli/standalone.rs

bartlomieju · 2021-08-06T17:50:55Z

extensions/net/ops_tls.rs

+  let root_cert_store = state
+    .borrow()
+    .borrow::<DefaultTlsOptions>()
+    .root_cert_store
+    .clone();


That's fine, but please add a TODO here so we don't forget about it :)

extensions/tls/lib.rs

justinmchase · 2021-08-06T20:51:40Z

One of the tests returned this result, I'm not sure how to run it locally to debug it. It passed in a previous iteration, so it's an interesting result. How can I tell which test is failing here exactly?

bartlomieju · 2021-08-06T20:57:41Z

One of the tests returned this result, I'm not sure how to run it locally to debug it. It passed in a previous iteration, so it's an interesting result. How can I tell which test is failing here exactly?

@justinmchase it looks like this error was returned when pulling code for deno_std - before any test was run - ie. when process was starting up and downloading dependencies.

justinmchase · 2021-08-07T02:42:07Z

Succeeded this time, I didn't change anything. Weird. 🤷

All comments have been addressed I believe. Thanks all.

bartlomieju

LGTM, thank you @justinmchase!

`fetch()` and client-side websocket used to support HTTP/2, but this regressed in denoland#11491. This patch reenables it by explicitly adding `h2` and `http/1.1` to the list of ALPN protocols on the HTTP and websocket clients. Fixes denoland#12517.

`fetch()` and client-side websocket used to support HTTP/2, but this regressed in #11491. This patch reenables it by explicitly adding `h2` and `http/1.1` to the list of ALPN protocols on the HTTP and websocket clients.

justinmchase marked this pull request as draft July 22, 2021 15:29

bartlomieju reviewed Jul 22, 2021

View reviewed changes

extensions/fetch/Cargo.toml Outdated Show resolved Hide resolved

bartlomieju mentioned this pull request Jul 22, 2021

Sending fatal alert BadCertificate: invalid certificate: UnknownIssuer #11482

Closed

justinmchase commented Jul 22, 2021

View reviewed changes

extensions/fetch/lib.rs Outdated Show resolved Hide resolved

justinmchase commented Jul 22, 2021

View reviewed changes

extensions/fetch/lib.rs Outdated Show resolved Hide resolved

justinmchase commented Jul 22, 2021

View reviewed changes

extensions/net/Cargo.toml Outdated Show resolved Hide resolved

justinmchase marked this pull request as ready for review July 23, 2021 03:51

lucacasonato reviewed Jul 23, 2021

View reviewed changes

cli/flags.rs Outdated Show resolved Hide resolved

lucacasonato reviewed Jul 23, 2021

View reviewed changes

cli/http_util.rs Outdated Show resolved Hide resolved

justinmchase changed the title ~~fix(tls): Switch to native certs~~ fix(tls): Optionally support loading native certs Jul 26, 2021

justinmchase force-pushed the feature/use-native-certs-for-tls branch from 7d0744a to a60f607 Compare July 26, 2021 14:53

justinmchase marked this pull request as draft July 26, 2021 15:01

justinmchase commented Jul 26, 2021

View reviewed changes

cli/standalone.rs Outdated Show resolved Hide resolved

justinmchase force-pushed the feature/use-native-certs-for-tls branch from a60f607 to 567eae6 Compare July 26, 2021 15:56

justinmchase marked this pull request as ready for review July 26, 2021 16:39

justinmchase requested review from bartlomieju and lucacasonato July 26, 2021 16:40

justinmchase force-pushed the feature/use-native-certs-for-tls branch from 567eae6 to 9ef120c Compare July 26, 2021 20:42

justinmchase mentioned this pull request Jul 27, 2021

connectPostPodExec is not implemented cloudydeno/deno-kubernetes_apis#2

Closed

bnoordhuis reviewed Jul 30, 2021

View reviewed changes

cli/program_state.rs Outdated Show resolved Hide resolved

cli/program_state.rs Outdated Show resolved Hide resolved

core/Cargo.toml Outdated Show resolved Hide resolved

bartlomieju reviewed Aug 3, 2021

View reviewed changes

cli/file_fetcher.rs Outdated Show resolved Hide resolved

cli/Cargo.toml Outdated Show resolved Hide resolved

cli/Cargo.toml Outdated Show resolved Hide resolved

cli/tests/integration/mod.rs Outdated Show resolved Hide resolved

core/Cargo.toml Outdated Show resolved Hide resolved

bartlomieju added this to the 1.13.0 milestone Aug 3, 2021

justinmchase requested review from bartlomieju and bnoordhuis August 3, 2021 20:28

bnoordhuis approved these changes Aug 5, 2021

View reviewed changes

justinmchase force-pushed the feature/use-native-certs-for-tls branch from ddb5082 to e7dcd32 Compare August 6, 2021 04:25

Optionally support loading native certs

19653bf

justinmchase force-pushed the feature/use-native-certs-for-tls branch from e7dcd32 to 19653bf Compare August 6, 2021 04:36

bnoordhuis reviewed Aug 6, 2021

View reviewed changes

extensions/tls/lib.rs Outdated Show resolved Hide resolved

justinmchase mentioned this pull request Aug 6, 2021

feat(tls): Add DENO_TLS_CA_STORE documentation denoland/manual#37

Merged

justinmchase and others added 3 commits August 6, 2021 09:07

Fix lint error

f6a8d67

Update extensions/tls/lib.rs

0c11010

Fix typo Co-authored-by: Ben Noordhuis <[email protected]>

Fix typos

c94308b

bartlomieju reviewed Aug 6, 2021

View reviewed changes

Minor fixes after pr feedback

a28984d

formatting

afaf4ba

bartlomieju approved these changes Aug 7, 2021

View reviewed changes

Merge branch 'main' into feature/use-native-certs-for-tls

62d23f1

bartlomieju merged commit 02c74fb into denoland:main Aug 7, 2021

justinmchase deleted the feature/use-native-certs-for-tls branch August 8, 2021 05:07

justinmchase mentioned this pull request Aug 9, 2021

rustls refuses to establish connections to self signed TLS certificates #10312

Open

This was referenced Oct 25, 2021

fetch can't connect to HTTP/2-only servers #12517

Closed

fix(tls): Make TLS clients support HTTP/2 #12530

Merged

twihno mentioned this pull request Sep 25, 2023

Corporate Proxy results in BadCertificate Alert #20668

Closed

ansg191 mentioned this pull request Oct 20, 2023

Adds macOS Keychain certs to default CA store oven-sh/bun#6588

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(tls): Optionally support loading native certs #11491

fix(tls): Optionally support loading native certs #11491

justinmchase commented Jul 22, 2021 •

edited

Loading

CLAassistant commented Jul 22, 2021 •

edited

Loading

justinmchase commented Jul 23, 2021 •

edited

Loading

lucacasonato left a comment

justinmchase commented Jul 26, 2021 •

edited

Loading

bnoordhuis left a comment

bartlomieju left a comment

justinmchase commented Aug 3, 2021

justinmchase commented Aug 3, 2021 •

edited

Loading

justinmchase commented Aug 3, 2021

bnoordhuis left a comment

bnoordhuis Aug 5, 2021

justinmchase Aug 6, 2021

justinmchase Aug 6, 2021

bartlomieju Aug 6, 2021

ry commented Aug 5, 2021

justinmchase commented Aug 6, 2021

bnoordhuis commented Aug 6, 2021

bartlomieju commented Aug 6, 2021

bartlomieju left a comment

bartlomieju Aug 6, 2021

justinmchase Aug 6, 2021

bartlomieju Aug 6, 2021

justinmchase commented Aug 6, 2021

bartlomieju commented Aug 6, 2021

justinmchase commented Aug 7, 2021

bartlomieju left a comment

fix(tls): Optionally support loading native certs #11491

fix(tls): Optionally support loading native certs #11491

Conversation

justinmchase commented Jul 22, 2021 • edited Loading

Overview

Issues

Testing

CLAassistant commented Jul 22, 2021 • edited Loading

justinmchase commented Jul 23, 2021 • edited Loading

Outstanding Issues

lucacasonato left a comment

Choose a reason for hiding this comment

justinmchase commented Jul 26, 2021 • edited Loading

bnoordhuis left a comment

Choose a reason for hiding this comment

bartlomieju left a comment

Choose a reason for hiding this comment

justinmchase commented Aug 3, 2021

justinmchase commented Aug 3, 2021 • edited Loading

justinmchase commented Aug 3, 2021

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Aug 5, 2021

Choose a reason for hiding this comment

justinmchase Aug 6, 2021

Choose a reason for hiding this comment

justinmchase Aug 6, 2021

Choose a reason for hiding this comment

bartlomieju Aug 6, 2021

Choose a reason for hiding this comment

ry commented Aug 5, 2021

justinmchase commented Aug 6, 2021

bnoordhuis commented Aug 6, 2021

bartlomieju commented Aug 6, 2021

bartlomieju left a comment

Choose a reason for hiding this comment

bartlomieju Aug 6, 2021

Choose a reason for hiding this comment

justinmchase Aug 6, 2021

Choose a reason for hiding this comment

bartlomieju Aug 6, 2021

Choose a reason for hiding this comment

justinmchase commented Aug 6, 2021

bartlomieju commented Aug 6, 2021

justinmchase commented Aug 7, 2021

bartlomieju left a comment

Choose a reason for hiding this comment

justinmchase commented Jul 22, 2021 •

edited

Loading

CLAassistant commented Jul 22, 2021 •

edited

Loading

justinmchase commented Jul 23, 2021 •

edited

Loading

justinmchase commented Jul 26, 2021 •

edited

Loading

justinmchase commented Aug 3, 2021 •

edited

Loading