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.

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

varnishd: add a feature flag to disable bans in VCL #4168

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

walid-git
Copy link
Member

This is useful on multi-tenant environments to prevent a single user from wiping the whole cache or other user's cache.

Add vcl_ban feature flag to disallow usage of ban() in VCL
to prevent a possible DoS scenario in a multi-tenant setup.
@nigoroll
Copy link
Member

nigoroll commented Aug 21, 2024

I am not against, but is this a generic solution to the problem? Would it not make much more sense to add some sort of cache partitioning where a certain value is forcibly added to the hash and bans?

@walid-git
Copy link
Member Author

I am not against, but is this a generic solution to the problem? Would it not make much more sense to add some sort of cache partitioning where a certain value is forcibly added to the hash and bans?

I agree that there is probably much more improvements to do in this area, but this is a low hanging fruit that can already help in a lot of use cases, and I think it makes sense to have it.

@gquintard
Copy link
Member

Aren't the two ideas a bit different? One removes the ability altogether, the other silos it. Both have merits, but they're different

@gquintard gquintard closed this Aug 21, 2024
@gquintard gquintard reopened this Aug 21, 2024
@nigoroll
Copy link
Member

nigoroll commented Aug 21, 2024

I agree with both Walid's and Guiallaume's comments, but having this as a global flag really seems to be quite limited. Next up, someone might want feature +disable_bans_for_labeled_vcls or +disable_bans_unless_on_admin_acl. I would hope that just investing a little more could make this much more useful.

@bsdphk
Copy link
Contributor

bsdphk commented Aug 26, 2024

I'm -1 on this one. It doesn't seem hard to do it properly (see todays IRC log)

@walid-git walid-git closed this Aug 26, 2024
@dridi
Copy link
Member

dridi commented Sep 9, 2024

I agree with both Walid's and Guiallaume's comments, but having this as a global flag really seems to be quite limited. Next up, someone might want feature +disable_bans_for_labeled_vcls or +disable_bans_unless_on_admin_acl. I would hope that just investing a little more could make this much more useful.

How about a new vcl_feature bits parameter?

Initial default value: +ban,-connect

Since VCL bans simply exist today, we keep them enabled by default for compatibility, but new features like CONNECT support (#4152 (comment)) can be introduced opt-in.

Then we can add a new option to vcl.load to override VCL flags on a per-VCL basis:

param.set vcl_feature -ban
vcl.load customer-a-1 customer-a.vcl
vcl.load -f +ban admin-1 admin.vcl
vcl.label customer-a customer-a-1
vcl.label admin admin-1
vcl.load dispatch-1 main.vcl
vcl.use dispatch

FYI I would be against adding an override at the label level.

We can keep track of VCL overrides separately for enabled and disabled features (with a simple sanity check of (enabled&disabled) == 0) and have a special case VCL_FEATURE(vcl, x) macro.

@dridi dridi reopened this Sep 9, 2024
@nigoroll
Copy link
Member

nigoroll commented Sep 9, 2024

@dridi per vcl feature flags sound like a very good idea. I'd like to keep the CONNECT discussion separate, but regarding BANs I think your proposal is sound!

@dridi
Copy link
Member

dridi commented Sep 9, 2024

I only mentioned CONNECT to connect (pun obviously intended) this feature (pun intended) to other sensitive VCL capabilities.

Another example could be "import ... from":

param.set vcl_feature -import_from

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants