support xdp auth in tailcall #985

weli-l · 2024-10-26T07:52:35Z

What type of PR is this?

What this PR does / why we need it:
support xdp auth in tail call
Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

codecov · 2024-10-26T08:10:29Z

Codecov Report

Attention: Patch coverage is 15.78947% with 48 lines in your changes missing coverage. Please review.

Project coverage is 48.91%. Comparing base (f14f7d2) to head (e22e641).
Report is 31 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/status/status_server.go	0.00%	34 Missing ⚠️
pkg/bpf/workload/xdp.go	50.00%	6 Missing and 3 partials ⚠️
pkg/bpf/bpf.go	0.00%	4 Missing ⚠️
pkg/controller/controller.go	0.00%	1 Missing ⚠️

Files with missing lines	Coverage Δ
pkg/controller/controller.go	`0.00% <0.00%> (ø)`
pkg/bpf/bpf.go	`37.30% <0.00%> (-0.15%)`	⬇️
pkg/bpf/workload/xdp.go	`48.27% <50.00%> (-0.30%)`	⬇️
pkg/status/status_server.go	`35.48% <0.00%> (-3.46%)`	⬇️

... and 2 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6cdc638...e22e641. Read the comment docs.

lec-bit · 2024-10-26T08:47:42Z

/retest

LiZhenCheng9527 · 2024-10-26T08:15:30Z

pkg/bpf/workload/xdp.go

+		uint32(constants.TailCallPolicyCheck),
+		uint32(xa.PolicyCheck.FD()),
+		ebpf.UpdateAny); err != nil {
+		return err


In the case where only error is returned, is it possible to know if it is a policyCheck error or a ruleCheck error?

please add some more error info

weli-l · 2024-10-27T04:54:59Z

/retest

hzxuzhonghu · 2024-10-28T03:58:09Z

bpf/kmesh/workload/include/authz.h

+    __uint(key_size, sizeof(struct bpf_sock_tuple));
+    __uint(value_size, sizeof(struct match_ctx));
+    __uint(max_entries, 256);
+} tailcall_info_map SEC(".maps");


suggest add kmesh prefix

Add __uint(map_flags, BPF_F_NO_PREALLOC);

Is 256 enough? I think we should greatly increase to support higher throughput if BPF_F_NO_PREALLOC set

hzxuzhonghu · 2024-10-28T04:04:28Z

bpf/kmesh/workload/include/authz.h

+
+    // Safely access policyId and check if the policy exists
+    if (bpf_probe_read_kernel(&policyId, sizeof(policyId), (void *)(policies->policyIds + res->policy_index)) != 0) {
+        BPF_LOG(ERR, AUTH, "Failed to read policyId, thrown it to user auth");


Suggested change

BPF_LOG(ERR, AUTH, "Failed to read policyId, thrown it to user auth");

BPF_LOG(ERR, AUTH, "Failed to read policyId, throw it to user auth");

hzxuzhonghu · 2024-10-28T04:04:57Z

bpf/kmesh/workload/include/authz.h

+    if (bpf_probe_read_kernel(&policyId, sizeof(policyId), (void *)(policies->policyIds + res->policy_index)) != 0) {
+        BPF_LOG(ERR, AUTH, "Failed to read policyId, thrown it to user auth");
+        if (bpf_map_delete_elem(&tailcall_info_map, &tuple_key) != 0) {
+            BPF_LOG(DEBUG, AUTH, "Failed to delete context from map");


Suggested change

BPF_LOG(DEBUG, AUTH, "Failed to delete context from map");

BPF_LOG(DEBUG, AUTH, "Failed to delete tailcall context from map");

hzxuzhonghu · 2024-10-28T06:12:04Z

bpf/kmesh/workload/include/authz.h

-        return AUTH_DENY;
+    res = bpf_map_lookup_elem(&tailcall_info_map, &tuple_key);
+    if (!res) {
+        BPF_LOG(ERR, AUTH, "Failed to retrieve res from map");


please be more clear

hzxuzhonghu · 2024-10-28T06:15:26Z

bpf/kmesh/workload/include/authz.h

+        BPF_LOG(ERR, AUTH, "Failed to retrieve match_ctx from map");
+        return XDP_PASS;
+    }
    for (i = 0; i < MAX_MEMBER_NUM_PER_POLICY; i++) {


MAX_MEMBER_NUM_PER_POLICY does not only limit the policy numbers but also the rules number per policy, right?

hzxuzhonghu · 2024-10-28T06:17:54Z

bpf/kmesh/workload/xdp.c

+int xdp_shutdown(struct xdp_md *ctx)
 {
-    int ret = 0;
+    struct match_ctx res;


please rename

hzxuzhonghu · 2024-10-28T06:22:14Z

bpf/kmesh/workload/include/authz.h

-        return AUTH_ALLOW;
-    } else {
-        return AUTH_DENY;
+    res->policy_index++;


should we move the policy index check here?

if (res->policy_index < 0 || res->policy_index >= MAX_MEMBER_NUM_PER_POLICY) { BPF_LOG(ERR, AUTH, "Policy index out of bounds"); return XDP_PASS; }

YaoZengzeng · 2024-10-28T06:23:52Z

bpf/kmesh/workload/include/authz.h

            break;
        }
-        rule = (Istio__Security__Rule *)kmesh_get_ptr_val((void *)*((__u64 *)rulesPtr + i));
+        if (!res) {


redundant check with line 342?

redundant check with line 342?

In xdp type eBPF program, the null pointer check may fail. An additional check is needed here, otherwise the verifier will report an error.

YaoZengzeng · 2024-10-28T06:32:45Z

bpf/kmesh/workload/include/authz.h

+static int construct_tuple_key(struct xdp_md *ctx, struct bpf_sock_tuple *tuple_info, struct xdp_info *info)
+{
+    int ret = parser_xdp_info(ctx, info);
+    if (ret != PARSER_SUCC) {


redundant check

weli-l · 2024-11-02T06:08:23Z

/retest

supercharge-xsy · 2024-11-06T03:21:29Z

bpf/kmesh/workload/include/authz.h

+        if (bpf_map_delete_elem(&kmesh_tc_info_map, &tuple_key) != 0) {
+            BPF_LOG(DEBUG, AUTH, "Failed to delete tailcall context from map");
+        }
+        bpf_tail_call(ctx, &xdp_tailcall_map, TAIL_CALL_AUTH_IN_USER_SPACE);


This code has multiple calls in the function, can it converge to one place?：
bpf_map_delete_elem(&kmesh_tc_info_map, &tuple_key)
bpf_tail_call(ctx, &xdp_tailcall_map, TAIL_CALL_AUTH_IN_USER_SPACE);

tacslon · 2024-11-06T09:16:31Z

bpf/kmesh/workload/include/authz.h

 }

-static inline int match_check(Istio__Security__Match *match, struct xdp_info *info, struct bpf_sock_tuple *tuple_info)
+static int match_check(Istio__Security__Match *match, struct xdp_info *info, struct bpf_sock_tuple *tuple_info)


Why remove the inline here?

hzxuzhonghu · 2024-11-07T03:36:24Z

bpf/kmesh/workload/include/authz.h

+    __uint(value_size, sizeof(struct match_context));
+    __uint(map_flags, BPF_F_NO_PREALLOC);
+    __uint(max_entries, MAP_SIZE_OF_AUTH_TAILCALL);
+} kmesh_tc_info_map SEC(".maps");


Suggested change

} kmesh_tc_info_map SEC(".maps");

} kmesh_tc_args SEC(".maps");

hzxuzhonghu · 2024-11-07T06:43:36Z

pkg/bpf/workload/xdp.go

+		uint32(constants.TailCallPolicyCheck),
+		uint32(xa.PolicyCheck.FD()),
+		ebpf.UpdateAny); err != nil {
+		return err


please add some more error info

hzxuzhonghu · 2024-11-07T06:44:57Z

bpf/kmesh/workload/xdp.c


-static inline int match_workload_policy(struct xdp_info *info, struct bpf_sock_tuple *tuple_info)
+SEC("xdp_auth")
+int xdp_shutdown(struct xdp_md *ctx)


Suggested change

int xdp_shutdown(struct xdp_md *ctx)

int xdp_authz(struct xdp_md *ctx)

hzxuzhonghu · 2024-11-07T06:45:03Z

bpf/kmesh/workload/xdp.c

 }

-static inline int match_workload_policy(struct xdp_info *info, struct bpf_sock_tuple *tuple_info)
+SEC("xdp_auth")


Suggested change

SEC("xdp_auth")

SEC("xdp_authz")

hzxuzhonghu · 2024-11-07T06:51:37Z

bpf/kmesh/workload/xdp.c

-        }
+    match_ctx.policies = policies;
+    match_ctx.policy_index = 0;
+    ret = bpf_map_update_elem(&kmesh_tc_info_map, &tuple_key, &match_ctx, BPF_ANY);


here we write a ctx to this map, and in the another place this ctx could be read , is it safe to access policies concurrently?

hzxuzhonghu · 2024-11-07T06:56:14Z

bpf/kmesh/workload/include/authz.h


-static inline int
-do_auth(Istio__Security__Authorization *policy, struct xdp_info *info, struct bpf_sock_tuple *tuple_info)
+SEC("xdp_auth")


Suggested change

SEC("xdp_auth")

SEC("xdp_authz")

weli-l · 2024-11-08T01:55:29Z

A switch needs to be added to determine whether to enable link-level authentication or packet-based authentication.

hzxuzhonghu · 2024-11-14T03:17:39Z

It is painful for me to review such a big pr with round and round update/review. We should incrementallly update. Never force push

hzxuzhonghu · 2024-11-14T03:39:32Z

bpf/include/common.h

 struct {
    __uint(type, BPF_MAP_TYPE_ARRAY);
-    __uint(max_entries, 1);
+    __uint(max_entries, 4);


nit https://github.com/kmesh-net/kmesh/pull/1015/files#diff-e531188f2d6f84d9df52b52c90eb506619ef6d6d6ec4169dd093fcc33531a3fbR66 we are changing to a config map

hzxuzhonghu · 2024-11-14T03:42:30Z

bpf/kmesh/workload/include/authz.h

+    return PARSER_SUCC;
+}
+
+static int matchDstPorts(Istio__Security__Match *match, struct xdp_info *info, struct bpf_sock_tuple *tuple_info)


I think this should be called match_dst_port

hzxuzhonghu · 2024-11-14T03:42:56Z

bpf/kmesh/workload/include/authz.h

    __u32 i;

    if (match->n_destination_ports == 0 && match->n_not_destination_ports == 0) {
+        BPF_LOG(DEBUG, AUTH, "No ports configured, matching by default");


log should be started with small cased word

hzxuzhonghu · 2024-11-14T03:44:06Z

bpf/kmesh/workload/include/authz.h

    __u32 i;

    if (rule->n_clauses == 0) {
        BPF_LOG(ERR, AUTH, "rule has no clauses\n");


this is not an error, it is a valid case to have {}

hzxuzhonghu · 2024-11-14T03:45:47Z

bpf/kmesh/workload/include/authz.h

+    struct xdp_info info = {0};
+    int ret;
+
+    if (construct_tuple_key(ctx, &tuple_key, &info) != PARSER_SUCC) {


instead of parsing this everywhere, can we move the parsed res into kmesh_tc_args

hzxuzhonghu · 2024-11-14T03:48:07Z

bpf/kmesh/workload/include/authz.h

+    int ret;
+    int i;
+
+    if (construct_tuple_key(ctx, &tuple_key, &info) != PARSER_SUCC) {


ditto if we add to the kmesh_tc_args, we will not need to parse again

hzxuzhonghu · 2024-11-14T03:50:23Z

bpf/kmesh/workload/xdp.c

    return XDP_DROP;
 }

+static bool is_authz_enabled()


Suggested change

static bool is_authz_enabled()

static bool is_authz_offload_enabled()

hzxuzhonghu · 2024-11-14T03:58:23Z

bpf/kmesh/workload/include/authz.h

+            if (bpf_map_update_elem(&map_of_auth, &tuple_key, &auth_result, BPF_ANY) != 0) {
+                BPF_LOG(ERR, AUTH, "Failed to update auth result in map_of_auth");
            }
+            return match_ctx->action == ISTIO__SECURITY__ACTION__DENY ? XDP_DROP : XDP_PASS;


this logic is not correct. Say we have two authorizationPolicy set, wih the returned result, i think we may miss the second policy check

Signed-off-by: weli-l <1289113577@qq.com>

hzxuzhonghu

/lgtm

kmesh-bot · 2024-11-18T12:31:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hzxuzhonghu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [hzxuzhonghu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kmesh-bot requested review from Okabe-Rintarou-0 and nlgwcy October 26, 2024 07:52

kmesh-bot added the size/L label Oct 26, 2024

weli-l force-pushed the dev/auth_multi_tailcall branch from 8a61e3f to bc1cc07 Compare October 26, 2024 07:58

LiZhenCheng9527 reviewed Oct 26, 2024

View reviewed changes

hzxuzhonghu reviewed Oct 28, 2024

View reviewed changes

bpf/kmesh/workload/xdp.c Outdated

int xdp_shutdown(struct xdp_md *ctx)

{

int ret = 0;

struct match_ctx res;

Copy link

Member

hzxuzhonghu Oct 28, 2024

Choose a reason for hiding this comment

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

please rename

hzxuzhonghu reviewed Oct 28, 2024

View reviewed changes

YaoZengzeng reviewed Oct 28, 2024

View reviewed changes

kmesh-bot added the do-not-merge/contains-merge-commits label Oct 30, 2024

weli-l force-pushed the dev/auth_multi_tailcall branch from f4e6c66 to 16dca7c Compare October 30, 2024 03:01

kmesh-bot removed the do-not-merge/contains-merge-commits label Oct 30, 2024

weli-l force-pushed the dev/auth_multi_tailcall branch 2 times, most recently from 8e71cd8 to 2ff203f Compare November 1, 2024 07:11

weli-l force-pushed the dev/auth_multi_tailcall branch from 2ff203f to 637d38e Compare November 4, 2024 01:55

supercharge-xsy reviewed Nov 6, 2024

View reviewed changes

tacslon reviewed Nov 6, 2024

View reviewed changes

hzxuzhonghu reviewed Nov 7, 2024

View reviewed changes

weli-l force-pushed the dev/auth_multi_tailcall branch 2 times, most recently from 2acab18 to 68a44df Compare November 9, 2024 02:52

weli-l force-pushed the dev/auth_multi_tailcall branch 4 times, most recently from 2d5c667 to 9380fa6 Compare November 13, 2024 01:43

kmesh-bot added size/XL and removed size/XXL labels Nov 13, 2024

weli-l force-pushed the dev/auth_multi_tailcall branch from 9380fa6 to f558a42 Compare November 13, 2024 01:51

kmesh-bot added size/XXL and removed size/XL labels Nov 13, 2024

weli-l force-pushed the dev/auth_multi_tailcall branch from 669dd29 to 949a4af Compare November 13, 2024 08:11

hzxuzhonghu reviewed Nov 14, 2024

View reviewed changes

weli-l added 5 commits November 18, 2024 11:05

support xdp auth in tailcall

46948c7

Signed-off-by: weli-l <1289113577@qq.com>

fix comment

0843437

Signed-off-by: weli-l <1289113577@qq.com>

fix comment

470be25

Signed-off-by: weli-l <1289113577@qq.com>

use kmeshctl to enable/disable authz

d06dd68

Signed-off-by: weli-l <1289113577@qq.com>

add cache for authz

1b864ca

Signed-off-by: weli-l <1289113577@qq.com>

weli-l force-pushed the dev/auth_multi_tailcall branch from 949a4af to cc2168d Compare November 18, 2024 03:19

resolve conflicts

87e5863

Signed-off-by: weli-l <1289113577@qq.com>

weli-l force-pushed the dev/auth_multi_tailcall branch 2 times, most recently from 7851592 to 88af784 Compare November 18, 2024 08:34

add authz for kmesh_config

7161af5

Signed-off-by: weli-l <1289113577@qq.com>

weli-l force-pushed the dev/auth_multi_tailcall branch from 88af784 to 7161af5 Compare November 18, 2024 09:04

modify kmesh_config entries to 1

e22e641

Signed-off-by: weli-l <1289113577@qq.com>

hzxuzhonghu approved these changes Nov 18, 2024

View reviewed changes

kmesh-bot assigned hzxuzhonghu Nov 18, 2024

kmesh-bot added the lgtm label Nov 18, 2024

kmesh-bot added the approved label Nov 18, 2024

kmesh-bot merged commit 7b5abca into kmesh-net:main Nov 18, 2024
9 checks passed

	BPF_LOG(ERR, AUTH, "Failed to read policyId, thrown it to user auth");
	BPF_LOG(ERR, AUTH, "Failed to read policyId, throw it to user auth");

	BPF_LOG(DEBUG, AUTH, "Failed to delete context from map");
	BPF_LOG(DEBUG, AUTH, "Failed to delete tailcall context from map");

	} kmesh_tc_info_map SEC(".maps");
	} kmesh_tc_args SEC(".maps");

	int xdp_shutdown(struct xdp_md *ctx)
	int xdp_authz(struct xdp_md *ctx)

	static bool is_authz_enabled()
	static bool is_authz_offload_enabled()

support xdp auth in tailcall #985

support xdp auth in tailcall #985

Uh oh!

Conversation

weli-l commented Oct 26, 2024

Uh oh!

codecov bot commented Oct 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lec-bit commented Oct 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weli-l commented Oct 27, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weli-l commented Nov 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weli-l commented Nov 8, 2024

Uh oh!

hzxuzhonghu commented Nov 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu left a comment

Choose a reason for hiding this comment

Uh oh!

kmesh-bot commented Nov 18, 2024

codecov bot commented Oct 26, 2024 •

edited

Loading