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

crypto: return a clearer error when loading an unsupported pkcs12 #54485

Merged

Conversation

pimterry
Copy link
Member

Currently when a PFX file with an unsupported format is used, it will throw an error because it's not supported (typically because you need the OpenSSL legacy provider). That fails with:

  • A message that is literally just unsupported
  • No error code
  • A stack that's not very useful for many users, e.g.:
    Error: unsupported
      at configSecureContext (node:internal/tls/secure-context:285:15)
      at Object.createSecureContext (node:_tls_common:116:3)
      at Object.connect (node:_tls_wrap:1763:48)
      at Agent.createConnection (node:https:170:22)
      at Agent.createSocket (node:_http_agent:340:26)
      at Agent.addRequest (node:_http_agent:288:10)
      at new ClientRequest (node:_http_client:337:16)
      at request (node:https:378:10)
    

Lots of users run into this (e.g. #40672 - and there's plenty of other similar issues) but it's not really clear from this error what's going on, or which of the options they've provided is failing and why.

This PR improves that for the common LoadPKCS12 case, with an explicit error describing what is not supported (you're loading a PFX file that is not supported) and a standard error code so you can recognize and google this more usefully (ERR_CRYPTO_UNSUPPORTED_OPERATION). The code is also useful for people building on Node.js and processing user-provided PFX files (this is me) who would like to be able to recognize failures in processing these automatically & reliably.

I explored trying to get more info from OpenSSL's errors on exactly what was unsupported, but the best available is the data string, which looks something like Global default library context, Algorithm (RC2-CBC : 3), Properties () (as a plain string - I can't see a way to reach the data itself directly). I assume that's not really useful/friendly enough to include here, but happy to add that if people disagree.

The tests here use a PFX I've generated manually with an old OpenSSL version using RC2-40-CBC. You can see the contents with openssl pkcs12 -info -legacy -in ./test/fixtures/keys/legacy.pfx and password legacy (note that without the -legacy OpenSSL flag it will fail to open).

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Aug 21, 2024
@pimterry pimterry force-pushed the improve-openssl-unsupported-error branch from 0c3e70b to 48b65f7 Compare August 21, 2024 13:37
Copy link

codecov bot commented Aug 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.60%. Comparing base (821ffab) to head (f4db363).
Report is 127 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #54485      +/-   ##
==========================================
+ Coverage   87.31%   87.60%   +0.28%     
==========================================
  Files         648      650       +2     
  Lines      182365   182832     +467     
  Branches    34988    35382     +394     
==========================================
+ Hits       159236   160173     +937     
+ Misses      16393    15927     -466     
+ Partials     6736     6732       -4     
Files with missing lines Coverage Δ
src/crypto/crypto_context.cc 68.28% <100.00%> (+0.22%) ⬆️

... and 122 files with indirect coverage changes

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 24, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 24, 2024
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mertcanaltin mertcanaltin left a comment

Choose a reason for hiding this comment

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

lgtm

@pimterry
Copy link
Member Author

CI errors here is an actual failures, in the sharedlibs_openssl111fips config - should be able to find some time to fix that up and finish this in the next couple of days, watch this space.

@pimterry pimterry added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2024
@nodejs-github-bot
Copy link
Collaborator

@pimterry pimterry force-pushed the improve-openssl-unsupported-error branch from 468e3e9 to f4db363 Compare September 4, 2024 12:39
@nodejs-github-bot
Copy link
Collaborator

@pimterry
Copy link
Member Author

pimterry commented Sep 4, 2024

OpenSSL 111 test are now passing! It looks like the only remaining failures here are spurious so I'll rerun those soon once CI finishes. A rereview would be nice when you have a minute @lpinca @mertcanaltin

Copy link
Member

@mertcanaltin mertcanaltin left a comment

Choose a reason for hiding this comment

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

lgtm

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@pimterry pimterry added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Sep 5, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 5, 2024
@nodejs-github-bot nodejs-github-bot merged commit 65b4fb8 into nodejs:main Sep 5, 2024
57 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 65b4fb8

aduh95 pushed a commit that referenced this pull request Sep 12, 2024
PR-URL: #54485
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants