havard / node-openid

OpenID for Node.js
MIT License
293 stars 100 forks source link

openid.sig does not match ourSignature in Node 6 #155

Closed earnubs closed 8 years ago

earnubs commented 8 years ago

Running sample.js against https://login.ubuntu.com on Node 6.4.0 fails with:

Authentication failed: Invalid signature

but succeeds with Node 4.5.0 and 5.12.0. Same result when I try on OS X or Ubuntu.

OS X 10.11.4 (nvm v0.29.0) Ubuntu 14.04.4 LTS (nvm v0.31.1)

havard commented 8 years ago

I am unable to reproduce this on Node 6.4.0.

earnubs commented 8 years ago

For my association.secret I see different results from the hmac result if Node 6, running this in openid project should demonstrate:

var crypto = require('crypto');
var convert = require('./lib/base64.js');

var secret = 'WLZuXHrDyCXKhvOh3Y1FNDQ/w7Azt1J58dFArwLdLh8=';
var str = convert.base64.decode(secret);

var hmac = crypto.createHmac('sha256', str);
var ourSignature = hmac.update('a', 'utf8').digest('base64');

console.log('Test  : ' + ourSignature);
console.log('Node 4: GVmNdtPScTiMu5fVSyQxjUpM26fbrJxaDXy9JGdQuNA=');
console.log('Node 6: aIQAi8OeMUMe0Jzlj+X9ApWdyoHA2hQIRKWZFM+GpIc=');
leider commented 8 years ago

Same problem here.

Possible fix: use standard nodejs functionality to decode base64 strings.

@havard why do you have your own base64 implementation?

havard commented 8 years ago

It's been a while (node 0.6), but if i remember correctly there was no suitable base64 encode/decode routine. If the problem is the base64 encode/decode routine, then obviously this should be changed.

Med vennlig hilsen / Best regards

Håvard Stranden

On Fri, Sep 2, 2016 at 2:11 PM, Andreas Leidig notifications@github.com wrote:

Same problem here.

Possible fix: use standard nodejs functionality to decode base64 strings.

@havard https://github.com/havard why do you have your own base64 implementation?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/havard/node-openid/issues/155#issuecomment-244358044, or mute the thread https://github.com/notifications/unsubscribe-auth/AACzBTHM_fKmpjL649xVsYdDg1gaValvks5qmBKIgaJpZM4Jrw1Q .

leider commented 8 years ago

we use your lib with passport-openid. Unfortunately that still wants openid.js v1. I could provide a pull request. It would be really great if you'd publish a new version 1 together with a new head version.

Is that a plan we could follow together?

leider commented 8 years ago

I'd rather fork passport-openid and update the node-openid version there. I'm fed up with outdated libs. :)

havard commented 8 years ago

Welcome to the wonderful world of Javascript packages. :(

Sure, I would be willing to backport to 1.x, but I'd also prefer a passport-openid dependency update to 2.x.

Med vennlig hilsen / Best regards

Håvard Stranden

On Fri, Sep 2, 2016 at 2:46 PM, Andreas Leidig notifications@github.com wrote:

I'd rather fork passport-openid and update the node-openid version there. I'm fed up with outdated libs. :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/havard/node-openid/issues/155#issuecomment-244365100, or mute the thread https://github.com/notifications/unsubscribe-auth/AACzBQlFINoIcY7pX4gpNkxe6JKBATtSks5qmBqdgaJpZM4Jrw1Q .

leider commented 8 years ago

ACK. I'll fork it and publish it to npm with a new name. @jaredhanson: alternatively you could give me the rights needed to do so (repo and npm)

leider commented 8 years ago

I'd like to currently ignore the ryan test. OK for you?

leider commented 8 years ago

The "Yahoo" tests do not work either. - Are they running on your machine?

havard commented 8 years ago

This has been fixed in 2.0.4 and backported to 1.0.1.