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

tls: expose openssl library version #24999

Closed

Conversation

sam-github
Copy link
Contributor

Node can be dynamically linked against a different version of OpenSSL
than it was compiled against, so expose the actual library version.

cf. #24658 (comment)

@bnoordhuis @rvagg @nodejs/crypto I'm not sure if this is a good idea, perhaps its overkill for an exceedingly rare occurence. Has this been discussed or considered before?

Still waiting on #24658 to see if they are built against a different version of ossl than they are linked against.

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

Node can be dynamically linked against a different version of OpenSSL
than it was compiled against, so expose the actual library version.
@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 Dec 12, 2018
@addaleax
Copy link
Member

Hm … would it make more sense to change the source of truth from OPENSSL_VERSION_TEXT to OpenSSL_version(OPENSSL_VERSION) and leave a single entry in process.versions?

@sam-github
Copy link
Contributor Author

Its not clear whether thats a benefit, it hides a different piece of information, then we'd know what the library version was, but wouldn't know what we were compiled against. Is that an improvement?

Btw, I didn't go through the work, but if its worth it, something similar would be possible for zlib which also has a macro and a function, maybe for cares, too. Both report the header/compiled version ATM. Maybe a versions.dll.{openssl,cares,zlib}?

@addaleax
Copy link
Member

@sam-github Yeah, I’m just having a hard time coming up with a benefit of exposing the version for which we compiled… might just be that I’m overlooking something. :)

@sam-github
Copy link
Contributor Author

The difference between the two could indicate a problem or explain odd behaviours.

@sam-github
Copy link
Contributor Author

particularly if you built against 1.1.0, but are running with 1.1.1, which might be what's happening in #24658

@addaleax
Copy link
Member

@sam-github I think it might make more sense then to expose the compile-time versions in a separate object? Usually inspecting versions would mean that one checks for some particular feature or run-time behaviour, and for those cases you’d want the actual version of the currently running code, right?

@sam-github
Copy link
Contributor Author

I can't think of a case where you'd want to check the version of node's deps programmatically, I think they are just for diagnostics when something went wrong.

Node's version someone might want to check, perhaps, as a last resort to figure out if it supports a feature, but not the underlying libs. Also, the header file version may change the features available, just as changing the underlying DLL may. For example, on upgrade_openssl1.1.1a branches, TLSv1.3 is a accepted as a min/max version string only if node was compiled against openssl 1.1.1. If it was compiled against 1.1.0, but running against 1.1.1, TLS1.3 will not be supported.

Anyhow, maybe the library versions are preferred. Its just not clear. Both the lib and header versions appear to convey different information, with different uses. But, they are always identical with our node.js builds, its only distros that do builds against system libs where they can even be different, and problems with those are the distro's problems, in general, not ours to fix.

@sam-github
Copy link
Contributor Author

Hm, turned out this would not have helped in #24658, though #22712 would have, if it printed the report on uncaught exception.

@bnoordhuis @rvagg Thoughts? This must have come up before.

@sam-github
Copy link
Contributor Author

closing due to underwhelming interest

@sam-github sam-github closed this Dec 17, 2018
@sam-github sam-github deleted the expose-openssl-library-version branch December 17, 2018 16:56
@rvagg
Copy link
Member

rvagg commented Dec 18, 2018

sorry, busy month, my head is elsewhere

I like the idea of exposing versions of linked libraries, but cluttering up process.versions like this seems like a smell.

What's the case for having both of them? Aren't we mostly interested in what's running in the current version? Or are there cases where OpenSSL isn't doing a good job at maintaining stability in major release lines?

Maybe, if there is a case for knowing the divergence, having a process.versions.opensslCompiled show up only when they diverge would make sense? If you only get the process.versions.openssl then you know that what you're running against now is different to what you were compiled against. That way we don't clutter the object up for the majority of users but the dynamic linkers (Linux distributions) get the pleasure of clutter.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants