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 New User Interface functions not exported in the Windows version of Node 10 #27494

Closed
nkochakian opened this issue Apr 30, 2019 · 8 comments
Labels
openssl Issues and PRs related to the OpenSSL dependency. windows Issues and PRs related to the Windows platform.

Comments

@nkochakian
Copy link

  • Version: 10.15.3
  • Platform: Windows
  • Subsystem: Native library/OpenSSL

Symbols for the UI_* functions are not present in either version node.lib for the Windows version of Node 10. This is an issue when building native addons, since OpenSSL doesn't appear to be configured in a way that would exclude them (OPENSSL_NO_UI is not defined.) These functions are available in the Linux version of Node, however.

@sam-github
Copy link
Contributor

I believe you, but I'm not having any success figuring out how that is controlled. Do you know?

@sam-github
Copy link
Contributor

OK, found fcaeb1f

It's possible this will work, can you check?

% git diff
diff --git a/node.gyp b/node.gyp
index 1ab6b15d73..15a11d0541 100644
--- a/node.gyp
+++ b/node.gyp
@@ -816,7 +816,7 @@
               # Categories to export.
               '-CAES,BF,BIO,DES,DH,DSA,EC,ECDH,ECDSA,ENGINE,EVP,HMAC,MD4,MD5,'
               'PSK,RC2,RC4,RSA,SHA,SHA0,SHA1,SHA256,SHA512,SOCK,STDIO,TLSEXT,'
-              'FP_API,TLS1_METHOD,TLS1_1_METHOD,TLS1_2_METHOD,SCRYPT,OCSP,'
+              'FP_API,TLS1_METHOD,TLS1_1_METHOD,TLS1_2_METHOD,SCRYPT,OCSP,UI,'
               'NEXTPROTONEG,RMD160,CAST,DEPRECATEDIN_1_1_0,DEPRECATEDIN_1_2_0',
               # Defines.
               '-DWIN32',

@nkochakian
Copy link
Author

nkochakian commented Apr 30, 2019

It's entirely possible that the intention was to not expose interactive functions, which is probably not something that you would want an addon to have access to. The issue is that the configuration doesn't seem to be consistent across platforms (these functions are present in Linux but not Windows), and the macros exposed by Node's copy of the OpenSSL configuration headers can't be used to reliably test for the presence or absence of this feature.

@sam-github
Copy link
Contributor

I'm not sure we go through much effort to decide what people can or cannot do with the linked copy of OpenSSL. Direct calls to OpenSSL are the users responsibility, for better or worse.

AFAICT, the situation on 10.x is same as master and 12.x.

What is it that you would like? I can regenerate the config with the 'no-ui' config option (though see below). Or, its possible the patch above will export the symbols. Exporting the symbols seems less likely to break any existing user, removing symbols would be semver-major, so wouldn't backport to 10.x

  *) The UI API becomes a permanent and integral part of libcrypto, i.e.
     not possible to disable entirely.  However, it's still possible to
     disable the console reading UI method, UI_OpenSSL() (use UI_null()
     as a fallback).

     To disable, configure with 'no-ui-console'.  'no-ui' is still
     possible to use as an alias.  Check at compile time with the
     macro OPENSSL_NO_UI_CONSOLE.  The macro OPENSSL_NO_UI is still
     possible to check and is an alias for OPENSSL_NO_UI_CONSOLE.
     [Richard Levitte]
  • openssl-1.1.1a/CHANGES

@nkochakian
Copy link
Author

It might be best to have the symbols exported in the Windows version for consistency. There might be an issue with the configuration headers, though. I was unable to check if the various opensslconf.h files reflect the configuration that Node is actually using.

@sam-github
Copy link
Contributor

They should be consistent, they are generated by the config scripts all in a single batch, windows included. At least, I'm not aware of any reports of problems.

Anyhow, if you check the diff above and confirm it works, I'll PR it and raise a backport to 10.x.

@nkochakian
Copy link
Author

Changing the gyp file adds the UI functions to the library

@ChALkeR ChALkeR added windows Issues and PRs related to the Windows platform. openssl Issues and PRs related to the OpenSSL dependency. labels Apr 30, 2019
@sam-github
Copy link
Contributor

@nkochakian PTAL: #27586

sam-github added a commit to sam-github/node that referenced this issue May 6, 2019
Node.js compiles them, their existence is indicated by OpenSSL header
defines, but they can't be linked to on Windows because their symbols
are not exported. Export them.

Fixes: nodejs#27494
targos pushed a commit that referenced this issue May 10, 2019
Node.js compiles them, their existence is indicated by OpenSSL header
defines, but they can't be linked to on Windows because their symbols
are not exported. Export them.

Fixes: #27494

PR-URL: #27586
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
openssl Issues and PRs related to the OpenSSL dependency. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants