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

Catching invalid target urls #149

Closed
montanaflynn opened this issue Apr 21, 2015 · 9 comments · Fixed by #269
Closed

Catching invalid target urls #149

montanaflynn opened this issue Apr 21, 2015 · 9 comments · Fixed by #269
Assignees
Milestone

Comments

@montanaflynn
Copy link

I put in a wrong target url (htttp://mockbin.com/, extra t in the protocol) and was getting 500 errors from Kong. I believe the standard response code to return for this case would be a 502 or 504. Kong could also catch this type of error during API creation, although I'm not sure if that's actually a good idea.

screen shot 2015-04-20 at 5 10 17 pm

@ahmadnassri
Copy link
Contributor

I think it should be clear that it's either http or https there really is no other choice (is there?!).
so best done at creation time.

@montanaflynn
Copy link
Author

Further API test cases, all Accepted with 201 when created through /apis/:

  • With an unregistered domain name returns with 502.
  • With a fake tld, server properly returns 502.
  • Without a specified protocol it returns 500.

The idea of having one protocol per API seems limiting to me. Services rely on the Upgrade header to switch protocols. As a user having to manage two APIs, one for each protocol, is no good. Why can't the target_url be switched to target_dns and behave like public_dns?

@ahmadnassri
Copy link
Contributor

protocol switching should certainly be allowed, I was referring to just the "adding an API" scenario, there's no third option other than http / https so just validate at that point..

however as you suggested a protocol change may occur at anytime ... which brought up a few scenarios / approaches to mind:

  • we could NOT store the protocol at all, let http/https requests be handled by the originating client and receiving server, Kong simply passes those through applying its own rules (rate limiting, api key authentication, etc ...)
  • Kong could be the SSL entry-point, which might be a good feature to have, but also limiting.
    • it's nice so I can have all my SSL in one place, on the Kong instance(s)
    • it's limiting since the APIs I might want to manage come with their own SSL and that's not changeable by the owner (3rd party services, consumer use case)

regardless, Kong should respond with appropriate error codes, and in this case should not even attempt to process an invalid protocol htttp

@montanaflynn
Copy link
Author

Not only can API protocols change, APIs can support multiple protocols. Maybe I should have created a new issue instead of adding that last paragraph since it seems to have sidetracked the main issue, but since we're here now I'll reply to your bullet points.

Your first bullet point is what I brought up at the end when I asked about switching target_url to target_dns, although that leaves the question, how does Kong know which protocol to use when connecting upstream? Using the protocol that connected to Kong, even when supporting an x-protocol-override header seems less than ideal.

Regarding your second bullet point, there should be a plugin to optionally add SSL/TLS but that is outside of the scope of this issue, so I would create a separate issue for that.

Here's where this issue stands:

  • We need to properly return a 502 for invalid target URLs
  • We need to validate target_url as it currently accepts invalid target URLs
  • We need to evaluate switching from target_url to target_dns

@subnetmarco subnetmarco added this to the 0.2.1 milestone Apr 24, 2015
@subnetmarco subnetmarco self-assigned this Apr 24, 2015
@subnetmarco subnetmarco added the api label May 1, 2015
@thibaultcha
Copy link
Member

We need to properly return a 502 for invalid target URLs
We need to validate target_url as it currently accepts invalid target URLs

If we do one then the other is useless? What do we do here? Which protocols do we support?

@montanaflynn
Copy link
Author

Well hopefully the validation will catch any new invalid URLs from being added, but if I upgrade Kong it should return a 502 instead of a 500 for the invalid ones I already have in the system. Also what if I added the API directly to Cassandra?

Today Kong supports HTTP 1.1 and HTTP over TLS/SSL (HTTPS) but there will be demand for SPDY and HTTP/2. My idea was to allow a single API to support multiple protocols but I didn't put much thought into it and it was more of an attempt to get a discussion and brainstorming session rolling. Anyway it should be in it's own issue as it's not really related to this one.

@subnetmarco
Copy link
Member

Just to add more data to the initial issue, nginx returns 500 error with the following message:

2015/05/09 21:25:50 [error] 59454#0: *6 invalid URL prefix in "htttp://httpbin.org/get", client: 127.0.0.1, server: , request: "GET /get HTTP/1.1", host: "httpbin.org"

This is going to be hard to catch at runtime, an easier to catch when creating the API. When creating the API in order to make sure the target_url is valid (or better, to make sure the protocol, host and port parts of the target_url are valid) we can:

  • Open a TCP connection to host:port and make sure the server exists and something is listening on that port
  • Only allow http or https (and ws for websockets?). Validating that any protocol is functioning is going to be impossible on our side, the most we can do is making sure it belongs to a list of accepted protocol names.

@subnetmarco subnetmarco modified the milestones: 0.3.0, 0.2.1 May 11, 2015
@subnetmarco
Copy link
Member

I think we should check the protocol at the API insertion time, but it's going to be tricky for us to implement a proper host:port validation: for example what if I am adding an API that is currently not listening to the port, but it will in the future?

Checking the protocol would be the first step, and make sure it's either http, https. If a request fails because the final host is not reachable that should be handled by the proxy or by a "Monitoring Plugin".

@subnetmarco
Copy link
Member

It now checks that the URL is valid, and the protocol is http or https.

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 a pull request may close this issue.

4 participants