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

Upgrade to OpenSSL-1.0.2 #589

Closed
indutny opened this issue Jan 24, 2015 · 55 comments
Closed

Upgrade to OpenSSL-1.0.2 #589

indutny opened this issue Jan 24, 2015 · 55 comments
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. tls Issues and PRs related to the tls subsystem.

Comments

@indutny
Copy link
Member

indutny commented Jan 24, 2015

See https://www.openssl.org/docs/ssl/SSL_CTX_set_cert_cb.html

Basically, we will be able to eliminate our hello parser and use new APIs for async OCSP/SNI and stuff like that.

cc @bnoordhuis

@jbergstroem
Copy link
Member

I started porting yesterday. The import is pretty simple but since I can't find any traces of google incorporating 1.0.2, the gyp-file needs work. I haven't even started looking at asm/yet..

@indutny
Copy link
Member Author

indutny commented Jan 25, 2015

@jbergstroem cool! Just FYI, this .gyp file is barely related to the one that was done by Google team. The comment above it is mostly a legacy thing, we rewrote it a couple of times.

@jbergstroem
Copy link
Member

See #723 for work in progress by @shigeki.

@shigeki
Copy link
Contributor

shigeki commented Feb 6, 2015

I missed this issue. Yes, I'm working on it. The current branch of openssl-1.0.2 supports only Linux and MacOS and I will do work on Windows next, but I don't have an arm environment for development and tests. I hope someone helps it.

@jbergstroem
Copy link
Member

@shigeki just wanted people visiting that issue to check your work out. I'm doing some testing on FreeBSD at the moment.

@indutny
Copy link
Member Author

indutny commented Feb 9, 2015

@shigeki need any help on this? I'd like to ship it as soon as possible ;)

@indutny
Copy link
Member Author

indutny commented Feb 9, 2015

@shigeki
Copy link
Contributor

shigeki commented Feb 10, 2015

@indutny asm was done for x64 in linux and macos. I will work windows today but I don't have arm and others. ALPN has already been done based on Node-0.11 as in shigeki@5d6c147 .

@chrisdickinson @rvagg How can I use jenkins-iojs.nodesource.com for build testing?

@rvagg
Copy link
Member

rvagg commented Feb 10, 2015

@shigeki I'll try and get around to adding you today to CI, I have a list of people I need to add

@indutny
Copy link
Member Author

indutny commented Feb 10, 2015

@shigeki there is no need for ALPN patch, this feature is in OpenSSL-1.0.2 anyway.

@shigeki
Copy link
Contributor

shigeki commented Feb 10, 2015

@indutny This patch uses ALPN API in OpenSSL-1.0.2 and add options.ALPNProtocols. Openssl-1.0.2 was applied to Node-v0.11 by git submodule without using gyp. I think this can be applied to io.js after upgrading openssl-1.0.2

@indutny
Copy link
Member Author

indutny commented Feb 10, 2015

@shigeki aaah, I see now. You was talking about node.js support, not about patching OpenSSL itself. Good job, man. Let's move it forward! ;)

@shigeki
Copy link
Contributor

shigeki commented Feb 17, 2015

@indutny Upgrading to openssl-1.0.2 is almost done in https://github.com/shigeki/io.js/tree/WIP_upgrade_openssl102 except testing on arm. But there is a build issue of x86-win32-masm asm with using ml.exe as reported in http://rt.openssl.org/Ticket/Display.html?id=3650&user=guest&pass=guest . A patch to fix this was shown but not merged yet. And I also found in the issue tracker that openssl team strongly recommended to use nasm. There is no error when we use nasm.

Upgrading of openssl cannot be finished without solving this issue, so now we have 4 options to go as

  1. Wait for the upstream to be fixed and released.
  2. The patch is applied to iojs for a tentative fix and use masm(ml.exe).
  3. Use no-asm for x86-win32.
  4. Move to use [EDIT nasm] as strongly recommended by openssl team .

I prefer 2 or 3 for now. I've confirmed both is working well and the above my WIP branch was worked on 2. Then, we need to discuss if we move to use nasm in the future. Do you have any thoughts?

@indutny
Copy link
Member Author

indutny commented Feb 17, 2015

I'm -1 for 3 and +1 for 2.

@shigeki
Copy link
Contributor

shigeki commented Feb 18, 2015

@indutny Thanks. I continue to work on 2. I'd like to have a directory to store this kind of private patches for a third party library somewhere in iojs repository so as not to miss it in the future. I've spent some time to fix TLS tests on Windows as your patch of ab71223 was missed to be applied.

@rvagg I'm waiting for my account for testing on arm. Did you already sent it to me?

@indutny
Copy link
Member Author

indutny commented Feb 18, 2015

@shigeki I usually just use git log deps/openssl to reapply all patches.

@shigeki
Copy link
Contributor

shigeki commented Feb 18, 2015

@indutny Yes, that's the way we need now. I just think of some kinds of http://src.chromium.org/chrome/trunk/deps/third_party/openssl/patches.chromium/ but patches should be already applied as it is not a good idea to apply patches during build.

@indutny
Copy link
Member Author

indutny commented Feb 19, 2015

@shigeki I'm -1 for this, don't see any point in duplicating information that is already available in git tree.

@indutny
Copy link
Member Author

indutny commented Feb 19, 2015

@shigeki I wonder if we could actually use nasm on windows. @bnoordhuis what are your thoughts on this?

@indutny
Copy link
Member Author

indutny commented Feb 19, 2015

@shigeki may I ask you to borrow the assembly stuff from indutny/bud@be4af45 if it works for you? (I'm in process of testing builds on various platforms)

@indutny
Copy link
Member Author

indutny commented Feb 19, 2015

UPDATE: It works on ia32/x86_64 OSX. Testing it on ubuntu.

@indutny
Copy link
Member Author

indutny commented Feb 19, 2015

UPDATE: Fixed ubuntu-ia32: indutny/bud@29c5e01

@bnoordhuis
Copy link
Member

I wonder if we could actually use nasm on windows.

If by 'use' you mean 'gyp-ify, bundle and build', I guess that could work. It's pretty easy to build and it's license compatible (BSD.)

@indutny
Copy link
Member Author

indutny commented Feb 19, 2015

Yes, exactly.

UPDATE: ia32/x86_64 Linux works.

@indutny
Copy link
Member Author

indutny commented Feb 19, 2015

UPDATE: indutny/bud@29c5e01

@indutny
Copy link
Member Author

indutny commented Feb 19, 2015

@shigeki
Copy link
Contributor

shigeki commented Feb 20, 2015

@indutny I've changed asm/Makefile largely in 019616bc9d609d429f2caf7441a97fe50e2f1463 so as to generate exact same asm files as those generated by Configure of openssl in various platforms. there are no diffs as far as I've checked. I summarized my changes in Makefile into 5 point as written in the commit log below.

    deps: update asm Makefile for openssl-1.0.2

    This includes following changes,
     - Updated asm files for each platforms which are required in
     openssl-1.0.2.
     - Some perl files need CC and ASM envs. Added a check if these envs
     exist. Followed asm files are to be generated with CC=gcc and
     ASM=nasm on Linux.
     - Added new 32bit targets/rules with a sse2 flag (OPENSSL_IA32_SSE2)
     to generate asm to use SSE2.
     - Generating sha512 asm files in x86_64 need output filename which
     has 512. Added a new rule and target directory of ~~asm/sha256~~ [Edit: fixed to] asm/sha512 to store
     them.
     - PERLASM_SCHEME of linux-armv4 is `void` as defined in openssl
     Configure. Changed its target/rule and all directories are moved from
     arm-elf-gas to arm-void-gas.

asm/sha512/ directory needs from the 4th point in above as https://github.com/shigeki/io.js/blob/upgrade_openssl102/deps/openssl/openssl/crypto/sha/asm/sha512-x86_64.pl#L137 . The new rule generates new asm files of sha512 by without using stdout but its filename args.

I got a Rasp Pi2 last night and can work asm target today. I can finish it soon so let's finish #723 before upgrading.

rvagg pushed a commit that referenced this issue Nov 23, 2018
`x86masm.pl` was mistakenly using .486 instruction set, why `cpuid` (and
perhaps others) are requiring .686 .

Fixes: #589
PR-URL: #1389
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
rvagg pushed a commit that referenced this issue Nov 23, 2018
See
https://mta.openssl.org/pipermail/openssl-dev/2015-February/000651.html

iojs needs to stop using masm and move to nasm or yasm on Win32.

Fixes: #589
PR-URL: #1389
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
rvagg pushed a commit that referenced this issue Nov 23, 2018
Reapply b910613 .

Fixes: #589
PR-URL: #1389
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
rvagg pushed a commit that referenced this issue Nov 23, 2018
`x86masm.pl` was mistakenly using .486 instruction set, why `cpuid` (and
perhaps others) are requiring .686 .

Fixes: #589
PR-URL: #1389
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
rvagg pushed a commit that referenced this issue Nov 23, 2018
See
https://mta.openssl.org/pipermail/openssl-dev/2015-February/000651.html

iojs needs to stop using masm and move to nasm or yasm on Win32.

Fixes: #589
PR-URL: #1389
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
rvagg pushed a commit that referenced this issue Nov 23, 2018
Reapply b910613 .

Fixes: #589
PR-URL: #1389
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
rvagg pushed a commit that referenced this issue Nov 24, 2018
`x86masm.pl` was mistakenly using .486 instruction set, why `cpuid` (and
perhaps others) are requiring .686 .

Fixes: #589
PR-URL: #1389
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
rvagg pushed a commit that referenced this issue Nov 24, 2018
See
https://mta.openssl.org/pipermail/openssl-dev/2015-February/000651.html

iojs needs to stop using masm and move to nasm or yasm on Win32.

Fixes: #589
PR-URL: #1389
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
rvagg pushed a commit that referenced this issue Nov 24, 2018
Reapply b910613 .

Fixes: #589
PR-URL: #1389
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
shigeki pushed a commit to shigeki/node that referenced this issue Feb 26, 2019
`x86masm.pl` was mistakenly using .486 instruction set, why `cpuid` (and
perhaps others) are requiring .686 .

Fixes: nodejs#589
PR-URL: nodejs#1389
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
shigeki pushed a commit to shigeki/node that referenced this issue Feb 26, 2019
See
https://mta.openssl.org/pipermail/openssl-dev/2015-February/000651.html

iojs needs to stop using masm and move to nasm or yasm on Win32.

Fixes: nodejs#589
PR-URL: nodejs#1389
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
shigeki pushed a commit to shigeki/node that referenced this issue Feb 26, 2019
Reapply b910613 .

Fixes: nodejs#589
PR-URL: nodejs#1389
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
sam-github pushed a commit to sam-github/node that referenced this issue Sep 5, 2019
`x86masm.pl` was mistakenly using .486 instruction set, why `cpuid` (and
perhaps others) are requiring .686 .

Fixes: nodejs#589
PR-URL: nodejs#1389
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
sam-github pushed a commit to sam-github/node that referenced this issue Sep 5, 2019
See
https://mta.openssl.org/pipermail/openssl-dev/2015-February/000651.html

iojs needs to stop using masm and move to nasm or yasm on Win32.

Fixes: nodejs#589
PR-URL: nodejs#1389
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
sam-github pushed a commit to sam-github/node that referenced this issue Sep 5, 2019
Reapply b910613 .

Fixes: nodejs#589
PR-URL: nodejs#1389
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
BethGriggs pushed a commit that referenced this issue Sep 19, 2019
`x86masm.pl` was mistakenly using .486 instruction set, why `cpuid` (and
perhaps others) are requiring .686 .

Fixes: #589
Backport-PR-URL: #28230
PR-URL: #1389
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
BethGriggs pushed a commit that referenced this issue Sep 19, 2019
See
https://mta.openssl.org/pipermail/openssl-dev/2015-February/000651.html

iojs needs to stop using masm and move to nasm or yasm on Win32.

Fixes: #589
Backport-PR-URL: #28230
PR-URL: #1389
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
BethGriggs pushed a commit that referenced this issue Sep 19, 2019
Reapply b910613 .

Fixes: #589
Backport-PR-URL: #28230
PR-URL: #1389
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this issue May 5, 2024
`x86masm.pl` was mistakenly using .486 instruction set, why `cpuid` (and
perhaps others) are requiring .686 .

Fixes: nodejs/node#589
PR-URL: nodejs/node#1389
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this issue May 5, 2024
See
https://mta.openssl.org/pipermail/openssl-dev/2015-February/000651.html

iojs needs to stop using masm and move to nasm or yasm on Win32.

Fixes: nodejs/node#589
PR-URL: nodejs/node#1389
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this issue May 5, 2024
Reapply b910613 .

Fixes: nodejs/node#589
PR-URL: nodejs/node#1389
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this issue May 5, 2024
`x86masm.pl` was mistakenly using .486 instruction set, why `cpuid` (and
perhaps others) are requiring .686 .

Fixes: nodejs/node#589
PR-URL: nodejs/node#1389
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this issue May 5, 2024
See
https://mta.openssl.org/pipermail/openssl-dev/2015-February/000651.html

iojs needs to stop using masm and move to nasm or yasm on Win32.

Fixes: nodejs/node#589
PR-URL: nodejs/node#1389
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this issue May 5, 2024
Reapply b910613 .

Fixes: nodejs/node#589
PR-URL: nodejs/node#1389
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

No branches or pull requests

7 participants