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

Adds fallback allowlist lookup of authenticated address #838

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pschork
Copy link
Collaborator

@pschork pschork commented Oct 25, 2024

Why are these changes needed?

This mitigates a recent incident where the allowlist contained a non-checksummed address for LayerN, but LayerN requests contained a checksummed address resulting in failed rateConfig lookup.

In addition to this change, we plan to normalize all rateLimit configs to use lowercase addresses enforced via blocking CI.

Checks

  • I've made sure the lint is passing in this PR.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • I've checked the new test coverage and the coverage percentage didn't drop.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

This mitigates a recent incident where allowlist contained a
non-checksummed address for LayerN, but LayerN requests contained a checksummed
address resulting in failed rateConfig lookup.
@pschork pschork force-pushed the pschork/checksum_ratelimit_fallback branch from ab4db13 to 4b53e54 Compare October 25, 2024 20:56
@@ -326,6 +326,15 @@ func (s *DispersalServer) getAccountRate(origin, authenticatedAddress string, qu
// Check if the address is in the allowlist
if len(authenticatedAddress) > 0 {
quorumRates, ok := s.rateConfig.Allowlist[authenticatedAddress]
Copy link
Contributor

@ian-shim ian-shim Oct 25, 2024

Choose a reason for hiding this comment

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

how about we always make the keys in this map lowercase (or checksummed) and compare with lowercased (or checksummed) addresses?
https://github.com/Layr-Labs/eigenda/blob/master/disperser/apiserver/config.go#L167-L170

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thx for pointing this out. That's a way more systemic solution and avoids us having to normalize the allowlist configs.

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.

2 participants