From d97d53bddc9d203fcfa9805cf284e70065cf10e8 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Tue, 12 Nov 2019 03:16:25 -0800 Subject: [PATCH] Honor etcd legacy v/s new config settings properly (#8510) This PR also fixes issues related to - Add proper newline for `mc admin config get` output for more than one targets - Fixes issue of temporary user credentials to have consistent output - Fixes a crash when setting a key with empty values - Fixes a parsing issue with `mc admin config history` - Fixes gateway ENV handling for etcd server and gateway --- cmd/config/config.go | 3 +++ cmd/config/etcd/etcd.go | 60 +++++++++++++++++++++++++++++++++++++---- cmd/gateway-main.go | 44 +++++++++++++++--------------- cmd/iam.go | 16 ++++++++--- cmd/server-main.go | 18 ++++++------- pkg/madmin/parse-kv.go | 17 ++++++++++-- 6 files changed, 116 insertions(+), 42 deletions(-) diff --git a/cmd/config/config.go b/cmd/config/config.go index fd26eb9b2..b1b3561c7 100644 --- a/cmd/config/config.go +++ b/cmd/config/config.go @@ -405,6 +405,9 @@ func (c Config) SetKVS(s string, defaultKVS map[string]KVS) error { kvs[prevK] = strings.Join([]string{kvs[prevK], sanitizeValue(kv[0])}, KvSpaceSeparator) continue } + if len(kv) == 1 { + return Error(fmt.Sprintf("key '%s', cannot have empty value", kv[0])) + } prevK = kv[0] kvs[kv[0]] = sanitizeValue(kv[1]) } diff --git a/cmd/config/etcd/etcd.go b/cmd/config/etcd/etcd.go index d28789e10..231eca487 100644 --- a/cmd/config/etcd/etcd.go +++ b/cmd/config/etcd/etcd.go @@ -95,6 +95,39 @@ func parseEndpoints(endpoints string) ([]string, bool, error) { return etcdEndpoints, etcdSecure, nil } +func lookupLegacyConfig(rootCAs *x509.CertPool) (Config, error) { + cfg := Config{} + endpoints := env.Get(EnvEtcdEndpoints, "") + if endpoints == "" { + return cfg, nil + } + etcdEndpoints, etcdSecure, err := parseEndpoints(endpoints) + if err != nil { + return cfg, err + } + cfg.Enabled = true + cfg.DialTimeout = defaultDialTimeout + cfg.DialKeepAliveTime = defaultDialKeepAlive + cfg.Endpoints = etcdEndpoints + cfg.CoreDNSPath = "/skydns" + if etcdSecure { + cfg.TLS = &tls.Config{ + RootCAs: rootCAs, + } + // This is only to support client side certificate authentication + // https://coreos.com/etcd/docs/latest/op-guide/security.html + etcdClientCertFile := env.Get(EnvEtcdClientCert, "") + etcdClientCertKey := env.Get(EnvEtcdClientCertKey, "") + if etcdClientCertFile != "" && etcdClientCertKey != "" { + cfg.TLS.GetClientCertificate = func(unused *tls.CertificateRequestInfo) (*tls.Certificate, error) { + cert, err := tls.LoadX509KeyPair(etcdClientCertFile, etcdClientCertKey) + return &cert, err + } + } + } + return cfg, nil +} + // LookupConfig - Initialize new etcd config. func LookupConfig(kv config.KVS, rootCAs *x509.CertPool) (Config, error) { cfg := Config{} @@ -102,20 +135,37 @@ func LookupConfig(kv config.KVS, rootCAs *x509.CertPool) (Config, error) { return cfg, err } - stateBool, err := config.ParseBool(env.Get(EnvEtcdState, kv.Get(config.State))) + stateBool, err := config.ParseBool(env.Get(EnvEtcdState, config.StateOn)) if err != nil { return cfg, err } - endpoints := env.Get(EnvEtcdEndpoints, kv.Get(Endpoints)) - if stateBool && len(endpoints) == 0 { - return cfg, config.Error("'endpoints' key cannot be empty if you wish to enable etcd") + if stateBool { + // By default state is 'on' to honor legacy config. + cfg, err = lookupLegacyConfig(rootCAs) + if err != nil { + return cfg, err + } + // If old legacy config is enabled honor it. + if cfg.Enabled { + return cfg, nil + } } - if len(endpoints) == 0 && !stateBool { + stateBool, err = config.ParseBool(env.Get(EnvEtcdState, kv.Get(config.State))) + if err != nil { + return cfg, err + } + + if !stateBool { return cfg, nil } + endpoints := env.Get(EnvEtcdEndpoints, kv.Get(Endpoints)) + if endpoints == "" { + return cfg, config.Error("'endpoints' key cannot be empty to enable etcd") + } + etcdEndpoints, etcdSecure, err := parseEndpoints(endpoints) if err != nil { return cfg, err diff --git a/cmd/gateway-main.go b/cmd/gateway-main.go index dc8331cab..b7dac33ec 100644 --- a/cmd/gateway-main.go +++ b/cmd/gateway-main.go @@ -147,6 +147,25 @@ func StartGateway(ctx *cli.Context, gw Gateway) { // Set when gateway is enabled globalIsGateway = true + enableConfigOps := gatewayName == "nas" + + // TODO: We need to move this code with globalConfigSys.Init() + // for now keep it here such that "s3" gateway layer initializes + // itself properly when KMS is set. + + // Initialize server config. + srvCfg := newServerConfig() + + // Override any values from ENVs. + if err := lookupConfigs(srvCfg); err != nil { + logger.FatalIf(err, "Unable to initialize server config") + } + + // hold the mutex lock before a new config is assigned. + globalServerConfigMu.Lock() + globalServerConfig = srvCfg + globalServerConfigMu.Unlock() + router := mux.NewRouter().SkipClean(true) if globalEtcdClient != nil { @@ -156,8 +175,6 @@ func StartGateway(ctx *cli.Context, gw Gateway) { // Initialize globalConsoleSys system globalConsoleSys = NewConsoleLogger(context.Background(), globalEndpoints) - - enableConfigOps := gatewayName == "nas" enableIAMOps := globalEtcdClient != nil // Enable IAM admin APIs if etcd is enabled, if not just enable basic @@ -187,32 +204,14 @@ func StartGateway(ctx *cli.Context, gw Gateway) { getCert = globalTLSCerts.GetCertificate } - globalHTTPServer = xhttp.NewServer([]string{globalCLIContext.Addr}, criticalErrorHandler{registerHandlers(router, globalHandlers...)}, getCert) + globalHTTPServer = xhttp.NewServer([]string{globalCLIContext.Addr}, + criticalErrorHandler{registerHandlers(router, globalHandlers...)}, getCert) go func() { globalHTTPServerErrorCh <- globalHTTPServer.Start() }() signal.Notify(globalOSSignalCh, os.Interrupt, syscall.SIGTERM) - if !enableConfigOps { - // TODO: We need to move this code with globalConfigSys.Init() - // for now keep it here such that "s3" gateway layer initializes - // itself properly when KMS is set. - - // Initialize server config. - srvCfg := newServerConfig() - - // Override any values from ENVs. - if err := lookupConfigs(srvCfg); err != nil { - logger.FatalIf(err, "Unable to initialize server config") - } - - // hold the mutex lock before a new config is assigned. - globalServerConfigMu.Lock() - globalServerConfig = srvCfg - globalServerConfigMu.Unlock() - } - newObject, err := gw.NewGatewayLayer(globalActiveCred) if err != nil { // Stop watching for any certificate changes. @@ -252,7 +251,6 @@ func StartGateway(ctx *cli.Context, gw Gateway) { // **** WARNING **** // Migrating to encrypted backend should happen before initialization of any // sub-systems, make sure that we do not move the above codeblock elsewhere. - if enableConfigOps { logger.FatalIf(globalConfigSys.Init(newObject), "Unable to initialize config system") diff --git a/cmd/iam.go b/cmd/iam.go index ce5b7df11..622999e58 100644 --- a/cmd/iam.go +++ b/cmd/iam.go @@ -578,7 +578,12 @@ func (sys *IAMSys) ListUsers() (map[string]madmin.UserInfo, error) { for k, v := range sys.iamUsersMap { users[k] = madmin.UserInfo{ PolicyName: sys.iamUserPolicyMap[k].Policy, - Status: madmin.AccountStatus(v.Status), + Status: func() madmin.AccountStatus { + if v.IsValid() { + return madmin.AccountEnabled + } + return madmin.AccountDisabled + }(), } } @@ -609,8 +614,13 @@ func (sys *IAMSys) GetUserInfo(name string) (u madmin.UserInfo, err error) { u = madmin.UserInfo{ PolicyName: sys.iamUserPolicyMap[name].Policy, - Status: madmin.AccountStatus(creds.Status), - MemberOf: sys.iamUserGroupMemberships[name].ToSlice(), + Status: func() madmin.AccountStatus { + if creds.IsValid() { + return madmin.AccountEnabled + } + return madmin.AccountDisabled + }(), + MemberOf: sys.iamUserGroupMemberships[name].ToSlice(), } return u, nil } diff --git a/cmd/server-main.go b/cmd/server-main.go index 46800adb1..0d2e2bc55 100644 --- a/cmd/server-main.go +++ b/cmd/server-main.go @@ -235,15 +235,6 @@ func initSafeModeInit(buckets []BucketInfo) (err error) { return err } - if globalEtcdClient != nil { - // **** WARNING **** - // Migrating to encrypted backend on etcd should happen before initialization of - // IAM sub-systems, make sure that we do not move the above codeblock elsewhere. - if err = migrateIAMConfigsEtcdToEncrypted(globalEtcdClient); err != nil { - return fmt.Errorf("Unable to handle encrypted backend for iam and policies: %v", err) - } - } - return nil } @@ -257,6 +248,15 @@ func initAllSubsystems(buckets []BucketInfo, newObject ObjectLayer) (err error) return fmt.Errorf("Unable to initialize notification target(s) from config: %w", err) } + if globalEtcdClient != nil { + // **** WARNING **** + // Migrating to encrypted backend on etcd should happen before initialization of + // IAM sub-systems, make sure that we do not move the above codeblock elsewhere. + if err = migrateIAMConfigsEtcdToEncrypted(globalEtcdClient); err != nil { + return fmt.Errorf("Unable to handle encrypted backend for iam and policies: %v", err) + } + } + if err = globalIAMSys.Init(newObject); err != nil { return fmt.Errorf("Unable to initialize IAM system: %w", err) } diff --git a/pkg/madmin/parse-kv.go b/pkg/madmin/parse-kv.go index 97c6152c2..f03871fec 100644 --- a/pkg/madmin/parse-kv.go +++ b/pkg/madmin/parse-kv.go @@ -37,10 +37,23 @@ const ( commentKey = "comment" ) +// Count - returns total numbers of target +func (t Targets) Count() int { + var count int + for _, targetKV := range t { + for range targetKV { + count++ + } + } + return count +} + func (t Targets) String() string { var s strings.Builder + count := t.Count() for subSys, targetKV := range t { for target, kv := range targetKV { + count-- c := kv[commentKey] data, err := base64.RawStdEncoding.DecodeString(c) if err == nil { @@ -73,7 +86,7 @@ func (t Targets) String() string { s.WriteString(KvDoubleQuote) s.WriteString(KvSpaceSeparator) } - if len(t) > 1 { + if (len(t) > 1 || len(targetKV) > 1) && count > 0 { s.WriteString(KvNewline) s.WriteString(KvNewline) } @@ -121,7 +134,7 @@ func convertTargets(s string, targets Targets) error { kvs[prevK] = strings.Join([]string{kvs[prevK], sanitizeValue(kv[0])}, KvSpaceSeparator) continue } - if len(kv[1]) == 0 { + if len(kv) == 1 { return fmt.Errorf("value for key '%s' cannot be empty", kv[0]) } prevK = kv[0]