Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from
Aug 27, 2024

Conversation

derekpierre
Copy link
Member

@derekpierre derekpierre commented Aug 19, 2024

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?

@derekpierre derekpierre self-assigned this Aug 19, 2024
Copy link

netlify bot commented Aug 19, 2024

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

Copy link

netlify bot commented Aug 19, 2024

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
Copy link

codecov-commenter commented Aug 19, 2024

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.

@derekpierre derekpierre force-pushed the expiring-storage branch 2 times, most recently from 7705775 to b77f3e9 Compare August 19, 2024 20:33
@derekpierre derekpierre force-pushed the expiring-storage branch 2 times, most recently from 4f9edeb to 332a570 Compare August 20, 2024 15:48
@derekpierre 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 derekpierre marked this pull request as ready for review August 20, 2024 18:57
Copy link
Member

@manumonti manumonti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 81 to 82
);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@derekpierre derekpierre Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@derekpierre derekpierre Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@derekpierre 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
Comment on lines +72 to +75
if (!siweMessage.issuedAt) {
// don't expect to ever happen; but just in case
return false;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a sentinel against undefined?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@derekpierre derekpierre Aug 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

@KPrasch KPrasch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

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.

Comment on lines 81 to 82
);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +72 to +75
if (!siweMessage.issuedAt) {
// don't expect to ever happen; but just in case
return false;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@derekpierre derekpierre merged commit a2f8e6b into nucypher:main Aug 27, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose registerEncrypters from taco API
6 participants