CSRF middleware: Add flag to skip login cookie check #66806

PoorlyDefinedBehaviour · 2023-04-18T18:54:27Z

What is this feature?

Adds a flag that when set to false, makes the CSRF middleware proceed with the CSRF check even if the login cookie is not present.

Flag defaults to false to preserve the current behavior.

Why do we need this feature?

When using auth proxy, there's no cookie header in requests which causes the CSRF check in the CSRF middleware to be skipped. Some customers would like to execute the CSRF check anyway.

Who is this feature for?

[Add information on what kind of user the feature is for.]

Which issue(s) does this PR fix?:

Issue 144 for one of the enteprise customers.

Special notes for your reviewer:

Please check that:

It works as expected from a user's perspective.
If this is a pre-GA feature, it is behind a feature toggle.
The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

Jguer · 2023-04-19T07:55:14Z

conf/defaults.ini

@@ -369,6 +369,9 @@ content_security_policy_report_only_template = """script-src 'self' 'unsafe-eval
 # Controls if old angular plugins are supported or not. This will be disabled by default in future release
 angular_support_enabled = true

+# The CSRF check will be skipped if the login cookie is not present when this value is set to true.
+csrf_skip_check_if_login_cookie_not_present = true


would be it be possible to turn this into a positive option instead of a negative one?

Examples:

csrf_login_only_check

or

csrf_always_check

Yes, we can change to csrf_always_check or csrf_login_only_check.

Should we default to always checking? (the current behavior doesn't always check)

Changed the name to csrf_always_check and defaulted it to false. Let me know if it should default to true(breaking the current behavior)

let's keep it csrf_always_check=false as default for now, we can investigate bringing it as a default to true

Hi! Just my 2 cents here: the default behavior should be the same as before, this is a request from one of our external partners, which uses a auth proxy in front of grafana, and therefore the cookie is not set. So, in fact, we don't really want our default/current value to change, but just give them the ability to adapt it.

Jguer

defaulting to existing behavior is nice on the dev side 👍 but on the security side would it be a stronger default to always check?

cc @jmatosgrafana

we avoid naming options skip_* as we've seen them increase complexity and become proxy values for other behavior.

papagian

I think there is some inconsistency with the default value in the configuration files (defaults and samples), the configuration and the fallback value (MustBool()) when the setting is read.

Jguer · 2023-04-20T12:17:41Z

conf/defaults.ini

@@ -369,6 +369,9 @@ content_security_policy_report_only_template = """script-src 'self' 'unsafe-eval
 # Controls if old angular plugins are supported or not. This will be disabled by default in future release
 angular_support_enabled = true

+# The CSRF check will be executed even if the request has no login cookie.
+csrf_always_check = true


Suggested change

csrf_always_check = true

csrf_always_check = false

align with @papagian's comment

docs/sources/setup-grafana/configure-grafana/_index.md

chri2547

One small suggestion. Thank you for the contribution!

Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>

mengjie-ms · 2023-06-30T02:33:51Z

Hi @zerok, @taleena and @PoorlyDefinedBehaviour, I found the v9.5.5 doesn't cover the update, v10.x includes. I wonder when will include the fix in v9.X?

PoorlyDefinedBehaviour requested review from torkelo, a team and chri2547 as code owners April 18, 2023 18:54

PoorlyDefinedBehaviour requested review from papagian, zserge and suntala and removed request for a team April 18, 2023 18:54

PoorlyDefinedBehaviour marked this pull request as draft April 18, 2023 18:54

grafanabot added type/docs Flags the technical writing team for documentation support; auto adds to org-wide docs project area/backend labels Apr 18, 2023

PoorlyDefinedBehaviour self-assigned this Apr 18, 2023

PoorlyDefinedBehaviour requested a review from leandro-deveikis April 18, 2023 18:59

PoorlyDefinedBehaviour marked this pull request as ready for review April 18, 2023 19:01

PoorlyDefinedBehaviour force-pushed the poorlydefinedbehaviour/add_flag_to_disable_skip_csrf_check_if_login_cookie_is_not_present branch from 8f795b0 to 3c81822 Compare April 18, 2023 19:03

Jguer reviewed Apr 19, 2023

View reviewed changes

PoorlyDefinedBehaviour force-pushed the poorlydefinedbehaviour/add_flag_to_disable_skip_csrf_check_if_login_cookie_is_not_present branch from 3c81822 to ccfc6fc Compare April 19, 2023 13:35

PoorlyDefinedBehaviour requested a review from Jguer April 19, 2023 13:36

Jguer added add to changelog no-backport Skip backport of PR labels Apr 19, 2023

Jguer added this to the 10.0.0 milestone Apr 19, 2023

papagian reviewed Apr 20, 2023

View reviewed changes

Jguer reviewed Apr 20, 2023

View reviewed changes

CSRF middleware: add flag to skip login cookie check

d061444

PoorlyDefinedBehaviour force-pushed the poorlydefinedbehaviour/add_flag_to_disable_skip_csrf_check_if_login_cookie_is_not_present branch from ccfc6fc to d061444 Compare April 20, 2023 13:33

PoorlyDefinedBehaviour requested review from papagian and Jguer April 20, 2023 13:35

Jguer changed the title ~~CSRF middleware: add flag to skip login cookie check~~ CSRF middleware: Add flag to skip login cookie check Apr 20, 2023

Jguer approved these changes Apr 20, 2023

View reviewed changes

mgyongyosi approved these changes Apr 20, 2023

View reviewed changes

chri2547 reviewed Apr 24, 2023

View reviewed changes

docs/sources/setup-grafana/configure-grafana/_index.md Outdated Show resolved Hide resolved

chri2547 approved these changes Apr 24, 2023

View reviewed changes

Update docs/sources/setup-grafana/configure-grafana/_index.md

f38a2d8

Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>

PoorlyDefinedBehaviour merged commit d4715a6 into main Apr 24, 2023

PoorlyDefinedBehaviour deleted the poorlydefinedbehaviour/add_flag_to_disable_skip_csrf_check_if_login_cookie_is_not_present branch April 24, 2023 13:11

zerok modified the milestones: 10.0.0, 10.0.0-preview May 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSRF middleware: Add flag to skip login cookie check #66806

CSRF middleware: Add flag to skip login cookie check #66806

PoorlyDefinedBehaviour commented Apr 18, 2023 •

edited

Loading

Jguer Apr 19, 2023

PoorlyDefinedBehaviour Apr 19, 2023 •

edited

Loading

PoorlyDefinedBehaviour Apr 19, 2023 •

edited

Loading

Jguer Apr 20, 2023

leandro-deveikis Apr 20, 2023

Jguer left a comment

papagian left a comment

Jguer Apr 20, 2023

PoorlyDefinedBehaviour Apr 20, 2023

chri2547 left a comment

mengjie-ms commented Jun 30, 2023

CSRF middleware: Add flag to skip login cookie check #66806

CSRF middleware: Add flag to skip login cookie check #66806

Conversation

PoorlyDefinedBehaviour commented Apr 18, 2023 • edited Loading

Jguer Apr 19, 2023

Choose a reason for hiding this comment

PoorlyDefinedBehaviour Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

PoorlyDefinedBehaviour Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

Jguer Apr 20, 2023

Choose a reason for hiding this comment

leandro-deveikis Apr 20, 2023

Choose a reason for hiding this comment

Jguer left a comment

Choose a reason for hiding this comment

papagian left a comment

Choose a reason for hiding this comment

Jguer Apr 20, 2023

Choose a reason for hiding this comment

PoorlyDefinedBehaviour Apr 20, 2023

Choose a reason for hiding this comment

chri2547 left a comment

Choose a reason for hiding this comment

mengjie-ms commented Jun 30, 2023

PoorlyDefinedBehaviour commented Apr 18, 2023 •

edited

Loading

PoorlyDefinedBehaviour Apr 19, 2023 •

edited

Loading

PoorlyDefinedBehaviour Apr 19, 2023 •

edited

Loading