feat: access agent proofs method would fail to return some session proofs #1047

gobengo · 2023-10-31T00:15:14Z

Motivation:

fix w3cli bug I witnessed in my local setup Unauthorized: Claim {"can":"provider/add"} is not authorized when trying to register space - when juggling prod/staging W3UP_SERVICE_DID w3cli#113

Context

Why does this fix it?
- previously, it was possible for calls to Agent#registerSpace to fail due to invalid proofs, even if the underlying AgentData had the right proof. This was because registerSpace ends up calling invokeAndExecute(Provider.add), which calls invoke to build the invocation, which calls proofs to get the proofs for the invocation. And proofs could fail to include a sessionProof that was appropriate for the invocation audience of the invocation built by Agent#invoke. Now, Agent#proofs returns all the sessionProofs that might be relevant.
explanation of underlying cause of that w3cli issue 113 feat: access agent proofs method would fail to return some session proofs #1047 (comment)

…n audience

…ture I later removed

alanshaw

I don't really understand how this fixes the issue.

packages/access-client/src/agent-data.js

packages/access-client/src/agent.js

…hSessionProof

gobengo · 2023-10-31T16:52:49Z

packages/access-client/src/agent-data.js

 * @returns {Record<string, Ucanto.Delegation>}
 */
-export function getSessionProofs(data) {
+export function getSessionProofs(data, issuer) {


Part of what was causing the bug is that this function ends up building a map of CIDs to proofs. The CID it uses is the nb.proof from a ucan/attest. The agent might actually have multiple ucan/attest ucans with the same nb.proof CID, but with different issuers. Previously only one of these delegations would end up in the returned map from this function, and the caller had no way of getting a session proof appropriate for the audience of their request.

I think this is fairly low level API so I suggest a breaking change that will change return type to

Record<Ucanto.DID, Record<string, ...Ucanto.Delegation>>

That way things will work correctly even if you do not pass the issuer. Furthermore I do have use case where I query proofs and I do not know the issuer of attestations so getting all the proofs would be desired there.

I did something like that here a9aedf6

travis

One bit of cleanup, but I do think the proofs method signature needs some thought - happy to chat in higher bandwidth whenever's convenient for you!

packages/access-client/src/agent.js

… object and options.sessionProofIssuer

…packages in monorepo, and instead uses npm to run the package's build script

…ild all packages in monorepo, and instead uses npm to run the package's build script" This reverts commit 456e6b2.

gobengo · 2023-10-31T21:49:01Z

I met with @Gozala about this and he agreed it makes sense. In the medium term we both don't love the Agent#proofs method API as is, and think maybe Agent#invoke should use something else to find the proofs for an invocation, but adding this option is a good stopgap to fix the bug in a backward-compatible way.

…service id. to avoid attestations from separate issuers having identical CID

travis

looks good, thanks!

packages/upload-api/src/access/confirm.js

…of just returning more proofs than are necessary in some cases

packages/upload-api/src/access/confirm.js

Gozala

Approved with some suggestions

packages/upload-api/src/access/confirm.js

Gozala · 2023-10-31T23:34:09Z

packages/access-client/src/agent-data.js

 */
 export function getSessionProofs(data) {
-  /** @type {Record<string, Ucanto.Delegation>} */
+  /** @type {Record<string, [Ucanto.Delegation, ...Ucanto.Delegation[]]>} */


This is ok but I would prefer Record<Ucanto.DID, Record<string, ...Ucanto.Delegation>> per #1047 (comment) because it makes it a lot easier to pick the ones you care and throw away others as opposed to having to go and group them in another loop.

Not a big deal, but unless there is compelling reason to prefer this I would suggest reverse.

@Gozala roger. I will change to that.

I assume the keys of the inner record (typed as string) would be session proof issuer? and keys of outer record would be the attested to proof cid?

did it 914405d

…-> accessConfirmInvocation per review suggestion

…ct to both proofs

…irst by sessionProof nb.proof and then by sessionProof issuer

…ilter session proofs to once issuer. Agent invoke method uses it. This is required for the registerSpace request to work against staging rn

gobengo · 2023-11-01T20:06:46Z

I was about to merge this, then did one last bit of testing in my local setup to make sure all this would still fix the situation for #113, but noticed that when I tried, I got a new 500 error from the backend. Two days ago I had tested and it worked. I think the regression happened because yesterday I took out the Agent#proofs option.sessionProofIssuer filter. This led to a simpler API, but at the cost of sometimes sending more sessionProofs than are needed in the invocation. I figured it would be harmless. But actually it was not harmless. Sending all those session proofs incl from unknown (to that service) issuers resulted in the upload-api lambda timing out for 10 seconds. I looked and was unable to find logs/sentry info that would hint at why, but my guess is that it was in the proof checking.

So I re-introduced options.sessionProofIssuer to Agent#proofs and re-added using it when Agent#invoke calls Agent#proofs to get proofs for the invocation. Once I made this change so ensure that Agent#invoke invocation doesn't include proof for sessions issued by issuers other than the invocation audience, then things work again.
I did that in this commit a1cee5e

🤖 I have created a release *beep* *boop* --- ## [16.4.0](access-v16.3.0...access-v16.4.0) (2023-11-01) ### Features * access agent proofs method would fail to return some session proofs ([#1047](#1047)) ([d23a1c9](d23a1c9)) ### Bug Fixes * use the issuer as the resource in revocation ([#992](#992)) ([7346d1f](7346d1f)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

…roof selection with `w3 register space` (#114) Motivation: * make use of storacha/w3up#1047 and expect it to fix #113

🤖 I have created a release *beep* *boop* --- ## [7.1.0](upload-api-v7.0.0...upload-api-v7.1.0) (2023-11-03) ### Features * access agent proofs method would fail to return some session proofs ([#1047](#1047)) ([d23a1c9](d23a1c9)) * expose test context of upload-api ([#1069](#1069)) ([f0757d1](f0757d1)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

gobengo added 2 commits October 30, 2023 16:59

tests pass on fix for session proofs

38fa0d3

access agent proofs method picks sessionProofs based on the invocatio…

c9547b6

…n audience

gobengo mentioned this pull request Oct 31, 2023

Unauthorized: Claim {"can":"provider/add"} is not authorized when trying to register space - when juggling prod/staging W3UP_SERVICE_DID storacha/w3cli#113

Closed

gobengo changed the title ~~W3cli 113 unauthorized sessionproofs~~ access agent proofs method chooses sessionProofs based on invocationAudience Oct 31, 2023

gobengo requested a review from travis October 31, 2023 00:17

gobengo changed the title ~~access agent proofs method chooses sessionProofs based on invocationAudience~~ fix: access agent proofs method chooses sessionProofs based on invocationAudience Oct 31, 2023

gobengo changed the title ~~fix: access agent proofs method chooses sessionProofs based on invocationAudience~~ feat: access agent proofs method chooses sessionProofs based on invocationAudience Oct 31, 2023

Merge branch 'main' into w3cli-113-unauthorized-sessionproofs

499939e

gobengo marked this pull request as ready for review October 31, 2023 00:17

gobengo added 3 commits October 31, 2023 09:31

Merge branch 'main' into w3cli-113-unauthorized-sessionproofs

07d068e

revert agent code back to how it was before I modifieid it with a fea…

ce4045b

…ture I later removed

readd comment I didn't mean to remove

018436a

alanshaw reviewed Oct 31, 2023

View reviewed changes

gobengo added 2 commits October 31, 2023 09:43

remove duplicate isSessionProof(ucan) check in access agent-data matc…

80f02ba

…hSessionProof

access-client eslint disable no-continue rule

7248d91

gobengo commented Oct 31, 2023

View reviewed changes

travis requested a review from Gozala October 31, 2023 17:07

travis requested changes Oct 31, 2023

View reviewed changes

packages/access-client/src/agent.js Outdated Show resolved Hide resolved

packages/access-client/src/agent.js Outdated Show resolved Hide resolved

gobengo added 7 commits October 31, 2023 10:26

make it more obvious that invocationTarget is optional

f1b2cd9

access agent proofs method invocationAudience param is now an options…

5100a0b

… object and options.sessionProofIssuer

packages/access-client npm test script doesn't use pnpm to build all …

456e6b2

…packages in monorepo, and instead uses npm to run the package's build script

Revert "packages/access-client npm test script doesn't use pnpm to bu…

2b59ef9

…ild all packages in monorepo, and instead uses npm to run the package's build script" This reverts commit 456e6b2.

Update agent test of sessionProofIssuer

5756ace

add tsc hints

a7937ba

get rid of some ts-expect-error in agent.test.js

4c5e6bc

gobengo changed the title ~~feat: access agent proofs method chooses sessionProofs based on invocationAudience~~ feat: access agent proofs method chooses sessionProofs based on options.sessionProofIssuer Oct 31, 2023

gobengo added 2 commits October 31, 2023 13:53

rm console.log

c417afe

rm unused fn

3ef079e

gobengo requested a review from travis October 31, 2023 21:28

upload-api createSessionProofs create ucan/attest with a fact of the …

fb3934a

…service id. to avoid attestations from separate issuers having identical CID

travis approved these changes Oct 31, 2023

View reviewed changes

Gozala reviewed Oct 31, 2023

View reviewed changes

packages/upload-api/src/access/confirm.js Outdated Show resolved Hide resolved

gobengo added 4 commits October 31, 2023 15:10

agent test for session proofs has more resilient assertions

cb90340

split agent test of proofs() vs test of invoke()

f47dcbc

further test agent proofs/invoke session proofs

79a544d

get rid of agent proofs function options.sessionProofIssuer in favor …

a9aedf6

…of just returning more proofs than are necessary in some cases

gobengo force-pushed the w3cli-113-unauthorized-sessionproofs branch from 7163363 to a9aedf6 Compare October 31, 2023 23:00

gobengo changed the title ~~feat: access agent proofs method chooses sessionProofs based on options.sessionProofIssuer~~ feat: access agent proofs method would fail to return some session proofs Oct 31, 2023

gobengo requested review from Gozala and alanshaw October 31, 2023 23:06

gobengo commented Oct 31, 2023

View reviewed changes

packages/upload-api/src/access/confirm.js Outdated Show resolved Hide resolved

Gozala approved these changes Oct 31, 2023

View reviewed changes

gobengo added 3 commits November 1, 2023 09:05

upload-api access createSessionProofs rename option attestationFacts …

105f29b

…-> accessConfirmInvocation per review suggestion

upload-api access createSessionProofs adds accessConfirmInvocation fa…

3c0a6e0

…ct to both proofs

agent-data getSessionProofs return type is now a nested map indexed f…

914405d

…irst by sessionProof nb.proof and then by sessionProof issuer

gobengo force-pushed the w3cli-113-unauthorized-sessionproofs branch from f062abb to 914405d Compare November 1, 2023 16:49

gobengo added 2 commits November 1, 2023 09:51

Merge branch 'main' into w3cli-113-unauthorized-sessionproofs

436d63c

access agent proofs method has options.sessionProofIssuer option to f…

a1cee5e

…ilter session proofs to once issuer. Agent invoke method uses it. This is required for the registerSpace request to work against staging rn

gobengo merged commit d23a1c9 into main Nov 1, 2023

gobengo deleted the w3cli-113-unauthorized-sessionproofs branch November 1, 2023 20:08

This was referenced Nov 1, 2023

chore(main): release upload-api 7.1.0 #1057

Merged

chore(main): release access 16.4.0 #1037

Merged

gobengo added a commit to storacha/w3cli that referenced this pull request Nov 1, 2023

upgrade @web3-storage/access to 16.4.0 to incorporate storacha/w3up#1047

252aa8e

gobengo mentioned this pull request Nov 1, 2023

feat: upgrade @web3-storage/access to 16.4.0 to fix bug with sessionProof selection with w3 register space storacha/w3cli#114

Merged

gobengo added a commit to storacha/w3cli that referenced this pull request Nov 1, 2023

feat: upgrade @web3-storage/access to 16.4.0 to fix bug with sessionP…

8ed3c90

…roof selection with `w3 register space` (#114) Motivation: * make use of storacha/w3up#1047 and expect it to fix #113

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: access agent proofs method would fail to return some session proofs #1047

feat: access agent proofs method would fail to return some session proofs #1047

gobengo commented Oct 31, 2023 •

edited

Loading

alanshaw left a comment

gobengo Oct 31, 2023

Gozala Oct 31, 2023

gobengo Oct 31, 2023

travis left a comment

gobengo commented Oct 31, 2023

travis left a comment

Gozala left a comment

Gozala Oct 31, 2023

gobengo Nov 1, 2023

gobengo Nov 1, 2023

gobengo commented Nov 1, 2023 •

edited

Loading

feat: access agent proofs method would fail to return some session proofs #1047

feat: access agent proofs method would fail to return some session proofs #1047

Conversation

gobengo commented Oct 31, 2023 • edited Loading

alanshaw left a comment

Choose a reason for hiding this comment

gobengo Oct 31, 2023

Choose a reason for hiding this comment

Gozala Oct 31, 2023

Choose a reason for hiding this comment

gobengo Oct 31, 2023

Choose a reason for hiding this comment

travis left a comment

Choose a reason for hiding this comment

gobengo commented Oct 31, 2023

travis left a comment

Choose a reason for hiding this comment

Gozala left a comment

Choose a reason for hiding this comment

Gozala Oct 31, 2023

Choose a reason for hiding this comment

gobengo Nov 1, 2023

Choose a reason for hiding this comment

gobengo Nov 1, 2023

Choose a reason for hiding this comment

gobengo commented Nov 1, 2023 • edited Loading

gobengo commented Oct 31, 2023 •

edited

Loading

gobengo commented Nov 1, 2023 •

edited

Loading