fix(server): JWT token checks (#14930)

[GeorgiYosifov]

Fixes: #14930

OIDC, Dex and Admin token revocation is done correctly.
API and CLI entry points are corrected.
Unit tests are added for each implementation change.

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• The title of the PR states what changed and the related issues number (used for the release note).
• The title of the PR conforms to the Toolchain Guide
• I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Does this PR require documentation updates?
• I've updated documentation as required by this PR.
• Optional. My organization is added to USERS.md.
• I have signed off all my commits as required by DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My build is green (troubleshooting builds).
• My new feature complies with the feature status guidelines.
• I have added a brief description of why this PR is necessary and/or what this PR solves.

Please see Contribution FAQs if you have questions about your pull-request.

[crenshaw-dev]

@GeorgiYosifov would you mind adding unit tests?

[GeorgiYosifov]

Ok, I will add unit tests but I encounter that there is also problem with token revocation when I use "argocd cli". I will continue my work on the bug.
By the way fix of DCO check hope that not lead to problem in future to the PR.

[GeorgiYosifov]

@crenshaw-dev Can you review the PR at this state, before I add unit tests and test also with dex configuration manually?

[codecov]

Codecov Report

Attention: Patch coverage is 47.77778% with 47 lines in your changes missing coverage. Please review.

Project coverage is 59.97%. Comparing base (722da4e) to head (fc90a95).

Files with missing lines	Patch %	Lines
cmd/argocd/commands/logout.go	0.00%	42 Missing ⚠️
util/session/sessionmanager.go	78.26%	3 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #15004      +/-   ##
==========================================
- Coverage   60.05%   59.97%   -0.08%     
==========================================
  Files         343      343              
  Lines       57830    57910      +80     
==========================================
+ Hits        34727    34733       +6     
- Misses      20332    20406      +74     
  Partials     2771     2771              

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

[GeorgiYosifov]

Unit tests are added, still remains to test it with Dex configured.

[GeorgiYosifov]

Hi @andrii-korotkov-verkada, sorry for the late response. Would you proceed with the review and merge this PR?

[andrii-korotkov-verkada]

Approved, but I don't have permissions to merge

[agaudreault]

@GeorgiYosifov Let me know when the unit tests are fixed so I can merge

[agaudreault]

Why removing this unit test?

It seems like the new unint tests are not validating assert.ErrorIs(t, err, common.ErrTokenVerification)

[GeorgiYosifov]

The unit test is shifted below in the same file:

argo-cd/util/session/sessionmanager_test.go

Lines 712 to 744 in 7a4e873

	t.Run("OIDC provider is Dex, TLS is configured", func(t *testing.T) {
	config := map[string]string{
	"url": dexTestServer.URL,
	"dex.config": `connectors:
	- type: github
	name: GitHub
	config:
	clientID: aabbccddeeff00112233
	clientSecret: aabbccddeeff00112233`,
	}
	// This is not actually used in the test. The test only calls the OIDC test server. But a valid cert/key pair
	// must be set to test VerifyToken's behavior when Argo CD is configured with TLS enabled.
	secretConfig := map[string][]byte{
	"tls.crt": utiltest.Cert,
	"tls.key": utiltest.PrivateKey,
	}
	settingsMgr := settings.NewSettingsManager(t.Context(), getKubeClientWithConfig(config, secretConfig), "argocd")
	mgr := NewSessionManager(settingsMgr, getProjLister(), dexTestServer.URL, &dex.DexTLSConfig{StrictValidation: false}, NewUserStateStorage(nil))
	mgr.verificationDelayNoiseEnabled = false
	claims := jwt.RegisteredClaims{Audience: jwt.ClaimStrings{"argo-cd"}, Subject: "admin", ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Hour * 24))}
	claims.Issuer = dexTestServer.URL + "/api/dex"
	token := jwt.NewWithClaims(jwt.SigningMethodRS512, claims)
	key, err := jwt.ParseRSAPrivateKeyFromPEM(utiltest.PrivateKey)
	require.NoError(t, err)
	tokenString, err := token.SignedString(key)
	require.NoError(t, err)
	_, _, err = mgr.VerifyToken(tokenString)
	require.NoError(t, err)
	})

[GeorgiYosifov]

I see that the file already have tests for error: "ErrTokenVerification". I don't mean to add more tests.

[agaudreault]

@GeorgiYosifov Can you write a comment in code to clearly explain the behavior for token that has no expiry. The admin token is a valid token even if it has no expiry. Project tokens with no expiry are also valid.

I feel like the parse method should not return an error for the project/admin tokens becasue we generate it without expiration. They are valid tokens that do not expire and do not need to be refreshed.

{
  "iss": "argocd",
  "sub": "proj:example:my-role",
  "nbf": 1746656250,
  "iat": 1746656250,
  "jti": "1f80b115-566c-4d6b-ae68-cf21cba47d3b"
}

[GeorgiYosifov]

The old test from: "cmd/argocd/commands/logout_test.go" is removed because the new implementation here: https://github.com/argoproj/argo-cd/blob/e2020559f1984d7b7c844978268368278fa2ff83/cmd/argocd/commands/logout.go
is tested with: https://github.com/argoproj/argo-cd/blob/e2020559f1984d7b7c844978268368278fa2ff83/server/logout/logout_test.go and https://github.com/argoproj/argo-cd/blob/e2020559f1984d7b7c844978268368278fa2ff83/util/localconfig/localconfig_test.go

[lsangelov]

@agaudreault and @crenshaw-dev can this changes be merged as it becomes harder and harder to keep changes up to date just to wait this PR to be merged? Is there something missing in order the PR to be merged?

[anandf]

@GeorgiYosifov I have verified the changes locally by integrating it with the master branch and it works well.
I see that the branch was NOT rebased but instead changes from master branch were merged to your feature branch. This is causing difficulty to rebase with the latest master. I faced lot of conflicts trying to rebase it locally. Would it be ok to resubmit after rebasing with the latest master. After failing to rebase it safely, I took the changes from each file and applied it in my branch. The changes are available here master...anandf:argo-cd:invalidate_jwt_token_on_logout

[anandf]

This http call needs to be made after reconfirming from the user, needs to be moved after checking thecanLogout.

[anandf]

This method will cause a fatal error and cause the program to terminate abruptly. If the remote server is unavailable or errors out due to network issue, it is probably its better to give a warning and logout locally and clean up the local storage.

[anandf]

better to check if the conversion to string was successful using an ok boolean (instead of ignoring). If the key is not found or if the conversion fails, id can be set to empty string probably with a warning log.

[anandf]

@GeorgiYosifov We are looking at speeding up the review and merge process. Please let us know if you can rebase the changes with the latest master branch. Let me know if you need any help.