Add connection state methods #106

dlon · 2024-06-28T07:56:50Z

Add methods for listing and removing individual PF connection states. The existing methods (clear_states, and clear_interface_states) only allow states associated with a given anchor or interface to be removed, so these give more control.

This change is

faern

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @dlon)

src/state.rs line 15 at r1 (raw file):

    Ip(IpState),
    /// Any connection state that could not be parsed
    Raw(pfsync_state),

This publicly exposes a type from the ffi module. We can't do this. Everything related to the FFI and automatically generated bindings are internal details only.

src/state.rs line 32 at r1 (raw file):

        IpState::try_from(state)
            .map(State::Ip)
            .unwrap_or_else(|_| State::Raw(state))

Is there no field in pfsync_state that indicate what kind of state it is? Having it be raw just because it fails to parse as an IP state feels slightly off, but I don't know the PF internals around this.

faern

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @dlon)

src/lib.rs line 344 at r1 (raw file):
I think the documentation would be clearer if it mentions that it removes states for which the filter returns false. Because that's the "action" that is taken in this method. Nothing actually happens to the states where the filter returns true, they are just ignored.

Keep all states for which filter returns true. The rest are removed from PF.

?

Would also be nice if it mentions that if the function returns an error, some states might have been removed already.

If an error is returned, all states except the last one for which the filter returned false has already been removed.

faern

Reviewed 4 of 5 files at r2.
Reviewable status: 4 of 5 files reviewed, 7 unresolved discussions (waiting on @dlon)

src/lib.rs line 92 at r2 (raw file):

mod state;
pub use crate::state::*;

The state module only has a single type. So this is fine. But otherwise I suggest we stop re-exporting everything from all modules like this. The main namespace of this crate is growing pretty big. Having a bit of a tree structure would probably clean it up a bit. But again, not important here since it's only one item.

src/lib.rs line 188 at r2 (raw file):

            InvalidAddressFamily(reason) => write!(f, "Invalid address family ({reason})"),
            InvalidDirection(reason) => write!(f, "Invalid direction ({reason})"),
            InvalidTransportProtocol(reason) => write!(f, "Invalid transport protocol ({reason})"),

I think we should use a better name than reason here. These are not reasons. These are address_family, direction and protocol respectively.

src/lib.rs line 436 at r2 (raw file):

        setup_pfioc_state_kill(state.as_raw(), &mut pfioc_state_kill);
        ioctl_guard!(ffi::pf_kill_states(self.fd(), &mut pfioc_state_kill))?;
        // psk_af holds the number of killed states, but it should be zero

Why zero? Would it not match zero or one states? Zero if the state was already gone at the time of issuing this call, or one if it was removed?

src/lib.rs line 445 at r2 (raw file):

        if num_states > 0 {
            let (mut pfioc_states, pfsync_states) = setup_pfioc_states(num_states);
            ioctl_guard!(ffi::pf_get_states(self.fd(), &mut pfioc_states))?;

Does this have a race? What happens if the number of states changes between the call to get number of states and the syscall to get the states?

Not fully related to this PR, but I noticed it now. Can we improve it to have retry logic or in some other way not fail in this way? Once we start using this feature to list and kill states we will be calling this method much more often, and be more likely to trigger the race, if there is one.

src/state.rs line 8 at r2 (raw file):

/// PF connection state created by a stateful rule
#[derive(Clone)]

Debug would be nice. I'm not sure how it would be automatically implemented, but good practice to have Debug on all public types.

src/state.rs line 25 at r2 (raw file):

    /// Return the transport protocol for this state
    pub fn proto(&self) -> Result<Proto> {
        Proto::try_from(self.sync_state.direction)

Typo? You pass in sync_state.direction and not the proto. Something we could catch with a unit test! Can you consider adding unit tests for this type? That would also help to make sure the endianness ninjaing in parse_address is correct and stays correct during refactoring.

src/state.rs line 54 at r2 (raw file):

        _ => return Err(Error::from(ErrorInternal::InvalidAddressFamily(family))),
    };
    let port = unsafe { host.xport.port }.to_be();

Should this not be from_be? 🤔 Is host.xport.port not stored in big endian? SocketAddr::new below wants the port in native-endianness.

src/rule/direction.rs line 36 at r2 (raw file):

        const INOUT: u8 = ffi::pfvar::PF_INOUT as u8;
        const IN: u8 = ffi::pfvar::PF_IN as u8;
        const OUT: u8 = ffi::pfvar::PF_OUT as u8;

We should probably not duplicate these. They are already defined and casted above.

Can we set #[repr(u8)] on the enum and assign these constants directly on the Rust variants? Then we can simply use our own enum when converting/matching.

Same goes for the other enums you implement TryFrom<u8> in this PR. Now when we go both ways, we should probably find a way to not duplicate a lot of FFI constants and their casting. Since your PR goes from needing them in a single place to two places, this work sort of lands on this PR.

faern

Reviewable status: 4 of 5 files reviewed, 8 unresolved discussions (waiting on @dlon)

src/lib.rs line 435 at r2 (raw file):

        let mut pfioc_state_kill = unsafe { mem::zeroed::<ffi::pfvar::pfioc_state_kill>() };
        setup_pfioc_state_kill(state.as_raw(), &mut pfioc_state_kill);
        ioctl_guard!(ffi::pf_kill_states(self.fd(), &mut pfioc_state_kill))?;

Was it possible to use some kind of ID from the state? To ensure we only remove one state, and not all matching states? 🤔

faern

When we have agreed on the API, can you please 1) Update the PR description and 2) Add this feature to the changelog?

Reviewable status: 4 of 5 files reviewed, 8 unresolved discussions (waiting on @dlon)

dlon

Reviewable status: 2 of 11 files reviewed, 8 unresolved discussions (waiting on @faern)

src/lib.rs line 435 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Was it possible to use some kind of ID from the state? To ensure we only remove one state, and not all matching states? 🤔

I looked into it and DIOCKILLSTATES does not check any kind of state ID: https://github.com/apple/darwin-xnu/blob/main/bsd/net/pf_ioctl.c#L3403

But I believe a (interface, protocol, local IP, local port, remote IP, remote port) tuple will be unique anyway?

src/lib.rs line 436 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Why zero? Would it not match zero or one states? Zero if the state was already gone at the time of issuing this call, or one if it was removed?

I removed the comment, since we don't even refer to the field.

src/state.rs line 8 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Debug would be nice. I'm not sure how it would be automatically implemented, but good practice to have Debug on all public types.

Unfortunately, pfsync_state contains unions, so the Debug macro didn't work. I reran bindgen with --disable-untagged-union. But the output is not particularly useful, IMO. Maybe just manually implementing Debug would be fine?

src/state.rs line 54 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Should this not be from_be? 🤔 Is host.xport.port not stored in big endian? SocketAddr::new below wants the port in native-endianness.

You're right! Fixed.

faern

Reviewed 1 of 8 files at r3.
Reviewable status: 3 of 11 files reviewed, 6 unresolved discussions (waiting on @dlon)

src/lib.rs line 435 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

I looked into it and DIOCKILLSTATES does not check any kind of state ID: https://github.com/apple/darwin-xnu/blob/main/bsd/net/pf_ioctl.c#L3403

But I believe a (interface, protocol, local IP, local port, remote IP, remote port) tuple will be unique anyway?

Ok. Maybe it will. I have not dived into the details. Given your question mark you don't sound sure either 😉 But yeah, at least I guess we can assume only states matching all the fields. If there happens to be >1 state matching then at least the user already told us to remove those state(s). So I guess it's fine. Let's move on!

src/lib.rs line 431 at r3 (raw file):
Please add information on how to obtain a &State. Otherwise it's hard to know how to use this method. Something like:

/// Remove the specified state.
///
/// All current states can be obtained via [get_states].

src/state.rs line 8 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Unfortunately, pfsync_state contains unions, so the Debug macro didn't work. I reran bindgen with --disable-untagged-union. But the output is not particularly useful, IMO. Maybe just manually implementing Debug would be fine?

I think we should revert the changes to the FFI. It became a very large change, and primarily just more complicated types. Either leave Debug out, or implement it manually IMO.

src/state.rs line 56 at r3 (raw file):

        _ => return Err(Error::from(ErrorInternal::InvalidAddressFamily(family))),
    };
    let port = u16::from_be(unsafe { *host.xport.port.as_ref() });

This function has a lot of unsafe. But no safety docs. Is it possible to cause UB by calling this function with carefully crafted bad arguments? If so, the function needs to be unsafe.

faern

Reviewed 1 of 8 files at r3.
Reviewable status: 4 of 11 files reviewed, 7 unresolved discussions (waiting on @dlon)

generate_bindings.sh line 25 at r3 (raw file):

    --allowlist-var PFRULE_.* \
    --default-enum-style rust \
    --disable-untagged-union \

I don't think we should change this unless we have a good reason. Currently it mostly looks like it messes a lot with the library code, for very little gain, if any.

dlon

Reviewable status: 0 of 11 files reviewed, 6 unresolved discussions (waiting on @faern)

src/lib.rs line 92 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

The state module only has a single type. So this is fine. But otherwise I suggest we stop re-exporting everything from all modules like this. The main namespace of this crate is growing pretty big. Having a bit of a tree structure would probably clean it up a bit. But again, not important here since it's only one item.

Agreed that we should not do this in general, but I think it's more important to be consistent. This would be a breaking change, though.

src/lib.rs line 188 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I think we should use a better name than reason here. These are not reasons. These are address_family, direction and protocol respectively.

Done.

src/lib.rs line 445 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Does this have a race? What happens if the number of states changes between the call to get number of states and the syscall to get the states?

Not fully related to this PR, but I noticed it now. Can we improve it to have retry logic or in some other way not fail in this way? Once we start using this feature to list and kill states we will be calling this method much more often, and be more likely to trigger the race, if there is one.

Yeah, this is definitely wrong! Retrying while the buffer is too small is also what pfctl does: https://github.com/openbsd/src/blob/master/sbin/pfctl/pfctl.c#L1047

src/state.rs line 8 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I think we should revert the changes to the FFI. It became a very large change, and primarily just more complicated types. Either leave Debug out, or implement it manually IMO.

Added a manual implementation.

src/state.rs line 25 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Typo? You pass in sync_state.direction and not the proto. Something we could catch with a unit test! Can you consider adding unit tests for this type? That would also help to make sure the endianness ninjaing in parse_address is correct and stays correct during refactoring.

This is now covered both by unit tests and the integration tests I've added.

src/state.rs line 56 at r3 (raw file):

Previously, faern (Linus Färnstrand) wrote…

This function has a lot of unsafe. But no safety docs. Is it possible to cause UB by calling this function with carefully crafted bad arguments? If so, the function needs to be unsafe.

It should always be safe, because the struct is zero-initialized. But it's not always meaningful. Commented to clarify.

src/rule/direction.rs line 36 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

We should probably not duplicate these. They are already defined and casted above.

Can we set #[repr(u8)] on the enum and assign these constants directly on the Rust variants? Then we can simply use our own enum when converting/matching.

Same goes for the other enums you implement TryFrom<u8> in this PR. Now when we go both ways, we should probably find a way to not duplicate a lot of FFI constants and their casting. Since your PR goes from needing them in a single place to two places, this work sort of lands on this PR.

Done.

generate_bindings.sh line 25 at r3 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I don't think we should change this unless we have a good reason. Currently it mostly looks like it messes a lot with the library code, for very little gain, if any.

Reverted.

faern

Reviewed 10 of 11 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dlon)

src/lib.rs line 445 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Yeah, this is definitely wrong! Retrying while the buffer is too small is also what pfctl does: https://github.com/openbsd/src/blob/master/sbin/pfctl/pfctl.c#L1047

Ok. Let's leave that for a separate PR! But this needs fixing. I created an issue: #109

src/state.rs line 25 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

This is now covered both by unit tests and the integration tests I've added.

Really nice!

src/state.rs line 56 at r3 (raw file):

because the struct is zero-initialized

This is not something this function guarantees. A function's safety contract has to be upheld by that function itself, not how it's called by others.

Maybe this is safe... What guarantees does Rust give about the unused memory of a union?

If I have

union Foo {
    small: u8,
    large: u128,
}

And I initialize a Foo { small: 0 }. Is it then safe to read foo.large? If so, this function is safe. Otherwise not. Because here we read host.addr.pfa._v6addr. This is not the smallest field in the enum, so what happens when someone calls parse_address with a pfa initialized to a smaller union variant than v6?

src/rule/direction.rs line 18 at r5 (raw file):

    Any = ffi::pfvar::PF_INOUT as u32 as u8,
    In = ffi::pfvar::PF_IN as u32 as u8,
    Out = ffi::pfvar::PF_OUT as u32 as u8,

Why must this be cast twice? The code that is removed in this PR did not have to do the double cast 🤔

tests/states.rs line 161 at r5 (raw file):

        .expect("Could not obtain states")
        .into_iter()
        .filter_map(|state| ExpectedState::try_from(state).ok()).collect::<Vec<_>>();

This construct is nice! May I suggest you use it above in the previous pf.get_states() also? That makes the code inside the for loop simpler (currently pretty hard to read it all since it does a lot in one go)

faern

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dlon)

src/state.rs line 56 at r3 (raw file):

Previously, faern (Linus Färnstrand) wrote…

because the struct is zero-initialized

This is not something this function guarantees. A function's safety contract has to be upheld by that function itself, not how it's called by others.

Maybe this is safe... What guarantees does Rust give about the unused memory of a union?

If I have
union Foo {
    small: u8,
    large: u128,
}
And I initialize a Foo { small: 0 }. Is it then safe to read foo.large? If so, this function is safe. Otherwise not. Because here we read host.addr.pfa._v6addr. This is not the smallest field in the enum, so what happens when someone calls parse_address with a pfa initialized to a smaller union variant than v6?

Valgrind does not like the following program:

fn main() {
    let foo = Foo { small: 123 };
    println!("{}", unsafe { foo.large });
}

union Foo {
    small: u8,
    large: u128,
}

But it does not trigger any errors if foo is initialized as Foo { large: 123 };. So yeah, The current implementation of parse_address is unsafe as far as I can tell.

faern

Reviewable status: 10 of 11 files reviewed, 5 unresolved discussions (waiting on @dlon)

src/state.rs line 44 at r6 (raw file):
Good documentation on parse_address!

But now this method makes a lot of assumptions that has nothing with local_address to do. When you (as discussed AFK) add unsafety to State::new and explain why it's unsafe, then this safety documentation can be something like:

If the safety contract in State::new is upheld, sync_state.lan is guaranteed to be initialized according to the sync_state.af_lan family.

src/state.rs line 131 at r6 (raw file):

        let family = u8::from(AddrFamily::Ipv6);

        assert_matches!(unsafe { parse_address(family, host) }, Ok(addr) if addr == SocketAddr::new(EXPECTED_IP.into(), EXPECTED_PORT));

Nit. But instead of making this line super long. I would probably have a line before it with let address = unsafe { parse_address(family, host) }.unwrap();. This splits the complexity into two statements.

dlon

Reviewable status: 8 of 11 files reviewed, 3 unresolved discussions (waiting on @faern)

src/lib.rs line 431 at r3 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Please add information on how to obtain a &State. Otherwise it's hard to know how to use this method. Something like:

/// Remove the specified state.
///
/// All current states can be obtained via [get_states].

Done.

src/state.rs line 44 at r6 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Good documentation on parse_address!

But now this method makes a lot of assumptions that has nothing with local_address to do. When you (as discussed AFK) add unsafety to State::new and explain why it's unsafe, then this safety documentation can be something like:

If the safety contract in State::new is upheld, sync_state.lan is guaranteed to be initialized according to the sync_state.af_lan family.

Done. Would it be acceptable to simply state that the object passed to new must be zero-initialized? I think that covers all cases without being overly verbose.

src/state.rs line 131 at r6 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Nit. But instead of making this line super long. I would probably have a line before it with let address = unsafe { parse_address(family, host) }.unwrap();. This splits the complexity into two statements.

Done. Much better.

src/rule/direction.rs line 18 at r5 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Why must this be cast twice? The code that is removed in this PR did not have to do the double cast 🤔

Done.

tests/states.rs line 161 at r5 (raw file):

Previously, faern (Linus Färnstrand) wrote…

This construct is nice! May I suggest you use it above in the previous pf.get_states() also? That makes the code inside the for loop simpler (currently pretty hard to read it all since it does a lot in one go)

I didn't do that since the original state is needed for kill_state.

faern

Reviewed 2 of 3 files at r7, 2 of 2 files at r8, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion

src/state.rs line 44 at r6 (raw file):

Previously, dlon (David Lönnhager) wrote…

Done. Would it be acceptable to simply state that the object passed to new must be zero-initialized? I think that covers all cases without being overly verbose.

Much simpler! Let's keep it simple :P

faern reviewed Jul 2, 2024

View reviewed changes

dlon force-pushed the improve-state-flush branch from 0cae587 to a565819 Compare July 23, 2024 10:46

dlon changed the title ~~Add connection state filter method~~ Add connection state methods Jul 23, 2024

faern reviewed Jul 23, 2024

View reviewed changes

dlon commented Jul 23, 2024

View reviewed changes

faern reviewed Jul 23, 2024

View reviewed changes

dlon force-pushed the improve-state-flush branch from d4d2b76 to 3d51caf Compare July 24, 2024 09:32

dlon commented Jul 24, 2024

View reviewed changes

faern reviewed Jul 24, 2024

View reviewed changes

dlon marked this pull request as ready for review July 24, 2024 11:26

faern requested changes Jul 24, 2024

View reviewed changes

dlon force-pushed the improve-state-flush branch from ab22368 to c336c51 Compare July 24, 2024 12:37

dlon commented Jul 24, 2024

View reviewed changes

dlon force-pushed the improve-state-flush branch from fe38bd8 to a5ed883 Compare July 24, 2024 12:58

faern approved these changes Jul 24, 2024

View reviewed changes

dlon added 3 commits July 24, 2024 16:24

Add methods for listing and removing states

e246607

Add tests for handling PF states

3813dd6

Update changelog

7bae6e8

dlon force-pushed the improve-state-flush branch from a5ed883 to 7bae6e8 Compare July 24, 2024 14:25

dlon merged commit 53a9c95 into main Jul 24, 2024
8 of 9 checks passed

dlon deleted the improve-state-flush branch July 24, 2024 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add connection state methods #106

Add connection state methods #106

dlon commented Jun 28, 2024 •

edited

Loading

faern left a comment

faern left a comment

faern left a comment

faern left a comment

faern left a comment

dlon left a comment

faern left a comment

faern left a comment

dlon left a comment

faern left a comment

faern left a comment

faern left a comment

dlon left a comment

faern left a comment

Add connection state methods #106

Add connection state methods #106

Conversation

dlon commented Jun 28, 2024 • edited Loading

faern left a comment

Choose a reason for hiding this comment

faern left a comment

Choose a reason for hiding this comment

faern left a comment

Choose a reason for hiding this comment

faern left a comment

Choose a reason for hiding this comment

faern left a comment

Choose a reason for hiding this comment

dlon left a comment

Choose a reason for hiding this comment

faern left a comment

Choose a reason for hiding this comment

faern left a comment

Choose a reason for hiding this comment

dlon left a comment

Choose a reason for hiding this comment

faern left a comment

Choose a reason for hiding this comment

faern left a comment

Choose a reason for hiding this comment

faern left a comment

Choose a reason for hiding this comment

dlon left a comment

Choose a reason for hiding this comment

faern left a comment

Choose a reason for hiding this comment

dlon commented Jun 28, 2024 •

edited

Loading