Optional dataset-level programmable acls - #536
Conversation
essamhassan
changed the title
c674c26 to
6ca36f4
Compare
essamhassan
changed the title
essamhassan
changed the title
|
One kind of griefing that would be hard to detect is a randomized authorizer. Suppose the validator approved 1% of signatures (regardless of what they were). There would be a 1% chance they would pass |
Use Claude.
No. An authorizer that always rejects is equivalent to a client who walks away. SPs can already terminate the data set unilaterally.
Because that doesn't solve the problem at all. Operation fees are not denominated in gas units. To ensure the operation fee pays for the operation, you have to know the gas of the entire operation, not just this subcall. You also have to know the gas price and the price ratio of the payment token to the gas token. This information is all available to the SP when they sign the transaction. I already suggested (on Github, on Slack) that Curio should reject operations where the operation fee does not cover the cost, or keep them until the gas price comes down (like a mempool).
I describe a much worse griefing strategy. It is worse because the SP does not get paid (which cannot be fixed without also allowing the SP to grief the client). I also described two ways to solve it. Please review that discussion. I think static analysis is possible and I have done something similar in the past (for ERC20). |
It's not about "solving", actually solving for the disconnect between fees paid and costs by the SP isn't something we're attempting to do, we're doing it directionally and adjusting. Over time we'll do an analysis of what it actually cost on average for SPs and adjust pricing accordingly. What I'm suggesting is that a cap is appropriate here to limit the damage and make it so we don't end up adjusting our pricing based on what clients are foisting on SPs. If we want something more unbounded here, then I'd suggest we levy an additional fee for just enabling this feature. Client wants complexity and gas? Fine, pay for it. |
@rvagg earlier version had an authorizer gas limit, I don't have particularly strong opinion on this as long as it's generous enough for a sane auth call. early discussions with @wjmelements noted that there is already a hard limit on the cost posed on the SP in the per op cost. |
|
|
@wjmelements : I'm not up on the specifics of this PR, and I understand there is discussion getting resolved, but while I'm thinking about it, is there any issue with it rolling out in a FWSS contract upgrade? Does it require special attention, user notification, documentation, Synapse support, etc? |
I had some discussion with @jennijuju earlier and we agreed there were some other benefits to the whitelist solution, particularly that we could document the availability of approved authorization registries and how to use them. This whitelist could live on or off-chain: worth discussing.
@BigLep It requires some curio changes (reject or delay over-cost actions, the client-auth whitelist if we do that off-chain), and we should document it and add support to synapse once we have some ACLs whitelisted (probably after deployment), but otherwise doesn't require any immediate special care. |
The three kinds of authenticator whitelist we could do:
I don't have a strong preference. |
|
First, genuine thanks to @essamhassan for the effort here, and to everyone who's put review cycles into it. It sees the iteration has been thorough and the I'm admittedly no expert here, and there's a lot about our contracts and system I'm still learning. But as I've been thinking about this effort and digging into it more, I have some thoughts I want to share. I realize I may be off base on some of this, and I'm very happy to be corrected. My instinct is that we should pause this PR rather than merge it right now, for a few reasons:
To be clear, I'm suggesting a pause, not a rejection, and I don't want to leave this contribution in limbo. What I think would help is: (a) confirming the product priority, and if it's there, (b) writing up the design so we can evaluate the shape and how it interacts with session keys, ACLs/encryption, and pricing more holistically. |
|
For visibility, we're using slack for quicker/tighter coordination on next steps between Lightshift and FOC working group in a private slack channel: https://filecoinproject.slack.com/archives/C0BE621FD98/p1783511763289569 . We'll report back here with decisions. |
|
I met with @rvagg. We agreed on the whitelisting approach for DoS. We can develop a static analysis approach if these truly proliferate enough to justify it (but I don't think they will). |
I was lack of context and after reading through the full PR/design doc, I don’t think the whitelist mitigation is sufficient here. It might address protocol security gap but I think the biggest blocker is from the product side: I don’t see how SP gas cost for isAuthorized gets covered. For context, we went through this exact problem for the base write path #469. The whole reason per-op pricing exists (ADD_PIECES_BASE_FEE, ADD_PIECES_PER_PIECE_FEE, etc.) is so SPs aren’t stuck absorbing unpredictable gas cost — fees are calibrated against known, bounded gas per op and reflected in what the customer actually pays. The current authorizer design breaks that assumption. isAuthorized can be arbitrarily expensive — P256 verification, UCAN-style path parsing, recursive permission checks, whatever a given authorizer implements, but the op fee for e.g. addPieces stays fixed and authorizer-agnostic. So either:
None of these are good outcomes: (1)/(2) push SP-facing operational cost modeling onto SPs at a stage where that’s not the work they signed up for — they’re here to provide storage service, and from talking to many SPs, understanding chain cost is already a known pain point for biz planning and potential walk-away cause. (3) undercuts adoption for exactly the platforms this feature is meant to serve. A whitelist might address malicious or nondeterministic cost, but it doesn’t solve the underlying gap: predictable pricing isn’t embedded into the operation/deal pricing itself for a legitimate, well-behaved authorizer. Context on why this matters: Filecoin SPs are effectively DC operators — they know their infra and ops costs and price accordingly. Onchain gas is outside that comfort zone; it’s an extra cost of supporting on-chain primitives on top of the storage service they actually signed up to provide. That’s exactly why we benchmark gas cost as platform builders and embed it into op pricing in the first place — SPs shouldn’t have to re-derive that per integration, per authorizer. Is there a plan for authorizer-tiered or authorizer-declared cost (e.g. authorizer publishes expected gas, op fee scales to match)? |
|
One potential mitigation could be pairing the whitelist with a declared max gas cost for operations like isAuthorized (anything SPs need to pay for), binding it into pricing:
That said, I’m not actually a fan of this…. It adds real ongoing complexity: every authorizer needs to define its gas cost and pricing correctly, and as maintainers we’d be on the hook for validating those declarations aren’t lowballed — indefinitely, per authorizer, forever. That’s not a one-time review, it’s a standing operational burden. So stepping back — I think the more useful question isn’t “how do we price arbitrary authorizers,” it’s: do we actually need full programmability at this stage, or is there one generic authorizer FWSS can offer that unblocks the need? so the requirements I’m reading
can we build that one authorized for now, benchmark the gas and define the ops fee; skip whitelist/per authorizer pricing; revisiting the programmability later if a 2nd use case shows up with genuinely incompatible auth needs? Feedback welcomed! |
| /// @param operationData The ABI-encoded raw operation payload forwarded by FWSS, carrying all | ||
| /// operation-specific data (e.g. piece metadata) so the authorizer can gate on its | ||
| /// contents — for example enforcing metadata- or path-based ACLs. Empty for operations | ||
| /// whose data is fully described by the other parameters (e.g. terminate). |
There was a problem hiding this comment.
raw operation payload forwarded and carrying all operation-specific data could be at conflict here.
The raw operation payload suggests that the operationData should only be the information covered by the digest/signature but carrying all operation-specific data might be more than that.
Are there any such cases? Do we want to separate the data covered by the signature from any other supplemental data that FWSS (or maybe the uploader) might provide in the future?
|
Because of how generic the interface is, no concerns about limiting ourselves in the scope of authorisation schemes. Although it also means that the authorizer becomes tied to the FWSS version. If the typed hash of the operation changes, it will require redeployments or upgrades to the authorizer. Admitting arbitrary authorizers exposes us to the griefing attack discussed above, and if we don't resolve that issue, providers will do so by whitelisting which authorizers they are willing to utilise during upload. I'm not saying that a whitelist is a good solution, but it is the easiest one to reach for. Assuming some kind of whitelisting, can we keep the authorizers under clients' control while allowing for whitelisting based on code and upgrades? Even if we were to deploy a single more constrained authorizer, I would suggest keeping the rough interface shape as long as we agree to make the first trade-off. |
|
Following discussion with @jennijuju, we will start with a gas cap on that call, in addition to the whitelist, in order to reduce our operational burden. The reason is that we will need to quantify the possible gas use anyway, as part of the approval process. Another benefit might be that the constraint will push developers to use less gas. This doesn't solve the pricing problem but it does prevent operational gas usage from becoming too excessive. A complexity with this cap is that storage gas costs increase over time due to HAMT traversal, so we would still need to ensure these plugins are well below the limit. If they begin just under it, storage growth can push them over, and then transactions would start reverting. This rejection behavior is identical to Curio rejecting the requests due to high gas cost, which is the behavior I want anyway. I still think we have to worry about network congestion and gas price in Curio. If SPs can't reject or delay messages during periods of high gas price, then they can be drained to DoS the network relatively cheaply. I recommend 100 million gas for the limit. A single addPiece uses about 500 million gas right now. That should be more than enough headroom. We also still need to agree on where the whitelist will live.
|
ah @Kubuxu — do you mean we only care about the gas cap; but making sure the authorizer meets the protocol constraint is then integrators’s concern? |
|
I mean the following:
|
|
For visibility, a public design document has been drafted:
|
|
I understand the use case better now and understand where the programmable requirement is coming from; and I can also see how the design here is extensible, in a good way. adding a bit more on my takeaway after discussing w/ Will on the gas cap; I'm now comfortable with the cap-only approach for the time being — i.e. we don't need to add an authorizer ops-fee immediately, as long as we size the gas cap so the uncompensated cost stays negligible against existing ops fees.
At batch sizes we actually see, 100M is ~3%, and at a full batch ~1% — negligible, agreed. My unease is on the worst case: a client doing single-piece adds gives up ~26% of the SP's fee, and i prefer size to the worst case, not the average. I'd like the worst case constrained to ≤5% if we can, which points to a cap around ~20M gas (that's ~0.6% on an average batch). However, the blocker on picking the number: the first authorizer I actually want to make sure we fully unblock — and the common case I'd like supported first — is P256 / passkey verification, and we don't yet know w So two ways forward to unblock this for now:
Can we get that from the FIP-0113 implementers, or measure it on calibnet? That single figure tells us whether the passkey case fits a conservative cap or genuinely needs the higher-cap-plus-surcharge tier . To be clear on the ops-fee question: I'm fine deferring it. The cap alone could be sufficient for now precisely because at these sizes the uncompensated gas is a rounding error on batched calls. We still need to revisit a surcharge if/when an authorizer genuinely requires a cap high enough to move SP margin. |
An attached authorizer is untrusted code: it is the sole gate for every write on its data set, so FWSS must bound what it can burn on the caller's behalf. Cap the subcall at 150M gas. verifyAuthorizer is the single funnel for all three authorizer paths (addPieces, schedulePieceRemovals, terminate), so one ceiling covers them all. Per EIP-150 the call already forwards at most 63/64 of the remaining gas, so the cap only binds when the caller supplies more and can never starve a call that would otherwise succeed. Revert semantics are unchanged: a high-level call bubbles an inner revert rather than returning false, so Errors.Unauthorized still fires only on a clean false, and an authorizer that exhausts its allotment reverts the operation, as IDataSetAuthorizer documents.
…reentrancy Change IDataSetAuthorizer.isAuthorized from view to a state-mutating CALL so authorizers can maintain state while deciding (nonces, rate-limits). Wrap the single untrusted call in a transient reentrancy latch (EIP-1153, confirmed on FEVM) scoped to the authorization path only; the 150M gas cap is preserved.
|
As discussed moved this the authorizer from external view to mutating call to leverage caching and reduce cost, this also unlock a number of interesting authorizer use-cases for other builders. |
Addresses review feedback (r3660109390): use a modifier for the set-before / clear-after latch instead of inline bracketing. Pure readability change — the single-use modifier inlines to identical bytecode (gas unchanged), and the latch still clears after `_;` and on revert.
|
Merge foundryup --update
foundryup --install v1.7.1Resolve merge conflicts and rerun |
10 participants
Summary
Optional per-dataset
IDataSetAuthorizerwrite-authorization hook. No authorizer → existing payer/session-key behavior unchanged. Authorizer set → it is the sole gate for add-pieces, schedule-removals, and signed termination.Behavior
dataSetId,payer, operation typehash, EIP-712 digest, raw signature, and ABI-encodedoperationData.isAuthorizedis a state-mutating CALL (authorizers may keep nonces/rate-limits):false→Errors.Unauthorized; a revert bubbles.Tests
Backward compat; delegated add/removal/termination; session keys via owner;
operationDataforwarding; realvm.signrecovery; false-vs-revert; state mutation during authorization; reentrancy latch (blocks + clears); cleanup on deletion.