Skip to content

Commit

Permalink
advancedtls: unexport parts of API not meant to be public (#7118)
Browse files Browse the repository at this point in the history
  • Loading branch information
gtcooke94 authored Apr 17, 2024
1 parent 006e2ba commit fc8da03
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 29 deletions.
13 changes: 7 additions & 6 deletions security/advancedtls/advancedtls.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -193,6 +189,11 @@ 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 and the target hostname used during server cert
// verification.
serverNameOverride string
}

// ServerOptions contains the fields needed to be filled by the server.
Expand Down Expand Up @@ -244,7 +245,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,
Expand Down Expand Up @@ -548,7 +549,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
}
}
Expand Down
2 changes: 1 addition & 1 deletion security/advancedtls/advancedtls_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
28 changes: 12 additions & 16 deletions security/advancedtls/crl.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,22 +75,18 @@ 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 {
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()
Expand Down Expand Up @@ -192,25 +188,25 @@ 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.
// - OnlySomeReasons is not supported.
// - 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.
count := make(map[RevocationStatus]int)
count := make(map[revocationStatus]int)
for _, chain := range verifiedChains {
switch checkChain(chain, cfg) {
case RevocationUnrevoked:
Expand All @@ -236,7 +232,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) {
Expand Down Expand Up @@ -318,7 +314,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
Expand All @@ -343,7 +339,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
Expand Down
12 changes: 6 additions & 6 deletions security/advancedtls/crl_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ qsSIp8gfxSyzkJP+Ngkm2DdLjlJQCZ9R0MZP9Xj4
var revocationTests = []struct {
desc string
in x509.Certificate
revoked RevocationStatus
revoked revocationStatus
}{
{
desc: "Single revoked",
Expand Down Expand Up @@ -601,24 +601,24 @@ 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 {
t.Error("Unrevoked certificate not allowed")
}
})
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 {
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit fc8da03

Please sign in to comment.