Skip to content

Should we add a new api, "tls.Server.prototype.removeContexts(servername)" and "tls.Server.prototype.replaceContext(servername,context)" ? #34451

Closed
Closed
Should we add a new api, "tls.Server.prototype.removeContexts(servername)" and "tls.Server.prototype.replaceContext(servername,context)" ?#34451
Labels
feature requestIssues requesting new Node.js features.staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.tlsIssues and PRs related to the tls subsystem.

Description

@masx200
masx200
opened on Jul 20, 2020

Is your feature request related to a problem? Please describe.
Please describe the problem you are trying to solve.

#34110
#34444

After adding the context with "tls.Server.prototype.addContext", the context corresponding to the specified servername cannot be deleted. Should we add a new api, "tls.Server.prototype.removeContexts(servername)"?

This is useful on HTTPS servers that need to replace ssl/tls certificates frequently, such as using "let's encrypt". When the certificate needs to be replaced, you don't want to restart the HTTPS server, you just need to replace the certificate and key.

If multiple secure contexts are added to the same domain name
, the last one added should take effect,

With frequent ssl/tls certificate updates, addContext is called constantly. If the old context is not deleted, the old context will take up more and more memory space, and they are useless. Eventually lead to memory leaks or even memory overflow.

Describe the solution you'd like
Please describe the desired behavior.

add a new api, "tls.Server.prototype.removeContexts(servername)"

the all contexts corresponding to the specified servername should be deleted.

Should we add a new api, "tls.Server.prototype.removeContexts(servername)" and "tls.Server.prototype.replaceContext(servername,context)" ?

"tls.Server.prototype.replaceContext(servername,context)" Its role should be the operation of "tls.Server.prototype.removeContexts(servername)" and "tls.Server.prototype.addContext(servername,context)"

Describe alternatives you've considered
Please describe alternative solutions or features you have considered.

 const server = https.createServer();
const hostname = 'foo.bar.com';
const keypath = 'key.pem';
const certpath = 'cert.pem';
function debounce(callback, timeout) {
    let timer;
    return function (...args) {
        timer && clearTimeout(timer);
        timer = setTimeout(callback, timeout, ...args);
    };
}
const reloadcertkey = debounce(function () {
    let key = fs.readFileSync(keypath);
    let cert = fs.readFileSync(certpath);
    let context = tls.createSecureContext({
        key,
        cert
        
    });
    server.removeContexts(hostname, );
    server.addContext(hostname, context);
}, 1000);
reloadcertkey();
fs.watch(keypath, reloadcertkey);
fs.watch(certpath, reloadcertkey);
 const server = https.createServer();
const hostname = 'foo.bar.com';
const keypath = 'key.pem';
const certpath = 'cert.pem';
function debounce(callback, timeout) {
    let timer;
    return function (...args) {
        timer && clearTimeout(timer);
        timer = setTimeout(callback, timeout, ...args);
    };
}
const reloadcertkey = debounce(function () {
    let key = fs.readFileSync(keypath);
    let cert = fs.readFileSync(certpath);
    let context = tls.createSecureContext({
        key,
        cert
        
    });
    
    server.replaceContext(hostname, context);
}, 1000);
reloadcertkey();
fs.watch(keypath, reloadcertkey);
fs.watch(certpath, reloadcertkey);

Activity

  1. bnoordhuis
    added
    feature requestIssues requesting new Node.js features.
    tlsIssues and PRs related to the tls subsystem.
    on Jul 21, 2020

  2. bnoordhuis commented on Jul 21, 2020

    @bnoordhuis
    bnoordhuis
    on Jul 21, 2020
    Member

    The problem is that it's really difficult to know when it's safe to remove a context.

    Removing it when it's still in use results in segfaults and even when it's gone from node's internal bookkeeping, all kinds of reference counting inside openssl can still keep it alive.

  3. masx200 commented on Jul 21, 2020

    @masx200
    masx200
    on Jul 21, 2020
    ContributorAuthor

    node/lib/_tls_wrap.js

    Line 1436 in 78cd108

    function SNICallback(servername, callback) {

    node/lib/_tls_wrap.js

    Line 1411 in 78cd108

    Server.prototype.addContext = function(servername, context) {

  4. masx200 commented on Jul 21, 2020

    @masx200
    masx200
    on Jul 21, 2020
    ContributorAuthor

    I think the operation of "removeContexts" will not affect existing connections, but will only affect new connections in the future.

  5. masx200 commented on Jul 21, 2020

    @masx200
    masx200
    on Jul 21, 2020
    ContributorAuthor

    "tls.Server.prototype.removeContexts(servername)" should be the reverse operation of "tls.Server.prototype.addContext(servername,context)"

  6. masx200
    changed the title [-] Should we add a new api, tls.Server.prototype.removeContexts(servername)?[/-] [+] Should we add a new api, "tls.Server.prototype.removeContexts(servername)" and "tls.Server.prototype.replaceContext(servername,context)" ?[/+] on Jul 21, 2020

  7. masx200 commented on Jul 21, 2020

    @masx200
    masx200
    on Jul 21, 2020
    ContributorAuthor

    "tls.Server.prototype.replaceContext(servername,context)" Its role should be the operation of "tls.Server.prototype.removeContexts(servername)" and "tls.Server.prototype.addContext(servername,context)"

  8. bnoordhuis commented on Jul 22, 2020

    @bnoordhuis
    bnoordhuis
    on Jul 22, 2020
    Member

    @masx200 I think you misunderstand my previous comment.

    server._contexts is an array of SecureContext objects. The array exists in JS land in order to look up the right context based on the servername property. Removing elements from that array is not the issue.

    The issue is that removing a SecureContext from the array doesn't mean it's no longer in use. There are a bunch of C++ resources tied to SecureContext objects, all with complex lifecycles.

    Establishing when it's safe to release those resources is hard but you have to fix that first before you can fix this:

    Eventually lead to memory leaks or even memory overflow.

  9. KapitanOczywisty commented on Jun 21, 2021

    @KapitanOczywisty
    KapitanOczywisty
    on Jun 21, 2021

    This is somewhat edge case, but seems like allowing server._contexts to grow infinitely, itself could be treated as memory leak. I've tested this a bit and seems like using own contexts array and removing old contexts keeps memory more or less the same, but with server.addContext memory is rising constantly.

    However, It's possible to clear server's certificates even now using: server._contexts.length = 0;, this is of course internal api, but good enough for testing. With that memory was rising slower but rising nonetheless.

    IMO There is no point to keep useless contexts in the server._contexts, but seems like they're kept around by something else - or my testing is flawed.

    If one is concerned about memory and constant certificate changes, should use SNICallback, but as mentioned this is edge case and isn't something to be concerned in 99.9% cases.

    Result of my quick testing: repeated 50 times: 10 certs loaded/replaced + 10 requests. After last series:

    // SNICallback w/ own array
rss: 37 257 216 (+8 773 632)
heapTotal: 9 986 048 (+3 416 064)
heapUsed: 8 805 720 (+3 653 072)
external: 1 499 930 (+535 949)
arrayBuffers: 509 062 (+491 452)

// addContext
rss: 48 406 528 (+19 922 944)
heapTotal: 10 248 192 (+3 678 208)
heapUsed: 8 983 832 (+3 850 184)
external: 1 483 498 (+519 517)
arrayBuffers: 492 630 (+475 020)

// addContext + clearing server._contexts
rss: 44 023 808 (+15 486 976)
heapTotal: 10 248 192 (+3 678 208)
heapUsed: 8 748 112 (+3 595 424)
external: 1 443 396 (+479 415)
arrayBuffers: 451 658 (+434 048)

  10. github-actions commented on Mar 29, 2022

    @github-actions
    github-actionsbot
    on Mar 29, 2022
    Contributor

    There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

    For more information on how the project manages feature requests, please consult the feature request management document.

  11. github-actions
    added
    staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.
    on Mar 29, 2022
  12. targos
    added this to Node.js feature requestson Mar 29, 2022
  13. targos
    moved this to Stale in Node.js feature requestson Mar 29, 2022

  14. github-actions commented on Apr 29, 2022

    @github-actions
    github-actionsbot
    on Apr 29, 2022
    Contributor

    There has been no activity on this feature request and it is being closed. If you feel closing this issue is not the right thing to do, please leave a comment.

    For more information on how the project manages feature requests, please consult the feature request management document.

  15. github-actions
    closed this as completedon Apr 29, 2022
  16. avivkeller
    removed this from Node.js feature requestson Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    feature requestIssues requesting new Node.js features.staleIssues and PRs marked stale due to inactivity and scheduled for automatic closure.tlsIssues and PRs related to the tls subsystem.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions