[TrafficControl] Support allowlisting #19242
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
williampsmith
force-pushed
the
william/allowlist-policy
branch
from
06b20f5
to
8d2eec6
Compare
williampsmith
force-pushed
the
william/allowlist-policy
branch
from
ee44d72
to
4b85a11
Compare
| .ok() | ||
| .map(|socket_addr| socket_addr.ip()) | ||
| .or_else(|| { | ||
| error!("Failed to parse value of {:?} to ip address or socket.", ip,); |
There was a problem hiding this comment.
| error!("Failed to parse value of {:?} to ip address or socket.", ip,); | |
| error!("Failed to parse value of {:?} to ip address and socket.", ip); |
There was a problem hiding this comment.
Actually this is correct. The parsing can take either an ip address or a socket object (ip + port). Will fix the trailing comma in the other PR since it's minor and want to get this landed
|
|
||
| match &self.acl { | ||
| Acl::Allowlist(allowlist) => { | ||
| let allowed = client.is_none() || allowlist.contains(&client.unwrap()); |
There was a problem hiding this comment.
Yeah the client should never be None in the happy path. This is just edge case handling for the case where the client parsing fails earlier in the flow. In such a case, we don't want to panic, since there have been some interesting cases in the past where this has failed in production after rollout, so we just set the client to None and essentially allow everything. There's logging and a metric that we bump (again, earlier in the flow) if that happens though so that we can investigate
There was a problem hiding this comment.
I wonder when it's None and it's in allowlist mode whether we should reject it as opposed to let it pass. Will leave it up to you
There was a problem hiding this comment.
I don't think there's a way that this can be made None by an attacker. It would generally mean that we have a bug in parsing, or traffic controller on the server itself is misconfigured. So it feels safer in this case to allow. If it's a bug, then we definitely don't want to be sinking all requests on the node, and if it's a misconfigured server, then that should be isolated to just that one node, so it should be safe from the perspective of the bridge network to just allow it. The worst case scenario if we reject in this case is that it's a bug that affects all nodes, and we effectively halt the bridge.
|
and @williampsmith just to make sure we are on the same page - as your summarized in that slack channel, we will use key based allowlist v.s. ip based (what this PR is about). But the latter will be useful so we will add it any way. Is this correct? |
Yes, we'll be using key based allowlist. The integration here is not concerned with that, and the generalization to support that will happen in a followup PR |
Description
In some cases, we may want to enable a more restrictive policy wherein the node must explicitly specify all IP's from which it will accept requests. Because
TrafficControllerpolicies do not easily support allowlisting, this is instead supported by introducing a separate mode of operation for traffic controller, enabled by providingallow_listfield in thePolicyConfig, which should map to a list of strings all parseable toIpAddr.When this config is present, we skip spawning a tally thread and ignore all calls to
TrafficController::tally, and instead initialize an in memory allowlist of IPs. On subsequent calls toTrafficController::check, we check this list against the requestor IP.Dry run mode in this mode still works as expected, as do block metrics (any request that is not in the allowlist is tallied against the block metric).
Test plan
Added simtest
Release notes
Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.
For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.