From 2b262ff5d6005b479864c52a36bb56b49dd98a24 Mon Sep 17 00:00:00 2001 From: Daniel Haus Date: Fri, 26 Nov 2021 21:27:25 +0100 Subject: [PATCH 1/2] Create setting to allow to trust the system root CAs Previously, when rootCA was set, the trusted system root CAs were ignored. Now, allow for both being able to be configured and used Signed-off-by: Daniel Haus --- connector/openshift/openshift.go | 73 +++++++++++++++------------ connector/openshift/openshift_test.go | 10 ++-- 2 files changed, 47 insertions(+), 36 deletions(-) diff --git a/connector/openshift/openshift.go b/connector/openshift/openshift.go index 469dbd96..e604dadf 100644 --- a/connector/openshift/openshift.go +++ b/connector/openshift/openshift.go @@ -28,13 +28,14 @@ const ( // Config holds configuration options for OpenShift login type Config struct { - Issuer string `json:"issuer"` - ClientID string `json:"clientID"` - ClientSecret string `json:"clientSecret"` - RedirectURI string `json:"redirectURI"` - Groups []string `json:"groups"` - InsecureCA bool `json:"insecureCA"` - RootCA string `json:"rootCA"` + Issuer string `json:"issuer"` + ClientID string `json:"clientID"` + ClientSecret string `json:"clientSecret"` + RedirectURI string `json:"redirectURI"` + Groups []string `json:"groups"` + InsecureCA bool `json:"insecureCA"` + RootCA string `json:"rootCA"` + IncludeSystemRootCAs bool `json:"includeSystemRootCAs"` } var ( @@ -43,17 +44,18 @@ var ( ) type openshiftConnector struct { - apiURL string - redirectURI string - clientID string - clientSecret string - cancel context.CancelFunc - logger log.Logger - httpClient *http.Client - oauth2Config *oauth2.Config - insecureCA bool - rootCA string - groups []string + apiURL string + redirectURI string + clientID string + clientSecret string + cancel context.CancelFunc + logger log.Logger + httpClient *http.Client + oauth2Config *oauth2.Config + insecureCA bool + rootCA string + includeSystemRootCAs bool + groups []string } type user struct { @@ -73,18 +75,19 @@ func (c *Config) Open(id string, logger log.Logger) (conn connector.Connector, e req, err := http.NewRequest(http.MethodGet, wellKnownURL, nil) openshiftConnector := openshiftConnector{ - apiURL: c.Issuer, - cancel: cancel, - clientID: c.ClientID, - clientSecret: c.ClientSecret, - insecureCA: c.InsecureCA, - logger: logger, - redirectURI: c.RedirectURI, - rootCA: c.RootCA, - groups: c.Groups, + apiURL: c.Issuer, + cancel: cancel, + clientID: c.ClientID, + clientSecret: c.ClientSecret, + insecureCA: c.InsecureCA, + logger: logger, + redirectURI: c.RedirectURI, + rootCA: c.RootCA, + includeSystemRootCAs: c.IncludeSystemRootCAs, + groups: c.Groups, } - if openshiftConnector.httpClient, err = newHTTPClient(c.InsecureCA, c.RootCA); err != nil { + if openshiftConnector.httpClient, err = newHTTPClient(c.InsecureCA, c.RootCA, c.IncludeSystemRootCAs); err != nil { cancel() return nil, fmt.Errorf("failed to create HTTP client: %v", err) } @@ -248,16 +251,24 @@ func validateAllowedGroups(userGroups, allowedGroups []string) bool { } // newHTTPClient returns a new HTTP client -func newHTTPClient(insecureCA bool, rootCA string) (*http.Client, error) { +func newHTTPClient(insecureCA bool, rootCA string, includeSystemRootCAs bool) (*http.Client, error) { tlsConfig := tls.Config{} if insecureCA { tlsConfig = tls.Config{InsecureSkipVerify: true} } else if rootCA != "" { - tlsConfig = tls.Config{RootCAs: x509.NewCertPool()} + if !includeSystemRootCAs { + tlsConfig = tls.Config{RootCAs: x509.NewCertPool()} + } else { + systemCAs, err := x509.SystemCertPool() + if err != nil { + return nil, fmt.Errorf("failed to read host CA: %w", err) + } + tlsConfig = tls.Config{RootCAs: systemCAs} + } rootCABytes, err := os.ReadFile(rootCA) if err != nil { - return nil, fmt.Errorf("failed to read root-ca: %v", err) + return nil, fmt.Errorf("failed to read root-ca: %w", err) } if !tlsConfig.RootCAs.AppendCertsFromPEM(rootCABytes) { return nil, fmt.Errorf("no certs found in root CA file %q", rootCA) diff --git a/connector/openshift/openshift_test.go b/connector/openshift/openshift_test.go index 6280b831..d119fe80 100644 --- a/connector/openshift/openshift_test.go +++ b/connector/openshift/openshift_test.go @@ -70,7 +70,7 @@ func TestGetUser(t *testing.T) { _, err = http.NewRequest("GET", hostURL.String(), nil) expectNil(t, err) - h, err := newHTTPClient(true, "") + h, err := newHTTPClient(true, "", false) expectNil(t, err) @@ -128,7 +128,7 @@ func TestVerifyGroup(t *testing.T) { _, err = http.NewRequest("GET", hostURL.String(), nil) expectNil(t, err) - h, err := newHTTPClient(true, "") + h, err := newHTTPClient(true, "", false) expectNil(t, err) @@ -164,7 +164,7 @@ func TestCallbackIdentity(t *testing.T) { req, err := http.NewRequest("GET", hostURL.String(), nil) expectNil(t, err) - h, err := newHTTPClient(true, "") + h, err := newHTTPClient(true, "", false) expectNil(t, err) @@ -198,7 +198,7 @@ func TestRefreshIdentity(t *testing.T) { }) defer s.Close() - h, err := newHTTPClient(true, "") + h, err := newHTTPClient(true, "", false) expectNil(t, err) oc := openshiftConnector{apiURL: s.URL, httpClient: h, oauth2Config: &oauth2.Config{ @@ -237,7 +237,7 @@ func TestRefreshIdentityFailure(t *testing.T) { }) defer s.Close() - h, err := newHTTPClient(true, "") + h, err := newHTTPClient(true, "", false) expectNil(t, err) oc := openshiftConnector{apiURL: s.URL, httpClient: h, oauth2Config: &oauth2.Config{ From 4088d4f89701b35bfe41956311b45d50d2f21ce5 Mon Sep 17 00:00:00 2001 From: Daniel Haus Date: Tue, 12 Apr 2022 17:38:01 +0200 Subject: [PATCH 2/2] Remove external setting, enable injection of HTTP client to config. Signed-off-by: Daniel Haus --- connector/openshift/openshift.go | 104 +++++++++++++------------- connector/openshift/openshift_test.go | 10 +-- 2 files changed, 57 insertions(+), 57 deletions(-) diff --git a/connector/openshift/openshift.go b/connector/openshift/openshift.go index e604dadf..05919973 100644 --- a/connector/openshift/openshift.go +++ b/connector/openshift/openshift.go @@ -28,14 +28,13 @@ const ( // Config holds configuration options for OpenShift login type Config struct { - Issuer string `json:"issuer"` - ClientID string `json:"clientID"` - ClientSecret string `json:"clientSecret"` - RedirectURI string `json:"redirectURI"` - Groups []string `json:"groups"` - InsecureCA bool `json:"insecureCA"` - RootCA string `json:"rootCA"` - IncludeSystemRootCAs bool `json:"includeSystemRootCAs"` + Issuer string `json:"issuer"` + ClientID string `json:"clientID"` + ClientSecret string `json:"clientSecret"` + RedirectURI string `json:"redirectURI"` + Groups []string `json:"groups"` + InsecureCA bool `json:"insecureCA"` + RootCA string `json:"rootCA"` } var ( @@ -44,18 +43,17 @@ var ( ) type openshiftConnector struct { - apiURL string - redirectURI string - clientID string - clientSecret string - cancel context.CancelFunc - logger log.Logger - httpClient *http.Client - oauth2Config *oauth2.Config - insecureCA bool - rootCA string - includeSystemRootCAs bool - groups []string + apiURL string + redirectURI string + clientID string + clientSecret string + cancel context.CancelFunc + logger log.Logger + httpClient *http.Client + oauth2Config *oauth2.Config + insecureCA bool + rootCA string + groups []string } type user struct { @@ -69,27 +67,34 @@ type user struct { // Open returns a connector which can be used to login users through an upstream // OpenShift OAuth2 provider. func (c *Config) Open(id string, logger log.Logger) (conn connector.Connector, err error) { + httpClient, err := newHTTPClient(c.InsecureCA, c.RootCA) + if err != nil { + return nil, fmt.Errorf("failed to create HTTP client: %w", err) + } + + return c.OpenWithHTTPClient(id, logger, httpClient) +} + +// OpenWithHTTPClient returns a connector which can be used to login users through an upstream +// OpenShift OAuth2 provider. It provides the ability to inject a http.Client. +func (c *Config) OpenWithHTTPClient(id string, logger log.Logger, + httpClient *http.Client) (conn connector.Connector, err error) { ctx, cancel := context.WithCancel(context.Background()) wellKnownURL := strings.TrimSuffix(c.Issuer, "/") + wellKnownURLPath req, err := http.NewRequest(http.MethodGet, wellKnownURL, nil) openshiftConnector := openshiftConnector{ - apiURL: c.Issuer, - cancel: cancel, - clientID: c.ClientID, - clientSecret: c.ClientSecret, - insecureCA: c.InsecureCA, - logger: logger, - redirectURI: c.RedirectURI, - rootCA: c.RootCA, - includeSystemRootCAs: c.IncludeSystemRootCAs, - groups: c.Groups, - } - - if openshiftConnector.httpClient, err = newHTTPClient(c.InsecureCA, c.RootCA, c.IncludeSystemRootCAs); err != nil { - cancel() - return nil, fmt.Errorf("failed to create HTTP client: %v", err) + apiURL: c.Issuer, + cancel: cancel, + clientID: c.ClientID, + clientSecret: c.ClientSecret, + insecureCA: c.InsecureCA, + logger: logger, + redirectURI: c.RedirectURI, + rootCA: c.RootCA, + groups: c.Groups, + httpClient: httpClient, } var metadata struct { @@ -100,14 +105,14 @@ func (c *Config) Open(id string, logger log.Logger) (conn connector.Connector, e resp, err := openshiftConnector.httpClient.Do(req.WithContext(ctx)) if err != nil { cancel() - return nil, fmt.Errorf("failed to query OpenShift endpoint %v", err) + return nil, fmt.Errorf("failed to query OpenShift endpoint %w", err) } defer resp.Body.Close() if err := json.NewDecoder(resp.Body).Decode(&metadata); err != nil { cancel() - return nil, fmt.Errorf("discovery through endpoint %s failed to decode body: %v", + return nil, fmt.Errorf("discovery through endpoint %s failed to decode body: %w", wellKnownURL, err) } @@ -131,7 +136,8 @@ func (c *openshiftConnector) Close() error { // LoginURL returns the URL to redirect the user to login with. func (c *openshiftConnector) LoginURL(scopes connector.Scopes, callbackURL, state string) (string, error) { if c.redirectURI != callbackURL { - return "", fmt.Errorf("expected callback URL %q did not match the URL in the config %q", callbackURL, c.redirectURI) + return "", fmt.Errorf("expected callback URL %q did not match the URL in the config %q", + callbackURL, c.redirectURI) } return c.oauth2Config.AuthCodeURL(state), nil } @@ -149,7 +155,8 @@ func (e *oauth2Error) Error() string { } // HandleCallback parses the request and returns the user's identity -func (c *openshiftConnector) HandleCallback(s connector.Scopes, r *http.Request) (identity connector.Identity, err error) { +func (c *openshiftConnector) HandleCallback(s connector.Scopes, + r *http.Request) (identity connector.Identity, err error) { q := r.URL.Query() if errType := q.Get("error"); errType != "" { return identity, &oauth2Error{errType, q.Get("error_description")} @@ -168,7 +175,8 @@ func (c *openshiftConnector) HandleCallback(s connector.Scopes, r *http.Request) return c.identity(ctx, s, token) } -func (c *openshiftConnector) Refresh(ctx context.Context, s connector.Scopes, oldID connector.Identity) (connector.Identity, error) { +func (c *openshiftConnector) Refresh(ctx context.Context, s connector.Scopes, + oldID connector.Identity) (connector.Identity, error) { var token oauth2.Token err := json.Unmarshal(oldID.ConnectorData, &token) if err != nil { @@ -180,7 +188,8 @@ func (c *openshiftConnector) Refresh(ctx context.Context, s connector.Scopes, ol return c.identity(ctx, s, &token) } -func (c *openshiftConnector) identity(ctx context.Context, s connector.Scopes, token *oauth2.Token) (identity connector.Identity, err error) { +func (c *openshiftConnector) identity(ctx context.Context, s connector.Scopes, + token *oauth2.Token) (identity connector.Identity, err error) { client := c.oauth2Config.Client(ctx, token) user, err := c.user(ctx, client) if err != nil { @@ -251,21 +260,12 @@ func validateAllowedGroups(userGroups, allowedGroups []string) bool { } // newHTTPClient returns a new HTTP client -func newHTTPClient(insecureCA bool, rootCA string, includeSystemRootCAs bool) (*http.Client, error) { +func newHTTPClient(insecureCA bool, rootCA string) (*http.Client, error) { tlsConfig := tls.Config{} - if insecureCA { tlsConfig = tls.Config{InsecureSkipVerify: true} } else if rootCA != "" { - if !includeSystemRootCAs { - tlsConfig = tls.Config{RootCAs: x509.NewCertPool()} - } else { - systemCAs, err := x509.SystemCertPool() - if err != nil { - return nil, fmt.Errorf("failed to read host CA: %w", err) - } - tlsConfig = tls.Config{RootCAs: systemCAs} - } + tlsConfig = tls.Config{RootCAs: x509.NewCertPool()} rootCABytes, err := os.ReadFile(rootCA) if err != nil { return nil, fmt.Errorf("failed to read root-ca: %w", err) diff --git a/connector/openshift/openshift_test.go b/connector/openshift/openshift_test.go index d119fe80..6280b831 100644 --- a/connector/openshift/openshift_test.go +++ b/connector/openshift/openshift_test.go @@ -70,7 +70,7 @@ func TestGetUser(t *testing.T) { _, err = http.NewRequest("GET", hostURL.String(), nil) expectNil(t, err) - h, err := newHTTPClient(true, "", false) + h, err := newHTTPClient(true, "") expectNil(t, err) @@ -128,7 +128,7 @@ func TestVerifyGroup(t *testing.T) { _, err = http.NewRequest("GET", hostURL.String(), nil) expectNil(t, err) - h, err := newHTTPClient(true, "", false) + h, err := newHTTPClient(true, "") expectNil(t, err) @@ -164,7 +164,7 @@ func TestCallbackIdentity(t *testing.T) { req, err := http.NewRequest("GET", hostURL.String(), nil) expectNil(t, err) - h, err := newHTTPClient(true, "", false) + h, err := newHTTPClient(true, "") expectNil(t, err) @@ -198,7 +198,7 @@ func TestRefreshIdentity(t *testing.T) { }) defer s.Close() - h, err := newHTTPClient(true, "", false) + h, err := newHTTPClient(true, "") expectNil(t, err) oc := openshiftConnector{apiURL: s.URL, httpClient: h, oauth2Config: &oauth2.Config{ @@ -237,7 +237,7 @@ func TestRefreshIdentityFailure(t *testing.T) { }) defer s.Close() - h, err := newHTTPClient(true, "", false) + h, err := newHTTPClient(true, "") expectNil(t, err) oc := openshiftConnector{apiURL: s.URL, httpClient: h, oauth2Config: &oauth2.Config{