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

"HTTPS Requirements" config not behaving as expected #319

Closed
jmcarp opened this issue Feb 19, 2016 · 4 comments
Closed

"HTTPS Requirements" config not behaving as expected #319

jmcarp opened this issue Feb 19, 2016 · 4 comments

Comments

@jmcarp
Copy link

jmcarp commented Feb 19, 2016

I'm not sure if this is a bug or if I'm misunderstanding how this option is meant to work, but when I set "HTTPS Requirements" to "Required & return redirects", I see that requests sent over http are rejected with a 400. I would expect to get a 301 or 302 pointing to the https URL instead.

screen shot 2016-02-19 at 10 11 22 am

@GUI
Copy link
Member

GUI commented Feb 23, 2016

Apologies for the delay in following up. Something does seem to be amiss here, since the redirect option shouldn't be resulting in that response. Thanks for bringing this to our attention, we'll look into it.

But out of curiosity, are you sure you want to return redirects? We added this redirect option when we first rolled out these HTTPS requirement options, but afterwards there was some discussion about removing this redirect option altogether, since the caveats surrounding redirects and API clients typically make this a somewhat dangerous option (despite it perhaps seeming like a good idea at first). But if you have a use-case for redirects, we'd definitely like to understand it.

The primary issues surrounding redirects are hinted at in the help tooltip within the admin:

  • Not all API clients will automatically follow redirects, so be careful if using a redirect strategy for existing APIs (since existing calls may break).
  • If API clients rely on the redirect for HTTPS access, this strategy does not secure the API keys, since the client may still be making an insecure initial HTTP request with their API key.

Here's some further discussion on some of the pitfalls of redirects: #34 (comment) and https://https.cio.gov/apis/

The "transitionary & return message" option is probably the best option that should have no impact on existing users, while ensuring all new users can only use HTTPS. The ultimate goal should be the "required & return message" option, but that may require a bit more coordination with previous users that are still making calls insecurely (which you can determine using the api.data.gov analytics).

But again, if you are still interested in using the redirect option, let us know, since it would be helpful to know that there's interest in that capability. On the other hand, if you're no longer interested in using the redirects, that would also be helpful to know, since this might be a good indication that the option is confusing and we should remove it.

@jmcarp
Copy link
Author

jmcarp commented Feb 23, 2016

Thanks for the context! In at least one case, the application I'm working on has to return redirects, as far as I can tell. We have an endpoint that returns ical-formatted events used to subscribe to our calendars by google calendar, icalendar, etc. Google calendar silently ignores https subscription links, and icalendar only responds to webcal:// links, so erroring on http links breaks our calendar subscriptions. These endpoints, as well as the rest of this API, are read-only and don't include private information.

It might make sense for our application to error on http requests globally, and to add an exception for this specific endpoint to send a redirect. We can also just not require https within api.data.gov for this endpoint, since cloud.gov redirects to https as well.

@GUI
Copy link
Member

GUI commented May 5, 2017

Sorry for the delay! Should be resolved by an upcoming upgrade we're planning soon: #382

@gbinal
Copy link
Member

gbinal commented May 5, 2017

Just to confirm, the possibility for confusion behind this is addressed by the upcoming v0.14 release. I'm going to go ahead and close this. Thanks!

@gbinal gbinal closed this as completed May 5, 2017
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

No branches or pull requests

3 participants