From 99c3e47bc37dc3377281f00415995692e481701e Mon Sep 17 00:00:00 2001 From: msivasubramaniaan Date: Tue, 30 Jun 2026 11:22:30 +0530 Subject: [PATCH 1/2] refactor: centralize kubeconfig authentication handlinig (CRW-11310) Signed-off-by: msivasubramaniaan --- .../gateway/kubeconfig/KubeConfigEntries.kt | 21 +++++--- .../gateway/kubeconfig/KubeConfigUpdate.kt | 51 +++++++++++-------- 2 files changed, 45 insertions(+), 27 deletions(-) diff --git a/src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigEntries.kt b/src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigEntries.kt index 79c40794..9c7a3636 100644 --- a/src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigEntries.kt +++ b/src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigEntries.kt @@ -193,7 +193,7 @@ data class KubeConfigNamedUser( fun getByName(userName: String, config: KubeConfig?): KubeConfigNamedUser? { return (config?.users ?: emptyList()) .mapNotNull { - it as? Map + it as? Map<*, *> } .firstOrNull { user -> userName == user["name"] @@ -224,11 +224,6 @@ data class KubeConfigNamedUser( fun getUserTokenForCluster(clusterName: String, kubeConfig: KubeConfig): String? = getUserForCluster(clusterName, kubeConfig)?.token - fun getUserClientCertForCluster(clusterName: String, kubeConfig: KubeConfig): Pair? { - val user = getUserForCluster(clusterName, kubeConfig) ?: return null - return Pair(user.clientCertificate, user.clientKey) - } - fun isTokenAuth(kubeConfig: KubeConfig): Boolean { return kubeConfig.credentials?.containsKey(KubeConfig.CRED_TOKEN_KEY) == true } @@ -275,6 +270,20 @@ data class KubeConfigUser( password = map["password"] as? String ) } + + fun tokenOnly(token: String): KubeConfigUser = + KubeConfigUser( + token = token.trim(), + clientCertificate = null, + clientKey = null + ) + + fun clientCertOnly(cert: CertificateSource, key: CertificateSource): KubeConfigUser = + KubeConfigUser( + token = null, + clientCertificate = cert, + clientKey = key + ) } fun toMap(): MutableMap { diff --git a/src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdate.kt b/src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdate.kt index 3e0a0bb3..d21eca01 100644 --- a/src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdate.kt +++ b/src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdate.kt @@ -14,6 +14,7 @@ package com.redhat.devtools.gateway.kubeconfig import com.intellij.openapi.diagnostic.thisLogger import com.intellij.util.text.UniqueNameGenerator import com.redhat.devtools.gateway.auth.tls.CertificateSource +import com.redhat.devtools.gateway.auth.tls.PemUtils import com.redhat.devtools.gateway.kubeconfig.KubeConfigUtils.path import com.redhat.devtools.gateway.openshift.Utils import io.kubernetes.client.persister.ConfigPersister @@ -114,6 +115,29 @@ abstract class KubeConfigUpdate private constructor( ) } + protected fun setTokenAuthentication( + namedUser: Any, + token: String + ) { + Utils.setValue(namedUser, token, arrayOf("user", "token")) + Utils.removeValue(namedUser, arrayOf("user", "client-certificate")) + Utils.removeValue(namedUser, arrayOf("user", "client-key")) + Utils.removeValue(namedUser, arrayOf("user", "client-certificate-data")) + Utils.removeValue(namedUser, arrayOf("user", "client-key-data")) + } + + protected fun setClientCertificateAuthentication( + namedUser: Any, + clientCertPem: String, + clientKeyPem: String + ) { + Utils.setValue(namedUser, PemUtils.toBase64(clientCertPem), arrayOf("user", "client-certificate-data")) + Utils.setValue(namedUser, PemUtils.toBase64(clientKeyPem), arrayOf("user", "client-key-data")) + Utils.removeValue(namedUser, arrayOf("user", "client-certificate")) + Utils.removeValue(namedUser, arrayOf("user", "client-key")) + Utils.removeValue(namedUser, arrayOf("user", "token")) + } + protected data class ContextEntries( val users: ArrayList, val clusters: ArrayList, @@ -210,17 +234,10 @@ abstract class KubeConfigUpdate private constructor( config.users?.find { user -> username == Utils.getValue(user, arrayOf("name")) }?.apply { - Utils.setValue(this, token, arrayOf("user", "token")) - - removeClientCerts(this) + setTokenAuthentication(this, token) } } - private fun removeClientCerts(namedUser: Any) { - Utils.removeValue(namedUser, arrayOf("user", "client-certificate-data")) - Utils.removeValue(namedUser, arrayOf("user", "client-key-data")) - } - } class CreateContext( @@ -234,7 +251,7 @@ abstract class KubeConfigUpdate private constructor( override fun apply() { val config = allConfigs.firstOrNull() ?: return val user = KubeConfigNamedUser( - KubeConfigUser(authToken), + KubeConfigUser.tokenOnly(authToken), uniqueUserName(allConfigs) ) val entries = createContext(user, config.users, config.clusters, config.contexts) @@ -268,16 +285,9 @@ abstract class KubeConfigUpdate private constructor( config.users?.find { user -> username == Utils.getValue(user, arrayOf("name")) }?.apply { - Utils.setValue(this, clientCertPem, arrayOf("user", "client-certificate-data")) - Utils.setValue(this, clientKeyPem, arrayOf("user", "client-key-data")) - - removeToken(this) + setClientCertificateAuthentication(this,clientCertPem,clientKeyPem) } } - - private fun removeToken(namedUser: Any) { - Utils.removeValue(namedUser, arrayOf("user", "token")) - } } class CreateContextWithClientCert( @@ -292,10 +302,9 @@ abstract class KubeConfigUpdate private constructor( override fun apply() { val config = allConfigs.firstOrNull() ?: return val user = KubeConfigNamedUser( - KubeConfigUser( - token = null, - clientCertificate = CertificateSource.fromData(clientCertPem), - clientKey = CertificateSource.fromData(clientKeyPem) + KubeConfigUser.clientCertOnly( + CertificateSource.fromData(clientCertPem), + CertificateSource.fromData(clientKeyPem) ), uniqueUserName(allConfigs) ) From 19921cc9e34c8481f0b474e7e6e958cfdf869f6e Mon Sep 17 00:00:00 2001 From: Andre Dietisheim Date: Tue, 30 Jun 2026 15:51:10 +0200 Subject: [PATCH 2/2] refactor: introduced constants and added tests (CRW-11310) Signed-off-by: Andre Dietisheim Co-authored-by: Cursor Agent --- .../gateway/kubeconfig/KubeConfigUpdate.kt | 26 +++-- .../kubeconfig/KubeConfigTestHelpers.kt | 5 +- .../kubeconfig/KubeConfigUpdateTest.kt | 103 ++++++++++++++++++ 3 files changed, 123 insertions(+), 11 deletions(-) diff --git a/src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdate.kt b/src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdate.kt index d21eca01..8faa1518 100644 --- a/src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdate.kt +++ b/src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdate.kt @@ -31,6 +31,12 @@ abstract class KubeConfigUpdate private constructor( ) { companion object { + private val USER = arrayOf("user") + private val USER_TOKEN = USER + "token" + private val USER_CLIENT_CERT = USER + "client-certificate" + private val USER_CLIENT_KEY = USER + "client-key" + private val USER_CLIENT_CERT_DATA = USER + "client-certificate-data" + private val USER_CLIENT_KEY_DATA = USER + "client-key-data" fun create(clusterName: String, clusterUrl: String, token: String): KubeConfigUpdate { val allConfigs = KubeConfigUtils.getAllConfigs(KubeConfigUtils.getAllConfigFiles()) val context = KubeConfigNamedContext.getByClusterName(clusterName, allConfigs) @@ -119,11 +125,11 @@ abstract class KubeConfigUpdate private constructor( namedUser: Any, token: String ) { - Utils.setValue(namedUser, token, arrayOf("user", "token")) - Utils.removeValue(namedUser, arrayOf("user", "client-certificate")) - Utils.removeValue(namedUser, arrayOf("user", "client-key")) - Utils.removeValue(namedUser, arrayOf("user", "client-certificate-data")) - Utils.removeValue(namedUser, arrayOf("user", "client-key-data")) + Utils.setValue(namedUser, token, USER_TOKEN) + Utils.removeValue(namedUser, USER_CLIENT_CERT) + Utils.removeValue(namedUser, USER_CLIENT_KEY) + Utils.removeValue(namedUser, USER_CLIENT_CERT_DATA) + Utils.removeValue(namedUser, USER_CLIENT_KEY_DATA) } protected fun setClientCertificateAuthentication( @@ -131,11 +137,11 @@ abstract class KubeConfigUpdate private constructor( clientCertPem: String, clientKeyPem: String ) { - Utils.setValue(namedUser, PemUtils.toBase64(clientCertPem), arrayOf("user", "client-certificate-data")) - Utils.setValue(namedUser, PemUtils.toBase64(clientKeyPem), arrayOf("user", "client-key-data")) - Utils.removeValue(namedUser, arrayOf("user", "client-certificate")) - Utils.removeValue(namedUser, arrayOf("user", "client-key")) - Utils.removeValue(namedUser, arrayOf("user", "token")) + Utils.setValue(namedUser, PemUtils.toBase64(clientCertPem), USER_CLIENT_CERT_DATA) + Utils.setValue(namedUser, PemUtils.toBase64(clientKeyPem), USER_CLIENT_KEY_DATA) + Utils.removeValue(namedUser, USER_CLIENT_CERT) + Utils.removeValue(namedUser, USER_CLIENT_KEY) + Utils.removeValue(namedUser, USER_TOKEN) } protected data class ContextEntries( diff --git a/src/test/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigTestHelpers.kt b/src/test/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigTestHelpers.kt index c70ceb1e..0802b9fd 100644 --- a/src/test/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigTestHelpers.kt +++ b/src/test/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigTestHelpers.kt @@ -116,7 +116,8 @@ object KubeConfigTestHelpers { } /** - * Creates a user map for testing with both token and client certificates + * Creates a user map for testing with token, data-path, and file-path client certificate fields. + * Includes both file-path and data-path fields so tests can assert cleanup of each. */ fun createUserMapWithClientCert( name: String, @@ -128,6 +129,8 @@ object KubeConfigTestHelpers { "name" to name, "user" to mutableMapOf( "token" to token, + "client-certificate" to "/path/to/cert", + "client-key" to "/path/to/key", "client-certificate-data" to PemUtils.toBase64(clientCertPem), "client-key-data" to PemUtils.toBase64(clientKeyPem) ) diff --git a/src/test/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdateTest.kt b/src/test/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdateTest.kt index 57804cb7..334f3d49 100644 --- a/src/test/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdateTest.kt +++ b/src/test/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigUpdateTest.kt @@ -504,6 +504,55 @@ class KubeConfigUpdateTest { } } + @Test + fun `#apply UpdateClientCert removes client-certificate and client-key file-path fields when setting client cert`() { + // given + val data = UpdateClientCertWithTokenTestData() + val existingUserMap = KubeConfigTestHelpers.createUserMapWithClientCert( + data.userName, + data.oldToken, + data.oldClientCertPem, + data.oldClientKeyPem + ) + val existingClusterMap = KubeConfigTestHelpers.createClusterMap(data.clusterName, data.clusterUrl) + val existingContextMap = KubeConfigTestHelpers.createContextMap(data.contextName, data.clusterName, data.userName) + val config = KubeConfigTestHelpers.createMockKubeConfig(kubeConfigPath, existingUserMap, existingClusterMap, existingContextMap) + val allConfigs = listOf(config) + val mockContext = setupUpdateExistingContextMocks(data.clusterName, data.userName, data.contextName, allConfigs, config, null) + + val update = KubeConfigUpdate.UpdateClientCert( + data.clusterName, + data.clusterUrl, + data.newClientCertPem, + data.newClientKeyPem, + mockContext, + allConfigs, + testPersisterFactory, + ) + + // when + update.apply() + + // then - file-path cert fields should be removed, only data-path fields should remain + verify { + persisterFor(kubeConfigPath).save( + any(), + any(), + match { users -> + assertThat(users).hasSize(1) + verifyUserWithClientCert( + users[0] as Map<*, *>, + data.userName, + data.newClientCertPem, + data.newClientKeyPem + ) + }, + any(), + any(), + ) + } + } + @Test fun `#apply UpdateToken sets current context on primary config when no config has current context`() { // given @@ -575,6 +624,42 @@ class KubeConfigUpdateTest { } } + @Test + fun `#apply UpdateToken removes client-certificate and client-key file-path fields when setting a new token`() { + // given + val data = UpdateTokenWithClientCertTestData() + val existingUserMap = KubeConfigTestHelpers.createUserMapWithClientCert( + data.userName, + data.oldToken, + data.clientCertPem, + data.clientKeyPem + ) + val existingClusterMap = KubeConfigTestHelpers.createClusterMap(data.clusterName, data.clusterUrl) + val existingContextMap = KubeConfigTestHelpers.createContextMap(data.contextName, data.clusterName, data.userName) + val config = KubeConfigTestHelpers.createMockKubeConfig(kubeConfigPath, existingUserMap, existingClusterMap, existingContextMap) + val allConfigs = listOf(config) + val mockContext = setupUpdateExistingContextMocks(data.clusterName, data.userName, data.contextName, allConfigs, config, null) + + val update = KubeConfigUpdate.UpdateToken(data.clusterName, data.clusterUrl, data.newToken, mockContext, allConfigs, testPersisterFactory) + + // when + update.apply() + + // then - both file-path and data-path cert fields should be removed, only token remains + verify { + persisterFor(kubeConfigPath).save( + any(), + any(), + match { users -> + assertThat(users).hasSize(1) + verifyUserWithTokenOnly(users[0] as Map<*, *>, data.userName, data.newToken) + }, + any(), + any(), + ) + } + } + @Test fun `#apply generates unique user name if user name already exists`() { // given @@ -1076,6 +1161,24 @@ class KubeConfigUpdateTest { assertThat(cert).isEqualTo(PemUtils.toBase64(expectedCertPem)) assertThat(key).isEqualTo(PemUtils.toBase64(expectedKeyPem)) assertThat(Utils.getValue(user, arrayOf("user", "token"))).isNull() + assertThat(Utils.getValue(user, arrayOf("user", "client-certificate"))).isNull() + assertThat(Utils.getValue(user, arrayOf("user", "client-key"))).isNull() + return true + } + + private fun verifyUserWithTokenOnly( + user: Map<*, *>, + expectedName: String, + expectedToken: String + ): Boolean { + val name = Utils.getValue(user, arrayOf("name")) as String + val token = Utils.getValue(user, arrayOf("user", "token")) as String + assertThat(name).isEqualTo(expectedName) + assertThat(token).isEqualTo(expectedToken) + assertThat(Utils.getValue(user, arrayOf("user", "client-certificate"))).isNull() + assertThat(Utils.getValue(user, arrayOf("user", "client-key"))).isNull() + assertThat(Utils.getValue(user, arrayOf("user", "client-certificate-data"))).isNull() + assertThat(Utils.getValue(user, arrayOf("user", "client-key-data"))).isNull() return true }