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

crypto: remove POINT_CONVERSION_HYBRID #4956

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
10 participants
@agl
Contributor

agl commented Jan 29, 2016

Compressed points are already rare and, as far as I know, nobody has used
the 'hybrid' format anywhere, ever. It's prohibited in X.509
certificates too[1].

This change removes support for it in the interests of trying to
pare-down the complexity of cryptography.

[1] https://tools.ietf.org/html/rfc5480#section-2.2

@agl

This comment has been minimized.

Contributor

agl commented Jan 29, 2016

Please note: this is different from my other pull requests in that it doesn't fix something. As a personal preference I like to try and keep cryptography options simple. I'm pretty sure that that path to this appearing in node.js was that Bodo implemented very general ECC support in OpenSSL 15 years ago, because things weren't settled then, and node.js supported everything that OpenSSL did because it's not clear which things have since been effectively discarded.

As noted, I'm not aware of any use of hybrid encoding at all and thus seeing this happened to trigger a "wtf" reaction—hence this pull request. But if you feel that you don't want to trim exposed API then that would be completely reasonable.

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Jan 29, 2016

Adding the semver-major tag because this is a backwards incompatible change. LGTM on the technical side but it's hard to say what the fallout of this change would be. It's quite possible the answer is 'none at all'.

CI: https://ci.nodejs.org/job/node-test-pull-request/1434/

/cc @nodejs/crypto

@indutny

This comment has been minimized.

Member

indutny commented Jan 29, 2016

LGTM

@jasnell

This comment has been minimized.

Member

jasnell commented Feb 1, 2016

@nodejs/crypto @nodejs/ctc ...

hmm... as opposed to removing it outright, this may be something we want to take through a deprecation cycle first. We can deprecate in v6 and remove in v7.

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Feb 1, 2016

Virtual shrug. We could also just remove the option from the documentation without actually removing or deprecating it.

@rvagg

This comment has been minimized.

Member

rvagg commented Feb 2, 2016

$ openssl ecparam ?
unknown option ?
ecparam [options] <infile >outfile
where options are
...
 -conv_form arg    specifies the point conversion form
                   possible values: compressed
                                    uncompressed (default)
                                    hybrid
...

openssl supports it on the commandline

although I agree with the sentiment on simplifying and removing clutter that nobody actually uses, I'm just not sure how we'd get data on this one

@shigeki

This comment has been minimized.

Contributor

shigeki commented Feb 2, 2016

OpenSSL has it because it is defined in ANSI X9.62. Apparently the hybrid format has no use so that it might be gone in SEC1 spec. Probably no one has ever used it, however, on the conservative side for deprecation it is good to remove it in the doc at first.

@jasnell

This comment has been minimized.

Member

jasnell commented Mar 22, 2016

@agl ... ping. Can you update this to only remove the option from the docs as suggested?

@estliberitas estliberitas force-pushed the nodejs:master branch 2 times, most recently to c7066fb Apr 26, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented Apr 30, 2016

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Jun 17, 2016

@agl ping

crypto: remove POINT_CONVERSION_HYBRID from documentation.
Compressed points are already rare and, as far as I know, nobody has used
the 'hybrid' format anywhere, ever. It's prohibited in X.509
certificates too[1].

This change removes mentions of it from the documentation in the
interests of trying to pare-down the complexity of cryptography.

[1] https://tools.ietf.org/html/rfc5480#section-2.2

@agl agl force-pushed the agl:hybrid branch to 68d9a64 Jul 5, 2016

@agl

This comment has been minimized.

Contributor

agl commented Jul 5, 2016

Sorry about the delay. This pull request has now been updated to only remove mention of hybrid from the documentation.

@shigeki

This comment has been minimized.

Contributor

shigeki commented Jul 6, 2016

LGTM

1 similar comment
@indutny

This comment has been minimized.

Member

indutny commented Jul 6, 2016

LGTM

The `format` arguments specifies point encoding and can be `'compressed'`,
`'uncompressed'`, or `'hybrid'`. If `format` is not specified, the point will
be returned in `'uncompressed'` format.
The `format` arguments specifies point encoding and can be `'compressed'` or

This comment has been minimized.

@thefourtheye

thefourtheye Jul 10, 2016

Contributor

either "argument specifies" or "arguments specify"

@jasnell

This comment has been minimized.

Member

jasnell commented Aug 27, 2016

LGTM

jasnell added a commit that referenced this pull request Aug 27, 2016

crypto: remove POINT_CONVERSION_HYBRID from documentation.
Compressed points are already rare and, as far as I know, nobody has used
the 'hybrid' format anywhere, ever. It's prohibited in X.509
certificates too[1].

This change removes mentions of it from the documentation in the
interests of trying to pare-down the complexity of cryptography.

[1] https://tools.ietf.org/html/rfc5480#section-2.2

PR-URL: #4956
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell

This comment has been minimized.

Member

jasnell commented Aug 27, 2016

Landed in f4aa2c2 .. fixed the last little nit when landing.

@jasnell jasnell closed this Aug 27, 2016

@sam-github sam-github added doc and removed doc labels Dec 1, 2016

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment