From bd08f789eba633b7a3afc707dc06b228bef4d4fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Edu=20G=C3=B3mez=20Escandell?= Date: Tue, 10 Oct 2023 12:22:19 +0200 Subject: [PATCH] Use secure credentials for Agent-Landscape connection --- .../proservices/landscape/landscape.go | 63 +++++++++++++++- .../proservices/landscape/landscape_test.go | 74 +++++++++++++++---- .../certificates/generate-certificate.sh | 11 +++ 3 files changed, 132 insertions(+), 16 deletions(-) create mode 100644 windows-agent/internal/proservices/landscape/testdata/TestConnect/certificates/generate-certificate.sh diff --git a/windows-agent/internal/proservices/landscape/landscape.go b/windows-agent/internal/proservices/landscape/landscape.go index 9b0642d1a..21bbfb822 100644 --- a/windows-agent/internal/proservices/landscape/landscape.go +++ b/windows-agent/internal/proservices/landscape/landscape.go @@ -3,11 +3,14 @@ package landscape import ( "context" + "crypto/tls" + "crypto/x509" "errors" "fmt" "io/fs" "os" "path/filepath" + "strings" "sync" "sync/atomic" "time" @@ -19,7 +22,9 @@ import ( "github.com/ubuntu/decorate" "google.golang.org/grpc" "google.golang.org/grpc/connectivity" + "google.golang.org/grpc/credentials" "google.golang.org/grpc/credentials/insecure" + "gopkg.in/ini.v1" ) // Client is a client to the landscape service, served remotely. @@ -58,6 +63,7 @@ const cacheFileBase = "landscape.conf" // Config is a configuration provider for ProToken and the Landscape URL. type Config interface { + LandscapeClientConfig(context.Context) (string, error) LandscapeAgentURL(context.Context) (string, error) Subscription(context.Context) (string, config.SubscriptionSource, error) } @@ -198,7 +204,8 @@ func (c *Client) connect(ctx context.Context, address string) (conn *connection, dialCtx, cancel := context.WithTimeout(ctx, 10*time.Second) defer cancel() - grpcConn, err := grpc.DialContext(dialCtx, address, grpc.WithTransportCredentials(insecure.NewCredentials())) + creds := c.transportCredentials(ctx) + grpcConn, err := grpc.DialContext(dialCtx, address, grpc.WithTransportCredentials(creds)) if err != nil { return nil, err } @@ -377,3 +384,57 @@ func (c *Client) getUID() string { func (c *Client) setUID(s string) { c.uid.Store(s) } + +// transportCredentials reads the Landscape client config to check if a SSL public key is specified. +// If this credential is not specified, or it cannot be used for any reason, an insecure credential +// is returned. +func (c *Client) transportCredentials(ctx context.Context) credentials.TransportCredentials { + conf, err := c.conf.LandscapeClientConfig(ctx) + if err != nil { + log.Warningf(ctx, "Landscape credentials: could not obtain Landscape config: %v", err) + return insecure.NewCredentials() + } + + if conf == "" { + // No Landscape config: default to insecure + return insecure.NewCredentials() + } + + ini, err := ini.Load(strings.NewReader(conf)) + if err != nil { + log.Errorf(ctx, "Landscape credentials: could not read Landscape config file: %v", err) + return insecure.NewCredentials() + } + + const section = "client" + const key = "ssl_public_key" + + sec, err := ini.GetSection(section) + if err != nil { + // No SSL public key provided: default to insecure + return insecure.NewCredentials() + } + + k, err := sec.GetKey(key) + if err != nil { + // No SSL public key provided: default to insecure + return insecure.NewCredentials() + } + + cert, err := os.ReadFile(k.String()) + if err != nil { + log.Errorf(ctx, "Landscape credentials: could not load SSL public key file: %v", err) + return insecure.NewCredentials() + } + + certPool := x509.NewCertPool() + if ok := certPool.AppendCertsFromPEM(cert); !ok { + log.Errorf(ctx, "Landscape credentials: failed to add server CA's certificate: %v", err) + return insecure.NewCredentials() + } + + return credentials.NewTLS(&tls.Config{ + RootCAs: certPool, + MinVersion: tls.VersionTLS12, + }) +} diff --git a/windows-agent/internal/proservices/landscape/landscape_test.go b/windows-agent/internal/proservices/landscape/landscape_test.go index 664ad7fe4..057d02e19 100644 --- a/windows-agent/internal/proservices/landscape/landscape_test.go +++ b/windows-agent/internal/proservices/landscape/landscape_test.go @@ -2,6 +2,7 @@ package landscape_test import ( "context" + "crypto/tls" "errors" "fmt" "net" @@ -14,6 +15,7 @@ import ( "time" landscapeapi "github.com/canonical/landscape-hostagent-api" + "github.com/canonical/ubuntu-pro-for-windows/common/golden" "github.com/canonical/ubuntu-pro-for-windows/common/wsltestutils" "github.com/canonical/ubuntu-pro-for-windows/mocks/landscape/landscapemockservice" "github.com/canonical/ubuntu-pro-for-windows/windows-agent/internal/config" @@ -27,6 +29,7 @@ import ( wsl "github.com/ubuntu/gowsl" wslmock "github.com/ubuntu/gowsl/mock" "google.golang.org/grpc" + "google.golang.org/grpc/credentials" ) func TestMain(m *testing.M) { @@ -98,6 +101,7 @@ func TestConnect(t *testing.T) { serverNotAvailable bool landscapeURLErr bool tokenErr bool + requireCertificate bool breakUIDFile bool uid string @@ -105,13 +109,15 @@ func TestConnect(t *testing.T) { wantErr bool wantDistroSkipped bool }{ - "Success in first contact": {}, - "Success in non-first contact": {uid: "123"}, + "Success in first contact": {}, + "Success in non-first contact": {uid: "123"}, + "Success with non-empty config": {}, + "Success with an SSL certificate": {requireCertificate: true}, "Error when the context is cancelled before Connected": {precancelContext: true, wantErr: true}, "Error when the landscape URL cannot be retrieved": {landscapeURLErr: true, wantErr: true}, "Error when the server cannot be reached": {serverNotAvailable: true, wantErr: true}, - "Error when the first-contact SendUpdatedInfo fails ": {tokenErr: true, wantErr: true}, + "Error when the first-contact SendUpdatedInfo fails": {tokenErr: true, wantErr: true}, } for name, tc := range testCases { @@ -123,7 +129,7 @@ func TestConnect(t *testing.T) { ctx = wsl.WithMock(ctx, wslmock.New()) } - lis, server, mockService := setUpLandscapeMock(t, ctx, "localhost:") + lis, server, mockService := setUpLandscapeMock(t, ctx, "localhost:", tc.requireCertificate) defer lis.Close() conf := &mockConfig{ @@ -137,6 +143,15 @@ func TestConnect(t *testing.T) { landscapeURLErr: tc.landscapeURLErr, } + out, err := os.ReadFile(filepath.Join(golden.TestFixturePath(t), "landscape.conf")) + if errors.Is(err, os.ErrNotExist) { + // This fixture is not compulsory + out = []byte{} + err = nil + } + require.NoError(t, err, "Setup: could not load landscape config") + conf.landscapeClientConfig = string(out) + if !tc.serverNotAvailable { //nolint:errcheck // We don't care about these errors go server.Serve(lis) @@ -188,7 +203,7 @@ func TestConnect(t *testing.T) { confFile := filepath.Join(dir, landscape.CacheFileBase) require.FileExists(t, confFile, "Landscape config file should be created after disconnecting") - out, err := os.ReadFile(confFile) + out, err = os.ReadFile(confFile) require.NoError(t, err, "Could not read landscape config file") wantUID := tc.uid @@ -242,7 +257,7 @@ func TestSendUpdatedInfo(t *testing.T) { ctx = wsl.WithMock(ctx, mock) } - lis, server, mockService := setUpLandscapeMock(t, ctx, "localhost:") + lis, server, mockService := setUpLandscapeMock(t, ctx, "localhost:", false) conf := &mockConfig{ proToken: "TOKEN", @@ -408,7 +423,7 @@ func TestAutoReconnection(t *testing.T) { ctx = wsl.WithMock(ctx, mock) } - lis, server, mockService := setUpLandscapeMock(t, ctx, "localhost:") + lis, server, mockService := setUpLandscapeMock(t, ctx, "localhost:", false) defer lis.Close() defer server.Stop() @@ -460,7 +475,7 @@ func TestAutoReconnection(t *testing.T) { }, 5*time.Second, 100*time.Millisecond, "Client should have disconnected after the server is stopped") // Restart server at the same address - lis, server, _ = setUpLandscapeMock(t, ctx, lis.Addr().String()) + lis, server, _ = setUpLandscapeMock(t, ctx, lis.Addr().String(), false) defer lis.Close() //nolint:errcheck // We don't care @@ -541,7 +556,7 @@ func TestReceiveCommands(t *testing.T) { t.Skip("This test can only run with the mock") } - lis, server, service := setUpLandscapeMock(t, ctx, "localhost:") + lis, server, service := setUpLandscapeMock(t, ctx, "localhost:", false) defer lis.Close() //nolint:errcheck // We don't care about these errors @@ -781,14 +796,31 @@ func isAppxInstalled(t *testing.T, appxPackage string) bool { } //nolint:revive // Context goes after testing.T -func setUpLandscapeMock(t *testing.T, ctx context.Context, addr string) (lis net.Listener, server *grpc.Server, service *landscapemockservice.Service) { +func setUpLandscapeMock(t *testing.T, ctx context.Context, addr string, requireCertificate bool) (lis net.Listener, server *grpc.Server, service *landscapemockservice.Service) { t.Helper() var cfg net.ListenConfig lis, err := cfg.Listen(ctx, "tcp", addr) require.NoError(t, err, "Setup: can't listen") - server = grpc.NewServer() + var opts []grpc.ServerOption + if requireCertificate { + certPath := filepath.Join(golden.TestFamilyPath(t), "certificates/cert.pem") + keyPath := filepath.Join(golden.TestFamilyPath(t), "certificates/key.pem") + + serverCert, err := tls.LoadX509KeyPair(certPath, keyPath) + require.NoError(t, err, "Setup: could not load Landscape mock server credentials") + + config := &tls.Config{ + Certificates: []tls.Certificate{serverCert}, + ClientAuth: tls.NoClientCert, + MinVersion: tls.VersionTLS12, + } + + opts = append(opts, grpc.Creds(credentials.NewTLS(config))) + } + + server = grpc.NewServer(opts...) service = landscapemockservice.New() landscapeapi.RegisterLandscapeHostAgentServer(server, service) @@ -796,15 +828,27 @@ func setUpLandscapeMock(t *testing.T, ctx context.Context, addr string) (lis net } type mockConfig struct { - proToken string - landscapeAgentURL string + proToken string + landscapeAgentURL string + landscapeClientConfig string - proTokenErr bool - landscapeURLErr bool + proTokenErr bool + landscapeURLErr bool + landscapeConfigErr bool mu sync.Mutex } +func (m *mockConfig) LandscapeClientConfig(ctx context.Context) (string, error) { + m.mu.Lock() + defer m.mu.Unlock() + + if m.landscapeConfigErr { + return "", errors.New("Mock error") + } + return m.landscapeClientConfig, nil +} + func (m *mockConfig) ProvisioningTasks(ctx context.Context, distroName string) ([]task.Task, error) { return nil, nil } diff --git a/windows-agent/internal/proservices/landscape/testdata/TestConnect/certificates/generate-certificate.sh b/windows-agent/internal/proservices/landscape/testdata/TestConnect/certificates/generate-certificate.sh new file mode 100644 index 000000000..ecf27cce6 --- /dev/null +++ b/windows-agent/internal/proservices/landscape/testdata/TestConnect/certificates/generate-certificate.sh @@ -0,0 +1,11 @@ +#!/bin/bash + +openssl req \ + -x509 \ + -newkey rsa:4096 \ + -keyout key.pem \ + -out cert.pem \ + -sha256 \ + -nodes \ + -addext 'subjectAltName = IP:127.0.0.1' \ + -subj "/C=US/O=Canonical/CN=CanonicalGroupLimited"