Skip to content

Commit

Permalink
Improve GNMI service to limit API access by role (#335)
Browse files Browse the repository at this point in the history 
Microsoft ADO: 30073317

#### Why I did it
GNMI/GNOI services need to support role in API operation. RO user is not allowed to call write API.

#### How I did it
We have supported common name to role mapping, and I have updated API authentication to verify role.

#### How to verify it
Run unit test and end to end test.
  • Loading branch information
@ganglyu
ganglyu authored Jan 10, 2025
1 parent c6f36b9 commit aa547ad
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 28 deletions.
2 changes: 1 addition & 1 deletion gnmi_server/debug.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import (

func (srv *Server) GetSubscribePreferences(req *spb_gnoi.SubscribePreferencesReq, stream spb_gnoi.Debug_GetSubscribePreferencesServer) error {
ctx := stream.Context()
ctx, err := authenticate(srv.config, ctx)
ctx, err := authenticate(srv.config, ctx, false)
if err != nil {
return err
}
Expand Down
36 changes: 18 additions & 18 deletions gnmi_server/gnoi.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ func ReadFileStat(path string) (*gnoi_file_pb.StatInfo, error) {
}

func (srv *FileServer) Stat(ctx context.Context, req *gnoi_file_pb.StatRequest) (*gnoi_file_pb.StatResponse, error) {
_, err := authenticate(srv.config, ctx)
_, err := authenticate(srv.config, ctx, false)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -116,7 +116,7 @@ func KillOrRestartProcess(restart bool, serviceName string) error {
}

func (srv *SystemServer) KillProcess(ctx context.Context, req *gnoi_system_pb.KillProcessRequest) (*gnoi_system_pb.KillProcessResponse, error) {
_, err := authenticate(srv.config, ctx)
_, err := authenticate(srv.config, ctx, true)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -166,7 +166,7 @@ func RebootSystem(fileName string) error {
func (srv *SystemServer) Reboot(ctx context.Context, req *gnoi_system_pb.RebootRequest) (*gnoi_system_pb.RebootResponse, error) {
fileName := common_utils.GNMI_WORK_PATH + "/config_db.json.tmp"

_, err := authenticate(srv.config, ctx)
_, err := authenticate(srv.config, ctx, true)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -202,7 +202,7 @@ func (srv *SystemServer) Reboot(ctx context.Context, req *gnoi_system_pb.RebootR

// TODO: Support GNOI RebootStatus
func (srv *SystemServer) RebootStatus(ctx context.Context, req *gnoi_system_pb.RebootStatusRequest) (*gnoi_system_pb.RebootStatusResponse, error) {
_, err := authenticate(srv.config, ctx)
_, err := authenticate(srv.config, ctx, false)
if err != nil {
return nil, err
}
Expand All @@ -212,7 +212,7 @@ func (srv *SystemServer) RebootStatus(ctx context.Context, req *gnoi_system_pb.R

// TODO: Support GNOI CancelReboot
func (srv *SystemServer) CancelReboot(ctx context.Context, req *gnoi_system_pb.CancelRebootRequest) (*gnoi_system_pb.CancelRebootResponse, error) {
_, err := authenticate(srv.config, ctx)
_, err := authenticate(srv.config, ctx, true)
if err != nil {
return nil, err
}
Expand All @@ -221,7 +221,7 @@ func (srv *SystemServer) CancelReboot(ctx context.Context, req *gnoi_system_pb.C
}
func (srv *SystemServer) Ping(req *gnoi_system_pb.PingRequest, rs gnoi_system_pb.System_PingServer) error {
ctx := rs.Context()
_, err := authenticate(srv.config, ctx)
_, err := authenticate(srv.config, ctx, true)
if err != nil {
return err
}
Expand All @@ -230,7 +230,7 @@ func (srv *SystemServer) Ping(req *gnoi_system_pb.PingRequest, rs gnoi_system_pb
}
func (srv *SystemServer) Traceroute(req *gnoi_system_pb.TracerouteRequest, rs gnoi_system_pb.System_TracerouteServer) error {
ctx := rs.Context()
_, err := authenticate(srv.config, ctx)
_, err := authenticate(srv.config, ctx, true)
if err != nil {
return err
}
Expand All @@ -239,23 +239,23 @@ func (srv *SystemServer) Traceroute(req *gnoi_system_pb.TracerouteRequest, rs gn
}
func (srv *SystemServer) SetPackage(rs gnoi_system_pb.System_SetPackageServer) error {
ctx := rs.Context()
_, err := authenticate(srv.config, ctx)
_, err := authenticate(srv.config, ctx, true)
if err != nil {
return err
}
log.V(1).Info("gNOI: SetPackage")
return status.Errorf(codes.Unimplemented, "")
}
func (srv *SystemServer) SwitchControlProcessor(ctx context.Context, req *gnoi_system_pb.SwitchControlProcessorRequest) (*gnoi_system_pb.SwitchControlProcessorResponse, error) {
_, err := authenticate(srv.config, ctx)
_, err := authenticate(srv.config, ctx, true)
if err != nil {
return nil, err
}
log.V(1).Info("gNOI: SwitchControlProcessor")
return nil, status.Errorf(codes.Unimplemented, "")
}
func (srv *SystemServer) Time(ctx context.Context, req *gnoi_system_pb.TimeRequest) (*gnoi_system_pb.TimeResponse, error) {
_, err := authenticate(srv.config, ctx)
_, err := authenticate(srv.config, ctx, false)
if err != nil {
return nil, err
}
Expand All @@ -267,7 +267,7 @@ func (srv *SystemServer) Time(ctx context.Context, req *gnoi_system_pb.TimeReque

func (srv *Server) Authenticate(ctx context.Context, req *spb_jwt.AuthenticateRequest) (*spb_jwt.AuthenticateResponse, error) {
// Can't enforce normal authentication here.. maybe only enforce client cert auth if enabled?
// ctx,err := authenticate(srv.config, ctx)
// ctx,err := authenticate(srv.config, ctx, false)
// if err != nil {
// return nil, err
// }
Expand All @@ -292,7 +292,7 @@ func (srv *Server) Authenticate(ctx context.Context, req *spb_jwt.AuthenticateRe

}
func (srv *Server) Refresh(ctx context.Context, req *spb_jwt.RefreshRequest) (*spb_jwt.RefreshResponse, error) {
ctx, err := authenticate(srv.config, ctx)
ctx, err := authenticate(srv.config, ctx, true)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -320,7 +320,7 @@ func (srv *Server) Refresh(ctx context.Context, req *spb_jwt.RefreshRequest) (*s
}

func (srv *Server) ClearNeighbors(ctx context.Context, req *spb.ClearNeighborsRequest) (*spb.ClearNeighborsResponse, error) {
ctx, err := authenticate(srv.config, ctx)
ctx, err := authenticate(srv.config, ctx, true)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -352,7 +352,7 @@ func (srv *Server) ClearNeighbors(ctx context.Context, req *spb.ClearNeighborsRe
}

func (srv *Server) CopyConfig(ctx context.Context, req *spb.CopyConfigRequest) (*spb.CopyConfigResponse, error) {
ctx, err := authenticate(srv.config, ctx)
ctx, err := authenticate(srv.config, ctx, true)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -383,7 +383,7 @@ func (srv *Server) CopyConfig(ctx context.Context, req *spb.CopyConfigRequest) (
}

func (srv *Server) ShowTechsupport(ctx context.Context, req *spb.TechsupportRequest) (*spb.TechsupportResponse, error) {
ctx, err := authenticate(srv.config, ctx)
ctx, err := authenticate(srv.config, ctx, false)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -415,7 +415,7 @@ func (srv *Server) ShowTechsupport(ctx context.Context, req *spb.TechsupportRequ
}

func (srv *Server) ImageInstall(ctx context.Context, req *spb.ImageInstallRequest) (*spb.ImageInstallResponse, error) {
ctx, err := authenticate(srv.config, ctx)
ctx, err := authenticate(srv.config, ctx, true)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -447,7 +447,7 @@ func (srv *Server) ImageInstall(ctx context.Context, req *spb.ImageInstallReques
}

func (srv *Server) ImageRemove(ctx context.Context, req *spb.ImageRemoveRequest) (*spb.ImageRemoveResponse, error) {
ctx, err := authenticate(srv.config, ctx)
ctx, err := authenticate(srv.config, ctx, true)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -477,7 +477,7 @@ func (srv *Server) ImageRemove(ctx context.Context, req *spb.ImageRemoveRequest)
}

func (srv *Server) ImageDefault(ctx context.Context, req *spb.ImageDefaultRequest) (*spb.ImageDefaultResponse, error) {
ctx, err := authenticate(srv.config, ctx)
ctx, err := authenticate(srv.config, ctx, true)
if err != nil {
return nil, err
}
Expand Down
18 changes: 13 additions & 5 deletions gnmi_server/server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ type Config struct {

var AuthLock sync.Mutex
var maMu sync.Mutex
const WriteAccessMode = "readwrite"

func (i AuthTypes) String() string {
if i["none"] {
Expand Down Expand Up @@ -240,7 +241,7 @@ func (srv *Server) Port() int64 {
return srv.config.Port
}

func authenticate(config *Config, ctx context.Context) (context.Context, error) {
func authenticate(config *Config, ctx context.Context, writeAccess bool) (context.Context, error) {
var err error
success := false
rc, ctx := common_utils.GetContext(ctx)
Expand Down Expand Up @@ -268,6 +269,13 @@ func authenticate(config *Config, ctx context.Context) (context.Context, error)
if err == nil {
success = true
}
// role must be readwrite to support write access
if writeAccess && config.ConfigTableName != "" {
role := rc.Auth.Roles[0]
if role != WriteAccessMode {
return ctx, fmt.Errorf("%s does not have write access, %s", rc.Auth.User, role)
}
}
}

//Allow for future authentication mechanisms here...
Expand All @@ -283,7 +291,7 @@ func authenticate(config *Config, ctx context.Context) (context.Context, error)
// Subscribe implements the gNMI Subscribe RPC.
func (s *Server) Subscribe(stream gnmipb.GNMI_SubscribeServer) error {
ctx := stream.Context()
ctx, err := authenticate(s.config, ctx)
ctx, err := authenticate(s.config, ctx, false)
if err != nil {
return err
}
Expand Down Expand Up @@ -368,7 +376,7 @@ func IsNativeOrigin(origin string) bool {
// 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)
ctx, err := authenticate(s.config, ctx)
ctx, err := authenticate(s.config, ctx, false)
if err != nil {
common_utils.IncCounter(common_utils.GNMI_GET_FAIL)
return nil, err
Expand Down Expand Up @@ -474,7 +482,7 @@ func (s *Server) Set(ctx context.Context, req *gnmipb.SetRequest) (*gnmipb.SetRe
common_utils.IncCounter(common_utils.GNMI_SET_FAIL)
return nil, grpc.Errorf(codes.Unimplemented, "GNMI is in read-only mode")
}
ctx, err := authenticate(s.config, ctx)
ctx, err := authenticate(s.config, ctx, true)
if err != nil {
common_utils.IncCounter(common_utils.GNMI_SET_FAIL)
return nil, err
Expand Down Expand Up @@ -576,7 +584,7 @@ func (s *Server) Set(ctx context.Context, req *gnmipb.SetRequest) (*gnmipb.SetRe
}

func (s *Server) Capabilities(ctx context.Context, req *gnmipb.CapabilityRequest) (*gnmipb.CapabilityResponse, error) {
ctx, err := authenticate(s.config, ctx)
ctx, err := authenticate(s.config, ctx, false)
if err != nil {
return nil, err
}
Expand Down
37 changes: 33 additions & 4 deletions gnmi_server/server_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4520,7 +4520,7 @@ func TestClientCertAuthenAndAuthor(t *testing.T) {
// check get 1 cert name
ctx, cancel = CreateAuthorizationCtx()
configDb.Flushdb()
gnmiTable.Hset("certname1", "role", "role1")
gnmiTable.Hset("certname1", "role", "readwrite")
ctx, err = ClientCertAuthenAndAuthor(ctx, "GNMI_CLIENT_CERT", false)
if err != nil {
t.Errorf("CommonNameMatch with correct cert name should success: %v", err)
Expand All @@ -4531,8 +4531,8 @@ func TestClientCertAuthenAndAuthor(t *testing.T) {
// check get multiple cert names
ctx, cancel = CreateAuthorizationCtx()
configDb.Flushdb()
gnmiTable.Hset("certname1", "role", "role1")
gnmiTable.Hset("certname2", "role", "role2")
gnmiTable.Hset("certname1", "role", "readwrite")
gnmiTable.Hset("certname2", "role", "readonly")
ctx, err = ClientCertAuthenAndAuthor(ctx, "GNMI_CLIENT_CERT", false)
if err != nil {
t.Errorf("CommonNameMatch with correct cert name should success: %v", err)
Expand All @@ -4543,7 +4543,7 @@ func TestClientCertAuthenAndAuthor(t *testing.T) {
// check a invalid cert cname
ctx, cancel = CreateAuthorizationCtx()
configDb.Flushdb()
gnmiTable.Hset("certname2", "role", "role2")
gnmiTable.Hset("certname2", "role", "readonly")
ctx, err = ClientCertAuthenAndAuthor(ctx, "GNMI_CLIENT_CERT", false)
if err == nil {
t.Errorf("CommonNameMatch with invalid cert name should fail: %v", err)
Expand All @@ -4555,6 +4555,35 @@ func TestClientCertAuthenAndAuthor(t *testing.T) {
swsscommon.DeleteDBConnector(configDb)
}

func TestAuthenticate(t *testing.T) {
if !swsscommon.SonicDBConfigIsInit() {
swsscommon.SonicDBConfigInitialize()
}

var tableName = "GNMI_CLIENT_CERT"
var configDb = swsscommon.NewDBConnector("CONFIG_DB", uint(0), true)
var gnmiTable = swsscommon.NewTable(configDb, tableName)
defer swsscommon.DeleteTable(gnmiTable)
defer swsscommon.DeleteDBConnector(configDb)
configDb.Flushdb()

// initialize err variable
err := status.Error(codes.Unauthenticated, "")

// check a invalid role
cfg := &Config{ConfigTableName: tableName, UserAuth: AuthTypes{"password": false, "cert": true, "jwt": false}}
ctx, cancel := CreateAuthorizationCtx()
configDb.Flushdb()
gnmiTable.Hset("certname1", "role", "readonly")
// Call authenticate to verify the user's role. This should fail if the role is "readonly".
_, err = authenticate(cfg, ctx, true)
if err == nil {
t.Errorf("authenticate with readonly role should fail: %v", err)
}

cancel()
}

type MockServerStream struct {
grpc.ServerStream
}
Expand Down

0 comments on commit aa547ad

Please sign in to comment.