From 0fb6375de088727cf524a4f76ba2ab0e1bdcf11f Mon Sep 17 00:00:00 2001
From: JinYang <130516674+gohalo@users.noreply.github.com>
Date: Wed, 6 Mar 2024 14:16:54 +0800
Subject: [PATCH] [BugFix] mask secret parameters when create storage volume
 failed(#41975) (#41975)

When create storage volume with invalid parameters, the sql and log will dump some error message which wasn't masked.
Just replace the secrete with mask.

Signed-off-by: GoHalo <gohalo@163.com>
---
 .../starrocks/storagevolume/StorageVolume.java | 18 ++++++++++--------
 .../storagevolume/StorageVolumeTest.java       | 15 +++++++++++++++
 2 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/fe/fe-core/src/main/java/com/starrocks/storagevolume/StorageVolume.java b/fe/fe-core/src/main/java/com/starrocks/storagevolume/StorageVolume.java
index d62aeece1a7ad..7a470cb767223 100644
--- a/fe/fe-core/src/main/java/com/starrocks/storagevolume/StorageVolume.java
+++ b/fe/fe-core/src/main/java/com/starrocks/storagevolume/StorageVolume.java
@@ -82,6 +82,13 @@ public enum StorageVolumeType {
 
     public static String CREDENTIAL_MASK = "******";
 
+    private String dumpMaskedParams(Map<String, String> params) {
+        Gson gson = new Gson();
+        Map<String, String> maskedParams = new HashMap<>(params);
+        addMaskForCredential(maskedParams);
+        return gson.toJson(maskedParams);
+    }
+
     public StorageVolume(String id, String name, String svt, List<String> locations,
                          Map<String, String> params, boolean enabled, String comment) throws DdlException {
         this.id = id;
@@ -95,8 +102,7 @@ public StorageVolume(String id, String name, String svt, List<String> locations,
         preprocessAuthenticationIfNeeded(configurationParams);
         this.cloudConfiguration = CloudConfigurationFactory.buildCloudConfigurationForStorage(configurationParams, true);
         if (!isValidCloudConfiguration()) {
-            Gson gson = new Gson();
-            throw new SemanticException("Storage params is not valid " + gson.toJson(params));
+            throw new SemanticException("Storage params is not valid " + dumpMaskedParams(params));
         }
         validateStorageVolumeConstraints();
     }
@@ -136,8 +142,7 @@ public void setCloudConfiguration(Map<String, String> params) {
         newParams.putAll(params);
         this.cloudConfiguration = CloudConfigurationFactory.buildCloudConfigurationForStorage(newParams, true);
         if (!isValidCloudConfiguration()) {
-            Gson gson = new Gson();
-            throw new SemanticException("Storage params is not valid " + gson.toJson(newParams));
+            throw new SemanticException("Storage params is not valid " + dumpMaskedParams(newParams));
         }
         this.params = newParams;
     }
@@ -211,15 +216,12 @@ public static void addMaskForCredential(Map<String, String> params) {
     }
 
     public void getProcNodeData(BaseProcResult result) {
-        Gson gson = new Gson();
-        Map<String, String> p = new HashMap<>(params);
-        addMaskForCredential(p);
         result.addRow(Lists.newArrayList(name,
                 svt.name(),
                 String.valueOf(GlobalStateMgr.getCurrentState().getStorageVolumeMgr()
                         .getDefaultStorageVolumeId().equals(id)),
                 Joiner.on(", ").join(locations),
-                String.valueOf(gson.toJson(p)),
+                dumpMaskedParams(params),
                 String.valueOf(enabled),
                 String.valueOf(comment)));
     }
diff --git a/fe/fe-core/src/test/java/com/starrocks/storagevolume/StorageVolumeTest.java b/fe/fe-core/src/test/java/com/starrocks/storagevolume/StorageVolumeTest.java
index 9e7297ad97982..bd987679dbb7d 100644
--- a/fe/fe-core/src/test/java/com/starrocks/storagevolume/StorageVolumeTest.java
+++ b/fe/fe-core/src/test/java/com/starrocks/storagevolume/StorageVolumeTest.java
@@ -52,6 +52,7 @@
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
 
@@ -580,4 +581,18 @@ public void testAddMaskForCredential() {
         Assert.assertEquals(StorageVolume.CREDENTIAL_MASK, storageParams.get(AZURE_BLOB_SAS_TOKEN));
         Assert.assertEquals(StorageVolume.CREDENTIAL_MASK, storageParams.get(AZURE_BLOB_SHARED_KEY));
     }
+
+    @Test
+    public void testAddMaskInvalidForInvalidCredential() {
+        String awsSecretKey = "SomeAWSSecretKey";
+        Map<String, String> storageParams = new HashMap<>();
+        storageParams.put(AWS_S3_ACCESS_KEY, "accessKey");
+        storageParams.put(AWS_S3_SECRET_KEY, awsSecretKey);
+        storageParams.put(AWS_S3_ENDPOINT, "endpoint");
+        Exception exception = Assert.assertThrows(SemanticException.class, () -> new StorageVolume(
+                "1", "test", "obs", Collections.singletonList("s3://foobar"), storageParams, true, ""
+        ));
+        Assert.assertFalse(exception.getMessage().contains(awsSecretKey));
+        Assert.assertTrue(exception.getMessage().contains(StorageVolume.CREDENTIAL_MASK));
+    }
 }