improve configuration for secrets usage

13 files changed, 208 insertions, 49 deletions

diff --git a/ydb/core/protos/flat_scheme_op.proto b/ydb/core/protos/flat_scheme_op.proto
index 7e3d329c39c..81b930edb53 100644
--- a/ydb/core/protos/flat_scheme_op.proto
+++ b/ydb/core/protos/flat_scheme_op.proto
@@ -851,6 +851,22 @@ message TYTSettings {
     optional bool UseTypeV3 = 8;
 };
 
+message TSecretId {
+    optional string Id = 1;
+    optional string OwnerId = 2;
+}
+
+message TSecretValue {
+    optional string Data = 1;
+}
+
+message TSecretableVariable {
+    oneof Data {
+        TSecretId SecretId = 1;
+        TSecretValue Value = 2;
+    }
+}
+
 message TS3Settings {
     enum EScheme {
         HTTP = 0;
@@ -869,6 +885,8 @@ message TS3Settings {
     optional uint32 ProxyPort = 10;
     optional EScheme ProxyScheme = 11;
     optional string Region = 12;
+    optional TSecretableVariable SecretableAccessKey = 13;
+    optional TSecretableVariable SecretableSecretKey = 14;
 
     message TLimits {
         optional uint32 ReadBatchSize = 1 [default = 8388608]; // 8 MB
diff --git a/ydb/core/tx/columnshard/columnshard_ut_common.cpp b/ydb/core/tx/columnshard/columnshard_ut_common.cpp
index 7425597b9f3..872d4df5bd1 100644
--- a/ydb/core/tx/columnshard/columnshard_ut_common.cpp
+++ b/ydb/core/tx/columnshard/columnshard_ut_common.cpp
@@ -304,10 +304,8 @@ NMetadata::NFetcher::ISnapshot::TPtr TTestSchema::BuildSnapshot(const TTableSpec
         }
         UNIT_ASSERT(tRule.GetDefaultColumn() == tier.TtlColumn);
         {
-            NColumnShard::NTiers::TTierConfig tConfig;
-            tConfig.SetTierName(tier.Name);
-            tConfig.MutableProtoConfig().SetName(tier.Name);
-            auto& cProto = tConfig.MutableProtoConfig();
+            NKikimrSchemeOp::TStorageTierConfig cProto;
+            cProto.SetName(tier.Name);
             if (tier.S3) {
                 *cProto.MutableObjectStorage() = *tier.S3;
             }
@@ -317,6 +315,7 @@ NMetadata::NFetcher::ISnapshot::TPtr TTestSchema::BuildSnapshot(const TTableSpec
             if (tier.CompressionLevel) {
                 cProto.MutableCompression()->SetCompressionLevel(*tier.CompressionLevel);
             }
+            NColumnShard::NTiers::TTierConfig tConfig(tier.Name, cProto);
             cs->MutableTierConfigs().emplace(tConfig.GetTierName(), tConfig);
         }
         tRule.AddInterval(tier.Name, TDuration::Seconds((*tier.EvictAfter).Seconds()));
diff --git a/ydb/core/tx/tiering/manager.cpp b/ydb/core/tx/tiering/manager.cpp
index 360f9974930..a64afc84e13 100644
--- a/ydb/core/tx/tiering/manager.cpp
+++ b/ydb/core/tx/tiering/manager.cpp
@@ -228,7 +228,7 @@ THashMap<ui64, NKikimr::NOlap::TTiering> TTiersManager::GetTiering() const {
                 for (auto& [name, tier] : pathTiering.TierByName) {
                     auto it = tierConfigs.find(name);
                     if (it != tierConfigs.end()) {
-                        tier->Compression = NTiers::ConvertCompression(it->second.GetProtoConfig().GetCompression());
+                        tier->Compression = NTiers::ConvertCompression(it->second.GetCompression());
                     }
                 }
             }
diff --git a/ydb/core/tx/tiering/rule/checker.cpp b/ydb/core/tx/tiering/rule/checker.cpp
index 9d6cb899567..5c7c2efb72a 100644
--- a/ydb/core/tx/tiering/rule/checker.cpp
+++ b/ydb/core/tx/tiering/rule/checker.cpp
@@ -26,11 +26,11 @@ void TRulePreparationActor::StartChecker() {
             if (!tier) {
                 Controller->OnPreparationProblem("unknown tier usage: " + interval.GetTierName());
                 return;
-            } else if (!Secrets->CheckSecretAccess(tier->GetProtoConfig().GetObjectStorage().GetAccessKey(), Context.GetUserToken())) {
-                Controller->OnPreparationProblem("no access for secret: " + tier->GetProtoConfig().GetObjectStorage().GetAccessKey());
+            } else if (!Secrets->CheckSecretAccess(tier->GetAccessKey(), Context.GetUserToken())) {
+                Controller->OnPreparationProblem("no access for secret: " + tier->GetAccessKey().DebugString());
                 return;
-            } else if (!Secrets->CheckSecretAccess(tier->GetProtoConfig().GetObjectStorage().GetSecretKey(), Context.GetUserToken())) {
-                Controller->OnPreparationProblem("no access for secret: " + tier->GetProtoConfig().GetObjectStorage().GetSecretKey());
+            } else if (!Secrets->CheckSecretAccess(tier->GetSecretKey(), Context.GetUserToken())) {
+                Controller->OnPreparationProblem("no access for secret: " + tier->GetSecretKey().DebugString());
                 return;
             }
         }
diff --git a/ydb/core/tx/tiering/tier/checker.cpp b/ydb/core/tx/tiering/tier/checker.cpp
index 46e24eeb229..e6fd1193516 100644
--- a/ydb/core/tx/tiering/tier/checker.cpp
+++ b/ydb/core/tx/tiering/tier/checker.cpp
@@ -31,11 +31,11 @@ void TTierPreparationActor::StartChecker() {
                 return;
             }
         }
-        if (!Secrets->CheckSecretAccess(tier.GetProtoConfig().GetObjectStorage().GetAccessKey(), Context.GetUserToken())) {
-            Controller->OnPreparationProblem("no access for secret: " + tier.GetProtoConfig().GetObjectStorage().GetAccessKey());
+        if (!Secrets->CheckSecretAccess(tier.GetAccessKey(), Context.GetUserToken())) {
+            Controller->OnPreparationProblem("no access for secret: " + tier.GetAccessKey().DebugString());
             return;
-        } else if (!Secrets->CheckSecretAccess(tier.GetProtoConfig().GetObjectStorage().GetSecretKey(), Context.GetUserToken())) {
-            Controller->OnPreparationProblem("no access for secret: " + tier.GetProtoConfig().GetObjectStorage().GetSecretKey());
+        } else if (!Secrets->CheckSecretAccess(tier.GetSecretKey(), Context.GetUserToken())) {
+            Controller->OnPreparationProblem("no access for secret: " + tier.GetSecretKey().DebugString());
             return;
         }
     }
diff --git a/ydb/core/tx/tiering/tier/manager.cpp b/ydb/core/tx/tiering/tier/manager.cpp
index 035f78f6650..d36efdf6749 100644
--- a/ydb/core/tx/tiering/tier/manager.cpp
+++ b/ydb/core/tx/tiering/tier/manager.cpp
@@ -21,16 +21,38 @@ NMetadata::NModifications::TOperationParsingResult TTiersManager::DoBuildPatchFr
                 if (context.GetUserToken()) {
                     defaultUserId = context.GetUserToken()->GetUserSID();
                 }
-                auto accessKey = NMetadata::NSecret::TSecretIdOrValue::DeserializeFromString(proto.GetObjectStorage().GetAccessKey(), defaultUserId);
-                if (!accessKey) {
-                    return "AccessKey is incorrect";
+
+                if (proto.GetObjectStorage().HasSecretableAccessKey()) {
+                    auto accessKey = NMetadata::NSecret::TSecretIdOrValue::DeserializeFromProto(proto.GetObjectStorage().GetSecretableAccessKey(), defaultUserId);
+                    if (!accessKey) {
+                        return "AccessKey description is incorrect";
+                    }
+                    *proto.MutableObjectStorage()->MutableSecretableAccessKey() = accessKey->SerializeToProto();
+                } else if (proto.GetObjectStorage().HasAccessKey()) {
+                    auto accessKey = NMetadata::NSecret::TSecretIdOrValue::DeserializeFromString(proto.GetObjectStorage().GetAccessKey(), defaultUserId);
+                    if (!accessKey) {
+                        return "AccessKey is incorrect";
+                    }
+                    *proto.MutableObjectStorage()->MutableAccessKey() = accessKey->SerializeToString();
+                } else {
+                    return "AccessKey not configured";
                 }
-                *proto.MutableObjectStorage()->MutableAccessKey() = accessKey->SerializeToString();
-                auto secretKey = NMetadata::NSecret::TSecretIdOrValue::DeserializeFromString(proto.GetObjectStorage().GetSecretKey(), defaultUserId);
-                if (!secretKey) {
-                    return "SecretKey is incorrect";
+
+                if (proto.GetObjectStorage().HasSecretableSecretKey()) {
+                    auto secretKey = NMetadata::NSecret::TSecretIdOrValue::DeserializeFromProto(proto.GetObjectStorage().GetSecretableSecretKey(), defaultUserId);
+                    if (!secretKey) {
+                        return "SecretKey description is incorrect";
+                    }
+                    *proto.MutableObjectStorage()->MutableSecretableSecretKey() = secretKey->SerializeToProto();
+                } else if (proto.GetObjectStorage().HasSecretKey()) {
+                    auto secretKey = NMetadata::NSecret::TSecretIdOrValue::DeserializeFromString(proto.GetObjectStorage().GetSecretKey(), defaultUserId);
+                    if (!secretKey) {
+                        return "SecretKey is incorrect";
+                    }
+                    *proto.MutableObjectStorage()->MutableSecretKey() = secretKey->SerializeToString();
+                } else {
+                    return "SecretKey not configured";
                 }
-                *proto.MutableObjectStorage()->MutableSecretKey() = secretKey->SerializeToString();
             }
             result.SetColumn(TTierConfig::TDecoder::TierConfig, NMetadata::NInternal::TYDBValue::Utf8(proto.DebugString()));
         }
diff --git a/ydb/core/tx/tiering/tier/object.cpp b/ydb/core/tx/tiering/tier/object.cpp
index ec9c93b8ce5..4e58255c8c4 100644
--- a/ydb/core/tx/tiering/tier/object.cpp
+++ b/ydb/core/tx/tiering/tier/object.cpp
@@ -49,10 +49,20 @@ NKikimrSchemeOp::TS3Settings TTierConfig::GetPatchedConfig(
 {
     auto config = ProtoConfig.GetObjectStorage();
     if (secrets) {
-        secrets->PatchString(*config.MutableAccessKey());
-        secrets->PatchString(*config.MutableSecretKey());
+        if (!secrets->GetSecretValue(GetAccessKey(), *config.MutableAccessKey())) {
+            ALS_ERROR(NKikimrServices::TX_TIERING) << "cannot read access key secret for " << GetAccessKey().DebugString();
+        }
+        if (!secrets->GetSecretValue(GetSecretKey(), *config.MutableSecretKey())) {
+            ALS_ERROR(NKikimrServices::TX_TIERING) << "cannot read secret key secret for " << GetSecretKey().DebugString();
+        }
     }
     return config;
 }
 
+NJson::TJsonValue TTierConfig::SerializeConfigToJson() const {
+    NJson::TJsonValue result;
+    NProtobufJson::Proto2Json(ProtoConfig, result);
+    return result;
+}
+
 }
diff --git a/ydb/core/tx/tiering/tier/object.h b/ydb/core/tx/tiering/tier/object.h
index 9a51d7998d5..5fe0a520efb 100644
--- a/ydb/core/tx/tiering/tier/object.h
+++ b/ydb/core/tx/tiering/tier/object.h
@@ -5,6 +5,7 @@
 #include <ydb/services/metadata/manager/table_record.h>
 #include <ydb/services/metadata/manager/object.h>
 #include <ydb/services/metadata/service.h>
+#include <ydb/services/metadata/secret/secret.h>
 
 #include <library/cpp/json/writer/json_value.h>
 
@@ -18,15 +19,45 @@ class TTierConfig: public NMetadata::NModifications::TObject<TTierConfig> {
 private:
     using TTierProto = NKikimrSchemeOp::TStorageTierConfig;
     YDB_ACCESSOR_DEF(TString, TierName);
-    YDB_ACCESSOR_DEF(TTierProto, ProtoConfig);
+    TTierProto ProtoConfig;
 public:
+
     TTierConfig() = default;
     TTierConfig(const TString& tierName)
+        : TierName(tierName) {
+
+    }
+
+    TTierConfig(const TString& tierName, const TTierProto& config)
         : TierName(tierName)
+        , ProtoConfig(config)
     {
 
     }
 
+    const NKikimrSchemeOp::TCompressionOptions& GetCompression() const {
+        return ProtoConfig.GetCompression();
+    }
+
+    NMetadata::NSecret::TSecretIdOrValue GetAccessKey() const {
+        auto accessKey = NMetadata::NSecret::TSecretIdOrValue::DeserializeFromOptional(ProtoConfig.GetObjectStorage().GetSecretableAccessKey(), ProtoConfig.GetObjectStorage().GetAccessKey());
+        if (!accessKey) {
+            return NMetadata::NSecret::TSecretIdOrValue::BuildEmpty();
+        }
+        return *accessKey;
+    }
+
+    NMetadata::NSecret::TSecretIdOrValue GetSecretKey() const {
+        auto secretKey = NMetadata::NSecret::TSecretIdOrValue::DeserializeFromOptional(ProtoConfig.GetObjectStorage().GetSecretableSecretKey(), ProtoConfig.GetObjectStorage().GetSecretKey());
+        if (!secretKey) {
+            return NMetadata::NSecret::TSecretIdOrValue::BuildEmpty();
+        }
+        return *secretKey;
+    }
+
+    NJson::TJsonValue SerializeConfigToJson() const;
+
+
     static NMetadata::IClassBehaviour::TPtr GetBehaviour();
     NKikimrSchemeOp::TS3Settings GetPatchedConfig(std::shared_ptr<NMetadata::NSecret::TSnapshot> secrets) const;
 
diff --git a/ydb/core/tx/tiering/ut/ut_tiers.cpp b/ydb/core/tx/tiering/ut/ut_tiers.cpp
index 1e66580131d..f6e592e1e8c 100644
--- a/ydb/core/tx/tiering/ut/ut_tiers.cpp
+++ b/ydb/core/tx/tiering/ut/ut_tiers.cpp
@@ -189,10 +189,10 @@ Y_UNIT_TEST_SUITE(ColumnShardTiers) {
                 NJson::TJsonValue jsonData;
                 if (i.first.StartsWith("TIER.")) {
                     auto value = snapshot->GetTierById(i.first.substr(5));
-                    NProtobufJson::Proto2Json(value->GetProtoConfig(), jsonData);
+                    jsonData = value->SerializeConfigToJson();
                 } else if (i.first.StartsWith("TIERING_RULE.")) {
                     auto value = snapshot->GetTierById(i.first.substr(13));
-                    NProtobufJson::Proto2Json(value->GetProtoConfig(), jsonData);
+                    jsonData = value->SerializeConfigToJson();
                 } else {
                     Y_VERIFY(false);
                 }
@@ -420,7 +420,12 @@ Y_UNIT_TEST_SUITE(ColumnShardTiers) {
         ObjectStorage : {
             Endpoint: "fake"
             Bucket: "fake"
-            AccessKey: "USId:root@builtin:secretAccessKey"
+            SecretableAccessKey: {
+                SecretId: {
+                    Id: "secretAccessKey"
+                    OwnerId: "root@builtin"
+                }
+            }
             SecretKey: "SId:secretSecretKey"
         }
     )";
diff --git a/ydb/services/metadata/secret/secret.cpp b/ydb/services/metadata/secret/secret.cpp
index 7cc57bcfc64..ec447c1d0ed 100644
--- a/ydb/services/metadata/secret/secret.cpp
+++ b/ydb/services/metadata/secret/secret.cpp
@@ -3,6 +3,7 @@
 #include "secret_behaviour.h"
 #include <ydb/core/base/appdata.h>
 #include <ydb/services/metadata/manager/ydb_value_operator.h>
+#include <library/cpp/digest/md5/md5.h>
 
 namespace NKikimr::NMetadata::NSecret {
 
@@ -37,4 +38,14 @@ TString TSecretId::SerializeToString() const {
     return sb;
 }
 
+
+TString TSecretIdOrValue::DebugString() const {
+    if (SecretId) {
+        return SecretId->SerializeToString();
+    } else if (Value) {
+        return MD5::Calc(*Value);
+    }
+    return "";
+}
+
 }
diff --git a/ydb/services/metadata/secret/secret.h b/ydb/services/metadata/secret/secret.h
index 54a84373613..f0d35df9ce3 100644
--- a/ydb/services/metadata/secret/secret.h
+++ b/ydb/services/metadata/secret/secret.h
@@ -76,17 +76,75 @@ private:
         }
         return true;
     }
-public:
-    TSecretIdOrValue(const TSecretId& id)
+    explicit TSecretIdOrValue(const TSecretId& id)
         : SecretId(id) {
 
     }
 
-    TSecretIdOrValue(const TString& value)
+    explicit TSecretIdOrValue(const TString& value)
         : Value(value) {
 
     }
 
+public:
+    bool operator!() const {
+        return !Value && !SecretId;
+    }
+
+    static TSecretIdOrValue BuildAsValue(const TString& value) {
+        return TSecretIdOrValue(value);
+    }
+
+    static TSecretIdOrValue BuildEmpty() {
+        return TSecretIdOrValue();
+    }
+
+    static TSecretIdOrValue BuildAsId(const TSecretId& id) {
+        return TSecretIdOrValue(id);
+    }
+
+    static std::optional<TSecretIdOrValue> DeserializeFromOptional(const NKikimrSchemeOp::TSecretableVariable& proto, const TString& secretInfo, const TString& defaultOwnerId = Default<TString>()) {
+        if (proto.HasSecretId()) {
+            return DeserializeFromProto(proto, defaultOwnerId);
+        } else if (secretInfo) {
+            return DeserializeFromString(secretInfo, defaultOwnerId);
+        } else {
+            return {};
+        }
+    }
+
+    NKikimrSchemeOp::TSecretableVariable SerializeToProto() const {
+        NKikimrSchemeOp::TSecretableVariable result;
+        if (SecretId) {
+            result.MutableSecretId()->SetId(SecretId->GetSecretId());
+            result.MutableSecretId()->SetOwnerId(SecretId->GetOwnerUserId());
+        } else if (Value) {
+            result.MutableValue()->SetData(*Value);
+        }
+        return result;
+    }
+
+    static std::optional<TSecretIdOrValue> DeserializeFromProto(const NKikimrSchemeOp::TSecretableVariable& proto, const TString& defaultOwnerId = Default<TString>()) {
+        if (proto.HasSecretId()) {
+            TString ownerId;
+            TString secretId;
+            if (!proto.GetSecretId().HasOwnerId() || !proto.GetSecretId().GetOwnerId()) {
+                ownerId = defaultOwnerId;
+            } else {
+                ownerId = proto.GetSecretId().GetOwnerId();
+            }
+            secretId = proto.GetSecretId().GetId();
+            if (!ownerId || !secretId) {
+                return {};
+            }
+            return TSecretIdOrValue::BuildAsId(TSecretId(ownerId, secretId));
+        } else if (proto.HasValue()) {
+            return TSecretIdOrValue::BuildAsValue(proto.GetValue().GetData());
+        } else {
+            return {};
+        }
+    }
+
     static std::optional<TSecretIdOrValue> DeserializeFromString(const TString& info, const TString& defaultOwnerId = Default<TString>()) {
         TSecretIdOrValue result;
         if (!result.DeserializeFromStringImpl(info, defaultOwnerId)) {
@@ -102,9 +160,10 @@ public:
         } else if (Value) {
             return *Value;
         }
-        Y_VERIFY(false);
         return "";
     }
+
+    TString DebugString() const;
 };
 
 class TSecret: public TSecretId, public NModifications::TObject<TSecret> {
diff --git a/ydb/services/metadata/secret/snapshot.cpp b/ydb/services/metadata/secret/snapshot.cpp
index 6cb42d9a3a2..1e0f8cf0dec 100644
--- a/ydb/services/metadata/secret/snapshot.cpp
+++ b/ydb/services/metadata/secret/snapshot.cpp
@@ -27,30 +27,20 @@ bool TSnapshot::PatchString(TString& stringForPath) const {
     if (!sId) {
         return false;
     }
-    if (sId->GetValue()) {
-        return true;
-    }
-    auto it = Secrets.find(*sId->GetSecretId());
-    if (it == Secrets.end()) {
-        return false;
-    }
-    stringForPath = it->second.GetValue();
-    return true;
+    return GetSecretValue(*sId, stringForPath);
 }
 
-bool TSnapshot::CheckSecretAccess(const TString& secretableString, const std::optional<NACLib::TUserToken>& userToken) const {
-    if (!userToken) {
+bool TSnapshot::CheckSecretAccess(const TSecretIdOrValue& sIdOrValue, const std::optional<NACLib::TUserToken>& userToken) const {
+    if (!userToken || !sIdOrValue) {
         return true;
     }
-    std::optional<TSecretIdOrValue> sIdOrValue = TSecretIdOrValue::DeserializeFromString(secretableString);
-    if (!sIdOrValue) {
-        return false;
-    } else if (sIdOrValue->GetValue()) {
+    if (sIdOrValue.GetValue()) {
         return true;
-    } else if (!sIdOrValue->GetSecretId()) {
+    }
+    if (!sIdOrValue.GetSecretId()) {
         return false;
     }
-    const auto sId = *sIdOrValue->GetSecretId();
+    const auto sId = *sIdOrValue.GetSecretId();
     auto it = Secrets.find(sId);
     if (it == Secrets.end()) {
         return false;
@@ -69,4 +59,17 @@ bool TSnapshot::CheckSecretAccess(const TString& secretableString, const std::op
     return false;
 }
 
+bool TSnapshot::GetSecretValue(const TSecretIdOrValue& sId, TString& result) const {
+    if (sId.GetValue()) {
+        result = *sId.GetValue();
+        return true;
+    }
+    auto it = Secrets.find(*sId.GetSecretId());
+    if (it == Secrets.end()) {
+        return false;
+    }
+    result = it->second.GetValue();
+    return true;
+}
+
 }
diff --git a/ydb/services/metadata/secret/snapshot.h b/ydb/services/metadata/secret/snapshot.h
index 62b54afc867..9acff259c7a 100644
--- a/ydb/services/metadata/secret/snapshot.h
+++ b/ydb/services/metadata/secret/snapshot.h
@@ -18,8 +18,9 @@ protected:
     virtual TString DoSerializeToString() const override;
 public:
     using TBase::TBase;
-    bool CheckSecretAccess(const TString& secretableString, const std::optional<NACLib::TUserToken>& userToken) const;
+    bool CheckSecretAccess(const TSecretIdOrValue& sIdOrValue, const std::optional<NACLib::TUserToken>& userToken) const;
     bool PatchString(TString& stringForPath) const;
+    bool GetSecretValue(const TSecretIdOrValue& secretId, TString& result) const;
 };
 
 }