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

URL Signing #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

URL Signing #1

wants to merge 1 commit into from

Conversation

matthewtapper
Copy link
Collaborator

This PR introduces URL signing to the AWS serverless image handler solution. It has been shamelessly stolen from this PR

You may be asking yourself why we are not using the builtin cloudfront URL signing... Unfortunately, the cloudfront solution does not support our use case:

Signed CloudFront URLs cannot contain extra query string arguments. If you add a query string to a signed URL after you create it, the URL returns an HTTP 403 status.

As a result, the functionality has been added to the lambda function.

It looks like this will be incorporated directly into the AWS supported solution in the not too distant future at which point we can chuck this and use their solution unmodified.

Copy link
Member

@jamestelfer jamestelfer left a comment

Choose a reason for hiding this comment

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

Qualified approval: you're the decision maker here. I advise against going ahead with SHA-1; it should be relatively easy to use a different algorithm.

Comment on lines +658 to +659
code: 'DecodeRequest::CannotReadPath',
message: 'The URL path you provided could not be read. Please ensure that it is properly formed according to the solution documentation.'
Copy link
Member

Choose a reason for hiding this comment

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

My reading of the diff makes this error message surprising. I didn't read the whole thing though.

});
}

const signature = crypto.createHmac("sha1", signKey).update(base64).digest("hex");
Copy link
Member

Choose a reason for hiding this comment

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

SHA1 is broken. I know this is signing, not encryption, but I question this algorithm choice. I'd expect SHA-256 at minimum. What algorithm does AWS use for its S3 URL signing? I doubt it's SHA1.

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.

2 participants