Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

crypto: move DEFAULT_CIPHERS, DEFAULT_ECDH_CURVE #7265

Closed
wants to merge 6 commits into from
Closed

crypto: move DEFAULT_CIPHERS, DEFAULT_ECDH_CURVE #7265

wants to merge 6 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Mar 6, 2014

Move DEFAULT_CIPHERS and DEFAULT_ECDH_CURVE from tls module into
the crypto module. Make crypto.createCredentials use default values.

fix #7249

@indutny
crypto: move DEFAULT_CIPHERS, DEFAULT_ECDH_CURVE
a851196 
Move `DEFAULT_CIPHERS` and `DEFAULT_ECDH_CURVE` from `tls` module into
the `crypto` module. Make `crypto.createCredentials` use default values.

fix nodejs#7249
@Nodejs-Jenkins
Copy link

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

The following commiters were not found in the CLA:

  • Fedor Indutny

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

@indutny indutny mentioned this pull request Mar 6, 2014
Closed
@bnoordhuis
Copy link
Member

Seem illogical. The cipher suite is an aspect of SSL/TLS, not of crypto in general.

@indutny
Copy link
Member Author

indutny commented Mar 7, 2014

Seems quite logical to me, but if you don't like it being there, let's move createCredentials to tls module.

@indutny
Copy link
Member Author

indutny commented Mar 13, 2014

ping @bnoordhuis and @tjfontaine

@bnoordhuis
Copy link
Member

let's move createCredentials to tls module.

That seems like a better approach, yes.

@indutny
Copy link
Member Author

indutny commented Mar 14, 2014

moved. cc @tjfontaine

tjfontaine
tjfontaine reviewed Mar 14, 2014
View reviewed changes
src/node_crypto.cc
@@ -1813,7 +1813,7 @@ void Connection::New(const FunctionCallbackInfo<Value>& args) {
HandleScope scope(env->isolate());

if (args.Length() < 1 || !args[0]->IsObject()) {
env->ThrowError("First argument must be a crypto module Credentials");
env->ThrowError("First argument must be a tls module Context");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SecureContext

@indutny
Copy link
Member Author

indutny commented Mar 14, 2014

@tjfontaine fixed!

@indutny
Copy link
Member Author

indutny commented Mar 17, 2014

@tjfontaine and added missing file

@indutny
Copy link
Member Author

indutny commented Mar 27, 2014

@tjfontaine ping

@tjfontaine
Copy link

lgtm, can we get a test for setting the default ciphers for the tls module?

@indutny
Copy link
Member Author

indutny commented Mar 28, 2014

@tjfontaine : sure, done.

@tjfontaine
Copy link

lgtm

@indutny
Copy link
Member Author

indutny commented Mar 29, 2014

Thank you, landed in 5d2aef1

@indutny indutny closed this Mar 29, 2014
@indutny indutny deleted the fix/7249-2 branch March 29, 2014 08:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
crypto master pr
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@indutny @Nodejs-Jenkins @bnoordhuis @tjfontaine @trevnorris