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

AWS sigv4 auth filter #2911

Open
szuecs opened this issue Feb 7, 2024 · 7 comments
Open

AWS sigv4 auth filter #2911

szuecs opened this issue Feb 7, 2024 · 7 comments
Labels
backlog maintainers backlog to use in planning enhancement

Comments

@szuecs
Copy link
Member

szuecs commented Feb 7, 2024

Sometimes you want to proxy to an aws service and aws uses sigv4 to do authnz.
It would be great to be able to sign with sigv4 the request with https://pkg.go.dev/github.com/aws/aws-sdk-go-v2/aws/signer/v4#HTTPSigner to be able to call AWS services from the proxy itself.

Maybe leverage a similar kind of roundtripper which depends also on aws sdk https://github.com/prometheus/common/blob/main/sigv4/sigv4.go

@Anurag252
Copy link

Hey @szuecs
I can take a look at this one. My thoughts around this

  • Create a new request filter named AWSSignV4
  • add a dependency to aws-sdk-go-v2 and call Sign.
  • Allow createfilter to accept credentials, take service and region from eskip file, and keep signtime as now documentation.

I believe we need to consider that

  • An instance of skipper would be able to sign requests for a single AWS account , but may target different services as credentials are being passed to create filter. Maybe there are other ways to generalize the credentials.

  • SignHTTP accepts payloadHash which is essentially hex encoded SHA-256 hash of the request payload. This would mean request body would need to be read completely in the filter and then a new ReadCloser would need to be assigned to request so that it can be propagated further. This could increase memory usage of skipper significantly. Although as docs says there are a few AWS services that do not need body to be signed.

  • Since request is signed here, it makes sense if this is the last filter before proxy as any other filter changing headers, query params or body may invalidate the signing.

@szuecs
Copy link
Member Author

szuecs commented Feb 21, 2024

@Anurag252 I am not sure if I want to have the AWS sdk as dependency, rather not. I think it seems to be open enough to build this without AWS SDK, but I am not sure.

@Anurag252
Copy link

Anurag252 commented Feb 21, 2024

@Anurag252 I am not sure if I want to have the AWS sdk as dependency, rather not. I think it seems to be open enough to build this without AWS SDK, but I am not sure.

@szuecs makes sense to me. I can try and implement this . What are your thoughts around reading the whole body in filter ( as described in point 2 of considerations) ?

@szuecs
Copy link
Member Author

szuecs commented Feb 21, 2024

@Anurag252 I am not sure if I want to have the AWS sdk as dependency, rather not. I think it seems to be open enough to build this without AWS SDK, but I am not sure.

@szuecs makes sense to me. I can try and implement this . What are your thoughts around reading the whole body in filter ( as described in point 2 of considerations) ?

Sounds like we have to do it. Not sure if it makes sense to have 2 kind of filters, 1 that requires body and the other which does not.

Similar to https://opensource.zalando.com/skipper/reference/filters/#opaauthorizerequest and https://opensource.zalando.com/skipper/reference/filters/#opaauthorizerequestwithbody

What do you think?

@Anurag252
Copy link

@Anurag252 I am not sure if I want to have the AWS sdk as dependency, rather not. I think it seems to be open enough to build this without AWS SDK, but I am not sure.

@szuecs makes sense to me. I can try and implement this . What are your thoughts around reading the whole body in filter ( as described in point 2 of considerations) ?

Sounds like we have to do it. Not sure if it makes sense to have 2 kind of filters, 1 that requires body and the other which does not.

Similar to https://opensource.zalando.com/skipper/reference/filters/#opaauthorizerequest and https://opensource.zalando.com/skipper/reference/filters/#opaauthorizerequestwithbody

What do you think?

Okay then I can probably take a param in CreateFilter which would denote the maximum body size (like opaAuthorizeRequestWithBody ) that could be read and keep default as 8kb (? or any other size) .

I could not think of a reason when having two filters would be better, but I maybe missing out some case 🤔 .
Even with no body present documentation mentions including empty body in signature generation like so
Hex(SHA256Hash(""))

@szuecs
Copy link
Member Author

szuecs commented Feb 22, 2024

Okay then I can probably take a param in CreateFilter which would denote the maximum body size (like opaAuthorizeRequestWithBody ) that could be read and keep default as 8kb (? or any other size) .

I could not think of a reason when having two filters would be better, but I maybe missing out some case 🤔 . Even with no body present documentation mentions including empty body in signature generation like so Hex(SHA256Hash(""))

Then I would ignore it and handle it inside the filter.
For example you can use ContentLength to detect a request without body.
So basically:

var data string
if ContentLength != 0 {
   data = ...read body and make it string..
}
..Hex(SHA256Hash(data)) ..

@Anurag252
Copy link

Okay then I can probably take a param in CreateFilter which would denote the maximum body size (like opaAuthorizeRequestWithBody ) that could be read and keep default as 8kb (? or any other size) .
I could not think of a reason when having two filters would be better, but I maybe missing out some case 🤔 . Even with no body present documentation mentions including empty body in signature generation like so Hex(SHA256Hash(""))

Then I would ignore it and handle it inside the filter. For example you can use ContentLength to detect a request without body. So basically:

var data string
if ContentLength != 0 {
   data = ...read body and make it string..
}
..Hex(SHA256Hash(data)) ..

makes sense. I will try to submit a PR 👍

@szuecs szuecs added the backlog maintainers backlog to use in planning label Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog maintainers backlog to use in planning enhancement
Projects
None yet
Development

No branches or pull requests

2 participants