From dcc954631be6431788a4b1377866359ae3a6ee9d Mon Sep 17 00:00:00 2001 From: Nikolay Edigaryev Date: Wed, 7 Jun 2023 15:31:55 +0400 Subject: [PATCH] TLS improvements (#90) * Fix typo when passing arguments to tls.LoadX509KeyPair() * Support TLS 1.2 too * Do not require a controller to only present a single certificate * No need to set ServerName since we use InsecureSkipVerify * Use host's root CA set by default and support normal SNI scenarios --- internal/command/context/create.go | 148 ++++++++++++++++++++-------- internal/command/controller/init.go | 2 +- internal/command/controller/run.go | 2 +- internal/config/context.go | 25 ++--- pkg/client/client.go | 72 +++++++++----- pkg/client/option.go | 14 +-- 6 files changed, 174 insertions(+), 89 deletions(-) diff --git a/internal/command/context/create.go b/internal/command/context/create.go index a1c1ec0..68bd9f6 100644 --- a/internal/command/context/create.go +++ b/internal/command/context/create.go @@ -1,17 +1,18 @@ package context import ( + "context" "crypto/sha256" "crypto/tls" "crypto/x509" "encoding/pem" - "errors" "fmt" "github.com/cirruslabs/orchard/internal/bootstraptoken" "github.com/cirruslabs/orchard/internal/config" "github.com/cirruslabs/orchard/internal/netconstants" - "github.com/cirruslabs/orchard/pkg/client" + clientpkg "github.com/cirruslabs/orchard/pkg/client" "github.com/manifoldco/promptui" + "github.com/pkg/errors" "github.com/spf13/cobra" "net/url" "strconv" @@ -24,6 +25,7 @@ var bootstrapTokenRaw string var serviceAccountName string var serviceAccountToken string var force bool +var noPKI bool func newCreateCommand() *cobra.Command { command := &cobra.Command{ @@ -43,6 +45,10 @@ func newCreateCommand() *cobra.Command { "service account token to use (alternative to --bootstrap-token)") command.PersistentFlags().BoolVar(&force, "force", false, "create the context even if a context with the same name already exists") + command.PersistentFlags().BoolVar(&noPKI, "no-pki", false, + "do not use the host's root CA set and instead validate the Controller's presented "+ + "certificate using a bootstrap token (or manually via fingerprint, "+ + "if no bootstrap token is provided)") return command } @@ -53,36 +59,29 @@ func runCreate(cmd *cobra.Command, args []string) error { return err } - // Establish trust - var trustedControllerCertificate *x509.Certificate + // If the bootstrap token is present, extract + // service account credentials from it + // and remember it for further use + var bootstrapToken *bootstraptoken.BootstrapToken if bootstrapTokenRaw != "" { - bootstrapToken, err := bootstraptoken.NewFromString(bootstrapTokenRaw) + bootstrapToken, err = bootstraptoken.NewFromString(bootstrapTokenRaw) if err != nil { return err } - serviceAccountName = bootstrapToken.ServiceAccountName() - serviceAccountToken = bootstrapToken.ServiceAccountToken() - trustedControllerCertificate = bootstrapToken.Certificate() - } else { - trustedControllerCertificate, err = probeControllerCertificate(controllerURL) - if err != nil { - return err + if serviceAccountName == "" { + serviceAccountName = bootstrapToken.ServiceAccountName() + } + if serviceAccountToken == "" { + serviceAccountToken = bootstrapToken.ServiceAccountToken() } } - client, err := client.New( - client.WithAddress(controllerURL.String()), - client.WithTrustedCertificate(trustedControllerCertificate), - client.WithCredentials(serviceAccountName, serviceAccountToken), - ) + trustedCertificate, err := tryToConnectToTheController(cmd.Context(), controllerURL, bootstrapToken) if err != nil { return err } - if err := client.Check(cmd.Context()); err != nil { - return err - } // Create and save the context configHandle, err := config.NewHandle() @@ -90,28 +89,89 @@ func runCreate(cmd *cobra.Command, args []string) error { return err } - certificatePEMBytes := pem.EncodeToMemory(&pem.Block{ - Type: "CERTIFICATE", - Bytes: trustedControllerCertificate.Raw, - }) - - return configHandle.CreateContext(contextName, config.Context{ + newContext := config.Context{ URL: controllerURL.String(), - Certificate: certificatePEMBytes, ServiceAccountName: serviceAccountName, ServiceAccountToken: serviceAccountToken, - }, force) + } + + if trustedCertificate != nil { + certificatePEMBytes := pem.EncodeToMemory(&pem.Block{ + Type: "CERTIFICATE", + Bytes: trustedCertificate.Raw, + }) + + newContext.Certificate = certificatePEMBytes + } + + return configHandle.CreateContext(contextName, newContext, force) } -func probeControllerCertificate(controllerURL *url.URL) (*x509.Certificate, error) { - // Do not use PKI - emptyPool := x509.NewCertPool() +func tryToConnectToTheController( + ctx context.Context, + controllerURL *url.URL, + bootstrapToken *bootstraptoken.BootstrapToken, +) (*x509.Certificate, error) { + if !noPKI { + if err := tryToConnectWithPKI(ctx, controllerURL); err == nil { + // Connection successful and no certificate retrieval is needed + return nil, nil + } else if errors.Is(err, clientpkg.ErrAPI) { + // Makes no sense to go any further since it's an upper layer (HTTP, not TLS) error + return nil, err + } + } - //nolint:gosec // since we're not using PKI, InsecureSkipVerify is a must here + return tryToConnectWithTrustedCertificate(ctx, controllerURL, bootstrapToken) +} + +func tryToConnectWithPKI(ctx context.Context, controllerURL *url.URL) error { + client, err := clientpkg.New( + clientpkg.WithAddress(controllerURL.String()), + clientpkg.WithCredentials(serviceAccountName, serviceAccountToken), + ) + if err != nil { + return err + } + + return client.Check(ctx) +} + +func tryToConnectWithTrustedCertificate( + ctx context.Context, + controllerURL *url.URL, + bootstrapToken *bootstraptoken.BootstrapToken, +) (*x509.Certificate, error) { + // Either (1) retrieve a trusted certificate from the bootstrap token + // or (2) retrieve it from the Controller and verify it interactively + var trustedControllerCertificate *x509.Certificate + var err error + + if bootstrapToken != nil { + trustedControllerCertificate = bootstrapToken.Certificate() + } else { + if trustedControllerCertificate, err = probeControllerCertificate(ctx, controllerURL); err != nil { + return nil, err + } + } + + // Now try again with the trusted certificate + client, err := clientpkg.New( + clientpkg.WithAddress(controllerURL.String()), + clientpkg.WithCredentials(serviceAccountName, serviceAccountToken), + clientpkg.WithTrustedCertificate(trustedControllerCertificate), + ) + if err != nil { + return nil, err + } + + return trustedControllerCertificate, client.Check(ctx) +} + +func probeControllerCertificate(ctx context.Context, controllerURL *url.URL) (*x509.Certificate, error) { + //nolint:gosec // without InsecureSkipVerify our VerifyConnection won't be called insecureTLSConfig := &tls.Config{ - MinVersion: tls.VersionTLS13, - RootCAs: emptyPool, - ServerName: netconstants.DefaultControllerServerName, + MinVersion: tls.VersionTLS12, InsecureSkipVerify: true, } @@ -123,11 +183,17 @@ func probeControllerCertificate(controllerURL *url.URL) (*x509.Certificate, erro ErrCreateFailed) } - if len(state.PeerCertificates) != 1 { - return fmt.Errorf("%w: controller presented %d certificate(s), expected only one", - ErrCreateFailed, len(state.PeerCertificates)) + if len(state.PeerCertificates) == 0 { + return fmt.Errorf("%w: controller presented no certificates, expected at least one", + ErrCreateFailed) } + // According to TLS 1.2[1] and TLS 1.3[2] specs: + // + // "The sender's certificate MUST come first in the list." + // + // [1]: https://www.rfc-editor.org/rfc/rfc5246#section-7.4.2 + // [2]: https://www.rfc-editor.org/rfc/rfc8446#section-4.4.2 controllerCert = state.PeerCertificates[0] controllerCertFingerprint := sha256.Sum256(controllerCert.Raw) formattedControllerCertFingerprint := formatFingerprint(controllerCertFingerprint[:]) @@ -177,7 +243,11 @@ func probeControllerCertificate(controllerURL *url.URL) (*x509.Certificate, erro } } - conn, err := tls.Dial("tcp", controllerURL.Host, insecureTLSConfig) + dialer := tls.Dialer{ + Config: insecureTLSConfig, + } + + conn, err := dialer.DialContext(ctx, "tcp", controllerURL.Host) if err != nil { return nil, err } diff --git a/internal/command/controller/init.go b/internal/command/controller/init.go index b3547e5..60eb89a 100644 --- a/internal/command/controller/init.go +++ b/internal/command/controller/init.go @@ -27,7 +27,7 @@ func FindControllerCertificate(dataDir *controller.DataDir) (controllerCert tls. if err = checkBothCertAndKeyAreSpecified(); err != nil { return controllerCert, err } - return tls.LoadX509KeyPair(controllerCertPath, controllerCertPath) + return tls.LoadX509KeyPair(controllerCertPath, controllerKeyPath) } else if !dataDir.ControllerCertificateExists() { // otherwise, generate a self-signed certificate if it's not already present controllerCert, err = GenerateSelfSignedControllerCertificate() diff --git a/internal/command/controller/run.go b/internal/command/controller/run.go index b03b251..7987d56 100644 --- a/internal/command/controller/run.go +++ b/internal/command/controller/run.go @@ -80,7 +80,7 @@ func runController(cmd *cobra.Command, args []string) (err error) { controller.WithDataDir(dataDir), controller.WithLogger(logger), controller.WithTLSConfig(&tls.Config{ - MinVersion: tls.VersionTLS13, + MinVersion: tls.VersionTLS12, Certificates: []tls.Certificate{ controllerCert, }, diff --git a/internal/config/context.go b/internal/config/context.go index a2bd87b..ac4e6ff 100644 --- a/internal/config/context.go +++ b/internal/config/context.go @@ -1,10 +1,9 @@ package config import ( - "crypto/tls" "crypto/x509" + "encoding/pem" "fmt" - "github.com/cirruslabs/orchard/internal/netconstants" ) type Context struct { @@ -14,20 +13,22 @@ type Context struct { ServiceAccountToken string `yaml:"serviceAccountToken,omitempty"` } -func (context *Context) TLSConfig() (*tls.Config, error) { +func (context *Context) TrustedCertificate() (*x509.Certificate, error) { if len(context.Certificate) == 0 { return nil, nil } - privatePool := x509.NewCertPool() - - if ok := privatePool.AppendCertsFromPEM(context.Certificate); !ok { - return nil, fmt.Errorf("%w: failed to load context's certificate", ErrConfigReadFailed) + block, _ := pem.Decode(context.Certificate) + if block == nil { + return nil, fmt.Errorf("%w: failed to load context's certificate: no PEM data found", + ErrConfigReadFailed) } - return &tls.Config{ - MinVersion: tls.VersionTLS13, - ServerName: netconstants.DefaultControllerServerName, - RootCAs: privatePool, - }, nil + trustedCertificate, err := x509.ParseCertificate(block.Bytes) + if err != nil { + return nil, fmt.Errorf("%w: failed to load context's certificate: %v", + ErrConfigReadFailed, err) + } + + return trustedCertificate, nil } diff --git a/pkg/client/client.go b/pkg/client/client.go index 2e0726f..60a4ab4 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -4,11 +4,13 @@ import ( "bytes" "context" "crypto/tls" + "crypto/x509" "encoding/base64" "encoding/json" "errors" "fmt" "github.com/cirruslabs/orchard/internal/config" + "github.com/cirruslabs/orchard/internal/netconstants" "github.com/cirruslabs/orchard/rpc" "golang.org/x/net/websocket" "google.golang.org/grpc/credentials" @@ -21,13 +23,15 @@ import ( var ( ErrFailed = errors.New("API client failed") + ErrAPI = errors.New("API client encountered an API error") ErrInvalidState = errors.New("invalid state") ) type Client struct { - address string - insecure bool - tlsConfig *tls.Config + address string + insecure bool + trustedCertificate *x509.Certificate + tlsConfig *tls.Config httpClient *http.Client baseURL *url.URL @@ -51,28 +55,23 @@ func New(opts ...Option) (*Client, error) { // Apply defaults if client.address == "" { - configHandle, err := config.NewHandle() - if err != nil { + if err := client.configureFromDefaultContext(); err != nil { return nil, err } - - defaultContext, err := configHandle.DefaultContext() - if err != nil { - return nil, err - } - - client.address = defaultContext.URL - client.serviceAccountName = defaultContext.ServiceAccountName - client.serviceAccountToken = defaultContext.ServiceAccountToken - - tlsConfig, err := defaultContext.TLSConfig() - if err != nil { - return nil, err - } - client.tlsConfig = tlsConfig } - // Instantiate client + if client.trustedCertificate != nil { + privatePool := x509.NewCertPool() + privatePool.AddCert(client.trustedCertificate) + + client.tlsConfig = &tls.Config{ + MinVersion: tls.VersionTLS12, + RootCAs: privatePool, + ServerName: netconstants.DefaultControllerServerName, + } + } + + // Instantiate the HTTP client client.httpClient = &http.Client{ Transport: &http.Transport{ TLSClientConfig: client.tlsConfig, @@ -125,6 +124,31 @@ func (client *Client) GPRCMetadata() metadata.MD { return metadata.New(result) } +func (client *Client) configureFromDefaultContext() error { + configHandle, err := config.NewHandle() + if err != nil { + return err + } + + defaultContext, err := configHandle.DefaultContext() + if err != nil { + return err + } + + client.address = defaultContext.URL + client.serviceAccountName = defaultContext.ServiceAccountName + client.serviceAccountToken = defaultContext.ServiceAccountToken + + if client.trustedCertificate == nil { + client.trustedCertificate, err = defaultContext.TrustedCertificate() + if err != nil { + return err + } + } + + return nil +} + func (client *Client) request( ctx context.Context, method string, @@ -174,18 +198,18 @@ func (client *Client) request( if response.StatusCode != http.StatusOK { return fmt.Errorf("%w to make a request: %d %s%s", - ErrFailed, response.StatusCode, http.StatusText(response.StatusCode), + ErrAPI, response.StatusCode, http.StatusText(response.StatusCode), detailsFromErrorResponseBody(response.Body)) } if out != nil { bodyBytes, err := io.ReadAll(response.Body) if err != nil { - return fmt.Errorf("%w to read response body: %v", ErrFailed, err) + return fmt.Errorf("%w to read response body: %v", ErrAPI, err) } if err := json.Unmarshal(bodyBytes, out); err != nil { - return fmt.Errorf("%w to unmarshal response body: %v", ErrFailed, err) + return fmt.Errorf("%w to unmarshal response body: %v", ErrAPI, err) } } diff --git a/pkg/client/option.go b/pkg/client/option.go index 78d9611..b5c291c 100644 --- a/pkg/client/option.go +++ b/pkg/client/option.go @@ -1,9 +1,7 @@ package client import ( - "crypto/tls" "crypto/x509" - "github.com/cirruslabs/orchard/internal/netconstants" ) type Option func(*Client) @@ -14,17 +12,9 @@ func WithAddress(address string) Option { } } -func WithTrustedCertificate(cert *x509.Certificate) Option { +func WithTrustedCertificate(trustedCertificate *x509.Certificate) Option { return func(client *Client) { - // Check that the API is accessible - privatePool := x509.NewCertPool() - privatePool.AddCert(cert) - - client.tlsConfig = &tls.Config{ - MinVersion: tls.VersionTLS13, - RootCAs: privatePool, - ServerName: netconstants.DefaultControllerServerName, - } + client.trustedCertificate = trustedCertificate } }