From 780ccc26f79ef28ecc7c0ddd38872f8e73015cdc Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Fri, 26 Aug 2016 00:11:53 -0700 Subject: [PATCH] server: Validate server arguments for duplicates. (#2554) - Validates invalid format inputs. - Validates duplicate entries. - Validates sufficient amount of disks. Partially fixes #2502 --- .travis.yml | 4 +-- cmd/bucket-notification-utils.go | 39 ++++++++++++----------- cmd/server-main.go | 54 ++++++++++++++++++++++++++++++++ cmd/utils.go | 45 ++++++++++++++++++++------ cmd/utils_nix_test.go | 10 ++++-- cmd/utils_test.go | 52 ++++++++++++++++++++++++++++++ cmd/utils_windows_test.go | 21 +++++++++++-- cmd/xl-v1-errors.go | 3 ++ cmd/xl-v1.go | 44 +++++--------------------- cmd/xl-v1_test.go | 15 ++++++++- 10 files changed, 216 insertions(+), 71 deletions(-) diff --git a/.travis.yml b/.travis.yml index 082b333b0..d046c1b7a 100644 --- a/.travis.yml +++ b/.travis.yml @@ -4,9 +4,9 @@ language: go os: - linux -#- osx +- osx -#osx_image: xcode7.2 +osx_image: xcode7.2 env: - ARCH=x86_64 diff --git a/cmd/bucket-notification-utils.go b/cmd/bucket-notification-utils.go index 27930440c..04b63ee02 100644 --- a/cmd/bucket-notification-utils.go +++ b/cmd/bucket-notification-utils.go @@ -268,18 +268,17 @@ func validateTopicConfigs(topicConfigs []topicConfig) APIErrorCode { // Check all the queue configs for any duplicates. func checkDuplicateQueueConfigs(configs []queueConfig) APIErrorCode { - configMaps := make(map[string]int) + var queueConfigARNS []string // Navigate through each configs and count the entries. for _, config := range configs { - configMaps[config.QueueARN]++ + queueConfigARNS = append(queueConfigARNS, config.QueueARN) } - // Validate if there are any duplicate counts. - for _, count := range configMaps { - if count != 1 { - return ErrOverlappingConfigs - } + // Check if there are any duplicate counts. + if err := checkDuplicates(queueConfigARNS); err != nil { + errorIf(err, "Invalid queue configs found.") + return ErrOverlappingConfigs } // Success. @@ -288,18 +287,17 @@ func checkDuplicateQueueConfigs(configs []queueConfig) APIErrorCode { // Check all the topic configs for any duplicates. func checkDuplicateTopicConfigs(configs []topicConfig) APIErrorCode { - configMaps := make(map[string]int) + var topicConfigARNS []string // Navigate through each configs and count the entries. for _, config := range configs { - configMaps[config.TopicARN]++ + topicConfigARNS = append(topicConfigARNS, config.TopicARN) } - // Validate if there are any duplicate counts. - for _, count := range configMaps { - if count != 1 { - return ErrOverlappingConfigs - } + // Check if there are any duplicate counts. + if err := checkDuplicates(topicConfigARNS); err != nil { + errorIf(err, "Invalid topic configs found.") + return ErrOverlappingConfigs } // Success. @@ -320,12 +318,17 @@ func validateNotificationConfig(nConfig notificationConfig) APIErrorCode { } // Check for duplicate queue configs. - if s3Error := checkDuplicateQueueConfigs(nConfig.QueueConfigs); s3Error != ErrNone { - return s3Error + if len(nConfig.QueueConfigs) > 1 { + if s3Error := checkDuplicateQueueConfigs(nConfig.QueueConfigs); s3Error != ErrNone { + return s3Error + } } + // Check for duplicate topic configs. - if s3Error := checkDuplicateTopicConfigs(nConfig.TopicConfigs); s3Error != ErrNone { - return s3Error + if len(nConfig.TopicConfigs) > 1 { + if s3Error := checkDuplicateTopicConfigs(nConfig.TopicConfigs); s3Error != ErrNone { + return s3Error + } } // Add validation for other configurations. diff --git a/cmd/server-main.go b/cmd/server-main.go index 7a45080c5..976da0ac4 100644 --- a/cmd/server-main.go +++ b/cmd/server-main.go @@ -202,16 +202,70 @@ func initServerConfig(c *cli.Context) { // Do not fail if this is not allowed, lower limits are fine as well. } +// Validate if input disks are sufficient for initializing XL. +func checkSufficientDisks(disks []string) error { + // Verify total number of disks. + totalDisks := len(disks) + if totalDisks > maxErasureBlocks { + return errXLMaxDisks + } + if totalDisks < minErasureBlocks { + return errXLMinDisks + } + + // isEven function to verify if a given number if even. + isEven := func(number int) bool { + return number%2 == 0 + } + + // Verify if we have even number of disks. + // only combination of 4, 6, 8, 10, 12, 14, 16 are supported. + if !isEven(totalDisks) { + return errXLNumDisks + } + + // Success. + return nil +} + +// Validates if disks are of supported format, invalid arguments are rejected. +func checkNamingDisks(disks []string) error { + for _, disk := range disks { + _, _, err := splitNetPath(disk) + if err != nil { + return err + } + } + return nil +} + // Check server arguments. func checkServerSyntax(c *cli.Context) { if !c.Args().Present() || c.Args().First() == "help" { cli.ShowCommandHelpAndExit(c, "server", 1) } + disks := c.Args() + if len(disks) > 1 { + // Validate if input disks have duplicates in them. + err := checkDuplicates(disks) + fatalIf(err, "Invalid disk arguments for server.") + + // Validate if input disks are sufficient for erasure coded setup. + err = checkSufficientDisks(disks) + fatalIf(err, "Invalid disk arguments for server.") + + // Validate if input disks are properly named in accordance with either + // - /mnt/disk1 + // - ip:/mnt/disk1 + err = checkNamingDisks(disks) + fatalIf(err, "Invalid disk arguments for server.") + } } // Extract port number from address address should be of the form host:port. func getPort(address string) int { _, portStr, _ := net.SplitHostPort(address) + // If port empty, default to port '80' if portStr == "" { portStr = "80" diff --git a/cmd/utils.go b/cmd/utils.go index 46e6d0a89..f861625aa 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -20,6 +20,7 @@ import ( "encoding/base64" "encoding/xml" "errors" + "fmt" "io" "net" "net/http" @@ -46,6 +47,33 @@ func cloneHeader(h http.Header) http.Header { return h2 } +// checkDuplicates - function to validate if there are duplicates in a slice of strings. +func checkDuplicates(list []string) error { + // Empty lists are not allowed. + if len(list) == 0 { + return errInvalidArgument + } + // Empty keys are not allowed. + for _, key := range list { + if key == "" { + return errInvalidArgument + } + } + listMaps := make(map[string]int) + // Navigate through each configs and count the entries. + for _, key := range list { + listMaps[key]++ + } + // Validate if there are any duplicate counts. + for key, count := range listMaps { + if count != 1 { + return fmt.Errorf("Duplicate key: \"%s\" found of count: \"%d\"", key, count) + } + } + // No duplicates. + return nil +} + // splits network path into its components Address and Path. func splitNetPath(networkPath string) (netAddr, netPath string, err error) { if runtime.GOOS == "windows" { @@ -54,16 +82,15 @@ func splitNetPath(networkPath string) (netAddr, netPath string, err error) { } } networkParts := strings.SplitN(networkPath, ":", 2) - switch len(networkParts) { - case 1: + if len(networkParts) == 1 { return "", networkPath, nil - case 2: - if networkParts[1] == "" { - return "", "", &net.AddrError{Err: "missing path in network path", Addr: networkPath} - } else if networkParts[0] == "" { - return "", "", &net.AddrError{Err: "missing address in network path", Addr: networkPath} - } - return networkParts[0], networkParts[1], nil + } + if networkParts[1] == "" { + return "", "", &net.AddrError{Err: "Missing path in network path", Addr: networkPath} + } else if networkParts[0] == "" { + return "", "", &net.AddrError{Err: "Missing address in network path", Addr: networkPath} + } else if !filepath.IsAbs(networkParts[1]) { + return "", "", &net.AddrError{Err: "Network path should be absolute", Addr: networkPath} } return networkParts[0], networkParts[1], nil } diff --git a/cmd/utils_nix_test.go b/cmd/utils_nix_test.go index 020b2ce08..215e4b5b1 100644 --- a/cmd/utils_nix_test.go +++ b/cmd/utils_nix_test.go @@ -31,11 +31,17 @@ func TestSplitNetPath(t *testing.T) { netPath string err error }{ - {"10.1.10.1:", "", "", &net.AddrError{Err: "missing path in network path", Addr: "10.1.10.1:"}}, + // Invalid cases 1-5. + {"10.1.10.1:", "", "", &net.AddrError{Err: "Missing path in network path", Addr: "10.1.10.1:"}}, + {"10.1.10.1:../1", "", "", &net.AddrError{Err: "Network path should be absolute", Addr: "10.1.10.1:../1"}}, + {":/tmp/1", "", "", &net.AddrError{Err: "Missing address in network path", Addr: ":/tmp/1"}}, + {"10.1.10.1:disk/1", "", "", &net.AddrError{Err: "Network path should be absolute", Addr: "10.1.10.1:disk/1"}}, + {"10.1.10.1:\\path\\test", "", "", &net.AddrError{Err: "Network path should be absolute", Addr: "10.1.10.1:\\path\\test"}}, + + // Valid cases 6-8 {"10.1.10.1", "", "10.1.10.1", nil}, {"10.1.10.1://", "10.1.10.1", "//", nil}, {"10.1.10.1:/disk/1", "10.1.10.1", "/disk/1", nil}, - {"10.1.10.1:\\path\\test", "10.1.10.1", "\\path\\test", nil}, } for i, test := range testCases { diff --git a/cmd/utils_test.go b/cmd/utils_test.go index 3b6215378..800bb73af 100644 --- a/cmd/utils_test.go +++ b/cmd/utils_test.go @@ -17,6 +17,7 @@ package cmd import ( + "fmt" "net/http" "reflect" "testing" @@ -46,6 +47,57 @@ func TestCloneHeader(t *testing.T) { } } +// Tests check duplicates function. +func TestCheckDuplicates(t *testing.T) { + tests := []struct { + list []string + err error + shouldPass bool + }{ + // Test 1 - for '/tmp/1' repeated twice. + { + list: []string{"/tmp/1", "/tmp/1", "/tmp/2", "/tmp/3"}, + err: fmt.Errorf("Duplicate key: \"/tmp/1\" found of count: \"2\""), + shouldPass: false, + }, + // Test 2 - for '/tmp/1' repeated thrice. + { + list: []string{"/tmp/1", "/tmp/1", "/tmp/1", "/tmp/3"}, + err: fmt.Errorf("Duplicate key: \"/tmp/1\" found of count: \"3\""), + shouldPass: false, + }, + // Test 3 - empty string. + { + list: []string{""}, + err: errInvalidArgument, + shouldPass: false, + }, + // Test 4 - empty string. + { + list: nil, + err: errInvalidArgument, + shouldPass: false, + }, + // Test 5 - non repeated strings. + { + list: []string{"/tmp/1", "/tmp/2", "/tmp/3"}, + err: nil, + shouldPass: true, + }, + } + + // Validate if function runs as expected. + for i, test := range tests { + err := checkDuplicates(test.list) + if test.shouldPass && err != test.err { + t.Errorf("Test: %d, Expected %s got %s", i+1, test.err, err) + } + if !test.shouldPass && err.Error() != test.err.Error() { + t.Errorf("Test: %d, Expected %s got %s", i+1, test.err, err) + } + } +} + // Tests maximum object size. func TestMaxObjectSize(t *testing.T) { sizes := []struct { diff --git a/cmd/utils_windows_test.go b/cmd/utils_windows_test.go index d50f717f6..b175c4a42 100644 --- a/cmd/utils_windows_test.go +++ b/cmd/utils_windows_test.go @@ -31,11 +31,26 @@ func TestSplitNetPath(t *testing.T) { netPath string err error }{ + // Invalid cases 1-8. + {":C:", "", "", &net.AddrError{Err: "Missing address in network path", Addr: ":C:"}}, + {"10.1.10.1:", "", "", &net.AddrError{Err: "Missing path in network path", Addr: "10.1.10.1:"}}, + {"10.1.10.1:C", "", "", &net.AddrError{Err: "Network path should be absolute", Addr: "10.1.10.1:C"}}, + {"10.1.10.1:C:", "", "", &net.AddrError{Err: "Network path should be absolute", Addr: "10.1.10.1:C:"}}, + {"10.1.10.1:C:../path", "", "", &net.AddrError{Err: "Network path should be absolute", Addr: "10.1.10.1:C:../path"}}, + {"10.1.10.1:C:tmp/1", "", "", &net.AddrError{Err: "Network path should be absolute", Addr: "10.1.10.1:C:tmp/1"}}, + {"10.1.10.1::C:\\path\\test", "", "", &net.AddrError{ + Err: "Network path should be absolute", + Addr: "10.1.10.1::C:\\path\\test", + }}, + {"10.1.10.1:\\path\\test", "", "", &net.AddrError{ + Err: "Network path should be absolute", + Addr: "10.1.10.1:\\path\\test", + }}, + + // Valid cases 9-11. {"10.1.10.1:C:\\path\\test", "10.1.10.1", "C:\\path\\test", nil}, - {"10.1.10.1:C:", "10.1.10.1", "C:", nil}, - {":C:", "", "", &net.AddrError{Err: "missing address in network path", Addr: ":C:"}}, {"C:\\path\\test", "", "C:\\path\\test", nil}, - {"10.1.10.1::C:\\path\\test", "10.1.10.1", ":C:\\path\\test", nil}, + {`10.1.10.1:\\?\UNC\path\test`, "10.1.10.1", `\\?\UNC\path\test`, nil}, } for i, test := range testCases { diff --git a/cmd/xl-v1-errors.go b/cmd/xl-v1-errors.go index faa03e92f..582f80f82 100644 --- a/cmd/xl-v1-errors.go +++ b/cmd/xl-v1-errors.go @@ -27,6 +27,9 @@ var errXLMinDisks = errors.New("Minimum '4' disks are required to enable erasure // errXLNumDisks - returned for odd number of disks. var errXLNumDisks = errors.New("Total number of disks should be multiples of '2'") +// errXLDuplicateArguments - returned for duplicate disks. +var errXLDuplicateArguments = errors.New("Duplicate disks found.") + // errXLReadQuorum - did not meet read quorum. var errXLReadQuorum = errors.New("Read failed. Insufficient number of disks online") diff --git a/cmd/xl-v1.go b/cmd/xl-v1.go index b2c4e8d1d..e3746186a 100644 --- a/cmd/xl-v1.go +++ b/cmd/xl-v1.go @@ -20,6 +20,7 @@ import ( "fmt" "sort" + "github.com/minio/minio-go/pkg/set" "github.com/minio/minio/pkg/disk" "github.com/minio/minio/pkg/objcache" ) @@ -66,49 +67,20 @@ type xlObjects struct { objCacheEnabled bool } -// Validate if input disks are sufficient for initializing XL. -func checkSufficientDisks(disks []string) error { - // Verify total number of disks. - totalDisks := len(disks) - if totalDisks > maxErasureBlocks { - return errXLMaxDisks - } - if totalDisks < minErasureBlocks { - return errXLMinDisks - } - - // isEven function to verify if a given number if even. - isEven := func(number int) bool { - return number%2 == 0 - } - - // Verify if we have even number of disks. - // only combination of 4, 6, 8, 10, 12, 14, 16 are supported. - if !isEven(totalDisks) { - return errXLNumDisks - } - - // Success. - return nil -} - -// isDiskFound - validates if the disk is found in a list of input disks. -func isDiskFound(disk string, disks []string) bool { - return contains(disks, disk) -} - // newXLObjects - initialize new xl object layer. func newXLObjects(disks, ignoredDisks []string) (ObjectLayer, error) { - // Validate if input disks are sufficient. - if err := checkSufficientDisks(disks); err != nil { - return nil, err + if disks == nil { + return nil, errInvalidArgument + } + disksSet := set.NewStringSet() + if len(ignoredDisks) > 0 { + disksSet = set.CreateStringSet(ignoredDisks...) } - // Bootstrap disks. storageDisks := make([]StorageAPI, len(disks)) for index, disk := range disks { // Check if disk is ignored. - if isDiskFound(disk, ignoredDisks) { + if disksSet.Contains(disk) { storageDisks[index] = nil continue } diff --git a/cmd/xl-v1_test.go b/cmd/xl-v1_test.go index e31aa803a..efb302406 100644 --- a/cmd/xl-v1_test.go +++ b/cmd/xl-v1_test.go @@ -130,8 +130,21 @@ func TestNewXL(t *testing.T) { erasureDisks = append(erasureDisks, disk) defer removeAll(disk) } + + // No disks input. + _, err := newXLObjects(nil, nil) + if err != errInvalidArgument { + t.Fatalf("Unable to initialize erasure, %s", err) + } + // Initializes all erasure disks - _, err := newXLObjects(erasureDisks, nil) + _, err = newXLObjects(erasureDisks, nil) + if err != nil { + t.Fatalf("Unable to initialize erasure, %s", err) + } + + // Initializes all erasure disks, ignoring first two. + _, err = newXLObjects(erasureDisks, erasureDisks[:2]) if err != nil { t.Fatalf("Unable to initialize erasure, %s", err) }