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

mirrord policy for enforcing header pattern #3048

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
2 changes: 1 addition & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions changelog.d/+operator-policy-http-filter.internal.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add mirrord policy support for specifying pattern requirment for header filter when performing steal-with-filter.
34 changes: 34 additions & 0 deletions mirrord/operator/src/crd/policy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ pub struct MirrordPolicySpec {
/// the user config.
#[serde(default)]
pub fs: FsPolicy,

/// Fine grained control over network features like specifiying required http filters.
#[serde(default)]
pub network: NetworkPolicy,
}

/// Custom cluster-wide resource for policies that limit what mirrord features users can use.
Expand Down Expand Up @@ -100,6 +104,9 @@ pub struct MirrordClusterPolicySpec {
/// the user config.
#[serde(default)]
pub fs: FsPolicy,

#[serde(default)]
pub network: NetworkPolicy,
}

/// Policy for controlling environment variables access from mirrord instances.
Expand Down Expand Up @@ -150,6 +157,33 @@ pub struct FsPolicy {
pub not_found: HashSet<String>,
}

/// Network operations policy that partialy mimics the mirrord network config.
#[derive(Clone, Default, Debug, Deserialize, Eq, PartialEq, Serialize, JsonSchema)]
#[serde(rename_all = "camelCase")]
pub struct NetworkPolicy {
#[serde(default)]
pub incoming: IncomingNetworkPolicy,
}

/// Incoming network operations policy that partialy mimics the mirrord network.incoming config.
DmitryDodzin marked this conversation as resolved.
Show resolved Hide resolved
#[derive(Clone, Default, Debug, Deserialize, Eq, PartialEq, Serialize, JsonSchema)]
#[serde(rename_all = "camelCase")]
pub struct IncomingNetworkPolicy {
#[serde(default)]
pub http_filter: HttpFilterPolicy,
}

/// Http filter policy that allows to specify any filter requirments that users must specify in
/// their config for a successful network steal
DmitryDodzin marked this conversation as resolved.
Show resolved Hide resolved
#[derive(Clone, Default, Debug, Deserialize, Eq, PartialEq, Serialize, JsonSchema)]
#[serde(rename_all = "camelCase")]
pub struct HttpFilterPolicy {
/// Require user's filter to match this regex if filter is provided. (this works in tandom with
/// `steal-without-filter` block to require the user to specify a header filter for network
/// steal feature)
DmitryDodzin marked this conversation as resolved.
Show resolved Hide resolved
pub header_filter: Option<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

What if multiple filters are used? Right now the user can specify any_of/all_of filters.

I think the semantics should be that an HTTP filter is accepted only when it has a header filter matching this pattern, and this header filter is required to match for the request to be stolen (so any_of(some-path-filter, valid-header-filter) would not pass the policy check). If so, this should probably be named like requires_header_filter or required_header_filter, and the doc should mention the case of composed filters.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is also the possibility of multiple rules being applied on single target so making a correct regex can be tricky

}

#[test]
fn check_one_api_group() {
use kube::Resource;
Expand Down
2 changes: 1 addition & 1 deletion mirrord/protocol/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "mirrord-protocol"
version = "1.16.1"
version = "1.17.0"
authors.workspace = true
description.workspace = true
documentation.workspace = true
Expand Down
16 changes: 14 additions & 2 deletions mirrord/protocol/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,11 @@ pub enum ResponseError {
#[error("Operation is not yet supported by mirrord.")]
NotImplemented,

#[error("{blocked_action} is forbidden by {} for this target (your organization does not allow you to use this mirrord feature with the chosen target).", policy_name_string(.policy_name.clone()))]
#[error("{blocked_action} is forbidden by {} for this target ({}).", policy_name_string(.policy_name.as_deref()), policy_reason(.reason.as_deref()))]
Forbidden {
blocked_action: BlockedAction,
policy_name: Option<String>,
reason: Option<String>,
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks compatibility, we can't add new fields.

Copy link
Member Author

Choose a reason for hiding this comment

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

It should be fine because it's at the end of all messages that contain this result and it looks working for old layer > new operator and new layer > old operator

Copy link
Member Author

Choose a reason for hiding this comment

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

Like we don't have any message where RemoteResult isn't the last field (well all the places it's the only field)

Copy link
Contributor

Choose a reason for hiding this comment

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

This test mocks new intproxy and old operator, decoding the first message fails :<

#[test]
fn compat_test() {
    #[derive(Encode, Decode, PartialEq, Eq, Debug)]
    enum ResponseErrorOld {
        IdsExhausted(String),
        NotFound(u64),
        NotDirectory(u64),
        NotFile(u64),
        RemoteIO(RemoteIOError),
        DnsLookup(DnsLookupError),
        Remote(RemoteError),
        PortAlreadyStolen(Port),
        NotImplemented,
        Forbidden {
            blocked_action: BlockedAction,
            policy_name: Option<String>,
        },
        StripPrefix(String),
        OpenLocal,
    }

    #[derive(Encode, Decode, PartialEq, Eq, Debug)]
    enum ResponseErrorNew {
        IdsExhausted(String),
        NotFound(u64),
        NotDirectory(u64),
        NotFile(u64),
        RemoteIO(RemoteIOError),
        DnsLookup(DnsLookupError),
        Remote(RemoteError),
        PortAlreadyStolen(Port),
        NotImplemented,
        Forbidden {
            blocked_action: BlockedAction,
            policy_name: Option<String>,
            reason: Option<String>,
        },
        StripPrefix(String),
        OpenLocal,
    }

    let mut codec_old_sender = ProtocolCodec::<(), ResponseErrorOld>::default();
    let mut codec_new_receiver = ProtocolCodec::<ResponseErrorNew, ()>::default();
    let mut buffer = BytesMut::default();

    codec_old_sender
        .encode(
            ResponseErrorOld::Forbidden {
                blocked_action: BlockedAction::Mirror(80),
                policy_name: Some("name_1".into()),
            },
            &mut buffer,
        )
        .unwrap();
    codec_old_sender
        .encode(
            ResponseErrorOld::Forbidden {
                blocked_action: BlockedAction::Steal(StealType::All(99)),
                policy_name: Some("name_2".into()),
            },
            &mut buffer,
        )
        .unwrap();

    let error_1 = codec_new_receiver.decode(&mut buffer).unwrap();
    assert_eq!(
        error_1,
        Some(ResponseErrorNew::Forbidden {
            blocked_action: BlockedAction::Mirror(80),
            policy_name: Some("name_1".into()),
            reason: None
        })
    );

    let error_2 = codec_new_receiver.decode(&mut buffer).unwrap();
    assert_eq!(
        error_2,
        Some(ResponseErrorNew::Forbidden {
            blocked_action: BlockedAction::Steal(StealType::All(99)),
            policy_name: Some("name_2".into()),
            reason: None
        })
    );
}

},

#[error("Failed stripping path with `{0}`!")]
Expand All @@ -79,18 +80,29 @@ impl From<StripPrefixError> for ResponseError {
}

/// If some then the name with a trailing space, else empty string.
fn policy_name_string(policy_name: Option<String>) -> String {
fn policy_name_string(policy_name: Option<&str>) -> String {
if let Some(name) = policy_name {
format!("the mirrord policy \"{name}\"")
} else {
"a mirrord policy".to_string()
}
}

fn policy_reason(reason: Option<&str>) -> String {
reason
.unwrap_or("your organization does not allow you to use this mirrord feature with the chosen target")
.into()
}

/// Minimal mirrord-protocol version that allows [`BlockedAction::Mirror`].
pub static MIRROR_BLOCK_VERSION: LazyLock<VersionReq> =
LazyLock::new(|| ">=1.12.0".parse().expect("Bad Identifier"));

/// Minimal mirrord-protocol version that allows [`ResponseError::Forbidden`] to have `reason`
/// member.
pub static MIRROR_POLICY_REASON_VERSION: LazyLock<VersionReq> =
LazyLock::new(|| ">=1.17.0".parse().expect("Bad Identifier"));

/// All the actions that can be blocked by the operator, to identify the blocked feature in a
/// [`ResponseError::Forbidden`] message.
#[derive(Encode, Decode, Debug, PartialEq, Clone, Eq, Error)]
Expand Down
8 changes: 8 additions & 0 deletions mirrord/protocol/src/tcp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,14 @@ impl Display for Filter {
}
}

impl std::ops::Deref for Filter {
type Target = str;

fn deref(&self) -> &Self::Target {
&self.0
}
}

/// Describes different types of HTTP filtering available
#[derive(Encode, Decode, Debug, PartialEq, Eq, Clone)]
pub enum HttpFilter {
Expand Down
7 changes: 7 additions & 0 deletions tests/src/operator/policies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ fn block_steal_without_qualifiers() -> PolicyTestCase {
block: vec![BlockedFeature::Steal],
env: Default::default(),
fs: Default::default(),
network: Default::default(),
},
),
service_b_can_steal: No,
Expand All @@ -141,6 +142,7 @@ fn block_steal_with_path_pattern() -> PolicyTestCase {
block: vec![BlockedFeature::Steal],
env: Default::default(),
fs: Default::default(),
network: Default::default(),
},
),
service_b_can_steal: EvenWithoutFilter,
Expand All @@ -161,6 +163,7 @@ fn block_unfiltered_steal_with_path_pattern() -> PolicyTestCase {
block: vec![BlockedFeature::StealWithoutFilter],
env: Default::default(),
fs: Default::default(),
network: Default::default(),
},
),
service_b_can_steal: EvenWithoutFilter,
Expand All @@ -181,6 +184,7 @@ fn block_unfiltered_steal_with_deployment_path_pattern() -> PolicyTestCase {
block: vec![BlockedFeature::StealWithoutFilter],
env: Default::default(),
fs: Default::default(),
network: Default::default(),
},
),
service_a_can_steal: OnlyWithFilter,
Expand All @@ -207,6 +211,7 @@ fn block_steal_with_label_selector() -> PolicyTestCase {
block: vec![BlockedFeature::Steal],
env: Default::default(),
fs: Default::default(),
network: Default::default(),
},
),
service_b_can_steal: EvenWithoutFilter,
Expand Down Expand Up @@ -234,6 +239,7 @@ fn block_steal_with_unmatching_policy() -> PolicyTestCase {
block: vec![BlockedFeature::Steal],
env: Default::default(),
fs: Default::default(),
network: Default::default(),
},
),
service_b_can_steal: EvenWithoutFilter,
Expand Down Expand Up @@ -376,6 +382,7 @@ pub async fn create_cluster_policy_and_try_to_mirror(
block: vec![BlockedFeature::Mirror],
env: Default::default(),
fs: Default::default(),
network: Default::default(),
},
),
)
Expand Down
1 change: 1 addition & 0 deletions tests/src/operator/policies/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ pub async fn create_namespaced_fs_policy_and_try_file_open(
local: HashSet::from_iter(vec!["file\\.local".to_string()]),
not_found: HashSet::from_iter(vec!["file\\.not-found".to_string()]),
},
network: Default::default(),
},
),
&service.namespace,
Expand Down
Loading