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

Use delegate wallet to determine content node for listen tracking #1314

Merged
merged 9 commits into from
Mar 19, 2021

Conversation

raymondjacobson
Copy link
Member

@raymondjacobson raymondjacobson commented Mar 16, 2021

Description

What is the purpose of this PR? What is the current behavior? New behavior? Relevant links (e.g. Trello) and/or information pertaining to PR?

Purely additive change and all 3 components can be released individually.

The gist of how this works:

  • Every time a content node sends a listen req. to identity, pass along a signed message & timestamp
  • On identity if a signature is present on a listen req. (clients won't be sending this), compare the signer against the known set of content node delegate wallets. Use that comparison to establish that the IP address of the content node is known
  • Use that IP to whitelist the rate limiting functions

Two optimizations to make the above work better:

  • Content nodes won't sign a new message every time they record a listen (there are a lot). Instead, we re-use the signature so long as it's valid (5 minutes)
  • Identity service doesn't verify the signature every time -- only if the IP is unknown AND there is a signature present on the request. This means that after the first time a content node sends a signature, identity remembers the IP and passes rate limits based on that

Tests

List the manual tests and repro instructions to verify that this PR works as anticipated. Include log analysis if possible.
❗ If this change impacts clients, make sure that you have tested the clients ❗

  1. Added a unit test for the isFromContentNode check
  2. Manually tested locally and ensured that listens began getting "whitelisted" as the signature was matched
    ...

❗ Reminder 💡❗:
If this PR touches a critical flow (such as Indexing, Uploads, Gateway or the Filesystem), make sure to add the requires-special-attention label. Add relevant labels as necessary.

Copy link
Contributor

@dmanjunath dmanjunath left a comment

Choose a reason for hiding this comment

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

direction is good, had a few questions

identity-service/src/audiusLibsInstance.js Show resolved Hide resolved
identity-service/src/utils/contentNodeIPCheck.js Outdated Show resolved Hide resolved
creator-node/src/apiSigning.js Outdated Show resolved Hide resolved
creator-node/src/apiSigning.js Outdated Show resolved Hide resolved
identity-service/src/utils/contentNodeIPCheckTest.js Outdated Show resolved Hide resolved
dmanjunath
dmanjunath previously approved these changes Mar 17, 2021
Copy link
Contributor

@dmanjunath dmanjunath left a comment

Choose a reason for hiding this comment

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

Okay this looks pretty good!

piazzatron
piazzatron previously approved these changes Mar 17, 2021
Copy link
Contributor

@piazzatron piazzatron left a comment

Choose a reason for hiding this comment

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

Looks good to me - clever design. Just a few nits

creator-node/src/apiSigning.js Show resolved Hide resolved
creator-node/src/apiSigning.js Outdated Show resolved Hide resolved
identity-service/src/app.js Show resolved Hide resolved
identity-service/src/rateLimiter.js Show resolved Hide resolved
identity-service/src/utils/apiSigning.js Outdated Show resolved Hide resolved
identity-service/src/utils/apiSigning.js Outdated Show resolved Hide resolved
identity-service/src/utils/apiSigning.js Show resolved Hide resolved
identity-service/src/utils/apiSigning.js Show resolved Hide resolved
identity-service/src/utils/apiSigning.js Outdated Show resolved Hide resolved
Copy link
Contributor

@dmanjunath dmanjunath left a comment

Choose a reason for hiding this comment

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

Looks good!

@raymondjacobson raymondjacobson merged commit 0f60fff into master Mar 19, 2021
@raymondjacobson raymondjacobson deleted the rj-aud-371 branch March 19, 2021 19:35
@SidSethi
Copy link
Contributor

good call on caching the signature!

@AudiusProject AudiusProject deleted a comment from linear bot Sep 11, 2023
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.

None yet

4 participants