From 9be47f789ceb548e4502237aaa6af1736cf96e7d Mon Sep 17 00:00:00 2001 From: "M. J. Fromberger" Date: Tue, 7 Feb 2023 14:34:04 -0800 Subject: [PATCH] ipn/ipnlocal: fix the path for writing cert files (#7203) Fixes #7202. Change-Id: I1f8e9c59d5e42e7df7a3fbbd82ae2b4293845916 Signed-off-by: M. J. Fromberger --- ipn/ipnlocal/cert.go | 36 ++++++++--- ipn/ipnlocal/cert_test.go | 76 ++++++++++++++++++++++- ipn/ipnlocal/testdata/example.com-key.pem | 28 +++++++++ ipn/ipnlocal/testdata/example.com.pem | 26 ++++++++ ipn/ipnlocal/testdata/rootCA.pem | 30 +++++++++ 5 files changed, 185 insertions(+), 11 deletions(-) create mode 100644 ipn/ipnlocal/testdata/example.com-key.pem create mode 100644 ipn/ipnlocal/testdata/example.com.pem create mode 100644 ipn/ipnlocal/testdata/rootCA.pem diff --git a/ipn/ipnlocal/cert.go b/ipn/ipnlocal/cert.go index b30878563..844b01037 100644 --- a/ipn/ipnlocal/cert.go +++ b/ipn/ipnlocal/cert.go @@ -146,46 +146,56 @@ var errCertExpired = errors.New("cert expired") func (b *LocalBackend) getCertStore(dir string) certStore { if hostinfo.GetEnvType() == hostinfo.Kubernetes && dir == "/tmp" { - return certStateStore{b.store} + return certStateStore{StateStore: b.store} } - return certFileStore(dir) + return certFileStore{dir: dir} } // certFileStore implements certStore by storing the cert & key files in the named directory. -type certFileStore string // dir +type certFileStore struct { + dir string + + // This field allows a test to override the CA root(s) for certificate + // verification. If nil the default system pool is used. + testRoots *x509.CertPool +} func (f certFileStore) Read(domain string, now time.Time) (*TLSCertKeyPair, error) { - certPEM, err := os.ReadFile(keyFile(string(f), domain)) + certPEM, err := os.ReadFile(certFile(f.dir, domain)) if err != nil { if os.IsNotExist(err) { return nil, ipn.ErrStateNotExist } return nil, err } - keyPEM, err := os.ReadFile(certFile(string(f), domain)) + keyPEM, err := os.ReadFile(keyFile(f.dir, domain)) if err != nil { if os.IsNotExist(err) { return nil, ipn.ErrStateNotExist } return nil, err } - if !validCertPEM(domain, keyPEM, certPEM, now) { + if !validCertPEM(domain, keyPEM, certPEM, f.testRoots, now) { return nil, errCertExpired } return &TLSCertKeyPair{CertPEM: certPEM, KeyPEM: keyPEM, Cached: true}, nil } func (f certFileStore) WriteCert(domain string, cert []byte) error { - return os.WriteFile(keyFile(string(f), domain), cert, 0644) + return os.WriteFile(certFile(f.dir, domain), cert, 0644) } func (f certFileStore) WriteKey(domain string, key []byte) error { - return os.WriteFile(keyFile(string(f), domain), key, 0600) + return os.WriteFile(keyFile(f.dir, domain), key, 0600) } // certStateStore implements certStore by storing the cert & key files in an ipn.StateStore. type certStateStore struct { ipn.StateStore + + // This field allows a test to override the CA root(s) for certificate + // verification. If nil the default system pool is used. + testRoots *x509.CertPool } func (s certStateStore) Read(domain string, now time.Time) (*TLSCertKeyPair, error) { @@ -197,7 +207,7 @@ func (s certStateStore) Read(domain string, now time.Time) (*TLSCertKeyPair, err if err != nil { return nil, err } - if !validCertPEM(domain, keyPEM, certPEM, now) { + if !validCertPEM(domain, keyPEM, certPEM, s.testRoots, now) { return nil, errCertExpired } return &TLSCertKeyPair{CertPEM: certPEM, KeyPEM: keyPEM, Cached: true}, nil @@ -458,7 +468,11 @@ func acmeKey(dir string) (crypto.Signer, error) { return privKey, nil } -func validCertPEM(domain string, keyPEM, certPEM []byte, now time.Time) bool { +// validCertPEM reports whether the given certificate is valid for domain at now. +// +// If roots != nil, it is used instead of the system root pool. This is meant +// to support testing, and production code should pass roots == nil. +func validCertPEM(domain string, keyPEM, certPEM []byte, roots *x509.CertPool, now time.Time) bool { if len(keyPEM) == 0 || len(certPEM) == 0 { return false } @@ -466,6 +480,7 @@ func validCertPEM(domain string, keyPEM, certPEM []byte, now time.Time) bool { if err != nil { return false } + var leaf *x509.Certificate intermediates := x509.NewCertPool() for i, certDER := range tlsCert.Certificate { @@ -485,6 +500,7 @@ func validCertPEM(domain string, keyPEM, certPEM []byte, now time.Time) bool { _, err = leaf.Verify(x509.VerifyOptions{ DNSName: domain, CurrentTime: now, + Roots: roots, Intermediates: intermediates, }) return err == nil diff --git a/ipn/ipnlocal/cert_test.go b/ipn/ipnlocal/cert_test.go index 387cbfbe4..6cc2f13c4 100644 --- a/ipn/ipnlocal/cert_test.go +++ b/ipn/ipnlocal/cert_test.go @@ -5,7 +5,15 @@ package ipnlocal -import "testing" +import ( + "crypto/x509" + "embed" + "testing" + "time" + + "github.com/google/go-cmp/cmp" + "tailscale.com/ipn/store/mem" +) func TestValidLookingCertDomain(t *testing.T) { tests := []struct { @@ -26,3 +34,69 @@ func TestValidLookingCertDomain(t *testing.T) { } } } + +//go:embed testdata/* +var certTestFS embed.FS + +func TestCertStoreRoundTrip(t *testing.T) { + const testDomain = "example.com" + + // Use a fixed verification timestamp so validity doesn't fall off when the + // cert expires. If you update the test data below, this may also need to be + // updated. + testNow := time.Date(2023, time.February, 10, 0, 0, 0, 0, time.UTC) + + // To re-generate a root certificate and domain certificate for testing, + // use: + // + // go run filippo.io/mkcert@latest example.com + // + // The content is not important except to be structurally valid so we can be + // sure the round-trip succeeds. + testRoot, err := certTestFS.ReadFile("testdata/rootCA.pem") + if err != nil { + t.Fatal(err) + } + roots := x509.NewCertPool() + if !roots.AppendCertsFromPEM(testRoot) { + t.Fatal("Unable to add test CA to the cert pool") + } + + testCert, err := certTestFS.ReadFile("testdata/example.com.pem") + if err != nil { + t.Fatal(err) + } + testKey, err := certTestFS.ReadFile("testdata/example.com-key.pem") + if err != nil { + t.Fatal(err) + } + + tests := []struct { + name string + store certStore + }{ + {"FileStore", certFileStore{dir: t.TempDir(), testRoots: roots}}, + {"StateStore", certStateStore{StateStore: new(mem.Store), testRoots: roots}}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if err := test.store.WriteCert(testDomain, testCert); err != nil { + t.Fatalf("WriteCert: unexpected error: %v", err) + } + if err := test.store.WriteKey(testDomain, testKey); err != nil { + t.Fatalf("WriteKey: unexpected error: %v", err) + } + + kp, err := test.store.Read(testDomain, testNow) + if err != nil { + t.Fatalf("Read: unexpected error: %v", err) + } + if diff := cmp.Diff(kp.CertPEM, testCert); diff != "" { + t.Errorf("Certificate (-got, +want):\n%s", diff) + } + if diff := cmp.Diff(kp.KeyPEM, testKey); diff != "" { + t.Errorf("Key (-got, +want):\n%s", diff) + } + }) + } +} diff --git a/ipn/ipnlocal/testdata/example.com-key.pem b/ipn/ipnlocal/testdata/example.com-key.pem new file mode 100644 index 000000000..06902f4c9 --- /dev/null +++ b/ipn/ipnlocal/testdata/example.com-key.pem @@ -0,0 +1,28 @@ +-----BEGIN PRIVATE KEY----- +MIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQCejQaJrntrJSgE +QtScyTU6TXOU+v1FdFjrsyHFK5mjV1C5pVQxnLn93GRshtIrGOLLrd3Wv2TVYZOX +xH7f1ZLFbneDURCXbS+7nmsg+TLHRSRKfODbE3oYZj7NSJ163CCvwSJKTdmLpXbn +ui9F04tyk0zxO4Wre4ukwf6xtse8G5zl2RJrueiVAiouTG/pJdIS08dGQa0GM1n9 +Aesa+TerlZcpRZR6X402yQqa8q/QqbIuzrlfDmgOb8sm6T8+JMtj3hEvnYdpMVOg +w/XiTlX0v/YrB9sVQ9XnqGsqwTL0OMG0choMNKipwLi2n+XPSCIiRhi666zNNivE +K1qaPS5RAgMBAAECggEAV9dAGQWPISR70CiKjLa5A60nbRHFQjackTE0c32daC6W +7dOYGsh/DxOMm8fyJqhp9nhEYJa3MbUWxU27ER3NbA6wrhM6gvqeKG8zYRhPNrGq +0o3vMdDPozb6cldZ0Fimz1jMO6h373NjtiyjxibWqkrLpRbaDtCq5EQKbMEcVa2D +Xt5hxCOaCA3OZ/mAcGUNFmDNgNsGP/r6eXdI5pbqnUNMPkv/JsHl8h2HuyKUm4hf +TRnXPAak6DkUod9QXYFKVBVPa5pjiO09e0aiMUvJ8vYd/6bNIsAKWLPa1PYuUE2l +kg8Nik+P/XLzffKsLxiFKY0nCqrorM9K5q7baofGdQKBgQDPujjebFg6OKw6MS3S +PESopvL//C/XgtgifcSSZCWzIZRVBVTbbJCGRtqFzF0XO4YRX3EOAyD/L7wYUPzO ++W3AU2W3/DVJYdcm2CASABbHNy0kk52LI0HHAssbFDgyB9XuuWP+vVZk7B5OmCAD +Bppuj6Mnu03i282nKNJzvRiVnwKBgQDDZUXv22K8y7GkKw/ZW/wQP2zBNtFc15he +1EOyUGHlXuQixnDSaqonkwec6IOlo7Sx/vwO/7+v4Jzc24Wq3DFAmMu/EYJgvI+m +m3kpB4H7Xus4JqnhxqN7GB7zOdguCWZF1HLemZNZlVrUjG5mQ9cizzvvYptnQDLq +FEJ1hddWDwKBgB+vy276Xfb7oCH8UH4KXXrQhK7RvEaGmgug3bRq/Gk3zRWvC4Ox +KtagxkK0qtqZZNkPkwJNLeJfWLTo3beAyuIUlqabHVHFT/mH7FRymQbofsVekyCf +TzBZV7wYuH3BPjv9IajBHwWkEvdwMyni/vmwhXXRF49schF2o6uuA6sHAoGBAL1J +Xnb+EKjUq0JedPwcIBOdXb3PXQKT2QgEmZAkTrHlOxx1INa2fh/YT4ext9a+wE2u +tn/RQeEfttY90z+yEASEAN0YGTWddYvxEW6t1z2stjGvQuN1ium0dEcrwkDW2jzL +knwSSqx+A3/kiw6GqeMO3wEIhYOArdIVzkwLXJABAoGAOXLGhz5u5FWjF3zAeYme +uHTU/3Z3jeI80PvShGrgAakPOBt3cIFpUaiOEslcqqgDUSGE3EnmkRqaEch+UapF +ty6Zz7cKjXhQSWOjew1uUW2ANNEpsnYbmZOOnfvosd7jfHSVbL6KIhWmIdC6h0NP +c/bJnTXEEVsWjLZTwYaq0Us= +-----END PRIVATE KEY----- \ No newline at end of file diff --git a/ipn/ipnlocal/testdata/example.com.pem b/ipn/ipnlocal/testdata/example.com.pem new file mode 100644 index 000000000..588850813 --- /dev/null +++ b/ipn/ipnlocal/testdata/example.com.pem @@ -0,0 +1,26 @@ +-----BEGIN CERTIFICATE----- +MIIEcDCCAtigAwIBAgIRAPmUKRkyFAkVVxFblB/233cwDQYJKoZIhvcNAQELBQAw +gZ8xHjAcBgNVBAoTFW1rY2VydCBkZXZlbG9wbWVudCBDQTE6MDgGA1UECwwxZnJv +bWJlcmdlckBzdGFyZHVzdC5sb2NhbCAoTWljaGFlbCBKLiBGcm9tYmVyZ2VyKTFB +MD8GA1UEAww4bWtjZXJ0IGZyb21iZXJnZXJAc3RhcmR1c3QubG9jYWwgKE1pY2hh +ZWwgSi4gRnJvbWJlcmdlcikwHhcNMjMwMjA3MjAzNDE4WhcNMjUwNTA3MTkzNDE4 +WjBlMScwJQYDVQQKEx5ta2NlcnQgZGV2ZWxvcG1lbnQgY2VydGlmaWNhdGUxOjA4 +BgNVBAsMMWZyb21iZXJnZXJAc3RhcmR1c3QubG9jYWwgKE1pY2hhZWwgSi4gRnJv +bWJlcmdlcikwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQCejQaJrntr +JSgEQtScyTU6TXOU+v1FdFjrsyHFK5mjV1C5pVQxnLn93GRshtIrGOLLrd3Wv2TV +YZOXxH7f1ZLFbneDURCXbS+7nmsg+TLHRSRKfODbE3oYZj7NSJ163CCvwSJKTdmL +pXbnui9F04tyk0zxO4Wre4ukwf6xtse8G5zl2RJrueiVAiouTG/pJdIS08dGQa0G +M1n9Aesa+TerlZcpRZR6X402yQqa8q/QqbIuzrlfDmgOb8sm6T8+JMtj3hEvnYdp +MVOgw/XiTlX0v/YrB9sVQ9XnqGsqwTL0OMG0choMNKipwLi2n+XPSCIiRhi666zN +NivEK1qaPS5RAgMBAAGjYDBeMA4GA1UdDwEB/wQEAwIFoDATBgNVHSUEDDAKBggr +BgEFBQcDATAfBgNVHSMEGDAWgBTXyq2jQVrnqQKL8fB9C4L0QJftwDAWBgNVHREE +DzANggtleGFtcGxlLmNvbTANBgkqhkiG9w0BAQsFAAOCAYEAQWzpOaBkRR4M+WqB +CsT4ARyM6WpZ+jpeSblCzPdlDRW+50G1HV7K930zayq4DwncPY/SqSn0Q31WuzZv +bTWHkWa+MLPGYANHsusOmMR8Eh16G4+5+GGf8psWa0npAYO35cuNkyyCCc1LEB4M +NrzCB2+KZ+SyOdfCCA5VzEKN3I8wvVLaYovi24Zjwv+0uETG92TlZmLQRhj8uPxN +deeLM45aBkQZSYCbGMDVDK/XYKBkNLn3kxD/eZeXxxr41v4pH44+46FkYcYJzdn8 +ccAg5LRGieqTozhLiXARNK1vTy6kR1l/Az8DIx6GN4sP2/LMFYFijiiOCDKS1wWA +xQgZeHt4GIuBym+Kd+Z5KXcP0AT+47Cby3+B10Kq8vHwjTELiF0UFeEYYMdynPAW +pbEwVLhsfMsBqFtj3dsxHr8Kz3rnarOYzkaw7EMZnLAthb2CN7y5uGV9imQC5RMI +/qZdRSuCYZ3A1E/WJkGbPY/YdPql/IE+LIAgKGFHZZNftBCo +-----END CERTIFICATE----- \ No newline at end of file diff --git a/ipn/ipnlocal/testdata/rootCA.pem b/ipn/ipnlocal/testdata/rootCA.pem new file mode 100644 index 000000000..88a16f47a --- /dev/null +++ b/ipn/ipnlocal/testdata/rootCA.pem @@ -0,0 +1,30 @@ +-----BEGIN CERTIFICATE----- +MIIFEDCCA3igAwIBAgIRANf5NdPojIfj70wMfJVYUg8wDQYJKoZIhvcNAQELBQAw +gZ8xHjAcBgNVBAoTFW1rY2VydCBkZXZlbG9wbWVudCBDQTE6MDgGA1UECwwxZnJv +bWJlcmdlckBzdGFyZHVzdC5sb2NhbCAoTWljaGFlbCBKLiBGcm9tYmVyZ2VyKTFB +MD8GA1UEAww4bWtjZXJ0IGZyb21iZXJnZXJAc3RhcmR1c3QubG9jYWwgKE1pY2hh +ZWwgSi4gRnJvbWJlcmdlcikwHhcNMjMwMjA3MjAzNDE4WhcNMzMwMjA3MjAzNDE4 +WjCBnzEeMBwGA1UEChMVbWtjZXJ0IGRldmVsb3BtZW50IENBMTowOAYDVQQLDDFm +cm9tYmVyZ2VyQHN0YXJkdXN0LmxvY2FsIChNaWNoYWVsIEouIEZyb21iZXJnZXIp +MUEwPwYDVQQDDDhta2NlcnQgZnJvbWJlcmdlckBzdGFyZHVzdC5sb2NhbCAoTWlj +aGFlbCBKLiBGcm9tYmVyZ2VyKTCCAaIwDQYJKoZIhvcNAQEBBQADggGPADCCAYoC +ggGBAL5uXNnrZ6dgjcvK0Hc7ZNUIRYEWst9qbO0P9H7le08pJ6d9T2BUWruZtVjk +Q12msv5/bVWHhVk8dZclI9FLXuMsIrocH8bsoP4wruPMyRyp6EedSKODN51fFSRv +/jHbS5vzUVAWTYy9qYmd6qL0uhsHCZCCT6gfigamHPUFKM3sHDn5ZHWvySMwcyGl +AicmPAIkBWqiCZAkB5+WM7+oyRLjmrIalfWIZYxW/rojGLwTfneHv6J5WjVQnpJB +ayWCzCzaiXukK9MeBWeTOe8UfVN0Engd74/rjLWvjbfC+uZSr6RVkZvs2jANLwPF +zgzBPHgRPfAhszU1NNAMjnNQ47+OMOTKRt7e6jYzhO5fyO1qVAAvGBqcfpj+JfDk +cccaUMhUvdiGrhGf1V1tN/PislxvALirzcFipjD01isBKwn0fxRugzvJNrjEo8RA +RvbcdeKcwex7M0o/Cd0+G2B13gZNOFvR33PmG7iTpp7IUrUKfQg28I83Sp8tMY3s +ljJSawIDAQABo0UwQzAOBgNVHQ8BAf8EBAMCAgQwEgYDVR0TAQH/BAgwBgEB/wIB +ADAdBgNVHQ4EFgQU18qto0Fa56kCi/HwfQuC9ECX7cAwDQYJKoZIhvcNAQELBQAD +ggGBAAzs96LwZVOsRSlBdQqMo8oMAvs7HgnYbXt8SqaACLX3+kJ3cV/vrCE3iJrW +ma4CiQbxS/HqsiZjota5m4lYeEevRnUDpXhp+7ugZTiz33Flm1RU99c9UYfQ+919 +ANPAKeqNpoPco/HF5Bz0ocepjcfKQrVZZNTj6noLs8o12FHBLO5976AcF9mqlNfh +8/F0gDJXq6+x7VT5y8u0rY004XKPRe3CklRt8kpeMiP6mhRyyUehOaHeIbNx8ubi +Pi44ByN/ueAnuRhF9zYtyZVZZOaSLysJge01tuPXF8rBXGruoJIv35xTTBa9BzaP +YDOGbGn1ZnajdNagHqCba8vjTLDSpqMvgRj3TFrGHdETA2LDQat38uVxX8gxm68K +va5Tyv7n+6BQ5YTpJjTPnmSJKaXZrrhdLPvG0OU2TxeEsvbcm5LFQofirOOw86Se +vzF2cQ94mmHRZiEk0Av3NO0jF93ELDrBCuiccVyEKq6TknuvPQlutCXKDOYSEb8I +MHctBg== +-----END CERTIFICATE----- \ No newline at end of file