chore: update azure/kubelogin to address CVE #20578

imjasonh · 2024-10-29T16:59:18Z

This PR attempts to update the project's dependency on github.com/Azure/kubelogin to the most recent version, which allows the project to update its dependency on github.com/Azure/azure-sdk-for-go/sdk/azidentity to a version that addresses a medium-severity CVE: GHSA-m5vv-6r4h-3vj9

I don't know whether this project is actually susceptible to this vulnerability, but it gets flagged for users of this project, and in general depending on very old releases of packages is not recommended -- this updates kubelogin from v0.0.20 (Aug 2022) to v0.1.4 (July this year)

The API has changed in breaking ways in the intervening ~2 years, and I assume this is part of the reason this old dependency has stuck around.

This is in part an update to #16661 -- I'm not sure if that PR should just be updated or if that's in some permanently stuck state.

Checklist:

bunnyshell · 2024-10-29T16:59:23Z

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

🚀 /bns:deploy to deploy the environment

imjasonh · 2024-10-29T17:02:08Z

build-only CI failure seems to have failed flakily

ERROR: failed to solve: docker.io/library/node:23.0.0@sha256:e643c0b70dca9704dff42e12b17f5b719dbe4f95e6392fc2dfa0c5f02ea8044d: failed to resolve source metadata for docker.io/library/node:23.0.0@sha256:e643c0b70dca9704dff42e12b17f5b719dbe4f95e6392fc2dfa0c5f02ea8044d: failed to authorize: failed to fetch oauth token: unexpected status from POST request to https://auth.docker.io/token: 502 Bad Gateway

blakepettersson · 2024-11-01T07:38:01Z

@imjasonh the unit test failure is a real one, which I can consistently reproduce.

What worked for me was reverting some of the unrelated changes (we would still need to find the actual reason for the failure).

blakepettersson

I added some changes to go.mod which makes the failing test pass (basically anything that's not directly related to kubelogin is reverted to its prior state for now), you'll then need to run go mod tidy afterwards.

go.mod

imjasonh · 2024-11-01T14:01:38Z

Thanks @blakepettersson, I've updated the deps and ran go mod tidy, let me know if I missed anything.

codecov · 2024-11-01T14:57:41Z

Codecov Report

Attention: Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.

Project coverage is 55.18%. Comparing base (36ef563) to head (53f9922).
Report is 392 commits behind head on master.

Files with missing lines	Patch %	Lines
cmd/argocd-k8s-auth/commands/azure.go	0.00%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #20578      +/-   ##
==========================================
+ Coverage   55.10%   55.18%   +0.08%     
==========================================
  Files         324      324              
  Lines       55239    55259      +20     
==========================================
+ Hits        30439    30495      +56     
+ Misses      22180    22145      -35     
+ Partials     2620     2619       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

imjasonh · 2024-11-01T19:05:23Z

Before this change (3625689):

govulncheck -show verbose ./...
Scanning your code and 2003 packages across 270 dependent modules for known vulnerabilities...

Fetching vulnerabilities from the database...

Checking the code against the vulnerabilities...

=== Symbol Results ===

No vulnerabilities found.

=== Package Results ===

Vulnerability #1: GO-2024-2918
    Azure Identity Libraries Elevation of Privilege Vulnerability in
    github.com/Azure/azure-sdk-for-go/sdk/azidentity
  More info: https://pkg.go.dev/vuln/GO-2024-2918
  Module: github.com/Azure/azure-sdk-for-go/sdk/azidentity
    Found in: github.com/Azure/azure-sdk-for-go/sdk/azidentity@v1.1.0
    Fixed in: github.com/Azure/azure-sdk-for-go/sdk/azidentity@v1.6.0

After this change (11ce485):

govulncheck -show verbose ./...
Scanning your code and 2015 packages across 269 dependent modules for known vulnerabilities...

Fetching vulnerabilities from the database...

Checking the code against the vulnerabilities...

=== Symbol Results ===

No vulnerabilities found.

=== Package Results ===

No other vulnerabilities found.

blakepettersson · 2024-11-01T20:09:25Z

@imjasonh I'm no Azure expert - how has this been verified that this still works?

imjasonh · 2024-11-01T20:13:11Z

@imjasonh I'm no Azure expert - how has this been verified that this still works?

I haven't verified anything at all. I'm just basing this on the previous PR from the feature's author to get the dep updated.

If you have any simple instructions to test this, or an end-to-end test that I could run, I'd be happy to give it a shot.

imjasonh · 2024-11-01T20:13:43Z

@bcho is the previous PR's author, 11 months ago

bcho

LGTM

blakepettersson · 2024-11-04T10:36:38Z

If you have any simple instructions to test this, or an end-to-end test that I could run, I'd be happy to give it a shot.

Sadly I do not know of any. I guess given that @bcho is one of the actual authors of kubelogin and works for Azure Cloud that it gives some reassurance that this should work. I'd pass your question along to bcho.

Signed-off-by: Jason Hall <jason@chainguard.dev>

imjasonh · 2024-11-05T03:02:07Z

Rebased and updated go mod tidy to resolve merge conflict.

Signed-off-by: Jason Hall <jason@chainguard.dev>

bcho · 2024-11-05T05:22:21Z

Hi, thanks for the tag and the fix! - i am the contributor of kubelogin. I will try verify the change with an AKS cluster w/ entra id auth enabled tomorrow and report back here.

bcho · 2024-11-06T18:58:10Z

cmd/argocd-k8s-auth/commands/azure.go

-			o.UpdateFromEnv()
+			o := token.OptionsWithEnv()
+			// we'll use default of WorkloadIdentityLogin for the login flow
+			o.LoginMethod = token.WorkloadIdentityLogin


This backfill breaks the behavior. The previous code is to set the default value for LoginMethod to "WorkloadIdentityLogin", then override with environment variable when "AAD_LOGIN_METHOD" is set. But the new implementation here is to force the LoginMethod to use only WorkloadIdentity.

I think a proper way to do that is like this:

o := token.OptionsWithEnv() if o.LoginMethod == "" { // no environment variable overrides // we'll use default of WorkloadIdentityLogin for the login flow o.LoginMethod = token.WorkloadIdentityLogin }

Thanks! I've applied this suggestion, please take a look.

bcho · 2024-11-06T19:02:03Z

I have verified above code from a test env against an AAD + workload identity enabled AKS cluster. It worked after applying above changes for updating the backfill behavior for both devicelogin / msi / workload identity login method.

Signed-off-by: Jason Hall <jason@chainguard.dev>

blakepettersson

Thanks for your review, @bcho! LGTM

bcho

LGTM

imjasonh · 2024-11-08T15:08:06Z

Thanks @blakepettersson and @bcho!

It looks like this still needs approval from an owner with write access. To avoid needing to keep this up to date (as go.mod is a fast-changing file), is there someone we could poke to get this merged soon? 🙏

crenshaw-dev · 2024-11-08T15:35:18Z

Since it's only medium severity, I don't plan to backport the fix. It'll go out with 2.14. Thanks all!

* update azure/kubelogin to address CVE Signed-off-by: Jason Hall <jason@chainguard.dev> * actually emit token Signed-off-by: Jason Hall <jason@chainguard.dev> * update deps, go mod tidy Signed-off-by: Jason Hall <jason@chainguard.dev> * fix go.sum Signed-off-by: Jason Hall <jason@chainguard.dev> * bcho's suggestion Signed-off-by: Jason Hall <jason@chainguard.dev> --------- Signed-off-by: Jason Hall <jason@chainguard.dev> Signed-off-by: Adrian Aneci <aneci@adobe.com>

imjasonh requested a review from a team as a code owner October 29, 2024 16:59

hectorj2f approved these changes Oct 29, 2024

View reviewed changes

blakepettersson reviewed Nov 1, 2024

View reviewed changes

bcho approved these changes Nov 1, 2024

View reviewed changes

imjasonh added 3 commits November 4, 2024 22:01

update azure/kubelogin to address CVE

6a7c694

Signed-off-by: Jason Hall <jason@chainguard.dev>

actually emit token

584d578

Signed-off-by: Jason Hall <jason@chainguard.dev>

update deps, go mod tidy

2a6f80e

Signed-off-by: Jason Hall <jason@chainguard.dev>

imjasonh force-pushed the aztoken branch from 11ce485 to 2a6f80e Compare November 5, 2024 03:01

fix go.sum

d66e0a5

Signed-off-by: Jason Hall <jason@chainguard.dev>

This was referenced Nov 5, 2024

feat: update azure/kubelogin to v0.1.0 #16661

Closed

chore(deps): bump github.com/Azure/azure-sdk-for-go/sdk/azidentity from 1.1.0 to 1.6.0 #18843

Closed

bcho reviewed Nov 6, 2024

View reviewed changes

bcho's suggestion

53f9922

Signed-off-by: Jason Hall <jason@chainguard.dev>

blakepettersson added the ready-for-review An approver should give a final review and merge the PR label Nov 7, 2024

blakepettersson approved these changes Nov 7, 2024

View reviewed changes

bcho approved these changes Nov 7, 2024

View reviewed changes

rumstead approved these changes Nov 8, 2024

View reviewed changes

crenshaw-dev approved these changes Nov 8, 2024

View reviewed changes

crenshaw-dev merged commit 4776e48 into argoproj:master Nov 8, 2024
32 checks passed

blakepettersson mentioned this pull request Nov 8, 2024

fix: Update Azure identity services (1.1.1 -> 1.11.1) and pygments (2.15.0 -> 2.15.1) #19576

Closed

14 tasks

chore: update azure/kubelogin to address CVE #20578

chore: update azure/kubelogin to address CVE #20578

Uh oh!

Conversation

imjasonh commented Oct 29, 2024

Uh oh!

bunnyshell bot commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ Preview Environment deleted from Bunnyshell

Uh oh!

imjasonh commented Oct 29, 2024

Uh oh!

blakepettersson commented Nov 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blakepettersson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

imjasonh commented Nov 1, 2024

Uh oh!

codecov bot commented Nov 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

imjasonh commented Nov 1, 2024

Uh oh!

blakepettersson commented Nov 1, 2024

Uh oh!

imjasonh commented Nov 1, 2024

Uh oh!

imjasonh commented Nov 1, 2024

Uh oh!

bcho left a comment

Choose a reason for hiding this comment

Uh oh!

blakepettersson commented Nov 4, 2024

Uh oh!

imjasonh commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bcho commented Nov 5, 2024

Uh oh!

bcho Nov 6, 2024

Choose a reason for hiding this comment

Uh oh!

imjasonh Nov 6, 2024

Choose a reason for hiding this comment

Uh oh!

bcho commented Nov 6, 2024

Uh oh!

blakepettersson left a comment

Choose a reason for hiding this comment

Uh oh!

bcho left a comment

Choose a reason for hiding this comment

Uh oh!

imjasonh commented Nov 8, 2024

Uh oh!

Uh oh!

crenshaw-dev commented Nov 8, 2024

Uh oh!

Uh oh!

bunnyshell bot commented Oct 29, 2024 •

edited

Loading

blakepettersson commented Nov 1, 2024 •

edited

Loading

codecov bot commented Nov 1, 2024 •

edited

Loading

imjasonh commented Nov 5, 2024 •

edited

Loading