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.

Sign up for GitHub

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

Jump to bottom

src,build: add --openssl-default-cipher-list #33708

Closed
wants to merge 4 commits into from
Closed

src,build: add --openssl-default-cipher-list #33708

wants to merge 4 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Jun 3, 2020

This commit adds a configuration option named
openssl-default-cipher-list which takes a colon separated string
specifying ciphers that should be used as the default ciphers instead of
the ones defined in node_constants.

The motivation for this is a use case where Fedora/RHEL would like
to be able to specify a default cipher in the format PROFILE=SYSTEM.
This would enable Fedora/RHEL to have a system wide security level for
all applications.

Refs: https://docs.fedoraproject.org/en-US/packaging-guidelines/CryptoPolicies/

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@danbev
src,build: add --openssl-default-cipher-list
a41cc6f 
This commit adds a configuration option named
openssl-default-cipher-list which takes a colon separated string
specifying ciphers that should be used as the default ciphers instead of
the ones defined in node_constants.

The motivation for this is a use case where Fedora/RHEL would like
to be able to specify a default cipher in the format PROFILE=SYSTEM.
This would enable Fedora/RHEL to have a system wide security level for
all applications.

Refs: https://docs.fedoraproject.org/en-US/packaging-guidelines/CryptoPolicies/
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. labels Jun 3, 2020
@mscdex
Copy link
Contributor

mscdex commented Jun 3, 2020 •
edited
Loading

Perhaps it would be a good idea to add a note to the tls documentation to let users know that the default TLS cipher list may not match the hard-coded list shown in the documentation and that they may need to inspect crypto.constants.defaultCoreCipherList and/or crypto.constants.defaultCipherList to determine what ciphers will be used.

I'm kind of surprised we don't already mention these crypto.constants properties in the appropriate section of the tls documentation.

@jasnell
Copy link
Member

jasnell commented Jun 3, 2020

How does this relate to the existing --tls-cipher-list= command-line argument?

@danbev
Copy link
Contributor Author

danbev commented Jun 4, 2020

Perhaps it would be a good idea to add a note to the tls documentation

Good point, I'll add something in that document. Thanks

@danbev
Copy link
Contributor Author

danbev commented Jun 4, 2020

How does this relate to the existing --tls-cipher-list= command-line argument?

Using the command-line option or the NODE_OPTIONS environment variable allows the cipher list to be overridden at runtime. The would be something the a user actively does, whereas being able to set the default cipher list at build time will allow distributions to have their own default list and the users can opt out of if they need to.

danbev added 2 commits June 4, 2020 06:00
@danbev
squash: add openssl-default-cipher-list to without_ssl check
ecc0787 
This commit adds this option to the without_ssl so that when used with
the --without-ssl option the following configuration error will be
displayed:
Node.js configure: Found Python 3.8.2...
ERROR: --without-ssl is incompatible with --openssl-default-cipher-list
@danbev
squash: add note about displaying the def cipher suite
66815e9
bnoordhuis
bnoordhuis approved these changes Jun 4, 2020
View reviewed changes
doc/api/tls.md Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/31731/

This was referenced Jun 6, 2020
Open
Open
Open
@danbev
Copy link
Contributor Author

danbev commented Jun 8, 2020 •
edited
Loading

Re-run of failing node-test-commit-linux-containered/ ✔️
Re-run of failing node-test-commit-osx ✔️

danbev added a commit that referenced this pull request Jun 8, 2020
@danbev
src,build: add --openssl-default-cipher-list
7f8e977 
This commit adds a configuration option named
openssl-default-cipher-list which takes a colon separated string
specifying ciphers that should be used as the default ciphers instead of
the ones defined in node_constants.

The motivation for this is a use case where Fedora/RHEL would like
to be able to specify a default cipher in the format PROFILE=SYSTEM.
This would enable Fedora/RHEL to have a system wide security level for
all applications.

PR-URL: #33708
Refs: https://docs.fedoraproject.org/en-US/packaging-guidelines/CryptoPolicies/
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@danbev
Copy link
Contributor Author

danbev commented Jun 8, 2020

Landed in 7f8e977.

@danbev danbev closed this Jun 8, 2020
@danbev danbev deleted the config-default-cipherlist branch June 8, 2020 11:29
This was referenced Jun 9, 2020
Open
Open
codebytere pushed a commit that referenced this pull request Jun 18, 2020
@danbev @codebytere
src,build: add --openssl-default-cipher-list
26a3cf0 
This commit adds a configuration option named
openssl-default-cipher-list which takes a colon separated string
specifying ciphers that should be used as the default ciphers instead of
the ones defined in node_constants.

The motivation for this is a use case where Fedora/RHEL would like
to be able to specify a default cipher in the format PROFILE=SYSTEM.
This would enable Fedora/RHEL to have a system wide security level for
all applications.

PR-URL: #33708
Refs: https://docs.fedoraproject.org/en-US/packaging-guidelines/CryptoPolicies/
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@codebytere codebytere mentioned this pull request Jun 28, 2020
Merged
codebytere pushed a commit that referenced this pull request Jun 30, 2020
@danbev @codebytere
src,build: add --openssl-default-cipher-list
b1f6584 
This commit adds a configuration option named
openssl-default-cipher-list which takes a colon separated string
specifying ciphers that should be used as the default ciphers instead of
the ones defined in node_constants.

The motivation for this is a use case where Fedora/RHEL would like
to be able to specify a default cipher in the format PROFILE=SYSTEM.
This would enable Fedora/RHEL to have a system wide security level for
all applications.

PR-URL: #33708
Refs: https://docs.fedoraproject.org/en-US/packaging-guidelines/CryptoPolicies/
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
codebytere pushed a commit that referenced this pull request Jul 10, 2020
@danbev @codebytere
src,build: add --openssl-default-cipher-list
abf04b2 
This commit adds a configuration option named
openssl-default-cipher-list which takes a colon separated string
specifying ciphers that should be used as the default ciphers instead of
the ones defined in node_constants.

The motivation for this is a use case where Fedora/RHEL would like
to be able to specify a default cipher in the format PROFILE=SYSTEM.
This would enable Fedora/RHEL to have a system wide security level for
all applications.

PR-URL: #33708
Refs: https://docs.fedoraproject.org/en-US/packaging-guidelines/CryptoPolicies/
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@codebytere codebytere mentioned this pull request Jul 13, 2020
Merged
codebytere pushed a commit that referenced this pull request Jul 14, 2020
@danbev @codebytere
src,build: add --openssl-default-cipher-list
cad115b 
This commit adds a configuration option named
openssl-default-cipher-list which takes a colon separated string
specifying ciphers that should be used as the default ciphers instead of
the ones defined in node_constants.

The motivation for this is a use case where Fedora/RHEL would like
to be able to specify a default cipher in the format PROFILE=SYSTEM.
This would enable Fedora/RHEL to have a system wide security level for
all applications.

PR-URL: #33708
Refs: https://docs.fedoraproject.org/en-US/packaging-guidelines/CryptoPolicies/
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@AdamMajer
Copy link
Contributor

Just a question about setting the default ciphers to PROFILE=SYSTEM. Is node suppose to resolve these ciphers via openssl for crypto.constants.defaultCipherList and crypto.constants.defaultCoreCipherList or will magic constants be retained as PROFILE=SYSTEM and then passed to OpenSSL at a later stage?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau approved these changes

@devnexen devnexen devnexen approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@danbev @mscdex @jasnell @nodejs-github-bot @AdamMajer @bnoordhuis @devnexen @richardlau