fix: Adds proper CSRF protection to ArgoCD API

[alexmt]

Closes #2496
Based on #2672 - mostly the same code with some changes mostly in UI ( token handling is done in requests.ts file).

This PR adds proper CSRF protection to the ArgoCD REST API by implementing the Gorilla CSRF framework (https://github.com/gorilla/csrf).

Behaviour for CSRF token validation is as follows:

• All HTTP verbs are protected except requests using GET, HEAD, TRACE and OPTIONS verbs
• Each other request is evaluated by validating the X-Csrf-Token header value sent with the request, except for the conditions mentioned below
• If the client sends the authentication cookie (i.e. argocd.token) with the request, CSRF checks are always performed
• If the client sends an Authorization: Bearer HTTP header and is not including the authorization cookie in the request (i.e. is a stateless REST API client), the CSRF validation is skipped (the Authorization header basically verified the request already)
• If the client does not have the authentication cookie, and no Authorization header, the only POST method allowed is on /api/v1/session to create a session for the client and retrieve the credentials for further communication
• A new switch --disable-csrf is provided for argocd-server command, when set CSRF checks will be disabled completey.

This introduces a new key in argocd-secret for storing the key used to sign CSRF token, named server.csrfkey, which is similar to server.secretkey - that is, will be generated if the key does not exist in the secret on server start, otherwise will persist.

The retrieve a token, the client can issue a GET (or HEAD or OPTIONS if implemented by API) request on an arbitrary REST API endpoint. Preferably, this is /api/v1/session/userinfo. The server will send back a valid token in the X-CSRF-Token header in the response, as long as the user is correctly authenticated. Endpoints that are generally unprotected (currently /api/version and /api/v1/settings) will not send the appropriate header.

[alexmt]

Reviewers, FYI: some questions already answered in #2672

[codecov]

Codecov Report

Attention: 33 lines in your changes are missing coverage. Please review.

Comparison is base (b12630c) 49.31% compared to head (a4c96db) 49.30%.
Report is 1 commits behind head on master.

Files	Patch %	Lines
util/settings/settings.go	19.04%	15 Missing and 2 partials ⚠️
server/server.go	60.97%	11 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #16856      +/-   ##
==========================================
- Coverage   49.31%   49.30%   -0.02%     
==========================================
  Files         272      272              
  Lines       47975    48044      +69     
==========================================
+ Hits        23660    23686      +26     
- Misses      21973    22009      +36     
- Partials     2342     2349       +7     

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

[aphtrinh]

(I wrote https://blog.calif.io/p/argo-cd-csrf)

This fixes the issue, but there may be problems.

Gorilla, or CSRF tokens in general make sense for web forms (application/x-www-form-urlencoded) but not JSON. First, the browser now has to make a redundant request to fetch the X-Csrf-Token first, before calling the API. Second, we have to use some csrf.UnsafeSkipCheck to bypass the token check when calling the API. This exception case may lead to bypasses in itself.

My suggestion would be to properly parse the Content-type header before the request body and strictly require application/json. This would trigger the default cross-site protections implemented by browsers: A preflight CORS request is sent, then Argo CD does not return Access-Control-Allow-Origin header, then the request fails). This is a key point that made the exploit possible because Argo CD allows text/plain content-type for JSON body (see the list of CORS-safelisted request headers).

I assume Argo CD does not really have a lot of cross-site use cases, judging from the fact there's no CORS header (Access-Control-Allow-Origin) implemented by the HTTP server. So maybe adding gorilla-csrf is an overhead we can avoid.

[alexmt]

Thank you for review @jessesuen ! Addressed both suggestions!

[ishitasequeira]

LGTM!!

[gdsoumya]

LGTM

[todaywasawesome]

LGTM

[jannfis]

LGTM

[crenshaw-dev]

I'd recommend holding off on this until we're certain that it's necessary in addition to content-type checking. Sounds like there are potential performance tradeoffs as well as additional maintenance to be sure the CSRF protection is functioning as expected.

[jannfis]

Just for consideration: We were holding off the original PR because we thought SameSite cookies would solve the issue at hand, but apparently it didn't. Worst case now would be when we think the Content-Type restriction solves the issue, but it doesn't.

Yes, it will have a slight performance impact. Yes maintenance might be a little bit more complex. But it will definitely implement current best practices.

[crenshaw-dev]

That's fair. In that case, I think we should merge this as a new feature, and we should add update 1) API docs and 2) upgrade docs to outline the change in API behavior.

[crenshaw-dev]

We should probably make this configurable via argocd-cmd-params-cm.

[jannfis]

Good catch.

[jannfis]

Agree, it should be a feature, not a a fix.

[kencochrane]

Where are we with this PR? Are we waiting? If so, what are we waiting for? We should make it clear in the PR so others who visit in the future will know the status.

[bh-tt]

I'll just bump this since @kencochrane 's comment had no real response.

[crenshaw-dev]

Sounds like there's general consensus behind the feature, so we're waiting for the things mentioned above: #16856 (review)