Add Auth MonitorConnections chain elements #1317

anastasia-malysheva · 2022-07-05T10:20:23Z

Description

Add Authorization Chain element for MonitorConnection {Client/Server}.

Add next MonitorConnections elements
Add auth MonitorConnections elements
Add usage of new MonitorConnections elements to the existing ones
Add Policies for authorizations

Issue link

#46

How Has This Been Tested?

Added unit testing to cover
Tested manually
Tested by integration testing
Have not tested

Types of changes

denis-tingaikin

Started looking into this PR. Added a few comments, I'll look deeper a bit later.

denis-tingaikin · 2022-07-15T10:12:04Z

pkg/networkservice/core/eventchannel/monitor_client.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2020 Cisco and/or its affiliates.
+// Copyright (c) 2020-2022 Cisco and/or its affiliates.


Why do we need this?

denis-tingaikin · 2022-07-15T10:13:49Z

pkg/networkservice/common/heal/eventloop.go

+	mClient := next.NewMonitorConnectionClient(networkservice.NewMonitorConnectionClient(cc))
+	client, err := mClient.MonitorConnections(eventLoopCtx, selector)


Do we need these changes?

denis-tingaikin · 2022-07-15T10:14:49Z

pkg/networkservice/common/monitor/eventloop.go

@@ -51,7 +53,8 @@ func newEventLoop(ctx context.Context, ec EventConsumer, cc grpc.ClientConnInter
 		},
 	}

-	client, err := networkservice.NewMonitorConnectionClient(cc).MonitorConnections(eventLoopCtx, selector)
+	client, err := next.NewMonitorConnectionClient(
+		networkservice.NewMonitorConnectionClient(cc)).MonitorConnections(eventLoopCtx, selector)


Do we need these changes?

denis-tingaikin · 2022-07-18T09:25:19Z

pkg/networkservice/chains/endpoint/server.go

@@ -41,6 +41,7 @@ import (
 	"github.com/networkservicemesh/sdk/pkg/networkservice/common/updatetoken"
 	"github.com/networkservicemesh/sdk/pkg/networkservice/core/chain"
 	"github.com/networkservicemesh/sdk/pkg/tools/grpcutils"
+	authMonitor "github.com/networkservicemesh/sdk/pkg/tools/monitor/authorize"


Please use lowercase package naming to follow effective go conventions.

denis-tingaikin · 2022-07-18T09:25:42Z

pkg/networkservice/chains/nsmgr/server.go

@@ -58,6 +58,7 @@ import (
 	registryadapter "github.com/networkservicemesh/sdk/pkg/registry/core/adapters"
 	"github.com/networkservicemesh/sdk/pkg/registry/core/chain"
 	"github.com/networkservicemesh/sdk/pkg/tools/grpcutils"
+	authMonitor "github.com/networkservicemesh/sdk/pkg/tools/monitor/authorize"


Please use lowercase package naming to follow effective go conventions.

denis-tingaikin · 2022-07-18T09:26:13Z

pkg/networkservice/chains/nsmgr/server.go

@@ -75,6 +76,7 @@ type nsmgrServer struct {

 type serverOptions struct {
 	authorizeServer      networkservice.NetworkServiceServer
+	authMonitorOptions   []authMonitor.Option


Please do the same as we do for auth networkservice.NetworkServiceServer

glazychev-art · 2022-07-19T12:38:13Z

pkg/networkservice/common/authorize/server.go

 	}
 	if _, ok := peer.FromContext(ctx); ok {
 		if err := a.policies.check(ctx, leftSide); err != nil {
 			return nil, err
 		}
 	}
+	if spiffeID, err := getSpiffeID(ctx); err == nil {
+		ids, _ := a.spiffeIDConnectionMap.Load(spiffeID)
+		a.spiffeIDConnectionMap.LoadOrStore(spiffeID, append(ids, conn.GetId()))


We have refreshes. For each refresh, the same connection ID will be added to this list.
We need to either check this list or use an internal map (instead of list).

I've added nested map. Please take a look.

glazychev-art · 2022-07-19T12:41:55Z

pkg/networkservice/common/authorize/server_test.go

 	for i := range suits {
 		s := suits[i]
 		t.Run(s.name, func(t *testing.T) {
-			srv := authorize.NewServer(authorize.WithPolicies(s.policy))
+			srv := authorize.NewServer(authorize.WithSpiffeIDConnectionMap(&spiffeIDConnectionMap), authorize.WithPolicies(s.policy))


Do we need this changes?

glazychev-art · 2022-07-19T12:47:32Z

pkg/networkservice/common/monitor/server_test.go

 func TestMonitorServer(t *testing.T) {
 	t.Cleanup(func() { goleak.VerifyNone(t) })

-	ctx, cancel := context.WithTimeout(context.Background(), time.Second)
+	// Put peer Certificate to context


Do we need these changes? We don't use authorize monitor here

glazychev-art · 2022-07-19T12:48:07Z

pkg/tools/monitor/authorize/server.go

@@ -0,0 +1,92 @@
+// Copyright (c) 2022 Doc.ai and/or its affiliates.


Please use Cisco for new added files

denis-tingaikin

Conceptually looks correct

denis-tingaikin · 2022-07-21T23:50:43Z

pkg/networkservice/common/monitor/server.go

-	chainCtx context.Context
+	chainCtx    context.Context
+	filters     map[string]*monitorFilter
+	executor    *serialize.Executor


Suggested change

executor *serialize.Executor

executor serialize.Executor

denis-tingaikin · 2022-07-21T23:52:49Z

pkg/networkservice/common/monitor/server.go

+	executor := serialize.Executor{}
+	connections := make(map[string]*networkservice.Connection)
+
+	*monitorServerPtr = newMonitorConnectionServer(chainCtx, &executor, filters, connections)
 	return &monitorServer{


Suggested change

return &monitorServer{

var rv = &monitorServer{

denis-tingaikin · 2022-07-21T23:53:05Z

pkg/networkservice/common/monitor/server.go

+	filters := make(map[string]*monitorFilter)
+	executor := serialize.Executor{}
+	connections := make(map[string]*networkservice.Connection)
+
+	*monitorServerPtr = newMonitorConnectionServer(chainCtx, &executor, filters, connections)


Suggested change

filters := make(map[string]*monitorFilter)

executor := serialize.Executor{}

connections := make(map[string]*networkservice.Connection)

*monitorServerPtr = newMonitorConnectionServer(chainCtx, &executor, filters, connections)

denis-tingaikin · 2022-07-21T23:53:47Z

pkg/networkservice/common/monitor/server.go

+	executor := serialize.Executor{}
+	connections := make(map[string]*networkservice.Connection)
+
+	*monitorServerPtr = newMonitorConnectionServer(chainCtx, &executor, filters, connections)
 	return &monitorServer{
 		chainCtx:                chainCtx,
 		MonitorConnectionServer: *monitorServerPtr,


Suggested change

MonitorConnectionServer: *monitorServerPtr,

rv.MonitorConnectionServer = newMonitorConnectionServer(rv.chainCtx, &rv.executor, rv.filters, rv.connections),

denis-tingaikin · 2022-07-21T23:59:56Z

pkg/networkservice/common/authorize/server.go

+				ids.Delete(connID)
+			}
+		}
+		a.spiffeIDConnectionMap.Store(spiffeID, ids)


When we clear this map?

denis-tingaikin · 2022-07-22T00:07:39Z

pkg/tools/monitor/next/client_test.go

@@ -0,0 +1,106 @@
+// Copyright (c) 2022 Cisco and/or its affiliates.


Do we need a next client for this PR?

no, we don't need it. I removed all the changes regarding next monitorConnectionClient

denis-tingaikin · 2022-07-22T00:09:30Z

pkg/tools/opa/opainput.go

@@ -55,7 +54,8 @@ func pemEncodingX509Cert(cert *x509.Certificate) string {
 	return string(certpem)
 }

-func parseX509Cert(authInfo credentials.AuthInfo) *x509.Certificate {
+// ParseX509Cert - parse authinfo to get peer sertificate


Suggested change

// ParseX509Cert - parse authinfo to get peer sertificate

// ParseX509Cert parses x509 certificate from the passed credentials.AuthInfo

denis-tingaikin · 2022-07-22T00:12:35Z

pkg/tools/opa/policies.go

+func WithServiceOwnConnectionPolicy() *AuthorizationPolicy {
+	return &AuthorizationPolicy{
+		policySource: tokensServiceConnectionPolicySource,
+		query:        "service_connection",
+		checker:      True("service_connection"),
+	}
+}


I think we need consider a better name to clarify a fact that the policy should be used only and only with monitors.

@denis-tingaikin How about WithMonitorServerOwnConnectionPolicy ?

WithMonitorConnectionServerPolicy

denis-tingaikin · 2022-07-22T00:13:19Z

pkg/tools/opa/service_connection_policy_test.go

+	"github.com/networkservicemesh/sdk/pkg/tools/opa"
+)
+
+func TestWithServiceConnectionPolicy(t *testing.T) {


Could we please add a test for a negatvie scenario?

denis-tingaikin · 2022-07-22T00:14:12Z

pkg/tools/spire/gen_conn_map.go

+//go:generate go-syncmap -output connection_map.gen.go -type ConnectionMap<string,bool>
+
+// ConnectionMap - sync.Map with key == string and value == bool
+type ConnectionMap sync.Map


This is not looking as a connection map/

I think it is a simple string set.

denis-tingaikin · 2022-07-24T21:22:20Z

pkg/networkservice/common/monitor/monitor_connection_server.go

@@ -22,24 +22,31 @@ import (
 	"github.com/edwarnicke/serialize"


In current solution we actually dont need changes in network service/monitor pkg. Please revert pkg/networkservice/monitor changes.

denis-tingaikin · 2022-07-24T21:22:27Z

pkg/networkservice/common/monitor/server.go

@@ -23,6 +23,7 @@ package monitor
 import (


In current solution we actually dont need changes in network service/monitor pkg. Please revert pkg/networkservice/monitor changes.

denis-tingaikin · 2022-07-24T21:23:24Z

pkg/tools/monitor/authorize/common.go

@@ -0,0 +1,44 @@
+// Copyright (c) 2022 Cisco and/or its affiliates.


Should we rename pkg tools/monitor into tools/monitorconnection?

yes, I think this name is better, renamed

denis-tingaikin · 2022-07-24T21:30:50Z

pkg/networkservice/common/authorize/server.go

+func getSpiffeID(ctx context.Context) (string, error) {
+	p, ok := peer.FromContext(ctx)
+	var cert *x509.Certificate
+	if !ok {
+		return "", errors.New("fail to get peer from context")
+	}
+	cert = opa.ParseX509Cert(p.AuthInfo)
+	if cert != nil {
+		spiffeID, err := x509svid.IDFromCert(cert)
+		if err == nil {
+			return spiffeID.String(), nil
+		}
+		return "", errors.New("fail to get Spiffe ID from certificate")
+	}
+	return "", errors.New("fail to get certificate from peer")
+}


I think we should move this into tools/spire

denis-tingaikin · 2022-07-24T21:31:03Z

pkg/networkservice/common/authorize/server.go

 	if _, ok := peer.FromContext(ctx); ok {
 		if err := a.policies.check(ctx, leftSide); err != nil {
 			return nil, err
 		}
 	}
 	return next.Server(ctx).Close(ctx, conn)
 }
+
+func getSpiffeID(ctx context.Context) (string, error) {


Suggested change

func getSpiffeID(ctx context.Context) (string, error) {

func SpiffeIDFromContext(ctx context.Context) (string, error) {

NikitaSkrynnik · 2022-07-25T09:22:27Z

pkg/tools/opa/opainput.go

@@ -55,7 +54,8 @@ func pemEncodingX509Cert(cert *x509.Certificate) string {
 	return string(certpem)
 }

-func parseX509Cert(authInfo credentials.AuthInfo) *x509.Certificate {
+// ParseX509Cert - parses x509 certificate from the passed  credentials.AuthInfo


Suggested change

// ParseX509Cert - parses x509 certificate from the passed credentials.AuthInfo

// ParseX509Cert - parses x509 certificate from the passed credentials.AuthInfo

denis-tingaikin · 2022-07-25T09:29:43Z

pkg/tools/spire/gen_conn_map.go

+//go:generate go-syncmap -output connection_id_set.gen.go -type ConnectionIDSet<string,bool>
+
+// ConnectionIDSet - sync.Map with key == string and value == bool
+type ConnectionIDSet sync.Map


Why do we need to store this in tools/spire?

Can we move this into tools/stringset?

Also please use zero value type struct{}.