Implement CSRF token validation on oauth2-proxy #2636

AthinaPl · 2024-05-09T12:19:09Z

Description

This PR introduces CSRF protection for sessions in oauth2-proxy, complementing the login CSRF mechanism already in place. The feature supports adding a CSRF token as part of the session stored in SessionState (cookie or Redis), which the proxy will return to the client application in multiple possible ways, and will expect to exist in incoming requests in a custom header to compare with the CSRF token for the active session.

CSRF token validation

CSRF for oauth2-proxy will implement the Synchronizer Token Pattern (link). Tokens will be random strings, 32 bytes in size. The CSRF token validation will apply when all of the following hold:

the CSRF feature is enabled during proxy configuration
the request is authenticated via cookie
the request uses an unsafe method according to RFC 7231

OAuth2 Proxy configuration for CSRF

Below is a list of the arguments introduced, with a description of their operation.

CSRF token generation

--csrftoken (boolean): Enable CSRF token generation on oauth2-proxy. Default: false
This argument will operate as a switch for including CSRF tokens in the session. It will be disabled by default, to maintain backwards compatibility and avoid breaking changes.

CSRF token cookie

The arguments below control the CSRF token cookie attributes, when the proxy is configured to return the session CSRF token as cookie to the client application.

--csrftoken-cookie-name (string): The name of the CSRF token cookie that the oauth2-proxy creates. If set to empty string, so CSRF cookie will be created for the session. Default: _oauth2_proxy_csrftoken
--csrftoken-cookie-path (string): The path of the CSRF token cookie (ie: /poc/)*. Default: /
--csrftoken-cookie-domain (string | list): The domain(s) of the CSRF token cookie (ie: .yourcompany.com). The longest domain matching the request's host will be used (or the shortest cookie domain if there is no match). Default: ""
--csrftoken-cookie-expire (duration): Expiration timeframe for the CSRF token cookie. Default: 168 hours
--csrftoken-cookie-secure (boolean): Set secure (HTTPS) cookie flag for CSRF token cookie. Default: true
--csrftoken-cookie-httponly (boolean): Set HttpOnly cookie flag for CSRF token cookie. Default: false
--csrftoken-cookie-samesite (string): Set SameSite cookie attribute for CSRF token cookie (ie: lax, strict, none, or ""). Default: strict

CSRF token header

The next argument configures the name of the header for the CSRF token, if the proxy is configured to return the CSRF token as header.

--csrftoken-response-header (string): The name of the actual CSRF token header to return to client. If set to empty string, no CSRF token header will be set by oauth2-proxy. Default: X-CSRF-Token

Client CSRF token header

The below argument controls the expected header that will carry the client's CSRF token, required for validation.

--csrftoken-header (string): The name of the header for holding the CSRF token sent from the client. Default: X-CSRF-Token

Allow skipping CSRF validation for certain routes

--skip-csrftoken-route (string | list): Bypass CSRF token validation for requests that match the method and path. Format: method=path_regex OR method!=path_regex. For all methods: path_regex OR !=path_regex. Default: "".

OAuth2 Proxy endpoint for CSRF tokens

This PR also introduces the GET /oauth2/csrftoken endpoint on the proxy. This is an authenticated endpoint (needs a valid session token) that returns the CSRF token for the session as a JSON response. This endpoint can be used by client applications to fetch the CSRF token right after completing the OAuth2 login cycle.

If the proxy if configured with the CSRF feature disabled, this endpoint will return a 404 NotFound response.

Motivation and Context

Extending oauth2-proxy with CSRF token validation is crucial for the following reasons:

Multiple applications might be set up behind an oauth2-proxy instance, so implementing CSRF token validation on the proxy offers a secure setup across all underlying components, compared to implementing CSRF token validation in respective applications.
oauth2-proxy owns the session and its attributes, which is helpful for implementing CSRF protection per session.

This PR will close issue #2573.

How Has This Been Tested?

I've made sure the test suites are up to date and don't break with the new changes. I've also tested in a live cluster.

Checklist:

I have some additions pending, see [Feature]: Implement CSRF token validation on oauth2-proxy #2573 (comment)
My change requires a change to the documentation or CHANGELOG.
I have updated the documentation/CHANGELOG accordingly.
I have created a feature (non-master) branch for my PR.
I have written tests for my code changes.

Introduce field CSRFToken in the session state struct. The CSRF token will be a random string (nonce) that is generated and populates the CSRFToken field when a session is created. Furthermore, introduce flag --csrftoken (default: false) to control CSRF token generation in a session. Co-authored-by: Athina Plaskasoviti <[email protected]> Co-authored-by: Konstantinos Lolos <[email protected]>

Expose GET /oauth2/csrftoken endpoint to fetch the CSRF token of the current session. If CSRF token generation is disabled (which is the default behavior currently), this endpoint will respond with 404 NotFound. The endpoint needs authentication, i.e. expects a valid session to exist. Co-authored-by: Athina Plaskasoviti <[email protected]> Co-authored-by: Konstantinos Lolos <[email protected]>

Introduce helpers that will wrap the CSRF token in a cookie. Set the cookie along with the session cookie, when the callback function is called. Expose the following flags to configure CSRF token cookie attributes: * --csrftoken-cookie-name: the name of the CSRF token cookie (default: _oauth2_proxy_csrftoken). If set to an empty string, the functionality is disabled. * --csrftoken-cookie-domain: the domain(s) of the cookie (optional, default "") * --csrftoken-cookie-path: the path of the cookie (optional, default: /) * --csrftoken-cookie-expire: expiration of the cookie (default: 168 hours) * --csrftoken-cookie-secure: set secure (HTTPS) cookie (default: true) * --csrftoken-cookie-httponly: set HTTPOnly for cookie (default: false) * --csrftoken-cookie-samesite: set SameSite for cookie (default: strict) Note that a CSRF cookie should be secure but not HttpOnly, since clients are expected to retrieve the value of the token to send in subsequent requests. Set SameSite to strict by default. Co-authored-by: Athina Plaskasoviti <[email protected]> Co-authored-by: Konstantinos Lolos <[email protected]>

Expose flag --csrftoken-response-header (string) to allow returning the session CSRF token as a custom header. Default header name is X-CSRF-Token, which the proxy adds to every request if the session has a CSRF token. If --csrftoken-response-header is set to an empty string, the functionality is disabled. Co-authored-by: Athina Plaskasoviti <[email protected]> Co-authored-by: Konstantinos Lolos <[email protected]>

Introduce field AuthMethod in request scope. Allow middlewares to store the authentication method in AuthMethod field of the RequestScope. It is necessary for the proxy to be aware if authentication was achieved through a cookie or a header, as only requests that are authenticated via cookie need CSRF protection, while programmatic client requests that use an authorization header do not need validation of CSRF tokens. Co-authored-by: Athina Plaskasoviti <[email protected]> Co-authored-by: Konstantinos Lolos <[email protected]>

Implement CSRF token validation on oauth2-proxy and expose flag --csrftoken-header (string) to allow setting the name of the CSRF token header expected in the client requests. The proxy implements the Synchonizer Token Pattern for CSRF protection: * reads the CSRF token from the header defined by --csrftoken-header * compares it with the token in the current session (session cookie or Redis). If the tokens are the same, request is authenticated, else denied. CSRF token validation is applied when all of the following hold: * when --csrftoken=true * in requests with unsafe methods, i.e. requests that the client does not request, and does not expect, any state change on the origin server as a result of applying a safe method to a target resource, according to RFC 7231. * in requests that are authenticated via cookie Co-authored-by: Athina Plaskasoviti <[email protected]> Co-authored-by: Konstantinos Lolos <[email protected]>

) Introduce flag --skip-csrftoken-route which accepts a string or a list of string METHOD=PATH combinations, to exclude from CSRF validation in the proxy. To completely disable CSRF token validation, set --skip-csrftoken-route="*". Co-authored-by: Athina Plaskasoviti <[email protected]> Co-authored-by: Konstantinos Lolos <[email protected]>

Extend the Overview page to include usage of arguments: * --csrftoken * --csrftoken-cookie-name * --csrftoken-cookie-path * --csrftoken-cookie-domain * --csrftoken-cookie-expire * --csrftoken-cookie-secure * --csrftoken-cookie-httponly * --csrftoken-cookie-samesite * --csrftoken-header * --csrftoken-response-header * --skip-csrftoken-route that were introduced in previous commits. Co-authored-by: Athina Plaskasoviti <[email protected]> Co-authored-by: Konstantinos Lolos <[email protected]>

tuunit

Some comments that come to mind after a quick scan. Overall the PR and especially all the tests seem really well done. I'm just concerned about feature creep if this is something we want to add and maintain in oauth2-proxy 🤔

@JoelSpeed what do you think?

tuunit · 2024-05-13T12:22:49Z

docs/docs/configuration/overview.md

@@ -209,7 +209,17 @@ An example [oauth2-proxy.cfg](https://github.com/oauth2-proxy/oauth2-proxy/blob/
 | `--version` | n/a | print version string | |
 | `--whitelist-domain` | string \| list | allowed domains for redirection after authentication. Prefix domain with a `.` or a `*.` to allow subdomains (e.g. `.example.com`, `*.example.com`)&nbsp;[^2] | |
 | `--trusted-ip` | string \| list | list of IPs or CIDR ranges to allow to bypass authentication (may be given multiple times). When combined with `--reverse-proxy` and optionally `--real-client-ip-header` this will evaluate the trust of the IP stored in an HTTP header by a reverse proxy rather than the layer-3/4 remote address. WARNING: trusting IPs has inherent security flaws, especially when obtaining the IP address from an HTTP header (reverse-proxy mode). Use this option only if you understand the risks and how to manage them. | |
-| `--encode-state` | bool | encode the state parameter as UrlEncodedBase64 | false |


I think you accidentally removed the encode-state flag

tuunit · 2024-05-13T12:24:03Z

docs/docs/configuration/overview.md

+| `--csrftoken-cookie-name` | string | The name of the CSRF cookie. If set to empty string, no CSRF cookie will be set by oauth2-proxy. | `"_oauth2_proxy_csrftoken"` |
+| `--csrftoken-cookie-domain` | string \| list | Optional cookie domains to set CSRF cookies to (e.g. `.yourcompany.com`). The longest domain matching the request's host will be used (or the shortest cookie domain if there is no match). | |
+| `--csrftoken-cookie-path` | string | An optional cookie path to set CSRF cookies to (e.g. `/poc/`). | `"/"` |
+| `--csrftoken-cookie-secure` | bool | set [secure (HTTPS only) cookie flag](https://owasp.org/www-community/controls/SecureFlag). | true |


The url is wrong

https://owasp.org/www-community/controls/SecureCookieAttribute

tuunit · 2024-05-13T12:27:07Z

oauthproxy.go

+const (
+ MethodGet = "GET"
+ MethodHead = "HEAD"
+ MethodOptions = "OPTIONS"
+ MethodTrace = "TRACE"
+)
+
+var SafeMethods = []string{
+ MethodGet,
+ MethodHead,
+ MethodOptions,
+ MethodTrace,
+}


Suggested change

const (

MethodGet = "GET"

MethodHead = "HEAD"

MethodOptions = "OPTIONS"

MethodTrace = "TRACE"

)

var SafeMethods = []string{

MethodGet,

MethodHead,

MethodOptions,

MethodTrace,

}

type RequestMethod string

const (

MethodGet RequestMethod = "GET"

MethodHead = "HEAD"

MethodOptions = "OPTIONS"

MethodTrace = "TRACE"

)

this would be the idiomatic golang way to define a string enum

tuunit · 2024-05-13T12:31:47Z

oauthproxy.go

+ allowedCSRFRoutes, err := buildAllowedRoutes(opts.SkipCSRFRoutes)
+ if err != nil {
+ return nil, err
+ }


allowedCSRFRoutes seems a bit confusing to me.

it could easily be misinterpreted like the following:

either routes that are protected through csrf cookies

or routes you allow csrf cookies not to be valid

We chose this name to be in line with the naming convention that --skip-auth-route follows. But I get your point, let's rename to something that is more clear.

tuunit · 2024-05-13T12:35:18Z

oauthproxy.go

+func isSafeMethod(req *http.Request) bool {
+ return slices.Contains(SafeMethods, req.Method)
+}


if the "safe methods" slice is only used in this method shouldn't it be defined locally instead of the root level?

AthinaPl · 2024-05-14T11:42:43Z

Hello! Thanks for the initial feedback!

I'm just concerned about feature creep if this is something we want to add and maintain in oauth2-proxy :thinking_face:

I understand how feature creep might be an issue, however we think this will be a valuable addition to the proxy security wise, which will be of use in many cases, especially since oauth2-proxy is usually running in front multiple services and implementing CSRF on the proxy avoids having to implement it multiple times in the respective applications. After all there was already an open request about it but no resources on the team to handle it (#2081 (comment)), which motivated us to implement and help move this forward. This is also why we added tests, to make sure that we don't introduce breaking changes to existing codebase and configurations.

In any case, we hope you'll find this necessary as well. And we are happy to receive feedback in order to help the review process and make a seamless integration.

If you do move forward with the review, let me know how you prefer changes and fixes - should I add them on top of the branch as fixups and rebase before merging, or should I rebase them immediately before sending them?

Modify the injector interface to handle request scope instead of session state. This makes the interface more flexible to retrieving values for header injection. The session state can be obtained from the request scope. Co-authored-by: Athina Plaskasoviti <[email protected]> Co-authored-by: Konstantinos Lolos <[email protected]>

Allow forwarding the authentication method from the request scope to upstream. This is necessary in case the CSRF token validation is implemented on the upstream, and not necessary to enable on the proxy, so upstream can decide when to apply or skip validation, since only requests that are authenticated by cookie should be subjected to CSRF protection. Co-authored-by: Athina Plaskasoviti <[email protected]> Co-authored-by: Konstantinos Lolos <[email protected]>

…2-proxy#2573) Remove the CSRF token header from the request if oauth2-proxy has been configured to run with --skip-auth-strip-headers=true Co-authored-by: Athina Plaskasoviti <[email protected]> Co-authored-by: Konstantinos Lolos <[email protected]>

Signed-off-by: Athina Plaskasoviti <[email protected]>

AthinaPl · 2024-05-22T13:00:04Z

I pushed 3 more commits that target #2573 (comment).

I also added some initial minor fixes (including comments #2636 (comment) and #2636 (comment)).

Looking forward to more!

AthinaPl and others added 8 commits May 9, 2024 14:45

AthinaPl requested a review from a team as a code owner May 9, 2024 12:19

github-actions bot added docs go tests labels May 9, 2024

tuunit suggested changes May 13, 2024

View reviewed changes

AthinaPl and others added 6 commits May 22, 2024 15:05

fixup! Return CSRF token as cookie (oauth2-proxy#2573)

95a5d16

Signed-off-by: Athina Plaskasoviti <[email protected]>

fixup! Modify injector interface to handle scope

82ed82d

Signed-off-by: Athina Plaskasoviti <[email protected]>

fixup! Add documentation for CSRF CLI arguments (oauth2-proxy#2573)

8d46987

Signed-off-by: Athina Plaskasoviti <[email protected]>

AthinaPl changed the title ~~[WIP] Implement CSRF token validation on oauth2-proxy~~ Implement CSRF token validation on oauth2-proxy May 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement CSRF token validation on oauth2-proxy #2636

Implement CSRF token validation on oauth2-proxy #2636

AthinaPl commented May 9, 2024 •

edited

tuunit left a comment

tuunit May 13, 2024

tuunit May 13, 2024

tuunit May 13, 2024

tuunit May 13, 2024

tuunit May 13, 2024

AthinaPl May 14, 2024

tuunit May 13, 2024 •

edited

AthinaPl commented May 14, 2024

AthinaPl commented May 22, 2024 •

edited

Implement CSRF token validation on oauth2-proxy #2636

Are you sure you want to change the base?

Implement CSRF token validation on oauth2-proxy #2636

Conversation

AthinaPl commented May 9, 2024 • edited

Description

CSRF token validation

OAuth2 Proxy configuration for CSRF

CSRF token generation

CSRF token cookie

CSRF token header

Client CSRF token header

Allow skipping CSRF validation for certain routes

OAuth2 Proxy endpoint for CSRF tokens

Motivation and Context

How Has This Been Tested?

Checklist:

tuunit left a comment

Choose a reason for hiding this comment

tuunit May 13, 2024

Choose a reason for hiding this comment

tuunit May 13, 2024

Choose a reason for hiding this comment

tuunit May 13, 2024

Choose a reason for hiding this comment

tuunit May 13, 2024

Choose a reason for hiding this comment

tuunit May 13, 2024

Choose a reason for hiding this comment

AthinaPl May 14, 2024

Choose a reason for hiding this comment

tuunit May 13, 2024 • edited

Choose a reason for hiding this comment

AthinaPl commented May 14, 2024

AthinaPl commented May 22, 2024 • edited

AthinaPl commented May 9, 2024 •

edited

tuunit May 13, 2024 •

edited

AthinaPl commented May 22, 2024 •

edited