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

Filter: drop packets not Valid (with basic rules). #404

Open
tuongnd7 opened this issue Sep 23, 2021 · 20 comments
Open

Filter: drop packets not Valid (with basic rules). #404

tuongnd7 opened this issue Sep 23, 2021 · 20 comments
Labels
help wanted Extra attention is needed kind/design Proposal discussing new features / fixes and how they should be implemented kind/feature New feature or request

Comments

@tuongnd7
Copy link

Hi everyone,
I see UDP attack packets about 1400 in length.
while game UDP packets are about 3 to 20 in length.
I only found examples of drop UDP packets with a length less than the "size" configuration.
Can you help me how to configure to drop packets with length greater than size or maybe a range in size.
Thank you.

@tuongnd7 tuongnd7 added the kind/feature New feature or request label Sep 23, 2021
@markmandel
Copy link
Member

I can see us having a Filter that drops packets based on a min and max size value.

Maybe a config like:

version: v1alpha1
static:
  filters:
    - name: quilkin.extensions.filters.debug.v1alpha1.PacketSize
      config:
        min: 0
        max: 500
  endpoints:
    - address: 127.0.0.1:7001

Other name ideas:

  • Size
  • PacketRange
  • AllowSizeRange

Thoughts? PacketSize was the clearest to me, but wouldn't mind something that makes it intuitive that if the packet is outside the size range, then it will get dropped.

@markmandel markmandel added help wanted Extra attention is needed kind/design Proposal discussing new features / fixes and how they should be implemented labels Sep 23, 2021
@tuongnd7
Copy link
Author

Thanks for your help

@tuongnd7
Copy link
Author

I tried with your example but return the following error:
Error: CreateFilterChain(Filter { filter_name: "quilkin.extensions.filters.debug.v1alpha1.PacketSize", error: FilterInvalid(NotFound("quilkin.extensions.filters.debug.v1alpha1.PacketSize")) })

please guide me how can it work

@iffyio
Copy link
Collaborator

iffyio commented Sep 24, 2021

Hi @tuongnd7 so the mentioned PacketSize filter isn't something that exists today, but it does sound like a useful addition we should include!

@iffyio
Copy link
Collaborator

iffyio commented Sep 24, 2021

Dropping packets that don't meet some criteria definitely sounds reasonable, re what we should call it, I think it can be something more generic like a Validate filter where you describe what packets should look like and the filter drops anything that doesn't comply.
Min and Max size are examples of those, then there are cases where you know valid packets should e.g have a magic header so you'd configure a prefix/suffix in addition to the size requirements, etc

@tuongnd7
Copy link
Author

I understood thank you

@markmandel
Copy link
Member

Let's keep this open! It's a good suggestion!

@markmandel markmandel reopened this Sep 24, 2021
@markmandel markmandel changed the title can you help me configure a filter that drop packets past a specific length. Filter: drop packets not within a length range. Sep 24, 2021
@markmandel
Copy link
Member

Updated the title, let me know if it doesn't work.

@markmandel markmandel changed the title Filter: drop packets not within a length range. Filter: drop packets not Valid (with basic rules). Sep 24, 2021
@markmandel
Copy link
Member

Dropping packets that don't meet some criteria definitely sounds reasonable, re what we should call it, I think it can be something more generic like a Validate filter where you describe what packets should look like and the filter drops anything that doesn't comply.

I like this idea a lot! We could even just start with min/max length, and expand the set of rules over time. 👍🏻

@markmandel
Copy link
Member

Coming back around, given the above conversation, a configuration like the following I think would work:

version: v1alpha1
static:
  filters:
    - name: quilkin.extensions.filters.debug.v1alpha1.Validate
      config:
        bytes:
          min: 0 # (defaults to 0)
          max: 500 # (defaults to max UDP packet size)
  endpoints:
    - address: 127.0.0.1:7001

Then we can also add extra potential requirements as needed, as their own top level item under config, but this seems like a good start.

Other options for bytes could be size or length - but I thought that was the most descriptive.

What do we think?

@tuongnd7
Copy link
Author

Hi Mark Mandel,
I think length is a good choice for me, because I can easily monitor it.
In addition, I often get a DNS amplification DDoS attack,
attack source port is 53, is there a mechanism to blacklist whitelist source port?
image

@iffyio
Copy link
Collaborator

iffyio commented Sep 27, 2021

Other options for bytes could be size or length - but I thought that was the most descriptive.

config wise we likely don't need to group options since all such fields would be some sort of validation on the packet, something like

config:
  min_size: 0
  max_size: 1

@tuongnd7 re blocking traffic we're looking into adding support for that here #158 (comment)

@markmandel
Copy link
Member

markmandel commented Sep 28, 2021

On review, I think I like size better as well (bytes seems too generic). But I would definitely advocate for top level elements on configurations, rather than grouping by _. We're currently already doing it in our configurations, and it's easier to expand over time in my experience.

To give an example, if we had (switched from bytes to size):

version: v1alpha1
static:
  filters:
    - name: quilkin.extensions.filters.debug.v1alpha1.Validate
      config:
        size:
          min: 0 # (defaults to 0)
          max: 500 # (defaults to max UDP packet size)
  endpoints:
    - address: 127.0.0.1:7001

And we wanted to add a prefix and suffix rule as well, it could then become:

version: v1alpha1
static:
  filters:
    - name: quilkin.extensions.filters.debug.v1alpha1.Validate
      config:
        size:
          min: 0 # (defaults to 0)
          max: 500 # (defaults to max UDP packet size)
        prefix:
          size: 2
          value: AA==
        suffix:
          size: 1
          value: AA==
  endpoints:
    - address: 127.0.0.1:7001

I find this much better to view and use. Otherwise, this will become:

version: v1alpha1
static:
  filters:
    - name: quilkin.extensions.filters.debug.v1alpha1.Validate
      config:
        min_length: 0:
        max_length: 500
        size_prefix: 2
        value_prefix: AA==
        size_suffix: 1
        value_suffix: AA==
  endpoints:
    - address: 127.0.0.1:7001

(a) I find this inconsistent and (b) I think it's harder to read (c) and I don't find it as aesthetically nice (for whatever that is worth 😄 )

@iffyio
Copy link
Collaborator

iffyio commented Sep 29, 2021

On review, I think I like size better as well (bytes seems too generic). But I would definitely advocate for top level elements on configurations, rather than grouping by _. We're currently already doing it in our configurations, and it's easier to expand over time in my experience.

That sounds reasonable yeah!

@XAMPPRocky
Copy link
Collaborator

XAMPPRocky commented Oct 4, 2021

I'm not sure what size_suffix is for. But on the topic of accepting a range of values, instead of adding two more nested properties. What if we instead used Rust's syntax for ranges? This would express the same concept in less characters, and we could re-use it anywhere else we want to accept a range of values.

static:
  filters:
    - name: quilkin.extensions.filters.debug.v1alpha1.Validate
      config:
        # Inclusive Ranges
        length: 0..=500
        # Exclusive Ranges
        length: 0..501
        # Single Value
        length: 42
        # Up To 42 bytes
        length: ..42
        # At least 42 bytes
        length: 42..

@markmandel
Copy link
Member

I'm not sure what size_suffix is for.

It was more of a potential example of how the config could expand, rather than a functionality design.

@markmandel
Copy link
Member

🤔 Going back and digging through old issues - this seemed like a fun one to tackle.

One thought I had here was - rather than have the Validate dropping anything that doesn't match, it should instead push a "true" or "false" result into a dynamic metadata key, so it can be paired with a Match filter. I figured this would be far more flexible, and instead of individual params for the settings, let's just use a regex.

So in theory, if I wanted to validate a packet is between 10 and 30 bytes in length, it could be something like:

version: v1alpha1
endpoints:
  - address: 127.0.0.1:26000
  - address: 127.0.0.1:26001
filters:
  - name: quilkin.extensions.filters.debug.v1alpha1.Validate
    config:
      metadataKey: myapp.com/valid
      regex: ^.{10,30}$
  - name: quilkin.filters.match.v1alpha1.Match
    config:
        on_read:
          metadataKey: myapp.com/valid
          branches:
              - value: true
                filter: quilkin.filters.pass.v1alpha1.Pass
          fallthrough: quilkin.filters.drop.v1alpha1.Drop

I could then do more complex packet structure checking, etc if I needed to, I expect with Unicode based regular expressions.

We could still add a size section with min and max for simplicity, but regex should provide a lot of flexibility (very similar to what we do with Capture).

Thoughts?

@XAMPPRocky
Copy link
Collaborator

My only thought is, could this use case be covered by adding a drop parameter to Capture? Then parsing and validating are done by the same filter (Maybe even rename from capture to parse to make more clear that it doesn’t have to always capture).

@markmandel
Copy link
Member

I was also wondering if this could be combined somehow into Capture -- my eventual thought there was that while they are similar, they aren't quite the same, and I'd rather err on the side of "have a Filter do one thing, and one thing well" - basically Unix philosophy.

Let's say you in a scenario wherein you want to (a) check if a packet is between 5 and 10 bytes in length, and if so, grab the first 3 bytes, and do something with it.

I could do that with:

version: v1alpha1
endpoints:
  - address: 127.0.0.1:26000
filters:
  - name: quilkin.extensions.filters.debug.v1alpha1.Validate
    config:
      metadataKey: myapp.com/valid
      regex: ^.{5,10}$
  - name: quilkin.filters.match.v1alpha1.Match
    config:
        on_read:
          metadataKey: myapp.com/valid
          branches:
              - value: true
                filter: quilkin.filters.pass.v1alpha1.Capture
                metadataKey: myapp.com/capture
                config:
                  prefix:
                  size: 3
                  remove: false
          fallthrough: quilkin.filters.drop.v1alpha1.Drop
# ...now do something with the capture

And I think that's verbose, but easy to read. Also, long term we can expand the functionality of Validate, without worrying about running into anything in Capture.

It would definitely be more succinct if we had something like the following:

version: v1alpha1
endpoints:
  - address: 127.0.0.1:26000
filters:
  - name: quilkin.extensions.filters.debug.v1alpha1.Parse
    config:
      metadataKey: myapp.com/capture
      regex: ^(.{3}).{2,7}$
      drop: true
  - name: quilkin.filters.match.v1alpha1.Match
    config:
        on_read:
          # ...now do something with the capture
          fallthrough: quilkin.filters.drop.v1alpha1.Drop

Then we have to manage having two captured elements -the outermost and the innermost, and that's a little scary, as it could allow data through that it wasn't meant to, if either the whole element or its part matches the Match filter. We could add more logic about which thing to match against (which admittedly might actually be useful), but at that point it feels to me like diminishing returns - as the end user has to managing pushing two pieces of functionality into a single Filter invocation, or just ends up two instances of Parse since it's easier to manage, and at that point, we might as well have separate Validate and Capture anyway.

Thoughts?

@markmandel
Copy link
Member

Chatting in community meeting - happy to have separate filters, but the internal functionality will likely be able to be shared, so try and avoid duplication between Capture and Validate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed kind/design Proposal discussing new features / fixes and how they should be implemented kind/feature New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants