NREL / api-umbrella

Open source API management platform
http://apiumbrella.io
MIT License
2.01k stars 324 forks source link

API Umbrella to add X-Api-Key header when OPTIONS request is handled #391

Open matleppa opened 6 years ago

matleppa commented 6 years ago

Change in original issue: API Umbrella to fix badly behaving API Backend.

There are problems when e.g. an API call is done by Swagger and API key is used. Instead of successful response, thew outcome is TypeError: Failed to fetch.

image

My current understanding is that the flow is following:

Solved

  1. operation request sent by Swagger via proxy (API Umbrella), e.g. GET operation
    • because API Umbrella requires API key use, the request contains X-Api-Key header, which causes request not to be a simple request, thus triggering browser to send a preflight request

Solved

  1. Browser sends an OPTIONS request
    • does not contain header X-API-Key with actual API key
    • contains header Access-Control-Request-Headers with string X-Api-Key
    • request without API key (header X-Api-Key) gets normally rejected by API Umbrella
    • however, when API key requirement for OPTIONS request is disabled by sub URL settings, the API Umbrella accepts it

Problem

  1. API Umbrella accepts and conveys OPTIONS request to API. The X-Api-Key header is included in A-C-R-H header (at least it should be there according to specification)
  2. API responds to OPTIONS request, but response may not contain X-Api-Key header in Access-Control-Allow-Headers
  3. Browser does not send original GET request, because of X-Api-Key header mismatch between A-C-A-H in OPTIONS response and A-C-R-H in GET request.

So it should be up to API to respond with accurate A-C-A-H list. However, because the API does not necessarily use API key, but API Umbrella is requiring it, the API may not be properly implemented handling of OPTIONS request, so maybe API Umbrella could be used to solve the problem.

Proposal to a new functionality

First thoughts about solving problem were following:

I was checking API Umbrella configuration, the override_response_headers functionality, but it is not enough, because there might be different list of custom headers in response depending on API.

So needed new functionality in API Umbrella configuration were e.g. a new option, such as append_response_headers, which would be used to add custom headers to original OPTIONS response from API before conveying it to browser.

Second thought was, that could API Umbrella have this kind of functionality:

This way the browser were informed about the support for headers and it can continue the operation.

adborden commented 6 years ago

@matleppa have you tried the option 2 work around mentioned in this issue?

I'm running into this myself. The history of this issue seems to go back a while and I'm not sure if the work around mentioned is still the right option. Would be great if there was an option out of the box for this. If the work around is still good, I'm happy to document it on the wiki.

GUI commented 6 years ago

@matleppa, @ccsr, @adborden: Whoops, I hadn't realized this was still an issue lurking from the closed #116. I think we can definitely come up with a better, more automatic solution.

@matleppa: Although, if you're only trying to achieve this for Swagger integration, have you considered passing the API key in the URL query string instead of HTTP header? That should sidestep this OPTIONS issue (although, I do agree we should come up with something more automatic). Here's an example Swagger page that integrates API keys that are passed via GET query parameters: https://developer.nrel.gov/docs/cleap/buildings_and_industry/ The underlying Swagger doc is at: https://developer.nrel.gov/docs/cleap/buildings_and_industry/spec.yml But I believe the important piece is:

securityDefinitions:
  api_key:
    type: apiKey
    name: api_key
    in: query
security:
  - api_key: []

@adborden: That second option outlined is still valid. I think there might technically be slightly easier ways to achieve this using the "Override Response Headers" functionality that API Umbrella now has (it didn't exist at the time), but I think that would still require the API backend respond to the OPTIONS request.

More generally, in terms of how we might address this, I think you've laid out some good options, @matleppa. I think we need to be careful to think about the ramifications of messing with this behavior, but I guess my initial quick take might be to implement the following functionality:

  1. For CORS prelight requests, automatically disable the API key requirements in API Umbrella.
    • While making this the default behavior might require some more consideration, it seems like this should be relatively safe, since you pretty much always have to have to disable API key requirements on these OPTIONS requests if you're passing API keys via headers. And I don't think OPTIONS requests should ever really be returning anything of use, so I don't think they need to be protected (but this logic could be even more specific to just CORS preflight OPTIONS requests, based on the detected headers, rather than all OPTIONS requests). However, if people see a reason to keep the API key requirement on these CORS preflight requests, we could make this an option you need to explicitly enable.
  2. Have API Umbrella automatically append X-Api-Key to the Access-Control-Allow-Headers HTTP response header that the underlying API might return on an CORS preflight request.
    • Since I think this X-Api-Key should always be considered a safe header within the context of an API being served via API Umbrella, this seems like a good default. We could add an option to disable this functionality, or make this functionality opt-in, but again, this seems safe, so unless someone has a use case, I'm not sure those extra options are necessary.
  3. Add an option (that defaults to disabled) that would allow API Umbrella to respond directly to CORS preflight requests, rather than proxying them to the API backend.
    • I think this functionality would have to default to being off, but if enabled, then API Umbrella could respond directly to the CORS preflight requests with the appropriate Access-Control-Allow-Headers header. I could see this being useful in cases where the API backend might not respond at all to OPTIONS requests. But since this would be a more opinionated/hard-coded response, that's why I think this might need to default to being off (otherwise, we might be masking the underlying API backend's intended OPTIONS behavior).

So basically, I think if we implemented items 1 & 2, that would go a long way to making the X-Api-Key HTTP header work with CORS requests without any additional configuration for most APIs.

If we also implemented item 3 as an option, then I think that would also give an easy path to supporting proxying to API backends that don't respond to OPTIONS at all.

Does all that seem to make sense? Any other thoughts or ideas?

matleppa commented 6 years ago

@GUI Thank you for your good answer with great analysis of solution alternatives.

This CORS-OPTIONS came up when using the Swagger, but I think that same problem will be faced when calling APIs via API Umbrella from other UIs (using browser), as well.

So, I agree completely with you about items 1 & 2. These were very useful, with this functionality API Umbrella would take care of it's own part of business concerning CORS-OPTIONS in call API via a proxy environment.

About item 3. When re-reading my original proposal now, I can see, that I had too narrow view on that matter, when I was writing only about Access-Control-Allow-Headers and Access-Control-Request-Header. Most probably there would also be needed at least headers Access-Control-Allow-Origin and Access-Control-Allow-Methods. But, as you said, implementing this kind of functionality is not straight-forward.

3+. One other possibility could be to possibility to have a configurable append_response_headers functionality with which different headers could be added into original response from API. This could be used in case API responds, but it can only partially indicate support in OPTIONS response. However, this might be overlapping with the item 2.

All in all, 3 and 3+ were more nice-to-have functionalities, with which API Umbrella tries also take care of part of APIs business in call API via a proxy environment.

So I vote for 1 & 2.