-
Notifications
You must be signed in to change notification settings - Fork 118
Add gimme-aws-creds assumer #810
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.
8000Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jpts, thanks so much for the contribution! I've taken a look over it and have some initial feedback.
For context: long term, I'd actually like to remove the Assumer
interface entirely. I believe that some of the third-party assumers are only used by one or two Granted users, and it seems like this interface adds complexity to Granted and additional code to maintain.
I'm thinking eventually that we'll replace this with some sort of hook - e.g. allow you to call a bash script which could then trigger gimme-aws-creds
(or any other sort of login tooling).
In the meantime though I'd be happy to accept additional assumer contributions until this change is implemented. Your PR here is good because it doesn't affect any of the core session management code etc.
It might actually be better here if we could add a bash_assumer
that would trigger a bash script - would this work to solve your use case? I'm just thinking that this might be a better step towards my long term goal of using hooks and eliminating the custom Assumer
code altogether. Let me know if this would work for your use case though.
Separately I have a few comments specific to the code - I'll add these inline on your diff!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few questions for you on the implementation!
@chrnorm thanks for the review and context around the direction of the tool. I've left my replies inline. A new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jpts, just the comment on the additional config flag is outstanding and once that is resolved this will be ready to merge!
Thanks @jpts! |
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [common-fate/granted](https://github.com/common-fate/granted) | minor | `v0.36.3` -> `v0.38.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>common-fate/granted (common-fate/granted)</summary> ### [`v0.38.0`](https://github.com/common-fate/granted/releases/tag/v0.38.0) [Compare Source](common-fate/granted@v0.37.0...v0.38.0) #### What's Changed - Bump golang.org/x/crypto from 0.25.0 to 0.31.0 by [@​dependabot](https://github.com/dependabot) in common-fate/granted#815 - Add gimme-aws-creds assumer by [@​jpts](https://github.com/jpts) in common-fate/granted#810 - Bump golang.org/x/net from 0.26.0 to 0.33.0 by [@​dependabot](https://github.com/dependabot) in common-fate/granted#824 - Make leading/trailing spaces optional in templating by [@​agershman](https://github.com/agershman) in common-fate/granted#792 - Fix golangci-lint CI failure by [@​jpts](https://github.com/jpts) in common-fate/granted#832 - Update service_map.go by [@​wayne-folkes](https://github.com/wayne-folkes) in common-fate/granted#829 - Bump github.com/go-jose/go-jose/v4 from 4.0.2 to 4.0.5 by [@​dependabot](https://github.com/dependabot) in common-fate/granted#831 - GHA: upgrade upload-artifact version by [@​chrnorm](https://github.com/chrnorm) in common-fate/granted#833 #### New Contributors - [@​agershman](https://github.com/agershman) made their first contribution in common-fate/granted#792 **Full Changelog**: common-fate/granted@v0.37.0...v0.38.0 ### [`v0.37.0`](https://github.com/common-fate/granted/releases/tag/v0.37.0) [Compare Source](common-fate/granted@v0.36.3...v0.37.0) #### Changelog - [`91e5e76`](common-fate/granted@91e5e76) Josh/cf 3767 add cli flag to attach jira ticket in assume ([#​808](common-fate/granted#808)) - [`e047104`](common-fate/granted@e047104) Service flag support for AWS Backup ([#​804](common-fate/granted#804)) Download this release by following our [getting started guide](https://docs.commonfate.io/granted/getting-started). </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOS4xODIuMyIsInVwZGF0ZWRJblZlciI6IjM5LjE4Mi4zIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJSZW5vdmF0ZSBCb3QiXX0=-->
I'm missing some documentation, or tutorial on how to use this, right? |
What changed?
Add support for the gimme-aws-creds helper as an assumer, which supports AWS accounts integrated with Okta directly via SAML. The program supports multiple versions of Okta, MFA configurations etc. so we don't have to reimplement any of that logic.
On top of the basic support, it has the following features:
Why?
Allows us to get credentials via Okta/SAML, which wasn't previously supported.
How did you test it?
granted compiled with
go build -ldflags='-w -s -X github.com/common-fate/granted/internal/build.ConfigFolderName=.granted' ./cmd/granted
Tested over the last few months on multiple AWS accounts/orgs, including a mixed config with Identity Centre and other credential providers.
Potential risks
None
Is patch release candidate?
Yes, no breaking changes
Link to relevant docs PRs