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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented RSA-SHA1 signatures for OAuth v1 #227

Merged
merged 3 commits into from Jun 13, 2016

Conversation

@PaulMougel
Copy link
Contributor

PaulMougel commented Jun 3, 2016

Here's a first draft which should fix #226. Feedback is welcome 馃憤

@@ -52,6 +52,7 @@ internals.schema = Joi.object({
password: Joi.string().required(),
clientId: Joi.string().required(),
clientSecret: Joi.string().required().allow(''),
signatureMethod: Joi.string().valid('HMAC-SHA1', 'RSA-SHA1').when('provider.protocol', { is: 'oauth', otherwise: Joi.forbidden() }),

This comment has been minimized.

Copy link
@ldesplat

ldesplat Jun 3, 2016

Contributor

I don't think there is a reason for a user of a provider would need to choose this option? If there is not, then it should be included in the provider object just above.

Also, when the protocol is oauth, do then: Joi.default('HMAC-SHA1') . This will ensure backwards compatibility and ensure it's always set. I know you take care of it on line 382 of lib/oauth.js but why not let Joi handle it for you.

@@ -378,7 +379,7 @@ internals.Client.prototype._request = function (method, uri, params, oauth, opti
oauth.oauth_nonce = Cryptiles.randomString(internals.nonceLength);
oauth.oauth_timestamp = Math.floor(Date.now() / 1000).toString();
oauth.oauth_consumer_key = this.settings.clientId;
oauth.oauth_signature_method = 'HMAC-SHA1';
oauth.oauth_signature_method = this.settings.signatureMethod || 'HMAC-SHA1';

This comment has been minimized.

Copy link
@ldesplat

ldesplat Jun 3, 2016

Contributor

If you change the Joi validation, then this is not needed anymore and you can just set this.settings.signatureMethod

if (oauth.oauth_signature_method === 'RSA-SHA1') { // RSA-SHA1 (3.4.3)
const sign = Crypto.createSign('sha1');
sign.update(baseString);
sig = sign.sign(this.settings.clientSecret).toString('base64');

This comment has been minimized.

Copy link
@ldesplat

ldesplat Jun 3, 2016

Contributor
  1. Instead of doing .toString you can pass the base64 to the 2nd parameter of sign.
  2. I would also suggest you chain the calls to match the code below.
  3. Format this slightly differently:
  4. instead of doing the else, just return from the function early. Then you don't need to create the let sig variable and I find we usually try to return early rather than doing if/else stmts.

mock.server.inject(res.headers.location, (mockRes) => {

expect(mockRes.headers.location).to.equal('http://localhost:80/login?oauth_token=1&oauth_verifier=123');

This comment has been minimized.

Copy link
@ldesplat

ldesplat Jun 3, 2016

Contributor

Due to the 100% test coverage and the other test cases, I know we exercise the RSA-SHA1 path here, but I don't remember if there's a way to verify in the test case itself. ie. check the signature is RSA-SHA1? That will be useful long term and make this a slightly better test.

@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Jun 3, 2016

Thank you so much. This looks great! I did an initial look over and made some comments. I've noted down a couple of things to check up on my side as well.

Now I just hope nobody asks for for the PLAINTEXT signature. Not one I am really enthusiastic about :)

@PaulMougel

This comment has been minimized.

Copy link
Contributor Author

PaulMougel commented Jun 4, 2016

Thank you for the comments.

Good call on moving the signatureMethod to the provider configuration object. However, I don't see how I can do that without messing up with the Mock object, see mock.js on line 141:

return callback({
    protocol: 'oauth',
    temporary: this.server.info.uri + '/temporary',
    auth: this.server.info.uri + '/auth',
    token: this.server.info.uri + '/token'
   // should I add signatureMethod here?
});

How can I send the new parameter without impacting every single test case?

@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Jun 10, 2016

Apologies for the delayed response.

I think this can easily be done by adding a parameter / option to the Mock constructor. Then simply make the default what it currently is, and only when that parameter is supplied do you override. So then, you only have your testcases impacted.

@ldesplat ldesplat added the feature label Jun 10, 2016
@ldesplat ldesplat added this to the 7.9.0 milestone Jun 10, 2016
@PaulMougel

This comment has been minimized.

Copy link
Contributor Author

PaulMougel commented Jun 13, 2016

Here you go! Tell me if that's better.

@ldesplat

This comment has been minimized.

Copy link
Contributor

ldesplat commented Jun 13, 2016

Thank you very much. This looks perfect! 馃憤

@ldesplat ldesplat merged commit 78d3243 into hapijs:master Jun 13, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented on lib/index.js in 71af0b1 Jun 22, 2016

How is this not a breaking change?

This comment has been minimized.

Copy link
Contributor

ldesplat replied Jun 23, 2016

This was part of this PR #227
It's a new feature and I asked the signatureMethod to be moved. It should default to HMAC-SHA1, I am surprised you had to set it explicitly. That was the whole point of setting Joi.default I guess the .when doesn't work the way I thought it did.

This comment has been minimized.

Copy link
Member

hueniverse replied Jun 23, 2016

First, changing the structure of the config is a breaking change. I set it explicitly because without it I can't use it with the OAuth Client directly which worked before. But this change dropped the default from the client code and moved it to the plugin setup which broke it.

This comment has been minimized.

Copy link
Contributor

ldesplat replied Jun 23, 2016

I'll make sure in the future that if those change, this becomes a breaking change but you do have to understand that from the publicly documented API this is a semver minor change. The default is still being set.

This comment has been minimized.

Copy link
Member

hueniverse replied Jun 23, 2016

The documentation means nothing. Whatever the code exposes as a public interface (e.g. not with an underscore prefix) is part of the deal.Is someone was using the signatureMethod option, you just broke their code.

This comment has been minimized.

Copy link
Contributor

ldesplat replied Jun 23, 2016

The signatureMethod did not exist before the #227 PR and this commit is part of that PR.

The one case where somebody's code is broken is when they create the Client explicitly themselves and I did not consider that scenario. That's definitely my fault.

This comment has been minimized.

Copy link
Member

hueniverse replied Jun 23, 2016

Ok. I see. You are correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can鈥檛 perform that action at this time.