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

providers/aws: Refactor + fix 2x Authorization header append issue. #5475

Merged
merged 1 commit into from Dec 12, 2022

Conversation

philipaconrad
Copy link
Contributor

@philipaconrad philipaconrad commented Dec 12, 2022

This commit refactors the shared AWS Sig v4 signing code, specifically to prevent the issue behind #5472. The underlying problem for #5472 was that the "Authorization" header was being appended twice to the request, but only for the AWS REST plugin, because the value was pulled twice from the signed headers map.

This was not caught by the unit tests, because the REST plugin's unit tests all assumed the header was single-valued and canonicalized.

We now test for this exact condition in the unit tests, and the signing code now returns the AWS headers map separately from the value for the "Authorization" header, reducing the potential for this mistake to happen in the future.

Fixes: #5472

This commit refactors the shared AWS Sig v4 signing code, specifically
to prevent the issue behind open-policy-agent#5472. The underlying problem for open-policy-agent#5472 was
that the `"Authorization"` header was being appended *twice* to the
request, but only for the AWS REST plugin, because the value was pulled
twice from the signed headers map.

This was not caught by the unit tests, because the REST plugin's unit
tests all assumed the header was single-valued and canonicalized.

We now explicitly test for that condition in the unit tests, and the
signing code now returns the AWS headers map separately from the value
for the `"Authorization"` header, reducing the potential for this
mistake to happen in the future.

Fixes: open-policy-agent#5472

Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
@philipaconrad
Copy link
Contributor Author

philipaconrad commented Dec 12, 2022

Looks like the macos-latest, 1.17 target is flaking again. Will re-run when the rest finish.

@charlieegan3
Copy link
Contributor

I have tested this with some config that looks like this:

services:
  s3_bundles:
    url: https://MY BUCKET.eu-west-2.amazonaws.com
    credentials:
      s3_signing:
        profile_credentials:
          path: /Users/ME/.aws/credentials

bundles:
  authz:
    service: s3_bundles
    resource: bundle.tar.gz
    polling:
      min_delay_seconds: 10
      max_delay_seconds: 20

(./opa run -s -c config.yaml)

Without these changes, I get the following error repeated in the consule:

{"level":"error","msg":"Bundle load failed: server replied with Not Implemented","name":"authz","plugin":"bundle","time":"2022-12-12T15:20:18Z"}

After building from this branch I no longer see these errors and see that my bundle code is loaded as expected (visible in /v1/policies).

@philipaconrad
Copy link
Contributor Author

Thanks for testing the PR out @charlieegan3! 😃

@philipaconrad philipaconrad merged commit 1d1cb35 into open-policy-agent:main Dec 12, 2022
philipaconrad added a commit to philipaconrad/opa that referenced this pull request Dec 12, 2022
…pen-policy-agent#5475)

This commit refactors the shared AWS Sig v4 signing code, specifically
to prevent the issue behind open-policy-agent#5472. The underlying problem for was
that the `"Authorization"` header was being appended *twice* to the
request, but only for the AWS REST plugin, because the value was pulled
twice from the signed headers map.

This was not caught by the unit tests, because the REST plugin's unit
tests all assumed the header was single-valued and canonicalized.

We now explicitly test for that condition in the unit tests, and the
signing code now returns the AWS headers map separately from the value
for the `"Authorization"` header, reducing the potential for this
mistake to happen in the future.

Fixes: open-policy-agent#5472

Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
(cherry picked from commit 1d1cb35)
philipaconrad added a commit that referenced this pull request Dec 12, 2022
…5475)

This commit refactors the shared AWS Sig v4 signing code, specifically
to prevent the issue behind #5472. The underlying problem for was
that the `"Authorization"` header was being appended *twice* to the
request, but only for the AWS REST plugin, because the value was pulled
twice from the signed headers map.

This was not caught by the unit tests, because the REST plugin's unit
tests all assumed the header was single-valued and canonicalized.

We now explicitly test for that condition in the unit tests, and the
signing code now returns the AWS headers map separately from the value
for the `"Authorization"` header, reducing the potential for this
mistake to happen in the future.

Fixes: #5472

Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
(cherry picked from commit 1d1cb35)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to fetch bundles: S3 returns 501 Not Implemented
2 participants