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

deps: update openssl config #5630

Closed
wants to merge 1 commit into from
Closed

deps: update openssl config #5630

wants to merge 1 commit into from

Conversation

shigeki
Copy link
Contributor

@shigeki shigeki commented Mar 9, 2016

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to [CONTRIBUTING.md][0]?
  • [CI tests] If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Affected core subsystem(s)

deps: openssl

Description of change

deps/openssl/config were not updated in the last upgrading. Two new defines of OPENSSL_NO_SSL2 and OPENSSL_NO_WEAK_SSL_CIPHERS are defined in opensslconf.h.

This disables several ciphers included in EXPORT and LOW. They are also disabled in the default cipher list but I think that applying this to LTS should be discussed in LTS issue. I will open it.

CI is running on https://ci.nodejs.org/job/node-test-commit/2502/ but something Jenkins error was on MacOS. I will try it again.

R: @bnoordhuis

deps: update openssl config
ca8adf0 
OPENSSL_NO_SSL2 and OPENSSL_NO_WEAK_SSL_CIPHERS are defined in
opensslconf.h
@shigeki
Copy link
Contributor Author

shigeki commented Mar 9, 2016

New CI is https://ci.nodejs.org/job/node-test-pull-request/1890/

@Fishrock123
Copy link
Contributor

s/udpate/update/ :)

@Fishrock123 Fishrock123 added the openssl Issues and PRs related to the OpenSSL dependency. label Mar 9, 2016
@shigeki shigeki mentioned this pull request Mar 9, 2016
Closed
@shigeki shigeki changed the title deps: udpate openssl config deps: update openssl config Mar 9, 2016
@shigeki
Copy link
Contributor Author

shigeki commented Mar 9, 2016

@Fishrock123 Oops. Fixed typo. Thanks.

@shigeki
Copy link
Contributor Author

shigeki commented Mar 10, 2016

CI in https://ci.nodejs.org/job/node-test-commit/2505/ is all green.

@shigeki
Copy link
Contributor Author

shigeki commented Mar 10, 2016

Strictly speaking, this fix corresponds to semver-major. As discussed in LTS on nodejs/Release#85, I would like to apply this to 5.x.

@jasnell
Copy link
Member

jasnell commented Mar 11, 2016

LGTM for master... Let's add it to the lts-agenda for Monday to discuss.

@shigeki
Copy link
Contributor Author

shigeki commented Mar 15, 2016

@indutny Could you review this PR?

@indutny
Copy link
Member

indutny commented Mar 15, 2016

LGTM, if CI is green

@shigeki
Copy link
Contributor Author

shigeki commented Mar 15, 2016

@indutny Thanks. CI was submitted in https://ci.nodejs.org/job/node-test-pull-request/1927/.

shigeki pushed a commit that referenced this pull request Mar 15, 2016
deps: update openssl config
668fb17 
OPENSSL_NO_SSL2 and OPENSSL_NO_WEAK_SSL_CIPHERS are defined in
opensslconf.h

Fixes: nodejs/Release#85
PR-URL: #5630
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@shigeki
Copy link
Contributor Author

shigeki commented Mar 15, 2016

CI is all green. Landed in 668fb17. Thanks.

@shigeki shigeki closed this Mar 15, 2016
@jasnell
Copy link
Member

jasnell commented Mar 15, 2016

@shigeki ... This was backported to v0.12 and v0.10 in #5712 correct?

Also, removed the lts-agenda label on this since it was decided via the github conversation to go ahead and land this in the lts branches.

@shigeki
Copy link
Contributor Author

shigeki commented Mar 15, 2016

@jasnell #5712 was the fix for openssl-1.0.1 and it has already landed to v0.12-staging and v0.10-staging branches. This PR is the fix for openssl-1.0.2 and it need to be backported from master to 5.x and 4.x. I would like you to take care of them.

@jasnell
Copy link
Member

jasnell commented Mar 15, 2016

Sounds good! :-)

evanlucas pushed a commit that referenced this pull request Mar 15, 2016
@evanlucas
deps: update openssl config
2e000d1 
OPENSSL_NO_SSL2 and OPENSSL_NO_WEAK_SSL_CIPHERS are defined in
opensslconf.h

Fixes: nodejs/Release#85
PR-URL: #5630
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@evanlucas evanlucas mentioned this pull request Mar 15, 2016
Merged
rvagg pushed a commit that referenced this pull request Mar 16, 2016
@rvagg
deps: update openssl config
29510aa 
OPENSSL_NO_SSL2 and OPENSSL_NO_WEAK_SSL_CIPHERS are defined in
opensslconf.h

Fixes: nodejs/Release#85
PR-URL: #5630
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@MylesBorins
Copy link
Contributor

@shigeki is this pressing enough to roll into the next v4 release asap or should it have a bit of time to live on v5 first?

@shigeki
Copy link
Contributor Author

shigeki commented Mar 18, 2016

@thealphanerd This fix is a kind of regular procedures in upgrading openssl and has just a small risk. So it's already enough to roll to v4.x.

MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
deps: update openssl config
d8a5a31 
OPENSSL_NO_SSL2 and OPENSSL_NO_WEAK_SSL_CIPHERS are defined in
opensslconf.h

Fixes: nodejs/Release#85
PR-URL: #5630
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
deps: update openssl config
19ae308 
OPENSSL_NO_SSL2 and OPENSSL_NO_WEAK_SSL_CIPHERS are defined in
opensslconf.h

Fixes: nodejs/Release#85
PR-URL: #5630
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
openssl Issues and PRs related to the OpenSSL dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@shigeki @Fishrock123 @jasnell @indutny @MylesBorins