pkg/*: use Kubernetes ServiceAccount as service identity

This change does the following:

- Uses kubernetes ServiceAccount as service identity for
  proxy service certificates. The service certificate
  CN/SAN will be of the form <svc-account>.<namespace>.<domain>.

- Refactors SDS to make it easier to share code in its internal
  implementation.

- Removes the overloaded usage of the certificate's CN for SNI.
  SNI (Server Name Identifier) is based on the service FQDN,
  while the service certificate is derived from the ServiceIdentity
  being used.

- Refactors sds/response_test.go to make it possible to use
  table-driven testing to test different unit testing scenarios.

Signed-off-by: Shashank Ram <shashank08@gmail.com>

go.sum

@@ -243,6 +243,7 @@ github.com/envoyproxy/go-control-plane v0.9.0/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymF
 github.com/envoyproxy/go-control-plane v0.9.1-0.20191026205805-5f8ba28d4473/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymFceY/DCBVvsKhRF0iEA4=
 github.com/envoyproxy/go-control-plane v0.9.6 h1:GgblEiDzxf5ajlAZY4aC8xp7DwkrGfauFNMGdB2bBv0=
 github.com/envoyproxy/go-control-plane v0.9.6/go.mod h1:GFqM7v0B62MraO4PWRedIbhThr/Rf7ev6aHOOPXeaDA=
+github.com/envoyproxy/go-control-plane v0.9.7 h1:EARl0OvqMoxq/UMgMSCLnXzkaXbxzskluEBlMQCJPms=
 github.com/envoyproxy/protoc-gen-validate v0.1.0 h1:EQciDnbrYxy13PgWoY8AqoxGiPrpgBZ1R8UNe3ddc+A=
 github.com/envoyproxy/protoc-gen-validate v0.1.0/go.mod h1:iSmxcyjqTsJpI2R4NaDN7+kN2VEUnK/pcBlmesArF7c=
 github.com/evanphx/json-patch v0.0.0-20200808040245-162e5629780b/go.mod h1:NAJj0yf/KaRKURN6nyi7A9IZydMivZEm9oQLWNjfKDc=

pkg/catalog/xds_certificates.go

@@ -192,3 +192,17 @@ func getCertificateCommonNameMeta(cn certificate.CommonName) (*certificateCommon
 func NewCertCommonNameWithProxyID(proxyUUID uuid.UUID, serviceAccount, namespace string) certificate.CommonName {
 	return certificate.CommonName(strings.Join([]string{proxyUUID.String(), serviceAccount, namespace}, constants.DomainDelimiter))
 }
+
+// GetServiceAccountFromProxyCertificate returns the ServiceAccount information encoded in the certificate CN
+func GetServiceAccountFromProxyCertificate(cn certificate.CommonName) (service.K8sServiceAccount, error) {
+	var svcAccount service.K8sServiceAccount
+	cnMeta, err := getCertificateCommonNameMeta(cn)
+	if err != nil {
+		return svcAccount, err
+	}
+
+	svcAccount.Name = cnMeta.ServiceAccount
+	svcAccount.Namespace = cnMeta.Namespace
+
+	return svcAccount, nil
+}

pkg/catalog/xds_certificates_test.go

@@ -494,4 +494,19 @@ var _ = Describe("Test XDS certificate tooling", func() {
 			Expect(actual).To(Equal(expected))
 		})
 	})
+
+	Context("Test GetServiceAccountFromProxyCertificate", func() {
+		It("should correctly return the ServiceAccount encoded in the XDS certificate CN", func() {
+			cn := certificate.CommonName(fmt.Sprintf("%s.sa-name.sa-namespace", uuid.New().String()))
+			svcAccount, err := GetServiceAccountFromProxyCertificate(cn)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(svcAccount).To(Equal(service.K8sServiceAccount{Name: "sa-name", Namespace: "sa-namespace"}))
+		})
+
+		It("should correctly error when the XDS certificate CN is invalid", func() {
+			svcAccount, err := GetServiceAccountFromProxyCertificate(certificate.CommonName("invalid"))
+			Expect(err).To(HaveOccurred())
+			Expect(svcAccount).To(Equal(service.K8sServiceAccount{}))
+		})
+	})
 })

pkg/envoy/cds/cluster.go

@@ -27,7 +27,7 @@ const (
 func getRemoteServiceCluster(remoteService, localService service.MeshService, cfg configurator.Configurator) (*xds_cluster.Cluster, error) {
 	clusterName := remoteService.String()
 	marshalledUpstreamTLSContext, err := envoy.MessageToAny(
-		envoy.GetUpstreamTLSContext(localService, remoteService.GetCommonName().String()))
+		envoy.GetUpstreamTLSContext(localService, remoteService.ServerName()))
 	if err != nil {
 		return nil, err
 	}

pkg/envoy/cds/response_test.go

@@ -187,7 +187,7 @@ var _ = Describe("CDS Response", func() {
 
 			// Checking for the value by generating the same value the same way is reduntant
 			// Nonetheless, as getRemoteServiceCluster logic gets more complicated, this might just be ok to have
-			upstreamTLSProto, err := envoy.MessageToAny(envoy.GetUpstreamTLSContext(proxyService, remoteService.GetCommonName().String()))
+			upstreamTLSProto, err := envoy.MessageToAny(envoy.GetUpstreamTLSContext(proxyService, remoteService.ServerName()))
 			Expect(err).ToNot(HaveOccurred())
 
 			expectedCluster := xds_cluster.Cluster{
@@ -256,7 +256,7 @@ var _ = Describe("CDS Response", func() {
 					},
 					AlpnProtocols: envoy.ALPNInMesh,
 				},
-				Sni:                remoteService.GetCommonName().String(),
+				Sni:                remoteService.ServerName(),
 				AllowRenegotiation: false,
 			}
 			Expect(upstreamTLSContext.CommonTlsContext.TlsParams).To(Equal(expectedTLSContext.CommonTlsContext.TlsParams))

pkg/envoy/lds/ingress.go

@@ -64,7 +64,7 @@ func getIngressFilterChains(svc service.MeshService, cfg configurator.Configurat
 		// Filter chain with SNI matching enabled for HTTPS clients that set the SNI
 		ingressFilterChainWithSNI := newIngressFilterChain(cfg, svc)
 		ingressFilterChainWithSNI.Name = inboundIngressHTTPSFilterChain
-		ingressFilterChainWithSNI.FilterChainMatch.ServerNames = []string{svc.GetCommonName().String()}
+		ingressFilterChainWithSNI.FilterChainMatch.ServerNames = []string{svc.ServerName()}
 		ingressFilterChains = append(ingressFilterChains, ingressFilterChainWithSNI)
 	}
 

pkg/envoy/lds/inmesh.go

@@ -47,7 +47,7 @@ func getInboundInMeshFilterChain(proxyServiceName service.MeshService, cfg confi
 		// This field is configured by the GetDownstreamTLSContext() function.
 		// This is not a field obtained from the mTLS Certificate.
 		FilterChainMatch: &xds_listener.FilterChainMatch{
-			ServerNames:          []string{proxyServiceName.GetCommonName().String()},
+			ServerNames:          []string{proxyServiceName.ServerName()},
 			TransportProtocol:    envoy.TransportProtocolTLS,
 			ApplicationProtocols: envoy.ALPNInMesh, // in-mesh proxies will advertise this, set in UpstreamTlsContext
 		},

pkg/envoy/lds/response_test.go

@@ -31,7 +31,7 @@ var _ = Describe("Test LDS response", func() {
 		})
 
 		It("constructs filter chain used for HTTPS ingress", func() {
-			expectedServerNames := []string{tests.BookstoreV1Service.GetCommonName().String()}
+			expectedServerNames := []string{tests.BookstoreV1Service.ServerName()}
 
 			mockConfigurator.EXPECT().UseHTTPSIngress().Return(true).AnyTimes()
 
@@ -67,7 +67,7 @@ var _ = Describe("Test LDS response", func() {
 			filterChain, err := getInboundInMeshFilterChain(tests.BookstoreV1Service, mockConfigurator)
 			Expect(err).ToNot(HaveOccurred())
 
-			expectedServerNames := []string{tests.BookstoreV1Service.GetCommonName().String()}
+			expectedServerNames := []string{tests.BookstoreV1Service.ServerName()}
 
 			// Show what this looks like (human readable)!  And ensure this is setup correctly!
 			Expect(expectedServerNames[0]).To(Equal("bookstore-v1.default.svc.cluster.local"))
@@ -77,7 +77,7 @@ var _ = Describe("Test LDS response", func() {
 
 			// Ensure the UpstreamTlsContext.Sni field from the client matches one of the strings
 			// in the servers FilterChainMatch.ServerNames
-			tlsContext := envoy.GetUpstreamTLSContext(tests.BookbuyerService, tests.BookstoreV1Service.GetCommonName().String())
+			tlsContext := envoy.GetUpstreamTLSContext(tests.BookbuyerService, tests.BookstoreV1Service.ServerName())
 			Expect(tlsContext.Sni).To(Equal(filterChain.FilterChainMatch.ServerNames[0]))
 
 			// Show what that actually looks like

pkg/envoy/sds/response.go

@@ -13,6 +13,7 @@ import (
 	"github.com/openservicemesh/osm/pkg/certificate"
 	"github.com/openservicemesh/osm/pkg/configurator"
 	"github.com/openservicemesh/osm/pkg/envoy"
+	"github.com/openservicemesh/osm/pkg/identity"
 	"github.com/openservicemesh/osm/pkg/service"
 )
 
@@ -23,59 +24,86 @@ var directionMap = map[envoy.SDSCertType]string{
 }
 
 // NewResponse creates a new Secrets Discovery Response.
-func NewResponse(catalog catalog.MeshCataloger, proxy *envoy.Proxy, request *xds_discovery.DiscoveryRequest, cfg configurator.Configurator, certManager certificate.Manager) (*xds_discovery.DiscoveryResponse, error) {
+func NewResponse(meshCatalog catalog.MeshCataloger, proxy *envoy.Proxy, request *xds_discovery.DiscoveryRequest, cfg configurator.Configurator, certManager certificate.Manager) (*xds_discovery.DiscoveryResponse, error) {
 	log.Info().Msgf("Composing SDS Discovery Response for proxy: %s", proxy.GetCommonName())
 
-	svcList, err := catalog.GetServicesFromEnvoyCertificate(proxy.GetCommonName())
-	// Github Issue #1575
-	serviceForProxy := svcList[0]
+	svcList, err := meshCatalog.GetServicesFromEnvoyCertificate(proxy.GetCommonName())
 	if err != nil {
-		log.Error().Err(err).Msgf("Error looking up service for Envoy with CN=%q", proxy.GetCommonName())
+		log.Error().Err(err).Msgf("Error getting services associated with proxy %s", proxy.GetCommonName())
 		return nil, err
 	}
 
-	cert, err := certManager.IssueCertificate(serviceForProxy.GetCommonName(), cfg.GetServiceCertValidityPeriod())
+	// OSM currently relies on kubernetes ServiceAccount for service identity
+	svcAccount, err := catalog.GetServiceAccountFromProxyCertificate(proxy.GetCommonName())
 	if err != nil {
-		log.Error().Err(err).Msgf("Error issuing a certificate for proxy service %s", serviceForProxy)
+		log.Error().Err(err).Msgf("Error retrieving SerivceAccount for proxy %s", proxy.GetCommonName())
 		return nil, err
 	}
 
-	// Iterate over the list of tasks and create response structs to be
-	// sent to the proxy that made the discovery request
+	sdsImpl := newSDSImpl(proxy, meshCatalog, certManager, cfg, svcList, svcAccount)
+	return sdsImpl.createDiscoveryResponse(request)
+}
+
+func newSDSImpl(proxy *envoy.Proxy, meshCatalog catalog.MeshCataloger, certManager certificate.Manager, cfg configurator.Configurator, proxyServices []service.MeshService, svcAccount service.K8sServiceAccount) *sdsImpl {
+	impl := &sdsImpl{
+		proxy:         proxy,
+		meshCatalog:   meshCatalog,
+		certManager:   certManager,
+		cfg:           cfg,
+		svcAccount:    svcAccount,
+		proxyServices: proxyServices,
+	}
+
+	return impl
+}
+
+func (s *sdsImpl) createDiscoveryResponse(request *xds_discovery.DiscoveryRequest) (*xds_discovery.DiscoveryResponse, error) {
+	// Resources corresponding to SDS secrets returned as a part of the DiscoveryResponse
 	var resources []*any.Any
 
+	// The DisvoceryRequest contains the requested certs
 	requestedCerts := request.ResourceNames
-	log.Trace().Msgf("Received SDS request for ResourceNames (certificates) %+v", requestedCerts)
-
-	// request.ResourceNames is expected to be a list of either "service-cert:namespace/service" or "root-cert:namespace/service"
-	for _, envoyProto := range getEnvoySDSSecrets(cert, proxy, requestedCerts, catalog) {
-		marshalledSecret, err := ptypes.MarshalAny(envoyProto)
+	log.Trace().Msgf("Received SDS request for ResourceNames (certificates) %+v on proxy %s, services: %v", requestedCerts, s.proxy.GetCommonName(), s.proxyServices)
+
+	for _, proxyService := range s.proxyServices {
+		log.Trace().Msgf("Creating SDS config for proxy service %s on proxy %s", proxyService, s.proxy.GetCommonName())
+		// 1. Issue a service certificate for this proxy
+		// OSM currently relies on kubernetes ServiceAccount for service identity
+		si := identity.GetKubernetesServiceIdentity(s.svcAccount, identity.ClusterLocalTrustDomain)
+		cert, err := s.certManager.IssueCertificate(certificate.CommonName(si), s.cfg.GetServiceCertValidityPeriod())
 		if err != nil {
-			log.Error().Err(err).Msgf("Error marshaling Envoy secret %s for proxy %s for service %s", envoyProto.Name, proxy.GetCommonName(), serviceForProxy.String())
-			continue
+			log.Error().Err(err).Msgf("Error issuing a certificate for proxy service %s", proxyService)
+			return nil, err
 		}
 
-		resources = append(resources, marshalledSecret)
+		// 2. Create SDS secret resources based on the requested certs in the DiscoveryRequest
+		// request.ResourceNames is expected to be a list of either "service-cert:namespace/service" or "root-cert:namespace/service"
+		for _, envoyProto := range s.getSDSSecrets(cert, requestedCerts, proxyService) {
+			marshalledSecret, err := ptypes.MarshalAny(envoyProto)
+			if err != nil {
+				log.Error().Err(err).Msgf("Error marshaling Envoy secret %s for proxy %s for service %s", envoyProto.Name, s.proxy.GetCommonName(), proxyService)
+				continue
+			}
+
+			resources = append(resources, marshalledSecret)
+		}
 	}
+
 	return &xds_discovery.DiscoveryResponse{
 		TypeUrl:   string(envoy.TypeSDS),
 		Resources: resources,
 	}, nil
 }
 
-func getEnvoySDSSecrets(cert certificate.Certificater, proxy *envoy.Proxy, requestedCerts []string, catalog catalog.MeshCataloger) []*xds_auth.Secret {
-	// requestedCerts is expected to be a list of either "service-cert:namespace/service" or "root-cert:namespace/service"
+func (s *sdsImpl) getSDSSecrets(cert certificate.Certificater, requestedCerts []string, proxyService service.MeshService) []*xds_auth.Secret {
+	// requestedCerts is expected to be a list of either of the following:
+	// - "service-cert:namespace/service"
+	// - "root-cert-for-mtls-outbound:namespace/service"
+	// - "root-cert-for-mtls-inbound:namespace/service"
+	// - "root-cert-for-https:namespace/service"
 
 	var envoySecrets []*xds_auth.Secret
 
-	svcList, err := catalog.GetServicesFromEnvoyCertificate(proxy.GetCommonName())
-	if err != nil {
-		log.Error().Err(err).Msgf("Error looking up service for Envoy with CN=%q", proxy.GetCommonName())
-		return nil
-	}
-	// Github Issue #1575
-	serviceForProxy := svcList[0]
-
 	// The Envoy makes a request for a list of resources (aka certificates), which we will send as a response to the SDS request.
 	for _, requestedCertificate := range requestedCerts {
 		// requestedCertType could be either "service-cert" or "root-cert"
@@ -85,17 +113,17 @@ func getEnvoySDSSecrets(cert certificate.Certificater, proxy *envoy.Proxy, reque
 			continue
 		}
 
-		if serviceForProxy != sdsCert.MeshService {
-			log.Error().Msgf("Proxy %s (service %s) requested service certificate %s; this is not allowed", proxy.GetCommonName(), serviceForProxy, sdsCert.MeshService)
+		if proxyService != sdsCert.MeshService {
+			log.Debug().Msgf("Proxy %s (service %s) requested service certificate %s; this is not allowed", s.proxy.GetCommonName(), proxyService, sdsCert.MeshService)
 			continue
 		}
 
 		switch sdsCert.CertType {
 		case envoy.ServiceCertType:
-			log.Info().Msgf("proxy %s (member of service %s) requested %s", proxy.GetCommonName(), serviceForProxy.String(), requestedCertificate)
+			log.Info().Msgf("proxy %s (member of service %s) requested %s", s.proxy.GetCommonName(), proxyService, requestedCertificate)
 			envoySecret, err := getServiceCertSecret(cert, requestedCertificate)
 			if err != nil {
-				log.Error().Err(err).Msgf("Error creating cert %s for proxy %s for service %s", requestedCertificate, proxy.GetCommonName(), serviceForProxy.String())
+				log.Error().Err(err).Msgf("Error creating cert %s for proxy %s for service %s", requestedCertificate, s.proxy.GetCommonName(), proxyService)
 				continue
 			}
 			envoySecrets = append(envoySecrets, envoySecret)
@@ -105,15 +133,16 @@ func getEnvoySDSSecrets(cert certificate.Certificater, proxy *envoy.Proxy, reque
 		case envoy.RootCertTypeForMTLSOutbound:
 			fallthrough
 		case envoy.RootCertTypeForHTTPS:
-			log.Info().Msgf("proxy %s (member of service %s) requested %s", proxy.GetCommonName(), serviceForProxy.String(), requestedCertificate)
-			envoySecret, err := getRootCert(cert, *sdsCert, serviceForProxy, catalog)
+			log.Info().Msgf("proxy %s (member of service %s) requested %s", s.proxy.GetCommonName(), proxyService, requestedCertificate)
+			envoySecret, err := s.getRootCert(cert, *sdsCert, proxyService)
 			if err != nil {
-				log.Error().Err(err).Msgf("Error creating cert %s for proxy %s for service %s", requestedCertificate, proxy.GetCommonName(), serviceForProxy.String())
+				log.Error().Err(err).Msgf("Error creating cert %s for proxy %s for service %s", requestedCertificate, s.proxy.GetCommonName(), proxyService)
 				continue
 			}
 			envoySecrets = append(envoySecrets, envoySecret)
 		}
 	}
+
 	return envoySecrets
 }
 
@@ -141,7 +170,7 @@ func getServiceCertSecret(cert certificate.Certificater, name string) (*xds_auth
 	return secret, nil
 }
 
-func getRootCert(cert certificate.Certificater, sdscert envoy.SDSCert, proxyServiceName service.MeshService, mc catalog.MeshCataloger) (*xds_auth.Secret, error) {
+func (s *sdsImpl) getRootCert(cert certificate.Certificater, sdscert envoy.SDSCert, proxyService service.MeshService) (*xds_auth.Secret, error) {
 	secret := &xds_auth.Secret{
 		// The Name field must match the tls_context.common_tls_context.tls_certificate_sds_secret_configs.name
 		Name: sdscert.String(),
@@ -156,41 +185,51 @@ func getRootCert(cert certificate.Certificater, sdscert envoy.SDSCert, proxyServ
 		},
 	}
 
+	if s.cfg.IsPermissiveTrafficPolicyMode() {
+		// In permissive mode, there are no SMI TrafficTarget policies, so
+		// SAN matching is not required.
+		return secret, nil
+	}
+
+	// Program SAN matching based on SMI TrafficTarget policies
 	switch sdscert.CertType {
 	case envoy.RootCertTypeForMTLSOutbound:
 		fallthrough
 	case envoy.RootCertTypeForMTLSInbound:
 		var matchSANs []*xds_matcher.StringMatcher
-		var serverNames []service.MeshService
+		var serviceAccounts []service.K8sServiceAccount
 		var err error
 
 		// This block constructs a list of Server Names (peers) that are allowed to connect to the given service.
 		// The allowed list is derived from SMI's Traffic Policy.
 		if sdscert.CertType == envoy.RootCertTypeForMTLSOutbound {
 			// Outbound
-			serverNames, err = mc.ListAllowedOutboundServices(proxyServiceName)
+			serviceAccounts, err = s.meshCatalog.ListAllowedOutboundServiceAccounts(s.svcAccount)
 		} else {
 			// Inbound
-			serverNames, err = mc.ListAllowedInboundServices(proxyServiceName)
+			serviceAccounts, err = s.meshCatalog.ListAllowedInboundServiceAccounts(s.svcAccount)
 		}
 
 		if err != nil {
-			log.Error().Err(err).Msgf("Error getting server names for %s allowed Services %s", directionMap[sdscert.CertType], proxyServiceName)
+			log.Error().Err(err).Msgf("Error getting %s service accounts for proxy service %s", directionMap[sdscert.CertType], proxyService)
 			return nil, err
 		}
 
+		log.Trace().Msgf("%s Service accounts for proxy service account %s: %v", directionMap[sdscert.CertType], s.svcAccount, serviceAccounts)
 		var matchingCerts []string
-		for _, serverName := range serverNames {
-			matchingCerts = append(matchingCerts, serverName.GetCommonName().String())
+		for _, svcAccount := range serviceAccounts {
+			// OSM currently relies on kubernetes ServiceAccount for service identity
+			si := identity.GetKubernetesServiceIdentity(svcAccount, identity.ClusterLocalTrustDomain)
+			matchingCerts = append(matchingCerts, si.String())
 			match := xds_matcher.StringMatcher{
 				MatchPattern: &xds_matcher.StringMatcher_Exact{
-					Exact: serverName.GetCommonName().String(),
+					Exact: si.String(),
 				},
 			}
 			matchSANs = append(matchSANs, &match)
 		}
 
-		log.Trace().Msgf("Proxy for service %s will only allow %s SANs exactly matching: %+v", proxyServiceName, directionMap[sdscert.CertType], matchingCerts)
+		log.Trace().Msgf("Proxy for service %s will only allow %s SANs exactly matching: %v", proxyService, directionMap[sdscert.CertType], matchingCerts)
 
 		// Ensure the Subject Alternate Names (SAN) added by CertificateManager.IssueCertificate()
 		// matches what is allowed to connect to the downstream service as defined in TrafficPolicy.