FT: Auth v4 route #37
Conversation
|
👍 |
| assert(typeof accessKey === 'string' && accessKey !== '', | ||
| 'accessKey is required'); | ||
| assert(typeof region === 'string' && accessKey !== '', | ||
| 'region is required'); |
There was a problem hiding this comment.
You're doing two assert for the accessKey parameter, I think the second is useless (done just before), or you mean region instead.
|
Please do like this: https://github.com/scality/vaultclient/blob/931d36e/lib/IAMClient.js#L185-L187 |
| accessKey, | ||
| region, | ||
| scopeDate } }; | ||
| data[requestUidKey] = options.reqUid; |
There was a problem hiding this comment.
You could put it in the object creation just before.
Nevermind, after reading @adrienverge comment, I get it.
There was a problem hiding this comment.
No she couldn't, because there's a trick ;)
It's not data.requestUidKey, it's data[requestUidKey] (i.e. data['x-scal-xxx'])
LaureVergeron
force-pushed
the
FT/authV4-2
branch
from
6ea0b28 to
561b12e
Compare
| * @param {IAMClient~requestCallback} callback - callback | ||
| * @returns {undefined} | ||
| */ | ||
| verifySignatureV4(string, signature, accessKey, region, scopeDate, options, |
There was a problem hiding this comment.
why not named it stringToSign ?
There was a problem hiding this comment.
For no valid reason - updated
|
@adrienverge : done, thanks for finding it! |
|
👍 |
|
Once you've fixed the problem at line 211 (pointed by @MathieuCassagne) it will be good for me. |
LaureVergeron
force-pushed
the
FT/authV4-2
branch
from
561b12e to
888a52a
Compare
|
@adrienverge : I reckon it's fixed already, no? |
LaureVergeron
force-pushed
the
FT/authV4-2
branch
from
888a52a to
7049f99
Compare
|
@adrienverge @MathieuCassagne : fixed line, I did mean region, thanks for spotting |
| * Verify AWS request signature using V4 auth (contrary to v2, hash is | ||
| * always sha256) | ||
| * | ||
| * @param {string} stringToSign - string to sign as built from the request |
There was a problem hiding this comment.
Sorry for waking up late, but I don't agree with @ThibaultRiviere.
verifySignatureV2() uses string, why not being consistent and change the var name? Moreover, it's not a string to sign but a string to verify.
Could you switch it back? (Once again, sorry for saying this 3 min too late).
There was a problem hiding this comment.
Actually, Thibault's point makes sense: in AWS IAM / permissions , this is called the stringToSign accross documentation... the right thing to do would be to update v2, but that's a different P, I guess?
The reason it was called string is because of consistency with v2, but consistency with an ambiguous nomenclature isn't worthwile, is it?
There was a problem hiding this comment.
Thanks for explaining the reason, this is a very good reason indeed. In this case V2 will have to be fixed, yes -- but later is fine for me. :)
|
Thanks @LaureVergeron 👍 |
|
👍 |
Implement authentication V4 signature verification route
| region, | ||
| scopeDate } }; | ||
| if (options.reqUid !== undefined) { | ||
| data[requestUidKey] = options.reqUid; |
There was a problem hiding this comment.
Just out of curiosity: which bug does this solve?
There was a problem hiding this comment.
Something about req.setHeader(key, value) complaining about an undefined value.
ghost
deleted the
No description provided.