iojs: Detect no start line error as sign error

[ryanseys]

I ran all the tests with iojs instead of node and only 1 failed due to a different error occurring when attempting to sign a key. I just added the check here so now all tests pass with iojs.

Aside: Not that we want to support iojs (yet) but figured I'd have a little fun with it and see how well supported it is. Actually, the fact that iojs isn't 100% compatible and that this PR is necessary at all may be a bug on their end, not ours. Thoughts?

[ryanseys]

Actually thinking about this a bit more, any sort of error that occurs with the sign will need to be detected. In Node, lib/crypto.js would return a SignFinal error which appears to still happen in iojs see here but it must be being short-circuited by this PEM error before hand, probably around here.

¯\_(ツ)_/¯

[stephenplusplus]

Sounds like an issue upstream.

[sindresorhus]

I would try to get it fixed upstream first. It seems they're very quick at fixing bugs and shipping new patch releases ;)

[addyosmani]

Upstream issue. +1 on what Sindre said.

[ryanseys]

Thanks all! An issue has been filed.

[ryanseys]

As the issue points out, this is a change present in 0.11 so it's not an upstream issue as far as they (io.js) are concerned but an artifact of using 0.11 for io.js. I've updated this PR to detect the difference in error messages for all PEM errors however still unsure whether this is ideal or desired.

[stephenplusplus]

I'm fine with merging... in its current form, I couldn't see this hurting anything.

2 questions though:

1. Could you add a comment above the new error check linking to this issue with a brief explanation of why we included this?
2. Do you know if there are any other variations of the string "PEM Routines" that could be found in a PEM-related error message? If so, we may as well cover all of them.

[ryanseys]

Yeah I can add a comment. I don't think so at least for what we are trying to do.

[ryanseys]

The error is in the form: "error:some_code:PEM routines:PEM_read_bio:<error_name>" so we can just look for "error:" or what I currently have: "PEM routines:". What do you think is best? I'm thinking maybe "error:" might be better.

[ryanseys]

I updated the PR to reflect checking for "error:" instead so it's pretty generic.

[stephenplusplus]

Does the error that is thrown with error:some_code:etc always originate from a PEM file? Just wondering if other areas of the codebase could throw error:some_code:something_not_pem_related.

[ryanseys]

Yeah by the looks of it here there's the possibility of basically anything going wrong. I'm surprised I don't see an "arriving late to your daughter's soccer practice" error in there.

[stephenplusplus]

I'm only concerned with using error: since this is our catch-all API error handling. If an error message from any endpoint were to return "error:" in the error.message string, we would print out the private key error. We should probably go back to the "PEM routines" method.

[ryanseys]

No problem. For the sake of signing a key, I believe it's always a PEM we
are using so this should in theory be the only error people encounter. I
can spend some more time testing with different keys to trust myself more.

On Tuesday, January 20, 2015, Stephen Sawchuk [email protected]
wrote:

I'm only concerned with using error: since this is our catch-all API
error handling. If an error message from any endpoint were to return
"error:" in the error.message string, we would print out the private key
error. We should probably go back to the "PEM routines" method.

—
Reply to this email directly or view it on GitHub
#346 (comment)
.

[ryanseys]

So I just mucked around with the key, replacing random characters and got a bunch of different errors:

[Error: error:0906D064:PEM routines:PEM_read_bio:bad base64 decode]
[Error: error:0D0680A8:asn1 encoding routines:ASN1_CHECK_TLEN:wrong tag]
[Error: error:0D07207B:asn1 encoding routines:ASN1_get_object:header too long]
[Error: invalid_grant]

The most notable being invalid_grant which was returned from the server i.e. the library didn't fail but Google certainly didn't like what it got.

[ryanseys]

Based on this, I think we are best to have a catch-all and search for "error:" and in the off-chance that our library doesn't fail but we get "invalid_grant" back, we might want to handle that separately?

[stephenplusplus]

It's very possible I'm wrong, but this code handles errors from all of our API requests, doesn't it? As in, if I'm uploading a file or running a query on a dataset, etc. I'm not fully sure all of those upstream API endpoints wouldn't use the phrase error: in an error message.

[ryanseys]

The if(err) block this PR is in is only reached when authorization fails, that is, google-service-account attempted to give us a token but couldn't. Yes this method is called on every request, but if auth doesn't fail first, then this line handles the normal API errors that come through.

[stephenplusplus]

Phew. Sounds good to me!

[ryanseys]

Aside: Yeah that auth code is pretty mind-busting to understand. Closures be everywhere! 😓

[ryanseys]

Hopefully that'll be the last of the iojs bugs HAHAHAHAHAHA.... hahahaha... ha ha ha .... sigh

[stephenplusplus]

Haha :'(