diff --git a/controllers/usernamespace/usernamespace_controller.go b/controllers/usernamespace/usernamespace_controller.go index 36dec2920..4e08ef1d9 100644 --- a/controllers/usernamespace/usernamespace_controller.go +++ b/controllers/usernamespace/usernamespace_controller.go @@ -460,7 +460,7 @@ func (r *CheUserNamespaceReconciler) reconcileGitTlsCertificate(ctx context.Cont return delConfigMap() } - if gitCert.Data["ca.crt"] == "" { + if gitCert.Data[constants.GitSelfSignedCertsConfigMapCertKey] == "" { return delConfigMap() } @@ -479,12 +479,12 @@ func (r *CheUserNamespaceReconciler) reconcileGitTlsCertificate(ctx context.Cont }), }, Data: map[string]string{ - "certificate": gitCert.Data["ca.crt"], + "certificate": gitCert.Data[constants.GitSelfSignedCertsConfigMapCertKey], }, } - if gitCert.Data["githost"] != "" { - target.Data["host"] = gitCert.Data["githost"] + if gitCert.Data[constants.GitSelfSignedCertsConfigMapGitHostKey] != "" { + target.Data["host"] = gitCert.Data[constants.GitSelfSignedCertsConfigMapGitHostKey] } _, err := deploy.Sync(deployContext, &target, diffs.ConfigMapAllLabels) diff --git a/pkg/common/constants/constants.go b/pkg/common/constants/constants.go index 2572fc859..50d2b703b 100644 --- a/pkg/common/constants/constants.go +++ b/pkg/common/constants/constants.go @@ -37,22 +37,31 @@ const ( DefaultPluginRegistryCpuRequest = "100m" // Server - DefaultServerMemoryLimit = "1024Mi" - DefaultServerMemoryRequest = "512Mi" - DefaultServerCpuLimit = "1" - DefaultServerCpuRequest = "100m" - DefaultServerLogLevel = "INFO" - DefaultServerDebug = false - DefaultServerMetricsPort = int32(8087) - DefaultServerDebugPort = int32(8000) - DefaultServerPort = int32(8080) + DefaultServerMemoryLimit = "1024Mi" + DefaultServerMemoryRequest = "512Mi" + DefaultServerCpuLimit = "1" + DefaultServerCpuRequest = "100m" + DefaultServerLogLevel = "INFO" + DefaultServerDebug = false + DefaultServerMetricsPort = int32(8087) + DefaultServerDebugPort = int32(8000) + DefaultServerPort = int32(8080) + DefaultProxyCredentialsSecret = "proxy-credentials" + DefaultJavaOpts = "-XX:MaxRAMPercentage=85.0" + DefaultSecurityContextFsGroup = 1724 + DefaultSecurityContextRunAsUser = 1724 + DefaultCheServiceAccountName = "che" + + // Certificates DefaultCaBundleCertsCMName = "ca-certs" - DefaultProxyCredentialsSecret = "proxy-credentials" DefaultGitSelfSignedCertsConfigMapName = "che-git-self-signed-cert" - DefaultJavaOpts = "-XX:MaxRAMPercentage=85.0" - DefaultSecurityContextFsGroup = 1724 - DefaultSecurityContextRunAsUser = 1724 - DefaultCheServiceAccountName = "che" + // GitSelfSignedCertsConfigMapCertKey is the ConfigMap data key that holds the CA certificate + // in the git self-signed certificates ConfigMap. + GitSelfSignedCertsConfigMapCertKey = "ca.crt" + // GitSelfSignedCertsConfigMapGitHostKey is the ConfigMap data key that holds the git server hostname + // in the git self-signed certificates ConfigMap. This value is not a certificate and must be + // excluded when merging CA bundles. + GitSelfSignedCertsConfigMapGitHostKey = "githost" // OAuth BitBucketOAuthConfigClientIdFileName = "id" diff --git a/pkg/deploy/tls/certificates.go b/pkg/deploy/tls/certificates.go index 44d884dda..d386bb8ab 100644 --- a/pkg/deploy/tls/certificates.go +++ b/pkg/deploy/tls/certificates.go @@ -15,7 +15,9 @@ package tls import ( "errors" "fmt" + "maps" "os" + "slices" "sort" "strings" @@ -213,7 +215,7 @@ func (c *CertificatesReconciler) syncGitTrustedCertificates(ctx *chetypes.Deploy return err == nil, err } - if gitTrustedCertsCM.Data["ca.crt"] != "" { + if gitTrustedCertsCM.Data[constants.GitSelfSignedCertsConfigMapCertKey] != "" { gitTrustedCertsCM.TypeMeta = metav1.TypeMeta{ Kind: "ConfigMap", APIVersion: "v1", @@ -318,22 +320,27 @@ func (c *CertificatesReconciler) syncCheCABundleCerts(ctx *chetypes.DeployContex return false, err } - // Sort configmaps by name, always have the same order and content - // to avoid endless reconcile loop + // Sort ConfigMaps by name and their data keys alphabetically to ensure + // deterministic ordering. This prevents spurious reconcile loops that occur + // when Go's random map iteration produces different output each time. sort.Slice(cheCABundlesCMs, func(i, j int) bool { return strings.Compare(cheCABundlesCMs[i].Name, cheCABundlesCMs[j].Name) < 0 }) - // Calculated revisions and content cheCABundlesContent := "" for _, cm := range cheCABundlesCMs { - for dataKey, dataValue := range cm.Data { - cheCABundlesContent += fmt.Sprintf( - "# ConfigMap: %s, Key: %s\n%s\n\n", - cm.Name, - dataKey, - dataValue, - ) + // Sort keys to produce deterministic output and avoid endless reconcile loop + dataKeys := slices.Collect(maps.Keys(cm.Data)) + sort.Strings(dataKeys) + + for _, dataKey := range dataKeys { + // Skip the "githost" key from the git trusted certs ConfigMap: + // it contains a hostname, not a certificate, and should not be included in the CA bundle. + if dataKey == constants.GitSelfSignedCertsConfigMapGitHostKey && isGitTrustedCertsConfigMap(ctx, &cm) { + continue + } + + cheCABundlesContent += printCert(&cm, dataKey) } } @@ -394,3 +401,26 @@ func readKubernetesCaBundle() ([]byte, error) { return data, nil } + +// printCert formats a single certificate entry with its ConfigMap name and key as a header comment. +func printCert(cm *corev1.ConfigMap, key string) string { + return fmt.Sprintf( + "# ConfigMap: %s, Key: %s\n%s\n\n", + cm.Name, + key, + cm.Data[key], + ) +} + +func isGitTrustedCertsConfigMap(ctx *chetypes.DeployContext, cm *corev1.ConfigMap) bool { + if cm.Name == constants.DefaultGitSelfSignedCertsConfigMapName { + return true + } + + if ctx.CheCluster.Spec.DevEnvironments.TrustedCerts != nil && + cm.Name == ctx.CheCluster.Spec.DevEnvironments.TrustedCerts.GitTrustedCertsConfigMapName { + return true + } + + return false +} diff --git a/pkg/deploy/tls/certificates_test.go b/pkg/deploy/tls/certificates_test.go index 6b438f4cb..4f3e93a75 100644 --- a/pkg/deploy/tls/certificates_test.go +++ b/pkg/deploy/tls/certificates_test.go @@ -263,6 +263,128 @@ func TestSyncCheCABundleCerts(t *testing.T) { assert.Equal(t, cm.Data[kubernetesCABundleCertsFile], "# ConfigMap: cert1, Key: a1\nb1\n\n# ConfigMap: cert2, Key: a2\nb2\n\n") } +func TestSyncCheCABundleCertsDeterministicKeyOrder(t *testing.T) { + ctx := test.NewCtxBuilder().WithObjects( + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cert1", + Namespace: "eclipse-che", + Labels: map[string]string{ + "app.kubernetes.io/component": "ca-bundle", + "app.kubernetes.io/part-of": "che.eclipse.org", + }, + }, + Data: map[string]string{ + "z-key": "z-value", + "a-key": "a-value", + "m-key": "m-value", + }, + }).Build() + + certificatesReconciler := NewCertificatesReconciler() + + _, err := certificatesReconciler.syncCheCABundleCerts(ctx) + assert.NoError(t, err) + + cm := &corev1.ConfigMap{} + err = ctx.ClusterAPI.Client.Get(context.TODO(), types.NamespacedName{Name: CheMergedCABundleCertsCMName, Namespace: "eclipse-che"}, cm) + assert.Nil(t, err) + + expected := "# ConfigMap: cert1, Key: a-key\na-value\n\n" + + "# ConfigMap: cert1, Key: m-key\nm-value\n\n" + + "# ConfigMap: cert1, Key: z-key\nz-value\n\n" + assert.Equal(t, expected, cm.Data[kubernetesCABundleCertsFile]) +} + +func TestSyncCheCABundleCertsGitTrustedCertsExcludesGitHostKey(t *testing.T) { + ctx := test.NewCtxBuilder().WithCheCluster( + &chev2.CheCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "eclipse-che", + Namespace: "eclipse-che", + }, + Spec: chev2.CheClusterSpec{ + DevEnvironments: chev2.CheClusterDevEnvironments{ + TrustedCerts: &chev2.TrustedCerts{ + GitTrustedCertsConfigMapName: "git-trusted-certs", + }, + }, + }, + }).WithObjects( + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "git-trusted-certs", + Namespace: "eclipse-che", + Labels: map[string]string{ + "app.kubernetes.io/component": constants.CheCABundle, + "app.kubernetes.io/part-of": constants.CheEclipseOrg, + }, + }, + Data: map[string]string{ + constants.GitSelfSignedCertsConfigMapCertKey: "git-cert-value", + constants.GitSelfSignedCertsConfigMapGitHostKey: "https://git.example.com", + }, + }, + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "other-cert", + Namespace: "eclipse-che", + Labels: map[string]string{ + "app.kubernetes.io/component": constants.CheCABundle, + "app.kubernetes.io/part-of": constants.CheEclipseOrg, + }, + }, + Data: map[string]string{ + "cert-key": "other-cert-value", + }, + }).Build() + + certificates := NewCertificatesReconciler() + + _, err := certificates.syncCheCABundleCerts(ctx) + assert.NoError(t, err) + + cm := &corev1.ConfigMap{} + err = ctx.ClusterAPI.Client.Get(context.TODO(), types.NamespacedName{Name: CheMergedCABundleCertsCMName, Namespace: "eclipse-che"}, cm) + assert.NoError(t, err) + + // Verify that the githost key is excluded from the merged bundle + expected := "# ConfigMap: git-trusted-certs, Key: ca.crt\ngit-cert-value\n\n" + + "# ConfigMap: other-cert, Key: cert-key\nother-cert-value\n\n" + + assert.Equal(t, expected, cm.Data[kubernetesCABundleCertsFile]) +} + +func TestSyncCheCABundleCertsExcludesGitHostKeyWithDefaultConfigMap(t *testing.T) { + ctx := test.NewCtxBuilder().WithObjects( + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: constants.DefaultGitSelfSignedCertsConfigMapName, // "che-git-self-signed-cert" + Namespace: "eclipse-che", + Labels: map[string]string{ + "app.kubernetes.io/component": constants.CheCABundle, + "app.kubernetes.io/part-of": constants.CheEclipseOrg, + }, + }, + Data: map[string]string{ + constants.GitSelfSignedCertsConfigMapCertKey: "cert-data", + constants.GitSelfSignedCertsConfigMapGitHostKey: "https://git.example.com", + }, + }).Build() + + certificates := NewCertificatesReconciler() + _, err := certificates.syncCheCABundleCerts(ctx) + assert.NoError(t, err) + + cm := &corev1.ConfigMap{} + err = ctx.ClusterAPI.Client.Get(context.TODO(), types.NamespacedName{Name: CheMergedCABundleCertsCMName, Namespace: "eclipse-che"}, cm) + assert.NoError(t, err) + + // Verify githost is excluded even when using default ConfigMap name + expected := "# ConfigMap: che-git-self-signed-cert, Key: ca.crt\ncert-data\n\n" + assert.Equal(t, expected, cm.Data[kubernetesCABundleCertsFile]) +} + func TestToggleDisableWorkspaceCaBundleMount(t *testing.T) { // Enable workspace CA bundle mount ctx := test.NewCtxBuilder().WithObjects(&corev1.ConfigMap{ @@ -372,3 +494,140 @@ func TestToggleDisableWorkspaceCaBundleMount(t *testing.T) { assert.Equal(t, caCertsMergedCM.Data["tls-ca-bundle.pem"], "# ConfigMap: ca-certs, Key: ca-bundle.crt\nopenshift-ca-bundle-new\n\n") assert.Equal(t, 1, len(caCertsMergedCM.Data)) } + +func TestSyncCheCABundleCertsWithEmptyConfigMap(t *testing.T) { + // A CA bundle ConfigMap exists but has no data entries + emptyCert := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "empty-cert", + Namespace: "eclipse-che", + Labels: map[string]string{ + "app.kubernetes.io/component": constants.CheCABundle, + "app.kubernetes.io/part-of": constants.CheEclipseOrg, + }, + }, + Data: map[string]string{}, + } + ctx := test.NewCtxBuilder().WithObjects(emptyCert).Build() + + certificates := NewCertificatesReconciler() + + _, err := certificates.syncCheCABundleCerts(ctx) + assert.NoError(t, err) + + cm := &corev1.ConfigMap{} + err = ctx.ClusterAPI.Client.Get(context.TODO(), types.NamespacedName{Name: CheMergedCABundleCertsCMName, Namespace: "eclipse-che"}, cm) + assert.NoError(t, err) + + // Merged CM should have no tls-ca-bundle.pem key when source ConfigMap is empty + assert.Empty(t, cm.Data) +} + +func TestSyncCheCABundleCertsWithEmptyAndNonEmptyConfigMaps(t *testing.T) { + emptyCert := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "empty-cert", + Namespace: "eclipse-che", + Labels: map[string]string{ + "app.kubernetes.io/component": constants.CheCABundle, + "app.kubernetes.io/part-of": constants.CheEclipseOrg, + }, + }, + Data: map[string]string{}, + } + nonEmptyCert := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "non-empty-cert", + Namespace: "eclipse-che", + Labels: map[string]string{ + "app.kubernetes.io/component": constants.CheCABundle, + "app.kubernetes.io/part-of": constants.CheEclipseOrg, + }, + }, + Data: map[string]string{"ca.crt": "some-cert"}, + } + ctx := test.NewCtxBuilder().WithObjects(emptyCert, nonEmptyCert).Build() + + certificates := NewCertificatesReconciler() + + _, err := certificates.syncCheCABundleCerts(ctx) + assert.NoError(t, err) + + cm := &corev1.ConfigMap{} + err = ctx.ClusterAPI.Client.Get(context.TODO(), types.NamespacedName{Name: CheMergedCABundleCertsCMName, Namespace: "eclipse-che"}, cm) + assert.NoError(t, err) + + // Only the non-empty ConfigMap's cert should be in the merged bundle + expected := "# ConfigMap: non-empty-cert, Key: ca.crt\nsome-cert\n\n" + assert.Equal(t, expected, cm.Data[kubernetesCABundleCertsFile]) +} + +func TestSyncCheCABundleCertsWithNilDataConfigMap(t *testing.T) { + // A CA bundle ConfigMap exists with nil Data (not initialized) + nilDataCert := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nil-data-cert", + Namespace: "eclipse-che", + Labels: map[string]string{ + "app.kubernetes.io/component": constants.CheCABundle, + "app.kubernetes.io/part-of": constants.CheEclipseOrg, + }, + }, + } + ctx := test.NewCtxBuilder().WithObjects(nilDataCert).Build() + + certificates := NewCertificatesReconciler() + + _, err := certificates.syncCheCABundleCerts(ctx) + assert.NoError(t, err) + + cm := &corev1.ConfigMap{} + err = ctx.ClusterAPI.Client.Get(context.TODO(), types.NamespacedName{Name: CheMergedCABundleCertsCMName, Namespace: "eclipse-che"}, cm) + assert.NoError(t, err) + + // Merged CM should have no tls-ca-bundle.pem key when source ConfigMap has nil Data + assert.Empty(t, cm.Data) +} + +func TestSyncCheCABundleCertsGitTrustedCertsOnlyGitHostKey(t *testing.T) { + // Git trusted certs ConfigMap has only the githost key (no actual cert) + ctx := test.NewCtxBuilder().WithCheCluster( + &chev2.CheCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "eclipse-che", + Namespace: "eclipse-che", + }, + Spec: chev2.CheClusterSpec{ + DevEnvironments: chev2.CheClusterDevEnvironments{ + TrustedCerts: &chev2.TrustedCerts{ + GitTrustedCertsConfigMapName: "git-trusted-certs", + }, + }, + }, + }).WithObjects( + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "git-trusted-certs", + Namespace: "eclipse-che", + Labels: map[string]string{ + "app.kubernetes.io/component": constants.CheCABundle, + "app.kubernetes.io/part-of": constants.CheEclipseOrg, + }, + }, + Data: map[string]string{ + constants.GitSelfSignedCertsConfigMapGitHostKey: "https://git.example.com", + }, + }).Build() + + certificates := NewCertificatesReconciler() + + _, err := certificates.syncCheCABundleCerts(ctx) + assert.NoError(t, err) + + cm := &corev1.ConfigMap{} + err = ctx.ClusterAPI.Client.Get(context.TODO(), types.NamespacedName{Name: CheMergedCABundleCertsCMName, Namespace: "eclipse-che"}, cm) + assert.NoError(t, err) + + // All keys were skipped (githost is excluded), so merged CM should be empty + assert.Empty(t, cm.Data) +}