filtering mTLS connections based on the subject name from Caller #4081
Conversation
ZetaoZhuang
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR adds TLS client certificate subject name filtering functionality to the CNS service. It introduces a new configuration field AllowedClientSubjectName that allows filtering TLS connections based on the subject name from the client certificate in mutual TLS connections.
- Added
AllowedClientSubjectNameconfiguration field to control client certificate validation - Implemented custom certificate verification logic using
VerifyPeerCertificatecallback - Updated tests to verify both allowed and disallowed client subject names
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/tls/tlscertificate_retriever.go | Added AllowedClientSubjectName field to TlsSettings struct |
| cns/service.go | Implemented verifyPeerCertificate function and integrated it into TLS configurations |
| cns/service/main.go | Wired up the new configuration field from CNS config to TLS settings |
| cns/configuration/configuration.go | Added AllowedClientSubjectName field to CNSConfig struct |
| cns/configuration/configuration_test.go | Updated tests to include the new configuration field |
| cns/configuration/cns_config.json | Added default empty value for the new configuration field |
| cns/service_test.go | Enhanced mutual TLS tests to verify client subject name filtering |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| err = cert.VerifyHostname(clientSubjectName) | ||
| if err != nil { | ||
| return errors.Errorf("failed to verify client certificate hostname: %v", err) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Using VerifyHostname for client certificate subject name verification is incorrect. VerifyHostname is designed for server certificate validation and checks hostname against DNS names and CN fields. For client certificate subject name validation, you should directly compare against the certificate's Subject.CommonName or implement proper subject name matching logic.
| err = cert.VerifyHostname(clientSubjectName) | |
| if err != nil { | |
| return errors.Errorf("failed to verify client certificate hostname: %v", err) | |
| } | |
| return nil | |
| if cert.Subject.CommonName != clientSubjectName { | |
| return errors.Errorf("client certificate subject name mismatch: got %q, want %q", cert.Subject.CommonName, clientSubjectName) | |
| } | |
| return nil |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Yep this function is designed for server certificate validation actually. But its functionality is exactly what I want. It will validate the given strings with SANs /IPaddress of the certs.
| if clientSubjectName == "" { | ||
| return nil | ||
| } | ||
|
|
There was a problem hiding this comment.
No validation that rawCerts slice is not empty before accessing rawCerts[0]. This could cause a panic if an empty slice is passed.
| if len(rawCerts) == 0 { | |
| return errors.New("no client certificate provided") | |
| } |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
No validation that rawCerts slice is not empty
if len(rawCerts) == 0 {
???
2 participants
Reason for Change:
filter mTLS connections based on SANs from client cert. This is pre-requisite to isolate CNS in separate infra virtual networks.
Issue Fixed:
Requirements:
Notes: