Hi Pierre,
Pierre Chifflier wrote:
> Thanks for the review. I have fixed almost every points you have
> mentioned, there are only a few points to discuss.
Thanks.
>> BTW, how does the "big picture" of this look like? I guess that your
>> nufw daemon is listening to packets via libnetfilter_queue and then it
>> sends unixsock messages that include the packet + extra authentication
>> information to ulogd2 to log them, right?
>
> Yes. We use ulogd as a central point for logging information, since
> packets (and decision) can come either from iptables (for non-user
> rules) or from the nufw+nuauth chain (for user filtering rules).
> When logging from nufw, we use the unixsock message to include the
> original packet (so the standard code from ulogd2 will be used) and add
> our additional information (user, application, etc).
>
> nflog + (drop|accept)
> / \
> netfilter ulogd2
> \ /
> (nfq) nufw --> nuauth
Interesting, I have a feeling that it was similar to this ;-).
> Yet this code has nothing specific to nufw, it can be used by any
> application to inject data with extra information.
I think so, but they would need to add new input keys to store that new
extra information or use the _NUFW_ keys. We can add a comment after the
_NUFW_ telling something that "add your new keys here".
>> could this be extended in the future to support flow-based information?
>> I mean, does this really fit into the "packet" directory of ulogd2?
>
> Well, I don't have any fixed opinion on this. If you want I can move
> this plugin to the flow directory (in some way it is similar to IPFIX).
My question is if this plugin is specific for packet-logging. If so, the
packet/ directory is fine.
>>> + [UNIXSOCK_KEY_NUFW_USER_NAME] = {
>>> + .type = ULOGD_RET_STRING,
>>> + .flags = ULOGD_RETF_NONE,
>>> + .name = "nufw.user.name",
>>> + },
>> Could we define some more generic instead of NUFW? I know it's your
>> thing but others may benefit from these fields I guess, right?
>>
>> Well, this is not important anyway, I don't mind too much.
>
> Sure. Should I use something like 'EXTRA' instead of NUFW in the constants
> names ?
I don't mind too much, I leave it up to you.
> [...]
>>> +
>>> +#define unixpath_ce(x) ((x)->ces[0])
>>> +#define bufsize_ce(x) ((x)->ces[1])
>>> +#define perms_ce(x) ((x)->ces[2])
>>> +#define owner_ce(x) ((x)->ces[3])
>>> +#define group_ce(x) ((x)->ces[4])
>> I know that this is xyz_ce() macros are the "standard way" in ulogd2 but
>> it's ugly. I would do the following:
>>
>> enum {
>> UNIXSOCK_OPT_UNIXPATH = 0,
>> UNIXSOCK_OPT_BUFSIZE,
>> UNIXSOCK_OPT_PERM,
>> UNIXSOCK_OPT_OWNER,
>> UNIXSOCK_OPT_GROUP,
>> };
>>
>> Then, we can do the transition easily to several functions like:
>>
>> ulogd_option_get_str(UNIXSOCK_OPT_UNIXPATH, upi->config_kset);
>> ulogd_option_get_u32(UNIXSOCK_OPT_PERM, upi->config_kset);
>>
>> and so on... this is something that I had in my head. I'm not asking you
>> to do so but to tell you about my intentions. If we start defining the
>> enums now, we can do the transition in the near future.
>
> I'd like that too ! I've defined the enum (and kept the xyz_ce macros
> for now). If you want, I can add these functions in another patch ?
Sure, go ahead if you want :-).
[...]
>>> +
>>> +struct ulogd_unixsock_option_t {
>>> + uint16_t option_id;
>>> + uint16_t option_length;
>>> + char option_value[0];
>>> +} __attribute__((packed));
>>> +
>>> +#define ALIGN_SIZE 8
>> Minor question: why align this to 64 bits?
>
> I originally used an alignment to 32 bits, but Jan noticed it would
> break if using options/values on 64 bits (and a test confirmed that). I
> took 64 bits as the biggest allowed value for integers.
I would need to look into this in more detail, not sure where the
problem is. I think that you can use something like `struct nlattr' (see
include/linux/netlink.h) and then nla_put() to add attributes in the TLV
format (see lib/nlattr.c). Those are align-safe. I'm using something
similar for conntrackd for the synchronization messages (src/build.c and
src/parse.c).
> [...]
>>> +/* callback called from ulogd core when fd is readable */
>>> +static int unixsock_instance_read_cb(int fd, unsigned int what, void *param)
>>> +{
>>> + struct ulogd_pluginstance *upi = param;
>>> + struct unixsock_input *ui = (struct unixsock_input*)upi->private;
>>> + int len;
>>> + u_int16_t needed_len;
>>> + u_int32_t packet_sig;
>>> + struct ulogd_unixsock_packet_t *unixsock_packet;
>>> +
>>> + char buf[4096];
>>> +
>>> + if (!(what & ULOGD_FD_READ))
>>> + return 0;
>>> +
>>> + len = read(fd, buf, sizeof(buf));
>> Ah, looking at this, I think that it would be a good idea to ignore
>> SIGPIPE to avoid crashing ulogd2 if the other peer has gone.
> Yes, I didn't want to do that in the plugin itself. Can I put that in
> src/ulogd.c ?
Yes, you can add a comment in ulogd.c telling that at least UNIXSOCK
needs this so we don't forget ;).
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
