diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/ExternalSecretsManager.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/ExternalSecretsManager.java index 8819de5dd835..856a75357abe 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/ExternalSecretsManager.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/ExternalSecretsManager.java @@ -33,6 +33,16 @@ protected ExternalSecretsManager( @Override protected String storeValue(String fieldName, String value, String secretId, boolean store) { String fieldSecretId = buildSecretId(false, secretId, fieldName.toLowerCase(Locale.ROOT)); + // Issue #21259: a null/empty value means "no credential". Handle this case BEFORE + // calling isSecret() — isSecret() invokes startsWith() on the value and would NPE + // on null. Returning null here ensures the entity does not carry a stale secret:/ + // reference to a deleted secret. + if (Objects.isNull(value) || value.isEmpty()) { + if (store) { + upsertSecret(fieldSecretId, value); + } + return null; + } // check if value does not start with 'config:' only String can have password annotation if (Boolean.FALSE.equals(isSecret(value))) { if (store) { @@ -45,12 +55,22 @@ protected String storeValue(String fieldName, String value, String secretId, boo } public void upsertSecret(String secretName, String secretValue) { - String sanitizedValue = cleanNullOrEmpty(secretValue); + // Issue #21259: when the value is null/empty, the user's intent is "remove this + // credential" — delete the backing secret instead of storing the literal string + // "null". This also satisfies the GCP/Kubernetes empty-string rejection constraint + // (PR #25224) by simply not calling upsert with an empty value. + if (Objects.isNull(secretValue) || secretValue.isEmpty()) { + if (existSecret(secretName)) { + deleteSecretInternal(secretName); + sleep(); + } + return; + } if (existSecret(secretName)) { - updateSecret(secretName, sanitizedValue); + updateSecret(secretName, secretValue); sleep(); } else { - storeSecret(secretName, sanitizedValue); + storeSecret(secretName, secretValue); sleep(); } } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java index 2732aa8bd1c8..5062c88a189c 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/secrets/SecretsManager.java @@ -411,13 +411,18 @@ private Object encryptPasswordFields(Object toEncryptObject, String secretId, bo fieldName, fernet.decryptIfApplies((String) obj), secretId, store); // get setMethod Method toSet = ReflectionUtil.getToSetMethod(toEncryptObject, obj, fieldName); - // set new value - ReflectionUtil.setValueInMethod( - toEncryptObject, - Fernet.isTokenized(newFieldValue) - ? newFieldValue - : store ? fernet.encrypt(newFieldValue) : newFieldValue, - toSet); + // Issue #21259: storeValue returns null when the user clears the + // password field. Propagate that null through instead of attempting + // to fernet-encrypt it. + String finalValue; + if (newFieldValue == null) { + finalValue = null; + } else if (Fernet.isTokenized(newFieldValue)) { + finalValue = newFieldValue; + } else { + finalValue = store ? fernet.encrypt(newFieldValue) : newFieldValue; + } + ReflectionUtil.setValueInMethod(toEncryptObject, finalValue, toSet); } }); } diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/ExternalSecretsManagerTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/ExternalSecretsManagerTest.java index 275c85dbb2c0..03e2f6396f36 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/ExternalSecretsManagerTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/ExternalSecretsManagerTest.java @@ -1,9 +1,9 @@ package org.openmetadata.service.secrets; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.openmetadata.schema.api.services.CreateDatabaseService.DatabaseServiceType.Mysql; import java.util.Map; @@ -236,10 +236,43 @@ void testEncryptDatabaseConnectionWithEmptyPassword() { mysqlConnection, Mysql.value(), "test-empty-password", ServiceType.DATABASE); assertNotNull(actualConnection, "Encryption should succeed even with empty password"); - assertFalse( - JsonUtils.convertValue(actualConnection.getAuthType(), basicAuth.class) - .getPassword() - .isEmpty(), - "Empty password should be converted to non-empty encrypted value"); + assertNull( + JsonUtils.convertValue(actualConnection.getAuthType(), basicAuth.class).getPassword(), + "Issue #21259: Empty password should resolve to null on the entity, " + + "not a Fernet-wrapped reference to a 'null' string in the secrets manager"); + } + + @Test + void testStoreValueWithNullDoesNotThrowNpe() { + // Defensive: storeValue must handle null directly — isSecret(null) would NPE, + // so the null guard has to come first. (Caught in PR review by Gitar bot.) + String result = secretsManager.storeValue("password", null, "test-null-input", true); + assertNull(result, "storeValue with null value should return null without throwing"); + } + + @Test + void testIssue21259ClearingPasswordDoesNotStoreNullString() { + String serviceName = "bug21259-clear-password"; + + Map> withPassword = + Map.of("authType", Map.of("password", "initial-password")); + secretsManager.encryptServiceConnectionConfig( + withPassword, Mysql.value(), serviceName, ServiceType.DATABASE); + + Map> clearedPassword = + Map.of("authType", Map.of("password", StringUtils.EMPTY)); + MysqlConnection cleared = + (MysqlConnection) + secretsManager.encryptServiceConnectionConfig( + clearedPassword, Mysql.value(), serviceName, ServiceType.DATABASE); + + String storedPassword = + JsonUtils.convertValue(cleared.getAuthType(), basicAuth.class).getPassword(); + assertNull( + storedPassword, + "Issue #21259: When a user clears a password field, the entity should " + + "carry null, not a stale secret:/ reference to a 'null'-valued secret. " + + "The downstream consumer (e.g. Snowflake driver) reads the password " + + "verbatim and breaks auth when it gets the literal string 'null'."); } } diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/KubernetesSecretsManagerTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/KubernetesSecretsManagerTest.java index 7325e16acd0c..4d640fdcc126 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/secrets/KubernetesSecretsManagerTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/secrets/KubernetesSecretsManagerTest.java @@ -21,6 +21,7 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.reset; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -261,27 +262,18 @@ void testBuildSecretIdHandlesSpecialCharacters() throws ApiException { } @Test - void testEmptySecretValueShouldBeStoredAsNullString() throws ApiException { - ArgumentCaptor secretCaptor = ArgumentCaptor.forClass(V1Secret.class); + void testEmptySecretValueShouldNotBeStored() throws ApiException { when(mockApiClient.readNamespacedSecret(anyString(), eq(NAMESPACE))).thenReturn(readRequest); when(readRequest.execute()).thenThrow(new ApiException(404, "Not Found")); - when(mockApiClient.createNamespacedSecret(eq(NAMESPACE), any(V1Secret.class))) - .thenReturn(createRequest); - when(createRequest.execute()).thenReturn(new V1Secret()); - - secretsManager.storeValue("field", "", SECRET_ID, true); - - verify(mockApiClient).createNamespacedSecret(eq(NAMESPACE), secretCaptor.capture()); - verify(createRequest).execute(); + String returned = secretsManager.storeValue("field", "", SECRET_ID, true); - V1Secret createdSecret = secretCaptor.getValue(); - Map data = createdSecret.getData(); - assert data != null; - assertEquals( - ExternalSecretsManager.NULL_SECRET_STRING, - new String(data.get("value"), StandardCharsets.UTF_8), - "Empty string should be stored as 'null' to prevent secrets manager rejection"); + // Issue #21259: empty value means "no credential" — storeValue must NOT call create, + // and must return null so the entity does not carry a stale secret:/ reference. + // This also satisfies the original GCP/Kubernetes empty-string rejection constraint + // (PR #25224) by simply not upserting an empty value at all. + verify(mockApiClient, never()).createNamespacedSecret(eq(NAMESPACE), any(V1Secret.class)); + assertNull(returned, "storeValue with empty value should return null, not a secret reference"); } @Test