feat(cmd/auth) New command to support Workload Identity on GKE #288

MickaelFontes · 2023-05-22T21:22:22Z

To add support of workload identity to authenticate to a Artifact Registry.

In this PR, I chose to add a auth gcp command, which is a mix of both existing basic and oauth commands.
Some context:

to authenticate to Artifact Registry, you have to use a short or long-term token as a password
to avoid to use long term tokens (service account keys), GCP allow you to get your pod inside a GKE cluster authenticated to retrieve a short-term oauth2 access token (see Workload Identity)
this is not possible using the auth oauth command (with uses the oauth2/clientcredentials but it can be possible using the oauth2/google : this is the goal of this PR.

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

/kind flaky-test

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area library

/area cli

/area tests

/area examples

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #277

Special notes for your reviewer:

I have not extensively tested it for now (only a few pull), it's the beginning.
I edited the go.mod file not on purpose when adding the imports 😬
I had to make a choice on how to add Workload Identity support for Artifact Registry and decided to go for the auth gcp command, but I don't know if you if it is a good choice or not.
It's my first Go project (hence the previous point I think 😅) so guidance/help and review is appreciated 😄

poiana · 2023-05-22T21:22:31Z

Welcome @MickaelFontes! It looks like this is your first PR to falcosecurity/falcoctl 🎉

leogr · 2023-05-23T08:17:21Z

Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits.

memo Please follow instructions in the contributing guide to update your commits with the DCO

Full details of the Developer Certificate of Origin can be found at developercertificate.org.

The list of commits missing DCO signoff:

eaa6086 feat(registry/auth): Beginning of Workload Identity for falcoctl

Hey @MickaelFontes

Thank you for this PR! Could you sign off your commits, please?
It is required by our policy 🙏

leogr · 2023-05-23T13:36:17Z

/milestone 0.6.0

poiana · 2023-05-23T13:36:19Z

@leogr: The provided milestone is not valid for this repository. Milestones in this repository: [v0.6.0, v0.7.0]

Use /milestone clear to clear the milestone.

In response to this:

/milestone 0.6.0

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

FedeDP · 2023-05-25T07:30:49Z

/milestone v0.6.0

MickaelFontes · 2023-05-25T13:55:33Z

Woops sorry for the forgotten sign off @leogr , it is done now!

Just for the record, this PR might not move for the coming days, since a refactor is needed before the actual add of workload identity support.

loresuso · 2023-05-25T13:58:04Z

Thanks @MickaelFontes ! Don't worry, we will wait for the refactor, but I'm seeing what I was expecting in NewClient() :) seems like it will be a good solution!

Signed-off-by: Mickaël Fontès <[email protected]>

MickaelFontes · 2023-06-23T13:39:28Z

Hi all!
I pushed a first working version for Workload Identity support, and it opened some questions, for which any help is appreciated!
I'll list some points bothering me right now:

using oauth/google, we can easily get a TokenSource that uses Workload Identity but we can get TokenSources using Application Default credentials or else (see here).
Hence my question about whether I should only use the Workload Identity Token source directly, or let the google.DefaultTokenSource choose among the available sources 🤔
(I started with the second approach initially, but the more I think about it, the more I tend to think it was a bad idea - and credential helper support with the refactor done by max makes Application Default credentials less relevant I think)
As @max-frank highlighted, caching the token source would be irrelevant if the only source is Workload Identity/GCE metadata, but what about if it is not and Application Default credentials are supported?
one point I wanted to mention: the oauth2/google library offers the possibility to get the authenticated HTTP client by simply giving a TokenSource as input (see here - I used that in my PoC)
To match the current Client creation logic, I instead used the TokenSource, assumed to get an access token and used it to perform a login/password authentication, as specified in Google's documentation here. Does this seems ok for you?
last point more about a simple technical question: here I pushed new go.mod/go.sum files since I added a new library, but I don't think I used the proper way to add them 😅. Could anyone tell me if the current state seems rights to him, and show me the proper way to edit them please 🙏 ?

max-frank · 2023-06-26T01:48:02Z

using oauth/google, we can easily get a TokenSource that uses Workload Identity but we can get TokenSources using Application Default credentials or else (see here).
Hence my question about whether I should only use the Workload Identity Token source directly, or let the google.DefaultTokenSource choose among the available sources 🤔

I think it makes the most sense to use DefaultTokenSource since thats expected behavior for most libraries supporting workload identity.

(I started with the second approach initially, but the more I think about it, the more I tend to think it was a bad idea - and credential helper support with the refactor done by max makes Application Default credentials less relevant I think)

As @max-frank highlighted, caching the token source would be irrelevant if the only source is Workload Identity/GCE metadata, but what about if it is not and Application Default credentials are supported?

With app default credentials (i.e., what. DefaultTokenSource resolves) we also only need 1 single DefaultTokenSource instance for the whole client (since it will always resolve to the same identity). So we can just load the DefaultTokenSource once and then re-use the same instance for all registries using GKE auth.

one point I wanted to mention: the oauth2/google library offers the possibility to get the authenticated HTTP client by simply giving a TokenSource as input (see here - I used that in my PoC)
To match the current Client creation logic, I instead used the TokenSource, assumed to get an access token and used it to perform a login/password authentication, as specified in Google's documentation here. Does this seems ok for you?

Thats exactly the type of application logic I was expecting. This is also exactly how google implemented the logic in their own go-containerregistry lib (see https://github.com/google/go-containerregistry/blob/v0.15.2/pkg/v1/google/auth.go#L49-L61). If we want to add more functionality we could also add support for gcloud as configurable source for dev environments (though you can always run gcloud auth application-default login for dev anyway).

last point more about a simple technical question: here I pushed new go.mod/go.sum files since I added a new library, but I don't think I used the proper way to add them 😅. Could anyone tell me if the current state seems rights to him, and show me the proper way to edit them please 🙏 ?

It looks fine to me as long as you added it with
go get <package> and then after ran go mod tidy (for cleaning the dependency tree) it should be fine. You can also run make fmt and make lint to make sure everything is as the CI/CD pipeline will expect and accepts

* confirm use of GCE and ApplicationDefault token sources * change tokenSource cache logic Signed-off-by: Mickaël Fontès <[email protected]>

Rename from gke to gcp, since it supports out of gke authentication. Signed-off-by: Mickaël Fontès <[email protected]>

alacuku · 2023-06-30T07:31:52Z

Hi @MickaelFontes, could you please look at the linting issue?

Signed-off-by: Mickaël Fontès <[email protected]>

MickaelFontes · 2023-07-03T14:10:30Z

Hi @alacuku it's done! I think everything should be good now.
I've been testing on my side the new command for authentification, no issue found.

What should I do next to ensure everything is correct?

alacuku

Hi @MickaelFontes, nice job!

I left some minor comments!

alacuku · 2023-07-04T08:14:15Z

pkg/oci/authn/gcp.go

+	}
+
+	return auth.Credential{
+		Username: "oauth2accesstoken",


Can we make it a constant and add a comment/link to the docs?

alacuku · 2023-07-04T09:32:01Z

internal/config/config.go

+// strings to string slices, when the target type is DotSeparatedStringList.
+// when passed as env should be in the following format:
+// "registry;registry1".
+func gcpAuthListHookFunc() mapstructure.DecodeHookFuncType {


We need to add the new ENV variable to the README.md: https://github.com/falcosecurity/falcoctl#falcoctl-environment-variables.

cmd/registry/auth/gcp/gcp.go

alacuku · 2023-07-04T09:35:23Z

cmd/registry/auth/gcp/gcp.go

+}
+
+// NewGcpCmd returns the gcp command.
+func NewGcpCmd(ctx context.Context, opt *options.CommonOptions) *cobra.Command {


We need to add the new command to the README.md: https://github.com/falcosecurity/falcoctl#falcoctl-registry.
Feel free to add examples too.

alacuku · 2023-07-04T09:44:19Z

Could you please rephrase the commit messages following this convention: https://github.com/falcosecurity/.github/blob/main/CONTRIBUTING.md#commit-convention?

MickaelFontes · 2023-07-04T15:23:28Z

Thanks for the feedback I'll work on it!
About the commit messages, I thought that it would be okay if the PR is "squashed and merged" and the squashed commit follows the guidelines, right @alacuku ?
Otherwise of course I'll update the messages accordingly.

alacuku · 2023-07-04T15:28:06Z

About the commit messages, I thought that it would be okay if the PR is "squashed and merged" and the squashed commit follows the guidelines, right @alacuku ?

Yeah, squashing the commits it's fine! Thanks!

Co-authored-by: Aldo Lacuku <[email protected]> Signed-off-by: MickaelFontes <[email protected]>

* Update README.md with examples * Update gcp command help * Set registry username as constant for GCP auth Signed-off-by: Mickaël Fontès <[email protected]>

MickaelFontes · 2023-07-05T16:05:08Z

It should be all good now! @alacuku
I added inside the README.md some examples, and in particular two cases I identified as relevant for this new authentication command.

alacuku

/LGTM

poiana · 2023-07-06T07:08:56Z

LGTM label has been added.

Git tree hash: 36c5fc4a69cd9372488651c5e6f53c5f2796db1a

FedeDP

/approve

poiana · 2023-07-06T07:10:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alacuku, FedeDP, MickaelFontes

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [FedeDP]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

MickaelFontes · 2023-07-06T15:47:53Z

Hey @alacuku! I think the PR was not squashed and merged but simply merged 😬
Should I do something on my side about that?

alacuku · 2023-07-06T16:15:17Z

Unfortunately, we can do nothing. I thought the commits were already squashed, my bad that I did not check.

poiana added kind/feature do-not-merge/work-in-progress dco-signoff: no area/library area/cli labels May 22, 2023

poiana requested review from alacuku and loresuso May 22, 2023 21:22

poiana added the size/L label May 22, 2023

MickaelFontes force-pushed the main branch from eaa6086 to 0bf0ffa Compare May 24, 2023 07:06

poiana added dco-signoff: yes and removed dco-signoff: no labels May 24, 2023

poiana added this to the v0.6.0 milestone May 25, 2023

MickaelFontes closed this Jun 21, 2023

MickaelFontes force-pushed the main branch 2 times, most recently from b6c7819 to 4105621 Compare June 21, 2023 14:44

poiana added size/XS and removed size/L labels Jun 21, 2023

🚧 Command for GKE auth

7e6bc4a

Signed-off-by: Mickaël Fontès <[email protected]>

MickaelFontes reopened this Jun 21, 2023

poiana added size/L and removed size/XS labels Jun 21, 2023

♻️ Change GKE client auth logic

9fd3a3b

* confirm use of GCE and ApplicationDefault token sources * change tokenSource cache logic Signed-off-by: Mickaël Fontès <[email protected]>

MickaelFontes changed the title ~~wip: New command to support Workload Identity on GKE~~ feat(cmd/auth) New command to support Workload Identity on GKE Jun 26, 2023

poiana removed the do-not-merge/work-in-progress label Jun 26, 2023

MickaelFontes mentioned this pull request Jun 26, 2023

Support Workload Identity for oauth login #277

Closed

🚚 Rename new auth command gcp

a9753c5

Rename from gke to gcp, since it supports out of gke authentication. Signed-off-by: Mickaël Fontès <[email protected]>

🚨 Fix linting issues

0ee01b3

Signed-off-by: Mickaël Fontès <[email protected]>

alacuku reviewed Jul 4, 2023

View reviewed changes

MickaelFontes and others added 2 commits July 5, 2023 17:28

📝 Fix wrong command example

ecf6e79

Co-authored-by: Aldo Lacuku <[email protected]> Signed-off-by: MickaelFontes <[email protected]>

📝 Update examples and documentation

c44aa33

* Update README.md with examples * Update gcp command help * Set registry username as constant for GCP auth Signed-off-by: Mickaël Fontès <[email protected]>

MickaelFontes force-pushed the main branch from eef8110 to c44aa33 Compare July 5, 2023 15:56

alacuku approved these changes Jul 6, 2023

View reviewed changes

poiana assigned alacuku Jul 6, 2023

poiana added the lgtm label Jul 6, 2023

FedeDP approved these changes Jul 6, 2023

View reviewed changes

poiana assigned FedeDP Jul 6, 2023

poiana added the approved label Jul 6, 2023

poiana merged commit 363a4dc into falcosecurity:main Jul 6, 2023
12 checks passed

alacuku mentioned this pull request Jul 26, 2023

falcoctl registry push: Add support for AWS ECR #302

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cmd/auth) New command to support Workload Identity on GKE #288

feat(cmd/auth) New command to support Workload Identity on GKE #288

MickaelFontes commented May 22, 2023 •

edited

Loading

poiana commented May 22, 2023

leogr commented May 23, 2023

leogr commented May 23, 2023

poiana commented May 23, 2023

FedeDP commented May 25, 2023

MickaelFontes commented May 25, 2023

loresuso commented May 25, 2023

MickaelFontes commented Jun 23, 2023

max-frank commented Jun 26, 2023 •

edited

Loading

alacuku commented Jun 30, 2023

MickaelFontes commented Jul 3, 2023

alacuku left a comment

alacuku Jul 4, 2023

alacuku Jul 4, 2023

alacuku Jul 4, 2023

alacuku commented Jul 4, 2023

MickaelFontes commented Jul 4, 2023

alacuku commented Jul 4, 2023

MickaelFontes commented Jul 5, 2023

alacuku left a comment

poiana commented Jul 6, 2023

FedeDP left a comment

poiana commented Jul 6, 2023

MickaelFontes commented Jul 6, 2023

alacuku commented Jul 6, 2023

feat(cmd/auth) New command to support Workload Identity on GKE #288

feat(cmd/auth) New command to support Workload Identity on GKE #288

Conversation

MickaelFontes commented May 22, 2023 • edited Loading

poiana commented May 22, 2023

leogr commented May 23, 2023

leogr commented May 23, 2023

poiana commented May 23, 2023

FedeDP commented May 25, 2023

MickaelFontes commented May 25, 2023

loresuso commented May 25, 2023

MickaelFontes commented Jun 23, 2023

max-frank commented Jun 26, 2023 • edited Loading

alacuku commented Jun 30, 2023

MickaelFontes commented Jul 3, 2023

alacuku left a comment

Choose a reason for hiding this comment

alacuku Jul 4, 2023

Choose a reason for hiding this comment

alacuku Jul 4, 2023

Choose a reason for hiding this comment

alacuku Jul 4, 2023

Choose a reason for hiding this comment

alacuku commented Jul 4, 2023

MickaelFontes commented Jul 4, 2023

alacuku commented Jul 4, 2023

MickaelFontes commented Jul 5, 2023

alacuku left a comment

Choose a reason for hiding this comment

poiana commented Jul 6, 2023

FedeDP left a comment

Choose a reason for hiding this comment

poiana commented Jul 6, 2023

MickaelFontes commented Jul 6, 2023

alacuku commented Jul 6, 2023

MickaelFontes commented May 22, 2023 •

edited

Loading

max-frank commented Jun 26, 2023 •

edited

Loading