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

Add support for netblocks to CookiesMiddlewareProcessor. #340

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

ximon18
Copy link
Member

@ximon18 ximon18 commented Jun 18, 2024

This PR adds support (via new dependency ipnetwork) for net blocks in the deny list of the cookie middleware processor, ala Unbound, otherwise the deny list is difficult to use beyond a few IP addresses.

…s test pass:

- Allow requests with invalid cookies to proceed if they are authenticated or not required to authenticate.

Improvements to the Stelline server test support needed by the Stelline cookies test:
- Advance mock system time in the Stelline server tests.
- Move Stelline server tests under src/ to permit #cfg(test) based swap out of real system time for mock system time.
- Use the thread_local version of mock_instant to ensure parallel mock time dependent tests don't interfere with each other (such tests also use tokio::time which only works if they run in a single thread).
- Set mock system time to start at zero for each Stelline server test (as expected by the cookies .rpl test script).
- Pass the IP address of the test client to the server so that the cookies middleware can match it against its deny list.

Other:
- Add support for net blocks in the IP deny list of the cookie middleware processor, ala Unbound, otherwise the deny list is difficult to use beyond a few simple speciifc IP addresses.
- Enable tracing log output in the server Stelline tests.
- Move unit server tests and Stelline server tests together under src/net/server/tests/.
- Additional logging.
…use a task based waker instead of a thread based waker for scenarios where CPU resource is low such as in GH Actions (or when simualted with taskset -c 0).
This reverts commit dd226dc.
@ximon18 ximon18 added the enhancement New feature or request label Jun 18, 2024
@ximon18 ximon18 requested a review from a team June 18, 2024 22:41
@partim
Copy link
Member

partim commented Jun 19, 2024

You might want to consider using inetnum rather than ipnetwork – and add things that are potentially missing.

@ximon18
Copy link
Member Author

ximon18 commented Jun 19, 2024

You might want to consider using inetnum rather than ipnetwork – and add things that are potentially missing.

Nice idea, but inetnum seems to lack all of the needed functionality.

@ximon18
Copy link
Member Author

ximon18 commented Jun 19, 2024

You might want to consider using inetnum rather than ipnetwork – and add things that are potentially missing.

Nice idea, but inetnum seems to lack all of the needed functionality.

My apoloiges @partim, of course it has what is needed, I just didn't see it as it is called Prefix rather than network or netblock etc.

I have updated this PR to use inetnum. I did note one regression compared to using ipnetwork which is that FromStr for Prefix is less accepting than that of ipnetwork as it will reject 127.0.0.1/24 as it has non-zero host bits. For end users however while stricter it still provides the functionality that's actually needed.

@ximon18
Copy link
Member Author

ximon18 commented Jun 20, 2024

You might want to consider using inetnum rather than ipnetwork – and add things that are potentially missing.

Nice idea, but inetnum seems to lack all of the needed functionality.

My apoloiges @partim, of course it has what is needed, I just didn't see it as it is called Prefix rather than network or netblock etc.

I have updated this PR to use inetnum. I did note one regression compared to using ipnetwork which is that FromStr for Prefix is less accepting than that of ipnetwork as it will reject 127.0.0.1/24 as it has non-zero host bits. For end users however while stricter it still provides the functionality that's actually needed.

I've updated the PR again to use its own FromStr impl that handles the bare IP address case which FromStr in inetnum rejects as lacking a prefix len. @partim and I briefly discussed extending the support in inetnum but that snowballed into a larger discussion which is parked for the moment, instead this is the pragmatic step forward for now.

@ximon18 ximon18 changed the base branch from fix-stelline-cookies-test to main June 27, 2024 10:01
/// If the given IP address is on our deny list it is required to
/// authenticate itself.
fn must_authenticate(&self, ip: IpAddr) -> bool {
self.deny_list.iter().any(|netblock| netblock.contains(ip))
Copy link
Member Author

Choose a reason for hiding this comment

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

This might not be very efficient for larger numbers of blocks. Maybe use (optionally, i.e. behind a feature gate) https://docs.rs/rotonda-store/latest/rotonda_store/struct.MultiThreadedStore.html here?

Also, looking up configuration based on incoming IP address is also needed for handling incoming XFR requests, so maybe this functionality should be moved to a common module under src/net/?

@partim? @density215?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thinking about this a bit more, the lookup result when using a simple vec of netblocks is dependent on the order they are added, so it's not just about efficiency, a proper prefix store will also return the best match rather than just the first match.

Copy link
Member

Choose a reason for hiding this comment

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

There’s probably also going to be rate limiting code that needs to work on prefixes, so having something more general may be useful. Would it make sense to do something generic here? Or are we happy to settle on rotonda-store at least under the hood?

Copy link
Member Author

@ximon18 ximon18 Oct 3, 2024

Choose a reason for hiding this comment

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

Maybe this needs turning around like was done for XFR. For XFR the code used to apply access control but there was quite a lot of configuration and exactly how you want to restrict access is perhaps not something we should decide here. So for XFR I moved the checking out to an XfrDataProvider trait that the middleware uses a user supplied impl of. The only sample impls of XfrDataProvider actually don't do any access control at all, that's left to the end user. @partim: Should we maybe do the same here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request unstable feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants