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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement support for signature v4 #660

Merged
merged 4 commits into from
Mar 3, 2021
Merged

Implement support for signature v4 #660

merged 4 commits into from
Mar 3, 2021

Conversation

idevelop
Copy link
Contributor

  • Implement v4 signing algorithm, minor code changes necessary to extract relevant data from the request
  • Fixed failing CORS OPTIONS request when using signed headers (I think OPTIONS should not be verified at all, since it's a different HTTP method so would definitely result in a different signature).
  • Resolved small issue when calculating the canonical query string regarding params with no value (v2 does not require the = to be present, v4 does), so I moved that stripping code into the v2-specific code path
  • Added unit tests for incorrect v4 signatures when using headers as well as using query params
  • Confirmed locally that signed GET and PUT requests work correctly, including CORS, from my web app
  • Confirmed locally that corrupting a signed URL results in a 403

Fixes #659

@idevelop
Copy link
Contributor Author

@kherock is this something you'd be interested in merging?

@fishcharlie fishcharlie mentioned this pull request Feb 11, 2021
test/middleware/authentication.spec.js Show resolved Hide resolved
* @param {String} region received From the credential header
* @param {String} service received From the credential header
*/
function calculateSignatureV4(stringToSign, secretKey, date, region, service) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to break out the V2 and V4 methods into 2 different files? That way it reduces the file size of this file, and we can separate out concerns a little bit?

@kherock
Copy link
Collaborator

kherock commented Feb 12, 2021

These just needs a couple more tests but otherwise this would be a great addition.

I did want to suggest that AWS signing is a problem that has already been solved and it might be a better use of this project's time to offload request signing to another library such as aws4.

Base automatically changed from master to main February 12, 2021 14:09
@kherock kherock force-pushed the sigv4 branch 2 times, most recently from 2b334fe to 338cca8 Compare March 3, 2021 05:55
@codecov
Copy link

codecov bot commented Mar 3, 2021

Codecov Report

Merging #660 (338cca8) into main (3df9541) will increase coverage by 0.34%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #660      +/-   ##
==========================================
+ Coverage   88.80%   89.15%   +0.34%     
==========================================
  Files          20       20              
  Lines        1349     1374      +25     
==========================================
+ Hits         1198     1225      +27     
+ Misses        151      149       -2     
Impacted Files Coverage Δ
lib/middleware/authentication.js 90.47% <100.00%> (+3.06%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3df9541...338cca8. Read the comment docs.

@kherock
Copy link
Collaborator

kherock commented Mar 3, 2021

Sorry for the super long delay with this, I was happy to see the code here mostly working. Thanks for finding the existing bug in the V2 signing as well!

I've switched the default S3 client used in tests to use v4 signing. There were some slight tweaks I needed to make for computing the canonicalized query string to get tests passing.

@kherock kherock merged commit 6083bd8 into jamhall:main Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Signature doesn't seem to be checked
3 participants