crypto: Show exponent in decimal and hex

[iamthechad]

Show the exponent in a manner that matches OpenSSL
This is a resubmission of nodejs/node-v0.x-archive#25674 to this repo per the comments there.

[bnoordhuis]

/cc @nodejs/crypto

One problem I see with this change is that +'65537' evaluates to 65537 but +'65537 (0x10001)' evaluates to NaN.

I don't know how common it is to use the exponent in calculations but that will stop working. -1 from me for that reason.

[indutny]

-1 from me too. Either dec or hex, not both.

[iamthechad]

I've got to admit I'm a bit confused here. I originally opened nodejs/node-v0.x-archive#17202 in the joyent/node repo. In the discussion for that issue, @misterdjules offered to mentor me regarding the changes. Based on his input, these changes were made to make the peerCertificate output of exponent mimic what OpenSSL shows.

Based on @jasnell 's input, the original PR I submitted at nodejs/node-v0.x-archive#25674 was resubmitted against this repo.

Is there a desire now to handle crypto differently? I know I use this exponent value in my project to display certificate information, so changing to only decimal or hex would require an extra step to calculate the other value and show it.

[bnoordhuis]

Sorry you got the wrong impression but @misterdjules and @jasnell don't normally sign off on changes to the crypto subsystem. @nodejs/crypto does and we're a pretty conservative bunch.

You are no doubt aware of this but if you want both the decimal and the hexadecimal representation, you can do this:

var exp = +peerCert.exponent;
var str = `${exp} (${exp.toString(16)})`;

[jasnell]

Yes, I'm sorry if we did give the wrong impression. My goal in recommending moving the conversation here was to make sure the suggested change received the appropriate review and attention. I completely trust the judgement of @bnoordhuis and @indutny on this.

[targos]

Changing it to hex literal seems fine. You can still convert it with +exp:

var exp = peerCert.exponent; // "0x10001"
var str = `${+exp} (${exp})`; // "65537 (0x10001)"

[iamthechad]

Okay. I think I'm straightened out on everything.

Should I change it to just hex with 0x prefix then?

[bnoordhuis]

Is there a point in changing it to hex? Change for the sake of change is not a good thing IMO.

[iamthechad]

It's already in hex, just without the 0x prefix which can be confusing. This was why I opened the original issue: nodejs/node-v0.x-archive#17202

[bnoordhuis]

You're right. I thought we were using BN_bn2dec() but I see now it's BN_print(). Okay, sounds like a good change.

[iamthechad]

Changed.

[bnoordhuis]

LGTM but can you update the commit log? Please add a line or two explaining that the value was already in hex but without the "0x" prefix.

CI: https://jenkins-iojs.nodesource.com/job/node-test-pull-request/56/

[bnoordhuis]

Failures on armv7-wheezy appear to be unrelated, everything else is green.

[iamthechad]

Commit message updated. I submitted a PR back in may to update the docs for 0.10 to explain the encoding: nodejs/node-v0.x-archive#25142. Should I resubmit that against this repo as well?

[jasnell]

nodejs/node-v0.x-archive#25142 should be fine (although the commits in it should be squashed). Documentation fixes in v0.10 and v0.12 are definitely more than welcome.

[jasnell]

This PR LGTM

[bnoordhuis]

@iamthechad Your Author field is 'iamthechad' now. We have a mild preference for real names. Do you want to update? (I can do it for you too when landing it if you tell me.)

[iamthechad]

@bnoordhuis I'll let you change it this time. If I end up making more contributions, I'll change my settings. I assume you're talking about the user.name setting, correct?

[jasnell]

@iamthechad ... thank you for being patient with us through this process. I do hope that you consider sticking around and making additional contributions. It's always great having another set of eyes on the code.

[bnoordhuis]

@iamthechad Yes, user.name. Thanks for the pull request, landed in 847459c.

[bnoordhuis]

By the way, from your blog post here:

Finally, I found a single page that offered anecdotal evidence that NodeJS cannot create sockets in a shared directory in a VM. Looking back at the failing tests, they were all socket or pipe related.
I have not been able to find “official” confirmation of this. If anyone can confirm or deny, please let me know.

That's correct, but it's not a node.js limitation, it's a limitation of the underlying file system. node.js just passes on the error from the operating system.

[silverwind]

Is this really a semver-major?

[bnoordhuis]

Rule of thumb: if there is the tiniest sliver of a chance of breaking working code, it's semver-major.

[silverwind]

Indeed, I'm just wondering why a major went in now. I thought we're holding off on these until the converged 4.0.0 release.

[bnoordhuis]

Maybe this is where @Fishrock123 should chime in. I thought the plan was to do development on master and cherry-pick bug fixes and incremental additions to the v3.x branch.

[Fishrock123]

semver-major changes can go into master, since it is the dev branch and we'll cut a new major off of it at some point.

I think we should still label everything though, simply because it will make cherry-picking to the stable and LTS branches easier for the people doing that. :)

I.e. Virtually everything non-breaking/major can go into stable, we can still bump stable minors, etc.