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

fixes #45 - Add a log expression with group argument #46

Closed
wants to merge 3 commits into from

Conversation

lafleurdeboum
Copy link

@lafleurdeboum lafleurdeboum commented Sep 28, 2021

This PR adds a log expression, solving #45 . To add a log rule do

use std::ffi::CString;
use nftnl::{Rule, Chain, nft_expr};
let chain = Chain::new(&CString::new("some-chain-name")?);
let rule = Rule::new(&chain);
rule.add_expr(&nft_expr!(log));

And then insert in the chain where you bet useful.

To set the group argument to a value between zero and seven, pick a value in the expr::LogGroup enum and do :

use std::ffi::CString;
use nftnl::{Rule, Chain, nft_expr, expr::LogGroup::LogGroupZero};
let chain = Chain::new(&CString::new("some-chain-name")?);
let rule = Rule::new(&chain);
rule.add_expr(&nft_expr!(log group LogGroupZero));

As stated in nftables' wiki, setting the group argument activates NFLog output, useable by ulogd2, a more advanced logging target than the default. The default is to output to kernel log, which is a pity, because it's often unprotected. ulogd needs a running daemon, and lets you choose output targets and log format.

So this PR is probably not the most urgent thing in Mullvad userland, but can prove useful, and is very compact. Besides, it is very much needed for other applications that would use this library for its quality. nftnl-rs rocks, by the way ! 🥇


This change is Reviewable

@lafleurdeboum lafleurdeboum changed the title Add a log expression with group argument Add a log expression with group argument - fixes #45 Oct 16, 2021
@lafleurdeboum lafleurdeboum changed the title Add a log expression with group argument - fixes #45 fixes #45 - Add a log expression with group argument Oct 16, 2021
@lafleurdeboum
Copy link
Author

@faern would you consider commenting this PR ? I believe it can't break anything from any mullvad crate, since it simply adds functionality that was nonexistent until then. Besides, it is (arguably) quite needed to use nftnl-rs as a main linux firewall library, since logging is a critical concern for those.

Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! 👍

}

#[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)]
pub enum LogGroup {
Copy link
Member

@dlon dlon Nov 12, 2021

Choose a reason for hiding this comment

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

I couldn't find any reason for limiting this to 8. u16 seems to be the actual type, so I think it would make sense to use that.

nft_expr_log!(group $group)
};
(log) => {
nft_expr_log!()
Copy link
Member

Choose a reason for hiding this comment

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

Could you call crate::expr::Log::new(None) here directly instead?

b"log\0" as *const _ as *const c_char
));
if let Some(group) = self.group {
sys::nftnl_expr_set_u32(
Copy link
Member

Choose a reason for hiding this comment

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

This should be nftnl_expr_set_u16.

@faern
Copy link
Member

faern commented May 29, 2024

Thanks for wanting to contribute! However, due to inactivity (from both sides I have to say), this is superseded by #56

@faern faern closed this May 29, 2024
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.

3 participants