This repository was archived by the owner on May 10, 2019. It is now read-only.
Fix check_primary_support to read responses #3185
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Link to #3221 |
Contributor
|
I should be able to review this. |
|
This change should also be merged into shane-tomlinson/checkmyidp.org |
Member
|
cc @callahad |
Contributor
|
r+, works as intended. Merging. |
callahad
added a commit
that referenced
this pull request
Apr 12, 2013
Fix check_primary_support to read responses
Contributor
|
Thanks @robottaway ! Sorry for the delay in getting that in. |
Contributor
Author
|
@dan notice you all have a few other things to do ;) Seems plenty fast cheers On Friday, April 12, 2013, Dan Callahan wrote:
|
wking
added a commit
to wking/persona
that referenced
this pull request
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 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.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi been working on implementing and IDP for my site, was using the check_primary_support script and found it was not reading the responses when checking for script tags in the provision and auth endpoints. May be worth taking a look at my fixes to make it work for me.
-rob