Skip to content
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.

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

Bug 1989055: logins to the web console fail with custom oauth cert #571

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/api/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const (
OpenShiftConsoleConfigMapName = "console-config"
OpenShiftConsolePublicConfigMapName = "console-public"
ServiceCAConfigMapName = "service-ca"
DefaultIngressCertConfigMapName = "default-ingress-cert"
OAuthServingCertConfigMapName = "oauth-serving-cert"
OpenShiftConsoleDeploymentName = OpenShiftConsoleName
OpenShiftConsoleServiceName = OpenShiftConsoleName
OpenshiftConsoleRedirectServiceName = "console-redirect"
Expand Down
4 changes: 2 additions & 2 deletions pkg/console/controllers/healthcheck/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ func NewHealthCheckController(

return factory.New().
WithFilteredEventsInformers( // service
util.NamesFilter(api.TrustedCAConfigMapName, api.DefaultIngressCertConfigMapName),
util.NamesFilter(api.TrustedCAConfigMapName, api.OAuthServingCertConfigMapName),
configMapInformer.Informer(),
).WithFilteredEventsInformers( // route
util.NamesFilter(api.OpenShiftConsoleRouteName, api.OpenshiftConsoleCustomRouteName),
Expand Down Expand Up @@ -164,7 +164,7 @@ func (c *HealthCheckController) getCA(ctx context.Context, tls *routev1.TLSConfi
}
}

for _, cmName := range []string{api.TrustedCAConfigMapName, api.DefaultIngressCertConfigMapName} {
for _, cmName := range []string{api.TrustedCAConfigMapName, api.OAuthServingCertConfigMapName} {
cm, err := c.configMapClient.ConfigMaps(api.OpenShiftConsoleNamespace).Get(ctx, cmName, metav1.GetOptions{})
if err != nil {
klog.V(4).Infof("failed to GET configmap %s / %s ", api.OpenShiftConsoleNamespace, cmName)
Expand Down
4 changes: 2 additions & 2 deletions pkg/console/controllers/route/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ func NewRouteSyncController(
operatorConfigInformer.Informer(),
configV1Informers.Ingresses().Informer(),
).WithFilteredEventsInformers( // service
util.NamesFilter(api.TrustedCAConfigMapName, api.DefaultIngressCertConfigMapName),
util.NamesFilter(api.TrustedCAConfigMapName, api.OAuthServingCertConfigMapName),
configMapInformer.Informer(),
).WithInformers(
secretInformer.Informer(),
Expand Down Expand Up @@ -385,7 +385,7 @@ func (c *RouteSyncController) getCA(ctx context.Context, tls *routev1.TLSConfig)
}
}

for _, cmName := range []string{api.TrustedCAConfigMapName, api.DefaultIngressCertConfigMapName} {
for _, cmName := range []string{api.TrustedCAConfigMapName, api.OAuthServingCertConfigMapName} {
cm, err := c.configMapClient.ConfigMaps(api.OpenShiftConsoleNamespace).Get(ctx, cmName, metav1.GetOptions{})
if err != nil {
klog.V(4).Infof("failed to GET configmap %s / %s ", api.OpenShiftConsoleNamespace, cmName)
Expand Down
36 changes: 18 additions & 18 deletions pkg/console/operator/sync_v400.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,10 +101,10 @@ func (co *consoleOperator) sync_v400(ctx context.Context, controllerContext fact
return statusHandler.FlushAndReturn(customLogoError)
}

defaultIngressCertConfigMap, defaultIngressCertErrReason, defaultIngressCertErr := co.ValidateDefaultIngressCertConfigMap(ctx)
statusHandler.AddConditions(status.HandleProgressingOrDegraded("DefaultIngressCertValidation", defaultIngressCertErrReason, defaultIngressCertErr))
if defaultIngressCertErr != nil {
return statusHandler.FlushAndReturn(defaultIngressCertErr)
oauthServingCertConfigMap, oauthServingCertErrReason, oauthServingCertErr := co.ValidateOAuthServingCertConfigMap(ctx)
statusHandler.AddConditions(status.HandleProgressingOrDegraded("OAuthServingCertValidation", oauthServingCertErrReason, oauthServingCertErr))
Copy link
Member

Choose a reason for hiding this comment

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

you're changing a condition name here, you'll want to use something like https://github.com/openshift/library-go/blob/14aa5b749d8ddbae509ea97c988228934dbbd3db/pkg/operator/staleconditions/remove_stale_conditions.go#L19 in order to remove a stale condition (otherwise this might brick upgrades)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if oauthServingCertErr != nil {
return statusHandler.FlushAndReturn(oauthServingCertErr)
}

sec, secChanged, secErr := co.SyncSecret(ctx, set.Operator, controllerContext.Recorder())
Expand All @@ -121,7 +121,7 @@ func (co *consoleOperator) sync_v400(ctx context.Context, controllerContext fact
return statusHandler.FlushAndReturn(oauthErr)
}

actualDeployment, depChanged, depErrReason, depErr := co.SyncDeployment(ctx, set.Operator, cm, serviceCAConfigMap, defaultIngressCertConfigMap, trustedCAConfigMap, sec, set.Proxy, set.Infrastructure, customLogoCanMount, controllerContext.Recorder())
actualDeployment, depChanged, depErrReason, depErr := co.SyncDeployment(ctx, set.Operator, cm, serviceCAConfigMap, oauthServingCertConfigMap, trustedCAConfigMap, sec, set.Proxy, set.Infrastructure, customLogoCanMount, controllerContext.Recorder())
toUpdate = toUpdate || depChanged
statusHandler.AddConditions(status.HandleProgressingOrDegraded("DeploymentSync", depErrReason, depErr))
if depErr != nil {
Expand Down Expand Up @@ -237,7 +237,7 @@ func (co *consoleOperator) SyncDeployment(
operatorConfig *operatorv1.Console,
cm *corev1.ConfigMap,
serviceCAConfigMap *corev1.ConfigMap,
defaultIngressCertConfigMap *corev1.ConfigMap,
oauthServingCertConfigMap *corev1.ConfigMap,
trustedCAConfigMap *corev1.ConfigMap,
sec *corev1.Secret,
proxyConfig *configv1.Proxy,
Expand All @@ -246,7 +246,7 @@ func (co *consoleOperator) SyncDeployment(
recorder events.Recorder) (consoleDeployment *appsv1.Deployment, changed bool, reason string, err error) {

updatedOperatorConfig := operatorConfig.DeepCopy()
requiredDeployment := deploymentsub.DefaultDeployment(operatorConfig, cm, serviceCAConfigMap, defaultIngressCertConfigMap, trustedCAConfigMap, sec, proxyConfig, infrastructureConfig, canMountCustomLogo)
requiredDeployment := deploymentsub.DefaultDeployment(operatorConfig, cm, serviceCAConfigMap, oauthServingCertConfigMap, trustedCAConfigMap, sec, proxyConfig, infrastructureConfig, canMountCustomLogo)
genChanged := operatorConfig.ObjectMeta.Generation != operatorConfig.Status.ObservedGeneration

if genChanged {
Expand Down Expand Up @@ -319,11 +319,11 @@ func (co *consoleOperator) SyncConfigMap(
}

useDefaultCAFile := false
// We are syncing the `default-ingress-cert` configmap from `openshift-config-managed` to `openshift-console`.
// `default-ingress-cert` is only published in `openshift-config-managed` in OpenShift 4.4.0 and newer.
// If the `default-ingress-cert` configmap in `openshift-console` exists, we should mount that to the console container,
// We are syncing the `oauth-serving-cert` configmap from `openshift-config-managed` to `openshift-console`.
// `oauth-serving-cert` is only published in `openshift-config-managed` in OpenShift 4.9.0 and newer.
// If the `oauth-serving-cert` configmap in `openshift-console` exists, we should mount that to the console container,
// otherwise default to `/var/run/secrets/kubernetes.io/serviceaccount/ca.crt`
_, rcaErr := co.configMapClient.ConfigMaps(api.OpenShiftConsoleNamespace).Get(ctx, api.DefaultIngressCertConfigMapName, metav1.GetOptions{})
_, rcaErr := co.configMapClient.ConfigMaps(api.OpenShiftConsoleNamespace).Get(ctx, api.OAuthServingCertConfigMapName, metav1.GetOptions{})
if rcaErr != nil && apierrors.IsNotFound(rcaErr) {
useDefaultCAFile = true
}
Expand Down Expand Up @@ -434,18 +434,18 @@ func (co *consoleOperator) SyncCustomLogoConfigMap(ctx context.Context, operator
return okToMount, reason, err
}

func (co *consoleOperator) ValidateDefaultIngressCertConfigMap(ctx context.Context) (defaultIngressCert *corev1.ConfigMap, reason string, err error) {
defaultIngressCertConfigMap, err := co.configMapClient.ConfigMaps(api.OpenShiftConsoleNamespace).Get(ctx, api.DefaultIngressCertConfigMapName, metav1.GetOptions{})
func (co *consoleOperator) ValidateOAuthServingCertConfigMap(ctx context.Context) (oauthServingCert *corev1.ConfigMap, reason string, err error) {
oauthServingCertConfigMap, err := co.configMapClient.ConfigMaps(api.OpenShiftConsoleNamespace).Get(ctx, api.OAuthServingCertConfigMapName, metav1.GetOptions{})
if err != nil {
klog.V(4).Infoln("default-ingress-cert configmap not found")
return nil, "FailedGet", fmt.Errorf("default-ingress-cert configmap not found")
klog.V(4).Infoln("oauth-serving-cert configmap not found")
return nil, "FailedGet", fmt.Errorf("oauth-serving-cert configmap not found")
}

_, caBundle := defaultIngressCertConfigMap.Data["ca-bundle.crt"]
_, caBundle := oauthServingCertConfigMap.Data["ca-bundle.crt"]
if !caBundle {
return nil, "MissingDefaultIngressCertBundle", fmt.Errorf("default-ingress-cert configmap is missing ca-bundle.crt data")
return nil, "MissingOAuthServingCertBundle", fmt.Errorf("oauth-serving-cert configmap is missing ca-bundle.crt data")
}
return defaultIngressCertConfigMap, "", nil
return oauthServingCertConfigMap, "", nil
}

// on each pass of the operator sync loop, we need to check the
Expand Down
6 changes: 3 additions & 3 deletions pkg/console/starter/starter.go
Original file line number Diff line number Diff line change
Expand Up @@ -400,12 +400,12 @@ func RunOperator(ctx context.Context, controllerContext *controllercmd.Controlle

// startResourceSyncing should start syncing process of all secrets and configmaps that need to be synced.
func startStaticResourceSyncing(resourceSyncer *resourcesynccontroller.ResourceSyncController) error {
// sync: 'default-ingress-cert' configmap
// sync: 'oauth-serving-cert' configmap
// from: 'openshift-config-managed' namespace
// to: 'openshift-console' namespace
return resourceSyncer.SyncConfigMap(
resourcesynccontroller.ResourceLocation{Name: api.DefaultIngressCertConfigMapName, Namespace: api.OpenShiftConsoleNamespace},
resourcesynccontroller.ResourceLocation{Name: api.DefaultIngressCertConfigMapName, Namespace: api.OpenShiftConfigManagedNamespace},
resourcesynccontroller.ResourceLocation{Name: api.OAuthServingCertConfigMapName, Namespace: api.OpenShiftConsoleNamespace},
resourcesynccontroller.ResourceLocation{Name: api.OAuthServingCertConfigMapName, Namespace: api.OpenShiftConfigManagedNamespace},
)
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/console/subresource/configmap/configmap.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func DefaultConfigMap(
LogoutURL(defaultLogoutURL).
Brand(DEFAULT_BRAND).
DocURL(DEFAULT_DOC_URL).
DefaultIngressCert(useDefaultCAFile).
OAuthServingCert(useDefaultCAFile).
APIServerURL(getApiUrl(infrastructureConfig)).
InactivityTimeout(inactivityTimeoutSeconds).
ConfigYAML()
Expand All @@ -65,7 +65,7 @@ func DefaultConfigMap(
LogoutURL(consoleConfig.Spec.Authentication.LogoutRedirect).
Brand(operatorConfig.Spec.Customization.Brand).
DocURL(operatorConfig.Spec.Customization.DocumentationBaseURL).
DefaultIngressCert(useDefaultCAFile).
OAuthServingCert(useDefaultCAFile).
APIServerURL(getApiUrl(infrastructureConfig)).
Plugins(pluginsEndpoingMap).
CustomLogoFile(operatorConfig.Spec.Customization.CustomLogoFile.Key).
Expand Down
6 changes: 3 additions & 3 deletions pkg/console/subresource/configmap/configmap_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ providers: {}
},
},
{
name: "Test configmap with default-ingress-cert",
name: "Test configmap with oauth-serving-cert",
args: args{
operatorConfig: &operatorv1.Console{},
consoleConfig: &configv1.Console{},
Expand Down Expand Up @@ -128,7 +128,7 @@ apiVersion: console.openshift.io/v1
auth:
clientID: console
clientSecretFile: /var/oauth-config/clientSecret
oauthEndpointCAFile: /var/default-ingress-cert/ca-bundle.crt
oauthEndpointCAFile: /var/oauth-serving-cert/ca-bundle.crt
clusterInfo:
consoleBaseAddress: https://` + host + `
masterPublicURL: ` + mockAPIServer + `
Expand Down Expand Up @@ -456,7 +456,7 @@ apiVersion: console.openshift.io/v1
auth:
clientID: console
clientSecretFile: /var/oauth-config/clientSecret
oauthEndpointCAFile: /var/default-ingress-cert/ca-bundle.crt
oauthEndpointCAFile: /var/oauth-serving-cert/ca-bundle.crt
clusterInfo:
consoleBaseAddress: https://` + customHostname + `
masterPublicURL: ` + mockAPIServer + `
Expand Down
12 changes: 6 additions & 6 deletions pkg/console/subresource/consoleserver/config_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ import (
)

const (
clientSecretFilePath = "/var/oauth-config/clientSecret"
defaultIngressCertFilePath = "/var/default-ingress-cert/ca-bundle.crt"
oauthEndpointCAFilePath = "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt"
clientSecretFilePath = "/var/oauth-config/clientSecret"
oauthServingCertFilePath = "/var/oauth-serving-cert/ca-bundle.crt"
oauthEndpointCAFilePath = "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt"
// serving info
certFilePath = "/var/serving-cert/tls.crt"
keyFilePath = "/var/serving-cert/tls.key"
Expand Down Expand Up @@ -107,12 +107,12 @@ func (b *ConsoleServerCLIConfigBuilder) StatusPageID(id string) *ConsoleServerCL
return b
}

func (b *ConsoleServerCLIConfigBuilder) DefaultIngressCert(useDefaultDefaultIngressCert bool) *ConsoleServerCLIConfigBuilder {
if useDefaultDefaultIngressCert {
func (b *ConsoleServerCLIConfigBuilder) OAuthServingCert(useDefaultCAFile bool) *ConsoleServerCLIConfigBuilder {
if useDefaultCAFile {
b.CAFile = oauthEndpointCAFilePath
return b
}
b.CAFile = defaultIngressCertFilePath
b.CAFile = oauthServingCertFilePath
return b
}

Expand Down
14 changes: 7 additions & 7 deletions pkg/console/subresource/consoleserver/config_builder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func TestConsoleServerCLIConfigBuilder(t *testing.T) {
APIServerURL("https://foobar.com/api").
Host("https://foobar.com/host").
LogoutURL("https://foobar.com/logout").
DefaultIngressCert(false).
OAuthServingCert(false).
Config()
},
output: Config{
Expand All @@ -69,7 +69,7 @@ func TestConsoleServerCLIConfigBuilder(t *testing.T) {
Auth: Auth{
ClientID: api.OpenShiftConsoleName,
ClientSecretFile: clientSecretFilePath,
OAuthEndpointCAFile: defaultIngressCertFilePath,
OAuthEndpointCAFile: oauthServingCertFilePath,
LogoutRedirect: "https://foobar.com/logout",
},
Customization: Customization{},
Expand All @@ -83,7 +83,7 @@ func TestConsoleServerCLIConfigBuilder(t *testing.T) {
APIServerURL("https://foobar.com/api").
Host("https://foobar.com/host").
LogoutURL("https://foobar.com/logout").
DefaultIngressCert(false).
OAuthServingCert(false).
Config()
},
output: Config{
Expand All @@ -102,7 +102,7 @@ func TestConsoleServerCLIConfigBuilder(t *testing.T) {
Auth: Auth{
ClientID: api.OpenShiftConsoleName,
ClientSecretFile: clientSecretFilePath,
OAuthEndpointCAFile: defaultIngressCertFilePath,
OAuthEndpointCAFile: oauthServingCertFilePath,
LogoutRedirect: "https://foobar.com/logout",
},
Customization: Customization{},
Expand Down Expand Up @@ -435,7 +435,7 @@ providers: {}
APIServerURL("https://foobar.com/api").
Host("https://foobar.com/host").
LogoutURL("https://foobar.com/logout").
DefaultIngressCert(false).
OAuthServingCert(false).
ConfigYAML()
},
output: `apiVersion: console.openshift.io/v1
Expand All @@ -450,7 +450,7 @@ clusterInfo:
auth:
clientID: console
clientSecretFile: /var/oauth-config/clientSecret
oauthEndpointCAFile: /var/default-ingress-cert/ca-bundle.crt
oauthEndpointCAFile: /var/oauth-serving-cert/ca-bundle.crt
logoutRedirect: https://foobar.com/logout
customization: {}
providers: {}
Expand Down Expand Up @@ -632,7 +632,7 @@ providers: {}
"plugin1": "plugin1_url",
"plugin2": "plugin2_url",
}).
DefaultIngressCert(true)
OAuthServingCert(true)
return b.ConfigYAML()
},
output: `apiVersion: console.openshift.io/v1
Expand Down
44 changes: 22 additions & 22 deletions pkg/console/subresource/deployment/deployment.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,16 @@ const (
)

const (
configMapResourceVersionAnnotation = "console.openshift.io/console-config-version"
proxyConfigResourceVersionAnnotation = "console.openshift.io/proxy-config-version"
infrastructureConfigResourceVersionAnnotation = "console.openshift.io/infrastructure-config-version"
serviceCAConfigMapResourceVersionAnnotation = "console.openshift.io/service-ca-config-version"
defaultIngressCertConfigMapResourceVersionAnnotation = "console.openshift.io/default-ingress-cert-config-version"
trustedCAConfigMapResourceVersionAnnotation = "console.openshift.io/trusted-ca-config-version"
secretResourceVersionAnnotation = "console.openshift.io/oauth-secret-version"
consoleImageAnnotation = "console.openshift.io/image"
workloadManagementAnnotation = "target.workload.openshift.io/management"
workloadManagementAnnotationValue = `{"effect": "PreferredDuringScheduling"}`
configMapResourceVersionAnnotation = "console.openshift.io/console-config-version"
proxyConfigResourceVersionAnnotation = "console.openshift.io/proxy-config-version"
infrastructureConfigResourceVersionAnnotation = "console.openshift.io/infrastructure-config-version"
serviceCAConfigMapResourceVersionAnnotation = "console.openshift.io/service-ca-config-version"
oauthServingCertConfigMapResourceVersionAnnotation = "console.openshift.io/oauth-serving-cert-config-version"
trustedCAConfigMapResourceVersionAnnotation = "console.openshift.io/trusted-ca-config-version"
secretResourceVersionAnnotation = "console.openshift.io/oauth-secret-version"
consoleImageAnnotation = "console.openshift.io/image"
workloadManagementAnnotation = "target.workload.openshift.io/management"
workloadManagementAnnotationValue = `{"effect": "PreferredDuringScheduling"}`
)

var (
Expand All @@ -45,7 +45,7 @@ var (
proxyConfigResourceVersionAnnotation,
infrastructureConfigResourceVersionAnnotation,
serviceCAConfigMapResourceVersionAnnotation,
defaultIngressCertConfigMapResourceVersionAnnotation,
oauthServingCertConfigMapResourceVersionAnnotation,
trustedCAConfigMapResourceVersionAnnotation,
secretResourceVersionAnnotation,
consoleImageAnnotation,
Expand All @@ -63,19 +63,19 @@ type volumeConfig struct {
mappedKeys map[string]string
}

func DefaultDeployment(operatorConfig *operatorv1.Console, cm *corev1.ConfigMap, serviceCAConfigMap *corev1.ConfigMap, defaultIngressCertConfigMap *corev1.ConfigMap, trustedCAConfigMap *corev1.ConfigMap, sec *corev1.Secret, proxyConfig *configv1.Proxy, infrastructureConfig *configv1.Infrastructure, canMountCustomLogo bool) *appsv1.Deployment {
func DefaultDeployment(operatorConfig *operatorv1.Console, cm *corev1.ConfigMap, serviceCAConfigMap *corev1.ConfigMap, oauthServingCertConfigMap *corev1.ConfigMap, trustedCAConfigMap *corev1.ConfigMap, sec *corev1.Secret, proxyConfig *configv1.Proxy, infrastructureConfig *configv1.Infrastructure, canMountCustomLogo bool) *appsv1.Deployment {
labels := util.LabelsForConsole()
meta := util.SharedMeta()
meta.Labels = labels
deploymentAnnotations := map[string]string{
configMapResourceVersionAnnotation: cm.GetResourceVersion(),
serviceCAConfigMapResourceVersionAnnotation: serviceCAConfigMap.GetResourceVersion(),
defaultIngressCertConfigMapResourceVersionAnnotation: defaultIngressCertConfigMap.GetResourceVersion(),
trustedCAConfigMapResourceVersionAnnotation: trustedCAConfigMap.GetResourceVersion(),
proxyConfigResourceVersionAnnotation: proxyConfig.GetResourceVersion(),
infrastructureConfigResourceVersionAnnotation: infrastructureConfig.GetResourceVersion(),
secretResourceVersionAnnotation: sec.GetResourceVersion(),
consoleImageAnnotation: util.GetImageEnv("CONSOLE_IMAGE"),
configMapResourceVersionAnnotation: cm.GetResourceVersion(),
serviceCAConfigMapResourceVersionAnnotation: serviceCAConfigMap.GetResourceVersion(),
oauthServingCertConfigMapResourceVersionAnnotation: oauthServingCertConfigMap.GetResourceVersion(),
trustedCAConfigMapResourceVersionAnnotation: trustedCAConfigMap.GetResourceVersion(),
proxyConfigResourceVersionAnnotation: proxyConfig.GetResourceVersion(),
infrastructureConfigResourceVersionAnnotation: infrastructureConfig.GetResourceVersion(),
secretResourceVersionAnnotation: sec.GetResourceVersion(),
consoleImageAnnotation: util.GetImageEnv("CONSOLE_IMAGE"),
}

// Set any annotations as needed so that `ApplyDeployment` rolls out a
Expand Down Expand Up @@ -590,9 +590,9 @@ func defaultVolumeConfig() []volumeConfig {
isConfigMap: true,
},
{
name: api.DefaultIngressCertConfigMapName,
name: api.OAuthServingCertConfigMapName,
readOnly: true,
path: "/var/default-ingress-cert",
path: "/var/oauth-serving-cert",
isConfigMap: true,
},
}
Expand Down