Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tls: add cipher flag #9216

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*.test
hack/tls-setup/certs
.idea
.vscode

# TODO: use dep prune
# https://github.com/golang/dep/issues/120#issuecomment-306518546
Expand Down
6 changes: 6 additions & 0 deletions embed/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -555,12 +555,15 @@ func (cfg Config) defaultClientHost() bool {
}

func (cfg *Config) ClientSelfCert() (err error) {
cipherSuites := cfg.ClientTLSInfo.CipherSuites

if cfg.ClientAutoTLS && cfg.ClientTLSInfo.Empty() {
chosts := make([]string, len(cfg.LCUrls))
for i, u := range cfg.LCUrls {
chosts[i] = u.Host
}
cfg.ClientTLSInfo, err = transport.SelfCert(filepath.Join(cfg.Dir, "fixtures", "client"), chosts)
cfg.ClientTLSInfo.CipherSuites = cipherSuites
return err
} else if cfg.ClientAutoTLS {
plog.Warningf("ignoring client auto TLS since certs given")
Expand All @@ -569,12 +572,15 @@ func (cfg *Config) ClientSelfCert() (err error) {
}

func (cfg *Config) PeerSelfCert() (err error) {
cipherSuites := cfg.PeerTLSInfo.CipherSuites

if cfg.PeerAutoTLS && cfg.PeerTLSInfo.Empty() {
phosts := make([]string, len(cfg.LPUrls))
for i, u := range cfg.LPUrls {
phosts[i] = u.Host
}
cfg.PeerTLSInfo, err = transport.SelfCert(filepath.Join(cfg.Dir, "fixtures", "peer"), phosts)
cfg.PeerTLSInfo.CipherSuites = cipherSuites
return err
} else if cfg.PeerAutoTLS {
plog.Warningf("ignoring peer auto TLS since certs given")
Expand Down
19 changes: 15 additions & 4 deletions etcdctl/ctlv2/command/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"time"

"github.com/coreos/etcd/client"
"github.com/coreos/etcd/pkg/flags"
"github.com/coreos/etcd/pkg/transport"

"github.com/bgentry/speakeasy"
Expand Down Expand Up @@ -155,6 +156,8 @@ func getTransport(c *cli.Context) (*http.Transport, error) {
cafile := c.GlobalString("ca-file")
certfile := c.GlobalString("cert-file")
keyfile := c.GlobalString("key-file")
insecureSkipVerify := c.GlobalBool("insecure-skip-tls-verify")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need this in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree, would like this removed

ciphers := c.GlobalGeneric("cipher-suites").(*flags.StringSliceFlag)

// Use an environment variable if nothing was supplied on the
// command line
Expand All @@ -167,16 +170,24 @@ func getTransport(c *cli.Context) (*http.Transport, error) {
if keyfile == "" {
keyfile = os.Getenv("ETCDCTL_KEY_FILE")
}
if !insecureSkipVerify {
insecureSkipVerify = strings.EqualFold(os.Getenv("ETCDCTL_INSECURE_SKIP_TLS_VERIFY"), "TRUE")
}
if c := ciphers.Slice(); len(c) == 0 {
ciphers.Set(os.Getenv("ETCDCTL_CIPHER_SUITES"))
}

discoveryDomain, insecure := getDiscoveryDomain(c)
if insecure {
discoveryDomain = ""
}
tls := transport.TLSInfo{
CAFile: cafile,
CertFile: certfile,
KeyFile: keyfile,
ServerName: discoveryDomain,
CAFile: cafile,
CertFile: certfile,
KeyFile: keyfile,
CipherSuites: ciphers.Slice(),
ServerName: discoveryDomain,
InsecureSkipVerify: insecureSkipVerify,
}

dialTimeout := defaultDialTimeout
Expand Down
9 changes: 9 additions & 0 deletions etcdctl/ctlv2/ctl.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,13 @@ package ctlv2
import (
"fmt"
"os"
"strings"
"time"

"github.com/coreos/etcd/etcdctl/ctlv2/command"
"github.com/coreos/etcd/internal/version"
"github.com/coreos/etcd/pkg/flags"
"github.com/coreos/etcd/pkg/tlsutil"

"github.com/urfave/cli"
)
Expand Down Expand Up @@ -55,6 +58,12 @@ func Start(apiv string) {
cli.StringFlag{Name: "cert-file", Value: "", Usage: "identify HTTPS client using this SSL certificate file"},
cli.StringFlag{Name: "key-file", Value: "", Usage: "identify HTTPS client using this SSL key file"},
cli.StringFlag{Name: "ca-file", Value: "", Usage: "verify certificates of HTTPS-enabled servers using this CA bundle"},
cli.BoolFlag{Name: "insecure-skip-tls-verify", Usage: "skip server certificate verification"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need this in this PR? or it can be separated to another one?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it to make local testing easier, can break it out if you want

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea. please break it out.

cli.GenericFlag{Name: "cipher-suites",
Value: flags.NewStringSliceFlag(tlsutil.AvailableCipherSuites()...),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this mean AvailableCipherSuites() is the default value? I didn't think all available ciphers were included in the default set in go

Copy link
Author

@ThatsMrTalbot ThatsMrTalbot Feb 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is used to validate the provided list, but the default value is still a nil slice (which is what is currently used)

Usage: "comma-separated list of cipher suites for the server. " +
"Available cipher suites include " + strings.Join(tlsutil.AvailableCipherSuites(), ",") + ". " +
"If omitted, the default Go cipher suites will be used"},
cli.StringFlag{Name: "username, u", Value: "", Usage: "provide username[:password] and prompt if password is not supplied."},
cli.DurationFlag{Name: "timeout", Value: 2 * time.Second, Usage: "connection timeout per request"},
cli.DurationFlag{Name: "total-timeout", Value: 5 * time.Second, Usage: "timeout for the command execution (except watch)"},
Expand Down
16 changes: 16 additions & 0 deletions etcdctl/ctlv3/command/global.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ type secureCfg struct {
cert string
key string
cacert string
ciphers []string
serverName string

insecureTransport bool
Expand Down Expand Up @@ -184,6 +185,11 @@ func newClientCfg(endpoints []string, dialTimeout, keepAliveTime, keepAliveTimeo
cfgtls = &tlsinfo
}

if len(scfg.ciphers) != 0 {
tlsinfo.CipherSuites = scfg.ciphers
cfgtls = &tlsinfo
}

if scfg.serverName != "" {
tlsinfo.ServerName = scfg.serverName
cfgtls = &tlsinfo
Expand Down Expand Up @@ -264,6 +270,7 @@ func secureCfgFromCmd(cmd *cobra.Command) *secureCfg {
cert, key, cacert := keyAndCertFromCmd(cmd)
insecureTr := insecureTransportFromCmd(cmd)
skipVerify := insecureSkipVerifyFromCmd(cmd)
ciphers := ciphersFromCmd(cmd)
discoveryCfg := discoveryCfgFromCmd(cmd)

if discoveryCfg.insecure {
Expand All @@ -274,6 +281,7 @@ func secureCfgFromCmd(cmd *cobra.Command) *secureCfg {
cert: cert,
key: key,
cacert: cacert,
ciphers: ciphers,
serverName: discoveryCfg.domain,

insecureTransport: insecureTr,
Expand Down Expand Up @@ -320,6 +328,14 @@ func keyAndCertFromCmd(cmd *cobra.Command) (cert, key, cacert string) {
return cert, key, cacert
}

func ciphersFromCmd(cmd *cobra.Command) []string {
ciphers, err := cmd.Flags().GetStringSlice("cipher-suites")
if err != nil {
ExitWithError(ExitError, err)
}
return ciphers
}

func authCfgFromCmd(cmd *cobra.Command) *authCfg {
userFlag, err := cmd.Flags().GetString("user")
if err != nil {
Expand Down
5 changes: 5 additions & 0 deletions etcdctl/ctlv3/ctl.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,11 @@
package ctlv3

import (
"strings"
"time"

"github.com/coreos/etcd/etcdctl/ctlv3/command"
"github.com/coreos/etcd/pkg/tlsutil"
"github.com/spf13/cobra"
)

Expand Down Expand Up @@ -63,6 +65,9 @@ func init() {
rootCmd.PersistentFlags().StringVar(&globalFlags.TLS.CertFile, "cert", "", "identify secure client using this TLS certificate file")
rootCmd.PersistentFlags().StringVar(&globalFlags.TLS.KeyFile, "key", "", "identify secure client using this TLS key file")
rootCmd.PersistentFlags().StringVar(&globalFlags.TLS.CAFile, "cacert", "", "verify certificates of TLS-enabled secure servers using this CA bundle")
rootCmd.PersistentFlags().StringSliceVar(&globalFlags.TLS.CipherSuites, "cipher-suites", nil, "comma-separated list of cipher suites for the server. "+
"Available cipher suites include "+strings.Join(tlsutil.AvailableCipherSuites(), ",")+". "+
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: join with ", " so text can wrap

"If omitted, the default Go cipher suites will be used")
rootCmd.PersistentFlags().StringVar(&globalFlags.User, "user", "", "username[:password] for authentication (prompt if password is not supplied)")
rootCmd.PersistentFlags().StringVarP(&globalFlags.TLS.ServerName, "discovery-srv", "d", "", "domain name to query for SRV records describing cluster endpoints")

Expand Down
18 changes: 14 additions & 4 deletions etcdmain/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/coreos/etcd/embed"
"github.com/coreos/etcd/internal/version"
"github.com/coreos/etcd/pkg/flags"
"github.com/coreos/etcd/pkg/tlsutil"
"github.com/coreos/etcd/pkg/types"

"github.com/ghodss/yaml"
Expand Down Expand Up @@ -85,10 +86,11 @@ type config struct {

// configFlags has the set of flags used for command line parsing a Config
type configFlags struct {
flagSet *flag.FlagSet
clusterState *flags.StringsFlag
fallback *flags.StringsFlag
proxy *flags.StringsFlag
flagSet *flag.FlagSet
clusterState *flags.StringsFlag
fallback *flags.StringsFlag
proxy *flags.StringsFlag
tlsCipherSuites *flags.StringSliceFlag
}

func newConfig() *config {
Expand Down Expand Up @@ -118,6 +120,9 @@ func newConfig() *config {
proxyFlagReadonly,
proxyFlagOn,
),
tlsCipherSuites: flags.NewStringSliceFlag(
tlsutil.AvailableCipherSuites()...,
),
}

fs := cfg.cf.flagSet
Expand Down Expand Up @@ -189,6 +194,9 @@ func newConfig() *config {
fs.BoolVar(&cfg.ec.PeerAutoTLS, "peer-auto-tls", false, "Peer TLS using generated certificates")
fs.StringVar(&cfg.ec.PeerTLSInfo.CRLFile, "peer-crl-file", "", "Path to the peer certificate revocation list file.")
fs.StringVar(&cfg.ec.PeerTLSInfo.AllowedCN, "peer-cert-allowed-cn", "", "Allowed CN for inter peer authentication.")
fs.Var(cfg.cf.tlsCipherSuites, "cipher-suites", "comma-separated list of cipher suites for the server. "+
"Available cipher suites include "+strings.Join(tlsutil.AvailableCipherSuites(), ",")+". "+
"If omitted, the default Go cipher suites will be used")

// logging
fs.BoolVar(&cfg.ec.Debug, "debug", false, "Enable debug-level logging for etcd.")
Expand Down Expand Up @@ -278,6 +286,8 @@ func (cfg *config) configFromCmdLine() error {
cfg.ec.ClusterState = cfg.cf.clusterState.String()
cfg.cp.Fallback = cfg.cf.fallback.String()
cfg.cp.Proxy = cfg.cf.proxy.String()
cfg.ec.ClientTLSInfo.CipherSuites = cfg.cf.tlsCipherSuites.Slice()
cfg.ec.PeerTLSInfo.CipherSuites = cfg.cf.tlsCipherSuites.Slice()

// disable default advertise-client-urls if lcurls is set
missingAC := flags.IsSet(cfg.cf.flagSet, "listen-client-urls") && !flags.IsSet(cfg.cf.flagSet, "advertise-client-urls")
Expand Down
4 changes: 4 additions & 0 deletions etcdmain/help.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@ package etcdmain

import (
"strconv"
"strings"

"github.com/coreos/etcd/embed"
"github.com/coreos/etcd/pkg/tlsutil"
)

var (
Expand Down Expand Up @@ -158,6 +160,8 @@ security flags:
peer TLS using self-generated certificates if --peer-key-file and --peer-cert-file are not provided.
--peer-crl-file ''
path to the peer certificate revocation list file.
--cipher-suites ''
comma-separated list of cipher suites for the server. Available cipher suites include ` + strings.Join(tlsutil.AvailableCipherSuites(), ",") + `. If omitted, the default Go cipher suites will be used.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

", "


logging flags

Expand Down
79 changes: 72 additions & 7 deletions pkg/flags/strings.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@

package flags

import "errors"
import (
"errors"
"strings"
)

// NewStringsFlag creates a new string flag for which any one of the given
// strings is a valid value, and any other value is an error.
Expand All @@ -25,25 +28,87 @@ func NewStringsFlag(valids ...string) *StringsFlag {
return &StringsFlag{Values: valids, val: valids[0]}
}

// StringsFlag implements the flag.Value interface.
// StringsFlag implements the flag.Value and pflag.Value interfaces.
type StringsFlag struct {
Values []string
val string
}

// Set verifies the argument to be a valid member of the allowed values
// before setting the underlying flag value.
func (ss *StringsFlag) Set(s string) error {
for _, v := range ss.Values {
func (sf *StringsFlag) Set(s string) error {
for _, v := range sf.Values {
if s == v {
ss.val = s
sf.val = s
return nil
}
}
return errors.New("invalid value")
}

// String returns the set value (if any) of the StringsFlag
func (ss *StringsFlag) String() string {
return ss.val
func (sf *StringsFlag) String() string {
return sf.val
}

// Type returns the given type as string
func (sf *StringsFlag) Type() string {
return "string"
}

// StringSliceFlag implements the flag.Value and pflag.Value interfaces.
type StringSliceFlag struct {
Values []string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a confusing name, maybe ValidValues instead?

val []string
}

// NewStringSliceFlag creates a new string slice flag for which any one of the given
// strings is a valid value, and any other value is an error.
func NewStringSliceFlag(valids ...string) *StringSliceFlag {
Copy link
Contributor

@liggitt liggitt Apr 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest giving separate control over valid values and default value(s), instead of assuming []string{}

return &StringSliceFlag{Values: valids, val: []string{}}
}

// Set verifies the argument to be a valid member of the allowed values
// before setting the underlying flag value.
func (ssf *StringSliceFlag) Set(s string) error {
sl := strings.Split(s, ",")
ssf.val = []string{}

for _, s := range sl {
if !ssf.has(s) {
return errors.New("invalid value")
}

ssf.val = append(ssf.val, s)
}

return nil
}

// String returns the set value (if any) of the StringSliceFlag
func (ssf *StringSliceFlag) String() string {
return strings.Join(ssf.val, ",")
}

// Slice returns the set value (if any) of the StringSliceFlag as a slice
func (ssf *StringSliceFlag) Slice() []string {
clone := make([]string, len(ssf.val))
copy(clone, ssf.val)
return clone
}

// Type returns the given type as stringSlice
func (ssf *StringSliceFlag) Type() string {
return "stringSlice"
}

// has checks to see if value in in allowed slice
func (ssf *StringSliceFlag) has(s string) bool {
for _, v := range ssf.Values {
if v == s {
return true
}
}

return false
}
Loading