Changing the wildcard matching logic for client cert common names (#178)

Microsoft ADO ID: 36341347

PR #168 added support for wildcard matching between client cert CNs and trusted CNs. This PR changes the wildcard matching logic based on a discussion with @qiluo-msft and @prsunny.

Changes:
1. Wildcard patterns starting with `*` but not `*.` (e.g., `*example.sonic`) are no longer valid. These patterns are skipped during the matching.
2. `*.` is not a valid trusted CN and will be skipped if present.
3. `*` must match one or more characters. For example, `*.example.sonic` will not match `.example.sonic`.

**Note 1:** Comparison is always case sensitive, but `*` can match both upper and lower case characters. For example, `*.example.sonic` matches `TEST.example.sonic`, but it does not match `TEST.EXAMPLE.SONIC`.

**Note 2:** Multi-level subdomain matching is allowed for wildcard patterns. For example, `*.example.sonic` matches `test.example.sonic`, `sub.test.example.sonic`, `one.sub.test.example.sonic`, and so on.

Author: mssonicbld

go-server-server/go/auth.go

@@ -7,26 +7,41 @@ import (
 )
 
 func CommonNameMatch(r *http.Request) bool {
-    // During client cert authentication, after the certificate chain is validated by
-    // TLS, here we will further check if at least one of the common names in the end-entity certificate
-	// matches one of the trusted common names of the server config.
-	for _, peercert := range r.TLS.PeerCertificates {
-		commonName := peercert.Subject.CommonName
-		log.Printf("info: CommonName in the client cert: %s", commonName)
-		for _, name := range trustedCertCommonNames {
-			if strings.HasPrefix(name, "*") {
+	// During client cert authentication, after the certificate chain is validated by
+	// TLS, here we will further check if at least one of the common names in the end-entity certificate
+	// matches one of the trusted common names in the server config.
+
+	for _, name := range trustedCertCommonNames {
+		is_wildcard := false
+		domain := name
+		if strings.HasPrefix(name, "*.") {
+			if len(name) < 3 {
+				log.Printf("warning: Skipping invalid trusted common name %s", name)
+				continue;
+			}
+			is_wildcard = true
+			domain = name[1:]  //strip "*" but keep the "." at the beginning
+		} else if strings.HasPrefix(name, "*") {
+			log.Printf("warning: Skipping invalid trusted common name %s", name)
+			continue;
+		}
+		for _, peercert := range r.TLS.PeerCertificates {
+			commonName := peercert.Subject.CommonName
+			if is_wildcard {
 				// wildcard common name matching
-				domain := name[1:]  //strip "*"
-				if strings.HasSuffix(commonName, domain) {
-					log.Printf("info: CommonName %s in the client cert matches trusted wildcard common name %s", commonName, name)
+				if len(commonName) > len(domain) && strings.HasSuffix(commonName, domain) {
+					log.Printf("info: Wildcard match between common name %s in the client cert and trusted common name %s", commonName, name)
+					return true;
+				}
+			} else {
+				if commonName == name {
+					log.Printf("info: Exact match with trusted common name %s", name)
 					return true;
 				}
-			} else if commonName == name {
-				return true;
 			}
 		}
 	}
 
-    log.Printf("error: Authentication Fail! None of the CommonNames in the client cert match any of the trusted common names")
-    return false;
+	log.Printf("error: Authentication Fail! None of the common names in the client cert match any of the trusted common names")
+	return false;
 }

supervisor/rest_api_test.conf

@@ -1,5 +1,5 @@
 [program:rest-api]
-command=/usr/sbin/go-server-server.test -test.coverprofile=/coverage.cov -systemtest=true -enablehttps=true -clientcert=/usr/sbin/cert/client/selfsigned.crt -servercert=/usr/sbin/cert/server/selfsigned.crt -serverkey=/usr/sbin/cert/server/selfsigned.key -localapitestdocker=true -clientcertcommonname=test.client.restapi.sonic,*.example.sonic,*test.sonic -loglevel trace
+command=/usr/sbin/go-server-server.test -test.coverprofile=/coverage.cov -systemtest=true -enablehttps=true -clientcert=/usr/sbin/cert/client/selfsigned.crt -servercert=/usr/sbin/cert/server/selfsigned.crt -serverkey=/usr/sbin/cert/server/selfsigned.key -localapitestdocker=true -clientcertcommonname=test.client.restapi.sonic,*.example.sonic,*test.sonic,*. -loglevel trace
 priority=1
 autostart=false
 autorestart=false

test/test_restapi.py

@@ -61,21 +61,39 @@ def __exit__(self, exc_type, exc_value, traceback):
 
 class TestClientCertAuth:
 
-    # Exact match tests for "test.client.restapi.sonic"
+    # Exact matching tests for "test.client.restapi.sonic"
 
     def test_exact_match_success(self, setup_restapi_client):
         _, _, _, restapi_client = setup_restapi_client
         with ClientCert("test.client.restapi.sonic") as client_cert:
             r = restapi_client.get_heartbeat(client_cert=(client_cert.cert, client_cert.key))
             assert r.status_code == 200
 
-    def test_exact_match_failure(self, setup_restapi_client):
+    def test_exact_match_failure_1(self, setup_restapi_client):
         _, _, _, restapi_client = setup_restapi_client
         with ClientCert("client.restapi.sonic") as client_cert:
             r = restapi_client.get_heartbeat(client_cert=(client_cert.cert, client_cert.key))
             assert r.status_code == 401
 
-    # Wildcard match tests for "*.example.sonic"
+    def test_exact_match_failure_2(self, setup_restapi_client):
+        _, _, _, restapi_client = setup_restapi_client
+        with ClientCert("test.client.restapi.com") as client_cert:
+            r = restapi_client.get_heartbeat(client_cert=(client_cert.cert, client_cert.key))
+            assert r.status_code == 401
+
+    def test_exact_match_failure_3(self, setup_restapi_client):
+        _, _, _, restapi_client = setup_restapi_client
+        with ClientCert("sub.test.client.restapi.sonic") as client_cert:
+            r = restapi_client.get_heartbeat(client_cert=(client_cert.cert, client_cert.key))
+            assert r.status_code == 401
+
+    def test_exact_match_failure_4(self, setup_restapi_client):
+        _, _, _, restapi_client = setup_restapi_client
+        with ClientCert("TEST.CLIENT.RESTAPI.SONIC") as client_cert:
+            r = restapi_client.get_heartbeat(client_cert=(client_cert.cert, client_cert.key))
+            assert r.status_code == 401
+
+    # Wildcard matching tests for "*.example.sonic"
 
     def test_wildcard_match_success_1(self, setup_restapi_client):
         _, _, _, restapi_client = setup_restapi_client
@@ -85,7 +103,19 @@ def test_wildcard_match_success_1(self, setup_restapi_client):
 
     def test_wildcard_match_success_2(self, setup_restapi_client):
         _, _, _, restapi_client = setup_restapi_client
-        with ClientCert("another.test.example.sonic") as client_cert:
+        with ClientCert("a.example.sonic") as client_cert:
+            r = restapi_client.get_heartbeat(client_cert=(client_cert.cert, client_cert.key))
+            assert r.status_code == 200
+
+    def test_wildcard_match_success_3(self, setup_restapi_client):
+        _, _, _, restapi_client = setup_restapi_client
+        with ClientCert("sub.test.example.sonic") as client_cert:
+            r = restapi_client.get_heartbeat(client_cert=(client_cert.cert, client_cert.key))
+            assert r.status_code == 200
+
+    def test_wildcard_match_success_4(self, setup_restapi_client):
+        _, _, _, restapi_client = setup_restapi_client
+        with ClientCert("TEST.example.sonic") as client_cert:
             r = restapi_client.get_heartbeat(client_cert=(client_cert.cert, client_cert.key))
             assert r.status_code == 200
 
@@ -119,38 +149,76 @@ def test_wildcard_match_failure_5(self, setup_restapi_client):
             r = restapi_client.get_heartbeat(client_cert=(client_cert.cert, client_cert.key))
             assert r.status_code == 401
 
-    # Wildcard match tests for "*test.sonic"
+    def test_wildcard_match_failure_6(self, setup_restapi_client):
+        _, _, _, restapi_client = setup_restapi_client
+        with ClientCert(".example.sonic") as client_cert:
+            r = restapi_client.get_heartbeat(client_cert=(client_cert.cert, client_cert.key))
+            assert r.status_code == 401
 
-    def test_wildcard_match_success_a(self, setup_restapi_client):
+    def test_wildcard_match_failure_7(self, setup_restapi_client):
+        _, _, _, restapi_client = setup_restapi_client
+        with ClientCert("TEST.EXAMPLE.SONIC") as client_cert:
+            r = restapi_client.get_heartbeat(client_cert=(client_cert.cert, client_cert.key))
+            assert r.status_code == 401
+
+    # Matching tests for "*test.sonic" (invalid CN)
+
+    def test_invalid_match_failure_1(self, setup_restapi_client):
         _, _, _, restapi_client = setup_restapi_client
         with ClientCert("mytest.sonic") as client_cert:
             r = restapi_client.get_heartbeat(client_cert=(client_cert.cert, client_cert.key))
-            assert r.status_code == 200
+            assert r.status_code == 401
 
-    def test_wildcard_match_success_b(self, setup_restapi_client):
+    def test_invalid_match_failure_2(self, setup_restapi_client):
         _, _, _, restapi_client = setup_restapi_client
         with ClientCert("test.sonic") as client_cert:
             r = restapi_client.get_heartbeat(client_cert=(client_cert.cert, client_cert.key))
-            assert r.status_code == 200
+            assert r.status_code == 401
 
-    def test_wildcard_match_success_c(self, setup_restapi_client):
+    def test_invalid_match_failure_3(self, setup_restapi_client):
         _, _, _, restapi_client = setup_restapi_client
-        with ClientCert("example.test.sonic") as client_cert:
+        with ClientCert("sub.test.sonic") as client_cert:
             r = restapi_client.get_heartbeat(client_cert=(client_cert.cert, client_cert.key))
-            assert r.status_code == 200
+            assert r.status_code == 401
 
-    def test_wildcard_match_failure_a(self, setup_restapi_client):
+    def test_invalid_match_failure_4(self, setup_restapi_client):
         _, _, _, restapi_client = setup_restapi_client
         with ClientCert("est.sonic") as client_cert:
             r = restapi_client.get_heartbeat(client_cert=(client_cert.cert, client_cert.key))
             assert r.status_code == 401
 
-    def test_wildcard_match_failure_b(self, setup_restapi_client):
+    def test_invalid_match_failure_5(self, setup_restapi_client):
         _, _, _, restapi_client = setup_restapi_client
         with ClientCert("test.sonico") as client_cert:
             r = restapi_client.get_heartbeat(client_cert=(client_cert.cert, client_cert.key))
             assert r.status_code == 401
 
+    # Corner cases
+
+    def test_empty_cn(self, setup_restapi_client):
+        _, _, _, restapi_client = setup_restapi_client
+        with ClientCert("") as client_cert:
+            r = restapi_client.get_heartbeat(client_cert=(client_cert.cert, client_cert.key))
+            assert r.status_code == 401
+
+    def test_missing_tld(self, setup_restapi_client):
+        _, _, _, restapi_client = setup_restapi_client
+        with ClientCert("test.example.") as client_cert:
+            r = restapi_client.get_heartbeat(client_cert=(client_cert.cert, client_cert.key))
+            assert r.status_code == 401
+
+    def test_match_all(self, setup_restapi_client):
+        _, _, _, restapi_client = setup_restapi_client
+        with ClientCert("*") as client_cert:
+            r = restapi_client.get_heartbeat(client_cert=(client_cert.cert, client_cert.key))
+            assert r.status_code == 401
+
+    def test_ends_with_dot(self, setup_restapi_client):
+        _, _, _, restapi_client = setup_restapi_client
+        with ClientCert("*.") as client_cert:
+            r = restapi_client.get_heartbeat(client_cert=(client_cert.cert, client_cert.key))
+            assert r.status_code == 401
+
 
 class TestRestApiPositive:
     """Normal behaviour tests"""