From b04872080c1207f738526959414204f355560d46 Mon Sep 17 00:00:00 2001 From: Gregory Cooke Date: Wed, 10 Apr 2024 19:55:23 +0000 Subject: [PATCH 1/5] unexport revocatio checking fns --- security/advancedtls/advancedtls.go | 2 +- security/advancedtls/crl.go | 10 +++++----- security/advancedtls/crl_test.go | 10 +++++----- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/security/advancedtls/advancedtls.go b/security/advancedtls/advancedtls.go index 4b5d1f4825c9..abc34fd9f9cd 100644 --- a/security/advancedtls/advancedtls.go +++ b/security/advancedtls/advancedtls.go @@ -548,7 +548,7 @@ func buildVerifyFunc(c *advancedTLSCreds, if verifiedChains == nil { verifiedChains = [][]*x509.Certificate{rawCertList} } - if err := CheckChainRevocation(verifiedChains, *c.revocationConfig); err != nil { + if err := checkChainRevocation(verifiedChains, *c.revocationConfig); err != nil { return err } } diff --git a/security/advancedtls/crl.go b/security/advancedtls/crl.go index a0e9eb3c6532..8b2fd2a79e9c 100644 --- a/security/advancedtls/crl.go +++ b/security/advancedtls/crl.go @@ -192,7 +192,7 @@ func x509NameHash(r pkix.RDNSequence) string { return fmt.Sprintf("%08x", fileHash) } -// CheckRevocation checks the connection for revoked certificates based on RFC5280. +// checkRevocation checks the connection for revoked certificates based on RFC5280. // This implementation has the following major limitations: // - Indirect CRL files are not supported. // - CRL loading is only supported from directories in the X509_LOOKUP_hash_dir format. @@ -200,13 +200,13 @@ func x509NameHash(r pkix.RDNSequence) string { // - Delta CRL files are not supported. // - Certificate CRLDistributionPoint must be URLs, but are then ignored and converted into a file path. // - CRL checks are done after path building, which goes against RFC4158. -func CheckRevocation(conn tls.ConnectionState, cfg RevocationConfig) error { - return CheckChainRevocation(conn.VerifiedChains, cfg) +func checkRevocation(conn tls.ConnectionState, cfg RevocationConfig) error { + return checkChainRevocation(conn.VerifiedChains, cfg) } -// CheckChainRevocation checks the verified certificate chain +// checkChainRevocation checks the verified certificate chain // for revoked certificates based on RFC5280. -func CheckChainRevocation(verifiedChains [][]*x509.Certificate, cfg RevocationConfig) error { +func checkChainRevocation(verifiedChains [][]*x509.Certificate, cfg RevocationConfig) error { // Iterate the verified chains looking for one that is RevocationUnrevoked. // A single RevocationUnrevoked chain is enough to allow the connection, and a single RevocationRevoked // chain does not mean the connection should fail. diff --git a/security/advancedtls/crl_test.go b/security/advancedtls/crl_test.go index ddef7862d3b7..134dca09b2ca 100644 --- a/security/advancedtls/crl_test.go +++ b/security/advancedtls/crl_test.go @@ -601,12 +601,12 @@ func TestRevokedCert(t *testing.T) { for _, tt := range revocationTests { t.Run(fmt.Sprintf("%v with x509 crl hash dir", tt.desc), func(t *testing.T) { - err := CheckRevocation(tt.in, RevocationConfig{ + err := checkRevocation(tt.in, RevocationConfig{ RootDir: testdata.Path("crl"), AllowUndetermined: tt.allowUndetermined, Cache: cache, }) - t.Logf("CheckRevocation err = %v", err) + t.Logf("checkRevocation err = %v", err) if tt.revoked && err == nil { t.Error("Revoked certificate chain was allowed") } else if !tt.revoked && err != nil { @@ -614,11 +614,11 @@ func TestRevokedCert(t *testing.T) { } }) t.Run(fmt.Sprintf("%v with static provider", tt.desc), func(t *testing.T) { - err := CheckRevocation(tt.in, RevocationConfig{ + err := checkRevocation(tt.in, RevocationConfig{ AllowUndetermined: tt.allowUndetermined, CRLProvider: cRLProvider, }) - t.Logf("CheckRevocation err = %v", err) + t.Logf("checkRevocation err = %v", err) if tt.revoked && err == nil { t.Error("Revoked certificate chain was allowed") } else if !tt.revoked && err != nil { @@ -739,7 +739,7 @@ func TestVerifyConnection(t *testing.T) { cliCfg := tls.Config{ RootCAs: cp, VerifyConnection: func(cs tls.ConnectionState) error { - return CheckRevocation(cs, RevocationConfig{RootDir: dir}) + return checkRevocation(cs, RevocationConfig{RootDir: dir}) }, } conn, err := tls.Dial(lis.Addr().Network(), lis.Addr().String(), &cliCfg) From 3a76a85643c1df93599e1ecf6ad8f9d61bf481c2 Mon Sep 17 00:00:00 2001 From: Gregory Cooke Date: Wed, 10 Apr 2024 19:58:41 +0000 Subject: [PATCH 2/5] unexport ServerNameOverride in advancedtls --- security/advancedtls/advancedtls.go | 10 +++++----- security/advancedtls/advancedtls_test.go | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/security/advancedtls/advancedtls.go b/security/advancedtls/advancedtls.go index abc34fd9f9cd..f5aa19eef8e1 100644 --- a/security/advancedtls/advancedtls.go +++ b/security/advancedtls/advancedtls.go @@ -172,10 +172,6 @@ type ClientOptions struct { // If this is set, we will perform this customized check after doing the // normal check(s) indicated by setting VType. VerifyPeer CustomVerificationFunc - // ServerNameOverride is for testing only. If set to a non-empty string, - // it will override the virtual host name of authority (e.g. :authority - // header field) in requests. - ServerNameOverride string // RootOptions is OPTIONAL on client side. If not set, we will try to use the // default trust certificates in users' OS system. RootOptions RootCertificateOptions @@ -193,6 +189,10 @@ type ClientOptions struct { // By default, the maximum version supported by this package is used, // which is currently TLS 1.3. MaxVersion uint16 + // serverNameOverride is for testing only. If set to a non-empty string, + // it will override the virtual host name of authority (e.g. :authority + // header field) in requests. + serverNameOverride string } // ServerOptions contains the fields needed to be filled by the server. @@ -244,7 +244,7 @@ func (o *ClientOptions) config() (*tls.Config, error) { return nil, fmt.Errorf("the minimum TLS version is larger than the maximum TLS version") } config := &tls.Config{ - ServerName: o.ServerNameOverride, + ServerName: o.serverNameOverride, // We have to set InsecureSkipVerify to true to skip the default checks and // use the verification function we built from buildVerifyFunc. InsecureSkipVerify: true, diff --git a/security/advancedtls/advancedtls_test.go b/security/advancedtls/advancedtls_test.go index f9671944edf4..f33a21a45cb3 100644 --- a/security/advancedtls/advancedtls_test.go +++ b/security/advancedtls/advancedtls_test.go @@ -902,7 +902,7 @@ func (s) TestAdvancedTLSOverrideServerName(t *testing.T) { RootOptions: RootCertificateOptions{ RootCACerts: cs.ClientTrust1, }, - ServerNameOverride: expectedServerName, + serverNameOverride: expectedServerName, } c, err := NewClientCreds(clientOptions) if err != nil { From 166ecf445bde8e7b5f83b3f644f4ad04aeea9999 Mon Sep 17 00:00:00 2001 From: Gregory Cooke Date: Wed, 10 Apr 2024 20:00:06 +0000 Subject: [PATCH 3/5] unexport RevocationStatus --- security/advancedtls/crl.go | 16 ++++++++-------- security/advancedtls/crl_test.go | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/security/advancedtls/crl.go b/security/advancedtls/crl.go index 8b2fd2a79e9c..b245c5a59b18 100644 --- a/security/advancedtls/crl.go +++ b/security/advancedtls/crl.go @@ -75,19 +75,19 @@ type RevocationConfig struct { CRLProvider CRLProvider } -// RevocationStatus is the revocation status for a certificate or chain. -type RevocationStatus int +// revocationStatus is the revocation status for a certificate or chain. +type revocationStatus int const ( // RevocationUndetermined means we couldn't find or verify a CRL for the cert. - RevocationUndetermined RevocationStatus = iota + RevocationUndetermined revocationStatus = iota // RevocationUnrevoked means we found the CRL for the cert and the cert is not revoked. RevocationUnrevoked // RevocationRevoked means we found the CRL and the cert is revoked. RevocationRevoked ) -func (s RevocationStatus) String() string { +func (s revocationStatus) String() string { return [...]string{"RevocationUndetermined", "RevocationUnrevoked", "RevocationRevoked"}[s] } @@ -210,7 +210,7 @@ func checkChainRevocation(verifiedChains [][]*x509.Certificate, cfg RevocationCo // Iterate the verified chains looking for one that is RevocationUnrevoked. // A single RevocationUnrevoked chain is enough to allow the connection, and a single RevocationRevoked // chain does not mean the connection should fail. - count := make(map[RevocationStatus]int) + count := make(map[revocationStatus]int) for _, chain := range verifiedChains { switch checkChain(chain, cfg) { case RevocationUnrevoked: @@ -236,7 +236,7 @@ func checkChainRevocation(verifiedChains [][]*x509.Certificate, cfg RevocationCo // 1. If any certificate is RevocationRevoked, return RevocationRevoked. // 2. If any certificate is RevocationUndetermined, return RevocationUndetermined. // 3. If all certificates are RevocationUnrevoked, return RevocationUnrevoked. -func checkChain(chain []*x509.Certificate, cfg RevocationConfig) RevocationStatus { +func checkChain(chain []*x509.Certificate, cfg RevocationConfig) revocationStatus { chainStatus := RevocationUnrevoked for _, c := range chain { switch checkCert(c, chain, cfg) { @@ -318,7 +318,7 @@ func fetchCRL(c *x509.Certificate, crlVerifyCrt []*x509.Certificate, cfg Revocat // RevocationUndetermined. // c is the certificate to check. // crlVerifyCrt is the group of possible certificates to verify the crl. -func checkCert(c *x509.Certificate, crlVerifyCrt []*x509.Certificate, cfg RevocationConfig) RevocationStatus { +func checkCert(c *x509.Certificate, crlVerifyCrt []*x509.Certificate, cfg RevocationConfig) revocationStatus { crl, err := fetchCRL(c, crlVerifyCrt, cfg) if err != nil { // We couldn't load any valid CRL files for the certificate, so we don't @@ -343,7 +343,7 @@ func checkCert(c *x509.Certificate, crlVerifyCrt []*x509.Certificate, cfg Revoca return revocation } -func checkCertRevocation(c *x509.Certificate, crl *CRL) (RevocationStatus, error) { +func checkCertRevocation(c *x509.Certificate, crl *CRL) (revocationStatus, error) { // Per section 5.3.3 we prime the certificate issuer with the CRL issuer. // Subsequent entries use the previous entry's issuer. rawEntryIssuer := crl.rawIssuer diff --git a/security/advancedtls/crl_test.go b/security/advancedtls/crl_test.go index 134dca09b2ca..3e06a6336e21 100644 --- a/security/advancedtls/crl_test.go +++ b/security/advancedtls/crl_test.go @@ -229,7 +229,7 @@ qsSIp8gfxSyzkJP+Ngkm2DdLjlJQCZ9R0MZP9Xj4 var revocationTests = []struct { desc string in x509.Certificate - revoked RevocationStatus + revoked revocationStatus }{ { desc: "Single revoked", From 533631be8530de01d3a19e3d27c3fc2c17eb042a Mon Sep 17 00:00:00 2001 From: Gregory Cooke Date: Wed, 10 Apr 2024 20:01:10 +0000 Subject: [PATCH 4/5] remove revocation status String because it is unused --- security/advancedtls/crl.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/security/advancedtls/crl.go b/security/advancedtls/crl.go index b245c5a59b18..efd404c89737 100644 --- a/security/advancedtls/crl.go +++ b/security/advancedtls/crl.go @@ -87,10 +87,6 @@ const ( RevocationRevoked ) -func (s revocationStatus) String() string { - return [...]string{"RevocationUndetermined", "RevocationUnrevoked", "RevocationRevoked"}[s] -} - // CRL contains a pkix.CertificateList and parsed extensions that aren't // provided by the golang CRL parser. // All CRLs should be loaded using NewCRL() for bytes directly or ReadCRLFile() From 4aac1bb12a35e6473a6c061af23a83eb6f8db777 Mon Sep 17 00:00:00 2001 From: Gregory Cooke Date: Thu, 11 Apr 2024 18:58:45 +0000 Subject: [PATCH 5/5] add more comment to serverNameOverride --- security/advancedtls/advancedtls.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/security/advancedtls/advancedtls.go b/security/advancedtls/advancedtls.go index f5aa19eef8e1..2aff11e7e41c 100644 --- a/security/advancedtls/advancedtls.go +++ b/security/advancedtls/advancedtls.go @@ -189,9 +189,10 @@ type ClientOptions struct { // By default, the maximum version supported by this package is used, // which is currently TLS 1.3. MaxVersion uint16 - // serverNameOverride is for testing only. If set to a non-empty string, - // it will override the virtual host name of authority (e.g. :authority - // header field) in requests. + // serverNameOverride is for testing only. If set to a non-empty string, it + // will override the virtual host name of authority (e.g. :authority header + // field) in requests and the target hostname used during server cert + // verification. serverNameOverride string }