Skip to content

Implement sigv4a signing in aws_request_signing extension#31336

Merged
mattklein123 merged 42 commits intoenvoyproxy:mainfrom
nbaws:sigv4a_redo
Jan 3, 2024
Merged

Implement sigv4a signing in aws_request_signing extension#31336
mattklein123 merged 42 commits intoenvoyproxy:mainfrom
nbaws:sigv4a_redo

Conversation

@nbaws
Copy link
Copy Markdown
Contributor

@nbaws nbaws commented Dec 13, 2023

Commit Message: Implement sigv4a signing in aws_request_signing extension
Additional Description: See #31000 for proposal
Risk Level: Low
Testing: Unit
Docs Changes: API Proto
Release Notes: N/A
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue] #31000
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

nbaws and others added 7 commits December 12, 2023 03:14
@nbaws
first commit
1a2eff7 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
@nbaws
refactor signer classes
72d2cf5 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
@nbaws
format fix
238bbe0 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
@nbaws
additional formattiong
93ca432 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
@nbaws
additional formattiong
5f89399 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
@nbaws
additional formattiong
8d01fa7 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
@nbaws
Merge branch 'envoyproxy:main' into sigv4a_redo
0bda756
@nbaws nbaws requested a review from mattklein123 as a code owner December 13, 2023 12:03
@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only Bot commented Dec 13, 2023

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @wbpcode
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #31336 was opened by nbaws.

see: more, trace.

@nbaws
Copy link
Copy Markdown
Contributor Author

nbaws commented Dec 13, 2023

/assign @suniltheta

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only Bot commented Dec 13, 2023

nbaws is not allowed to assign users.

🐱

Caused by: a #31336 (comment) was created by @nbaws.

see: more, trace.

@suniltheta
Copy link
Copy Markdown
Contributor

suniltheta commented Dec 13, 2023

/assign @suniltheta

suniltheta
suniltheta reviewed Dec 13, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@suniltheta suniltheta left a comment

Choose a reason for hiding this comment

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

Initial set of review with just skimming the changes.

  1. Missing release notes update on changelogs/current.yaml
  2. Run CI script format is failing with spell check. See if you can fix those by running ./tools/local_fix_format.sh while the diffs are not yet committed.

Comment thread test/extensions/common/aws/metadata_fetcher_test.cc
Comment thread source/extensions/common/aws/signer_base.cc Outdated
Comment thread api/envoy/extensions/filters/http/aws_request_signing/v3/aws_request_signing.proto
Comment thread source/common/crypto/utility.h Outdated
Comment thread source/common/crypto/utility_impl.cc Outdated
@nbaws nbaws changed the title Sigv4a redo Implement sigv4a signing in aws_request_signing extension Dec 15, 2023
nbaws and others added 13 commits December 15, 2023 17:16
@nbaws
Merge branch 'envoyproxy:main' into sigv4a_redo
c0d176e
@nbaws
address feedback
081faa0 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
@nbaws
trailing spaces
1680808 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
@nbaws
format fix
5c5a3f1 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
@nbaws
reverting invalid changes
c68a272 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
@nbaws
uint fix for windows build
61f2edd 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
@nbaws
remove elf dependency
45bfab6 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
@nbaws
spelling
58e4b19 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
@nbaws
spelling
90bf1e3 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
@nbaws
fix changelog
6beecdc 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
@nbaws
change volatile computation
ff9d93a 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
@nbaws
additional volatile fix
a6d8371 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
@nbaws
Merge branch 'envoyproxy:main' into sigv4a_redo
756c08d
nbaws and others added 4 commits December 22, 2023 22:39
@nbaws
Merge branch 'envoyproxy:main' into sigv4a_redo
e52ec0c
@nbaws
address review feedback
3fe007e 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
@nbaws
update to include explicit region_set
a35468b 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
@nbaws
add additional test
040264b 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
@nbaws
Copy link
Copy Markdown
Contributor Author

nbaws commented Dec 24, 2023

Thanks for your great contribution. Some comments are added to the API or code style, but not to the new logic.

I think this require review from @mattklein123 because I have no background knowledge about the aws signing.

thank you for your feedback!

wbpcode
wbpcode reviewed Dec 27, 2023
View reviewed changes
Comment thread api/envoy/extensions/filters/http/aws_request_signing/v3/aws_request_signing.proto Outdated
Comment thread api/envoy/extensions/filters/http/aws_request_signing/v3/aws_request_signing.proto
Comment thread source/extensions/common/aws/signer_base_impl.cc Outdated
nbaws and others added 4 commits December 27, 2023 11:46
@nbaws
revert to previous proto
4becaaf 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
@nbaws
fix tests
d42afcf 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
@nbaws
Merge branch 'envoyproxy:main' into sigv4a_redo
6ddf435
@nbaws
update documentation, revert proto to region required
10277e7 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
suniltheta
suniltheta reviewed Dec 29, 2023
View reviewed changes
Comment thread changelogs/current.yaml Outdated
Comment thread docs/root/configuration/http/http_filters/aws_request_signing_filter.rst Outdated
Comment thread docs/root/configuration/http/http_filters/aws_request_signing_filter.rst
Comment thread source/extensions/filters/http/aws_request_signing/config.cc Outdated
Comment thread source/extensions/filters/http/aws_request_signing/config.cc
Comment thread source/extensions/common/aws/signer_base_impl.h
Comment thread source/extensions/common/aws/sigv4a_key_derivation.cc Outdated
suniltheta
suniltheta reviewed Dec 29, 2023
View reviewed changes
Comment thread source/extensions/common/aws/sigv4_signer_impl.h Outdated
Comment thread docs/root/configuration/http/http_filters/_include/aws-request-signing-filter-sigv4a.yaml Outdated
nbaws added 2 commits December 30, 2023 11:23
@nbaws
address feedback
71936f0 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
@nbaws
todo for lambda filter
9cd6b60 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
wbpcode
wbpcode reviewed Dec 30, 2023
View reviewed changes
Comment thread api/envoy/extensions/filters/http/aws_request_signing/v3/aws_request_signing.proto Outdated
wbpcode
wbpcode reviewed Dec 30, 2023
View reviewed changes
Comment thread api/envoy/extensions/filters/http/aws_request_signing/v3/aws_request_signing.proto Outdated
wbpcode
wbpcode reviewed Dec 30, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@wbpcode wbpcode left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Only two nit comments to the API, then the API looks good to me.

(Thanks for you in timely update, and sorry for my delay.)

@nbaws
address feedback
8540d5e 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
suniltheta
suniltheta reviewed Dec 31, 2023
View reviewed changes
Comment thread source/extensions/filters/http/aws_request_signing/config.cc Outdated
@nbaws
address feedback
b55cc66 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
mattklein123
mattklein123 requested changes Jan 2, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

LGTM with small nit, thanks.

/wait

Comment thread source/extensions/common/aws/BUILD Outdated
Comment thread source/extensions/common/aws/sigv4a_key_derivation.cc
@nbaws
address BUILD feedback
b62dcb0 
Signed-off-by: Nigel Brittain <nbaws@amazon.com>
@wbpcode
Copy link
Copy Markdown
Member

wbpcode commented Jan 3, 2024

/lgtm api

@repokitteh-read-only repokitteh-read-only Bot removed the api label Jan 3, 2024
@mattklein123 mattklein123 merged commit 48ced96 into envoyproxy:main Jan 3, 2024
@nbaws nbaws mentioned this pull request Jun 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wbpcode wbpcode wbpcode left review comments

@mattklein123 mattklein123 mattklein123 approved these changes

+1 more reviewer

@suniltheta suniltheta suniltheta approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mattklein123 mattklein123

@wbpcode wbpcode

@suniltheta suniltheta

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nbaws @suniltheta @wbpcode @mattklein123