From edbbd9e52410c8bb63a27eebbefb9882d76d9349 Mon Sep 17 00:00:00 2001 From: Mitch Shao Date: Mon, 22 May 2023 14:48:01 -0700 Subject: [PATCH 1/5] change parameter name of buildHubConfig: useCAAuth => useCertificateAuth --- cmd/memberagent/main.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/memberagent/main.go b/cmd/memberagent/main.go index 9db2ced9d..269893578 100644 --- a/cmd/memberagent/main.go +++ b/cmd/memberagent/main.go @@ -116,11 +116,11 @@ func main() { } } -func buildHubConfig(hubURL string, useCAAuth bool, tlsClientInsecure bool) (*rest.Config, error) { +func buildHubConfig(hubURL string, useCertificateAuth bool, tlsClientInsecure bool) (*rest.Config, error) { var hubConfig = &rest.Config{ Host: hubURL, } - if useCAAuth { + if useCertificateAuth { keyFilePath := os.Getenv("IDENTITY_KEY") certFilePath := os.Getenv("IDENTITY_CERT") if keyFilePath == "" { From 4c5bfda7e3ffbd7e6aba7b40c37eae3f7ebec046 Mon Sep 17 00:00:00 2001 From: Mitch Shao Date: Tue, 23 May 2023 11:45:05 -0700 Subject: [PATCH 2/5] return error if both of CA_BUNDLE and HUB_CERTIFICATE_AUTHORITY be set at same time --- cmd/memberagent/main.go | 9 ++++++++- cmd/memberagent/main_test.go | 36 ++++++++++++++++++++++-------------- 2 files changed, 30 insertions(+), 15 deletions(-) diff --git a/cmd/memberagent/main.go b/cmd/memberagent/main.go index 269893578..af201502d 100644 --- a/cmd/memberagent/main.go +++ b/cmd/memberagent/main.go @@ -160,8 +160,15 @@ func buildHubConfig(hubURL string, useCertificateAuth bool, tlsClientInsecure bo hubConfig.TLSClientConfig.Insecure = tlsClientInsecure if !tlsClientInsecure { - hubConfig.TLSClientConfig.CAFile = os.Getenv("CA_BUNDLE") + caBundle := os.Getenv("CA_BUNDLE") hubCA := os.Getenv("HUB_CERTIFICATE_AUTHORITY") + if caBundle != "" && hubCA != "" { + err := errors.New("environment variables CA_BUNDLE and HUB_CERTIFICATE_AUTHORITY should not be set at same time") + klog.ErrorS(err, "failed to validate system variables") + return nil, err + } + + hubConfig.TLSClientConfig.CAFile = os.Getenv("CA_BUNDLE") if hubCA != "" { caData, err := base64.StdEncoding.DecodeString(hubCA) if err != nil { diff --git a/cmd/memberagent/main_test.go b/cmd/memberagent/main_test.go index c1a47ac06..2a4058ec8 100644 --- a/cmd/memberagent/main_test.go +++ b/cmd/memberagent/main_test.go @@ -52,6 +52,28 @@ func Test_buildHubConfig(t *testing.T) { }, }, *config) }) + t.Run("use CA data - success", func(t *testing.T) { + t.Setenv("CONFIG_PATH", "./testdata/token") + t.Setenv("HUB_CERTIFICATE_AUTHORITY", "dGhpcyBpcyBhIGZha2UgY2E=") + config, err := buildHubConfig("https://hub.domain.com", false, false) + assert.NotNil(t, config) + assert.Nil(t, err) + assert.Equal(t, rest.Config{ + Host: "https://hub.domain.com", + BearerTokenFile: "./testdata/token", + TLSClientConfig: rest.TLSClientConfig{ + CAData: []byte("this is a fake ca"), + }, + }, *config) + }) + t.Run("both of CA bundle and CA data present - error", func(t *testing.T) { + t.Setenv("CONFIG_PATH", "./testdata/token") + t.Setenv("HUB_CERTIFICATE_AUTHORITY", "dGhpcyBpcyBhIGZha2UgY2E=") + t.Setenv("CA_BUNDLE", "/path/to/ca/bundle") + config, err := buildHubConfig("https://hub.domain.com", false, false) + assert.Nil(t, config) + assert.NotNil(t, err) + }) t.Run("use token auth, no toke path - error", func(t *testing.T) { t.Setenv("CONFIG_PATH", "") config, err := buildHubConfig("https://hub.domain.com", false, false) @@ -74,20 +96,6 @@ func Test_buildHubConfig(t *testing.T) { BearerTokenFile: "./testdata/token", }, *config) }) - t.Run("use hub ca data - success", func(t *testing.T) { - t.Setenv("CONFIG_PATH", "./testdata/token") - t.Setenv("HUB_CERTIFICATE_AUTHORITY", "dGhpcyBpcyBhIGZha2UgY2E=") - config, err := buildHubConfig("https://hub.domain.com", false, false) - assert.NotNil(t, config) - assert.Nil(t, err) - assert.Equal(t, rest.Config{ - Host: "https://hub.domain.com", - BearerTokenFile: "./testdata/token", - TLSClientConfig: rest.TLSClientConfig{ - CAData: []byte("this is a fake ca"), - }, - }, *config) - }) t.Run("No CA bundle, no Hub CA, not insecure - success", func(t *testing.T) { t.Setenv("CONFIG_PATH", "./testdata/token") config, err := buildHubConfig("https://hub.domain.com", false, false) From 52b9aaa882e5156dd0dd6ef7326492be4f8677c2 Mon Sep 17 00:00:00 2001 From: Mitch Shao Date: Tue, 23 May 2023 16:54:38 -0700 Subject: [PATCH 3/5] use os.LookupEnv to get the CA_BUNDLE and HUB_CERTIFICATE_AUTHORITY alternative system environment --- cmd/memberagent/main.go | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/cmd/memberagent/main.go b/cmd/memberagent/main.go index af201502d..fec161304 100644 --- a/cmd/memberagent/main.go +++ b/cmd/memberagent/main.go @@ -160,15 +160,23 @@ func buildHubConfig(hubURL string, useCertificateAuth bool, tlsClientInsecure bo hubConfig.TLSClientConfig.Insecure = tlsClientInsecure if !tlsClientInsecure { - caBundle := os.Getenv("CA_BUNDLE") - hubCA := os.Getenv("HUB_CERTIFICATE_AUTHORITY") + caBundle, ok := os.LookupEnv("CA_BUNDLE") + if ok && caBundle == "" { + err := errors.New("environment variable CA_BUNDLE should not be empty") + klog.ErrorS(err, "failed to validate system variables") + } + hubCA, ok := os.LookupEnv("HUB_CERTIFICATE_AUTHORITY") + if ok && hubCA == "" { + err := errors.New("environment variable HUB_CERTIFICATE_AUTHORITY should not be empty") + klog.ErrorS(err, "failed to validate system variables") + } if caBundle != "" && hubCA != "" { err := errors.New("environment variables CA_BUNDLE and HUB_CERTIFICATE_AUTHORITY should not be set at same time") klog.ErrorS(err, "failed to validate system variables") return nil, err } - hubConfig.TLSClientConfig.CAFile = os.Getenv("CA_BUNDLE") + hubConfig.TLSClientConfig.CAFile = caBundle if hubCA != "" { caData, err := base64.StdEncoding.DecodeString(hubCA) if err != nil { From f1f019903055963bbd77be13303157da388d600f Mon Sep 17 00:00:00 2001 From: Mitch Shao Date: Wed, 24 May 2023 13:04:41 -0700 Subject: [PATCH 4/5] fix a bug: not return error after CA_BUNDLE and HUB_CERTIFICATE_AUTHORITY validation --- cmd/memberagent/main.go | 2 ++ cmd/memberagent/main_test.go | 15 +++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/cmd/memberagent/main.go b/cmd/memberagent/main.go index fec161304..bb01a9a57 100644 --- a/cmd/memberagent/main.go +++ b/cmd/memberagent/main.go @@ -164,11 +164,13 @@ func buildHubConfig(hubURL string, useCertificateAuth bool, tlsClientInsecure bo if ok && caBundle == "" { err := errors.New("environment variable CA_BUNDLE should not be empty") klog.ErrorS(err, "failed to validate system variables") + return nil, err } hubCA, ok := os.LookupEnv("HUB_CERTIFICATE_AUTHORITY") if ok && hubCA == "" { err := errors.New("environment variable HUB_CERTIFICATE_AUTHORITY should not be empty") klog.ErrorS(err, "failed to validate system variables") + return nil, err } if caBundle != "" && hubCA != "" { err := errors.New("environment variables CA_BUNDLE and HUB_CERTIFICATE_AUTHORITY should not be set at same time") diff --git a/cmd/memberagent/main_test.go b/cmd/memberagent/main_test.go index 2a4058ec8..6ffed013d 100644 --- a/cmd/memberagent/main_test.go +++ b/cmd/memberagent/main_test.go @@ -36,6 +36,14 @@ func Test_buildHubConfig(t *testing.T) { }, }, *config) }) + t.Run("empty CA bundle - error", func(t *testing.T) { + t.Setenv("IDENTITY_KEY", "/path/to/key") + t.Setenv("IDENTITY_CERT", "/path/to/cert") + t.Setenv("CA_BUNDLE", "") + config, err := buildHubConfig("https://hub.domain.com", true, false) + assert.Nil(t, config) + assert.NotNil(t, err) + }) t.Run("use CA bundle - success", func(t *testing.T) { t.Setenv("IDENTITY_KEY", "/path/to/key") t.Setenv("IDENTITY_CERT", "/path/to/cert") @@ -66,6 +74,13 @@ func Test_buildHubConfig(t *testing.T) { }, }, *config) }) + t.Run("empty CA data - error", func(t *testing.T) { + t.Setenv("CONFIG_PATH", "./testdata/token") + t.Setenv("HUB_CERTIFICATE_AUTHORITY", "") + config, err := buildHubConfig("https://hub.domain.com", false, false) + assert.Nil(t, config) + assert.NotNil(t, err) + }) t.Run("both of CA bundle and CA data present - error", func(t *testing.T) { t.Setenv("CONFIG_PATH", "./testdata/token") t.Setenv("HUB_CERTIFICATE_AUTHORITY", "dGhpcyBpcyBhIGZha2UgY2E=") From 84e6d516bad972a4345acfc5894a9c2bab42998d Mon Sep 17 00:00:00 2001 From: Mitch Shao Date: Wed, 24 May 2023 13:12:54 -0700 Subject: [PATCH 5/5] use 'else if' to make sure one of caBundle and caData be set in hubconfig --- cmd/memberagent/main.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/memberagent/main.go b/cmd/memberagent/main.go index bb01a9a57..362e335df 100644 --- a/cmd/memberagent/main.go +++ b/cmd/memberagent/main.go @@ -178,8 +178,9 @@ func buildHubConfig(hubURL string, useCertificateAuth bool, tlsClientInsecure bo return nil, err } - hubConfig.TLSClientConfig.CAFile = caBundle - if hubCA != "" { + if caBundle != "" { + hubConfig.TLSClientConfig.CAFile = caBundle + } else if hubCA != "" { caData, err := base64.StdEncoding.DecodeString(hubCA) if err != nil { klog.ErrorS(err, "cannot decode hub cluster certificate authority data") @@ -188,7 +189,6 @@ func buildHubConfig(hubURL string, useCertificateAuth bool, tlsClientInsecure bo hubConfig.TLSClientConfig.CAData = caData } } - return hubConfig, nil }