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

Live renewal of the TLS options #331

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bartbutenaers
Copy link
Contributor

@bartbutenaers bartbutenaers commented Jun 26, 2022

Dear,

When working with FTPS, a keypair (private key and certificate) are being passed to the server constructor. Which means you need to restart the server every time the certificate is being renewed.

I use LetsEncrypt certificates for my home automation, which will expire every 3 months. Which means I have to renew the certificate every 3 months, and as a result I need to restart my FTP server every 3 months. Restarting my FTP server only to replace a certificate is not what I want. Because if one of my IP cams is uploading a video recording at that moment, that recording would be lost.

In NodeJs version 11, the setSecureContext function has been added to solve this. By calling this function you can instruct the server to use updated TLS options.

A bit of explanation:

  1. I start my FTP server with the old keypair via the TLS options:

    let options = {};
    options.tls = {
       url: "ftps://my_host_name:7021",
       key: fs.readFileSync("\path\to\old\key.pem"),
       cert: fs.readFileSync("\path\to\old\cert.pem")
    }
    let myFtpServer = new FtpServer(options);
    
  2. Now I can easily determine which certificate is being used by the FTP server (during the TLS handshake phase), by using this test code to connect to my FTP server:

    var testOptions = {
       host: "my_host_name",
       port: 7021,
       // Skip certificate validation, because we only want to do the handshake without failure on an invalid certificate
       checkServerIdentity: () => undefined,
       rejectUnauthorized: false
    }
    var tlsSocket = tls.connect(options, function () {
       let certificate = tlsSocket.getPeerCertificate();
       let validToDate = certificate.valid_to;
       ...
    })
    

    Which shows that my old (expired) certificate is being used:

    image

  3. Now I tell the server to start using my new certificate (using the new function from this pull request):

    let newTlsOptions = {
       key: fs.readFileSync("\path\to\new\key.pem"),
       cert: fs.readFileSync("\path\to\new\cert.pem")
    }
    myFtpServer.renewTlsOptions(newTlsOptions);
    
  4. When I repeat the test from step 2 again, now indeed I see that my new certificate is used by the server already:

    image

Note that this new certificate will only be used for new connections. For existing connections the old certificate will still being used, since the handshake phase is already finished. So there is no impact on existing connections!

Don't hesitate to ask for extra information or updates!

Bart

@bartbutenaers bartbutenaers requested a review from a team as a code owner June 26, 2022 21:46
@bartbutenaers
Copy link
Contributor Author

BTW I have now also added a section to the readme page:

image

@matt-forster
Copy link
Contributor

matt-forster commented Jun 27, 2022

I like it, and you didn't make any assumptions about how the user of the library should call it - awesome.

I don't see the readme update, does it need to be pushed to this branch still?

@bartbutenaers
Copy link
Contributor Author

@matt-forster,
Glad to hear that you like this proposal!
Seems I forgot this morning to push my changes, before I left for my daily job.
It should be visible now...

README.md Outdated
@@ -403,6 +403,23 @@ __Used in:__ `SITE CHMOD`
Returns a unique file name to write to. Client requested filename available if you want to base your function on it.
__Used in:__ `STOU`

#### [`renewTlsOptions(tlsOptions)`](src/fs.js#L172)
Copy link
Contributor

@matt-forster matt-forster Jun 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an instance interface - so not quite the same as the above functions, which are filesystem specific.

I would actually insert a new section here, under API and above CLI;

<!-- Line 163 -->
## [`#renewTlsOptions(tlsOptions)`](src/fs.js#L172)

Used to read and use a new set of TLS certificates without restarting the server.
Receives the same options as the [tls parameter](#tls) parameter in the constructor.
...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matt-forster
That makes sense. I have moved it now to a separate section, and made two other changes:

  1. The section header is now no link (to the source) anymore, because all other header titles were also no links. And otherwise it looked very weird...
  2. The link pointed to the wrong source file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants