leather-io / extension

Leather browser extension
https://leather.io
MIT License
303 stars 142 forks source link

Audit API request volume and perhaps introduce “x-hiro-api-key” #3260

Open sabbyanandan opened 1 year ago

sabbyanandan commented 1 year ago

As touched on https://github.com/hirosystems/stacks-wallet-web/issues/2446, Hiro is implementing the backend infrastructure to relax the blanket rate limiting characteristics on the hosted Hiro APIs. By introducing x-hiro-api-key and the corresponding "value" pair with the HTTP requests originating from Hiro Wallet, Hiro can distinguish and relax the rate limiting rules for the inbound traffic, thus serving more user/developer requests from the Hiro Wallet.

There is no reliance on Kong (as previously experimented with #2446); however, the Hiro API URL would be different, and the only other thing required is the addition of x-hiro-api-key and the value (likely pulled from a secret vault or environment variable). The following code should include the additional key/value pair: https://github.com/hirosystems/stacks-wallet-web/blob/c07a910307a70506c8b206e382428e7c8be3eaf8/src/app/common/api/wrapped-fetch.ts#L1-L4

kyranjamie commented 1 year ago

likely pulled from a secret vault or environment variable

There is no way to hide this key, it will be visible in our source code @sabbyanandan

sabbyanandan commented 1 year ago

@kyranjamie: The key, for sure, will be in the source code. However, can you resolve the "value" at the build/deploy time? It would be similar to the credentials you'd resolve for sentry, segment, and the like.

cc:/ @CharlieC3

markmhendrickson commented 1 year ago

I don't see how we'd be able to include it in request headers as plaintext if we otherwise resolve / encrypt during build?

IIRC Sentry and Segment use publicly viewable keys.

sabbyanandan commented 1 year ago

Ah, I get your point now. I also liberally assume there may be other private keys and use of a secrets manager or vault-like systems in the Wallet already.

The use of .env and an entry in .gitignore for this file can make it not so transparent to an extent, but still not a robust solution. At least, with that in place, you should be able to resolve the value from process.env.X-HIRO-API-VALUE or similar. Example:

const hiroHeaders: HeadersInit = { 
   'x-hiro-product': 'stacks-wallet-web', 
   'x-hiro-version': VERSION, 
   'x-hiro-api-key': process.env.X-HIRO-API-VALUE, 
 }; 

A use of secretes manager could be ideal, but I understand that would be a bigger lift.

CharlieC3 commented 1 year ago

Additionally, if an API key is encrypted then our infra would need a way to decrypt it before validation. And that would potentially be a large challenge, and would consequently mean every single API key would need to be decrypted or "checked" to see if it needs to be decrypted. It would also mean we'd need to maintain a mapping of encrypted keys we keep to decrypted keys we hand out, which is a much larger effort than anticipated.

If implementing a common API key in the web wallet securely is too big of an ask, alternatively we could start asking questions that would help us reach the same goal -- that is wallet users not being rate-limited and wallet traffic being attributed. Some questions and comments below to encourage further discussion:

Avoiding Rate Limits

  1. If we can't use an API key in the web wallet securely (it will be used in other scenarios outside of the web wallet), what does this say about our infrastructure and services? Are they running at too low a scale, or need to be rethought-out to support higher capacities?
  2. Should Hiro services by default run in such a way to accommodate the volume of traffic received from wallet requests without needing a special API key to increase the rate limits? Or should we continue looking for ways to incorporate a global API key used in all web wallets securely?
  3. If "yes" to the above question, what kind of an impact could making those default rate-limit adjustments have on our work-load across Hiro, as well as our business prospects in the future?

HTTP Traffic Attribution

  1. We don't necessarily need an API key for traffic attribution. The same could theoretically be accomplished with the x-hiro-x headers.
markmhendrickson commented 1 year ago

I guess I'm wondering mainly.... are we hitting the API "too much" as a wallet provider from a hosted service's point of view?

If so, is that because we are querying excessively and can make our caching and queries more efficient? Because we can happily look at optimizing client-side code here.

If they're already efficient, then do we actually want to deny certain power users their data after a certain limit? Or is that just going to create problems that aren't worth the additional servicing costs?

And if we aren't hitting the API "too much" in general, what's the goal of this rate limiting, with a new key or not?

CharlieC3 commented 1 year ago

@markmhx Great questions/observations. If the wallet could make fewer calls to the API without losing usability/functionality, I think that would be the most effective kind of long-term change. For example, If the rate-limit is most often hit for power users that have hundreds of addresses, are all of these addresses being loaded at the same time? If so, maybe pages can be implemented in the web wallet so only a few API calls need to execute per page.

@sabbyanandan I'm guessing this might warrant a larger conversation, because now we're getting into the more philosophical questions and it seems we're not sure if the header is the best solution anymore given the high level of effort involved for multiple teams to make it secure.

markmhendrickson commented 1 year ago

@CharlieC3 We can audit queries on our end, though do you all have a sense of which endpoints are getting hit more than you'd like? Because if so, that'll help narrow our focus on analyzing those to start.

I imagine we can figure out ways to cache or query better, and perhaps even suggest API enhancements that allow us to batch request data more efficiently as well.

sabbyanandan commented 1 year ago

I opened a can and surprised myself and others with its content.

@markmhx: You're asking the right questions. FWIW, our only visibility is the API paths and the splits to the traffic pattern.

Based on my naive look at /extended/v1/tx, I noticed ~5000 blocked requests/day. And no, we cannot differentiate how much of it is Wallet-specific. Maybe x-hiro-x headers can be used, but I didn't explore that yet. Anyway, you can look specifically at how much of /extended/v1/** you are hitting and how many return with 403 HTTP status codes. That will give you an idea.

In the end, if you don't see a significant percentage of your requests getting rate limited, it's reasonable to ignore this request.

My running hypothesis, however, is that with the relaxed rate limits, Wallet could resolve more traffic, and unblock more user interactions with it, hence more delight to Wallet users/developers on usability + speed. Likely also increases in transaction volume, among others.

markmhendrickson commented 1 year ago

Thanks! We can do a general audit of our /extended/v1/tx usage and report what we find here.

Sentry should in theory catch these, but we'll double check.