From 32c200fe12c0747f3eefe5c8625319346eb651b5 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Thu, 14 Nov 2019 14:19:57 -0800 Subject: [PATCH] Fix console logger crash in gateway mode (#8525) This PR also fixes config migration only for credentials and region which are valid and set. Also fix implicit `state="on"` behavior --- cmd/config/config.go | 30 +++++++++++++++------- cmd/config/config_test.go | 44 ++++++++++++++++++++++++++++++++ cmd/config/legacy.go | 10 ++++++++ cmd/gateway-main.go | 5 ++-- pkg/madmin/config-kv-commands.go | 10 -------- 5 files changed, 78 insertions(+), 21 deletions(-) create mode 100644 cmd/config/config_test.go diff --git a/cmd/config/config.go b/cmd/config/config.go index dfca85789..5eafd868e 100644 --- a/cmd/config/config.go +++ b/cmd/config/config.go @@ -19,6 +19,7 @@ package config import ( "fmt" + "regexp" "strings" "github.com/minio/minio-go/pkg/set" @@ -233,6 +234,8 @@ func LookupCreds(kv KVS) (auth.Credentials, error) { return auth.CreateCredentials(accessKey, secretKey) } +var validRegionRegex = regexp.MustCompile("^[a-zA-Z][a-zA-Z0-9-_-]+$") + // LookupRegion - get current region. func LookupRegion(kv KVS) (string, error) { if err := CheckValidKeys(RegionSubSys, kv, DefaultRegionKVS); err != nil { @@ -242,7 +245,14 @@ func LookupRegion(kv KVS) (string, error) { if region == "" { region = env.Get(EnvRegionName, kv.Get(RegionName)) } - return region, nil + if region != "" { + if validRegionRegex.MatchString(region) { + return region, nil + } + return "", Errorf("region '%s' is invalid, expected simple characters such as [us-east-1, myregion...]", + region) + } + return "", nil } // CheckValidKeys - checks if inputs KVS has the necessary keys, @@ -299,9 +309,8 @@ func (c Config) GetKVS(s string) (map[string]KVS, error) { } found := SubSystems.Contains(subSystemValue[0]) if !found { - // Check for sub-prefix only if the input value - // is only a single value, this rejects invalid - // inputs if any. + // Check for sub-prefix only if the input value is only a + // single value, this rejects invalid inputs if any. found = !SubSystems.FuncMatch(strings.HasPrefix, subSystemValue[0]).IsEmpty() && len(subSystemValue) == 1 } if !found { @@ -431,17 +440,20 @@ func (c Config) SetKVS(s string) error { if len(subSystemValue) == 2 { tgt = subSystemValue[1] } + _, ok := c[subSystemValue[0]][tgt] if !ok { c[subSystemValue[0]][tgt] = KVS{} } for k, v := range kvs { - if len(subSystemValue) == 2 { - c[subSystemValue[0]][subSystemValue[1]][k] = v - } else { - c[subSystemValue[0]][Default][k] = v - } + c[subSystemValue[0]][tgt][k] = v + } + + _, ok = c[subSystemValue[0]][tgt][State] + if !ok { + // implicit state "on" if not specified. + c[subSystemValue[0]][tgt][State] = StateOn } return nil diff --git a/cmd/config/config_test.go b/cmd/config/config_test.go new file mode 100644 index 000000000..e5d407680 --- /dev/null +++ b/cmd/config/config_test.go @@ -0,0 +1,44 @@ +/* + * MinIO Cloud Storage, (C) 2019 MinIO, Inc. + * + * 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 config + +import "testing" + +func TestValidRegion(t *testing.T) { + tests := []struct { + name string + success bool + }{ + {name: "us-east-1", success: true}, + {name: "us_east", success: true}, + {name: "helloWorld", success: true}, + {name: "-fdslka", success: false}, + {name: "^00[", success: false}, + {name: "my region", success: false}, + {name: "%%$#!", success: false}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + ok := validRegionRegex.MatchString(test.name) + if test.success != ok { + t.Errorf("Expected %t, got %t", test.success, ok) + } + }) + } +} diff --git a/cmd/config/legacy.go b/cmd/config/legacy.go index 58fc93459..b684a4042 100644 --- a/cmd/config/legacy.go +++ b/cmd/config/legacy.go @@ -23,6 +23,13 @@ import "github.com/minio/minio/pkg/auth" // SetCredentials - One time migration code needed, for migrating from older config to new for server credentials. func SetCredentials(c Config, cred auth.Credentials) { + creds, err := auth.CreateCredentials(cred.AccessKey, cred.SecretKey) + if err != nil { + return + } + if !creds.IsValid() { + return + } c[CredentialsSubSys][Default] = KVS{ State: StateOn, Comment: "Settings for credentials, after migrating config", @@ -33,6 +40,9 @@ func SetCredentials(c Config, cred auth.Credentials) { // SetRegion - One time migration code needed, for migrating from older config to new for server Region. func SetRegion(c Config, name string) { + if name == "" { + return + } c[RegionSubSys][Default] = KVS{ RegionName: name, State: StateOn, diff --git a/cmd/gateway-main.go b/cmd/gateway-main.go index f96e6a1de..68d13aa52 100644 --- a/cmd/gateway-main.go +++ b/cmd/gateway-main.go @@ -145,6 +145,9 @@ func StartGateway(ctx *cli.Context, gw Gateway) { // Set when gateway is enabled globalIsGateway = true + // Initialize globalConsoleSys system + globalConsoleSys = NewConsoleLogger(context.Background(), globalEndpoints) + enableConfigOps := gatewayName == "nas" // TODO: We need to move this code with globalConfigSys.Init() @@ -171,8 +174,6 @@ func StartGateway(ctx *cli.Context, gw Gateway) { registerSTSRouter(router) } - // Initialize globalConsoleSys system - globalConsoleSys = NewConsoleLogger(context.Background(), globalEndpoints) enableIAMOps := globalEtcdClient != nil // Enable IAM admin APIs if etcd is enabled, if not just enable basic diff --git a/pkg/madmin/config-kv-commands.go b/pkg/madmin/config-kv-commands.go index 36a402229..330fac6fc 100644 --- a/pkg/madmin/config-kv-commands.go +++ b/pkg/madmin/config-kv-commands.go @@ -55,16 +55,6 @@ func (adm *AdminClient) SetConfigKV(kv string) (err error) { if err != nil { return err } - for subSys, targetKV := range targets { - for target := range targetKV { - _, ok := targets[subSys][target][stateKey] - if !ok { - // If client asked for state preserve. - // otherwise implicitly add state to "on" - targets[subSys][target][stateKey] = stateOn - } - } - } econfigBytes, err := EncryptData(adm.secretAccessKey, []byte(targets.String())) if err != nil { return err