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

fix invalid signature generation caused by unawareness of host prefix #862

Closed
wants to merge 1 commit into from

Conversation

moriyoshi
Copy link

@moriyoshi moriyoshi commented Oct 29, 2020

This patch modifies autogenerated code too and I don't think it's appropriate to merge it as a whole, so please take it as just a reference.

Background of the problem

Because V4 signer doesn't take it into account that the host prefix is propagated outside the regular net/http Request, it fails to build the valid signature string for the services that require a host prefix.

See also

@moriyoshi moriyoshi force-pushed the fix-invalid-signature-errors branch from 63427ef to 65f5b4f Compare October 29, 2020 02:13
@jasdel
Copy link
Contributor

jasdel commented Oct 29, 2020

Thanks for reporting this issue @moriyoshi I created #863 to track this bug.

The request signer intentionally took a standard library http.Request so that it could be more generic. I think an alternative approach that should also be considered is to drop the HostPrefix member of smithyhttp.Request. and instead add the endpoint prefix similar to the SDK's Amazon S3 customization for bucket name and accesspoints.

@moriyoshi
Copy link
Author

That sounds like a cleaner approach. Where do you think the final host name would be resolved then? If the original intention of HostPrefix is to delay the resolution so it'll be done right before the actual request is made, inserting a service-specific middleware that determines and sets the canonical host name to the request passed before the signer should do. However this might result in bloated code to generate.

@jasdel
Copy link
Contributor

jasdel commented Nov 3, 2020

Thanks for taking the time to report this issue and create this PR @moriyoshi. We've updated the SDK's code generation change in aws/smithy-go#233 and relevant Amazon S3 Control API client change in #876. Lets use those PRs for tracking this issue.

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