Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 2 commits into from
Apr 24, 2023
Merged

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

merged 2 commits into from
Apr 24, 2023

Conversation

PoorlyDefinedBehaviour
Copy link
Member

@PoorlyDefinedBehaviour PoorlyDefinedBehaviour commented Apr 18, 2023 •
edited

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.

@PoorlyDefinedBehaviour PoorlyDefinedBehaviour requested review from papagian, zserge and suntala and removed request for a team April 18, 2023 18:54
@PoorlyDefinedBehaviour PoorlyDefinedBehaviour marked this pull request as draft April 18, 2023 18:54
@PoorlyDefinedBehaviour PoorlyDefinedBehaviour marked this pull request as ready for review April 18, 2023 19:01
@PoorlyDefinedBehaviour 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
Jguer reviewed Apr 19, 2023
View reviewed changes
conf/defaults.ini Outdated
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Jguer reviewed Apr 19, 2023
View reviewed changes
Copy link
Contributor

@Jguer Jguer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@PoorlyDefinedBehaviour 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
@Jguer Jguer added add to changelog no-backport Skip backport of PR labels Apr 19, 2023
@Jguer Jguer added this to the 10.0.0 milestone Apr 19, 2023
papagian
papagian reviewed Apr 20, 2023
View reviewed changes
Copy link
Contributor

@papagian papagian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Jguer reviewed Apr 20, 2023
View reviewed changes
conf/defaults.ini Outdated
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
csrf_always_check = true
csrf_always_check = false

align with @papagian's comment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@PoorlyDefinedBehaviour 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
@Jguer 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
chri2547
chri2547 approved these changes Apr 24, 2023
View reviewed changes
Copy link
Collaborator

@chri2547 chri2547 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small suggestion. Thank you for the contribution!

@PoorlyDefinedBehaviour @chri2547
Update docs/sources/setup-grafana/configure-grafana/_index.md
f38a2d8 
Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>
@PoorlyDefinedBehaviour PoorlyDefinedBehaviour merged commit d4715a6 into main Apr 24, 2023
14 checks passed
@PoorlyDefinedBehaviour PoorlyDefinedBehaviour deleted the poorlydefinedbehaviour/add_flag_to_disable_skip_csrf_check_if_login_cookie_is_not_present branch April 24, 2023 13:11
@zerok zerok modified the milestones: 10.0.0, 10.0.0-preview May 31, 2023
@mengjie-ms
Copy link

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgyongyosi mgyongyosi mgyongyosi approved these changes

@Jguer Jguer Jguer approved these changes

@chri2547 chri2547 chri2547 approved these changes

@torkelo torkelo Awaiting requested review from torkelo torkelo is a code owner

@zserge zserge Awaiting requested review from zserge zserge was automatically assigned from grafana/backend-platform

@suntala suntala Awaiting requested review from suntala suntala was automatically assigned from grafana/backend-platform

@leandro-deveikis leandro-deveikis Awaiting requested review from leandro-deveikis

@papagian papagian Awaiting requested review from papagian

Assignees

@PoorlyDefinedBehaviour PoorlyDefinedBehaviour

Labels
add to changelog area/backend no-backport Skip backport of PR type/docs
Projects
None yet
Milestone
10.0.0-preview
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@PoorlyDefinedBehaviour @mengjie-ms @mgyongyosi @papagian @Jguer @leandro-deveikis @chri2547 @zerok @grafanabot