refactor upstreamgithub.ProviderConfig to hold more config

This commit is contained in:
Ryan Richard
2024-05-09 15:35:37 -07:00
parent 29eb3dd384
commit 7277d00e1a
11 changed files with 341 additions and 193 deletions
@@ -58,6 +58,9 @@ const (
ClientCredentialsObtained string = "ClientCredentialsObtained" //nolint:gosec // this is not a credential
GitHubConnectionValid string = "GitHubConnectionValid"
ClaimsValid string = "ClaimsValid"
defaultHost = "github.com"
defaultApiBaseURL = "https://api.github.com"
)
// UpstreamGitHubIdentityProviderICache is a thread safe cache that holds a list of validated upstream GitHub IDP configurations.
@@ -73,6 +76,7 @@ type gitHubWatcherController struct {
gitHubIdentityProviderInformer idpinformers.GitHubIdentityProviderInformer
secretInformer corev1informers.SecretInformer
clock clock.Clock
dialFunc func(network, addr string, config *tls.Config) (*tls.Conn, error)
}
// New instantiates a new controllerlib.Controller which will populate the provided UpstreamGitHubIdentityProviderICache.
@@ -85,6 +89,7 @@ func New(
log plog.Logger,
withInformer pinnipedcontroller.WithInformerOptionFunc,
clock clock.Clock,
dialFunc func(network, addr string, config *tls.Config) (*tls.Conn, error),
) controllerlib.Controller {
c := gitHubWatcherController{
namespace: namespace,
@@ -94,6 +99,7 @@ func New(
gitHubIdentityProviderInformer: gitHubIdentityProviderInformer,
secretInformer: secretInformer,
clock: clock,
dialFunc: dialFunc,
}
return controllerlib.New(
@@ -202,7 +208,7 @@ func (c *gitHubWatcherController) validateClientSecret(secretName string) (*meta
}, clientID, clientSecret, nil
}
func validateOrganizationsPolicy(organizationsSpec *v1alpha1.GitHubOrganizationsSpec) (*metav1.Condition, v1alpha1.GitHubAllowedAuthOrganizationsPolicy) {
func validateOrganizationsPolicy(organizationsSpec *v1alpha1.GitHubOrganizationsSpec) *metav1.Condition {
var policy v1alpha1.GitHubAllowedAuthOrganizationsPolicy
if organizationsSpec.Policy != nil {
policy = *organizationsSpec.Policy
@@ -217,7 +223,7 @@ func validateOrganizationsPolicy(organizationsSpec *v1alpha1.GitHubOrganizations
Status: metav1.ConditionTrue,
Reason: upstreamwatchers.ReasonSuccess,
Message: fmt.Sprintf("spec.allowAuthentication.organizations.policy (%q) is valid", policy),
}, policy
}
}
if len(organizationsSpec.Allowed) > 0 {
@@ -226,7 +232,7 @@ func validateOrganizationsPolicy(organizationsSpec *v1alpha1.GitHubOrganizations
Status: metav1.ConditionFalse,
Reason: "Invalid",
Message: "spec.allowAuthentication.organizations.policy must be 'OnlyUsersFromAllowedOrganizations' when spec.allowAuthentication.organizations.allowed has organizations listed",
}, policy
}
}
return &metav1.Condition{
@@ -234,7 +240,7 @@ func validateOrganizationsPolicy(organizationsSpec *v1alpha1.GitHubOrganizations
Status: metav1.ConditionFalse,
Reason: "Invalid",
Message: "spec.allowAuthentication.organizations.policy must be 'AllGitHubUsers' when spec.allowAuthentication.organizations.allowed is empty",
}, policy
}
}
func (c *gitHubWatcherController) validateUpstreamAndUpdateConditions(ctx controllerlib.Context, upstream *v1alpha1.GitHubIdentityProvider) (
@@ -256,7 +262,7 @@ func (c *gitHubWatcherController) validateUpstreamAndUpdateConditions(ctx contro
userAndGroupCondition, groupNameAttribute, usernameAttribute := validateUserAndGroupAttributes(upstream)
conditions = append(conditions, userAndGroupCondition)
organizationPolicyCondition, policy := validateOrganizationsPolicy(&upstream.Spec.AllowAuthentication.Organizations)
organizationPolicyCondition := validateOrganizationsPolicy(&upstream.Spec.AllowAuthentication.Organizations)
conditions = append(conditions, organizationPolicyCondition)
hostCondition, hostPort := validateHost(upstream.Spec.GitHubAPI)
@@ -295,22 +301,36 @@ func (c *gitHubWatcherController) validateUpstreamAndUpdateConditions(ctx contro
upstreamgithub.ProviderConfig{
Name: upstream.Name,
ResourceUID: upstream.UID,
Host: hostURL,
APIBaseURL: apiBaseUrl(*upstream.Spec.GitHubAPI.Host, hostURL),
GroupNameAttribute: groupNameAttribute,
UsernameAttribute: usernameAttribute,
OAuth2Config: &oauth2.Config{
ClientID: clientID,
ClientSecret: clientSecret,
// See https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/authorizing-oauth-apps
Endpoint: oauth2.Endpoint{
AuthURL: fmt.Sprintf("%s/login/oauth/authorize", hostURL),
DeviceAuthURL: "", // we do not use device code flow
TokenURL: fmt.Sprintf("%s/login/oauth/access_token", hostURL),
AuthStyle: oauth2.AuthStyleInParams,
},
RedirectURL: "", // this will be different for each FederationDomain, so we do not set it here
Scopes: []string{"read:user", "read:org"},
},
AllowedOrganizations: upstream.Spec.AllowAuthentication.Organizations.Allowed,
OrganizationLoginPolicy: policy,
AuthorizationURL: fmt.Sprintf("%s/login/oauth/authorize", hostURL),
HttpClient: httpClient,
AllowedOrganizations: upstream.Spec.AllowAuthentication.Organizations.Allowed,
HttpClient: httpClient,
},
)
return provider, k8sutilerrors.NewAggregate(applicationErrors)
}
func apiBaseUrl(upstreamSpecHost string, hostURL string) string {
if upstreamSpecHost != defaultHost {
return fmt.Sprintf("%s/api/v3", hostURL)
}
return defaultApiBaseURL
}
func validateHost(gitHubAPIConfig v1alpha1.GitHubAPIConfig) (*metav1.Condition, *endpointaddr.HostPort) {
buildInvalidHost := func(host, reason string) *metav1.Condition {
return &metav1.Condition{
@@ -376,7 +396,7 @@ func (c *gitHubWatcherController) validateGitHubConnection(
}, "", nil, nil
}
conn, tlsDialErr := tls.Dial("tcp", hostPort.Endpoint(), ptls.Default(certPool))
conn, tlsDialErr := c.dialFunc("tcp", hostPort.Endpoint(), ptls.Default(certPool))
if tlsDialErr != nil {
return &metav1.Condition{
Type: GitHubConnectionValid,
@@ -6,6 +6,7 @@ package githubupstreamwatcher
import (
"bytes"
"context"
"crypto/tls"
"encoding/base64"
"fmt"
"net"
@@ -368,13 +369,17 @@ func TestController(t *testing.T) {
name string
githubIdentityProviders []runtime.Object
secrets []runtime.Object
mockDialer func(network, addr string, config *tls.Config) (*tls.Conn, error)
wantErr string
wantLogs []string
wantResultingCache []*upstreamgithub.ProviderConfig
wantResultingUpstreams []v1alpha1.GitHubIdentityProvider
}{
{
name: "no GitHubIdentityProviders",
name: "no GitHubIdentityProviders",
wantResultingCache: []*upstreamgithub.ProviderConfig{},
wantResultingUpstreams: []v1alpha1.GitHubIdentityProvider{},
wantLogs: []string{},
},
{
name: "happy path with all fields",
@@ -386,17 +391,23 @@ func TestController(t *testing.T) {
{
Name: "some-idp-name",
ResourceUID: "some-resource-uid",
Host: fmt.Sprintf("https://%s", *validFilledOutIDP.Spec.GitHubAPI.Host),
APIBaseURL: fmt.Sprintf("https://%s/api/v3", *validFilledOutIDP.Spec.GitHubAPI.Host),
UsernameAttribute: "id",
GroupNameAttribute: "name",
OAuth2Config: &oauth2.Config{
ClientID: "some-client-id",
ClientSecret: "some-client-secret",
Endpoint: oauth2.Endpoint{
AuthURL: fmt.Sprintf("https://%s/login/oauth/authorize", *validFilledOutIDP.Spec.GitHubAPI.Host),
DeviceAuthURL: "", // not used
TokenURL: fmt.Sprintf("https://%s/login/oauth/access_token", *validFilledOutIDP.Spec.GitHubAPI.Host),
AuthStyle: oauth2.AuthStyleInParams,
},
RedirectURL: "", // not used
Scopes: []string{"read:user", "read:org"},
},
AllowedOrganizations: []string{"organization1", "org2"},
OrganizationLoginPolicy: "OnlyUsersFromAllowedOrganizations",
AuthorizationURL: fmt.Sprintf("https://%s/login/oauth/authorize", *validFilledOutIDP.Spec.GitHubAPI.Host),
HttpClient: nil, // let the test runner populate this for us
AllowedOrganizations: []string{"organization1", "org2"},
HttpClient: nil, // let the test runner populate this for us
},
},
wantResultingUpstreams: []v1alpha1.GitHubIdentityProvider{
@@ -436,16 +447,22 @@ func TestController(t *testing.T) {
{
Name: "minimal-idp-name",
ResourceUID: "minimal-uid",
Host: fmt.Sprintf("https://%s", goodServerDomain),
APIBaseURL: fmt.Sprintf("https://%s/api/v3", *validFilledOutIDP.Spec.GitHubAPI.Host),
UsernameAttribute: "login",
GroupNameAttribute: "slug",
OAuth2Config: &oauth2.Config{
ClientID: "some-client-id",
ClientSecret: "some-client-secret",
Endpoint: oauth2.Endpoint{
AuthURL: fmt.Sprintf("https://%s/login/oauth/authorize", *validFilledOutIDP.Spec.GitHubAPI.Host),
DeviceAuthURL: "", // not used
TokenURL: fmt.Sprintf("https://%s/login/oauth/access_token", *validFilledOutIDP.Spec.GitHubAPI.Host),
AuthStyle: oauth2.AuthStyleInParams,
},
RedirectURL: "", // not used
Scopes: []string{"read:user", "read:org"},
},
OrganizationLoginPolicy: "AllGitHubUsers",
AuthorizationURL: fmt.Sprintf("https://%s/login/oauth/authorize", goodServerDomain),
HttpClient: nil, // let the test runner populate this for us
HttpClient: nil, // let the test runner populate this for us
},
},
wantResultingUpstreams: []v1alpha1.GitHubIdentityProvider{
@@ -475,6 +492,80 @@ func TestController(t *testing.T) {
buildLogForUpdatingPhase("minimal-idp-name", "Ready"),
},
},
{
name: "happy path using github.com",
secrets: []runtime.Object{goodSecret},
githubIdentityProviders: []runtime.Object{
func() runtime.Object {
githubIDP := validMinimalIDP.DeepCopy()
githubIDP.Spec.GitHubAPI.Host = ptr.To("github.com")
// don't change the CA because we are not really going to dial github.com in this test
return githubIDP
}(),
},
mockDialer: func(network, addr string, config *tls.Config) (*tls.Conn, error) {
require.Equal(t, "github.com:443", addr)
// don't actually dial github.com to avoid making external network calls in unit test
certPool, _, err := pinnipedcontroller.BuildCertPoolIDP(validMinimalIDP.Spec.GitHubAPI.TLS)
require.NoError(t, err)
configClone := config.Clone()
configClone.RootCAs = certPool
return tls.Dial(network, goodServerDomain, configClone)
},
wantResultingCache: []*upstreamgithub.ProviderConfig{
{
Name: "minimal-idp-name",
ResourceUID: "minimal-uid",
APIBaseURL: "https://api.github.com",
UsernameAttribute: "login",
GroupNameAttribute: "slug",
OAuth2Config: &oauth2.Config{
ClientID: "some-client-id",
ClientSecret: "some-client-secret",
Endpoint: oauth2.Endpoint{
AuthURL: "https://github.com:443/login/oauth/authorize",
DeviceAuthURL: "", // not used
TokenURL: "https://github.com:443/login/oauth/access_token",
AuthStyle: oauth2.AuthStyleInParams,
},
RedirectURL: "", // not used
Scopes: []string{"read:user", "read:org"},
},
HttpClient: nil, // let the test runner populate this for us
},
},
wantResultingUpstreams: []v1alpha1.GitHubIdentityProvider{
{
ObjectMeta: validMinimalIDP.ObjectMeta,
Spec: func() v1alpha1.GitHubIdentityProviderSpec {
githubIDP := validMinimalIDP.DeepCopy()
githubIDP.Spec.GitHubAPI.Host = ptr.To("github.com")
// don't change the CA because we are not really going to dial github.com in this test
return githubIDP.Spec
}(),
Status: v1alpha1.GitHubIdentityProviderStatus{
Phase: v1alpha1.GitHubPhaseReady,
Conditions: []metav1.Condition{
buildClaimsValidatedTrue(t),
buildClientCredentialsObtainedTrue(t, validMinimalIDP.Spec.Client.SecretName),
buildGitHubConnectionValidTrue(t, "github.com:443"),
buildHostValidTrue(t, "github.com"),
buildOrganizationsPolicyValidTrue(t, *validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy),
buildTLSConfigurationValidTrue(t),
},
},
},
},
wantLogs: []string{
buildLogForUpdatingClientCredentialsObtained("minimal-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validMinimalIDP.Spec.Client.SecretName)),
buildLogForUpdatingClaimsValidTrue("minimal-idp-name"),
buildLogForUpdatingOrganizationPolicyValid("minimal-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validMinimalIDP.Spec.AllowAuthentication.Organizations.Policy))),
buildLogForUpdatingHostValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, "github.com"),
buildLogForUpdatingTLSConfigurationValid("minimal-idp-name", "True", "Success", "spec.githubAPI.tls.certificateAuthorityData is valid"),
buildLogForUpdatingGitHubConnectionValid("minimal-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is reachable and TLS verification succeeds`, "github.com:443"),
buildLogForUpdatingPhase("minimal-idp-name", "Ready"),
},
},
{
name: "happy path with IPv6",
secrets: []runtime.Object{goodSecret},
@@ -492,16 +583,22 @@ func TestController(t *testing.T) {
{
Name: "minimal-idp-name",
ResourceUID: "minimal-uid",
Host: fmt.Sprintf("https://%s", goodServerIPv6Domain),
APIBaseURL: fmt.Sprintf("https://%s/api/v3", goodServerIPv6Domain),
UsernameAttribute: "login",
GroupNameAttribute: "slug",
OAuth2Config: &oauth2.Config{
ClientID: "some-client-id",
ClientSecret: "some-client-secret",
Endpoint: oauth2.Endpoint{
AuthURL: fmt.Sprintf("https://%s/login/oauth/authorize", goodServerIPv6Domain),
DeviceAuthURL: "", // not used
TokenURL: fmt.Sprintf("https://%s/login/oauth/access_token", goodServerIPv6Domain),
AuthStyle: oauth2.AuthStyleInParams,
},
RedirectURL: "", // not used
Scopes: []string{"read:user", "read:org"},
},
OrganizationLoginPolicy: "AllGitHubUsers",
AuthorizationURL: fmt.Sprintf("https://%s/login/oauth/authorize", goodServerIPv6Domain),
HttpClient: nil, // let the test runner populate this for us
HttpClient: nil, // let the test runner populate this for us
},
},
wantResultingUpstreams: []v1alpha1.GitHubIdentityProvider{
@@ -573,32 +670,44 @@ func TestController(t *testing.T) {
{
Name: "some-idp-name",
ResourceUID: "some-resource-uid",
Host: fmt.Sprintf("https://%s", *validFilledOutIDP.Spec.GitHubAPI.Host),
APIBaseURL: fmt.Sprintf("https://%s/api/v3", *validFilledOutIDP.Spec.GitHubAPI.Host),
UsernameAttribute: "id",
GroupNameAttribute: "name",
OAuth2Config: &oauth2.Config{
ClientID: "some-client-id",
ClientSecret: "some-client-secret",
Endpoint: oauth2.Endpoint{
AuthURL: fmt.Sprintf("https://%s/login/oauth/authorize", *validFilledOutIDP.Spec.GitHubAPI.Host),
DeviceAuthURL: "", // not used
TokenURL: fmt.Sprintf("https://%s/login/oauth/access_token", *validFilledOutIDP.Spec.GitHubAPI.Host),
AuthStyle: oauth2.AuthStyleInParams,
},
RedirectURL: "", // not used
Scopes: []string{"read:user", "read:org"},
},
AllowedOrganizations: []string{"organization1", "org2"},
OrganizationLoginPolicy: "OnlyUsersFromAllowedOrganizations",
AuthorizationURL: fmt.Sprintf("https://%s/login/oauth/authorize", *validFilledOutIDP.Spec.GitHubAPI.Host),
HttpClient: nil, // let the test runner populate this for us
AllowedOrganizations: []string{"organization1", "org2"},
HttpClient: nil, // let the test runner populate this for us
},
{
Name: "other-idp-name",
ResourceUID: "some-resource-uid",
Host: fmt.Sprintf("https://%s", *validFilledOutIDP.Spec.GitHubAPI.Host),
APIBaseURL: fmt.Sprintf("https://%s/api/v3", *validFilledOutIDP.Spec.GitHubAPI.Host),
UsernameAttribute: "login:id",
GroupNameAttribute: "name",
OAuth2Config: &oauth2.Config{
ClientID: "other-client-id",
ClientSecret: "other-client-secret",
Endpoint: oauth2.Endpoint{
AuthURL: fmt.Sprintf("https://%s/login/oauth/authorize", *validFilledOutIDP.Spec.GitHubAPI.Host),
DeviceAuthURL: "", // not used
TokenURL: fmt.Sprintf("https://%s/login/oauth/access_token", *validFilledOutIDP.Spec.GitHubAPI.Host),
AuthStyle: oauth2.AuthStyleInParams,
},
RedirectURL: "", // not used
Scopes: []string{"read:user", "read:org"},
},
AllowedOrganizations: []string{"organization1", "org2"},
OrganizationLoginPolicy: "OnlyUsersFromAllowedOrganizations",
AuthorizationURL: fmt.Sprintf("https://%s/login/oauth/authorize", *validFilledOutIDP.Spec.GitHubAPI.Host),
HttpClient: nil, // let the test runner populate this for us
AllowedOrganizations: []string{"organization1", "org2"},
HttpClient: nil, // let the test runner populate this for us
},
},
wantResultingUpstreams: []v1alpha1.GitHubIdentityProvider{
@@ -1727,6 +1836,11 @@ func TestController(t *testing.T) {
gitHubIdentityProviderInformer := supervisorInformers.IDP().V1alpha1().GitHubIdentityProviders()
dialer := tt.mockDialer
if dialer == nil {
dialer = tls.Dial
}
controller := New(
namespace,
cache,
@@ -1736,6 +1850,7 @@ func TestController(t *testing.T) {
logger,
controllerlib.WithInformer,
frozenClock,
dialer,
)
ctx, cancel := context.WithCancel(context.Background())
@@ -1759,25 +1874,22 @@ func TestController(t *testing.T) {
require.Equal(t, len(tt.wantResultingCache), len(actualIDPList))
for i := 0; i < len(tt.wantResultingCache); i++ {
// Do not expect any particular order in the cache
var actualIDP *upstreamgithub.Provider
var actualProvider *upstreamgithub.Provider
for _, possibleIDP := range actualIDPList {
if possibleIDP.GetName() == tt.wantResultingCache[i].Name {
var ok bool
actualIDP, ok = possibleIDP.(*upstreamgithub.Provider)
actualProvider, ok = possibleIDP.(*upstreamgithub.Provider)
require.True(t, ok)
break
}
}
require.Equal(t, tt.wantResultingCache[i].Name, actualIDP.GetName())
require.Equal(t, tt.wantResultingCache[i].ResourceUID, actualIDP.GetResourceUID())
require.Equal(t, tt.wantResultingCache[i].Host, actualIDP.GetHost())
require.Equal(t, tt.wantResultingCache[i].OAuth2Config.ClientID, actualIDP.GetClientID())
require.Equal(t, tt.wantResultingCache[i].GroupNameAttribute, actualIDP.GetGroupNameAttribute())
require.Equal(t, tt.wantResultingCache[i].UsernameAttribute, actualIDP.GetUsernameAttribute())
require.Equal(t, tt.wantResultingCache[i].AllowedOrganizations, actualIDP.GetAllowedOrganizations())
require.Equal(t, tt.wantResultingCache[i].OrganizationLoginPolicy, actualIDP.GetOrganizationLoginPolicy())
require.Equal(t, tt.wantResultingCache[i].AuthorizationURL, actualIDP.GetAuthorizationURL())
require.Equal(t, tt.wantResultingCache[i].Name, actualProvider.GetName())
require.Equal(t, tt.wantResultingCache[i].ResourceUID, actualProvider.GetResourceUID())
require.Equal(t, tt.wantResultingCache[i].OAuth2Config.ClientID, actualProvider.GetClientID())
require.Equal(t, tt.wantResultingCache[i].GroupNameAttribute, actualProvider.GetGroupNameAttribute())
require.Equal(t, tt.wantResultingCache[i].UsernameAttribute, actualProvider.GetUsernameAttribute())
require.Equal(t, tt.wantResultingCache[i].AllowedOrganizations, actualProvider.GetAllowedOrganizations())
require.GreaterOrEqual(t, len(tt.githubIdentityProviders), i+1, "there must be at least as many input identity providers as items in the cache")
githubIDP, ok := tt.githubIdentityProviders[i].(*v1alpha1.GitHubIdentityProvider)
@@ -1785,8 +1897,9 @@ func TestController(t *testing.T) {
certPool, _, err := pinnipedcontroller.BuildCertPoolIDP(githubIDP.Spec.GitHubAPI.TLS)
require.NoError(t, err)
compareTLSClientConfigWithinHttpClients(t, phttp.Default(certPool), actualIDP.GetHttpClient())
require.Equal(t, tt.wantResultingCache[i].OAuth2Config, actualIDP.GetOAuth2Config())
compareTLSClientConfigWithinHttpClients(t, phttp.Default(certPool), actualProvider.GetConfig().HttpClient)
require.Equal(t, tt.wantResultingCache[i].OAuth2Config, actualProvider.GetConfig().OAuth2Config)
require.Contains(t, tt.wantResultingCache[i].APIBaseURL, actualProvider.GetConfig().APIBaseURL)
}
// Verify the status conditions as reported in Kubernetes
@@ -2120,6 +2233,7 @@ func TestController_OnlyWantActions(t *testing.T) {
logger,
controllerlib.WithInformer,
frozenClock,
tls.Dial,
)
ctx, cancel := context.WithCancel(context.Background())
@@ -2230,6 +2344,7 @@ func TestGitHubUpstreamWatcherControllerFilterSecret(t *testing.T) {
logger,
observableInformers.WithInformer,
clock.RealClock{},
tls.Dial,
)
unrelated := &corev1.Secret{}
@@ -2299,6 +2414,7 @@ func TestGitHubUpstreamWatcherControllerFilterGitHubIDP(t *testing.T) {
logger,
observableInformers.WithInformer,
clock.RealClock{},
tls.Dial,
)
unrelated := &v1alpha1.GitHubIdentityProvider{}