feat(storage): service-wide, stackable guard layer for API limits (#1630) - #1925
feat(storage): service-wide, stackable guard layer for API limits (#1630)#1925AkramBitar wants to merge 2 commits into
Conversation
AkramBitar
changed the title
a177bc5 to
2a4b101
Compare
|
Hi @AkramBitar , thanks for submitting this. My concern is the following: The storage service has many queries that would require similar constraints. Right now, this opts-based approach works only for the Transactions db. I would suggest to perform an analysis of the entire storage service to understand which limits need to be put in place and then design a flexible solution to address the issues. A wrapper-based solution could also be interested so that we can stack different layers on top of each other each perform a different function and the bottom one running the actual SQL query. What do you think? Thanks 🙏 |
d3b4171 to
5b0249d
Compare
AkramBitar
changed the title
ba65cd2 to
364565e
Compare
Hi @adecaro, Thanks a lot for the review. Good idea. I implemented it. Thanks, |
|
@adecaro thanks a lot for the comments. |
3ea0527 to
6fb9045
Compare
…1630) Signed-off-by: AkramBitar <akram@il.ibm.com>
6fb9045 to
97de92f
Compare
The guard layer applied LimitIterator to QueryValidations,
QueryTokenRequests and IteratorConfigurations. None of those three
accepts a pagination argument, so the error they raised once the cap was
exceeded ("must paginate") asked for something the API cannot do.
IteratorConfigurations made this a functional regression rather than
just misleading advice: LocalMembership.storedIdentityConfigurations
drains it in full via collections.ReadAll on the identity Load path, so
a node holding more than maxPageSize (default 1000) stored identity
configurations of one type failed to load identities, recoverable only
by raising or disabling the limit globally.
Remove the cap from all three and record at each site why the read is
not bounded here, matching the reasoning already applied to the
token-store iterators. Row-capping these reads needs a SQL-level LIMIT
or paging in their signatures, which the docs now track as follow-up.
LimitIterator has no remaining callers, so it and its tests are removed
rather than left as dead code.
maxPageSize continues to bound QueryTransactions on the owner and audit
transaction stores, which does take a pagination argument, so rejecting
an unbounded page there is actionable. Write payload limits are
unchanged.
Also correct the docs, which claimed the token store's
unspent/spendable/unsupported iterators were row-capped while
guard/token.go explicitly did not cap them and overrode no read method.
Signed-off-by: AkramBitar <akram@il.ibm.com>
2 participants
Add size limits to the storage service so one huge write or one unlimited query can't overload the database. Fixes the [MED] issue #1630 (denial of service from unlimited resource use, CWE-400 / CWE-770).
What changed since the first revision
The first revision implemented the limits only for the transaction store, via per-constructor
WithMax*options + inline checks. Per review feedback (that approach doesn't scale beyond one store), this has been reworked into a single, stackable guard decorator layer applied across the whole storage service. The transaction store was migrated onto it, so there is now one mechanism.How it works
token/services/storage/db/guard/: aPolicy(loaded once from config), payload-size checks, a row-cappingLimitIterator, and interface-embedding decorators for the transaction, token, endorser, identity and wallet stores. Each decorator embeds the store interface (delegating everything) and overrides only the methods that need a check, plus the nested write-transactions and returned iterators.multiplexed.Driverseam: everyNewXxxresult is wrapped, so all backing drivers (SQLite, PostgreSQL) and all stores are covered uniformly. Additional layers (metrics/tracing) can stack the same way, with the concrete SQL store at the bottom.What is guarded
Write payload size —
maxPayloadSize(default 4 MiB;0disables), rejected before reaching the DB:AddTokenRequest,AddTransaction,AddMovement,AddTransactionEndorsementAckStoreToken,StorePublicParams,StoreCertificationsAddValidationRecordStoreIdentityData,StoreSignerInfo,RegisterIdentityDescriptor,AddConfigurationStoreIdentityRead caps —
maxPageSize(default 1000):QueryTransactions—nil/pagination.None()/ over-max pages are rejectedQueryValidations,QueryTokenRequests,IteratorConfigurations— capped by aLimitIteratorthat errors (rather than silently truncating) once the cap is exceededIntentionally uncapped:
QueryMovements(feeds balance totals; dropping rows would silently return wrong balances).Also closes two gaps in
pagination.ValidateLimited: keyset page size is now capped againstmaxPageSize, and a typed-nil offset is guarded.Configuration / upgrade note
Limits are on by default (4 MiB / 1000), so existing deployments get them automatically. Override via
token.storage.maxPayloadSize/token.storage.maxPageSize; an explicit0disables the respective check. Paging onQueryTransactionscannot be turned off —nil/Noneare rejected — so any caller that read everything at once now pages (seecollectAllTransactions/checks.go).Not yet addressed (tracked follow-up)
These vectors can't be closed by a wrapper alone, because the SQL has already materialised the full result before the decorator sees it — they need query-level changes and will be handled in a follow-up:
ListUnspentTokens,ListUnspentTokensByWallets,ListHistoryIssuedTokens,ListAuditTokens,QueryTokenDetails,ConfigurationsByID,GetWalletIDs,GetExistingSignerInfo— need a SQL-levelLIMIT(with a defined order + documented truncation) or conversion to iterators.DeleteTokens,GetTokens,GetTokenRequests— a huge id list expands into a hugeIN (...)clause.Keystore.Put: opaque value serialised inside the store, so its size is only known at marshal time.Closes #1630.