From 77017bae59c2b89d80a2428524abd53042948e9c Mon Sep 17 00:00:00 2001 From: Irbe Krumina Date: Fri, 10 Jan 2025 07:31:28 +0000 Subject: [PATCH] cmd/containerboot: load containerboot serve config that does not contain HTTPS endpoint in tailnets with HTTPS disabled (#14538) cmd/containerboot: load containerboot serve config that does not contain HTTPS endpoint in tailnets with HTTPS disabled Fixes an issue where, if a tailnet has HTTPS disabled, no serve config set via TS_SERVE_CONFIG was loaded, even if it does not contain an HTTPS endpoint. Now for tailnets with HTTPS disabled serve config provided to containerboot is considered invalid (and therefore not loaded) only if there is an HTTPS endpoint defined in the config. Fixes tailscale/tailscale#14495 Signed-off-by: Irbe Krumina --- cmd/containerboot/serve.go | 34 ++-- cmd/containerboot/serve_test.go | 267 ++++++++++++++++++++++++++++++++ 2 files changed, 290 insertions(+), 11 deletions(-) create mode 100644 cmd/containerboot/serve_test.go diff --git a/cmd/containerboot/serve.go b/cmd/containerboot/serve.go index 14c7f00d7..1729e65b5 100644 --- a/cmd/containerboot/serve.go +++ b/cmd/containerboot/serve.go @@ -68,7 +68,6 @@ func watchServeConfigChanges(ctx context.Context, path string, cdChanged <-chan if prevServeConfig != nil && reflect.DeepEqual(sc, prevServeConfig) { continue } - validateHTTPSServe(certDomain, sc) if err := updateServeConfig(ctx, sc, certDomain, lc); err != nil { log.Fatalf("serve proxy: error updating serve config: %v", err) } @@ -88,27 +87,34 @@ func certDomainFromNetmap(nm *netmap.NetworkMap) string { return nm.DNS.CertDomains[0] } -func updateServeConfig(ctx context.Context, sc *ipn.ServeConfig, certDomain string, lc *tailscale.LocalClient) error { - // TODO(irbekrm): This means that serve config that does not expose HTTPS endpoint will not be set for a tailnet - // that does not have HTTPS enabled. We probably want to fix this. - if certDomain == kubetypes.ValueNoHTTPS { +// localClient is a subset of tailscale.LocalClient that can be mocked for testing. +type localClient interface { + SetServeConfig(context.Context, *ipn.ServeConfig) error +} + +func updateServeConfig(ctx context.Context, sc *ipn.ServeConfig, certDomain string, lc localClient) error { + if !isValidHTTPSConfig(certDomain, sc) { return nil } log.Printf("serve proxy: applying serve config") return lc.SetServeConfig(ctx, sc) } -func validateHTTPSServe(certDomain string, sc *ipn.ServeConfig) { - if certDomain != kubetypes.ValueNoHTTPS || !hasHTTPSEndpoint(sc) { - return - } - log.Printf( - `serve proxy: this node is configured as a proxy that exposes an HTTPS endpoint to tailnet, +func isValidHTTPSConfig(certDomain string, sc *ipn.ServeConfig) bool { + if certDomain == kubetypes.ValueNoHTTPS && hasHTTPSEndpoint(sc) { + log.Printf( + `serve proxy: this node is configured as a proxy that exposes an HTTPS endpoint to tailnet, (perhaps a Kubernetes operator Ingress proxy) but it is not able to issue TLS certs, so this will likely not work. To make it work, ensure that HTTPS is enabled for your tailnet, see https://tailscale.com/kb/1153/enabling-https for more details.`) + return false + } + return true } func hasHTTPSEndpoint(cfg *ipn.ServeConfig) bool { + if cfg == nil { + return false + } for _, tcpCfg := range cfg.TCP { if tcpCfg.HTTPS { return true @@ -127,6 +133,12 @@ func readServeConfig(path, certDomain string) (*ipn.ServeConfig, error) { if err != nil { return nil, err } + // Serve config can be provided by users as well as the Kubernetes Operator (for its proxies). User-provided + // config could be empty for reasons. + if len(j) == 0 { + log.Printf("serve proxy: serve config file is empty, skipping") + return nil, nil + } j = bytes.ReplaceAll(j, []byte("${TS_CERT_DOMAIN}"), []byte(certDomain)) var sc ipn.ServeConfig if err := json.Unmarshal(j, &sc); err != nil { diff --git a/cmd/containerboot/serve_test.go b/cmd/containerboot/serve_test.go new file mode 100644 index 000000000..4563c52fc --- /dev/null +++ b/cmd/containerboot/serve_test.go @@ -0,0 +1,267 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +//go:build linux + +package main + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/google/go-cmp/cmp" + "tailscale.com/client/tailscale" + "tailscale.com/ipn" + "tailscale.com/kube/kubetypes" +) + +func TestUpdateServeConfig(t *testing.T) { + tests := []struct { + name string + sc *ipn.ServeConfig + certDomain string + wantCall bool + }{ + { + name: "no_https_no_cert_domain", + sc: &ipn.ServeConfig{ + TCP: map[uint16]*ipn.TCPPortHandler{ + 80: {HTTP: true}, + }, + }, + certDomain: kubetypes.ValueNoHTTPS, // tailnet has HTTPS disabled + wantCall: true, // should set serve config as it doesn't have HTTPS endpoints + }, + { + name: "https_with_cert_domain", + sc: &ipn.ServeConfig{ + TCP: map[uint16]*ipn.TCPPortHandler{ + 443: {HTTPS: true}, + }, + Web: map[ipn.HostPort]*ipn.WebServerConfig{ + "${TS_CERT_DOMAIN}:443": { + Handlers: map[string]*ipn.HTTPHandler{ + "/": {Proxy: "http://10.0.1.100:8080"}, + }, + }, + }, + }, + certDomain: "test-node.tailnet.ts.net", + wantCall: true, + }, + { + name: "https_without_cert_domain", + sc: &ipn.ServeConfig{ + TCP: map[uint16]*ipn.TCPPortHandler{ + 443: {HTTPS: true}, + }, + }, + certDomain: kubetypes.ValueNoHTTPS, + wantCall: false, // incorrect configuration- should not set serve config + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fakeLC := &fakeLocalClient{} + err := updateServeConfig(context.Background(), tt.sc, tt.certDomain, fakeLC) + if err != nil { + t.Errorf("updateServeConfig() error = %v", err) + } + if fakeLC.setServeCalled != tt.wantCall { + t.Errorf("SetServeConfig() called = %v, want %v", fakeLC.setServeCalled, tt.wantCall) + } + }) + } +} + +func TestReadServeConfig(t *testing.T) { + tests := []struct { + name string + gotSC string + certDomain string + wantSC *ipn.ServeConfig + wantErr bool + }{ + { + name: "empty_file", + }, + { + name: "valid_config_with_cert_domain_placeholder", + gotSC: `{ + "TCP": { + "443": { + "HTTPS": true + } + }, + "Web": { + "${TS_CERT_DOMAIN}:443": { + "Handlers": { + "/api": { + "Proxy": "https://10.2.3.4/api" + }}}}}`, + certDomain: "example.com", + wantSC: &ipn.ServeConfig{ + TCP: map[uint16]*ipn.TCPPortHandler{ + 443: { + HTTPS: true, + }, + }, + Web: map[ipn.HostPort]*ipn.WebServerConfig{ + ipn.HostPort("example.com:443"): { + Handlers: map[string]*ipn.HTTPHandler{ + "/api": { + Proxy: "https://10.2.3.4/api", + }, + }, + }, + }, + }, + }, + { + name: "valid_config_for_http_proxy", + gotSC: `{ + "TCP": { + "80": { + "HTTP": true + } + }}`, + wantSC: &ipn.ServeConfig{ + TCP: map[uint16]*ipn.TCPPortHandler{ + 80: { + HTTP: true, + }, + }, + }, + }, + { + name: "config_without_cert_domain", + gotSC: `{ + "TCP": { + "443": { + "HTTPS": true + } + }, + "Web": { + "localhost:443": { + "Handlers": { + "/api": { + "Proxy": "https://10.2.3.4/api" + }}}}}`, + certDomain: "", + wantErr: false, + wantSC: &ipn.ServeConfig{ + TCP: map[uint16]*ipn.TCPPortHandler{ + 443: { + HTTPS: true, + }, + }, + Web: map[ipn.HostPort]*ipn.WebServerConfig{ + ipn.HostPort("localhost:443"): { + Handlers: map[string]*ipn.HTTPHandler{ + "/api": { + Proxy: "https://10.2.3.4/api", + }, + }, + }, + }, + }, + }, + { + name: "invalid_json", + gotSC: "invalid json", + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "serve-config.json") + if err := os.WriteFile(path, []byte(tt.gotSC), 0644); err != nil { + t.Fatal(err) + } + + got, err := readServeConfig(path, tt.certDomain) + if (err != nil) != tt.wantErr { + t.Errorf("readServeConfig() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !cmp.Equal(got, tt.wantSC) { + t.Errorf("readServeConfig() diff (-got +want):\n%s", cmp.Diff(got, tt.wantSC)) + } + }) + } +} + +type fakeLocalClient struct { + *tailscale.LocalClient + setServeCalled bool +} + +func (m *fakeLocalClient) SetServeConfig(ctx context.Context, cfg *ipn.ServeConfig) error { + m.setServeCalled = true + return nil +} + +func TestHasHTTPSEndpoint(t *testing.T) { + tests := []struct { + name string + cfg *ipn.ServeConfig + want bool + }{ + { + name: "nil_config", + cfg: nil, + want: false, + }, + { + name: "empty_config", + cfg: &ipn.ServeConfig{}, + want: false, + }, + { + name: "no_https_endpoints", + cfg: &ipn.ServeConfig{ + TCP: map[uint16]*ipn.TCPPortHandler{ + 80: { + HTTPS: false, + }, + }, + }, + want: false, + }, + { + name: "has_https_endpoint", + cfg: &ipn.ServeConfig{ + TCP: map[uint16]*ipn.TCPPortHandler{ + 443: { + HTTPS: true, + }, + }, + }, + want: true, + }, + { + name: "mixed_endpoints", + cfg: &ipn.ServeConfig{ + TCP: map[uint16]*ipn.TCPPortHandler{ + 80: {HTTPS: false}, + 443: {HTTPS: true}, + }, + }, + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := hasHTTPSEndpoint(tt.cfg) + if got != tt.want { + t.Errorf("hasHTTPSEndpoint() = %v, want %v", got, tt.want) + } + }) + } +}