Openssl clientcertengine support2 #14903

Trott · 2017-08-17T22:25:53Z

This is an attempt to finish #6569 which stalled. First commit is a squashed commit mostly of work done by @joelostrowski with their authorship preserved.

Original PR description:

Added an option 'clientCertEngine' to tls.createSecureContext which gets wired up to OpenSSL function SSL_CTX_set_client_cert_engine. The option is passed through from https.request as well. This allows using a custom OpenSSL engine to provide the client certificate.

PTAL @bnoordhuis @indutny
PTAL @sam-github at the doc changes and anything else you want

@danbev If you have time to look to make sure there aren't any "NOOOOO, this will fail if compiled without OpenSSL!!!!" problems that are super-obvious, that would be great. The stuff in test/addons/openssl-client-cert-engine seems like it needs a common.hasCrypto() check, no? Anything else anywhere in the code that looks like it might be problematic?

Marking as in progress because I can't get the test addon to compile on MacOS. Can someone help me make sense of this output from make test-addons?

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

tls http crypto

addaleax · 2017-08-17T22:28:22Z

test/addons/openssl-client-cert-engine/binding.gyp

This should get indented like JS objects

addaleax · 2017-08-17T22:29:52Z

lib/_tls_common.js

This should typcheck that options.clientCertEngine is a string

@addaleax Fixed (hopefully in a way that is acceptable). Thanks.

Trott · 2017-08-21T05:24:05Z

I did a CI run to see if it would have the same compilation issues I ran into. The only platform that seems to have the problem is MacOS, which is what my local development environment is. /ping @nodejs/platform-macos I guess.

https://ci.nodejs.org/job/node-test-commit-osx/11890/nodes=osx1010/console:

gyp info spawn make
gyp info spawn args [ 'BUILDTYPE=Release', '-C', 'build', '--jobs', 2 ]
  CXX(target) Release/obj.target/testengine/testengine.o
  SOLINK(target) Release/testengine.engine
Undefined symbols for architecture x86_64:
  "_BIO_new_mem_buf", referenced from:
      EngineLoadSSLClientCert(engine_st*, ssl_st*, stack_st_X509_NAME*, x509_st**, evp_pkey_st**, stack_st_X509**, ui_method_st*, void*) in testengine.o
  "_BIO_vfree", referenced from:
      EngineLoadSSLClientCert(engine_st*, ssl_st*, stack_st_X509_NAME*, x509_st**, evp_pkey_st**, stack_st_X509**, ui_method_st*, void*) in testengine.o
  "_CRYPTO_set_add_lock_callback", referenced from:
      _bind_engine in testengine.o
  "_CRYPTO_set_dynlock_create_callback", referenced from:
      _bind_engine in testengine.o
  "_CRYPTO_set_dynlock_destroy_callback", referenced from:
      _bind_engine in testengine.o
  "_CRYPTO_set_dynlock_lock_callback", referenced from:
      _bind_engine in testengine.o
  "_CRYPTO_set_ex_data_implementation", referenced from:
      _bind_engine in testengine.o
  "_CRYPTO_set_locking_callback", referenced from:
      _bind_engine in testengine.o
  "_CRYPTO_set_mem_functions", referenced from:
      _bind_engine in testengine.o
  "_ENGINE_get_static_state", referenced from:
      _bind_engine in testengine.o
  "_ENGINE_set_destroy_function", referenced from:
      _bind_engine in testengine.o
  "_ENGINE_set_finish_function", referenced from:
      _bind_engine in testengine.o
  "_ENGINE_set_id", referenced from:
      _bind_engine in testengine.o
  "_ENGINE_set_init_function", referenced from:
      _bind_engine in testengine.o
  "_ENGINE_set_load_ssl_client_cert_function", referenced from:
      _bind_engine in testengine.o
  "_ENGINE_set_name", referenced from:
      _bind_engine in testengine.o
  "_ERR_set_implementation", referenced from:
      _bind_engine in testengine.o
  "_PEM_read_bio_PrivateKey", referenced from:
      EngineLoadSSLClientCert(engine_st*, ssl_st*, stack_st_X509_NAME*, x509_st**, evp_pkey_st**, stack_st_X509**, ui_method_st*, void*) in testengine.o
  "_PEM_read_bio_X509", referenced from:
      EngineLoadSSLClientCert(engine_st*, ssl_st*, stack_st_X509_NAME*, x509_st**, evp_pkey_st**, stack_st_X509**, ui_method_st*, void*) in testengine.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[2]: *** [Release/testengine.engine] Error 1
gyp ERR! build error 
gyp ERR! stack Error: `make` failed with exit code: 2
gyp ERR! stack     at ChildProcess.onExit (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/deps/npm/node_modules/node-gyp/lib/build.js:258:23)
gyp ERR! stack     at emitTwo (events.js:125:13)
gyp ERR! stack     at ChildProcess.emit (events.js:213:7)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:201:12)
gyp ERR! System Darwin 14.5.0
gyp ERR! command "/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/out/Release/node" "/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/deps/npm/node_modules/node-gyp/bin/node-gyp" "--loglevel=info" "rebuild" "--python=/usr/bin/python" "--directory=/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/addons/openssl-client-cert-engine/" "--nodedir=/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010"
gyp ERR! cwd /Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/addons/openssl-client-cert-engine
gyp ERR! node -v v9.0.0-pre
gyp ERR! node-gyp -v v3.6.2
gyp ERR! not ok 
make[1]: *** [test/addons/.buildstamp] Error 1
make: *** [run-ci] Error 2
Build step 'Execute shell' marked build as failure
Run condition [Always] enabling perform for step [[]]
TAP Reports Processing: START
Looking for TAP results report in workspace using pattern: *.tap
Did not find any matching files. Setting build result to FAILURE.
Checking ^not ok
Jenkins Text Finder: File set '*.tap' is empty
Notifying upstream projects of job completion
Finished: FAILURE

danbev · 2017-08-21T05:40:31Z

test/addons/openssl-client-cert-engine/test.js

A hasCrypto check should be added (before require('https')) to enable it to pass when --without-ssl is specified:

if (!common.hasCrypto) common.skip('missing crypto');

Fixed, thanks!

danbev · 2017-08-21T05:46:14Z

test/addons/openssl-client-cert-engine/binding.gyp

Nit: indentation

Fixed (I think).

danbev · 2017-08-21T07:18:23Z

test/addons/openssl-client-cert-engine/test.js

I think this needs to use:

`./build/${common.buildType}/testengine.engine`);

to be able to support debug and release build types.

Ooh, good, thanks, fixed.

indutny

LGTM, but I'd split re-ordering of options in documentation into a separate commit. It was pretty hard to review because of this.

danbev · 2017-08-21T07:58:57Z

@Trott Not sure if this helps or not, but I was able to get this to link using:

{
  'targets': [
    {
      'target_name': 'testengine',
      'conditions': [
         ['node_use_openssl=="true"', {
           'type': 'shared_library',
           'sources': [ 'testengine.cc' ],
           'include_dirs': ['../../../deps/openssl/openssl/include'],
           'product_extension': 'engine',
           'link_settings': {
             'libraries': [
               '<(PRODUCT_DIR)/../../../../../out/<(PRODUCT_DIR)/libopenssl.a',
             ],
           },
         }]
      ]
    },
  ]
}

This allowed me to run the build successfully. But this does not take into account the case if openssl is configured as shared (and perhaps others cases).

Trott · 2017-09-08T05:33:26Z

@danbev Cool, I incorporated your gyp file changes and stuff works. Hooray! Guess I need to build a debug instance, a shared lib instance, and a no-ssl instance to check that it works under those situations. Here we go....

Debug test
--without-ssl test (using the Python test runner fails miserably, but it does on master too; running the tests individually without the test runner is successful for the tests modified/added in this PR)
shared library test

addaleax

Looks good as far as I can tell!

mhdawson · 2017-09-08T18:39:45Z

@sam-github FYI

Trott · 2017-09-11T06:35:52Z

I compiled with shared OpenSSL Predictably, the addon test failed.

I used:

rm -rf out/Release && ./configure --shared-openssl --shared-openssl-includes=/usr/local/opt/openssl/include --shared-openssl-libpath=/usr/local/opt/openssl/lib && make -j4 && make test-addons

Haven't looked much at fixing it, but did want to document what I did so far for when I come back to it later (or, even better, if someone else wants to pick it up and come up with the solution).

danbev · 2017-09-11T13:14:08Z

@Trott I think a condition for node_shared_openssl might be needed. For example:

{
  'targets': [
    {
      'target_name': 'testengine',
      'conditions': [
        ['node_use_openssl=="true"', {
          'type': 'shared_library',
          'sources': [ 'testengine.cc' ],
          'include_dirs': ['../../../deps/openssl/openssl/include'],
          'product_extension': 'engine',
          'link_settings': {
            'libraries': [
              '<(PRODUCT_DIR)/../../../../../out/<(PRODUCT_DIR)/libopenssl.a',
            ],
          },
        }],
        ['node_shared_openssl=="true"', {
          'include_dirs=': ['/usr/local/opt/openssl/include'],
          'link_settings': {
            'libraries=': [
              '/usr/local/opt/openssl/lib/libcrypto.a',
            ],
          },
        }]
      ]
    },
  ]
}

sam-github · 2017-09-11T17:20:33Z

doc/api/tls.md

good idea to alphabetize - don't capitals come before lower case in ANSI/C/ASCII locales?

To simplify the diff and to avoid discussion about whether or not it makes sense to take case into consideration (probably for machines, probably not for humans, etc.), I'm backing out this alphabetization. There are options objects all over the docs where the options are not alphabetized.

The way forward on this IMO would be to add a guideline about it to the style guide, let people bikeshed over it, and land it. Then we can apply it to all the options objects in the docs.

sam-github · 2017-09-11T17:21:38Z

lib/_tls_common.js

null and undefined should be allowed (and ignored), but perhaps any other type should be rejected as invalid usage?

Sure, done. (Still have to add a test, but since this has to be moved to the new error system anyway, I've made a note-to-self to add a test then.)

sam-github · 2017-09-11T17:42:17Z

My guess, from having worked with hardward based sec devices, is that if you (somehow?) have a ossl engine that stores your private key in hardward, that the client cert engine can be used to do the client-side cryptographic operations to prove its identity to the server side.

I can't really tell from the docs whether my guess is right. I even googled for "openssl client cert engine", but didn't see anything obvious. So, I think the docs are not so complete, but I guess that can be patched up later.

Trott · 2017-09-12T06:57:03Z

@danbev Since the root config.gypi contains the necessary paths to the shared libraries/headers, I've done this:

{
  'includes': ['../../../config.gypi'],

  'targets': [
    {
      'target_name': 'testengine',
      'conditions': [
        ['node_use_openssl=="true"', {
          'type': 'shared_library',
          'sources': [ 'testengine.cc' ],
          'product_extension': 'engine'
        }],
        ['node_use_openssl=="true" and node_shared_openssl=="false"', {
          'include_dirs': ['../../../deps/openssl/openssl/include'],
          'link_settings': {
            'libraries': [
              '<(PRODUCT_DIR)/../../../../../out/<(PRODUCT_DIR)/libopenssl.a',
            ],
          }     
        }]
      ]
    },
  ]
}

That seems to work with both the default and with shared libs. Looks good to you?

Trott · 2017-11-08T13:49:48Z

Argh, typo in the ifndef. Fixing....

~~OK, one more time, CI: https://ci.nodejs.org/job/node-test-pull-request/11306/~~

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

Trott · 2017-11-11T21:42:55Z

Landed in 6ee985f, de917f8, and 829d8f1.

Congratulations, @joelostrowski! 🎉 Sorry it took so long.

bnoordhuis · 2017-11-12T14:42:47Z

Back-porters: this should be back-ported along with #16964 or #16965, whichever lands.

joelostrowski · 2017-11-13T08:42:18Z

Amazing stuff @Trott :-)

MylesBorins · 2017-12-12T09:16:28Z

@Trott you missed the meta data on this one 😢

The PR number included for this api addition was originally incorrect. Refs: nodejs#14903

The PR number included for this api addition was originally incorrect. PR-URL: nodejs#17630 Refs: nodejs#14903 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Add an option 'clientCertEngine' to `tls.createSecureContext()` which gets wired up to OpenSSL function `SSL_CTX_set_client_cert_engine`. The option is passed through from `https.request()` as well. This allows using a custom OpenSSL engine to provide the client certificate.

Added `clientCertEngine` option to `https` and `tls` docs.

The PR number included for this api addition was originally incorrect. PR-URL: #17630 Refs: #14903 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Add an option 'clientCertEngine' to `tls.createSecureContext()` which gets wired up to OpenSSL function `SSL_CTX_set_client_cert_engine`. The option is passed through from `https.request()` as well. This allows using a custom OpenSSL engine to provide the client certificate.

Added `clientCertEngine` option to `https` and `tls` docs.

MylesBorins · 2018-05-22T14:56:56Z

We have an upcoming Semver-Minor release on v8.x
Should this be considered?

Trott · 2018-05-22T21:41:38Z

We have an upcoming Semver-Minor release on v8.x
Should this be considered?

I'm not sure what the criteria are, but I'd say "no" unless it happens to cherry-pick cleanly, which would surprise me.

jasnell · 2018-08-17T20:30:53Z

Not convinced this needs to be backported to 8.x

Trott added the wip Issues and PRs that are still a work in progress. label Aug 17, 2017

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 17, 2017

addaleax reviewed Aug 17, 2017

View reviewed changes

mscdex added the openssl Issues and PRs related to the OpenSSL dependency. label Aug 18, 2017

Trott force-pushed the openssl-clientcertengine-support2 branch 3 times, most recently from 0b3a4a8 to 4b829ff Compare August 20, 2017 23:01

danbev suggested changes Aug 21, 2017

View reviewed changes

danbev reviewed Aug 21, 2017

View reviewed changes

indutny approved these changes Aug 21, 2017

View reviewed changes

Trott force-pushed the openssl-clientcertengine-support2 branch from 4b829ff to bf5ef60 Compare September 8, 2017 02:54

Trott force-pushed the openssl-clientcertengine-support2 branch from 3eeb978 to d6fee40 Compare September 8, 2017 11:37

addaleax approved these changes Sep 8, 2017

View reviewed changes

danbev approved these changes Sep 8, 2017

View reviewed changes

Trott changed the title ~~[WIP] Openssl clientcertengine support2~~ Openssl clientcertengine support2 Sep 8, 2017

Trott removed the wip Issues and PRs that are still a work in progress. label Sep 8, 2017

sam-github reviewed Sep 11, 2017

View reviewed changes

Trott force-pushed the openssl-clientcertengine-support2 branch 3 times, most recently from f55f5a8 to 5c1c0e4 Compare September 12, 2017 06:48

Trott added 2 commits November 8, 2017 13:48

squash: nits

1501e2c

squash: typo

085a847

Trott force-pushed the openssl-clientcertengine-support2 branch from 474fb50 to 085a847 Compare November 8, 2017 13:50

Trott closed this Nov 11, 2017

MylesBorins mentioned this pull request Dec 12, 2017

docs: fix link for https api change #17630

Closed

MylesBorins added a commit to MylesBorins/node that referenced this pull request Dec 12, 2017

docs: fix link for https api change

2627f85

The PR number included for this api addition was originally incorrect. Refs: nodejs#14903

gibfahn referenced this pull request Dec 19, 2017

doc: add clientCertEngine to docs

7d49bd0

Added `clientCertEngine` option to `https` and `tls` docs.

gibfahn referenced this pull request Dec 19, 2017

test: add tls clientcertengine tests

121245f

gibfahn mentioned this pull request Dec 19, 2017

src: fix SetClientCertEngine() nullptr dereference #16965

Merged

MylesBorins added the dont-land-on-v6.x label Feb 27, 2018

JackOfMostTrades mentioned this pull request Mar 5, 2018

EC_POINT_cmp returns error when using a dynamic ENGINE and statically linked openssl openssl/openssl#5475

Closed

targos referenced this pull request Apr 6, 2018

doc: add clientCertEngine to docs

de917f8

Added `clientCertEngine` option to `https` and `tls` docs.

targos referenced this pull request Apr 6, 2018

test: add tls clientcertengine tests

829d8f1

Trott deleted the openssl-clientcertengine-support2 branch January 13, 2022 22:47

Uh oh!

Openssl clientcertengine support2 #14903

Openssl clientcertengine support2 #14903

Uh oh!

Conversation

Trott commented Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott commented Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

indutny left a comment

Choose a reason for hiding this comment

Uh oh!

danbev commented Aug 21, 2017

Uh oh!

Trott commented Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson commented Sep 8, 2017

Uh oh!

Trott commented Sep 11, 2017

Uh oh!

danbev commented Sep 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-github commented Sep 11, 2017

Uh oh!

Trott commented Sep 12, 2017

Uh oh!

Trott commented Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Nov 11, 2017

Uh oh!

bnoordhuis commented Nov 12, 2017

Uh oh!

joelostrowski commented Nov 13, 2017

Uh oh!

MylesBorins commented Dec 12, 2017

Uh oh!

MylesBorins commented May 22, 2018

Uh oh!

Trott commented May 22, 2018

Uh oh!

jasnell commented Aug 17, 2018

Uh oh!

Reviewers

Trott commented Aug 17, 2017 •

edited

Loading

Trott commented Aug 21, 2017 •

edited

Loading

Trott commented Sep 8, 2017 •

edited

Loading

Trott commented Nov 8, 2017 •

edited

Loading