Cherry-pick: [gnmi_server]: Run access checks before native Set bypass writes (#791)

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

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)
+			}
+		})
+	}
+}

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
 	}

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)
 			}
 		})
 	}