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).
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)
There was a problem hiding this comment.
ok, removed this
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.
Co-authored-by: Ilia Churin <ilia@parity.io>
| // key from the address. No raw keypair here, so provider requests go | ||
| // unsigned (same as this app always behaved). | ||
| const [publicKey] = ss58Decode(this.signerAddress); | ||
| // drive-ui derives raw dev-account keypairs, so provider requests are |
There was a problem hiding this comment.
why is this mentioning dev-account. The UIs should be account-agnostic, no UI code should directly work with dev-accounts. I know that we support that, but it should be "one-place" where we either allow using dev-accounts or not (per network configuration). I mean the UIs code should not think about "what kind of account" is this and do some IF-ing (ok, as I said before unless really necessary handling cfg). Other words, if the UI allows to select dev-account, ok, no problem, but the rest of the code should handle it as a regular account (signing and everything), no distinction dev vs non-dev account.
@RafalMirowski1 I know this is not related to your changes, but please, we need to clean this also for UIs/clients/SDKs - no explicit usage of dev-accounts. Even selected dev-account should be converted to ChainSigner or whatever without any assumptions or exceptions. I am not sure know, if this could/should land as a separate PR before this one, or we can join it here? What is easier for you :)
There was a problem hiding this comment.
Some loose thoughts here: agree we should have uniform ChainSigner. I think also above the problem is that wallets sign like <Bytes>{msg}</Bytes> and don't expose the raw keypair to us. Before auth was not enforced so it was fine for UI to fallback to unsigned requests but now that would be broken, so when we do verify_signature we should probably make a change there to accept wallet-style signing and then also come up with a way not to have bad UX and pop-ups all the time. We then could drop keypair and DevAccountName from ChainSigner. Seems large enough in scope to do as a follow-up on top of this one, and I'd move the rest of the dev-account cleanup there too. What do you think?
…emove-disable-auth-flag
Resolve provider-node/tests/api_integration.rs: keep the enforced-auth refactor (SignedClient / common::serve / request_bucket) and adopt #252's grouped `Commitment {}` argument to CommitmentPayload::new.
The shared `tests/common` module is compiled once per integration-test crate, and each suite uses only a subset of its helpers (e.g. `auth_integration` drives raw `reqwest` and never uses `SignedClient`), so per-crate dead-code analysis flags the rest and `cargo clippy --all-targets -D warnings` fails. Removing the allow (an applied review suggestion) broke that lint; a module-wide allow is the standard fix for a shared `tests/common` module. Add a comment explaining the rationale so it is not mistaken for an oversight.
…er() signer_keypair() only differed from signer() by returning an owned clone instead of a reference. Remove it and clone the &Keypair at its sole caller (S3Client::new); signer() becomes pub(crate) for that call.
Introduce storage_client::Signer — one signing identity built from a keypair, a secret URI/mnemonic (from_seed), or a dev account name (dev) — implementing subxt::tx::Signer so it serves both provider-auth headers and on-chain extrinsics. It wraps an Arc<Keypair>, so clones are cheap. This replaces the previous mix of raw Keypair, dev-name strings, and Option<Arc<Keypair>> spread across the clients. - Provider auth is mandatory: StorageUserClient::new(config, signer) takes a Signer and sign() always signs (no silent-unsigned bypass); with_defaults() keeps a no-arg dev-Alice convenience. - The chain-signer API speaks Signer everywhere: SubstrateClient / BaseClient and the Admin/Challenger/Provider/Checkpoint clients use set_signer(Signer) / with_signer(Signer); the Keypair/dev-name setters and Option<Arc<Keypair>> are gone. Read-only chain access (no signer) is preserved. - S3Client::new / FileSystemClient::new take a Signer; their SubstrateClients store it unconditionally and FS builds its Layer 0 client eagerly. - Drop the now-unused bip39 dependency.
…ned provider requests The provider always enforces auth and needs a raw sr25519 signature, so a keypair-less ChainSigner could never talk to it — encode that in the type instead of a runtime 401. putChunk/uploadChunk take the signer unconditionally, layer1 authHeaders throws 'Signer not set' like the chain ops already did, and drive-ui/s3-ui build the ChainSigner only when the locally derived keypair is present.
| // Provider HTTP authentication | ||
| // ───────────────────────────────────────────────────────────────────────────── | ||
|
|
||
| fn hex_lower(bytes: &[u8]) -> alloc::string::String { |
There was a problem hiding this comment.
@RafalMirowski1 please, move this hex_lower/auth_message/build_auth_header to the provider-node file, this is provider code, and primitives is supposed to be common module for sharing some common code between pallets and provider code - #178
There was a problem hiding this comment.
moved to negotiation - feels a bit weird place but used by both client and provider to have same source of auth_message
| } | ||
|
|
||
| /// A well-known dev account by name: `"alice"`..`"ferdie"` (case-insensitive). | ||
| pub fn dev(name: &str) -> ClientResult<Self> { |
There was a problem hiding this comment.
not sure where this is used, but probably should be feature-guarded just for tests maybe?
| /// A well-known dev account by name: `"alice"`..`"ferdie"` (case-insensitive). | ||
| pub fn dev(name: &str) -> ClientResult<Self> { | ||
| let keypair = match name.to_ascii_lowercase().as_str() { | ||
| "alice" => dev::alice(), |
There was a problem hiding this comment.
well, usually we use sp_keyring for this dev accounts
| @@ -120,8 +120,7 @@ export class FileSystemClient { | |||
| } | |||
|
|
|||
| private authHeaders(method: string, bucketId: bigint): Record<string, string> { | |||
There was a problem hiding this comment.
@RafalMirowski1 can we deduplicate this authHeaders function (+ probably more, signProviderRequest, ...) between fs and s3? E.g. move to layer0 or base?
There was a problem hiding this comment.
extracted a common base client in base-client.ts
| @@ -23,72 +21,32 @@ pub const PALLET_NAME: &str = "DriveRegistry"; | |||
| #[derive(Clone)] | |||
| pub struct SubstrateClient { | |||
There was a problem hiding this comment.
@RafalMirowski1 I didn't check in deep, but looks like we have multiple pub struct SubstrateClient { which looks pretty same, I think that we should simplify this and have just one in client, probably they are doing the same thing - please, let's create and follow-up issue for this clients/SDKs cleanup and do it when #178 lands (link it here please, as an follow-up)
| /// Authentication is enforced by default: unless the operator explicitly passed | ||
| /// `--disable-auth-i-know-what-i-am-doing`, we wire in the on-chain membership | ||
| /// resolver so every bucket-scoped request is checked against the caller's role. | ||
| fn configure_state(state: ProviderState, cli: &Cli) -> ProviderState { |
There was a problem hiding this comment.
@RafalMirowski1 I would like to clean up this provider configuration little bit now, something like this, because we have here multiple patterns -> take state: ProviderState -> return new state, lots of this mutable setter set_auth_config ...
/// Everything a servable ProviderState requires. No Options.
pub struct ProviderDeps {
pub storage: Arc<dyn StorageBackend>,
pub nonce_store: Arc<dyn NonceStore>,
pub membership: Arc<auth::MembershipCache>,
pub auth_max_skew: Duration,
}
impl ProviderState {
pub fn with_seed(deps: ProviderDeps, seed: &str) -> Result<Self, String> { ... }
pub fn with_provider_id(deps: ProviderDeps, provider_id: String) -> Self { ... }
// CORS is the only genuine optional → stays a consuming builder:
pub fn with_cors_origins(mut self, origins: Option<Vec<String>>) -> Self { ... }
}
This deletes:
- membership_cache: Option<...> → plain Arc
- set_auth_config — constructor arg now
- the "No membership cache" 500 branch — unrepresentable
- set_nonce_store + its Arc::get_mut fallback — the "must be called while solely owned" doc contract becomes the type signature
- configure_state — nothing left to configure post-hoc
There was a problem hiding this comment.
ProviderDeps is now as above.
| pub fn auth_message(method: &str, bucket_id: BucketId, timestamp: &str) -> alloc::string::String { | ||
| alloc::format!("web3storage:{method}:{bucket_id}:{timestamp}") | ||
| } | ||
|
|
There was a problem hiding this comment.
Replay attack vulnerability.
The message binds none of path, body, provider, nonce.
Attacker can capture header of a bucket then put arbitrary bodies on any requests for that bucket, and replays across providers
There was a problem hiding this comment.
@RafalMirowski1 since we already have RBAC in pallet storage provider, I wonder why do we need this?
There was a problem hiding this comment.
@RafalMirowski1 since we already have RBAC in pallet storage provider, I wonder why do we need this?
This is for http endpoints, so I think signature is used to prove to the provider that you are who you say you are, otherwise if say Alice is Writer and provider http endpoints are open, I can claim I'm Alice. Or am i misunderstanding this?
Replay attack vulnerability. The message binds none of path, body, provider, nonce. Attacker can capture header of a bucket then put arbitrary bodies on any requests for that bucket, and replays across providers
Yes, but I did not change it here since I think we're going to have to fix signature anyway to enable wallet signing: #245 (comment) since enabling auth breaks it with current scheme.
| let req = self | ||
| .base | ||
| .http | ||
| .put(format!("{provider_url}/node")) | ||
| .json(&request) | ||
| .send() | ||
| .await?; | ||
| .json(&request); | ||
| let response = self.sign(req, "PUT", bucket_id).send().await?; |
| let req = self | ||
| .base | ||
| .http | ||
| .post(format!("{provider_url}/commit")) | ||
| .json(&request) | ||
| .send() | ||
| .await?; | ||
| .json(&request); | ||
| let response = self.sign(req, "POST", bucket_id).send().await?; |
…e) and StorageUserClient::with_defaults
…egotiation auth_message/build_auth_header are the client<->provider HTTP wire format, not pallet<->provider shared code, so they move to the crate whose charter is exactly that (and which both sides already depend on). hex_lower is dropped in favor of hex::encode, available there. Also enables serde_json/std for provider-negotiation dev-deps: the crate's tests never compiled standalone (workspace feature unification masked it).
The signer both submits extrinsics and identifies the admin account, so the separate admin_account ctor arg and post-connect set_signer() are gone; connect() installs the signer. Test helper dev_account() now delegates to Signer::dev instead of re-matching dev names.
Signer state, authHeaders/requireSigner/submitOpts, and the common ctor options were duplicated verbatim between FileSystemClient and S3Client. Option types stay as aliases, so the public API is unchanged.
…rive-ui and s3-ui setSigner/setKeypair fed the client from two subscriptions, leaving it partially updated between emissions (stale keypair signing as the previous account). One setSigner(signer, address, keypair) + one combineLatest.
# Conflicts: # client/src/admin.rs # storage-interfaces/s3/client/src/substrate.rs
4 participants
Removes the --disable-auth-i-know-what-i-am-doing flag.
closes #232