DAOS-12100 control: Update admin cert key required perms (#11271)

The admin certificate key may be owned by an admin group, rather than an individual user. In practice most private keys should be owner-read-only (0400), but the admin cert may also be readable by the admin group (0440). Signed-off-by: Kris Jacque <[email protected]>
daos-stack · Jan 24, 2023 · 1a16e69 · 1a16e69
1 parent 2b21a82
commit 1a16e69
Show file tree

Hide file tree

Showing 11 changed files with 88 additions and 23 deletions.
diff --git a/src/control/security/config.go b/src/control/security/config.go
@@ -1,5 +1,5 @@
 //
-// (C) Copyright 2019-2021 Intel Corporation.
+// (C) Copyright 2019-2023 Intel Corporation.
 //
 // SPDX-License-Identifier: BSD-2-Clause-Patent
 //
@@ -11,6 +11,7 @@ import (
 	"crypto/tls"
 	"crypto/x509"
 	"fmt"
+	"io/fs"
 
 	"github.com/pkg/errors"
 )
@@ -51,6 +52,7 @@ type CertificateConfig struct {
 	PrivateKeyPath  string           `yaml:"key"`
 	tlsKeypair      *tls.Certificate `yaml:"-"`
 	caPool          *x509.CertPool   `yaml:"-"`
+	maxKeyPerms     fs.FileMode      `yaml:"-"`
 }
 
 // DefaultAgentTransportConfig provides a default transport config disabling
@@ -66,14 +68,14 @@ func DefaultAgentTransportConfig() *TransportConfig {
 			PrivateKeyPath:  defaultAgentKey,
 			tlsKeypair:      nil,
 			caPool:          nil,
+			maxKeyPerms:     MaxUserOnlyKeyPerm,
 		},
 	}
 }
 
 // DefaultClientTransportConfig provides a default transport config disabling
 // certificate usage and specifying certificates located under /etc/daos/certs.
-// As this
-// credential is meant to be used as a client credential it specifies a default
+// As this credential is meant to be used as a client credential it specifies a default
 // ServerName as well.
 func DefaultClientTransportConfig() *TransportConfig {
 	return &TransportConfig{
@@ -86,6 +88,7 @@ func DefaultClientTransportConfig() *TransportConfig {
 			PrivateKeyPath:  defaultAdminKey,
 			tlsKeypair:      nil,
 			caPool:          nil,
+			maxKeyPerms:     MaxGroupKeyPerm,
 		},
 	}
 }
@@ -103,6 +106,7 @@ func DefaultServerTransportConfig() *TransportConfig {
 			PrivateKeyPath:  defaultServerKey,
 			tlsKeypair:      nil,
 			caPool:          nil,
+			maxKeyPerms:     MaxUserOnlyKeyPerm,
 		},
 	}
 }
@@ -119,7 +123,7 @@ func (tc *TransportConfig) PreLoadCertData() error {
 		// In order to reload data use ReloadCertDatA
 		return nil
 	}
-	certificate, certPool, err := loadCertWithCustomCA(tc.CARootPath, tc.CertificatePath, tc.PrivateKeyPath)
+	certificate, certPool, err := loadCertWithCustomCA(tc.CARootPath, tc.CertificatePath, tc.PrivateKeyPath, tc.maxKeyPerms)
 	if err != nil {
 		return err
 	}

diff --git a/src/control/security/config_test.go b/src/control/security/config_test.go
@@ -1,5 +1,5 @@
 //
-// (C) Copyright 2019-2021 Intel Corporation.
+// (C) Copyright 2019-2023 Intel Corporation.
 //
 // SPDX-License-Identifier: BSD-2-Clause-Patent
 //
@@ -12,6 +12,8 @@ import (
 	"os"
 	"strings"
 	"testing"
+
+	"github.com/google/go-cmp/cmp"
 )
 
 func InsecureTC() *TransportConfig {
@@ -42,6 +44,7 @@ func ServerTC() *TransportConfig {
 			PrivateKeyPath:  "testdata/certs/server.key",
 			tlsKeypair:      nil,
 			caPool:          nil,
+			maxKeyPerms:     MaxUserOnlyKeyPerm,
 		},
 	}
 }
@@ -55,6 +58,7 @@ func AgentTC() *TransportConfig {
 			PrivateKeyPath:  "testdata/certs/agent.key",
 			tlsKeypair:      nil,
 			caPool:          nil,
+			maxKeyPerms:     MaxUserOnlyKeyPerm,
 		},
 	}
 }
@@ -66,7 +70,7 @@ func SetupTCFilePerms(t *testing.T, conf *TransportConfig) {
 	if err := os.Chmod(conf.CertificatePath, MaxCertPerm); err != nil {
 		t.Fatal(err)
 	}
-	if err := os.Chmod(conf.PrivateKeyPath, MaxKeyPerm); err != nil {
+	if err := os.Chmod(conf.PrivateKeyPath, MaxUserOnlyKeyPerm); err != nil {
 		t.Fatal(err)
 	}
 }
@@ -240,3 +244,59 @@ func TestPublicKey(t *testing.T) {
 		})
 	}
 }
+
+func TestSecurity_DefaultTransportConfigs(t *testing.T) {
+	for name, tc := range map[string]struct {
+		genTransportConfig func() *TransportConfig
+		expResult          *TransportConfig
+	}{
+		"admin": {
+			genTransportConfig: DefaultClientTransportConfig,
+			expResult: &TransportConfig{
+				CertificateConfig: CertificateConfig{
+					ServerName:      defaultServer,
+					CARootPath:      defaultCACert,
+					CertificatePath: defaultAdminCert,
+					PrivateKeyPath:  defaultAdminKey,
+					maxKeyPerms:     MaxGroupKeyPerm,
+				},
+			},
+		},
+		"agent": {
+			genTransportConfig: DefaultAgentTransportConfig,
+			expResult: &TransportConfig{
+				CertificateConfig: CertificateConfig{
+					ServerName:      defaultServer,
+					CARootPath:      defaultCACert,
+					CertificatePath: defaultAgentCert,
+					PrivateKeyPath:  defaultAgentKey,
+					maxKeyPerms:     MaxUserOnlyKeyPerm,
+				},
+			},
+		},
+		"server": {
+			genTransportConfig: DefaultServerTransportConfig,
+			expResult: &TransportConfig{
+				CertificateConfig: CertificateConfig{
+					ServerName:      defaultServer,
+					CARootPath:      defaultCACert,
+					ClientCertDir:   defaultClientCertDir,
+					CertificatePath: defaultServerCert,
+					PrivateKeyPath:  defaultServerKey,
+					maxKeyPerms:     MaxUserOnlyKeyPerm,
+				},
+			},
+		},
+	} {
+		t.Run(name, func(t *testing.T) {
+			result := tc.genTransportConfig()
+
+			if diff := cmp.Diff(tc.expResult, result, cmp.AllowUnexported(
+				TransportConfig{},
+				CertificateConfig{},
+			)); diff != "" {
+				t.Fatalf("(want-, got+)\n %s", diff)
+			}
+		})
+	}
+}
diff --git a/src/control/security/pem.go b/src/control/security/pem.go
@@ -1,5 +1,5 @@
 //
-// (C) Copyright 2019-2021 Intel Corporation.
+// (C) Copyright 2019-2023 Intel Corporation.
 //
 // SPDX-License-Identifier: BSD-2-Clause-Patent
 //
@@ -21,9 +21,10 @@ import (
 )
 
 const (
-	MaxKeyPerm  os.FileMode = 0700
-	MaxCertPerm os.FileMode = 0664
-	MaxDirPerm  os.FileMode = 0700
+	MaxUserOnlyKeyPerm os.FileMode = 0400
+	MaxGroupKeyPerm    os.FileMode = 0440
+	MaxCertPerm        os.FileMode = 0664
+	MaxDirPerm         os.FileMode = 0750
 )
 
 type badPermsError struct {
@@ -47,7 +48,7 @@ func checkMaxPermissions(filePath string, mode os.FileMode, modeMax os.FileMode)
 	return nil
 }
 
-func loadCertWithCustomCA(caRootPath, certPath, keyPath string) (*tls.Certificate, *x509.CertPool, error) {
+func loadCertWithCustomCA(caRootPath, certPath, keyPath string, maxKeyPerm os.FileMode) (*tls.Certificate, *x509.CertPool, error) {
 	caPEM, err := LoadPEMData(caRootPath, MaxCertPerm)
 	if err != nil {
 		switch {
@@ -72,7 +73,7 @@ func loadCertWithCustomCA(caRootPath, certPath, keyPath string) (*tls.Certificat
 		}
 	}
 
-	keyPEM, err := LoadPEMData(keyPath, MaxKeyPerm)
+	keyPEM, err := LoadPEMData(keyPath, maxKeyPerm)
 	if err != nil {
 		switch {
 		case os.IsNotExist(err):
@@ -149,7 +150,7 @@ func LoadCertificate(certPath string) (*x509.Certificate, error) {
 // LoadPrivateKey loads the private key specified at the given path into an
 // crypto.PrivateKey interface compliant object.
 func LoadPrivateKey(keyPath string) (crypto.PrivateKey, error) {
-	pemData, err := LoadPEMData(keyPath, MaxKeyPerm)
+	pemData, err := LoadPEMData(keyPath, MaxUserOnlyKeyPerm)
 
 	if err != nil {
 		return nil, err

diff --git a/src/control/security/pem_test.go b/src/control/security/pem_test.go
@@ -1,5 +1,5 @@
 //
-// (C) Copyright 2019-2021 Intel Corporation.
+// (C) Copyright 2019-2023 Intel Corporation.
 //
 // SPDX-License-Identifier: BSD-2-Clause-Patent
 //
@@ -62,26 +62,26 @@ func TestLoadPrivateKey(t *testing.T) {
 	malformed := "testdata/certs/bad.key"
 	toomany := "testdata/certs/toomanypem.key"
 	notkey := "testdata/certs/notkey.crt"
-	badKeyPermError := &badPermsError{badKeyPerm, MaxCertPerm, MaxKeyPerm}
+	badKeyPermError := &badPermsError{badKeyPerm, MaxCertPerm, MaxUserOnlyKeyPerm}
 	malformedError := fmt.Sprintf("%s does not contain PEM data", malformed)
 	toomanyError := "Only one key allowed per file"
 	notkeyError := "PEM Block is not a Private Key"
 
 	// Setup permissions for tests below.
-	if err := os.Chmod(goodKeyPath, MaxKeyPerm); err != nil {
+	if err := os.Chmod(goodKeyPath, MaxUserOnlyKeyPerm); err != nil {
 		t.Fatal(err)
 	}
 	// Intentionallly safe cert perm as it should be incorrect
 	if err := os.Chmod(badKeyPerm, MaxCertPerm); err != nil {
 		t.Fatal(err)
 	}
-	if err := os.Chmod(malformed, MaxKeyPerm); err != nil {
+	if err := os.Chmod(malformed, MaxUserOnlyKeyPerm); err != nil {
 		t.Fatal(err)
 	}
-	if err := os.Chmod(toomany, MaxKeyPerm); err != nil {
+	if err := os.Chmod(toomany, MaxUserOnlyKeyPerm); err != nil {
 		t.Fatal(err)
 	}
-	if err := os.Chmod(notkey, MaxKeyPerm); err != nil {
+	if err := os.Chmod(notkey, MaxUserOnlyKeyPerm); err != nil {
 		t.Fatal(err)
 	}
 
@@ -115,7 +115,7 @@ func TestLoadCertificate(t *testing.T) {
 	badPerm := "testdata/certs/badperms.crt"
 	malformed := "testdata/certs/bad.crt"
 	toomany := "testdata/certs/toomanypem.crt"
-	badError := &badPermsError{badPerm, MaxKeyPerm, MaxCertPerm}
+	badError := &badPermsError{badPerm, 0700, MaxCertPerm}
 	malformedError := fmt.Sprintf("%s does not contain PEM data", malformed)
 	toomanyError := "Only one cert allowed per file"
 
@@ -124,7 +124,7 @@ func TestLoadCertificate(t *testing.T) {
 		t.Fatal(err)
 	}
 	// Intentionally safe cert perm as it should be incorrect
-	if err := os.Chmod(badPerm, MaxKeyPerm); err != nil {
+	if err := os.Chmod(badPerm, 0700); err != nil {
 		t.Fatal(err)
 	}
 	if err := os.Chmod(malformed, MaxCertPerm); err != nil {

diff --git a/src/control/security/signature_test.go b/src/control/security/signature_test.go
@@ -1,5 +1,5 @@
 //
-// (C) Copyright 2019-2021 Intel Corporation.
+// (C) Copyright 2019-2023 Intel Corporation.
 //
 // SPDX-License-Identifier: BSD-2-Clause-Patent
 //
@@ -33,7 +33,7 @@ func SeededSigner() *TokenSigner {
 
 func SignTestSetup(t *testing.T) (rsaKey, ecdsaKey crypto.PrivateKey, source []byte) {
 	keyPath := "testdata/certs/daosCA.key"
-	if err := os.Chmod(keyPath, MaxKeyPerm); err != nil {
+	if err := os.Chmod(keyPath, MaxUserOnlyKeyPerm); err != nil {
 		t.Fatal(err)
 	}
 	rsaKey, err := LoadPrivateKey(keyPath)

diff --git a/src/control/security/testdata/certs/agent.key b/src/control/security/testdata/certs/agent.key
diff --git a/src/control/security/testdata/certs/bad.key b/src/control/security/testdata/certs/bad.key
diff --git a/src/control/security/testdata/certs/daosCA.key b/src/control/security/testdata/certs/daosCA.key
diff --git a/src/control/security/testdata/certs/notkey.crt b/src/control/security/testdata/certs/notkey.crt
diff --git a/src/control/security/testdata/certs/server.key b/src/control/security/testdata/certs/server.key
diff --git a/src/control/security/testdata/certs/toomanypem.key b/src/control/security/testdata/certs/toomanypem.key