src: revert removal of SecureContext _external getter

[dyatlov]

This _external getter is essential for some libs to work:
uWebSockets as an example.

It reverts this PR: #20237
Use case which that PR broke: https://github.com/uNetworking/uWebSockets-bindings/blob/master/nodejs/src/uws.js#L508

Checklist

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

[dyatlov]

@addaleax created a PR as you suggested.

[addaleax]

Can you link to the original PR in the commit message?

Also, I think you need to fix this up a bit:

  CXX(target) /home/travis/build/nodejs/node/out/Release/obj.target/node_lib/src/tls_wrap.o
../src/node_crypto.cc:1365:58: error: no viable conversion from 'node::crypto::SSLCtxPointer' (aka 'std::unique_ptr<ssl_ctx_st, node::FunctionDeleter<ssl_ctx_st, &SSL_CTX_free> >') to 'void *'
  Local<External> ext = External::New(info.GetIsolate(), sc->ctx_);
                                                         ^~~~~~~~
../deps/v8/include/v8.h:5092:54: note: passing argument to parameter 'value' here
  static Local<External> New(Isolate* isolate, void* value);
                                                     ^
1 error generated.

Adding .get() to sc->ctx_ might be enough.

[addaleax]

LGTM as a short-term fix, but just to be clear, this does not mean that this is a supported API and will be kept around longer than necessary.

If you want to expect being able to use the library without risking breakage, you need to take the larger issue of internals misuse up with the library’s maintainers as soon as possible and push it through.

[jasnell]

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

[jasnell]

What's the status on this one?

[addaleax]

I think it could land, if somebody just started a new CI (I can currently not access the interface).

[addaleax]

Still LGTM, but again, this is a temporary workaround; you should expect this to be deprecated and removed again at some point, and you need to address this in the library’s code.

If necessary, we can work out a public API.

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

[addaleax]

@dyatlov Sorry, we don’t usually do merge commits here – CI can’t handle that…

New CI (without automatic rebasing): https://ci.nodejs.org/job/node-test-pull-request/17708/

[SirAnthony]

Still LGTM, but again, this is a temporary workaround; you should expect this to be deprecated and removed again at some point, and you need to address this in the library’s code.

If necessary, we can work out a public API.

Since it is just wrapper for c++ SSL structure pointer and useful only inside node c++ addons, maybe it will be enough to provide a way to get that pointer only in c++ code?

[addaleax]

Since it is just wrapper for c++ SSL structure pointer and useful only inside node c++ addons, maybe it will be enough to provide a way to get that pointer only in c++ code?

Yes, that would most likely be a C++ API… It’s a bit hard to tell what the expected semantics are, since the original repository linked in the PR description seems to be gone?

[addaleax]

Landed in 3f08c00

[hermanbanken]

@dyatlov, did you get UWS to work with Node 11 again after this?

I'm still struggling. I patched uws.js to look into ssl._secureContext.context._external, but that doesn't seem to be enough.

- const sslState = socket.ssl ? socket.ssl._external : null;
+ const sslState = socket.ssl ? (socket.ssl._external || socket.ssl._secureContext.context._external) : null;

[sam-github]

@hermanbanken I found uws was using V8 APIs deprecated in 11.x and wouldn't compile at all. On the other hand, this PR is in https://github.com/nodejs/node/commits/v10.x-staging so should be in the upcoming 10.x release. Try building against that branch.

[addaleax]

@hermanbanken @sam-github That’s good to know… since you mentioned it’s unmaintained, should we go back to removing or at least deprecating this?

[sam-github]

Removing is demonstrably semver-major, so I don't think we should do that 10.x. Deprecating in 11.x and removing in 12.x would be workable.

I understand why you removed it, @addaleax , less code := more maintainable almost by definition... but the cost of this code is pretty low. I don't object to its orderly removal, but I also don't see see any problem leaving it around.

[addaleax]

@sam-github Yeah, I don’t disagree. And I’m still open to providing some sort of official public API to achieve these goals, but … I am somewhat afraid of this being a blocker for some future refactor.

I would aim for a runtime deprecation not before Node 12 (and so removal not before Node 13), just to be clear.

[sam-github]

As long as there is an OpenSSL, there will be an SSL context. Its more work for a fringe use-case, but maybe there is some way to have a pure C++ API, perhaps node::crypto::GetSecureContextFromHandle(), that would allow C++ addons to get the SecureContext? This would make more sense to me, given that the SecureContext can only be used with the SSL_ APIs by C++ code, only C++ needs to get it. This would still have the positive effect of removing the context from the js API. Just a thought.

[addaleax]

@sam-github Yes, that sounds about right for an “official public API” :)