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

feat(auth): SigV4 Signing for calls that are taking IAM Auth #2435

Open
wants to merge 23 commits into
base: develop
Choose a base branch
from

Conversation

stephenbawks
Copy link
Contributor

@stephenbawks stephenbawks commented Jun 11, 2023

Issue number: #2493

Summary

Changes

Adding the ability to create authentication using SigV4.

User experience

Please share what the user experience looks like before and after this change

Checklist

If your change doesn't seem to apply, please leave them unchecked.

Is this a breaking change?

RFC issue number:

Checklist:

  • Migration process documented
  • Implement warnings (if it can live side by side)

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

@stephenbawks stephenbawks requested a review from a team as a code owner June 11, 2023 20:39
@stephenbawks stephenbawks requested review from heitorlessa and removed request for a team June 11, 2023 20:39
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 11, 2023
@stephenbawks
Copy link
Contributor Author

stephenbawks commented Jun 11, 2023

Work still progressing on this, but interested in early feedback.

@rubenfonseca
Copy link
Contributor

Hi @stephenbawks, thank you so much for your effort in creating this!

I wanted to let you know that in order to add a new feature, we have a specific process that requires opening a feature request (possibly with a Request for Comments or RFC)[https://github.com/awslabs/aws-lambda-powertools-python/issues/new?assignees=&labels=RFC%2Ctriage&projects=&template=rfc.yml&title=RFC%3A+TITLE] before we consider implementing a new utility.

Additionally, to ensure better visibility, it would be really helpful if you could answer the following questions:

1/ What are the differences or gaps between SigV4Auth and the well-tested utility available at https://github.com/davidmuller/aws-requests-auth?
2/ Can we find this utility in the Lambda Runtime's boto version (1.29.90)?
3/ Is it possible to make the utility name more general, allowing for future growth? For example, JWT is another popular requirement.

I kindly request you to open a Feature Request/RFC with these answers, so that we can gather feedback on the proposed utility! Once again, I sincerely appreciate your valuable time :)

@rubenfonseca rubenfonseca added help wanted Could use a second pair of eyes/hands do-not-merge need-issue PRs that are missing related issues need-rfc labels Jun 13, 2023
@stephenbawks stephenbawks changed the title Sigv4 AWS Signed Request Jun 17, 2023
@rubenfonseca rubenfonseca removed the need-issue PRs that are missing related issues label Jun 28, 2023
@heitorlessa
Copy link
Contributor

Folks - what's the latest here?

@heitorlessa heitorlessa changed the title AWS Signed Request feet(auth) support to sign aws sdk calls with sigv4 Jul 7, 2023
@stephenbawks stephenbawks mentioned this pull request Jul 7, 2023
2 tasks
@stephenbawks
Copy link
Contributor Author

Hi @stephenbawks, thank you so much for your effort in creating this!

I wanted to let you know that in order to add a new feature, we have a specific process that requires opening a feature request (possibly with a Request for Comments or RFC)[https://github.com/awslabs/aws-lambda-powertools-python/issues/new?assignees=&labels=RFC%2Ctriage&projects=&template=rfc.yml&title=RFC%3A+TITLE] before we consider implementing a new utility.

Additionally, to ensure better visibility, it would be really helpful if you could answer the following questions:

1/ What are the differences or gaps between SigV4Auth and the well-tested utility available at https://github.com/davidmuller/aws-requests-auth? 2/ Can we find this utility in the Lambda Runtime's boto version (1.29.90)? 3/ Is it possible to make the utility name more general, allowing for future growth? For example, JWT is another popular requirement.

I kindly request you to open a Feature Request/RFC with these answers, so that we can gather feedback on the proposed utility! Once again, I sincerely appreciate your valuable time :)

Sorry for the delay but I have gone ahead and created the RFC.

#2713

@heitorlessa
Copy link
Contributor

Thanks a lot @stephenbawks !! Gonna make comments in the RFC shortly

@heitorlessa
Copy link
Contributor

Quick heads up it's missing details and CI checks still. I'll put this for August

@sonarcloud
Copy link

sonarcloud bot commented Aug 18, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 15 Code Smells

No Coverage information No Coverage information
0.5% 0.5% Duplication

@stephenbawks
Copy link
Contributor Author

@leandrodamascena @heitorlessa @rubenfonseca

This is not feature complete yet, but I wanted some feedback. I added some stuff for JWT auth (not 100% done yet) but I wanted to get thoughts and opinions on the basic structure.

The trick with JWT Auth is that there are a lot of different providers out there and there are slight differences between them in terms of what they want or need to do JWT Client Credentials for example. I added some of the major players as an Enum but I am looking to see what you all think if that is a good idea or not. The basic structure is there but like I said, needs some polishing and error handling but wanted to see if I should proceed down this route or change direction.

@leandrodamascena
Copy link
Contributor

We are currently prioritizing RFC #3040 and after completing this work we return to this PR.

@leandrodamascena leandrodamascena added the revisit Maintainer to provide update or revisit prioritization in the next cycle label Sep 4, 2023
@sthulb
Copy link
Contributor

sthulb commented Feb 7, 2024

@heitorlessa / @stephenbawks Coming back to this PR, is there anything else needed before we review and merge?

@heitorlessa
Copy link
Contributor

@heitorlessa / @stephenbawks Coming back to this PR, is there anything else needed before we review and merge?

Leandro shared that docs and UX need a final pass -- I'm scheduling this for the next iteration cycle starting on Monday (Feb 12th-22nd) to make the cut for that release cycle.

Copy link

sonarcloud bot commented Feb 9, 2024

Quality Gate Passed Quality Gate passed

Issues
4 New issues

Measures
0 Security Hotspots
No data about Coverage
1.5% Duplication on New Code

See analysis details on SonarCloud

@stephenbawks
Copy link
Contributor Author

stephenbawks commented Feb 9, 2024

@heitorlessa @sthulb

Let's do it. I have been waiting to complete my trifecta of PRs so that I can do more with VPC Lattice so I will do whatever we need here. I saw your comment about a RFC so I will get that going.

I did create an RFC a while back but I am assuming it needs to be updated.

#2713

@sthulb
Copy link
Contributor

sthulb commented Feb 9, 2024

@stephenbawks Thanks for commenting. Initially, I'll say this is missing tests for sure (unit/func/e2e) before we can review this properly.

@sthulb
Copy link
Contributor

sthulb commented Feb 9, 2024

After a deeper look into the PR & RFC, we're missing a few things like docs and tests.

I'm wondering if we care about the JWT side of this PR – perhaps this should be split into a different RFC so we can decide it's something we need. There's definitely a need for SigV4(a) signing util since that would be a nice convenience utility.

@boring-cyborg boring-cyborg bot added the typing Static typing definition related issues (mypy, pyright, etc.) label Mar 28, 2024
Copy link

sonarcloud bot commented Mar 28, 2024

Quality Gate Passed Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
1.5% Duplication on New Code

See analysis details on SonarCloud

@stephenbawks
Copy link
Contributor Author

stephenbawks commented Apr 11, 2024

After a deeper look into the PR & RFC, we're missing a few things like docs and tests.

I'm wondering if we care about the JWT side of this PR – perhaps this should be split into a different RFC so we can decide it's something we need. There's definitely a need for SigV4(a) signing util since that would be a nice convenience utility.

I am getting back into the swing of things here. I would like to get this PR over the finish line, its been a while in the making.

I think we could certainly split it into two different PRs. Just wondering if there is any more thought on that from anyone else. I feel like most of the work is done for adding JWT/client credentials but I am interested in feedback.

@leandrodamascena

@boring-cyborg boring-cyborg bot added the dependencies Pull requests that update a dependency file label May 1, 2024
Copy link

sonarcloud bot commented May 1, 2024

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
1.8% Duplication on New Code

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file feature New feature or functionality help wanted Could use a second pair of eyes/hands need-rfc revisit Maintainer to provide update or revisit prioritization in the next cycle size/L Denotes a PR that changes 100-499 lines, ignoring generated files. typing Static typing definition related issues (mypy, pyright, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: Module to Sign Requests Feature request: Signing AWS Request
5 participants