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

Sending fatal alert BadCertificate: invalid certificate: UnknownIssuer #11482

Closed
justinmchase opened this issue Jul 21, 2021 · 11 comments · Fixed by #11491
Closed

Sending fatal alert BadCertificate: invalid certificate: UnknownIssuer #11482

justinmchase opened this issue Jul 21, 2021 · 11 comments · Fixed by #11491
Labels
needs info needs further information to be properly triaged needs investigation requires further investigation before determining if it is an issue or not

Comments

@justinmchase
Copy link
Contributor

The issuer is my company. I am behind a corporate firewall and so is the destination server. I have the same CA cert the server is using installed locally.

I am also on macOS Catalina 10.15.7

I'm using fetch(url) and its giving me this error:

Sending fatal alert BadCertificate
error: Uncaught (in promise) TypeError: error sending request for url (https://foo.bar.com/health): 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)

My code is basically:

deno run -A --cert /etc/ssl/cert.pem main.ts
const res = await fetch('https://foo.bar.com/health', {
  method: 'GET',
  cache: 'no-cache',
  keepalive: false,
  signal: controller.signal
})

Querying the same url using curl with the same cert from the terminal gives me a success result (domains and cert details altered):

$  curl -v https://foo.bar.com/health
*   Trying 10.202.14.185...
* TCP_NODELAY set
* Connected to foo.bar.com (10.202.14.185) port 443 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* successfully set certificate verify locations:
*   CAfile: /etc/ssl/cert.pem
  CApath: none
* TLSv1.2 (OUT), TLS handshake, Client hello (1):
* TLSv1.2 (IN), TLS handshake, Server hello (2):
* TLSv1.2 (IN), TLS handshake, Certificate (11):
* TLSv1.2 (IN), TLS handshake, Server key exchange (12):
* TLSv1.2 (IN), TLS handshake, Server finished (14):
* TLSv1.2 (OUT), TLS handshake, Client key exchange (16):
* TLSv1.2 (OUT), TLS change cipher, Change cipher spec (1):
* TLSv1.2 (OUT), TLS handshake, Finished (20):
* TLSv1.2 (IN), TLS change cipher, Change cipher spec (1):
* TLSv1.2 (IN), TLS handshake, Finished (20):
* SSL connection using TLSv1.2 / ECDHE-RSA-AES256-GCM-SHA384
* ALPN, server did not agree to a protocol
* Server certificate:
*  subject: C=US; ST=State; L=City; O=Foo Bar; OU=Baz; CN=foo.bar.com
*  start date: Oct 20 13:30:16 2020 GMT
*  expire date: Oct 20 13:30:16 2021 GMT
*  subjectAltName: host "foo.bar.com" matched cert's "foo.bar.com"
*  issuer: C=US; ST=State; L=City; O=Name; CN=More
*  SSL certificate verify ok.
> GET /health HTTP/1.1
> Host: foo.bar.com
> User-Agent: curl/7.64.1
> Accept: */*
> 
< HTTP/1.1 200 OK
< Date: Tue, 20 Jul 2021 22:22:39 GMT
< Content-Type: text/plain
< Server: Kestrel
< Cache-Control: no-store, no-cache
< Pragma: no-cache
< Transfer-Encoding: chunked
< Expires: Thu, 01 Jan 1970 00:00:00 GMT
< Strict-Transport-Security: max-age=300; includeSubDomains
< 
* Connection #0 to host foo.bar.com left intact
Healthy* Closing connection 0

May be related to #10312 (comment)

Is there a way for me to turn on deeper tracing to get a more detailed explanation of why its failing?

@lucacasonato
Copy link
Member

You can enabled -Ldebug which might give some more detailed logs.

@justinmchase
Copy link
Contributor Author

Thanks, here is the extra log info from adding that flag:

DEBUG RS - deno_runtime::permissions:40 - ⚠️️  Granted net access to "foo.bar.com"
DEBUG RS - reqwest::connect:503 - starting new connection: https://foo.bar.com/
DEBUG RS - rustls::client::hs:89 - No cached session for DNSNameRef("foo.bar.com")
DEBUG RS - rustls::client::hs:211 - Not resuming any session
DEBUG RS - rustls::client::hs:430 - ALPN protocol is None
DEBUG RS - rustls::client::hs:598 - Using ciphersuite TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
DEBUG RS - rustls::client::tls12:209 - ECDHE curve is ECParameters { curve_type: NamedCurve, named_group: secp256r1 }
DEBUG RS - rustls::client::tls12:508 - Server DNS name is DNSName("foo.bar.com")
Sending fatal alert BadCertificate

@justinmchase
Copy link
Contributor Author

justinmchase commented Jul 21, 2021

I am able to reproduce it purely using the rustls example client, so it seems to not be a Deno issue per-se.

I've narrowed it down to this line:
https://github.com/ctz/rustls/blob/92a60a54e084834cf66ea26a8daeb3b360f9ecbd/rustls/src/client/tls12.rs#L734

$ cargo run --example tlsclient -- --http foo.bar.com --verbose
...
[2021-07-21T21:29:58Z DEBUG rustls::client::tls12] Server DNS name is DnsName(DnsName(DnsName("foo.bar.com")))
[2021-07-21T21:29:58Z WARN  rustls::conn] Sending fatal alert BadCertificate
TLS error: WebPkiError(UnknownIssuer, ValidateServerCert)
Connection closed

@lucacasonato
Copy link
Member

@justinmchase Could you share the exact certificate that is causing issues with me so we can debug this? If you don't want to share it in public, feel free to email me at [email protected].

@justinmchase
Copy link
Contributor Author

I think its becuase the server cert has a root ca of my company. I have the cert installed and trusted locally of course in order to access various internal services and Chrome and Curl both seem to respect it and thus accessing this particular server succeeds via chrome and curl.

Screen Shot 2021-07-21 at 5 21 39 PM

Screen Shot 2021-07-21 at 5 23 20 PM

For the rustls client I see by default it just loads up some root certs from a crate full of trusted certs published by Mozilla. But if I add the flag --cafile=/etc/ssl/cert.pem to their test client and it prints out an additional line:

[2021-07-21T22:18:57Z DEBUG rustls::anchors] add_parsable_certificates processed 132 valid and 0 invalid certs

But still ends up having the same error later.

Here is the exact line where it fails:
https://github.com/ctz/rustls/blob/2aa40d248b20d2c8513ce0c2a1335ad27f2b2b29/rustls/src/verify.rs#L313

And I think I found another clue the ca cert does not appear to be in /etc/ssl/cert.pem but rather in my System keychain:

Screen Shot 2021-07-21 at 5 39 52 PM

I wonder if I can export that file as pem format if I'll be able to the pass the path directly to it. I'll have to try it tomorrow, I'll email you @lucacasonato if I hit a wall.

I suspect that this library is supposed to use some kind of mac specific API to access the os system keychain but it isn't. Perhaps specifying the pem would work but my systems root pem is just full of generic public certificate authorities not my companies special cert.

@justinmchase
Copy link
Contributor Author

I found a link to this project from within the rustls project:
https://github.com/rustls/rustls-native-certs

I think this is basically what is needed to make it work.

Without being an expert in this area it definitely seems like it should be using just the keystore of the OS by default. Those are the certs the user or the users network admin has deemed trustworthy, it seems like pulling in the mozilla certs is not the greatest way to handle the problem.

@lucacasonato
Copy link
Member

@justinmchase We use the same approach as Firefox, which is to disregard the OS keystore because it might be very out of date (e.g. Windows XP not having ISRG Root X1). To add a cert to the root store that Deno uses you have to specify the certificate with the --cert flag.

If this still results in verification errors there are two possible issues:

  1. the CA certificate isn't valid for the domain you are trying to connect to
  2. there is a bug in rustls or Deno

Does the file you are passing to --cert contain a PEM encoded version of the Root CA?

@lucacasonato lucacasonato added needs info needs further information to be properly triaged needs investigation requires further investigation before determining if it is an issue or not labels Jul 22, 2021
@justinmchase
Copy link
Contributor Author

The file I was passing to --cert does not contain the companies CA it turns out, the right root CA only exists in the system certificate store for me, I don't have the CA file anywhere else it seems. I could theoretically obtain it or extract it from the keystore though but it was installed by IT when the machine was provisioned, theoretically they maintain the certs in the store by policy and it could be updated by them too.

I was actually able to finally get it to work this morning with a few changes to Deno :)

There isn't a bug per se but I had to add support for native-tls in the reqwest component and tell it to use_native_tls() instead of use_rustls_tls() and boom it started working. So confirmed that is 100% the problem. I'll fire up a draft PR for reference but its pretty trivial really.

I think the real question is what is the right thing to do?

... which is to disregard the OS keystore because it might be very out of date (e.g. Windows XP not having ISRG Root X1).

First off, this doesn't seem like a great strategy to me. Two main reasons:

1. It shouldn't be Denos responsibility to dictate certs

Using the systems certificate store would take the responsibility out of the hands of Deno and put it solely on the user. If the users certs in Windows XP don't work... well they need to get the right certs and install them. (Also, maybe Windows XP shouldn't be on the support matrix 😛 )

But conversely, what if a user wants to revoke a cert or only support a specific set of certs? There isn't a way to do that, Deno right now is loading the rustls mozilla certs then adding the one specified by --cert. There is no way to prevent the mozilla certs from being used.

2. The certs are baked into Deno right now

All of these builds of Deno appear to have the certs baked in. If one is updated or revoked for some reason, all of those older builds of Deno become broken I believe.

By depending on the certificate store you decouple Deno from the certs and put the responsibility of managing which certs are valid onto the user.

Now I get that in some cases that is annoying to users, users on old machines who don't care about certs who feel broken by default, that's not a great scenario. But on the other hand you have people with strict certs and working in corporate environments with internal certificate authorities and strict policy based management of certs, they're broken by default also.

Conclusion

I think Deno should get out of the business of providing certs and just use the systems store. Put the responsibility on the user to install and update valid certificates. I think Deno could provide documentation on how to update the latest set of mozilla certs and possibly an option and automatically install them for users during a Deno upgrade.

Proposals

  1. reqwest is altered to use use_native_tls() instad of use_rustls_tls()
  2. op_start_tls is altered to use rustls_native_certs::load_native_certs() instead of webpki_roots::TLS_SERVER_ROOTS
  3. op_connect_tls ^
  4. op_ws_create ^
  5. A new command is added deno upgrade --certs which installs the latest set of webpki_roots::TLS_SERVER_ROOTS into the users certificate store

Other possibilities could include additional flags such as --use-native-certs or --use-webpki-certs... But I honestly think that using the system store by default is the right thing to do.

@lucacasonato
Copy link
Member

lucacasonato commented Jul 22, 2021

reqwest is altered to use use_native_tls() instad of use_rustls_tls()

Reqwest with use_native_tls pulls in native-tls which links to openssl. Pulling in openssl is a non-negotiable no. Instead, the same approach as with the low level TLS APIs can be used (specifying rustls_native_certs::load_native_certs() as default certs) should be used.

I would be open to having a DENO_TLS_CA_STORE=system and DENO_TLS_CA_STORE=mozilla env var to switch between rustls_native_certs::load_native_certs() and webpki_roots::TLS_SERVER_ROOTS. The mozilla root certs should continue to be the default though, just like they are in Firefox and Deno currently.

I am not in favor of deno upgrade --certs. This is a system level task, and not something Deno should be involved in. On Ubuntu this is something that an apt upgrade can do for example.

cc @bartlomieju @ry, opinions requested

@bartlomieju
Copy link
Member

Ah, I already commented on the #11491, but I see Luca already said it here, that using native-tls from reqwest is a no-go.

As for DENO_TLS_CA_STORE env variable - I'm not against it, if it solves real world problem I'll be fine with it, I'm a bit wary about discoverability but I'm sure we can explain it properly in docs.

@justinmchase
Copy link
Contributor Author

Ok I'll remove native-tls and figure out how to update the ClientBuilder without needing that feature.

How would you feel about adding both the webpki certs and the native certs by default? Will they stomp on each other, or is a valid cert a valid cert?

So in that scenario we would have Deno do what its doing now plus we add all of the certs from the users native store. That way by default they get modern updated certs but also if you have some self signed or corporate CA installed by IT then you get that automatically too, then if one of your built-in mozilla certs expires or gets revoked for some reason then your path forward is to either update Deno or to get a new version of the mozilla certs and install them into your certificate store.

The flags could control each step but they would both be enabled by default something like:

DENO_TLS_CA_STORE=system,mozilla

Then if you wanted to disable either one, or both you could use that variable to control it and then you'd have fine grained control with the --cert flag or passing a specific cert in when calling fetch, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs info needs further information to be properly triaged needs investigation requires further investigation before determining if it is an issue or not
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants