Remove disable auth flag#245
Conversation
The provider node now enforces signed, role-checked requests unconditionally. Removes the --disable-auth-i-know-what-i-am-doing flag, the ProviderState::auth_enabled field, and with_auth_disabled(). - primitives: add auth_message + build_auth_header as the single source of truth for the signed-request format, shared by the provider verifier and the Rust client. - client SDK (Rust): StorageUserClient gains with_auth_signer/set_auth_signer; the S3 and file-system clients reuse the chain signer for provider auth. - TS SDK: layer0 provider-http now signs PUT /node and POST /commit via core's signProviderRequest (reusing the existing primitive); the L0 PAPI demos thread the bucket owner's signer through putChunk/uploadChunk. - StorageMarketplace.sol: add grantWriter so a buyer can authorize a real key, since the contract is the bucket admin; sc-flow grants the client Writer access before uploading. - tests: shared provider-node/tests/common (SignedClient, with_admin_member, serve) and a public auth::StaticMembershipResolver reused across the provider-node and client suites; functional suites sign as //Alice (Admin). - CI/justfile: drop the flag from the integration-tests workflow and the start-provider recipe (auth is always enforced).
RafalMirowski1
force-pushed
the
remove-disable-auth-flag
branch
from
b1aec98 to
89bb56f
Compare
The provider reads bucket membership from finalized chain state, but the
demos established agreements at in-block inclusion and uploaded immediately,
racing finalization — the owner/writer was not yet in the provider's
finalized view, so signed uploads got 403 insufficient_role.
Per the SDK's tx.ts convention ("finalized" is opt-in for txs whose effect a
later operation references), finalize the membership-establishing tx before
the first upload:
- helpers: negotiateAndEstablish gains an opt-in `finalized` param.
- e2e/04, e2e/05, and the four upload-preceding establishes in e2e/10 set it.
- e2e/03: createS3BucketWithStorage finalizes create_s3_bucket.
- full-flow: establish_storage_agreement finalized.
- sc-coverage: bucketC's precompile establish finalized.
- sc-flow: grantWriter finalized (the granted Writer must be in the finalized
view before the upload signs as that key).
Non-uploading suites keep fast in-block submission.
ChainMembershipResolver hand-decodes StorageProvider.Buckets.members via
scale_value, but assumed a flat shape. On this runtime the decoded value nests
the BoundedVec sequence inside a wrapper composite, and each AccountId32 inside
a [u8; 32] newtype wrapper, so the original `for item in members` iterated the
wrapper's single child and extracted zero members. Every signed upload then
failed its role check with 403 insufficient_role — even for the bucket owner,
who is seeded as Admin at creation.
This decoder never ran in CI before: the provider was always started with
--disable-auth, so the path shipped untested. Replace the fixed-shape decode
with a recursive walk that finds every { account, role } struct and collects
the account bytes / role variant through any wrapper layers.
Add a regression unit test that reproduces the exact on-chain nesting
(confirmed against a live chain) — the StaticMembershipResolver suites never
exercise this code — plus a warn! that dumps the value shape if a present
bucket ever decodes to zero members.
Verified end-to-end locally with auth enforced: baseline (original decoder)
reproduced the 403; with this fix `just demo` uploads, defends both
challenges, and claims payment.
With --disable-auth gone, every bucket-scoped provider request must carry a signed Authorization header or the node answers 401 (AuthRequired). Two client paths still went unsigned and only surfaced now that auth is the only path: - drive-ui discarded the raw keypair when building its ChainSigner, so the FileSystemClient sent no Authorization on /fs requests — every UI upload, list, and delete 401'd (the drive-ui e2e "multi-file upload" was the first to hit it). Thread the already-derived keypair through wallet.state -> drive.state -> DriveClient into the ChainSigner, mirroring s3-ui which already did this. authHeaders now actually signs. - e2e workflow 04 called the provider's /s3 routes with bare fetch(). Sign PUT/GET/HEAD/list/DELETE with the bucket owner's keypair, and de-mask 4.5/4.7/4.8 — they previously swallowed the 401 as "S3 not enabled" and skipped, which left 4.6 (HEAD) as the only S3 test that actually ran (and failed). They now assert the real signed roundtrip. Verified locally against a paseo dev chain + inmemory provider: just e2e 11/11 (04 now 12/12), drive-ui e2e 19/19, provider-dashboard e2e 10/10.
# Conflicts: # examples/papi/e2e/04-data-upload-and-retrieval.ts # examples/papi/e2e/05-checkpoint-and-challenges.ts # examples/papi/e2e/10-edge-cases-and-adversarial.ts # examples/papi/full-flow.ts # examples/papi/sc-coverage.ts # examples/papi/sc-flow.ts # packages/layer0/src/provider-http.ts
| @@ -177,64 +188,108 @@ impl MembershipResolver for ChainMembershipResolver { | |||
| None => return Ok(vec![]), | |||
| }; | |||
|
|
|||
There was a problem hiding this comment.
Decoding was bugged after enabling the auth, here is some ad-hoc fix but the proper way is to wait for #239
…ckpoint challenge The finalized establish plus the finalized off-chain challenge push Step 6's challenge_checkpoint to ~block 16 of the flow. With a 15-block agreement the challenge landed at/after expiry, tripping the newly-added live-agreement guard (AgreementExpired). 30 leaves ~14 blocks of margin for finality jitter while Step 8 still exercises the expiry-claim path. Verified end-to-end on zombienet.
| PROVIDER_COV=$(cargo llvm-cov $TEST_FLAGS \ | ||
| # --lib counts the crate's own unit tests (matching the pallet job); | ||
| # $TEST_FLAGS adds the HTTP integration tests — measure both. | ||
| PROVIDER_COV=$(cargo llvm-cov --lib $TEST_FLAGS \ |
There was a problem hiding this comment.
@RafalMirowski1 what is the consequence of this? Unit-tests are also accounted? So, better overall coverage score/number with --lib for provider?
Relates to: #196
There was a problem hiding this comment.
yes, with --lib unit tests are accounted, so the reported coverage number is higher. After I added this
provider coverage on this PR went from 59.41% to 69.43%. I think if something is covered by unit tests we should count it or do we only want to count integration tests?
EDIT:
I guess here #196 (comment) Naren says not including units was intentional.
There was a problem hiding this comment.
yes, with --lib unit tests are accounted, so the reported coverage number is higher. After I added this provider coverage on this PR went from 59.41% to 69.43%. I think if something is covered by unit tests we should count it or do we only want to count integration tests? EDIT: I guess here #196 (comment) Naren says not including units was intentional.
right, I see, can you please, extract this change from this PR? We can merge this change with #196 (let's think about the coverage usage there)
ilchu
left a comment
ilchu
left a comment
There was a problem hiding this comment.
Great job on fixing things that secretly weren't working properly with auth disabled. I'm requesting some changes as there are still ways to make it more airtight.
| // Reuse the same key to authenticate bucket-scoped provider requests. | ||
| if let Ok(keypair) = substrate_client.signer_keypair() { | ||
| storage_client.set_auth_signer(keypair); | ||
| } |
There was a problem hiding this comment.
Here there could be danger of fallthrough, but by virtue of construction this should never fail. So my suggestion would be to unwrap the keypair from option in S3 layer as well.
|
|
||
| /// Get the signer keypair (cloned), for reuse by other components such as | ||
| /// the Layer 0 client's provider-request authentication. | ||
| pub fn signer_keypair(&self) -> std::result::Result<Keypair, String> { |
There was a problem hiding this comment.
This is pretty much a duplicate of .signer() right above it (barring a reference).
| let Some(signer) = self.auth_signer.as_ref() else { | ||
| return req; | ||
| }; |
There was a problem hiding this comment.
If we have this short circuit then we can't really say that auth route is enforced, or am I missing something here? Like we should force the presence of a keypair at all times too, otherwise no actual signing and authorization is happening.
| verifier: ClientVerifier, | ||
| cipher: Option<Box<dyn Cipher>>, | ||
| /// Keypair used to sign bucket-scoped provider requests. | ||
| auth_signer: Option<Keypair>, |
There was a problem hiding this comment.
I believe we'd benefit from making it mandatory.
Co-authored-by: Ilia Churin <ilia@parity.io>
3 participants
Removes the --disable-auth-i-know-what-i-am-doing flag.
closes #232