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

Openssl clientcertengine support2 #14903

Closed
wants to merge 5 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Aug 17, 2017

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

@Trott Trott added the wip Issues and PRs that are still a work in progress. label Aug 17, 2017
@nodejs-github-bot 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
'conditions': [
['node_use_openssl=="true"', {
'type': 'shared_library',
'sources': [ 'testengine.cc' ],
Copy link
Member

Choose a reason for hiding this comment

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

This should get indented like JS objects

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@@ -162,6 +162,13 @@ exports.createSecureContext = function createSecureContext(options, context) {
c.context.setFreeListLength(0);
}

if (options.clientCertEngine) {
if (c.context.setClientCertEngine)
c.context.setClientCertEngine(options.clientCertEngine);
Copy link
Member

Choose a reason for hiding this comment

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

This should typcheck that options.clientCertEngine is a string

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@mscdex mscdex added the openssl Issues and PRs related to the OpenSSL dependency. label Aug 18, 2017
@Trott Trott force-pushed the openssl-clientcertengine-support2 branch 3 times, most recently from 0b3a4a8 to 4b829ff Compare August 20, 2017 23:01
@Trott
Copy link
Member Author

Trott commented Aug 21, 2017

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

@@ -0,0 +1,55 @@
'use strict';
const common = require('../../common');
const assert = require('assert');
Copy link
Contributor

Choose a reason for hiding this comment

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

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');

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, thanks!

'target_name': 'testengine',
'conditions': [
['node_use_openssl=="true"', {
'type': 'shared_library',
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: indentation

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed (I think).

const fs = require('fs');
const path = require('path');

const engine = path.join(__dirname, '/build/Release/testengine.engine');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to use:

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

to be able to support debug and release build types.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ooh, good, thanks, fixed.

Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

danbev commented Aug 21, 2017

@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 Trott force-pushed the openssl-clientcertengine-support2 branch from 4b829ff to bf5ef60 Compare September 8, 2017 02:54
@Trott
Copy link
Member Author

Trott commented Sep 8, 2017

@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

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

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Looks good as far as I can tell!

@mhdawson
Copy link
Member

mhdawson commented Sep 8, 2017

@sam-github FYI

@Trott Trott changed the title [WIP] Openssl clientcertengine support2 Openssl clientcertengine support2 Sep 8, 2017
@Trott Trott removed the wip Issues and PRs that are still a work in progress. label Sep 8, 2017
@Trott
Copy link
Member Author

Trott commented Sep 11, 2017

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
Copy link
Contributor

danbev commented Sep 11, 2017

@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',
            ],
          },
        }]
      ]
    },
  ]
}

doc/api/tls.md Outdated
@@ -1067,18 +1076,19 @@ changes:
When the server receives both NPN and ALPN extensions from the client,
ALPN takes precedence over NPN and the server does not send an NPN
extension to the client.
* `NPNProtocols` {string[]|Buffer[]|Uint8Array[]|Buffer|Uint8Array}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

c.context.setClientCertEngine(options.clientCertEngine);
else
throw new Error('Custom engines not supported by this OpenSSL');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

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 Trott force-pushed the openssl-clientcertengine-support2 branch 3 times, most recently from f55f5a8 to 5c1c0e4 Compare September 12, 2017 06:48
@Trott
Copy link
Member Author

Trott commented Sep 12, 2017

@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
Copy link
Member Author

Trott commented Nov 8, 2017

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 Trott force-pushed the openssl-clientcertengine-support2 branch from 474fb50 to 085a847 Compare November 8, 2017 13:50
@Trott
Copy link
Member Author

Trott commented Nov 11, 2017

Landed in 6ee985f, de917f8, and 829d8f1.

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

@Trott Trott closed this Nov 11, 2017
@bnoordhuis
Copy link
Member

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

@joelostrowski
Copy link
Contributor

Amazing stuff @Trott :-)

@MylesBorins
Copy link
Contributor

@Trott you missed the meta data on this one 😢

MylesBorins added a commit to MylesBorins/node that referenced this pull request Dec 12, 2017
The PR number included for this api addition was originally incorrect.

Refs: nodejs#14903
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Dec 12, 2017
The PR number included for this api addition was originally incorrect.

PR-URL: nodejs#17630
Refs: nodejs#14903
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
gibfahn referenced this pull request Dec 19, 2017
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.
gibfahn referenced this pull request Dec 19, 2017
Added `clientCertEngine` option to `https` and `tls` docs.
MylesBorins added a commit that referenced this pull request Jan 10, 2018
The PR number included for this api addition was originally incorrect.

PR-URL: #17630
Refs: #14903
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins added a commit that referenced this pull request Jan 10, 2018
The PR number included for this api addition was originally incorrect.

PR-URL: #17630
Refs: #14903
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos referenced this pull request Apr 6, 2018
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.
targos referenced this pull request Apr 6, 2018
Added `clientCertEngine` option to `https` and `tls` docs.
@MylesBorins
Copy link
Contributor

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

@Trott
Copy link
Member Author

Trott commented May 22, 2018

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
Copy link
Member

jasnell commented Aug 17, 2018

Not convinced this needs to be backported to 8.x

@Trott Trott deleted the openssl-clientcertengine-support2 branch January 13, 2022 22:47
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++. lib / src Issues and PRs related to general changes in the lib or src directory. openssl Issues and PRs related to the OpenSSL dependency. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.