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

the solution for the issue #6374 has being applied only for Node 6.0.0 #9738

Closed
Anatoliy4041 opened this issue Nov 22, 2016 · 15 comments
Closed
Labels
crypto Issues and PRs related to the crypto subsystem. question Issues that look for answers.

Comments

@Anatoliy4041
Copy link

Dear all,
The issue #6374 was solved, however, it was applied just fo Node 6.0.0 version and there is no any changes in node_crypto.cc in higher versions. Could we sort it out?
Thanks!

@Anatoliy4041 Anatoliy4041 changed the title the issue #6374 has being applied only for Node 6.0.0 the solution for the issue #6374 has being applied only for Node 6.0.0 Nov 22, 2016
@mscdex
Copy link
Contributor

mscdex commented Nov 22, 2016

I'm not sure what you mean, originally it landed in v6.0.0 and it is also in v7.0.0.

@mscdex mscdex added crypto Issues and PRs related to the crypto subsystem. question Issues that look for answers. labels Nov 22, 2016
@Anatoliy4041
Copy link
Author

I just downloaded Node 7.1.0 and did't find it. In addition, there is no changes in the current https://github.com/nodejs/node/blob/master/src/node_crypto.cc

@gibfahn
Copy link
Member

gibfahn commented Nov 22, 2016

@Anatoliy4041 So this was added in v6.0.0, and was in v6 until v6.8.0.

It was overwritten by db411cf in master, and that was backported to v6.x in 1ea0358a. That commit should be in v6, v7, and master. Are you saying that @indutny's commit caused a regression?

@sam-github
Copy link
Contributor

@Anatoliy4041 We'll close this soon unless you give more information, it doesn't look like an issue.

@Anatoliy4041
Copy link
Author

Hi Guys! I'm appologize for the late answer.

Yes, I found the regression with regards to #6374 in lateset verion and it leads (in my case) to the issue I can't apply in NodeJS other crypto engines then those to be set as default in openssl (e.g. NodeJS doesn't recognize the algorithms provided by GOST engine). This problem was described in detail in #5739 by stefanmb so I just would like to propouse to fix it in lates versions of NodeJS

@sam-github
Copy link
Contributor

@Anatoliy4041 sorry, but that didn't clarify anything... can we close this issue?

@Anatoliy4041
Copy link
Author

@sam-github Could you please exlain what's not clear? Then I'll try to clerify in another way.
In my opinion, if there was the issue (described #5739, #6374) which were perfectly solved for elder version of NodeJS, however it is not applied for the latest one - that should be dealt in some way.

@gibfahn
Copy link
Member

gibfahn commented Dec 16, 2016

In my opinion, if there was the issue (described #5739, #6374) which were perfectly solved for elder version of NodeJS, however it is not applied for the latest one

This is not true, the commit is still in the latest versions of node (master, v6, and v7). However it was superseded by @indutny 's commit in v6.9.0, which made this change:

-  OPENSSL_config(NULL);
+  OPENSSL_no_config();

The issue should still be fixed though. So master, the latest v7, and the latest v6 should work.

If you think @indutny 's change broke the fix that @stefanmb applied, then you should find that v6.8.1 works, but v6.9.0 doesn't. If you have confirmed this then please let us know and we'll look into the issue. However the answer won't be to just apply #6374 to v6, that's already been done.

@gibfahn
Copy link
Member

gibfahn commented Dec 16, 2016

@Anatoliy4041 if you want to see if the node v6 contains the backport, you can just look for the commit in the tag like so:

Commit: d6237aa

git clone https://github.com/nodejs/node.git && cd node
git checkout v6.9.2 # Latest v6 version
git branch --contains d6237aa7c642e82b539803b6d2069eb4eddce6a6

You should see:

➜  node git:(94a3aef) git branch --contains d6237aa7c642e82b539803b6d2069eb4eddce6a6
* (HEAD detached at v6.9.2)

Which tells you that the v6.9.2 tag (latest v6 release) still contains the backport of Stefan's fix, it was just overwritten by @indutny's fix as explained above.

@temaivanoff
Copy link

temaivanoff commented Dec 17, 2016

I understand how this debate is designed to discuss what you need to save OPENSSL_no_config (null); in all versions, and take this into account in future versions as a solution to the problem with additional encryption methods. The impetus was the use of [PR] (stefanmb@4029815) version 6.x.x and completely absent in 7.x.x although complaints about this solution had not been reported. Maybe it's not a very elegant solution, and you need to consider more substantial, but it is very helpful, and I would like to keep it. Sorry for my English :\

@gibfahn
Copy link
Member

gibfahn commented Dec 18, 2016

@fast0490f This sentence isn't true:

and completely absent in 7.x.x

The commit was in v7, but it was replaced by something better.


what you need to save OPENSSL_no_config (null); in all versions

There was never an OPENSSL_no_config (null); in the code anywhere. Here's what happened:


v7 should still work due to the second commit. No-one is saying they don't want this fix, we're saying we already have a better version of this fix.

@gibfahn
Copy link
Member

gibfahn commented Dec 18, 2016

@indutny this comment from @Anatoliy4041 appears to be saying that c32be9a caused a regression in the fix from 56b9478. Can you comment?

@indutny
Copy link
Member

indutny commented Dec 18, 2016

@gibfahn I see.

@Anatoliy4041 have you tried passing --openssl-config= argument to node? I can't see it anywhere in discussion. Hopefully this should work.

@temaivanoff
Copy link

temaivanoff commented Dec 19, 2016

@gibfahn

what you need to save OPENSSL_no_config (null); in all versions

I made a mistake and meant `OPENSSL_config(NULL)

@indutny @Anatoliy4041

have you tried passing --openssl-config= argument to node?

I tried node --openssl-config="/etc/ssl/openssl.cnf" and it works.

tls.getCiphers();
Showing all connected encryption algorithms from openssl.cnf also checked the GOST algorithms and they are working. I confirm that everything works fine. I think can close the discussion

@gibfahn
Copy link
Member

gibfahn commented Dec 19, 2016

Okay great, I'll close the issue. @Anatoliy4041 if you still think there's a regression feel free to comment on here.

@gibfahn gibfahn closed this as completed Dec 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. question Issues that look for answers.
Projects
None yet
Development

No branches or pull requests

6 participants