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

Setting of the verdict #10

Open
vorner opened this issue Dec 8, 2017 · 5 comments
Open

Setting of the verdict #10

vorner opened this issue Dec 8, 2017 · 5 comments

Comments

@vorner
Copy link
Contributor

vorner commented Dec 8, 2017

The current API has some drawbacks around the setting of verdict:

  • It is possible to set the verdict multiple times on the same message.
  • On the other hand, it is not possible to delay setting the verdict outside of the callback. This is well supported by the C library ‒ they even go so far to promise it's OK to verdict packets out of order and inside another thread.

Is it possible to pass owned (not borrowed) message to the callback and consume it during verdicting? And maybe go so far as to panic (or at least warn) if the message is destroyed without getting a verdict?

If the problem is the buffer of data, it could be passed to the callback as two parameters ‒ one Message, that'd contain the metadata and the ability to verdict, and borrowed slice of bytes as the payload.

@sharksforarms
Copy link

Hey @vorner and @chifflier, take a look at these changes:

sharksforarms/nfqueue-rs@improvements...sharksforarms:issue-#10

Let me know what you think!

@vorner
Copy link
Contributor Author

vorner commented Mar 23, 2019

I know it's mostly what I suggested that 1.5 years ago, but I wonder if it was the best idea. The downsides I see:

  • I might want to store the message for a while around and set the verdict later. I don't need to keep the content of packet around for that. I guess it's the same problem currently too, so it doesn't make it worse, but maybe it could be taken in one go with the changes? Maybe a way to clear the data off the message and leave it „hollow“?
  • The panicking is a nice reminder during normal application run (messages should not be just forgotten, because they are then stuck in the kernel side of nfqueue until the application disconnects), but I wonder if this may make shutting down without panics harder ‒ when I'm tearing everything down, I may want to just throw the messages out without verdicting them and kernel will drop once I terminate. This way the shutdown will explode.

Though I'd be glad to see the changes in either way 😇, these are just some points where it could not be the best solution (it's at least some solution).

@sharksforarms
Copy link

sharksforarms commented Mar 23, 2019

  1. Yeah that's my use case. Did you try any of the other copy modes?
/// Copy modes
pub enum CopyMode {
    /// Do not copy packet contents nor metadata
    CopyNone,
    /// Copy only packet metadata, not payload
    CopyMeta,
    /// Copy packet metadata and not payload
    CopyPacket,
}
  1. Not sure I like having it panic, better to have documentation around it and leave it up to the user (flexibility)

Thanks for the quick response @vorner !

@sharksforarms
Copy link

Interesting to note, I tried not setting a verdict and got this on stdout error in nfq_handle_packet(), so I guess there's some sort of warning. This function was returning -1 indicating a failure

@vorner
Copy link
Contributor Author

vorner commented Mar 25, 2019

Did you try any of the other copy modes?

Well, the use case I have in mind is to first inspect the packet (including the content) and decide that:

  • I may let it through.
  • I want to drop it/mark it for rejection in iptables.
  • A need to see some more packets before I can decide on this one. I've already mined all the info I ever will be able to from this one, so there's little use storing all its data any more, but I'll still want to give it the verdict eventually.

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

No branches or pull requests

2 participants