Openssl clientcertengine support2 #6569
Conversation
nodejs-github-bot
added
the
c++
|
cc @nodejs/crypto |
|
@indutny this is my new pull-request - I hope you are happy with it and no further changes are required. If you still have comments it would be great with a hint or two on how to successfully rebase without polluting the pull-request with other peoples commits. Thanks! |
|
@indutny you were so responsive earlier so I just wonder if something went wrong with my pull-request? If you are just busy I understand of course. Thanks. |
src/node_crypto.cc
Outdated
| // This is because SSL_CTX_set_client_cert_engine does not itself | ||
| // 'support' multiple calls by cleaning up before overwriting the | ||
| // client_cert_engine internal context variable | ||
| if (sc->ctx_->client_cert_engine != NULL) { |
There was a problem hiding this comment.
It doesn't look like this is being set anywhere else.
There was a problem hiding this comment.
What do you mean?
There was a problem hiding this comment.
Aaah, I have overlooked that you are accessing SSL_CTX here. Isn't this something that OpenSSL should do? If OpenSSL doesn't do this, perhaps we should store it in a property that we have explicit control over?
There was a problem hiding this comment.
It's a good question. It's a bug I believe in OpenSSL that SSL_CTX_set_client_cert_engine does not clean up a previous set engine by calling ENGINE_finish. We could maintain an explicit property that can tell us if we have previously set an engine - but if OpenSSL suddenly fixes their issue it would still be wrong to ENGINE_finish the engine (as OpenSSL would do this)... I came up with a solution that I think has the greatest chance of not breaking in the future. If it breaks it would be because OpenSSL changed their internal variable name but in that case it would cause a build error which is to be preferred over a run-time crash due to double ENGINE_finish.
Also given my git problems I would really love not having to continue work on this pull-request ;-)
There was a problem hiding this comment.
I'm inclined to say it should throw a JS exception instead of trying to silently fix up. I mean, when would you change the engine during the lifetime of a SSL_CTX anyway?
There was a problem hiding this comment.
I don't know... from an API point of view I think it would violate the 'principle of least surprise' if it threw the second time you called it (given the name of the function). I may not be able to come up with an example of changing the engine during the lifetime but maybe someone needs to set an engine on an object outside his own life-cycle-control? That guy would be annoyed! With the above work-around for missing cleanup in OpenSSL the API behaves as one would expect?
There was a problem hiding this comment.
We can address that when it comes up. I'm hesitant to accept changes that silently fix up things behind openssl's back.
Maybe you can report this to the openssl project and see what they say? I'd be alright with cherry-picking an upstream fix ahead of time.
|
@joelostrowski sorry, I got a bit distracted. One last nit, otherwise LGTM. Need another +1 from @nodejs/crypto before landing this. |
src/node_crypto.cc
Outdated
| @@ -226,6 +226,43 @@ static int CryptoPemCallback(char *buf, int size, int rwflag, void *u) { | |||
| return 0; | |||
| } | |||
|
|
|||
| // Loads OpenSSL engine by engine id and returns it. The loaded engine | |||
| // gets a reference so remember the corresponding call to ENGINE_free | |||
| // In case of error the appropriate js exception is schduled | |||
|
@bnoordhuis @indutny all good comments! I would need to go one more time I can see... can one of you help me do the rebase correctly? Last time git screwed me over and the pull-request suddenly contained all the new stuff I rebased in. I just did like the guide stated: Alternatively can I just not rebase and push my changes only? |
|
@indutny @bnoordhuis with 6.7.0 which "Remove support for loading dynamic third-party engine modules" is this then dead in the water or is it only related to default loading of third-party engine modules? |
|
@joelostrowski That was about built-in engines that silently loaded drivers from shared objects. Manually loading an engine should still work. Apropos this pull request, I'm afraid I don't have time to review it at the moment. |
|
@sam-github maybe related to the other OpenSSL stuff you've been looking at? |
|
@bnoordhuis I think this has already been reviewed down to the last whitespace ;-) |
|
Conceptually I'm +1 on this but don't feel entirely qualified to sign off. I'm wondering though whether the change in the result of @joelostrowski I know this has been (and may continue to be) a long process, I'm glad you've stuck with it so far! Large changes like this that involve specialist knowledge that only a limited number of team members have can be difficult to get through and it takes patience and persistence. |
|
I'd be interested in looking at this more closely if I can make some time, but off the top of my head I can say the docs are inadequate. What is it for? Its basically documented just by reference to This literally just loads the cert? Or is it loading the cert and private key so the ENGINE is being used to do full client authentication to the server? And what about servers? Would an equivalent be useful for them? Is there going to be a growing set of properties to load specific engines for specific parts of the crypto during TLS? Also, is there any way to test this, without a hardware card? |
sam-github
added
tls
|
Hi @sam-github. True that OpenSSL documentation is bad - go figure :-) There should not be a need for a server-side equivalent of this. I have provided a test which builds a custom client engine which implements the EngineLoadSSLClientCert by providing a test certificate. I needed to add this functionality because of a customer demand. They have a device that require reading the client certificate from a hardware area in a custom manner. What would you suggest for getting to the finish line? Thanks, |
|
I suggest extending the docs to answer my questions above (such as whether its just the cert being read, or the cert and the key). I'll read the test code as soon as I get a chance, but our user's shouldn't have to do that to understand how a feature works. |
|
ping |
|
@sam-github pong |
|
docs? cf. #6569 (comment) |
|
@sam-github sorry, this have dropped down my priority list so I have not gotten around to progressing this. I really don't know a lot about security and OpenSSL. I just wired up a function to the library that was not wired up that my client needed. This is a fairly simple patch but I thought at the time that it could be useful to provide it back to the Node project - in case others needed it and also to not having to manage the patch when upgrading Node for my customer. My initial experience with providing a pull-request was good - there was good responsiveness and feedback and I felt I was moving quickly towards the goal. However, it has been like cutting the remaining distance in half for each iteration and at some point I felt discouraged about the outlook of actually reaching the finish line. It feels like the nitpicking is never-ending to a degree that has been surprising considering other areas of Node, it's API design decisions, implementation details and documentation. So currently I am a bit at the point of "whatEVER"... it seems that I am the only one needing this anyway? Happy New Year... |
|
It can be tedious getting a PR through the final rounds of review, its slow for some of my PRs, too. It goes faster when you respond to comments faster. Ben pinged you once a month all summer to ask you to punctuate your comments, for example, and you still have not. While I can understand your frustration given some of the current state of the repo, its a mistake to compare the standard this PR is being held to to past node history when PRs merged without docs, and with other quality probems. Its because of the past that we try not to do that anymore. This PR adds a feature, but lacks sufficient docs for the feature to be used, IMO. Merging it creates a maintenance problem, some future dev will come, look at the docs, and leave unhappy messages in the issue tracker that we then have to deal with, long after you (and maybe me) are gone. I'd like to avoid that. I have some interest in engines, and may complete this some time if you aren't interested in doing so, or perhaps someone else who needs the feature immediately will see your work and pick it up. Thanks for getting this much done, its sure to be useful sometime. |
|
@joelostrowski Any chance getting this rebased? Or are you unlikely to come back to this at this point? |
|
@Trott is this likely to get pulled? I did not have a very good personal experience with this PR in its course. |
Sorry to hear that. Let me see if I can rebase it and fix the last few nits... |
|
:-) thanks @Trott!! Much appreciated! Let me know if you want me to do something. |
|
Ping @Trott |
|
@Trott nice! @joelostrowski thanks a lot for your contribution. I am sorry that your could not land as is but I guess it is the basis for the new PR and your work is much appreciated! Hopefully this feature is soon in Node.js. Closing this to reduce the overhead of multiple PRs. |
|
YAY :-) |
Checklist
Affected core subsystem(s)
tls, crypto, https
Description of change
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.