Poll for v0.10 feature: Crypto default to 'binary' strings vs defaulting to buffers

[Isaac Schlueter]

Currently, the crypto module defaults to using 'binary' encoded
strings everywhere as the default input and output encoding.

This is problematic for a few reasons:

1. It's slower than necessary.
2. It doesn't match the rest of Node.

The reason for this is that crypto predates Buffers, and no one ever
bothered to go through and change it. (The same reason it's got some
odd hodgepodge of update/digest methods vs the Stream interface you
see everywhere else in node.)

The reason it persists in 0.8 (and perhaps in 0.10) is that we
(perhaps overly optimistically) labelled that API "stable", and don't
want to break anyone's programs. It's going to change eventually to
match the rest of node. The only question is whether the change will
come in 0.10 or 0.12. A stream interface to all the crypto classes is
coming in 0.10; using 'binary' strings by default is thus even more
obviously a departure from the rest of node.

Note that, if you only use crypto for hashes, and set the 'hex'
encoding, then it won't affect you. If you only ever pass the output
of one crypto function to the input of another (sign/verify, for
example) then it also won't affect you; you'll just pass buffers
around instead of binary strings.

Please select one, and reply with your choice and perhaps any other
feedback you have on this issue. Thanks.

a) Go for it. This won't affect me, and if by chance it does, I don't
mind putting 'binary' args here and there.
b) Please wait. Mark the API as unstable in 0.10, but don't change it
until 0.12.
c) I have no opinion, because I don't use the crypto API directly.


(Disclaimer: Node is not a democracy. The "winning" vote might still
be out-voted by reasonable considerations of the core dev team. This
is informative only ;)

[Joshua Holbrook]

I say go for it. :)

--Josh

> --
> Job Board: http://jobs.nodejs.org/
> Posting guidelines: https://github.com/joyent/node/wiki/Mailing-List-Posting-Guidelines
> You received this message because you are subscribed to the Google
> Groups "nodejs" group.
> To post to this group, send email to nod...@googlegroups.com
> To unsubscribe from this group, send email to
> nodejs+un...@googlegroups.com
> For more options, visit this group at
> http://groups.google.com/group/nodejs?hl=en?hl=en



--
Joshua Holbrook
Head of Support
Nodejitsu Inc.
jo...@nodejitsu.com

[Christian Tellnes]

a) Go for it

[Michael Schoonmaker]

My vote is a', "Please for the love of all that is holy go for it." The incongruence is annoying, and I have to context-switch every time I work with our crypto code.

[codepilot Account]

a)

[Andrew Stone]

Go For it.

[Joshua Gross]

I like B) in theory, as a way to manage APIs. Anyone not reading this newsgroup will at least have *some* advanced warning.

-- Joshua Gross
Christian / SpanDeX.io / BA Candidate of Computer Science, UW-Madison 2013
414-377-1041 / http://www.joshisgross.com

[mscdex]

a) times 100000000

[codepilot Account]

I agree. We aren't to version 1.0 yet, so anything should be fair game.

On Oct 8, 2012 7:19 PM, "mscdex" <msc...@gmail.com> wrote:

a) times 100000000

[Austin William Wright]

(a) Yes, please. Changes in the behavior of binary strings, and the usage of binary strings alone, has hurt me in the past.

And even if Node.js was version 1.0.0, that's still no excuse to not improve the API.

It should go without saying, remember to document and announce the behavior change accordingly.

[Bradley Meck]

a. who is actually messing with crypto after the fact. I would like to know the reasons to do so.

[Andrey]

a) Go for it. This will probably affect me, but I'll be happy to change code for better

[Simon]

a) Go for it. API-breaking changes are somewhat expected in Node and the quality and consistency of Node's API is one of it's strongest points. Keep the quality high even if you make a few breaking changes pre-1.0. The sooner the better.

[Eric S]

a), though in interest of full disclosure, the impact on my current code will be minimal, and for future stuff, I'd rather get the change over with rather than have even more things to change at a later date.

On Monday, October 8, 2012 5:08:54 PM UTC-7, Joshua Gross wrote:

I like B) in theory, as a way to manage APIs. Anyone not reading this newsgroup will at least have *some* advanced warning.

Understandable, but with Node not at 1.0 yet, I'd hope that anyone using node in a production environment has at least one team member keeping up with this list.  Then again, we're talking reality, not some place that makes sense.

[Mike Pilsbury]

a)

(My only use of crypto is to createCredentials for tls.)

[Paddy Byers]

Hi,

a) Go for it.  This won't affect me, and if by chance it does, I don't
mind putting 'binary' args here and there.

I definitely support (a). Might I make a plea also for a proper X509Certificate class to be supported in addition to PEM and other encodings of certificates in the factory methods for Credentials, Signer and Verifier?

We have a glimpse of a certificate class in the tls module with cleartextStream.getPeerCertificate(); but this is the only place in the API where fields of a certificate are exposed. There are also use-cases in signing and verifying where you want to know about certificate details, and details also about non-trivial certificate paths that were constructed in the course of verifying a signature. An example would be knowing whether or not your validated path qualifies as a valid EV path, or verifying the signature on in an XML signature document.

I know the argument is always that this functionality can go in user land in an independent module, instead of in core; and there are modules that do some of this such as dcrypt [1]. The problem is that when you do that you have to re-implement all of the core functionality as well on top of your external certificate library, just because you're unable to pass a certificate object into the APIs in the core.

So my suggestion would be to include X509Certificate and X509CRL classes that wrap native OpenSSL X509 structures, and for these to be supported as well as strings in the relevant APIs. Once that is in place, I think the more esoteric use cases can be supported in userland without lots of duplication of code.

I'm happy to contribute to the work, and some time ago started implementing support for this [2] based on dcrypt. You can see from the amount of code in there that's simply cut+paste from core that it really would be a fairly modest delta; much of the functionality is already there, but disorganised.

Thanks - Paddy

[1]: https://github.com/dekz/dcrypt

[2]: https://github.com/paddybyers/dcrypt

[Diogo Resende]

I go for a) but also agree that b) would be better for people outside this list. Could we have some kind of mixing, having the old and new interface together, a warning on old interface and then on the next version it could be removed (or throw..).

-- 

Diogo Resende

--

[Micheil Smith]

a) Go for it.

Although, probably wise to make sure that it's publicised a fair bit; Anyone who
hits issues can hang around on an older version of Node, until they can upgrade.

[phidelta]

d) I use it a lot and find the strangeness of binary strings so dumb that I'd rather have it changed sooner or later even if that means having to rewrite/modify a bit of code.

[Matt]

a - go for it. This has bugged me for long enough.

[Jimb Esser]

a) Go for it.  Looks like it would have no effect on almost all of our crypto code.

On Monday, October 8, 2012 4:24:36 PM UTC-7, Isaac Schlueter wrote:

[Isaac Schlueter]

Seems pretty unanimous here. So, unless some new objection comes up
that is very compelling, let's assume that 0.10 will use Buffers by
default in crypto instead of binary strings.

Also, a streaming interface to the crypto classes is already underway.

[Jannick]

a) go for it!

[Dan Shaw]

a) go for it.

Daniel Shaw
@dshaw

[Jason Brumwell]

a++

[shawn wilson]

No idea why the comment about warning when you give crypt binary didn't gain more notice, but... why not make a new interface instead of changing the current one and possibly breaking stuff?

You could eventually make the old API the same as the new (in a year? Whenever github searches come up empty?) It wont affect me and it seems the modules I use that use crypt are updated frequently enough that I trust I'll be fine, but what's the point of breaking stuff if you don't have to?

[codepilot Account]

Just out of curiosity, will this be the last nail in the coffin of 'binary' encoding? At least as the default, I mean.

[mscdex]

On Oct 9, 6:21 pm, codepilot Account <codepi...@gmail.com> wrote:
> Just out of curiosity, will this be the last nail in the coffin of 'binary'
> encoding? At least as the default, I mean.

As a default, I'd hope so.

Last nail in the coffin totally though? I'd hope not. This particular
discussion (eliminating this encoding altogether) has been had on the
list here and on node's github issues almost ad nauseum, but I am
still in favor of keeping it around (with a note discouraging its use
in the docs) until Buffer's core capabilities are equivalent or nearly
equivalent to those of strings (e.g. indexOf, lastIndexOf, etc).

[Stewart Mckinney]

my vote is a), go for it

[murvinlai]

Go for it. 

[Luke Arduini]

b)

Remember the sys/util situation in 0.8?

>1. What is the cost of keeping "sys" throwing?
>2. What is the cost of putting it back?

This is a different type of change entirely but I think the general
idea of the questions is still applicable:

1. What is the cost of this change being made as soon as 0.10 when
the crypto API is currently marked as stable in 0.8?
2. What is the cost of marking the crypto API as unstable for 0.10,
then making the change in 0.12?

With the sys/util situation, the concern was that the 'increased
difficulty of migrating code' could be harmful to the project from a
long term perspective, including the possibility of it injuring
node's credibility in the "no, it'll work, trust me" sense.

If the labels for stability of node's core modules aren't respected,
aren't we sort of in the same situation?

Cost of 1 is it might break a bunch of peoples modules even though
the API was marked as stable. I guess these people should have
subscribed to the mailing list.

I don't know of any cost to 2, just the benefit of giving people
notice that the API is unstable again so they should expect
potential module breakage on the next version.

I like the idea of stuff changing for the better as quickly as
possible as much as the next person here, but I think being
consistent with breaking API changes is more important.

On Oct 8, 7:24 pm, Isaac Schlueter <i...@izs.me> wrote:
> Currently, the crypto module defaults to using 'binary' encoded
> strings everywhere as the default input and output encoding.
>
> This is problematic for a few reasons:
>
> 1. It's slower than necessary.
> 2. It doesn't match the rest of Node.
>
> The reason for this is that crypto predates Buffers, and no one ever
> bothered to go through and change it.  (The same reason it's got some
> odd hodgepodge of update/digest methods vs the Stream interface you
> see everywhere else in node.)
>
> The reason it persists in 0.8 (and perhaps in 0.10) is that we
> (perhaps overly optimistically) labelled that API "stable", and don't
> want to break anyone's programs.  It's going to change eventually to
> match the rest of node.  The only question is whether the change will
> come in 0.10 or 0.12.  A stream interface to all the crypto classes is
> coming in 0.10; using 'binary' strings by default is thus even more
> obviously a departure from the rest of node.
>
> Note that, if you only use crypto for hashes, and set the 'hex'
> encoding, then it won't affect you.  If you only ever pass the output
> of one crypto function to the input of another (sign/verify, for
> example) then it also won't affect you; you'll just pass buffers
> around instead of binary strings.
>
> Please select one, and reply with your choice and perhaps any other
> feedback you have on this issue.  Thanks.
>

[Ege Özcan]

Crypto API breaking can create security problems. Also something marked as "stable" which stops working in the next version is not good for any project. I'd go for the option b.

[Isaac Schlueter]

Luke,

That's a good point, I should have made the costs more clear initially.

In this case, the cost of delaying until 0.12 is that we delay what I
consider to be one of the 2 main features of 0.10. Those features
are:

1. The Streams API is consistent across node, used wherever
appropriate, and easier to extend.
2. HTTP/TLS are less crappy by default.

The only change is that crypto methods that previously expected a
binary string by default will expect a buffer by default, just like
every other function in node, and that methods that return a binary
string by default will return a buffer by default, just like every
other function in node.

So, this is not a zero-benefit change (as sys/util was), and it's not
zero-cost to keep (as sys/util was). I don't feel like an idiot
trying to explain to someone why their program requires them to add a
new 'binary' argument (or refactor to use Buffers). And, unlike the
sys module, very few people actually use these APIs directly.

The cost of keeping it as it is, or delaying until 0.12, is that it
continues to be a confusing pain-point for users, and an awful
overly-complicated part of the code. At least in master today, you
can pass 'buffer' as an argument to get a buffer out of it, which
previously was impossible. But it's still

On Thu, Oct 11, 2012 at 12:28 AM, Ege Özcan <ege...@gmail.com> wrote:
> Crypto API breaking can create security problems.

Sorry, that's FUD. Show me a use-case where that's the case, and
we'll figure out what is the best way to handle it.

> Also something marked as
> "stable" which stops working in the next version is not good for any
> project. I'd go for the option b.

Overdue: https://github.com/joyent/node/commit/99b2368a6cd408e75850ac73585de8800e2a10a1

That doesn't mean that it's necessarily going to change in 0.10. But
it was a mistake to call something stable that is such an odd wart on
the Node API.

[Isaac Schlueter]

Oops, premature send, sorry.

On Thu, Oct 11, 2012 at 8:54 AM, Isaac Schlueter <i...@izs.me> wrote:
> The cost of keeping it as it is, or delaying until 0.12, is that it
> continues to be a confusing pain-point for users, and an awful
> overly-complicated part of the code. At least in master today, you
> can pass 'buffer' as an argument to get a buffer out of it, which
> previously was impossible. But it's still

But it's still confusing and weird, because it doesn't match the rest
of the Node API. If we put a streaming interface on it, then we'll
have to do a bunch of hoop-jumping to get it right. Also, there are
parts that are just broken. For example, using hex encoding in some
of these functions just blows up with an assertion error, and
apparently always has. The fact that no one has ever complained about
this makes me think that it's just not an API that gets a lot of use,
and is probably pretty safe to fix.

[Trevor Norris]

(a) go for it

It seems like it would be reasonable to make the change at the same time you introduce the streams2 branch. Don't worry about migrating it the current, just to change it over.

On Monday, October 8, 2012 4:24:36 PM UTC-7, Isaac Schlueter wrote:

[Diogo Resende]

Not crypto related but since you're trying to give consistency to the API, why not make other changes like:

var sock1 = new require("net").Socket(...);

var sock1 = require("dgram").createSocket(..);

I'm sure there aren't a lot of inconsistencies but this is one that bugs me..

-- 

Diogo Resende

--

[Diogo Resende]

I just realized there is dgram.Socket.. but the documentation says:

The dgram Socket class encapsulates the datagram functionality. It should be created viadgram.createSocket(type, [callback]).

-- 

Diogo Resende

[Isaac Schlueter]

If you have other complaints about Node's API, please post issues at
https://github.com/joyent/node/issues so that we can keep this thread
on topic.

Thanks.