Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

advancedtls: unexport parts of API not meant to be public #7118

Merged
merged 5 commits into from
Apr 17, 2024
Merged

advancedtls: unexport parts of API not meant to be public #7118

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b048720
unexport revocatio checking fns
gtcooke94 Apr 10, 2024
3a76a85
unexport ServerNameOverride in advancedtls
gtcooke94 Apr 10, 2024
166ecf4
unexport RevocationStatus
gtcooke94 Apr 10, 2024
533631b
remove revocation status String because it is unused
gtcooke94 Apr 10, 2024
4aac1bb
add more comment to serverNameOverride
gtcooke94 Apr 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions security/advancedtls/advancedtls.go
View file
Open in desktop
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,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
matthewstevenson88 marked this conversation as resolved.
Show resolved Hide resolved
// header field) in requests.
serverNameOverride string
}

// ServerOptions contains the fields needed to be filled by the server.
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
}
}
Expand Down
2 changes: 1 addition & 1 deletion security/advancedtls/advancedtls_test.go
View file
Open in desktop
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
View file
Open in desktop
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any usages of it inside the codebase, but it looks like it can be used as an embedded part of tls.Config. In case of OSS users use it that way, would it make sense just to mark it as deprecated (in favor of new CRL solution) and remove later?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you link what you mean? I didn't see any usage in grpc-go or in google

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grpc-go/security/advancedtls/crl_test.go

Line 739 in 0baa668

cliCfg := tls.Config{

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me, I think we should unexport - we don't want anyone depending on this. If someone is using it in this way currently, they can stay on a version of the advancedTLS package that has it until they move off.
It's not really something to deprecate, rather it's an internal API that was written with a capital letter

@dfawley what do you think here?

// 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 {
erm-g marked this conversation as resolved.
Show resolved Hide resolved
// 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
View file
Open in desktop
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
Loading