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

tls: docs error regarding dhparam #958

Closed
silverwind opened this issue Feb 25, 2015 · 7 comments
Closed

tls: docs error regarding dhparam #958

silverwind opened this issue Feb 25, 2015 · 7 comments
Labels
tls Issues and PRs related to the tls subsystem.

Comments

@silverwind
Copy link
Contributor

The docs say:

dhparam: DH parameter file to use for DHE key agreement. Use openssl dhparam command to create it. If the file is invalid to load, it is silently discarded.

The fact that this seems to take a file instead of a buffer or string strikes me odd, as all other TLS options take these types. I've searched a bit and it seems there are no fs calls to read such a file. I see this in lib/_tls_common.js:

 if (options.dhparam) c.context.setDHParam(options.dhparam);

Could it be that the docs are just plainly wrong about this option?

@silverwind
Copy link
Contributor Author

cc: @shigeki @indutny
original pr: nodejs/node-v0.x-archive#8272

@silverwind
Copy link
Contributor Author

Reading through test/simple/test-tls-dhe.js makes it pretty obvious that the fs call happens in module-space. Can anyone confirm that crypto handles both string and buffer? I'll submit a PR.

@indutny
Copy link
Member

indutny commented Feb 25, 2015

Yeah, I'm +1 for buffer, though we may need to try to load file anyway, since we are doing it now. Mind helping with this?

@silverwind
Copy link
Contributor Author

If buffer isn't supported yet, I can add it. Should be easy enough as cert, ca and key support it already, so I can lend code from these parts.

@shigeki
Copy link
Contributor

shigeki commented Feb 26, 2015

@silverwind Good catch. Yes, it is not a file but string or Buffer that are the same as other pem parameters. Could you submit a PR to fix this? Thanks.

@shigeki shigeki added the tls Issues and PRs related to the tls subsystem. label Feb 26, 2015
@shigeki shigeki self-assigned this Feb 26, 2015
@silverwind
Copy link
Contributor Author

Sure, will do a PR tomorrow if that's timely enough. You mean that string and buffer are already supported?

@shigeki
Copy link
Contributor

shigeki commented Feb 26, 2015

Yes, both is supported. I've just tested it with using fs.readFileSync(path + '/dh' + n + '.pem').toString() .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants