From eed3e5dc611f17de9ca435523bb21ff312f21389 Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Wed, 30 Jul 2025 13:39:59 +0100 Subject: [PATCH] ipn/store/kubestore,kube: fix cert error in admin UI (#16717) Also adds a test to kube/kubeclient to defend against the error type returned by the client changing in future. Fixes tailscale/corp#30855 Change-Id: Id11d4295003e66ad5c29a687f1239333c21226a4 Signed-off-by: Tom Proctor --- ipn/store/kubestore/store_kube.go | 18 ++++++ ipn/store/kubestore/store_kube_test.go | 7 +++ kube/kubeclient/client_test.go | 76 ++++++++++++++++++++++++++ 3 files changed, 101 insertions(+) diff --git a/ipn/store/kubestore/store_kube.go b/ipn/store/kubestore/store_kube.go index a9ad514e7..5b25471c7 100644 --- a/ipn/store/kubestore/store_kube.go +++ b/ipn/store/kubestore/store_kube.go @@ -9,6 +9,7 @@ import ( "fmt" "log" "net" + "net/http" "os" "strings" "time" @@ -203,6 +204,23 @@ func (s *Store) ReadTLSCertAndKey(domain string) (cert, key []byte, err error) { // that wraps ipn.ErrStateNotExist here. return nil, nil, ipn.ErrStateNotExist } + st, ok := err.(*kubeapi.Status) + if ok && st.Code == http.StatusForbidden && (s.certShareMode == "ro" || s.certShareMode == "rw") { + // In cert share mode, we read from a dedicated Secret per domain. + // To get here, we already had a cache miss from our in-memory + // store. For write replicas, that means it wasn't available on + // start and it wasn't written since. For read replicas, that means + // it wasn't available on start and it hasn't been reloaded in the + // background. So getting a "forbidden" error is an expected + // "not found" case where we've been asked for a cert we don't + // expect to issue, and so the forbidden error reflects that the + // operator didn't assign permission for a Secret for that domain. + // + // This code path gets triggered by the admin UI's machine page, + // which queries for the node's own TLS cert existing via the + // "tls-cert-status" c2n API. + return nil, nil, ipn.ErrStateNotExist + } return nil, nil, fmt.Errorf("getting TLS Secret %q: %w", domain, err) } cert = secret.Data[keyTLSCert] diff --git a/ipn/store/kubestore/store_kube_test.go b/ipn/store/kubestore/store_kube_test.go index 9a49f3028..8c8e5e870 100644 --- a/ipn/store/kubestore/store_kube_test.go +++ b/ipn/store/kubestore/store_kube_test.go @@ -426,6 +426,13 @@ func TestReadTLSCertAndKey(t *testing.T) { secretGetErr: &kubeapi.Status{Code: 404}, wantErr: ipn.ErrStateNotExist, }, + { + name: "cert_share_ro_mode_forbidden", + certShareMode: "ro", + domain: testDomain, + secretGetErr: &kubeapi.Status{Code: 403}, + wantErr: ipn.ErrStateNotExist, + }, { name: "cert_share_ro_mode_empty_cert_in_secret", certShareMode: "ro", diff --git a/kube/kubeclient/client_test.go b/kube/kubeclient/client_test.go index 31878befe..8599e7e3c 100644 --- a/kube/kubeclient/client_test.go +++ b/kube/kubeclient/client_test.go @@ -7,6 +7,9 @@ import ( "context" "encoding/json" "net/http" + "net/http/httptest" + "os" + "path/filepath" "testing" "github.com/google/go-cmp/cmp" @@ -104,6 +107,48 @@ func Test_client_Event(t *testing.T) { } } +// TestReturnsKubeStatusError ensures HTTP error codes from the Kubernetes API +// server can always be extracted by casting the error to the *kubeapi.Status +// type, as lots of calling code relies on this cast succeeding. Note that +// transport errors are not expected or required to be of type *kubeapi.Status. +func TestReturnsKubeStatusError(t *testing.T) { + cl := clientForKubeHandler(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + _ = json.NewEncoder(w).Encode(kubeapi.Status{Code: http.StatusForbidden, Message: "test error"}) + })) + + _, err := cl.GetSecret(t.Context(), "test-secret") + if err == nil { + t.Fatal("expected error, got nil") + } + if st, ok := err.(*kubeapi.Status); !ok || st.Code != http.StatusForbidden { + t.Fatalf("expected kubeapi.Status with code %d, got %T: %v", http.StatusForbidden, err, err) + } +} + +// clientForKubeHandler creates a client using the externally accessible package +// API to ensure it's testing behaviour as close to prod as possible. The passed +// in handler mocks the Kubernetes API server's responses to any HTTP requests +// made by the client. +func clientForKubeHandler(t *testing.T, handler http.Handler) Client { + t.Helper() + tmpDir := t.TempDir() + rootPathForTests = tmpDir + saDir := filepath.Join(tmpDir, "var", "run", "secrets", "kubernetes.io", "serviceaccount") + _ = os.MkdirAll(saDir, 0755) + _ = os.WriteFile(filepath.Join(saDir, "token"), []byte("test-token"), 0600) + _ = os.WriteFile(filepath.Join(saDir, "namespace"), []byte("test-namespace"), 0600) + _ = os.WriteFile(filepath.Join(saDir, "ca.crt"), []byte(ca), 0644) + cl, err := New("test-client") + if err != nil { + t.Fatalf("New() error = %v", err) + } + srv := httptest.NewServer(handler) + t.Cleanup(srv.Close) + cl.SetURL(srv.URL) + return cl +} + // args is a set of values for testing a single call to client.kubeAPIRequest. type args struct { // wantsMethod is the expected value of 'method' arg. @@ -149,3 +194,34 @@ func fakeKubeAPIRequest(t *testing.T, argSets []args) kubeAPIRequestFunc { } return f } + +const ca = `-----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-----`