Skip to content

feat: allow member agent use OS's root certificate authority (follow up) #365

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ryanzhang-oss merged 5 commits into from
May 30, 2023
Merged

feat: allow member agent use OS's root certificate authority (follow up) #365

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
edbbd9e
change parameter name of buildHubConfig: useCAAuth => useCertificateAuth
mingqishao May 22, 2023
4c5bfda
return error if both of CA_BUNDLE and HUB_CERTIFICATE_AUTHORITY be se…
mingqishao May 23, 2023
52b9aaa
use os.LookupEnv to get the CA_BUNDLE and HUB_CERTIFICATE_AUTHORITY a…
mingqishao May 23, 2023
f1f0199
fix a bug: not return error after CA_BUNDLE and HUB_CERTIFICATE_AUTHO…
mingqishao May 24, 2023
84e6d51
use 'else if' to make sure one of caBundle and caData be set in hubco…
mingqishao May 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 23 additions & 6 deletions cmd/memberagent/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 == "" {
Expand Down Expand Up @@ -160,9 +160,27 @@ func buildHubConfig(hubURL string, useCAAuth bool, tlsClientInsecure bool) (*res

hubConfig.TLSClientConfig.Insecure = tlsClientInsecure
if !tlsClientInsecure {
hubConfig.TLSClientConfig.CAFile = os.Getenv("CA_BUNDLE")
hubCA := os.Getenv("HUB_CERTIFICATE_AUTHORITY")
if hubCA != "" {
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")
Copy link
Contributor

@ryanzhang-oss ryanzhang-oss May 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not return the err here? I am not sure why we just log this as user can't see the error log. I don't think an empty env is valid. I assume that not setting it means using os default but setting it as "" ambiguous.

Copy link
Contributor Author

@mingqishao mingqishao May 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is a copy/paste bug. I fixed that and added unit test in this commit.

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")
klog.ErrorS(err, "failed to validate system variables")
return nil, err
}

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")
Expand All @@ -171,7 +189,6 @@ func buildHubConfig(hubURL string, useCAAuth bool, tlsClientInsecure bool) (*res
hubConfig.TLSClientConfig.CAData = caData
}
}

return hubConfig, nil
}

Expand Down
51 changes: 37 additions & 14 deletions cmd/memberagent/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -52,6 +60,35 @@ 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("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=")
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)
Expand All @@ -74,20 +111,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)
Expand Down