diff --git a/gnmi_server/server.go b/gnmi_server/server.go index 5841dfc5..e871f9b3 100644 --- a/gnmi_server/server.go +++ b/gnmi_server/server.go @@ -641,6 +641,46 @@ 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 + } + + 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 +840,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) } }) }