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

https server should allow changing credentials dynamically #4464

Closed
petkaantonov opened this issue Dec 29, 2015 · 36 comments

Comments

Projects
None yet
@petkaantonov
Copy link
Contributor

commented Dec 29, 2015

We need to change the certificate info (credentials/secureContext) dynamically while the server is online without having to shut down and start a new server. We have patched node with code that enables this, but it would be nice to have it on upstream as well.

The API we have is:


server.setSharedCredentials(secureContext)

Used to change the server's TLS options such as the server certificate on the fly. secureContext must be an instance of SecureContext as is created with tls.createSecureContext().


After the call, if there were active sockets using old context, those sockets will be disconnected next time data is sent over them.

@mscdex mscdex added tls and removed https labels Dec 29, 2015

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Dec 30, 2015

An API like server.setSharedCredentials(secureContext) seems like a reasonable if somewhat esoteric addition to me. This however:

After the call, if there were active sockets using old context, those sockets will be disconnected next time data is sent over them.

Doesn't seem so reasonable if the implication is that the server needs to track active sockets. If the logic is something like if (this._secureContext !== server._secureContext) this.destroy(); in socket.write() it might be more palatable.

/cc @nodejs/crypto

@indutny

This comment has been minimized.

Copy link
Member

commented Dec 30, 2015

Perhaps it should be called secureContext and not sharedCredentials?

@indutny

This comment has been minimized.

Copy link
Member

commented Dec 30, 2015

I don't really like disconnecting sockets either, IMO this is not reasonable and has way too much implicit behavior for suggested method name.

@rvagg

This comment has been minimized.

Copy link
Member

commented Dec 30, 2015

@petkaantonov could you explain some use-cases for this? I can't imagine one so perhaps it'd be helpful for others as well to hear why this might be needed.

@tallskog

This comment has been minimized.

Copy link

commented Feb 9, 2016

The use case is an embedded system where https server is handling mission critical information. If administrator wants/needs to change the certificates, it will happen on the fly. Without this modification https server needs to be restarted leading to possible loss of data.

@petkaantonov

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2016

If the logic is something like if (this._secureContext !== server._secureContext) this.destroy(); in socket.write() it might be more palatable.

Iirc this check was in the "data" event. It doesn't make sense to allow invalid certificate to be used.

@jasnell

This comment has been minimized.

Copy link
Member

commented May 17, 2016

@petkaantonov ... still interested in this? I share some of the same concerns voiced already but would definitely welcome a PR to review.

@petkaantonov

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2016

I could work on a PR this weekend

@voxsoftware

This comment has been minimized.

Copy link

commented Oct 1, 2016

This it's really necessary

@sam-github

This comment has been minimized.

Copy link
Member

commented Dec 20, 2016

Without this modification https server needs to be restarted leading to possible loss of data.

By terminating existing connections when they try to write something, aren't you losing data?

@fresheneesz

This comment has been minimized.

Copy link

commented Dec 20, 2016

Why would you have to terminate existing connections? Why not just let existing connections stay open as long as they want, and only give the new cert to new connections? If you want to, you can always kill connections yourself.

@retrohacker

This comment has been minimized.

Copy link

commented Apr 21, 2017

For a Let's Encrypt certificate, the cert needs to be cycled every 90 days or so. Long-term, they are planning on pushing this number as low as 2 days.

In P2P protocols that have "trust", up-time and reliability are important in maintaining high trust on the network. Severing existing connections to cycle a server can hurt your reputation, but gracefully shutting down by rejecting new connections and finishing current connections increases the time your service is offline, especially when serving long-term connections (i.e. streaming a GB file to a client). While you can get around this in most enterprise settings by doing a rolling update of your clients, most end users aren't setup with load balancing, let alone rolling updates. They simply git clone or npm install -g and then start the app. Having the ability to hotswap a cert in memory seems to be the only clean way to do this.

SNI is a solution, but not all clients support SNI and it is really just a hack for the job, we aren't trying to serve multiple hostnames from a single ip. Also, SNI bleeds information outside of SSL (the hostname) in order for the server to determine which cert needs to be served.

It looks like the tls_server object already exposes tls_server.ca, tls_server.cert, and tls_server.key. I can't find these values documented anywhere, is it safe to update them during runtime?

cert

If so, we can simply

server.key = fs.readFileSync('./key')
server.cert = fs.readFileSync('./cert')
server.ca = fs.readFileSync('./ca')
@indutny

This comment has been minimized.

Copy link
Member

commented Apr 21, 2017

I know it is unofficial API, but has anyone tried: server._sharedContext.setCert() and setKey(). I wonder if it works?

@retrohacker

This comment has been minimized.

Copy link

commented Apr 21, 2017

@indutny, I can give it a shot! I'm out of my depth here though, I know just enough about SSL right now to be dangerous, which isn't enough to put together a convincing test case :-\

@retrohacker

This comment has been minimized.

Copy link

commented Apr 23, 2017

@indutny I'm not sure if this is convincing, but it seems to work!

Script:

var https = require('https')
var fs = require('fs')

var opts = {
  key: fs.readFileSync('./good_cert/privkey.pem', 'utf8'),
  cert: fs.readFileSync('./good_cert/fullchain.pem', 'utf8'),
  ca: fs.readFileSync('./good_cert/chain.pem', 'utf8')
}

var bad_opts = {
  key: fs.readFileSync('./bad_cert/privkey.pem', 'utf8'),
  cert: fs.readFileSync('./bad_cert/fullchain.pem', 'utf8'),
  ca: fs.readFileSync('./bad_cert/chain.pem', 'utf8')
}

var s = https.createServer(opts, (req, res) => {
  console.log('connect')
  res.writeHead(200)
  res.write('Hello World!\n')
  s._sharedCreds.context.setCert(bad_opts.cert)
  s._sharedCreds.context.setKey(bad_opts.key)
  res.end('Goodbye World!\n')
})
s.listen(9999)

Result:

blocksite_unmodified1

It looks like the current connection continues using the old cert, but the next connection uses the new cert.

Edit:
Even when I break the event loop (in case that mattered) the current connection still uses the old cert! This looks like it might be a reliable way to handle hotswapping certs! Broke event loop with: setTimeout(() => res.end('Goodbye World!\n'), 2000)

Edit 2:
I added global.gc() abvoe setTimeout to see if it would deallocate the old certs. It still works! Though that leads me to wonder how this is working under the hood. Does each connection maintain it's own reference to the cert it is using?

@indutny

This comment has been minimized.

Copy link
Member

commented Apr 23, 2017

Cool, thank you for testing! Each connection maintains the reference to the cert.

@bnoordhuis what do you think about making an API on top of this?

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Apr 23, 2017

@indutny No objections.

@grantila

This comment has been minimized.

Copy link

commented Aug 31, 2017

Another, perhaps more composable and flexible and even secure approach, would be to allow the certificate data to be provided through a callback instead of as static properties - key and cert (whether resettable through a setter or as currently, not).

This would allow for packages dedicated to ACMEv1 or ACMEv2 (or any other custom CA protocol) to fully take over responsibility of providing the certificates, including things like storing (disk or ram - potentially unswappable memory), caching, invalidation, lifecycle management etc. Leaving this to (forcing it upon) each application developer to handle such critical flow is surely possible but perhaps not ideal.

@grantila

This comment has been minimized.

Copy link

commented Aug 31, 2017

Each connection maintains the reference to the cert.

@indutny, not available to the application I hope? The cert should be irrelevant once the handshake is established, and especially the private key shouldn't be thrown around, imo. I don't want to have to trust all npm packages that get direct or indirect access to the socket.

@indutny

This comment has been minimized.

Copy link
Member

commented Aug 31, 2017

@grantila npm packages that you use have full access to the file system and networking. Although, getting cert is complicated, I don't see how one can trust more to them than we already do.

@nolimitdev

This comment has been minimized.

Copy link

commented Jan 16, 2018

Dear @retrohacker your code seems to works fine for me in websocket server. Thank you! I think we do not need global.gc() or close existing connections because valid certificate is only needed during handshake. When connection is established expired certificate is not problem for that connection. Once certificate file is changed new connections will use new certificate for handshake.

Question is why _sharedCreds.context.setCert and _sharedCreds.context.setKey are not part of public API? We can use it but without warranty because it is not documented.

Code below is minimalistic example for WSS websocket server with hot reloading certificate after certificate file is changed (by certbot or acme.sh)...

var ws = require('ws');
var fs = require('fs');
var https = require('https');

var options = {
    server : https.createServer({
        cert : fs.readFileSync('/path/to/crt'),
        key : fs.readFileSync('/path/to/key'),
    }).listen(2000)
};

var timeout;
fs.watch('/path/to/crt', () => {
    clearTimeout(timeout);
    timeout = setTimeout(() => {
        options.server._sharedCreds.context.setCert(fs.readFileSync('/path/to/crt'));
        options.server._sharedCreds.context.setKey(fs.readFileSync('/path/to/key'));
    }, 1000);
});

new ws.Server(options);

It is enough to watch just certificate file (because when private key is changed also certificate must be changed). Timeout is used because fs.watch fires callback several times depending on OS/platform.

@danosaure

This comment has been minimized.

Copy link

commented Jun 7, 2018

@nolimitdev I'm not sure how the setTimeout() helps you here, as it will still be called as many times as the fs.watch() is fired. You should look into threshold or debounce.

Thanks for your example, it helps me think about a solution for LetsEncrypt and 0-down time.

@nolimitdev

This comment has been minimized.

Copy link

commented Jun 7, 2018

@danosaure thanks to setTimeout() methods setCert() and setKey() fire only once per certificate file change. Depending on OS/platform re-saving file only once fires fs.watch callback several times during short time. You can add console.log() into fs.watch callback and you will understand. Of course you can ignore that little overhead by calling setCert() and setKey() more times and remove setTimeout() completely.

@danosaure

This comment has been minimized.

Copy link

commented Jun 7, 2018

@nolimitdev Sorry, I just noticed the clearTimeout(), that explains it. You are effectively doing a debounce() function.

For this case, you want the cert to be available as soon as possible, so you should look into throttle() (I misspoke with threshold earlier).

@nolimitdev

This comment has been minimized.

Copy link

commented Jun 7, 2018

@danosaure I do not need cert to be available as soon as possible because the old one is still valid for about next 2 weeks :)

@amruthar

This comment has been minimized.

Copy link

commented Oct 8, 2018

hi,

I have an express server that I start with
var opts = { key: fs.readFileSync('./certs/serverKey.pem','utf8'), cert: fs.readFileSync('./certs/serverCert.pem', 'utf8') , requestCert: true , rejectUnauthorized: true , ca:fs.readFileSync('./certs/clientCert.pem', 'utf8') }

var server = https.createServer(opts, app).listen(port, function() { logger.info("Server is running on port " + port ); })

server._sharedCreds.context.setCert(fs.readFileSync('/path/to/crt'));
server._sharedCreds.context.setKey(fs.readFileSync('/path/to/key'));

don't seem to be working for me (No such method setCert or setKey in the context object). The server still serves requests with the same server cert and key.

I have exposed an api in my server to change the server cert and key before it expires. I need to be able to do this without a server.close() and server.listen() again as it takes as long as a few minutes for the server.close() callback to be executed. Does anybody have a solution to this?

@nolimitdev

This comment has been minimized.

Copy link

commented Oct 8, 2018

@amruthar How it is possible that you have no such method setCert or setKey in the context object? I just tested different nodejs versions (v6, v7, v8, v9 and newest v10) and methods are available in each version. What version exactly do you use?

@amruthar

This comment has been minimized.

Copy link

commented Oct 8, 2018

@nolimitdev my bad, seems to work. I had checked with setCa which isn't a method apparently. Thank you. However, how does one modify the ca field in options to add or delete the ca certs used to authenticate client certs?

@nolimitdev

This comment has been minimized.

Copy link

commented Oct 8, 2018

@amruthar you can concat your domain cert with CA cert to one file and link it in "cert" option without using "ca" option and then you do not need sth. like setCa(). Try to search sth. like https://www.google.sk/search?q=concat+ca+and+cert

@amruthar

This comment has been minimized.

Copy link

commented Oct 9, 2018

@nolimitdev , I tried concatenating the end domain cert of my server along with the ca certs that are required for authenticating the client certs for incoming requests, my code looks like this now:

var certs = fs.readFileSync('./certs/serverCert.pem', 'utf8')+fs.readFileSync('./certs/clientCert.pem', 'utf8');

var opts = { key: fs.readFileSync('./certs/serverKey.pem','utf8'), cert: certs , requestCert: true , rejectUnauthorized: true }

var server = https.createServer(opts, app).listen(port, function() { logger.info("Server is running on port " + port ); })

However I am not able to make any requests as the server isn't able to verify the client cert that I am providing for the request. (since I've removed the ca field in options and concatenated ca certs in cert field itself.)

@nolimitdev

This comment has been minimized.

Copy link

commented Oct 9, 2018

Ca certs usually have expiration for several years. Maybe you do not need hot reloading for it at all. Just use hot reloading for domain certificate and key using setCert() and setKey().

@amruthar

This comment has been minimized.

Copy link

commented Oct 9, 2018

@nolimitdev , context.addCACert seems to work for adding additional CA certs, still can't find a way to remove an already added CA cert from the array though.

@amruthar

This comment has been minimized.

Copy link

commented Oct 9, 2018

@nolimitdev , I need to add a new CA cert, not a renewed CA cert that already existed. It is a custom CA cert, which means I have to add it in my trusted ca fields option.

cjihrig added a commit to cjihrig/node-1 that referenced this issue Oct 13, 2018

tls: support changing credentials dynamically
This commit adds a setSecureContext() method to TLS servers. In
order to maintain backwards compatibility, the method takes the
options needed to create a new SecureContext, rather than an
instance of SecureContext.

Fixes: nodejs#4464
Refs: nodejs#10349
Refs: nodejs/help#603
Refs: nodejs#15115

@cjihrig cjihrig referenced this issue Oct 13, 2018

Merged

tls: support changing credentials dynamically #23644

4 of 4 tasks complete

cjihrig added a commit to cjihrig/node-1 that referenced this issue Oct 16, 2018

tls: support changing credentials dynamically
This commit adds a setSecureContext() method to TLS servers. In
order to maintain backwards compatibility, the method takes the
options needed to create a new SecureContext, rather than an
instance of SecureContext.

Fixes: nodejs#4464
Refs: nodejs#10349
Refs: nodejs/help#603
Refs: nodejs#15115

cjihrig added a commit to cjihrig/node-1 that referenced this issue Oct 21, 2018

tls: support changing credentials dynamically
This commit adds a setSecureContext() method to TLS servers. In
order to maintain backwards compatibility, the method takes the
options needed to create a new SecureContext, rather than an
instance of SecureContext.

Fixes: nodejs#4464
Refs: nodejs#10349
Refs: nodejs/help#603
Refs: nodejs#15115
PR-URL: nodejs#23644
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

jasnell added a commit that referenced this issue Oct 21, 2018

tls: support changing credentials dynamically
This commit adds a setSecureContext() method to TLS servers. In
order to maintain backwards compatibility, the method takes the
options needed to create a new SecureContext, rather than an
instance of SecureContext.

Fixes: #4464
Refs: #10349
Refs: nodejs/help#603
Refs: #15115
PR-URL: #23644
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@gaxolio

This comment has been minimized.

Copy link

commented Nov 30, 2018

@nolimitdev , context.addCACert seems to work for adding additional CA certs, still can't find a way to remove an already added CA cert from the array though.

Hi. Have you found a method to remove a single CA cert ? Thanks

@suparpat

This comment has been minimized.

Copy link

commented Dec 10, 2018

As I'm using pfx, I modified @nolimitdev 's solution to:

fs.watch('/path/to/pfx', () => {
    clearTimeout(timeout);
    timeout = setTimeout(() => {
        options.server._sharedCreds.context.loadPKCS12(toBuf(fs.readFileSync('/path/to/pfx')))
    }, 1000);
});

function toBuf(str, encoding) {
	if (typeof str === 'string') {
	  if (encoding === 'buffer' || !encoding)
		encoding = 'utf8';
	  return Buffer.from(str, encoding);
	}
	return str;
}

sam-github added a commit to sam-github/node that referenced this issue Apr 26, 2019

tls: support changing credentials dynamically
This commit adds a setSecureContext() method to TLS servers. In
order to maintain backwards compatibility, the method takes the
options needed to create a new SecureContext, rather than an
instance of SecureContext.

Fixes: nodejs#4464
Refs: nodejs#10349
Refs: nodejs/help#603
Refs: nodejs#15115
PR-URL: nodejs#23644
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

sam-github added a commit to sam-github/node that referenced this issue Apr 26, 2019

tls: support changing credentials dynamically
This commit adds a setSecureContext() method to TLS servers. In
order to maintain backwards compatibility, the method takes the
options needed to create a new SecureContext, rather than an
instance of SecureContext.

Fixes: nodejs#4464
Refs: nodejs#10349
Refs: nodejs/help#603
Refs: nodejs#15115
PR-URL: nodejs#23644
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@matth-c3

This comment has been minimized.

Copy link

commented Apr 27, 2019

Hello,
if you generate a certificate pkcs#12 to authenticate users (TLS), and then on server side use setSecureContext to switch certificates, even if you push the correct ca that was use to sign the user certificates, you are getting a ERR_CERT_AUTHORITY_INVALID, is it expected ?

I also try with _sharedCreds.context.setCert and _sharedCreds.context.setKey, to just update the certificate, same result

use case : automatic rotation of the certificates on random interval on the servers.

Thanks by advance.

sam-github added a commit to sam-github/node that referenced this issue Apr 29, 2019

tls: support changing credentials dynamically
This commit adds a setSecureContext() method to TLS servers. In
order to maintain backwards compatibility, the method takes the
options needed to create a new SecureContext, rather than an
instance of SecureContext.

Fixes: nodejs#4464
Refs: nodejs#10349
Refs: nodejs/help#603
Refs: nodejs#15115
PR-URL: nodejs#23644
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.