From 35baf4d74551726df3d78dfef3006482a7e1937c Mon Sep 17 00:00:00 2001 From: Sagar Vemuri Date: Tue, 26 Apr 2022 09:28:38 -0700 Subject: [PATCH 1/5] Validate commit request tenant prefixes on commit proxy --- .../sphinx/source/api-error-codes.rst | 2 + fdbserver/CommitProxyServer.actor.cpp | 112 +++++++++++++----- flow/error_definitions.h | 1 + 3 files changed, 86 insertions(+), 29 deletions(-) diff --git a/documentation/sphinx/source/api-error-codes.rst b/documentation/sphinx/source/api-error-codes.rst index 7c06dbb6df..7dac09bd9c 100644 --- a/documentation/sphinx/source/api-error-codes.rst +++ b/documentation/sphinx/source/api-error-codes.rst @@ -176,6 +176,8 @@ FoundationDB may return the following error codes from API functions. If you nee +-----------------------------------------------+-----+--------------------------------------------------------------------------------+ | unknown_tenant | 2137| Tenant is not available from this server | +-----------------------------------------------+-----+--------------------------------------------------------------------------------+ +| illegal_tenant_access | 2138| Tenant information mismatch | ++-----------------------------------------------+-----+--------------------------------------------------------------------------------+ | api_version_unset | 2200| API version is not set | +-----------------------------------------------+-----+--------------------------------------------------------------------------------+ | api_version_already_set | 2201| API version may be set only once | diff --git a/fdbserver/CommitProxyServer.actor.cpp b/fdbserver/CommitProxyServer.actor.cpp index 097c6f632f..1a9d5d8706 100644 --- a/fdbserver/CommitProxyServer.actor.cpp +++ b/fdbserver/CommitProxyServer.actor.cpp @@ -28,6 +28,7 @@ #include "fdbclient/Knobs.h" #include "fdbclient/CommitProxyInterface.h" #include "fdbclient/NativeAPI.actor.h" +#include "fdbclient/StorageServerInterface.h" #include "fdbclient/SystemData.h" #include "fdbclient/TransactionLineage.h" #include "fdbrpc/sim_validation.h" @@ -236,6 +237,80 @@ struct ResolutionRequestBuilder { } }; +ErrorOr> getTenantEntry(ProxyCommitData* commitData, + Optional tenant, + Optional tenantId, + bool logOnFailure) { + if (tenant.present()) { + auto itr = commitData->tenantMap.find(tenant.get()); + if (itr == commitData->tenantMap.end()) { + if (logOnFailure) { + TraceEvent(SevWarn, "CommitProxyUnknownTenant", commitData->dbgid).detail("Tenant", tenant.get()); + } + + return unknown_tenant(); + } else if (tenantId.present() && tenantId.get() != itr->second.id) { + if (logOnFailure) { + TraceEvent(SevWarn, "CommitProxyTenantIdMismatch", commitData->dbgid) + .detail("Tenant", tenant.get()) + .detail("TenantId", tenantId) + .detail("ExistingId", itr->second.id); + } + + return unknown_tenant(); + } + + return ErrorOr>(Optional(itr->second)); + } + + return Optional(); +} + +bool verifyPrefix(ProxyCommitData* const commitData, const CommitTransactionRequest& req) { + ErrorOr> tenantEntry = + getTenantEntry(commitData, req.tenantInfo.name.castTo(), req.tenantInfo.tenantId, true); + + if (tenantEntry.isError()) { + return true; + } + + auto te = tenantEntry.get(); + if (te.present()) { + Key tenantPrefix = te.get().prefix; + + // auto& tenantPrefix = tenantEntry.get().get().prefix; + for (auto& m : req.transaction.mutations) { + if (m.param1 != metadataVersionKey) { + if (!m.param1.startsWith(tenantPrefix)) { + return false; + } + if (m.type == MutationRef::ClearRange && !m.param2.startsWith(tenantPrefix)) { + return false; + } else if (m.type != MutationRef::SetVersionstampedKey) { + // TODO: How should this be handled? + // Maybe skip checking this for now? If so, fall-through. + } + } + } + + for (auto& rc : req.transaction.read_conflict_ranges) { + if (rc.begin != metadataVersionKey && + (!rc.begin.startsWith(tenantPrefix) || !rc.end.startsWith(tenantPrefix))) { + return false; + } + } + + for (auto& wc : req.transaction.write_conflict_ranges) { + if (wc.begin != metadataVersionKey && + (!wc.begin.startsWith(tenantPrefix) || !wc.end.startsWith(tenantPrefix))) { + return false; + } + } + } + + return true; +} + ACTOR Future commitBatcher(ProxyCommitData* commitData, PromiseStream, int>> out, FutureStream in, @@ -282,6 +357,14 @@ ACTOR Future commitBatcher(ProxyCommitData* commitData, .detail("Size", bytes) .detail("Client", req.reply.getEndpoint().getPrimaryAddress()); } + + if (!verifyPrefix(commitData, req)) { + ++commitData->stats.txnCommitErrors; + req.reply.sendError(illegal_tenant_access()); + TraceEvent(SevWarnAlways, "TenantPrefixMismatch").suppressFor(60); + continue; + } + ++commitData->stats.txnCommitIn; if (req.debugID.present()) { @@ -450,35 +533,6 @@ ACTOR static Future trackResolutionMetrics(Referen return reply; } -ErrorOr> getTenantEntry(ProxyCommitData* commitData, - Optional tenant, - Optional tenantId, - bool logOnFailure) { - if (tenant.present()) { - auto itr = commitData->tenantMap.find(tenant.get()); - if (itr == commitData->tenantMap.end()) { - if (logOnFailure) { - TraceEvent(SevWarn, "CommitProxyUnknownTenant", commitData->dbgid).detail("Tenant", tenant.get()); - } - - return unknown_tenant(); - } else if (tenantId.present() && tenantId.get() != itr->second.id) { - if (logOnFailure) { - TraceEvent(SevWarn, "CommitProxyTenantIdMismatch", commitData->dbgid) - .detail("Tenant", tenant.get()) - .detail("TenantId", tenantId) - .detail("ExistingId", itr->second.id); - } - - return unknown_tenant(); - } - - return ErrorOr>(Optional(itr->second)); - } - - return Optional(); -} - namespace CommitBatch { struct CommitBatchContext { diff --git a/flow/error_definitions.h b/flow/error_definitions.h index a80bd2d3a8..e24d840602 100755 --- a/flow/error_definitions.h +++ b/flow/error_definitions.h @@ -226,6 +226,7 @@ ERROR( invalid_tenant_name, 2134, "Tenant name cannot begin with \\xff"); ERROR( tenant_prefix_allocator_conflict, 2135, "The database already has keys stored at the prefix allocated for the tenant"); ERROR( tenants_disabled, 2136, "Tenants have been disabled in the cluster"); ERROR( unknown_tenant, 2137, "Tenant is not available from this server") +ERROR( illegal_tenant_access, 2138, "Tenant information mismatch") // 2200 - errors from bindings and official APIs ERROR( api_version_unset, 2200, "API version is not set" ) From fd993df807aa5c8f3811d00e5dca7564ad5e4696 Mon Sep 17 00:00:00 2001 From: Sagar Vemuri Date: Tue, 26 Apr 2022 13:52:38 -0700 Subject: [PATCH 2/5] Minor cleanup --- fdbserver/CommitProxyServer.actor.cpp | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/fdbserver/CommitProxyServer.actor.cpp b/fdbserver/CommitProxyServer.actor.cpp index 1a9d5d8706..2b8a6079de 100644 --- a/fdbserver/CommitProxyServer.actor.cpp +++ b/fdbserver/CommitProxyServer.actor.cpp @@ -274,11 +274,8 @@ bool verifyPrefix(ProxyCommitData* const commitData, const CommitTransactionRequ return true; } - auto te = tenantEntry.get(); - if (te.present()) { - Key tenantPrefix = te.get().prefix; - - // auto& tenantPrefix = tenantEntry.get().get().prefix; + if (tenantEntry.get().present()) { + Key tenantPrefix = tenantEntry.get().get().prefix; for (auto& m : req.transaction.mutations) { if (m.param1 != metadataVersionKey) { if (!m.param1.startsWith(tenantPrefix)) { From ed60afc96435bdf15231cf03415e178ba0ad36ef Mon Sep 17 00:00:00 2001 From: Sagar Vemuri Date: Wed, 27 Apr 2022 11:12:01 -0700 Subject: [PATCH 3/5] Handle versionstamped keys, and include additonal trace information --- .../sphinx/source/api-error-codes.rst | 2 -- fdbserver/CommitProxyServer.actor.cpp | 36 ++++++++++++++++--- flow/error_definitions.h | 2 +- 3 files changed, 33 insertions(+), 7 deletions(-) diff --git a/documentation/sphinx/source/api-error-codes.rst b/documentation/sphinx/source/api-error-codes.rst index 7dac09bd9c..7c06dbb6df 100644 --- a/documentation/sphinx/source/api-error-codes.rst +++ b/documentation/sphinx/source/api-error-codes.rst @@ -176,8 +176,6 @@ FoundationDB may return the following error codes from API functions. If you nee +-----------------------------------------------+-----+--------------------------------------------------------------------------------+ | unknown_tenant | 2137| Tenant is not available from this server | +-----------------------------------------------+-----+--------------------------------------------------------------------------------+ -| illegal_tenant_access | 2138| Tenant information mismatch | -+-----------------------------------------------+-----+--------------------------------------------------------------------------------+ | api_version_unset | 2200| API version is not set | +-----------------------------------------------+-----+--------------------------------------------------------------------------------+ | api_version_already_set | 2201| API version may be set only once | diff --git a/fdbserver/CommitProxyServer.actor.cpp b/fdbserver/CommitProxyServer.actor.cpp index 2b8a6079de..2ced6949ef 100644 --- a/fdbserver/CommitProxyServer.actor.cpp +++ b/fdbserver/CommitProxyServer.actor.cpp @@ -279,13 +279,32 @@ bool verifyPrefix(ProxyCommitData* const commitData, const CommitTransactionRequ for (auto& m : req.transaction.mutations) { if (m.param1 != metadataVersionKey) { if (!m.param1.startsWith(tenantPrefix)) { + TraceEvent(SevWarnAlways, "TenantPrefixMismatch") + .suppressFor(60) + .detail("Prefix", tenantPrefix.toHexString()) + .detail("Key", m.param1.toHexString()) + .detail("KeyType", "Key"); return false; } + if (m.type == MutationRef::ClearRange && !m.param2.startsWith(tenantPrefix)) { + TraceEvent(SevWarnAlways, "TenantPrefixMismatch") + .suppressFor(60) + .detail("Prefix", tenantPrefix.toHexString()) + .detail("Key", m.param2.toHexString()) + .detail("KeyType", "ClearRangeKey"); return false; - } else if (m.type != MutationRef::SetVersionstampedKey) { - // TODO: How should this be handled? - // Maybe skip checking this for now? If so, fall-through. + } else if (m.type == MutationRef::SetVersionstampedKey) { + uint8_t* key = const_cast(m.param1.begin()); + int* offset = reinterpret_cast(&key[m.param1.size() - 4]); + if (*offset <= tenantPrefix.size()) { + TraceEvent(SevWarnAlways, "TenantPrefixMismatch") + .suppressFor(60) + .detail("Prefix", tenantPrefix.toHexString()) + .detail("Key", m.param1.toHexString()) + .detail("KeyType", "VersionstampedKey"); + return false; + } } } } @@ -293,6 +312,11 @@ bool verifyPrefix(ProxyCommitData* const commitData, const CommitTransactionRequ for (auto& rc : req.transaction.read_conflict_ranges) { if (rc.begin != metadataVersionKey && (!rc.begin.startsWith(tenantPrefix) || !rc.end.startsWith(tenantPrefix))) { + TraceEvent(SevWarnAlways, "TenantPrefixMismatch") + .suppressFor(60) + .detail("Prefix", tenantPrefix.toHexString()) + .detail("BeginKey", rc.begin.toHexString()) + .detail("EndKey", rc.end.toHexString()); return false; } } @@ -300,6 +324,11 @@ bool verifyPrefix(ProxyCommitData* const commitData, const CommitTransactionRequ for (auto& wc : req.transaction.write_conflict_ranges) { if (wc.begin != metadataVersionKey && (!wc.begin.startsWith(tenantPrefix) || !wc.end.startsWith(tenantPrefix))) { + TraceEvent(SevWarnAlways, "TenantPrefixMismatch") + .suppressFor(60) + .detail("Prefix", tenantPrefix.toHexString()) + .detail("BeginKey", wc.begin.toHexString()) + .detail("EndKey", wc.end.toHexString()); return false; } } @@ -358,7 +387,6 @@ ACTOR Future commitBatcher(ProxyCommitData* commitData, if (!verifyPrefix(commitData, req)) { ++commitData->stats.txnCommitErrors; req.reply.sendError(illegal_tenant_access()); - TraceEvent(SevWarnAlways, "TenantPrefixMismatch").suppressFor(60); continue; } diff --git a/flow/error_definitions.h b/flow/error_definitions.h index e24d840602..7147164749 100755 --- a/flow/error_definitions.h +++ b/flow/error_definitions.h @@ -226,7 +226,7 @@ ERROR( invalid_tenant_name, 2134, "Tenant name cannot begin with \\xff"); ERROR( tenant_prefix_allocator_conflict, 2135, "The database already has keys stored at the prefix allocated for the tenant"); ERROR( tenants_disabled, 2136, "Tenants have been disabled in the cluster"); ERROR( unknown_tenant, 2137, "Tenant is not available from this server") -ERROR( illegal_tenant_access, 2138, "Tenant information mismatch") +ERROR( illegal_tenant_access, 2138, "Illegal tenant access") // 2200 - errors from bindings and official APIs ERROR( api_version_unset, 2200, "API version is not set" ) From d013ff7457faa2e9e729d9bfd33eb0d909518ab5 Mon Sep 17 00:00:00 2001 From: Sagar Vemuri Date: Wed, 27 Apr 2022 11:27:19 -0700 Subject: [PATCH 4/5] Remove unnecessary include --- fdbserver/CommitProxyServer.actor.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/fdbserver/CommitProxyServer.actor.cpp b/fdbserver/CommitProxyServer.actor.cpp index 2ced6949ef..70e998e6cf 100644 --- a/fdbserver/CommitProxyServer.actor.cpp +++ b/fdbserver/CommitProxyServer.actor.cpp @@ -28,7 +28,6 @@ #include "fdbclient/Knobs.h" #include "fdbclient/CommitProxyInterface.h" #include "fdbclient/NativeAPI.actor.h" -#include "fdbclient/StorageServerInterface.h" #include "fdbclient/SystemData.h" #include "fdbclient/TransactionLineage.h" #include "fdbrpc/sim_validation.h" From ed0732a012dc199c6cd14f0dfc81c97a4461536c Mon Sep 17 00:00:00 2001 From: Sagar Vemuri Date: Wed, 27 Apr 2022 15:20:41 -0700 Subject: [PATCH 5/5] Address review comments: Update TraceEvents and an assert in versionstampkey check --- fdbserver/CommitProxyServer.actor.cpp | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/fdbserver/CommitProxyServer.actor.cpp b/fdbserver/CommitProxyServer.actor.cpp index 70e998e6cf..a931566ac7 100644 --- a/fdbserver/CommitProxyServer.actor.cpp +++ b/fdbserver/CommitProxyServer.actor.cpp @@ -265,7 +265,7 @@ ErrorOr> getTenantEntry(ProxyCommitData* commitData, return Optional(); } -bool verifyPrefix(ProxyCommitData* const commitData, const CommitTransactionRequest& req) { +bool verifyTenantPrefix(ProxyCommitData* const commitData, const CommitTransactionRequest& req) { ErrorOr> tenantEntry = getTenantEntry(commitData, req.tenantInfo.name.castTo(), req.tenantInfo.tenantId, true); @@ -281,27 +281,26 @@ bool verifyPrefix(ProxyCommitData* const commitData, const CommitTransactionRequ TraceEvent(SevWarnAlways, "TenantPrefixMismatch") .suppressFor(60) .detail("Prefix", tenantPrefix.toHexString()) - .detail("Key", m.param1.toHexString()) - .detail("KeyType", "Key"); + .detail("Key", m.param1.toHexString()); return false; } if (m.type == MutationRef::ClearRange && !m.param2.startsWith(tenantPrefix)) { - TraceEvent(SevWarnAlways, "TenantPrefixMismatch") + TraceEvent(SevWarnAlways, "TenantClearRangePrefixMismatch") .suppressFor(60) .detail("Prefix", tenantPrefix.toHexString()) - .detail("Key", m.param2.toHexString()) - .detail("KeyType", "ClearRangeKey"); + .detail("Key", m.param2.toHexString()); return false; } else if (m.type == MutationRef::SetVersionstampedKey) { + ASSERT(m.param1.size() >= 4); uint8_t* key = const_cast(m.param1.begin()); int* offset = reinterpret_cast(&key[m.param1.size() - 4]); - if (*offset <= tenantPrefix.size()) { - TraceEvent(SevWarnAlways, "TenantPrefixMismatch") + if (*offset < tenantPrefix.size()) { + TraceEvent(SevWarnAlways, "TenantVersionstampInvalidOffset") .suppressFor(60) .detail("Prefix", tenantPrefix.toHexString()) .detail("Key", m.param1.toHexString()) - .detail("KeyType", "VersionstampedKey"); + .detail("Offset", *offset); return false; } } @@ -311,7 +310,7 @@ bool verifyPrefix(ProxyCommitData* const commitData, const CommitTransactionRequ for (auto& rc : req.transaction.read_conflict_ranges) { if (rc.begin != metadataVersionKey && (!rc.begin.startsWith(tenantPrefix) || !rc.end.startsWith(tenantPrefix))) { - TraceEvent(SevWarnAlways, "TenantPrefixMismatch") + TraceEvent(SevWarnAlways, "TenantReadConflictPrefixMismatch") .suppressFor(60) .detail("Prefix", tenantPrefix.toHexString()) .detail("BeginKey", rc.begin.toHexString()) @@ -323,7 +322,7 @@ bool verifyPrefix(ProxyCommitData* const commitData, const CommitTransactionRequ for (auto& wc : req.transaction.write_conflict_ranges) { if (wc.begin != metadataVersionKey && (!wc.begin.startsWith(tenantPrefix) || !wc.end.startsWith(tenantPrefix))) { - TraceEvent(SevWarnAlways, "TenantPrefixMismatch") + TraceEvent(SevWarnAlways, "TenantWriteConflictPrefixMismatch") .suppressFor(60) .detail("Prefix", tenantPrefix.toHexString()) .detail("BeginKey", wc.begin.toHexString()) @@ -383,7 +382,7 @@ ACTOR Future commitBatcher(ProxyCommitData* commitData, .detail("Client", req.reply.getEndpoint().getPrimaryAddress()); } - if (!verifyPrefix(commitData, req)) { + if (!verifyTenantPrefix(commitData, req)) { ++commitData->stats.txnCommitErrors; req.reply.sendError(illegal_tenant_access()); continue;