Fix #4882, use different secrets for different signing purposes #4883
Conversation
This adds signing 'scopes', so that if you get something signed for one scope, you can't use it for another scope. E.g., you can't get a download URL and use that for an authentication key. This keeps the 'legacy' scope, which is the current single key. This can be used now to make sure everything works when people upgrade, but removed later as people have used to the new specific scopes. But nothing new will be signed with the legacy scope once this is deployed. This also updates some functions to use async/await.
There was a problem hiding this comment.
Looks good! We should add tests for:
- a request with one scope cannot be used for a different scope/action
- a request with the same scope has a unique resource ID (proxy url, download filename, etc.) and cannot be used to fetch a different resource with the same scope
- a request with a legacy scope cannot be used to request new-scope resources
A test for the scope upgrade process would be good too.
| textKeysByScope = fetchedTextKeysByScope; | ||
| keysByScope = {}; | ||
| for (const scope in textKeysByScope) { | ||
| keysByScope[scope] = new Keygrip(textKeysByScope[scope]); |
There was a problem hiding this comment.
We should also bump the HMAC algorithm:
new Keygrip(textKeysByScope[scope], 'sha256');
Ah, good point, I'm pretty sure that's not currently correct
The intention is for that to be allowed, with #4884 to remove those keys, since that's the migration strategy we have here.
That's also going to break migration, unless we actually instantiate two Keygrips and do our own fallback checking, and/or wrap Keygrip. I don't think breaking signatures for downloading matters, but we can't break the authentication headers. |
Ah OK, I misunderstood the migration plan.
OK that can be a separate issue for later. |
…or another image Before this change you could take the ?download=...&sig=... from one image and put it on another image URL, causing that other image to be downloaded with the other filename.
…-url-scoped-signed value
|
This should be ready, except for the errors in CircleCI. The server tests pass for me locally. I haven't been able to see what the error is; it's a 500 error in the server, but I'm not seeing the server logs in Artifacts. |
| @@ -13,7 +13,8 @@ exports.createProxyUrl = function(req, url, hash) { | |||
| }; | |||
|
|
|||
| exports.createDownloadUrl = function(url, filename) { | |||
| const sig = dbschema.getKeygrip().sign(new Buffer(filename, "utf8")); | |||
| const path = (new URL(url)).pathname; | |||
There was a problem hiding this comment.
This needs const { URL } = require("url");
In newer Node versions URL is a built-in, but not all versions we're using
|
Just a couple flake8 lint errors now. |
|
Green at last! Will squash and merge... |
This reverts commit f9b4bcb.
This adds signing 'scopes', so that if you get something signed for one scope, you can't use it for another scope. E.g., you can't get a download URL and use that for an authentication key.
This keeps the 'legacy' scope, which is the current single key. This can be used now to make sure everything works when people upgrade, but removed later as people have used to the new specific scopes. But nothing new will be signed with the legacy scope once this is deployed.
This also updates some functions to use async/await.