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

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

Closed
wants to merge 7 commits into from

Conversation

masx200
Copy link
Contributor

@masx200 masx200 commented Jul 20, 2020

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
Copy link
Member

@addaleax addaleax left a comment

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
Member

/cc @mkrawczuk

@masx200
Copy link
Contributor Author

masx200 commented Jul 20, 2020

Fixes: #34110

@masx200
Copy link
Contributor Author

masx200 commented Jul 20, 2020

servername should also be case-insensitive.

lib/_tls_wrap.js Outdated Show resolved Hide resolved
Co-authored-by: James M Snell <jasnell@gmail.com>
lib/_tls_wrap.js Outdated Show resolved Hide resolved
Co-authored-by: James M Snell <jasnell@gmail.com>
lib/_tls_wrap.js Outdated Show resolved Hide resolved
Co-authored-by: James M Snell <jasnell@gmail.com>
@lpinca
Copy link
Member

lpinca commented Jul 20, 2020

Can you add a test?

@masx200
Copy link
Contributor Author

masx200 commented Jul 20, 2020

Can you add a test?

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

@masx200
Copy link
Contributor Author

masx200 commented Jul 21, 2020

#34451

@rickyes
Copy link
Contributor

rickyes commented Jul 22, 2020

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
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
Copy link
Contributor Author

masx200 commented Aug 5, 2020

@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
Copy link
Contributor Author

masx200 commented Aug 5, 2020

@mkrawczuk

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

@aduh95
Copy link
Contributor

aduh95 commented Nov 8, 2020

@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
Copy link
Contributor

github-actions bot commented Nov 8, 2020

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
Copy link
Contributor Author

masx200 commented Nov 8, 2020

#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