FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

crypto: load PFX chain the same way as regular one by indutny · Pull Request #4165 · nodejs/node · GitHub

/ node Public

crypto: load PFX chain the same way as regular one - #4165

Closed
indutny wants to merge 11 commits into
nodejs:masterfrom
indutny:fix/gh-4127
ConversationCommits (11)ChecksFiles changed
Closed

crypto: load PFX chain the same way as regular one#4165
indutny wants to merge 11 commits into
nodejs:masterfrom
indutny:fix/gh-4127

Conversation

indutny commented Dec 5, 2015

Copy link
Copy Markdown
Member

Load the certificate chain from the PFX file the same as we do it for a
regular certificate chain.

Fix: #4127

crypto: load PFX chain the same way as regular one
d04c1f6 
Load the certificate chain from the PFX file the same as we do it for a
regular certificate chain.

Fix: nodejs#4127
indutny mentioned this pull request Dec 5, 2015
Closed

indutny commented Dec 5, 2015

Copy link
Copy Markdown
Member Author

cc @nodejs/crypto

indutny added tls Issues and PRs related to the tls subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. lts-watch-v4.x labels Dec 5, 2015

indutny commented Dec 6, 2015

Copy link
Copy Markdown
Member Author

Actually just added the test that demonstrates the problem, feel free to review it now ;)

indutny commented Dec 6, 2015

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/934/

mscdex reviewed Dec 6, 2015
View reviewed changes
Comment thread src/node_crypto.cc Outdated

mscdex Dec 6, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

s/editted/edited/

indutny Dec 6, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ack.

indutny commented Dec 6, 2015

Copy link
Copy Markdown
Member Author

There was failure on CI FIPS bot, fixed in latest commit. Running CI again: https://ci.nodejs.org/job/node-test-pull-request/935/

indutny commented Dec 6, 2015

Copy link
Copy Markdown
Member Author

Gosh, messed it up. New CI: https://ci.nodejs.org/job/node-test-pull-request/936/

indutny commented Dec 6, 2015

Copy link
Copy Markdown
Member Author

CI seems to be green, failures are completely unrelated.

bnoordhuis reviewed Dec 7, 2015
View reviewed changes
Comment thread src/node_crypto.cc

bnoordhuis Dec 7, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you add a CHECK_EQ(*issuer, nullptr) and maybe a CHECK_EQ(*cert, nullptr) while you're here?

indutny Dec 7, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

You are totally right. It needs something more complex though, since we don't prevent people from passing in both cert+key and pfx for TLS server.

indutny commented Dec 7, 2015

Copy link
Copy Markdown
Member Author

@bnoordhuis PTAL, should be better now.

indutny commented Dec 9, 2015

Copy link
Copy Markdown
Member Author

@bnoordhuis ping ;)

indutny commented Dec 11, 2015

Copy link
Copy Markdown
Member Author

erm @bnoordhuis or @nodejs/crypto : I need your LGTM ;)

indutny commented Dec 14, 2015

Copy link
Copy Markdown
Member Author

Anyone?

indutny commented Dec 17, 2015

Copy link
Copy Markdown
Member Author

ping @bnoordhuis @nodejs/crypto

bnoordhuis reviewed Dec 17, 2015
View reviewed changes
Comment thread src/node_crypto.cc

bnoordhuis Dec 17, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I realize it's not new code but I believe PEM_read_bio_X509_AUX() also registers an error, but with tag ERR_LIB_PEM, not ERR_LIB_SSL.

indutny Dec 17, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I would keep it as it is right now, just because it may make this PR a semver-major. Let's reconsider it later.

paddybyers commented Jan 3, 2016

Copy link
Copy Markdown
Contributor

Is it planned that this will be be included in 4.2.x as well as 5.x? The reason I ask is that I believe a side-effect of this change is to fix a memory leak which is relevant to production stability. I can raise an issue/PR against 4.2.x with a fix only for the memory leak if necessary.

indutny commented Jan 4, 2016

Copy link
Copy Markdown
Member Author

@paddybyers yep, it has lts-watch tag ;)

paddybyers commented Jan 4, 2016

Copy link
Copy Markdown
Contributor

it has lts-watch tag ;)

I see, thanks :)

Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Jan 6, 2016
crypto: load PFX chain the same way as regular one
fcf632b 
Load the certificate chain from the PFX file the same as we do it for a
regular certificate chain.

Fix: nodejs#4127
PR-URL: nodejs#4165
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Jan 13, 2016
crypto: load PFX chain the same way as regular one
bb0a964 
Load the certificate chain from the PFX file the same as we do it for a
regular certificate chain.

Fix: #4127
PR-URL: #4165
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
crypto: load PFX chain the same way as regular one
d19da66 
Load the certificate chain from the PFX file the same as we do it for a
regular certificate chain.

Fix: #4127
PR-URL: #4165
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins mentioned this pull request Jan 19, 2016
Merged
diegossilveira mentioned this pull request Feb 5, 2016
Closed
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
crypto: load PFX chain the same way as regular one
dacb2fc 
Load the certificate chain from the PFX file the same as we do it for a
regular certificate chain.

Fix: nodejs#4127
PR-URL: nodejs#4165
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
This was referenced Mar 18, 2020
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. tls Issues and PRs related to the tls subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL