From 23eedf8c8bda55163a304171ff8ea33b6adff066 Mon Sep 17 00:00:00 2001 From: aji-aju Date: Thu, 30 Apr 2026 11:52:15 +0530 Subject: [PATCH 1/2] Fixes #21259: Don't store null secrets in Secrets Manager MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a user clears a password field on a service that uses an external Secrets Manager (managed-aws, GCP, Azure KV, Kubernetes), the previous behavior stored the literal string "null" in the secrets backing store instead of removing the secret. Downstream consumers (e.g. the Snowflake driver) then read back the four-character string "null" as a real password, breaking authentication for users who actually authenticate via private key and don't want a password configured at all. The fix is three small, layered changes: 1. ExternalSecretsManager.upsertSecret() — when the value is null/empty, the user's intent is "remove this credential", so delete the backing secret instead of writing "null". This also satisfies the GCP/K8s empty-string rejection constraint from PR #25224 by simply not calling upsert with an empty value. 2. ExternalSecretsManager.storeValue() — return null for null/empty values so the entity does not carry a stale secret:/ reference to a deleted secret. Without this, the next read would attempt to follow the reference and either get a ResourceNotFoundException or (pre-fix) the literal "null" string. 3. SecretsManager.encryptPasswordFields() — propagate the null returned by storeValue() through to the entity field instead of attempting to fernet-encrypt null. Tests: - New: testIssue21259ClearingPasswordDoesNotStoreNullString — exercises the full clear-password scenario and asserts the entity field is null, not a Fernet-wrapped secret:/ reference. - Flipped: testEncryptDatabaseConnectionWithEmptyPassword — was asserting the buggy contract; now asserts the corrected one. - Flipped: KubernetesSecretsManagerTest.testEmptySecretValueShouldNotBeStored (renamed from testEmptySecretValueShouldBeStoredAsNullString) — the previous name codified the bug; the new test verifies storeValue with empty value does NOT call createNamespacedSecret and returns null. All 80 tests in *SecretsManager*Test pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../secrets/ExternalSecretsManager.java | 21 +++++++++-- .../service/secrets/SecretsManager.java | 19 ++++++---- .../secrets/ExternalSecretsManagerTest.java | 37 ++++++++++++++++--- .../secrets/KubernetesSecretsManagerTest.java | 26 +++++-------- 4 files changed, 70 insertions(+), 33 deletions(-) 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..0282f6e12ced 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 @@ -38,6 +38,11 @@ protected String storeValue(String fieldName, String value, String secretId, boo if (store) { upsertSecret(fieldSecretId, value); } + // Issue #21259: a null/empty value means "no credential" — return null so the + // entity does not carry a stale secret:/ reference to a deleted secret. + if (Objects.isNull(value) || value.isEmpty()) { + return null; + } return SECRET_FIELD_PREFIX + fieldSecretId; } else { return value; @@ -45,12 +50,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..a172e1d08253 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,35 @@ 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 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 From 4eea0c27f6fe6d1548a378457cc5e47803af3e57 Mon Sep 17 00:00:00 2001 From: aji-aju Date: Thu, 30 Apr 2026 11:59:08 +0530 Subject: [PATCH 2/2] Address Gitar bot review: move null guard above isSecret() to prevent NPE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous version of storeValue() placed the null/empty guard AFTER the isSecret() call. Today the path is unreachable because encryptPasswordFields() upstream-guards on `obj != null`, so null never actually reaches storeValue() — but the new null-handling branch was technically dead code for the null case (isSecret() does `string.startsWith(SECRET_FIELD_PREFIX)` which NPEs on null input). Move the guard to the top so storeValue() is self-defending against null inputs from any caller, present or future. Add testStoreValueWithNullDoesNotThrowNpe regression test (runs across all ExternalSecretsManager subclasses via the abstract test class). All 83 tests in *SecretsManager*Test pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../service/secrets/ExternalSecretsManager.java | 15 ++++++++++----- .../secrets/ExternalSecretsManagerTest.java | 8 ++++++++ 2 files changed, 18 insertions(+), 5 deletions(-) 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 0282f6e12ced..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,16 +33,21 @@ 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) { upsertSecret(fieldSecretId, value); } - // Issue #21259: a null/empty value means "no credential" — return null so the - // entity does not carry a stale secret:/ reference to a deleted secret. - if (Objects.isNull(value) || value.isEmpty()) { - return null; - } return SECRET_FIELD_PREFIX + fieldSecretId; } else { return value; 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 a172e1d08253..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 @@ -242,6 +242,14 @@ void testEncryptDatabaseConnectionWithEmptyPassword() { + "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";