Skip to content

tls default SNICallback should Check the servername and select the appropriate secure context in Reverse order #34444

Closed
masx200 wants to merge 7 commits into
nodejs:masterfrom
masx200:patch-2
Closed

tls default SNICallback should Check the servername and select the appropriate secure context in Reverse order #34444
masx200 wants to merge 7 commits into
nodejs:masterfrom
masx200:patch-2

Conversation

@masx200

@masx200 masx200 commented Jul 20, 2020

Copy link
Copy Markdown
Contributor
Checklist
  • [x ] make -j4 test (UNIX), or vcbuild test (Windows) passes
  • [ x] tests and/or benchmarks are included
  • [ x] documentation is changed or added
  • [ x] commit message follows commit guidelines

tls default SNICallback should Check the servername and select the appropriate secure context in Reverse order

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

 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.addContext(hostname, context);
}, 1000);
reloadcertkey();
fs.watch(keypath, reloadcertkey);
fs.watch(certpath, reloadcertkey);

@nodejs-github-bot nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Jul 20, 2020

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This fixes #34110, right? Can you add a Fixes: https://github.com/nodejs/node/issues/34110 line to the the commit message, and run make lint-js-fix to address the linter errors here?

@addaleax

Copy link
Copy Markdown
Member

/cc @mkrawczuk

@masx200

masx200 commented Jul 20, 2020

Copy link
Copy Markdown
Contributor Author

Fixes: #34110

@masx200

masx200 commented Jul 20, 2020

Copy link
Copy Markdown
Contributor Author

servername should also be case-insensitive.

Comment thread lib/_tls_wrap.js Outdated
Co-authored-by: James M Snell <jasnell@gmail.com>
Comment thread lib/_tls_wrap.js Outdated
Co-authored-by: James M Snell <jasnell@gmail.com>
Comment thread lib/_tls_wrap.js Outdated
Co-authored-by: James M Snell <jasnell@gmail.com>
@lpinca

lpinca commented Jul 20, 2020

Copy link
Copy Markdown
Member

Can you add a test?

@masx200

masx200 commented Jul 20, 2020

Copy link
Copy Markdown
Contributor Author

Can you add a test?

I don’t know much about writing tests. Can you ask someone to write tests?

@masx200

masx200 commented Jul 21, 2020

Copy link
Copy Markdown
Contributor Author

#34451

@rickyes

rickyes commented Jul 22, 2020

Copy link
Copy Markdown
Contributor

Can you add a test?

I don’t know much about writing tests. Can you ask someone to write tests?

@masx200 The test cases and features/fixes are usually in the same PR, you can try to write them by referring to an existing test case, the test folder is test/parallel, you can find the corresponding module according to the file name, take your time.

@mkrawczuk

Copy link
Copy Markdown
Contributor

I have reported a relevant issue some time ago (#34110). I've been working on a PR but got dragged away by other projects. A proper solution, with tests and documentation, can be found in PR #34638

@masx200

masx200 commented Aug 5, 2020

Copy link
Copy Markdown
Contributor Author

@mkrawczuk we should not use Array.prototype.reverse(),This method will change the original array, which produces unnecessary side effects and will cause a great performance loss.

@masx200

masx200 commented Aug 5, 2020

Copy link
Copy Markdown
Contributor Author

@mkrawczuk

Since we are solving the same problem, could you help me write a test for this pull request?

@aduh95

aduh95 commented Nov 8, 2020

Copy link
Copy Markdown
Contributor

@masx200 There are linter errors that need fixing.

@aduh95 aduh95 added the stalled Issues and PRs that are stalled. label Nov 8, 2020
@github-actions

github-actions Bot commented Nov 8, 2020

Copy link
Copy Markdown
Contributor

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

@masx200

masx200 commented Nov 8, 2020

Copy link
Copy Markdown
Contributor Author

#34638

There is already the same pr to solve this problem

@masx200 masx200 closed this Nov 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stalled Issues and PRs that are stalled. tls Issues and PRs related to the tls subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants