Impossible to create a chain with a DROP default policy

[Minaru]

Hello,

As I've started to try around some basic nftables manipulation through this package, I encountered a behavior which doesn't seem wanted.

As we can see here, Chain is a struct defined with a Policy field, typed as uint32. It takes some research (netlink.h) to know what value to use to get the result we want (as in, DROP or ACCEPT).

From the previous link, the dev now knows that they should use 0 to make their default policy DROP, and use 1 to make their default policy ACCEPT. Default policies can't be STOLEN, QUEUE, REPEAT or STOP, and as such, the netlink lib returns you an error if you try to do so.

The issue lies in the fact that if you set the Policy to DROP (0), the resulting chain has an ACCEPT default policy.

After some research within the code, I've encountered that particular function which seems to be the faulty part.

In that function, we can find this snippet of code:

if c.Policy > 0 {
        data = append(data, cc.marshalAttr([]netlink.Attribute{
            {Type: unix.NFTA_CHAIN_POLICY, Data: binaryutil.BigEndian.PutUint32(uint32(c.Policy))},
        })...)
    }

Basically, the function AddChain ignores the policy if it's equal to 0, which is the value for the default policy to be set to DROP.

There's several ways to fix this, which is why I'm writing this issue toward the goal of getting feedback on how to solve it.

• We could just fix the > operator issue, which would simply result into using a >= instead.
• We could create a new type in chain.go, called PolicyVerdict, that would be a uint32. We also make two const, PolicyDrop and PolicyAccept, which would be defined as equal to iota (so 0 and 1, exactly the netfilter numbers for DROP and ACCEPT. This solution makes things easily understandable without any extra import, allow additional documentation about the Policy field (currently there isn't any doc about that field beside its definition) but is kind of redundant with the VerdictKind type already existing in expr submodule.
• We use VerdictKind type as existing in expr submodule, which means we pass an int64 as an uint32 through the type. In that solution, I'm a bit scared that the endianness might create issues at some point/on some systems, due to the different size of int64/uint32 that will lead in some case to values not reflecting the initial user entry.
• We use VerdictKind type, but this time we modify VerdictKind type to be an uint32; As defined in netfilter.h, verdict values are included between 0 and 5, so it shouldn't create any issue to functions already expecting an int64 under the VerdictKind type. It's, in my opinion, the best solution, beside the fact that an extra export will be needed to use the policy field (both nftables and nftables/expr)

I'm up to make the PR as soon as something is decided

Awaiting for your input, Regards

[stapelberg]

Thanks for the detailed report!

I like this approach the most:

• We could create a new type in chain.go, called PolicyVerdict, that would be a uint32. We also make two const, PolicyDrop and PolicyAccept, which would be defined as equal to iota (so 0 and 1, exactly the netfilter numbers for DROP and ACCEPT.

Wouldn’t we have the same problem that we currently have if DROP maps to 0? Shouldn’t we use 1+x as our value space to distinguish the empty value from an explicitly set value?

This solution makes things easily understandable without any extra import, allow additional documentation about the Policy field (currently there isn't any doc about that field beside its definition) but is kind of redundant with the VerdictKind type already existing in expr submodule.

I’m not too worried about that redundancy. Verdicts are used in different contexts than Policies, so they can have a separate type.

[Minaru]

You indeed have a point about DROP mapping to 0, which makes it impossible to make the difference between an explicitely defined DROP policy and an undefined one.

I personally dislike the "hacky" approach of +1/-1, as it tends to get confusing for the end user dev and also can cause problems if you forget for a minute that it's here while working onto improving the type.

I'd suggest making the Policy field in Chain a pointer upon our new type. It will come with the drawback that the user cannot simply use our new type constants directly, (because you'd need to take the address of the constant through &, which isn't possible because of limitations of the language itself), but at the same time it'll ensure the difference between uninitialised Policy field (which would be nil) and initialised field set to DROP (which would be a pointer to a myType which value would be 0 after dereferencing it).

Also, for the sake of keeping the same formatting as other types in chain.go, I'll name the new type ChainPolicy. It sounds better than PolicyVerdict and fits the Chain_somethingsomething_ format that is already used.

Here's few snipets of how I imagine the changes in the existing code and how it'd impact the use

Definition:

// ChainPolicy defines what this chain default policy will be.
type ChainPolicy uint32

// Possible ChainPolicy values.
const (
    ChainPolicyDrop ChainPolicy = iota
    ChainPolicyAccept
)

// A Chain contains Rules. See also
// https://wiki.nftables.org/wiki-nftables/index.php/Configuring_chains
type Chain struct {
    Name     string
    Table    *Table
    Hooknum  ChainHook
    Priority ChainPriority
    Type     ChainType
    Policy   *ChainPolicy
}

That way we just have to check if it's nil or not, and change the way Policy is used in Chain related functions so it dereferences the pointer when the value is needed.

Effectively, the user would have to create his chains that way with those modifications:

var defaultPolicy = nftables.ChainPolicyDrop

conn.AddTable(&nftables.Table{
        Family: nftables.TableFamilyIPv4,
        Name: "filter",
    })

conn.AddChain(&nftables.Chain{
        Name: "forward",
        Table: filterTable,
        Type: nftables.ChainTypeFilter,
        Hooknum: nftables.ChainHookForward,
        Priority: nftables.ChainPriorityFilter,
        Policy: &defaultPolicy,
    })

I don't see any other way to keep the user space values aligned with the kernel space values, while solving our undefined != defined at DROP issue.

Let me know if you have any other idea or feedback on this one.

[stapelberg]

I’m a bit torn on using pointers. We don’t consistently do that in our current data structures, so introducing that here creates a bit of a mismatch.

On the other hand, I understand your concern about the constants not matching, which can be very confusing when debugging, depending on how you print data/values.

Perhaps using a pointer is the lesser evil after all.

[Minaru]

There could be another approach but it also has its own pros and cons.

The MySQL connector for go deals with non-nullable types by creating its own types, such has this NullInt32.

Pros:

• Structs have their fields initialised by their default values. In our case, the uint32 would be initialised to 0, and the bool to false, which makes the check of an explicitly defined policy very easy.
• DROP will be equal to 0, ACCEPT to 1, so we don't need to use the +1/-1 solution.
• It won't use pointers, which, I agree, can get confusing on devs who come from more high level languages/networking world and have reduced knowledge on how (de)referencing works. It should save some of us a lot of headaches.

Cons:

• We can't create constant structs (language limitation) BUT there would be a workaround were we could call a function that returns us an ACCEPT/DROP struct (so we can imagine something like nftables.ChainDefaultPolicy(nftables.ChainPolicyDrop) that returns a struct initialised with uint32 at 0 and bool at true).
• This approach is probably harder to understand at first even if its usability is better than pointers.

[stapelberg]

Thanks for the thorough research.

I think the pointer solution is the most appealing over all, if only because some of our tables already have a *Table pointer field.

[Minaru]

I'm closing this as PR has been merged.