From efb5ba36aaed960717ba4dc44c215bb0712d20f3 Mon Sep 17 00:00:00 2001 From: Dawei Huang Date: Mon, 28 Sep 2026 13:08:02 -0500 Subject: [PATCH 1/2] [gnmi_server]: Run access checks before native Set bypass writes (#791) * [gnmi_server]: Authorize native Set bypass before backend Require configured authentication and gNMI write-role authorization before invoking bypass.TrySet. Add focused certificate coverage for mapped read-write success, mapped read-only and unknown-identity denial, and zero backend effect after denial. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Dawei Huang * [gnmi_server]: Clarify bypass access-check wording Describe the bypass header as a path request and use precise access-denial wording in the focused certificate tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Dawei Huang * [gnmi_server]: Use CONFIG_DB role for native Set bypass Resolve the native database target before bypass authorization. When role authorization is configured, require `gnmi_config_db_readwrite` for direct CONFIG_DB bypass requests. Do not attempt this bypass for other database targets. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Dawei Huang --------- Signed-off-by: Dawei Huang Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- gnmi_server/server.go | 44 ++++++- gnmi_server/set_bypass_authz_test.go | 184 +++++++++++++++++++++++++++ pkg/bypass/bypass.go | 9 +- pkg/bypass/bypass_test.go | 6 +- 4 files changed, 230 insertions(+), 13 deletions(-) create mode 100644 gnmi_server/set_bypass_authz_test.go diff --git a/gnmi_server/server.go b/gnmi_server/server.go index 5841dfc5..57b9793a 100644 --- a/gnmi_server/server.go +++ b/gnmi_server/server.go @@ -641,6 +641,27 @@ func IsNativeOrigin(origin string) bool { return origin == "sonic-db" } +func nativeSetTarget(prefix *gnmipb.Path, paths []*gnmipb.Path) string { + if target := prefix.GetTarget(); target != "" { + return target + } + + var target string + prefixElems := elemNames(prefix) + for _, path := range paths { + elems := append(append([]string(nil), prefixElems...), elemNames(path)...) + if len(elems) == 0 { + return "" + } + if target == "" { + target = elems[0] + } else if target != elems[0] { + return "" + } + } + return target +} + // Get implements the Get RPC in gNMI spec. func (s *Server) Get(ctx context.Context, req *gnmipb.GetRequest) (*gnmipb.GetResponse, error) { common_utils.IncCounter(common_utils.GNMI_GET) @@ -800,15 +821,26 @@ func (s *Server) Set(ctx context.Context, req *gnmipb.SetRequest) (*gnmipb.SetRe return nil, grpc.Errorf(codes.Unimplemented, "GNMI native write is disabled") } - // Fast path: bypass validation for allowed tables/SKUs - allUpdates := append(req.GetReplace(), req.GetUpdate()...) - if resp, used, err := bypass.TrySet(ctx, prefix, req.GetDelete(), allUpdates); used { + bypassTarget := nativeSetTarget(prefix, paths) + if bypass.IsRequested(ctx) && strings.EqualFold(bypassTarget, "CONFIG_DB") { + // Bypass metadata requests the bypass path. + // It does not grant write access. + ctx, err = authenticate(s.config, ctx, "gnmi_"+bypassTarget, true) if err != nil { common_utils.IncCounter(common_utils.GNMI_SET_FAIL) - return nil, status.Error(codes.Internal, err.Error()) + return nil, err + } + + // Fast path: bypass validation for allowed tables/SKUs. + allUpdates := append(req.GetReplace(), req.GetUpdate()...) + if resp, used, err := bypass.TrySet(ctx, prefix, req.GetDelete(), allUpdates); used { + if err != nil { + common_utils.IncCounter(common_utils.GNMI_SET_FAIL) + return nil, status.Error(codes.Internal, err.Error()) + } + common_utils.IncCounter(common_utils.GNMI_SET_BYPASS) + return resp, nil } - common_utils.IncCounter(common_utils.GNMI_SET_BYPASS) - return resp, nil } var targetDbName string diff --git a/gnmi_server/set_bypass_authz_test.go b/gnmi_server/set_bypass_authz_test.go new file mode 100644 index 00000000..750f9df8 --- /dev/null +++ b/gnmi_server/set_bypass_authz_test.go @@ -0,0 +1,184 @@ +package gnmi + +import ( + "context" + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" + "testing" + + "github.com/agiledragon/gomonkey/v2" + gnmipb "github.com/openconfig/gnmi/proto/gnmi" + "github.com/sonic-net/sonic-gnmi/common_utils" + "github.com/sonic-net/sonic-gnmi/pkg/bypass" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/credentials" + "google.golang.org/grpc/metadata" + "google.golang.org/grpc/peer" + "google.golang.org/grpc/status" +) + +func bypassCertificateContext(commonName string) context.Context { + ctx := metadata.NewIncomingContext( + context.Background(), + metadata.Pairs(bypass.MetadataKeyBypassValidation, "true"), + ) + cert := &x509.Certificate{Subject: pkix.Name{CommonName: commonName}} + return peer.NewContext(ctx, &peer.Peer{ + AuthInfo: credentials.TLSInfo{ + State: tls.ConnectionState{ + VerifiedChains: [][]*x509.Certificate{{cert}}, + }, + }, + }) +} + +func TestNativeSetTarget(t *testing.T) { + tests := []struct { + name string + prefix *gnmipb.Path + paths []*gnmipb.Path + want string + }{ + { + name: "prefix target", + prefix: &gnmipb.Path{Origin: "sonic-db", Target: "CONFIG_DB"}, + paths: []*gnmipb.Path{{Elem: []*gnmipb.PathElem{{Name: "VNET"}, {Name: "blue"}}}}, + want: "CONFIG_DB", + }, + { + name: "database in path", + prefix: &gnmipb.Path{Origin: "sonic-db"}, + paths: []*gnmipb.Path{{Elem: []*gnmipb.PathElem{ + {Name: "CONFIG_DB"}, + {Name: "localhost"}, + {Name: "VNET"}, + {Name: "blue"}, + }}}, + want: "CONFIG_DB", + }, + { + name: "conflicting databases", + prefix: &gnmipb.Path{Origin: "sonic-db"}, + paths: []*gnmipb.Path{ + {Elem: []*gnmipb.PathElem{{Name: "CONFIG_DB"}, {Name: "localhost"}, {Name: "VNET"}}}, + {Elem: []*gnmipb.PathElem{{Name: "APPL_DB"}, {Name: "localhost"}, {Name: "VNET"}}}, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if got := nativeSetTarget(test.prefix, test.paths); got != test.want { + t.Fatalf("nativeSetTarget() = %q, want %q", got, test.want) + } + }) + } +} + +func TestSetBypassCertificateAuthorization(t *testing.T) { + rolesByCommonName := map[string][]string{ + "mapped-readwrite": {"gnmi_config_db_readwrite"}, + "mapped-readonly": {"gnmi_config_db_readonly"}, + } + backend := map[string]string{} + backendCalls := 0 + + patches := gomonkey.NewPatches() + defer patches.Reset() + patches.ApplyFunc(PopulateAuthStructByCommonName, func(commonName string, auth *common_utils.AuthInfo, _ string) error { + roles, ok := rolesByCommonName[commonName] + if !ok { + return status.Error(codes.Unauthenticated, "certificate identity is not mapped") + } + auth.User = commonName + auth.Roles = append([]string(nil), roles...) + return nil + }) + patches.ApplyFunc(bypass.TrySet, func(_ context.Context, prefix *gnmipb.Path, _ []*gnmipb.Path, _ []*gnmipb.Update) (*gnmipb.SetResponse, bool, error) { + backendCalls++ + backend["VNET|blue"] = "1000" + return &gnmipb.SetResponse{Prefix: prefix}, true, nil + }) + + server := &Server{ + config: &Config{ + EnableNativeWrite: true, + ConfigTableName: "GNMI_CLIENT_CERT", + UserAuth: AuthTypes{"cert": true}, + }, + SaveStartupConfig: saveOnSetDisabled, + ReqFromMaster: ReqFromMasterDisabledMA, + } + request := &gnmipb.SetRequest{ + Prefix: &gnmipb.Path{Origin: "sonic-db", Target: "CONFIG_DB"}, + Update: []*gnmipb.Update{{ + Path: &gnmipb.Path{ + Elem: []*gnmipb.PathElem{ + {Name: "VNET"}, + {Name: "blue"}, + }, + }, + }}, + } + + tests := []struct { + name string + commonName string + wantErr bool + wantCode codes.Code + wantBackendCalls int + }{ + { + name: "mapped read-write success", + commonName: "mapped-readwrite", + wantBackendCalls: 1, + }, + { + name: "mapped read-only denial", + commonName: "mapped-readonly", + wantErr: true, + }, + { + name: "unmapped identity denial", + commonName: "unknown", + wantErr: true, + wantCode: codes.Unauthenticated, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + clear(backend) + backendCalls = 0 + + response, err := server.Set(bypassCertificateContext(test.commonName), request) + if test.wantErr { + if err == nil { + t.Fatal("Set() succeeded; want access denial") + } + if test.wantCode != codes.OK && status.Code(err) != test.wantCode { + t.Fatalf("Set() error code = %s, want %s", status.Code(err), test.wantCode) + } + if response != nil { + t.Fatalf("Set() response = %v, want nil after denial", response) + } + } else { + if err != nil { + t.Fatalf("Set() error = %v, want nil", err) + } + if response == nil { + t.Fatal("Set() response is nil after authorized bypass") + } + } + + if backendCalls != test.wantBackendCalls { + t.Fatalf("bypass backend calls = %d, want %d", backendCalls, test.wantBackendCalls) + } + _, changed := backend["VNET|blue"] + if changed != (test.wantBackendCalls == 1) { + t.Fatalf("bypass backend changed = %t, want %t", changed, test.wantBackendCalls == 1) + } + }) + } +} diff --git a/pkg/bypass/bypass.go b/pkg/bypass/bypass.go index b4747537..4f374830 100644 --- a/pkg/bypass/bypass.go +++ b/pkg/bypass/bypass.go @@ -88,7 +88,7 @@ func getConfigDbClientDefault() (*redis.Client, error) { // 2. SKU matches allowed prefixes // 3. All target tables are in the allowlist func ShouldBypass(ctx context.Context, prefix *gnmipb.Path, updates []*gnmipb.Update) bool { - if !hasBypassHeader(ctx) { + if !IsRequested(ctx) { return false } if !checkSKU() { @@ -103,7 +103,7 @@ func ShouldBypass(ctx context.Context, prefix *gnmipb.Path, updates []*gnmipb.Up // ShouldBypassDelete checks if delete paths should use the fast bypass path. // Same conditions as ShouldBypass but for delete operations. func ShouldBypassDelete(ctx context.Context, prefix *gnmipb.Path, deletes []*gnmipb.Path) bool { - if !hasBypassHeader(ctx) { + if !IsRequested(ctx) { return false } if !checkSKU() { @@ -131,8 +131,9 @@ func checkAllowedDeletePaths(prefix *gnmipb.Path, deletes []*gnmipb.Path) bool { return true } -// hasBypassHeader checks gRPC metadata for bypass header -func hasBypassHeader(ctx context.Context) bool { +// IsRequested reports whether incoming gRPC metadata contains a bypass request. +// Callers must complete authentication and the write-access check before calling TrySet. +func IsRequested(ctx context.Context) bool { if ctx == nil { return false } diff --git a/pkg/bypass/bypass_test.go b/pkg/bypass/bypass_test.go index 820e82f0..7f854b6a 100644 --- a/pkg/bypass/bypass_test.go +++ b/pkg/bypass/bypass_test.go @@ -15,7 +15,7 @@ import ( "google.golang.org/grpc/metadata" ) -func TestHasBypassHeader(t *testing.T) { +func TestIsRequested(t *testing.T) { tests := []struct { name string ctx context.Context @@ -67,9 +67,9 @@ func TestHasBypassHeader(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := hasBypassHeader(tt.ctx) + result := IsRequested(tt.ctx) if result != tt.expected { - t.Errorf("hasBypassHeader() = %v, want %v", result, tt.expected) + t.Errorf("IsRequested() = %v, want %v", result, tt.expected) } }) } From 405743dfd4d253ce0d3899e8e9a9caab2b6f824b Mon Sep 17 00:00:00 2001 From: Ubuntu Date: Sat, 3 Oct 2026 01:36:23 +0000 Subject: [PATCH 2/2] add elemNames def for cherry-pick Signed-off-by: Ubuntu --- gnmi_server/server.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/gnmi_server/server.go b/gnmi_server/server.go index 57b9793a..e871f9b3 100644 --- a/gnmi_server/server.go +++ b/gnmi_server/server.go @@ -641,6 +641,25 @@ func IsNativeOrigin(origin string) bool { return origin == "sonic-db" } +// elemNames returns the element names of a single path part, supporting the +// deprecated Element field for backward compatibility. Handling each part +// independently keeps requests that mix encodings between prefix and path +// from dropping elements. Key qualifiers on elements are ignored; matching +// is by element name only. +func elemNames(p *gnmipb.Path) []string { + if len(p.GetElem()) > 0 { + names := make([]string, 0, len(p.GetElem())) + for _, pe := range p.GetElem() { + names = append(names, pe.GetName()) + } + return names + } + if len(p.GetElement()) > 0 { + return append([]string(nil), p.GetElement()...) + } + return nil +} + func nativeSetTarget(prefix *gnmipb.Path, paths []*gnmipb.Path) string { if target := prefix.GetTarget(); target != "" { return target