fix: remove current CN checks on CSMS client certificate

The requirements for checking the CN are rather different from what we expected. Removing the check for now. A different implementation will be required.
thoughtworks · Mar 1, 2024 · fe7b99d · fe7b99d
1 parent 13dd1ba
commit fe7b99d
Show file tree

Hide file tree

Showing 5 changed files with 116 additions and 117 deletions.
diff --git a/.talismanrc b/.talismanrc
@@ -22,7 +22,7 @@ fileignoreconfig:
   - filename: gateway/server/ws.go
     checksum: 4cbde936242380603e07cf8bd049dbca9d1c3108843d10e58f588540176c6d23
   - filename: gateway/server/ws_test.go
-    checksum: 4a9379d10dc5a63bb6af399f77ef8598f6a6a289f60b4a634a9150c5cec03dcf
+    checksum: 844d3cbd6d95dab9555f901c21fe37b6882831a18ea2b6b9120a9230ff2e6006
   - filename: manager/Dockerfile
     checksum: db69536d308b905d6214db1fb0e10442fe3e770ef3fed47809ca1f3ec099409d
   - filename: manager/adminui/server.go
@@ -182,7 +182,7 @@ fileignoreconfig:
   - filename: manager/services/charge_station_certificate_provider.go
     checksum: 3865ed0f6bb839cae026651b11839399aa4bd76476a973022a88c09f8cb491e0
   - filename: manager/services/charge_station_certificate_provider_test.go
-    checksum: 96505b6d7f2aa497f30982671d3c09a8988c02df5228beafe7aff5628d4dc025
+    checksum: e7f575707c64c6797bab5a6e6437a4ecc3c7e804335bae6ff92ae536c7968a9d
   - filename: manager/services/http_token_hubject_test.go
     checksum: 8c86e20c1bc4a233db56055dde20f62d90fc24e6f4981465cbcb638571cc07db
   - filename: manager/services/http_token_service.go

diff --git a/gateway/server/ws.go b/gateway/server/ws.go
@@ -420,13 +420,13 @@ func checkCertificate(ctx context.Context, r *http.Request, orgNames []string, c
 		return false
 	}
 
-	result := cs.ClientId == leafCertificate.Subject.CommonName
+	//result := cs.ClientId == leafCertificate.Subject.CommonName
+	//
+	//if !result {
+	//	span.SetAttributes(attribute.String("auth.failure_reason", "bad common name"))
+	//}
 
-	if !result {
-		span.SetAttributes(attribute.String("auth.failure_reason", "bad common name"))
-	}
-
-	return result
+	return foundOrg
 }
 
 func goPublishToCSMS(ctx context.Context, tracer trace.Tracer, csmsTx chan *pipe.GatewayMessage, mqttConn *autopaho.ConnectionManager, topicPrefix, protocol, clientId string) {

diff --git a/gateway/server/ws_test.go b/gateway/server/ws_test.go
@@ -533,63 +533,63 @@ func TestTlsConnectionWithCertificateAuthExpired(t *testing.T) {
 	}
 }
 
-func TestTlsConnectionWithCertificateAuthMismatchedName(t *testing.T) {
-	//defer goleak.VerifyNone(t)
-
-	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
-
-	cs := &registry.ChargeStation{
-		ClientId:        "tlsWithCertificateAuthMismatchedName",
-		SecurityProfile: registry.TLSWithClientSideCertificates,
-	}
-
-	caCert, _, clientCert, clientKeyPair := createTestKeyPairs(t, cs.ClientId+"Wrong")
-
-	mockRegistry := registry.NewMockRegistry()
-	mockRegistry.ChargeStations[cs.ClientId] = cs
-
-	srv := httptest.NewUnstartedServer(server.NewWebsocketHandler(
-		server.WithDeviceRegistry(mockRegistry),
-		server.WithOrgName("Thoughtworks")))
-	clientCAPool := x509.NewCertPool()
-	clientCAPool.AddCert(caCert)
-	srv.TLS = &tls.Config{
-		ClientCAs:  clientCAPool,
-		ClientAuth: tls.VerifyClientCertIfGiven,
-	}
-	srv.StartTLS()
-	defer srv.Close()
-
-	rootCAPool := x509.NewCertPool()
-	rootCAPool.AddCert(srv.Certificate())
-
-	clientTlsCert := tls.Certificate{
-		Certificate: [][]byte{clientCert.Raw, caCert.Raw},
-		PrivateKey:  clientKeyPair,
-	}
-
-	httpClient := &http.Client{
-		Transport: &http.Transport{
-			TLSClientConfig: &tls.Config{
-				RootCAs:      rootCAPool,
-				Certificates: []tls.Certificate{clientTlsCert},
-			},
-		},
-	}
-	dialOptions := &websocket.DialOptions{
-		HTTPClient:   httpClient,
-		Subprotocols: []string{"ocpp1.6", "ocpp2.0.1"},
-	}
-
-	_, resp, err := websocket.Dial(ctx, fmt.Sprintf("%s/ws/%s", srv.URL, cs.ClientId), dialOptions)
-	if err == nil {
-		t.Fatalf("expected error dialing CSMS")
-	}
-	if resp.StatusCode != http.StatusUnauthorized {
-		t.Fatalf("status code: want %d, got %d", http.StatusUnauthorized, resp.StatusCode)
-	}
-}
+//func TestTlsConnectionWithCertificateAuthMismatchedName(t *testing.T) {
+//	//defer goleak.VerifyNone(t)
+//
+//	ctx, cancel := context.WithCancel(context.Background())
+//	defer cancel()
+//
+//	cs := &registry.ChargeStation{
+//		ClientId:        "tlsWithCertificateAuthMismatchedName",
+//		SecurityProfile: registry.TLSWithClientSideCertificates,
+//	}
+//
+//	caCert, _, clientCert, clientKeyPair := createTestKeyPairs(t, cs.ClientId+"Wrong")
+//
+//	mockRegistry := registry.NewMockRegistry()
+//	mockRegistry.ChargeStations[cs.ClientId] = cs
+//
+//	srv := httptest.NewUnstartedServer(server.NewWebsocketHandler(
+//		server.WithDeviceRegistry(mockRegistry),
+//		server.WithOrgName("Thoughtworks")))
+//	clientCAPool := x509.NewCertPool()
+//	clientCAPool.AddCert(caCert)
+//	srv.TLS = &tls.Config{
+//		ClientCAs:  clientCAPool,
+//		ClientAuth: tls.VerifyClientCertIfGiven,
+//	}
+//	srv.StartTLS()
+//	defer srv.Close()
+//
+//	rootCAPool := x509.NewCertPool()
+//	rootCAPool.AddCert(srv.Certificate())
+//
+//	clientTlsCert := tls.Certificate{
+//		Certificate: [][]byte{clientCert.Raw, caCert.Raw},
+//		PrivateKey:  clientKeyPair,
+//	}
+//
+//	httpClient := &http.Client{
+//		Transport: &http.Transport{
+//			TLSClientConfig: &tls.Config{
+//				RootCAs:      rootCAPool,
+//				Certificates: []tls.Certificate{clientTlsCert},
+//			},
+//		},
+//	}
+//	dialOptions := &websocket.DialOptions{
+//		HTTPClient:   httpClient,
+//		Subprotocols: []string{"ocpp1.6", "ocpp2.0.1"},
+//	}
+//
+//	_, resp, err := websocket.Dial(ctx, fmt.Sprintf("%s/ws/%s", srv.URL, cs.ClientId), dialOptions)
+//	if err == nil {
+//		t.Fatalf("expected error dialing CSMS")
+//	}
+//	if resp.StatusCode != http.StatusUnauthorized {
+//		t.Fatalf("status code: want %d, got %d", http.StatusUnauthorized, resp.StatusCode)
+//	}
+//}
 
 //func TestConnectionWhenUnableToConnectToMqtt(t *testing.T) {
 //	//defer goleak.VerifyNone(t)

diff --git a/manager/services/charge_station_certificate_provider.go b/manager/services/charge_station_certificate_provider.go
@@ -260,14 +260,14 @@ func (l *LocalChargeStationCertificateProvider) ProvideCertificate(ctx context.C
 		return "", fmt.Errorf("local provider cannot provide V2G certificate")
 	}
 
-	cn, err := getCommonNameFromCSR(pemEncodedCSR)
-	if err != nil {
-		return "", err
-	}
-
-	if cn != csId {
-		return "", fmt.Errorf("certificate common name does not match charge station id")
-	}
+	//cn, err := getCommonNameFromCSR(pemEncodedCSR)
+	//if err != nil {
+	//	return "", err
+	//}
+	//
+	//if cn != csId {
+	//	return "", fmt.Errorf("certificate common name does not match charge station id")
+	//}
 
 	certificate, err := readSigningCertificate(ctx, l.CertificateReader)
 	if err != nil {
@@ -445,14 +445,14 @@ func (d *DelegatingChargeStationCertificateProvider) ProvideCertificate(ctx cont
 	}
 }
 
-func getCommonNameFromCSR(csr string) (string, error) {
-	block, _ := pem.Decode([]byte(csr))
-	if block != nil && block.Type == "CERTIFICATE REQUEST" {
-		csr, err := x509.ParseCertificateRequest(block.Bytes)
-		if err != nil {
-			return "", fmt.Errorf("parsing certificate request: %w", err)
-		}
-		return csr.Subject.CommonName, nil
-	}
-	return "", fmt.Errorf("no certificate request in PEM block")
-}
+//func getCommonNameFromCSR(csr string) (string, error) {
+//	block, _ := pem.Decode([]byte(csr))
+//	if block != nil && block.Type == "CERTIFICATE REQUEST" {
+//		csr, err := x509.ParseCertificateRequest(block.Bytes)
+//		if err != nil {
+//			return "", fmt.Errorf("parsing certificate request: %w", err)
+//		}
+//		return csr.Subject.CommonName, nil
+//	}
+//	return "", fmt.Errorf("no certificate request in PEM block")
+//}
diff --git a/manager/services/charge_station_certificate_provider_test.go b/manager/services/charge_station_certificate_provider_test.go
@@ -230,39 +230,38 @@ func TestLocalChargeStationCertificateProvider(t *testing.T) {
 	require.Equal(t, 2, count)
 }
 
-func TestLocalChargeStationCertificateProviderWithWrongId(t *testing.T) {
-	store := inmemory.NewStore(clock.RealClock{})
-
-	caCert, caKey := createRootCACertificate(t, "test")
-	intCert, intKey := createIntermediateCACertificate(t, "int", "", caCert, caKey)
-
-	privateKey, err := x509.MarshalPKCS8PrivateKey(intKey)
-	require.NoError(t, err)
-
-	pemCertificate := pem.EncodeToMemory(&pem.Block{
-		Type:  "CERTIFICATE",
-		Bytes: intCert.Raw,
-	})
-
-	pemPrivateKey := pem.EncodeToMemory(&pem.Block{
-		Type:  "PRIVATE KEY",
-		Bytes: privateKey,
-	})
-
-	certificateProvider := &services.LocalChargeStationCertificateProvider{
-		Store:             store,
-		CertificateReader: services.StringSource{Data: string(pemCertificate)},
-		PrivateKeyReader:  services.StringSource{Data: string(pemPrivateKey)},
-	}
-
-	pemCsr := createCertificateSigningRequest(t)
-
-	ctx := context.TODO()
-	chain, err := certificateProvider.ProvideCertificate(ctx, services.CertificateTypeCSO, string(pemCsr), "not-cs001")
-	assert.Error(t, err)
-	assert.Equal(t, "", chain)
-}
-
+//	func TestLocalChargeStationCertificateProviderWithWrongId(t *testing.T) {
+//		store := inmemory.NewStore(clock.RealClock{})
+//
+//		caCert, caKey := createRootCACertificate(t, "test")
+//		intCert, intKey := createIntermediateCACertificate(t, "int", "", caCert, caKey)
+//
+//		privateKey, err := x509.MarshalPKCS8PrivateKey(intKey)
+//		require.NoError(t, err)
+//
+//		pemCertificate := pem.EncodeToMemory(&pem.Block{
+//			Type:  "CERTIFICATE",
+//			Bytes: intCert.Raw,
+//		})
+//
+//		pemPrivateKey := pem.EncodeToMemory(&pem.Block{
+//			Type:  "PRIVATE KEY",
+//			Bytes: privateKey,
+//		})
+//
+//		certificateProvider := &services.LocalChargeStationCertificateProvider{
+//			Store:             store,
+//			CertificateReader: services.StringSource{Data: string(pemCertificate)},
+//			PrivateKeyReader:  services.StringSource{Data: string(pemPrivateKey)},
+//		}
+//
+//		pemCsr := createCertificateSigningRequest(t)
+//
+//		ctx := context.TODO()
+//		chain, err := certificateProvider.ProvideCertificate(ctx, services.CertificateTypeCSO, string(pemCsr), "not-cs001")
+//		assert.Error(t, err)
+//		assert.Equal(t, "", chain)
+//	}
 func TestLocalChargeStationCertificateProviderWithRSAKey(t *testing.T) {
 	store := inmemory.NewStore(clock.RealClock{})