Prevent perpetual use of expired auth signature; remove superfluous API functions #567

derekpierre · 2024-08-19T16:33:50Z

Type of PR:

Bugfix
Feature
Documentation
Other

Required reviews:

1
2
3

What this does:

High-level idea of the changes introduced in this PR. List relevant API
changes (if any), as well as related PRs and issues.

Based over #560 (somewhat of a follow-up PR).

EIP4361AuthProvider caches the auth signature it first generates perpetually (unless storage is explicitly cleared eg. localStorage.clear() in apps). However, this auth signature can become expired (> 2h old), and so the provider should detect that on cache retrieval and regenerate the auth signature if it becomes expired.
Add test for SingleSignOnEIP4361AuthProvider
Clean up tests so that signer is obtained from provider, instead of creating separate fakeProvider() and fakeSigner(); this way they are better linked for tests. This only applies to tests - in real life it is possible to have a signer with no provider, but this was not the case for tests prior to this change anyway.
Remove no longer applicable/superfluous API calls from being exposed:
- isAuthorized(...) - calls into the Coordinator contract to call this function, but we are moving away from this where calls should be made directly to the AccessController. However the accessController only checks for isEncryptionAuthorized and doesn't provide the ability to check if an encrypted address is authorized - which is what is implemented here. Therefore, it only applies to the GlobalAllowList contracts, but it's unclear if we need that ability in taco-web
- registerEncrypters(...) - Was never publicly exposed and it is unclear if we need this ability in taco-web. Expose registerEncrypters from taco API #324 alluded to it, but not sure if we still care. Let me know if we do (cc @arjunhassard ).
If we decide down the road we need them, we can add them back.

Issues fixed/closed:

Fixes #...

Closes #324

Why it's needed:

Explain how this PR fits in the greater context of the NuCypher Network. E.g.,
if this PR address a nucypher/productdev issue, let reviewers know!

Notes for reviewers:

What should reviewers focus on? Is there a particular commit/function/section
of your PR that requires more attention from reviewers?

netlify · 2024-08-19T16:34:10Z

✅ Deploy Preview for taco-nft-demo canceled.

Name	Link
🔨 Latest commit	`236f296`
🔍 Latest deploy log	https://app.netlify.com/sites/taco-nft-demo/deploys/66c4e6ce70156c0009928c63

netlify · 2024-08-19T16:34:13Z

✅ Deploy Preview for taco-demo canceled.

Name	Link
🔨 Latest commit	`236f296`
🔍 Latest deploy log	https://app.netlify.com/sites/taco-demo/deploys/66c4e6ceb8611e0008572d8f

codecov-commenter · 2024-08-19T16:39:05Z

Codecov Report

Attention: Patch coverage is 84.61538% with 6 lines in your changes missing coverage. Please review.

Project coverage is 88.55%. Comparing base (e8f9098) to head (236f296).
Report is 177 commits behind head on main.

Files	Patch %	Lines
...ackages/taco-auth/src/providers/eip4361/eip4361.ts	80.00%	3 Missing and 1 partial ⚠️
packages/taco-auth/src/storage.ts	85.71%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #567       +/-   ##
===========================================
+ Coverage   23.12%   88.55%   +65.42%     
===========================================
  Files          62       59        -3     
  Lines       10175     6098     -4077     
  Branches      260      300       +40     
===========================================
+ Hits         2353     5400     +3047     
+ Misses       7763      660     -7103     
+ Partials       59       38       -21

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ng; if expired remove from storage and regenerate it.

There is no need to have both fakeSigner() and fakeProvider() utilities. Only fakeProvider() is needed, and the signer can be obtained from the provider. It also better links the provider and signer.

…keSigner() utility.

manumonti

LGTM! I've just left a concern I have.

manumonti · 2024-08-21T12:10:30Z

examples/taco/nodejs/src/index.ts

);

Are we still "checking contract compatibility" even when removing this from the Testnet examples that are run every hour?

https://github.com/nucypher/taco-web/actions/workflows/lynx.yml

At the moment, I don't think isAuthorized is useful as part of the exposed taco API, hence the removal here. If others feel that it is useful I can put it back.

It's unclear to me how comprehensive this check is as it pertains to "checking compatibility" based on one function. Do you have any additional insight there, perhaps I'm missing something? Perhaps testing the DkgCoordinatorAgent with lynx Coordinator contract via a separate workflow could be a better solution to CI checking compatibility...maybe - not sure what that entails or if it is feasible?

Additionally, Coordinator.isEncryptionAuthorized() which is called by isAuthorized(), is actually deprecated in the latest Coordinator contract, and will be removed down the road. Instead, the underlying acccessController for the ritual should be called directly for that check.

No, I don't have a clear idea about this check beyond that we can check that this specific method on the Coordinator contract has not been modified.

So yes, I agree we can delete this.

My concern came after I read the commented lines in the code talking about "checking contract compatibility": I was wondering if I'm missing something here.

My concern came after I read the commented lines in the code talking about "checking contract compatibility": I was wondering if I'm missing something here.

Totally! I was hoping someone had a better picture of it than I did. Definitely a good conversation to have.

It was added as part of #543 (comment), 😅 , but I don't remember why that is - maybe a side discussion with Piotr. But the problem it is trying to solve seems like a stretch.

Perhaps others may weigh in.

the problem it is trying to solve seems like a stretch.

Agreed. Including a compatibility check as part of an example is not a good idea (although I understand that it stems from the example doubling as the CI smoke test). But the original question still stands, and I'm afraid I can't help. I don't understand why it was added originally.

KPrasch · 2024-08-27T11:44:37Z

packages/taco-auth/src/providers/eip4361/eip4361.ts

+    if (!siweMessage.issuedAt) {
+      // don't expect to ever happen; but just in case
+      return false;
+    }


This is a sentinel against undefined?

Yes.

From the EIP4361 spec,issued-at is a required value - https://docs.login.xyz/general-information/siwe-overview/eip-4361#specification.

However, in the siwe typescript library implementation, it is an optional value in the constructor - https://github.com/spruceid/siwe/blob/main/packages/siwe/lib/client.ts#L49. As a result, Typescript complains if we don't check whether it is defined first before using it.

Is this a divergence of siwe wrt to the spec then?

As far as I can tell, yes, it is a divergence.

I take that back. If issued at is not provided, the current date is used i.e. either the creator can specify the date or the current date is used.

Well, it's not the constructor but the SiweMessage struct - so still a deviation - https://github.com/spruceid/siwe/blob/main/packages/siwe/lib/client.ts#L49

Without the check, the compiler returns the following:

packages/taco-auth/src/providers/eip4361/eip4361.ts:72:36 - error TS2769: No overload matches this call. Overload 1 of 4, '(value: string | number | Date): Date', gave the following error. Argument of type 'string | undefined' is not assignable to parameter of type 'string | number | Date'. Type 'undefined' is not assignable to type 'string | number | Date'. Overload 2 of 4, '(value: string | number): Date', gave the following error. Argument of type 'string | undefined' is not assignable to parameter of type 'string | number'. Type 'undefined' is not assignable to type 'string | number'.

I'll file an issue for the siwe library, and perhaps they can either clarify or fix.

(Update: filed spruceid/siwe#211)

KPrasch

Overall looks good 👍🏻 While I do find this acceptable (and very convenient for implementers) it's interesting to observe the lines between application and library logic blurring a bit here.

derekpierre · 2024-08-27T12:02:55Z

Overall looks good 👍🏻 While I do find this acceptable (and very convenient for implementers) it's interesting to observe the lines between application and library logic blurring a bit here.

Because we opted to cache the message ourselves to prevent users having to sign every time for proof of wallet ownership, it forces us to manage the cache appropriately - this PR does the latter. Definitely open to ideas here if you have any.

cygnusv · 2024-08-27T11:42:47Z

examples/taco/nodejs/src/index.ts

);

the problem it is trying to solve seems like a stretch.

Agreed. Including a compatibility check as part of an example is not a good idea (although I understand that it stems from the example doubling as the CI smoke test). But the original question still stands, and I'm afraid I can't help. I don't understand why it was added originally.

cygnusv · 2024-08-27T12:03:26Z

packages/taco-auth/src/providers/eip4361/eip4361.ts

+    if (!siweMessage.issuedAt) {
+      // don't expect to ever happen; but just in case
+      return false;
+    }


Is this a divergence of siwe wrt to the spec then?

derekpierre self-assigned this Aug 19, 2024

derekpierre force-pushed the expiring-storage branch 2 times, most recently from 7705775 to b77f3e9 Compare August 19, 2024 20:33

Add ability for IStorage to remove items.

443922f

derekpierre force-pushed the expiring-storage branch 2 times, most recently from 4f9edeb to 332a570 Compare August 20, 2024 15:48

derekpierre added 7 commits August 20, 2024 14:55

Check whether prior auth signature is now expired (2h) before returni…

be3af16

…ng; if expired remove from storage and regenerate it.

Add test for EIP4361AuthProvider storage expiry.

09437cc

Add test for SingleSignOnEIP4361AuthProvider.

35019f3

Use a legit signer with the fakeProvider() test utility.

959f40e

There is no need to have both fakeSigner() and fakeProvider() utilities. Only fakeProvider() is needed, and the signer can be obtained from the provider. It also better links the provider and signer.

Update tests to obtain signer from fakeProvider() instead of using fa…

c53ea31

…keSigner() utility.

Exclude non-applicable files from code coverage reports.

61176ca

Remove unnecessary taco api calls; one of which is no longer applicable.

236f296

derekpierre force-pushed the expiring-storage branch from 332a570 to 236f296 Compare August 20, 2024 18:56

derekpierre changed the title ~~[WIP] Prevent usage of already expired auth signature for long running application~~ Prevent perpectual use of expired auth signature; remove superfluous API functions Aug 20, 2024

derekpierre requested review from KPrasch, cygnusv, theref, vzotova and manumonti August 20, 2024 18:57

derekpierre marked this pull request as ready for review August 20, 2024 18:57

derekpierre requested a review from arjunhassard August 20, 2024 19:05

manumonti approved these changes Aug 21, 2024

View reviewed changes

vzotova approved these changes Aug 21, 2024

View reviewed changes

derekpierre changed the title ~~Prevent perpectual use of expired auth signature; remove superfluous API functions~~ Prevent perpetual use of expired auth signature; remove superfluous API functions Aug 21, 2024

KPrasch reviewed Aug 27, 2024

View reviewed changes

KPrasch approved these changes Aug 27, 2024

View reviewed changes

cygnusv approved these changes Aug 27, 2024

View reviewed changes

derekpierre merged commit a2f8e6b into nucypher:main Aug 27, 2024
9 checks passed

derekpierre mentioned this pull request Aug 27, 2024

Ensure signature freshness on client side #452

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent perpetual use of expired auth signature; remove superfluous API functions #567

Prevent perpetual use of expired auth signature; remove superfluous API functions #567

derekpierre commented Aug 19, 2024 •

edited

Loading

netlify bot commented Aug 19, 2024 •

edited

Loading

netlify bot commented Aug 19, 2024 •

edited

Loading

codecov-commenter commented Aug 19, 2024 •

edited

Loading

manumonti left a comment

manumonti Aug 21, 2024

derekpierre Aug 21, 2024 •

edited

Loading

manumonti Aug 21, 2024

derekpierre Aug 21, 2024 •

edited

Loading

cygnusv Aug 27, 2024

KPrasch Aug 27, 2024

derekpierre Aug 27, 2024

cygnusv Aug 27, 2024

derekpierre Aug 27, 2024

derekpierre Aug 27, 2024

derekpierre Aug 27, 2024 •

edited

Loading

KPrasch left a comment

derekpierre commented Aug 27, 2024

cygnusv Aug 27, 2024

cygnusv Aug 27, 2024

Prevent perpetual use of expired auth signature; remove superfluous API functions #567

Prevent perpetual use of expired auth signature; remove superfluous API functions #567

Conversation

derekpierre commented Aug 19, 2024 • edited Loading

netlify bot commented Aug 19, 2024 • edited Loading

✅ Deploy Preview for taco-nft-demo canceled.

netlify bot commented Aug 19, 2024 • edited Loading

✅ Deploy Preview for taco-demo canceled.

codecov-commenter commented Aug 19, 2024 • edited Loading

Codecov Report

manumonti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekpierre Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekpierre Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekpierre Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

KPrasch left a comment

Choose a reason for hiding this comment

derekpierre commented Aug 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekpierre commented Aug 19, 2024 •

edited

Loading

netlify bot commented Aug 19, 2024 •

edited

Loading

netlify bot commented Aug 19, 2024 •

edited

Loading

codecov-commenter commented Aug 19, 2024 •

edited

Loading

derekpierre Aug 21, 2024 •

edited

Loading

derekpierre Aug 21, 2024 •

edited

Loading

derekpierre Aug 27, 2024 •

edited

Loading