Skip to content
This repository has been archived by the owner on May 10, 2019. It is now read-only.

lib/primary.js: Pass https.globalAgent.options.ca to https.get #4059

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

wking
Copy link

@wking wking commented Dec 30, 2013

Otherwise

$ ./scripts/check_primary_support my.net /path/to/my/ca.pem

fails with:

error: my.net is not a browserid primary: Error: SELF_SIGNED_CERT_IN_CHAIN

The user-specified certificate authority path was added in #3185
(3848755, Fix check_primary_support to read responses, 2013-04-03), but
because agent is set to false in the https.get call, the CA I'd specified was
not being used.

With this commit, I pass the globalAgent's ca through to https.get
1, which wraps https.request 2, which accepts the ca option (among others)
from tls.connect 3. Once it has the ca option, https.get verifies my
self-signed CA just fine (with Node v0.10.21).

I don't understand the httpProxy bit deciding between http.get and https.get,
or how key-verification works in that case. It's possible that this patch
needs to be adjusted to handle that case, but http.get
4, which wraps http.request 5 does not support the TLS options
(not much of a surprise ;).

I also don't understand why we set agent to false in the https.get call.
0e39dc6 (Explicitly set rejectUnauthorized to false when using
https.request(), 2013-08-08 [6]) mentions Node 0.8 needing both
"rejectUnauthorized: true" and "agent: false" to reject invalid certificated.
Maybe we're far enough past Node 0.8 that we can remove the "agent: false"
part of this pair? The "agent: false" bit dates back to the initial
primary.js addition in cc186ea (an abstraction around primary support checking
and a command line script to test primary support, 2011-12-19), so I'm not
clear on exactly what it's accomplishing.

There are also a bunch of other https.get calls in the codebase. I'm not far
enough into my local Persona install to have a fully successful
check_primary_support, so it's possible that some of the remaining calls also
need the globalAgent CA data to get through check_primary_support with a
self-signed CA.

[6]: It really does say "set rejectUnauthorized to false" in the
commit summary. Oops :p. The rest of the commit correctly sets
rejectUnauthorized to true.

Otherwise

  $ ./scripts/check_primary_support my.net /path/to/my/ca.pem

fails with:

  error: my.net is not a browserid primary: Error: SELF_SIGNED_CERT_IN_CHAIN

The user-specified certificate authority path was added in mozilla#3185
(3848755, Fix check_primary_support to read responses, 2013-04-03),
but because agent is set to false in the https.get call, the CA I'd
specified was not being used.

With this commit, I pass the globalAgent's ca through to https.get
[1], which wraps https.request [2], which accepts the ca option (among
others) from tls.connect [3].  Once it has the ca option, https.get
verifies my self-signed CA just fine (with Node v0.10.21).

I don't understand the httpProxy bit deciding between http.get and
https.get, or how key-verification works in that case.  It's possible
that this patch needs to be adjusted to handle that case, but http.get
[4], which wraps http.request [5] does not support the TLS options
(not much of a surprise ;).

I also don't understand why we set agent to false in the https.get
call.  0e39dc6 (Explicitly set rejectUnauthorized to false when using
https.request(), 2013-08-08 [6]) mentions Node 0.8 needing both
"rejectUnauthorized: true" and "agent: false" to reject invalid
certificated.  Maybe we're far enough past Node 0.8 that we can remove
the "agent: false" part of this pair?  The "agent: false" bit dates
back to the initial primary.js addition in cc186ea (an abstraction
around primary support checking and a command line script to test
primary support, 2011-12-19), so I'm not clear on exactly what it's
accomplishing.

There are also a bunch of other https.get calls in the codebase.  I'm
not far enough into my local Persona install to have a fully
successful check_primary_support, so it's possible that some of the
remaining calls also need the globalAgent CA data to get through
check_primary_support with a self-signed CA.

[1]: http://nodejs.org/api/https.html#https_https_get_options_callback
[2]: http://nodejs.org/api/https.html#https_https_request_options_callback
[3]: http://nodejs.org/api/tls.html#tls_tls_connect_options_callback
[4]: http://nodejs.org/api/http.html#http_http_get_options_callback
[5]: http://nodejs.org/api/http.html#http_http_request_options_callback
[6]: It really does say "set rejectUnauthorized to false" in the
     commit summary.  Oops :p.  The rest of the commit correctly sets
     rejectUnauthorized to true.
@wking
Copy link
Author

wking commented Dec 30, 2013

Thinking about this more, it seems like removing agent: false and always checking for TLS verification errors would be a better path. If this is the first https.get, we get the current error handling. If another https.get fired earlier, maybe this call piggybacks on the first's socket, and misses the TLS handshake. However, in that case, the first https.get should be complaining about the invalid certificate. Alternatively, maybe there is a way to easily pass all of the globalAgent's options through here, and in other special cases where reusing pooled connections could cause trouble.

@fmarier
Copy link
Contributor

fmarier commented Jan 14, 2014

As I remember, in order for the rejectUnauthorized setting to work, we had to set agent to false. We'll have to keep it that way as long as it's needed on node 0.8 since that's what we're running on production.

@jaredhirsch
Copy link
Member

Hey @wking, we're currently focused on getting a release out the door, but I'll take a look once that's done.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants