Fixes #21259: Don't store null secrets in Secrets Manager

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();
     }
   }

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);
                   }
                 });
       }

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<String, Map<String, String>> withPassword =
+        Map.of("authType", Map.of("password", "initial-password"));
+    secretsManager.encryptServiceConnectionConfig(
+        withPassword, Mysql.value(), serviceName, ServiceType.DATABASE);
+
+    Map<String, Map<String, String>> 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'.");
   }
 }

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<V1Secret> 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<String, byte[]> 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