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

Signature V4 Signing for AWS API Gateway Endpoints #3193

Closed
cogwirrel opened this issue Feb 25, 2021 · 8 comments · Fixed by #3210
Closed

Signature V4 Signing for AWS API Gateway Endpoints #3193

cogwirrel opened this issue Feb 25, 2021 · 8 comments · Fixed by #3210

Comments

@cogwirrel
Copy link
Contributor

cogwirrel commented Feb 25, 2021

Feature Request

It would be really useful to be able to define services for bundles or decision logs that are fronted by API Gateway with IAM auth.

At the moment, the sigv4 signer is hardcoded to use the service s3. It would be great if the signing service could be specified in the service configuration, defaulting to s3 to preserve backwards compatibility.

@anderseknert
Copy link
Member

Thanks @cogwirrel! Seems like a useful addition.

@anderseknert
Copy link
Member

@cogwirrel so if I understand you correctly, all that's needed here is to make the "s3" path component in /s3/aws4_request configurable? Feel free to submit a PR, or I can probably add this if you'd rather want that.

@cogwirrel
Copy link
Contributor Author

@anderseknert yes that's right, I think everything else with the signing should be fine :) it'd need to be the 3 occurences of "s3" in that method.

If it's easy for you to add this that'd be great, but if you're low on time I should be able to get some time next week to send a PR (will need to learn a little Go too haha!)

@anderseknert
Copy link
Member

Of course you should contribute the fix then! I too started my Go journey by contributing to this project :)
Should be fairly simple:

  1. Introduce service name as config option (with s3 as default).
  2. Replace hardcoded reference to service with above config option.
  3. A test to verify that configured service name is used in signature.

I'd be happy to help you along the way. See the contributing section of the README for how to best get started. And join the OPA Slack if you haven't already :)

@cogwirrel
Copy link
Contributor Author

Sounds good, thanks! I'll let you know if I get stuck :)

@cogwirrel
Copy link
Contributor Author

@anderseknert Would you be opposed to me sending a PR which uses the AWS SDK for Go to sign requests?

From some local testing, I discovered the existing signing algorithm doesn't work for API Gateway, and there are quite a few nuances with signing for the different AWS services or the different request headers used, so to save reimplementing all that complexity we can just delegate to the SDK's signer :).

I did find some objections in an older issue here: #1600

I'm not super familiar with the Go ecosystem so I'm not sure how big an issue the mentioned "version conflicts" are, but it feels like the most robust and maintainable option would be to use the authoritative library for SigV4 signing :)

@anderseknert
Copy link
Member

Yeah, since so many use OPA as a library, introducing new dependencies potentially introduces conflicting versions. For a library as common as the AWS SDK this is probably even likely to happen. So if we can avoid it we should. Is there any docs on how the signature is built when sending requests to the API Gateway as opposed to S3?

@cogwirrel
Copy link
Contributor Author

Ok all good, yeah there are some apigateway specific docs, and I can use the SDK's implementation as a reference. Should have a PR for you shortly :)

cogwirrel added a commit to cogwirrel/opa that referenced this issue Mar 9, 2021
This adds a new `service` option to the `s3_signing` config, allowing for other AWS services (such
as API Gateway endpoints) to be used for bundles, decision logs etc.

For example:

```
services:
  decision-log-service:
    url: https://myrestapi.execute-api.ap-southeast-2.amazonaws.com/prod/
    credentials:
      s3_signing:
        service: execute-api
        environment_credentials: {}

decision_logs:
  service: decision-log-service
  reporting:
    min_delay_seconds: 300
    max_delay_seconds: 600
```

If no service is specified, we default to `s3` to maintain backwards compatibility.

This updates the sigv4 signer to include the specified service in the signature, and to sign all
request headers for better compatibility with other AWS services, except an explicit ignore list,
as per https://github.com/aws/aws-sdk-go/blob/master/aws/signer/v4/v4.go#L92

Additionally, this fixes a bug in the signer where the body ReadCloser was consumed and not reset,
meaning requests that were signed were always sent with an empty body!

Fixes open-policy-agent#3193

Signed-off-by: Jack Stevenson <jacsteve@amazon.com>
srenatus pushed a commit that referenced this issue Mar 10, 2021
This adds a new `service` option to the `s3_signing` config, allowing for other AWS services (such
as API Gateway endpoints) to be used for bundles, decision logs etc.

For example:

```
services:
  decision-log-service:
    url: https://myrestapi.execute-api.ap-southeast-2.amazonaws.com/prod/
    credentials:
      s3_signing:
        service: execute-api
        environment_credentials: {}

decision_logs:
  service: decision-log-service
  reporting:
    min_delay_seconds: 300
    max_delay_seconds: 600
```

If no service is specified, we default to `s3` to maintain backwards compatibility.

This updates the sigv4 signer to include the specified service in the signature, and to sign all
request headers for better compatibility with other AWS services, except an explicit ignore list,
as per https://github.com/aws/aws-sdk-go/blob/master/aws/signer/v4/v4.go#L92

Additionally, this fixes a bug in the signer where the body ReadCloser was consumed and not reset,
meaning requests that were signed were always sent with an empty body!

Fixes #3193

Signed-off-by: Jack Stevenson <jacsteve@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants