fix(pki): add control character check to domain validation
Resolve merge conflict from stash and deduplicate validateDomain. Keep the hardened version with control character rejection (ASCII 0-31) as suggested by CodeRabbit review.
- commit
- 450331c9b098b0d2f1d28cc4c7b860b2395e606b
- author
- brice · · 2026-03-30 18:42 UTC
- parent
- 63f113e
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
2 files · +31 -25
M internal/adapters/out/pki/ca.go +26 -21
| @@ -432,6 +432,32 @@ func parseCertAndKey(certPEM, keyPEM []byte) (*x509.Certificate, crypto.Signer, | |||
| 432 | 432 | return cert, key, nil | |
| 433 | 433 | } | |
| 434 | 434 | ||
| 435 | + | // validateDomain performs defense-in-depth validation of a domain name | |
| 436 | + | // before issuing a certificate. | |
| 437 | + | func validateDomain(domain string) error { | |
| 438 | + | if domain == "" { | |
| 439 | + | return fmt.Errorf("empty domain") | |
| 440 | + | } | |
| 441 | + | for _, r := range domain { | |
| 442 | + | if r < 32 { | |
| 443 | + | return fmt.Errorf("domain contains control characters") | |
| 444 | + | } | |
| 445 | + | } | |
| 446 | + | if len(domain) > 253 { | |
| 447 | + | return fmt.Errorf("domain too long (%d chars), limit is 253", len(domain)) | |
| 448 | + | } | |
| 449 | + | if strings.HasPrefix(domain, "*") { | |
| 450 | + | return fmt.Errorf("wildcard domains not allowed") | |
| 451 | + | } | |
| 452 | + | if strings.ContainsAny(domain, "/\\") { | |
| 453 | + | return fmt.Errorf("domain contains path separators") | |
| 454 | + | } | |
| 455 | + | if net.ParseIP(domain) != nil { | |
| 456 | + | return fmt.Errorf("IP addresses not allowed, must be a domain name") | |
| 457 | + | } | |
| 458 | + | return nil | |
| 459 | + | } | |
| 460 | + | ||
| 435 | 461 | func writeSecure(path string, data []byte, perm os.FileMode) (retErr error) { | |
| 436 | 462 | dir := filepath.Dir(path) | |
| 437 | 463 | if err := os.MkdirAll(dir, 0700); err != nil { | |
| @@ -464,27 +490,6 @@ func writeSecure(path string, data []byte, perm os.FileMode) (retErr error) { | |||
| 464 | 490 | return nil | |
| 465 | 491 | } | |
| 466 | 492 | ||
| 467 | - | // validateDomain performs defense-in-depth validation of a domain name | |
| 468 | - | // before issuing a certificate. | |
| 469 | - | func validateDomain(domain string) error { | |
| 470 | - | if domain == "" { | |
| 471 | - | return fmt.Errorf("empty domain") | |
| 472 | - | } | |
| 473 | - | if len(domain) > 253 { | |
| 474 | - | return fmt.Errorf("domain too long (%d chars)", len(domain)) | |
| 475 | - | } | |
| 476 | - | if strings.HasPrefix(domain, "*") { | |
| 477 | - | return fmt.Errorf("wildcard domains not allowed") | |
| 478 | - | } | |
| 479 | - | if strings.ContainsAny(domain, "/\\") { | |
| 480 | - | return fmt.Errorf("domain contains path separator") | |
| 481 | - | } | |
| 482 | - | if net.ParseIP(domain) != nil { | |
| 483 | - | return fmt.Errorf("IP addresses not allowed, must be a hostname") | |
| 484 | - | } | |
| 485 | - | return nil | |
| 486 | - | } | |
| 487 | - | ||
| 488 | 493 | // verifyKeyPair checks that a certificate's public key matches the private key. | |
| 489 | 494 | func verifyKeyPair(cert *x509.Certificate, key crypto.Signer) error { | |
| 490 | 495 | certPub, ok := cert.PublicKey.(*ecdsa.PublicKey) | |
| @@ -432,6 +432,32 @@ func parseCertAndKey(certPEM, keyPEM []byte) (*x509.Certificate, crypto.Signer, | |||||
| 432 | return cert, key, nil | 432 | return cert, key, nil | ||
| 433 | } | 433 | } | ||
| 434 | 434 | ||||
| 435 | + | // validateDomain performs defense-in-depth validation of a domain name | |||
| 436 | + | // before issuing a certificate. | |||
| 437 | + | func validateDomain(domain string) error { | |||
| 438 | + | if domain == "" { | |||
| 439 | + | return fmt.Errorf("empty domain") | |||
| 440 | + | } | |||
| 441 | + | for _, r := range domain { | |||
| 442 | + | if r < 32 { | |||
| 443 | + | return fmt.Errorf("domain contains control characters") | |||
| 444 | + | } | |||
| 445 | + | } | |||
| 446 | + | if len(domain) > 253 { | |||
| 447 | + | return fmt.Errorf("domain too long (%d chars), limit is 253", len(domain)) | |||
| 448 | + | } | |||
| 449 | + | if strings.HasPrefix(domain, "*") { | |||
| 450 | + | return fmt.Errorf("wildcard domains not allowed") | |||
| 451 | + | } | |||
| 452 | + | if strings.ContainsAny(domain, "/\\") { | |||
| 453 | + | return fmt.Errorf("domain contains path separators") | |||
| 454 | + | } | |||
| 455 | + | if net.ParseIP(domain) != nil { | |||
| 456 | + | return fmt.Errorf("IP addresses not allowed, must be a domain name") | |||
| 457 | + | } | |||
| 458 | + | return nil | |||
| 459 | + | } | |||
| 460 | + | ||||
| 435 | func writeSecure(path string, data []byte, perm os.FileMode) (retErr error) { | 461 | func writeSecure(path string, data []byte, perm os.FileMode) (retErr error) { | ||
| 436 | dir := filepath.Dir(path) | 462 | dir := filepath.Dir(path) | ||
| 437 | if err := os.MkdirAll(dir, 0700); err != nil { | 463 | if err := os.MkdirAll(dir, 0700); err != nil { | ||
| @@ -464,27 +490,6 @@ func writeSecure(path string, data []byte, perm os.FileMode) (retErr error) { | |||||
| 464 | return nil | 490 | return nil | ||
| 465 | } | 491 | } | ||
| 466 | 492 | ||||
| 467 | - | // validateDomain performs defense-in-depth validation of a domain name | |||
| 468 | - | // before issuing a certificate. | |||
| 469 | - | func validateDomain(domain string) error { | |||
| 470 | - | if domain == "" { | |||
| 471 | - | return fmt.Errorf("empty domain") | |||
| 472 | - | } | |||
| 473 | - | if len(domain) > 253 { | |||
| 474 | - | return fmt.Errorf("domain too long (%d chars)", len(domain)) | |||
| 475 | - | } | |||
| 476 | - | if strings.HasPrefix(domain, "*") { | |||
| 477 | - | return fmt.Errorf("wildcard domains not allowed") | |||
| 478 | - | } | |||
| 479 | - | if strings.ContainsAny(domain, "/\\") { | |||
| 480 | - | return fmt.Errorf("domain contains path separator") | |||
| 481 | - | } | |||
| 482 | - | if net.ParseIP(domain) != nil { | |||
| 483 | - | return fmt.Errorf("IP addresses not allowed, must be a hostname") | |||
| 484 | - | } | |||
| 485 | - | return nil | |||
| 486 | - | } | |||
| 487 | - | ||||
| 488 | // verifyKeyPair checks that a certificate's public key matches the private key. | 493 | // verifyKeyPair checks that a certificate's public key matches the private key. | ||
| 489 | func verifyKeyPair(cert *x509.Certificate, key crypto.Signer) error { | 494 | func verifyKeyPair(cert *x509.Certificate, key crypto.Signer) error { | ||
| 490 | certPub, ok := cert.PublicKey.(*ecdsa.PublicKey) | 495 | certPub, ok := cert.PublicKey.(*ecdsa.PublicKey) | ||
M internal/usecase/pki/service.go +5 -4
| @@ -74,6 +74,11 @@ func (s *Service) GetCertificate(hello *tls.ClientHelloInfo) (*tls.Certificate, | |||
| 74 | 74 | return nil, fmt.Errorf("no SNI in ClientHello") | |
| 75 | 75 | } | |
| 76 | 76 | ||
| 77 | + | if !s.isDomainAllowed(hello.Context(), domain) { | |
| 78 | + | s.cache.Delete(domain) | |
| 79 | + | return nil, fmt.Errorf("domain %q not in route table", domain) | |
| 80 | + | } | |
| 81 | + | ||
| 77 | 82 | if entry, ok := s.cache.Load(domain); ok { | |
| 78 | 83 | if cached, ok := entry.(*cachedCert); ok { | |
| 79 | 84 | if time.Now().Before(cached.expiresAt) { | |
| @@ -83,10 +88,6 @@ func (s *Service) GetCertificate(hello *tls.ClientHelloInfo) (*tls.Certificate, | |||
| 83 | 88 | s.cache.Delete(domain) | |
| 84 | 89 | } | |
| 85 | 90 | ||
| 86 | - | if !s.isDomainAllowed(hello.Context(), domain) { | |
| 87 | - | return nil, fmt.Errorf("domain %q not in route table", domain) | |
| 88 | - | } | |
| 89 | - | ||
| 90 | 91 | cert, err := s.ca.IssueCertificate(domain) | |
| 91 | 92 | if err != nil { | |
| 92 | 93 | s.log.Error().Err(err).Str("domain", domain).Msg("failed to issue leaf cert") | |
| @@ -74,6 +74,11 @@ func (s *Service) GetCertificate(hello *tls.ClientHelloInfo) (*tls.Certificate, | |||||
| 74 | return nil, fmt.Errorf("no SNI in ClientHello") | 74 | return nil, fmt.Errorf("no SNI in ClientHello") | ||
| 75 | } | 75 | } | ||
| 76 | 76 | ||||
| 77 | + | if !s.isDomainAllowed(hello.Context(), domain) { | |||
| 78 | + | s.cache.Delete(domain) | |||
| 79 | + | return nil, fmt.Errorf("domain %q not in route table", domain) | |||
| 80 | + | } | |||
| 81 | + | ||||
| 77 | if entry, ok := s.cache.Load(domain); ok { | 82 | if entry, ok := s.cache.Load(domain); ok { | ||
| 78 | if cached, ok := entry.(*cachedCert); ok { | 83 | if cached, ok := entry.(*cachedCert); ok { | ||
| 79 | if time.Now().Before(cached.expiresAt) { | 84 | if time.Now().Before(cached.expiresAt) { | ||
| @@ -83,10 +88,6 @@ func (s *Service) GetCertificate(hello *tls.ClientHelloInfo) (*tls.Certificate, | |||||
| 83 | s.cache.Delete(domain) | 88 | s.cache.Delete(domain) | ||
| 84 | } | 89 | } | ||
| 85 | 90 | ||||
| 86 | - | if !s.isDomainAllowed(hello.Context(), domain) { | |||
| 87 | - | return nil, fmt.Errorf("domain %q not in route table", domain) | |||
| 88 | - | } | |||
| 89 | - | ||||
| 90 | cert, err := s.ca.IssueCertificate(domain) | 91 | cert, err := s.ca.IssueCertificate(domain) | ||
| 91 | if err != nil { | 92 | if err != nil { | ||
| 92 | s.log.Error().Err(err).Str("domain", domain).Msg("failed to issue leaf cert") | 93 | s.log.Error().Err(err).Str("domain", domain).Msg("failed to issue leaf cert") | ||