From b82b5242600c476281304782ffe516f7a5a90b74 Mon Sep 17 00:00:00 2001 From: JulienBalestra Date: Mon, 27 Aug 2018 19:16:32 +0200 Subject: [PATCH 1/3] stream: can use user certificates Signed-off-by: JulienBalestra --- pkg/config/config.go | 4 ++++ pkg/server/streaming.go | 26 +++++++++++++++++++------- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index 6919ce6b8..a5f27f48e 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -114,6 +114,10 @@ type PluginConfig struct { SystemdCgroup bool `toml:"systemd_cgroup" json:"systemdCgroup"` // EnableTLSStreaming indicates to enable the TLS streaming support. EnableTLSStreaming bool `toml:"enable_tls_streaming" json:"enableTLSStreaming"` + // TLSCertFileStreaming is the path to a certificate file + TLSCertFileStreaming string `toml:"tls_cert_file_streaming" json:"tlsCertFileStreaming"` + // TLSKeyFileStreaming is the path to a private key file + TLSKeyFileStreaming string `toml:"tls_key_file_streaming" json:"tlsKeyFileStreaming"` // MaxContainerLogLineSize is the maximum log line size in bytes for a container. // Log line longer than the limit will be split into multiple lines. Non-positive // value means no limit. diff --git a/pkg/server/streaming.go b/pkg/server/streaming.go index 4e103248e..255b7c9a6 100644 --- a/pkg/server/streaming.go +++ b/pkg/server/streaming.go @@ -44,18 +44,30 @@ func newStreamServer(c *criService, addr, port string) (streaming.Server, error) } config := streaming.DefaultConfig config.Addr = net.JoinHostPort(addr, port) - runtime := newStreamRuntime(c) - if c.config.EnableTLSStreaming { - tlsCert, err := newTLSCert() + run := newStreamRuntime(c) + if !c.config.EnableTLSStreaming { + return streaming.NewServer(config, run) + } + if c.config.TLSCertFileStreaming != "" && c.config.TLSKeyFileStreaming != "" { + tlsCert, err := tls.LoadX509KeyPair(c.config.TLSCertFileStreaming, c.config.TLSKeyFileStreaming) if err != nil { - return nil, errors.Wrap(err, "failed to generate tls certificate for stream server") + return nil, errors.Wrap(err, "failed to load x509 key pair for stream server") } config.TLSConfig = &tls.Config{ - Certificates: []tls.Certificate{tlsCert}, - InsecureSkipVerify: true, + Certificates: []tls.Certificate{tlsCert}, } + return streaming.NewServer(config, run) } - return streaming.NewServer(config, runtime) + // generating self-sign certs + tlsCert, err := newTLSCert() + if err != nil { + return nil, errors.Wrap(err, "failed to generate tls certificate for stream server") + } + config.TLSConfig = &tls.Config{ + Certificates: []tls.Certificate{tlsCert}, + InsecureSkipVerify: true, + } + return streaming.NewServer(config, run) } type streamRuntime struct { From 859003a940b8c48569f5064efcf4c14d8444ce1a Mon Sep 17 00:00:00 2001 From: JulienBalestra Date: Tue, 28 Aug 2018 11:02:56 +0200 Subject: [PATCH 2/3] stream: struct for x509 key pair, update the docs, error management Signed-off-by: JulienBalestra --- docs/config.md | 10 +++++++++- pkg/config/config.go | 26 ++++++++++++++++++-------- pkg/server/streaming.go | 9 +++++++-- 3 files changed, 34 insertions(+), 11 deletions(-) diff --git a/docs/config.md b/docs/config.md index 8fa77b55b..61b49f1bb 100644 --- a/docs/config.md +++ b/docs/config.md @@ -24,8 +24,16 @@ The explanation and default value of each configuration item are as follows: # systemd_cgroup enables systemd cgroup support. systemd_cgroup = false - # enable_tls_streaming enables the TLS streaming support. + # enable_tls_streaming enables the TLS streaming support. + # It generates a self-sign certificate unless the following x509_key_pair_streaming are both set. enable_tls_streaming = false + + # "plugins.cri.x509_key_pair_streaming" constains a x509 valid key pair to stream with tls. + [plugins.cri.x509_key_pair_streaming] + # tls_cert_file is the filepath to the certificate paired with the "tls_key_file" + tls_cert_file = "" + # tls_key_file is the filepath to the private key paired with the "tls_cert_file" + tls_key_file = "" # max_container_log_line_size is the maximum log line size in bytes for a container. # Log line longer than the limit will be split into multiple lines. -1 means no diff --git a/pkg/config/config.go b/pkg/config/config.go index a5f27f48e..49288b59a 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -114,16 +114,22 @@ type PluginConfig struct { SystemdCgroup bool `toml:"systemd_cgroup" json:"systemdCgroup"` // EnableTLSStreaming indicates to enable the TLS streaming support. EnableTLSStreaming bool `toml:"enable_tls_streaming" json:"enableTLSStreaming"` - // TLSCertFileStreaming is the path to a certificate file - TLSCertFileStreaming string `toml:"tls_cert_file_streaming" json:"tlsCertFileStreaming"` - // TLSKeyFileStreaming is the path to a private key file - TLSKeyFileStreaming string `toml:"tls_key_file_streaming" json:"tlsKeyFileStreaming"` + // X509KeyPairStreaming is a x509 key pair used for TLS streaming + X509KeyPairStreaming `toml:"x509_key_pair_streaming" json:"x509KeyPairStreaming"` // MaxContainerLogLineSize is the maximum log line size in bytes for a container. // Log line longer than the limit will be split into multiple lines. Non-positive // value means no limit. MaxContainerLogLineSize int `toml:"max_container_log_line_size" json:"maxContainerLogSize"` } +// X509KeyPairStreaming contains the x509 configuration for streaming +type X509KeyPairStreaming struct { + // TLSCertFile is the path to a certificate file + TLSCertFile string `toml:"tls_cert_file" json:"tlsCertFile"` + // TLSKeyFile is the path to a private key file + TLSKeyFile string `toml:"tls_key_file" json:"tlsKeyFile"` +} + // Config contains all configurations for cri server. type Config struct { // PluginConfig is the config for CRI plugin. @@ -156,10 +162,14 @@ func DefaultConfig() PluginConfig { }, NoPivot: false, }, - StreamServerAddress: "127.0.0.1", - StreamServerPort: "0", - EnableSelinux: false, - EnableTLSStreaming: false, + StreamServerAddress: "127.0.0.1", + StreamServerPort: "0", + EnableSelinux: false, + EnableTLSStreaming: false, + X509KeyPairStreaming: X509KeyPairStreaming{ + TLSKeyFile: "", + TLSCertFile: "", + }, SandboxImage: "k8s.gcr.io/pause:3.1", StatsCollectPeriod: 10, SystemdCgroup: false, diff --git a/pkg/server/streaming.go b/pkg/server/streaming.go index 255b7c9a6..8d732eb60 100644 --- a/pkg/server/streaming.go +++ b/pkg/server/streaming.go @@ -46,10 +46,13 @@ func newStreamServer(c *criService, addr, port string) (streaming.Server, error) config.Addr = net.JoinHostPort(addr, port) run := newStreamRuntime(c) if !c.config.EnableTLSStreaming { + if c.config.X509KeyPairStreaming.TLSCertFile != "" || c.config.X509KeyPairStreaming.TLSKeyFile != "" { + return nil, errors.Errorf("X509KeyPairStreaming.TLSCertFile and/or X509KeyPairStreaming.TLSKeyFile are set but EnableTLSStreaming is not set") + } return streaming.NewServer(config, run) } - if c.config.TLSCertFileStreaming != "" && c.config.TLSKeyFileStreaming != "" { - tlsCert, err := tls.LoadX509KeyPair(c.config.TLSCertFileStreaming, c.config.TLSKeyFileStreaming) + if c.config.X509KeyPairStreaming.TLSCertFile != "" && c.config.X509KeyPairStreaming.TLSKeyFile != "" { + tlsCert, err := tls.LoadX509KeyPair(c.config.X509KeyPairStreaming.TLSCertFile, c.config.X509KeyPairStreaming.TLSKeyFile) if err != nil { return nil, errors.Wrap(err, "failed to load x509 key pair for stream server") } @@ -57,6 +60,8 @@ func newStreamServer(c *criService, addr, port string) (streaming.Server, error) Certificates: []tls.Certificate{tlsCert}, } return streaming.NewServer(config, run) + } else if c.config.X509KeyPairStreaming.TLSCertFile != "" || c.config.X509KeyPairStreaming.TLSKeyFile != "" { + return nil, errors.Errorf("must set both X509KeyPairStreaming.TLSCertFile and X509KeyPairStreaming.TLSKeyFile") } // generating self-sign certs tlsCert, err := newTLSCert() From dffd0dfa0ed2a1fe941ed57f53e0bcea7eddbb86 Mon Sep 17 00:00:00 2001 From: JulienBalestra Date: Thu, 30 Aug 2018 14:50:25 +0200 Subject: [PATCH 3/3] streaming: tls conf validation to func with tests Signed-off-by: JulienBalestra --- docs/config.md | 2 +- pkg/server/streaming.go | 67 ++++++++++----- pkg/server/streaming_test.go | 153 +++++++++++++++++++++++++++++++++++ 3 files changed, 203 insertions(+), 19 deletions(-) create mode 100644 pkg/server/streaming_test.go diff --git a/docs/config.md b/docs/config.md index 61b49f1bb..0b8798bc9 100644 --- a/docs/config.md +++ b/docs/config.md @@ -28,7 +28,7 @@ The explanation and default value of each configuration item are as follows: # It generates a self-sign certificate unless the following x509_key_pair_streaming are both set. enable_tls_streaming = false - # "plugins.cri.x509_key_pair_streaming" constains a x509 valid key pair to stream with tls. + # "plugins.cri.x509_key_pair_streaming" contains a x509 valid key pair to stream with tls. [plugins.cri.x509_key_pair_streaming] # tls_cert_file is the filepath to the certificate paired with the "tls_key_file" tls_cert_file = "" diff --git a/pkg/server/streaming.go b/pkg/server/streaming.go index 8d732eb60..0ed948617 100644 --- a/pkg/server/streaming.go +++ b/pkg/server/streaming.go @@ -34,6 +34,36 @@ import ( ctrdutil "github.com/containerd/cri/pkg/containerd/util" ) +type streamListenerMode int + +const ( + x509KeyPairTLS streamListenerMode = iota + selfSignTLS + withoutTLS +) + +func getStreamListenerMode(c *criService) (streamListenerMode, error) { + if c.config.EnableTLSStreaming { + if c.config.X509KeyPairStreaming.TLSCertFile != "" && c.config.X509KeyPairStreaming.TLSKeyFile != "" { + return x509KeyPairTLS, nil + } + if c.config.X509KeyPairStreaming.TLSCertFile != "" && c.config.X509KeyPairStreaming.TLSKeyFile == "" { + return -1, errors.New("must set X509KeyPairStreaming.TLSKeyFile") + } + if c.config.X509KeyPairStreaming.TLSCertFile == "" && c.config.X509KeyPairStreaming.TLSKeyFile != "" { + return -1, errors.New("must set X509KeyPairStreaming.TLSCertFile") + } + return selfSignTLS, nil + } + if c.config.X509KeyPairStreaming.TLSCertFile != "" { + return -1, errors.New("X509KeyPairStreaming.TLSCertFile is set but EnableTLSStreaming is not set") + } + if c.config.X509KeyPairStreaming.TLSKeyFile != "" { + return -1, errors.New("X509KeyPairStreaming.TLSKeyFile is set but EnableTLSStreaming is not set") + } + return withoutTLS, nil +} + func newStreamServer(c *criService, addr, port string) (streaming.Server, error) { if addr == "" { a, err := k8snet.ChooseBindAddress(nil) @@ -45,13 +75,12 @@ func newStreamServer(c *criService, addr, port string) (streaming.Server, error) config := streaming.DefaultConfig config.Addr = net.JoinHostPort(addr, port) run := newStreamRuntime(c) - if !c.config.EnableTLSStreaming { - if c.config.X509KeyPairStreaming.TLSCertFile != "" || c.config.X509KeyPairStreaming.TLSKeyFile != "" { - return nil, errors.Errorf("X509KeyPairStreaming.TLSCertFile and/or X509KeyPairStreaming.TLSKeyFile are set but EnableTLSStreaming is not set") - } - return streaming.NewServer(config, run) + tlsMode, err := getStreamListenerMode(c) + if err != nil { + return nil, errors.Wrapf(err, "invalid stream server configuration") } - if c.config.X509KeyPairStreaming.TLSCertFile != "" && c.config.X509KeyPairStreaming.TLSKeyFile != "" { + switch tlsMode { + case x509KeyPairTLS: tlsCert, err := tls.LoadX509KeyPair(c.config.X509KeyPairStreaming.TLSCertFile, c.config.X509KeyPairStreaming.TLSKeyFile) if err != nil { return nil, errors.Wrap(err, "failed to load x509 key pair for stream server") @@ -60,19 +89,21 @@ func newStreamServer(c *criService, addr, port string) (streaming.Server, error) Certificates: []tls.Certificate{tlsCert}, } return streaming.NewServer(config, run) - } else if c.config.X509KeyPairStreaming.TLSCertFile != "" || c.config.X509KeyPairStreaming.TLSKeyFile != "" { - return nil, errors.Errorf("must set both X509KeyPairStreaming.TLSCertFile and X509KeyPairStreaming.TLSKeyFile") + case selfSignTLS: + tlsCert, err := newTLSCert() + if err != nil { + return nil, errors.Wrap(err, "failed to generate tls certificate for stream server") + } + config.TLSConfig = &tls.Config{ + Certificates: []tls.Certificate{tlsCert}, + InsecureSkipVerify: true, + } + return streaming.NewServer(config, run) + case withoutTLS: + return streaming.NewServer(config, run) + default: + return nil, errors.New("invalid configuration for the stream listener") } - // generating self-sign certs - tlsCert, err := newTLSCert() - if err != nil { - return nil, errors.Wrap(err, "failed to generate tls certificate for stream server") - } - config.TLSConfig = &tls.Config{ - Certificates: []tls.Certificate{tlsCert}, - InsecureSkipVerify: true, - } - return streaming.NewServer(config, run) } type streamRuntime struct { diff --git a/pkg/server/streaming_test.go b/pkg/server/streaming_test.go new file mode 100644 index 000000000..a88903d3b --- /dev/null +++ b/pkg/server/streaming_test.go @@ -0,0 +1,153 @@ +/* +Copyright 2017 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package server + +import ( + "testing" + + "github.com/containerd/cri/pkg/config" + "github.com/stretchr/testify/assert" +) + +func TestValidateStreamServer(t *testing.T) { + for desc, test := range map[string]struct { + *criService + tlsMode streamListenerMode + expectErr bool + }{ + "should pass with default withoutTLS": { + criService: &criService{ + config: config.Config{ + PluginConfig: config.DefaultConfig(), + }, + }, + tlsMode: withoutTLS, + expectErr: false, + }, + "should pass with x509KeyPairTLS": { + criService: &criService{ + config: config.Config{ + PluginConfig: config.PluginConfig{ + EnableTLSStreaming: true, + X509KeyPairStreaming: config.X509KeyPairStreaming{ + TLSKeyFile: "non-empty", + TLSCertFile: "non-empty", + }, + }, + }, + }, + tlsMode: x509KeyPairTLS, + expectErr: false, + }, + "should pass with selfSign": { + criService: &criService{ + config: config.Config{ + PluginConfig: config.PluginConfig{ + EnableTLSStreaming: true, + }, + }, + }, + tlsMode: selfSignTLS, + expectErr: false, + }, + "should return error with X509 keypair but not EnableTLSStreaming": { + criService: &criService{ + config: config.Config{ + PluginConfig: config.PluginConfig{ + EnableTLSStreaming: false, + X509KeyPairStreaming: config.X509KeyPairStreaming{ + TLSKeyFile: "non-empty", + TLSCertFile: "non-empty", + }, + }, + }, + }, + tlsMode: -1, + expectErr: true, + }, + "should return error with X509 TLSCertFile empty": { + criService: &criService{ + config: config.Config{ + PluginConfig: config.PluginConfig{ + EnableTLSStreaming: true, + X509KeyPairStreaming: config.X509KeyPairStreaming{ + TLSKeyFile: "non-empty", + TLSCertFile: "", + }, + }, + }, + }, + tlsMode: -1, + expectErr: true, + }, + "should return error with X509 TLSKeyFile empty": { + criService: &criService{ + config: config.Config{ + PluginConfig: config.PluginConfig{ + EnableTLSStreaming: true, + X509KeyPairStreaming: config.X509KeyPairStreaming{ + TLSKeyFile: "", + TLSCertFile: "non-empty", + }, + }, + }, + }, + tlsMode: -1, + expectErr: true, + }, + "should return error without EnableTLSStreaming and only TLSCertFile set": { + criService: &criService{ + config: config.Config{ + PluginConfig: config.PluginConfig{ + EnableTLSStreaming: false, + X509KeyPairStreaming: config.X509KeyPairStreaming{ + TLSKeyFile: "", + TLSCertFile: "non-empty", + }, + }, + }, + }, + tlsMode: -1, + expectErr: true, + }, + "should return error without EnableTLSStreaming and only TLSKeyFile set": { + criService: &criService{ + config: config.Config{ + PluginConfig: config.PluginConfig{ + EnableTLSStreaming: false, + X509KeyPairStreaming: config.X509KeyPairStreaming{ + TLSKeyFile: "non-empty", + TLSCertFile: "", + }, + }, + }, + }, + tlsMode: -1, + expectErr: true, + }, + } { + t.Run(desc, func(t *testing.T) { + tlsMode, err := getStreamListenerMode(test.criService) + if test.expectErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + assert.Equal(t, test.tlsMode, tlsMode) + }) + } +}