allow to specify --response-code for 3xx codes without specifying -r or --response-code option

[ydkn]

allow to specify --response-code for 3xx codes without specifying -r or --redirect-to

Pull Request Checklist

General

• [ x] Update Changelog following the conventions laid out here

• [ x] RuboCop passes

• [x ] Existing tests pass

Purpose

Currently if you run check-http.rb with --response-code for a 3xx response code e.g. 301 and do not specify -r or --redirect-to the check raises a warning instead of the expected behaviour of passing the check. This PR will fix this by not returning a warning if a 3xx response code was returned by the server and --response-code is set.

[majormoses]

Thanks for your contribution to Sensu plugins! Without people like you submitting PRs we couldn't run the project. I will review it shortly.

[majormoses]

It's been a while since I looked through this code and I seem to recall that explicitly requiring --redirect-to was done for security reasons. We don't want to blindly follow redirects.

[majormoses]

When I have some more time (tonight or tomorrow evening) I will take a closer look at the code to see if this is something we want to support.

[ydkn]

My take on this was that if I set the expected response code explicitly then I already know what I'm doing like when setting it to e.g. 503 or 404 and why should 3xx any different? (I have all 3 cases in my environment)

But I definitely get your point to avoid counting a redirect as success by accident.
Looking into it I found something that I was not aware of:
check-http.rb -u http://foobar/--response-code=""
Setting the response code to an empty string will be a wildcard for all codes. Maybe an empty string for this option should count as not set (nil) but this would introduce a breaking change if somebody uses it in that way (Setting the option to e.g. "." would result in the current behaviour).

[ydkn]

To avoid any confusion or misunderstanding here an example of the current behaviour and thus why I created this PR:

Server returns 201:
check-http.rb -u http://foo/bar --response-code="201"
-> Check will return OK

Server returns 503:
check-http.rb -u http://foo/bar --response-code="503"
-> Check will return OK

Server returns 404:
check-http.rb -u http://foo/bar --response-code="404"
-> Check will return OK

Server returns 301:
check-http.rb -u http://foo/bar --response-code="301"
-> Check will return CRITICAL

[majormoses]

My take on this was that if I set the expected response code explicitly then I already know what I'm doing like when setting it to e.g. 503 or 404 and why should 3xx any different? (I have all 3 cases in my environment)

The very nature of 2xx, 3xx, 4xx, and 5xx status responses are very different and pretending that these should all be handled the same does not make sense to me. A 3xx indicates that it is a redirect of some kind, what is the value of asserting that there is a redirect if you don't check what its link location is? Even still the -r option I think covers this use case where you don't care about anything other than it is redirecting. If we either silently OK on redirects or follow them blindly that would actually subvert my expectations and could pose a serious security risk. If I am an attacker and I manage to gain control of a web server I could insert a redirect to another malicious server this would detect nothing wrong. Keep in mind folks use monitoring solutions not just for uptime, availability, performance but also cover security for example. The behavior you are describing would actually subvert my expectations and would be at odds with what the code was explicitly written to do.

Looking into it I found something that I was not aware of:
check-http.rb -u http://foobar/ --response-code=""

Due to the specific and intentional logic around redirects it still triggers a warning without the -r option:

$ bundle exec ./bin/check-http.rb -u https://facebook.com/ --response-code=""
CheckHttp WARNING: 301

I get the expected response with -r:

$ bundle exec ./bin/check-http.rb -u https://facebook.com/ --response-code="" -r
CheckHttp OK: 301, 0 bytes

That being said that's absolutely true for non redirects:

$ bundle exec ./bin/check-http.rb -u http://benabrams.it/blarg --response-code=""
CheckHttp WARNING: 301

$ bundle exec ./bin/check-http.rb -u https://benabrams.it/blarg --response-code=""
CheckHttp OK: 404, 27881 bytes

$ bundle exec ./bin/check-http.rb -u http://benabrams.it/blarg --response-code="" -r
CheckHttp OK: 301, 0 bytes

I do think that is a bug but realistically the thing about exposing regex to the cli is that these kind of hacks will probably always be possible to write a regex to "trick" the program into doing something "incorrect" that feature was implemented with the best of intentions. We can probably fix this up fairly easy for this edge case but in general it will be hard to save the user from themselves.

To avoid any confusion or misunderstanding here an example of the current behaviour and thus why I created this PR:

...

Server returns 301:
check-http.rb -u http://foo/bar --response-code="301"
-> Check will return CRITICAL

I think I could be talked into allowing the logic to allow a special case when response code is specified and matches a 3xx to make it OK but I would need to think it over more to contemplate the edge cases. I guess my question is why is it so bad to specify -r or --redirect-to when your checking a redirect? I am just trying to understand your perspective.

BTW response code offers full regex support so you can do more complicated conditions and highlights the difficulty of protecting people from themselves:

$ bundle exec ./bin/check-http.rb -u https://benabrams.it/blarg --response-code='^((?!200).+)$'
CheckHttp OK: 404, 27881 bytes

$ bundle exec ./bin/check-http.rb -u http://benabrams.it/blarg --response-code='^((?!200).+)$'
CheckHttp WARNING: 301

$ bundle exec ./bin/check-http.rb -u http://benabrams.it/blarg --response-code='^((?!200).+)$' -r
CheckHttp OK: 301, 0 bytes

$ bundle exec ./bin/check-http.rb -u http://benabrams.it/blarg --response-code='^((?!30[1-2]).+)$'
CheckHttp WARNING: 301

$ bundle exec ./bin/check-http.rb -u http://benabrams.it/blarg --response-code='^((?!30[1-2]).+)$' -r
CheckHttp CRITICAL: 301

$ bundle exec ./bin/check-http.rb -u https://benabrams.it/blarg --response-code='^((?!40.).+)$'
CheckHttp CRITICAL: 404

$ bundle exec ./bin/check-http.rb -u https://benabrams.it/blarg --response-code='^(40[1,3,4])$'
CheckHttp OK: 404, 27881 bytes

$ bundle exec ./bin/check-http.rb -u https://benabrams.it/blarg --response-code='^(40.)$'
CheckHttp OK: 404, 27881 bytes

$ bundle exec ./bin/check-http.rb -u https://benabrams.it/blarg --response-code='^((?!80.).+)$'
CheckHttp OK: 404, 27881 bytes

$ bundle exec ./bin/check-http.rb -u http://benabrams.it/blarg --response-code='^(30[1-2])$'
CheckHttp WARNING: 301

$ bundle exec ./bin/check-http.rb -u http://benabrams.it/blarg --response-code='^(30[1-2])$' --redirect-to https://benabrams.it/blarg
CheckHttp OK: 301 found redirect to https://benabrams.it/blarg

I won't pretend this check is perfect (the code could definitely use some cleanup) but the behavior I see [1] matches my expectations as a user just reading the descriptions on the arguments. I can see why this might not match your initial expectations but we want to be very careful about trying to divine what people will use this for, hence the explicit argument around redirects. This offers max flexibility and takes the secure by default approach and allows users to opt out of security in multiple ways depending on what they are trying to accomplish. Perhaps adding some more documentation to the readme around this would help?

$ bundle exec ./bin/check-http.rb --help | grep redirect
    -r                               Check if a redirect is ok
    -R, --redirect-to URL            Redirect to another page

[1]: other than specifying the response code an empty string, which makes perfect sense I just did not consider that edge case when I reviewed #99 which added regex support for response code. Given that this defaults to a reasonable regex and you can always write a nonsensical regex I am not even sure I consider this a bug and more of a potential to improve the ux around useless matches.

[majormoses]

I spent some more time looking at the code and I think it honestly needs an overhaul. I think we can give what you want but it requires changes to other sections of the code to accomplish this without some side effects. I think we inspect the desired response code and if there is a 3xx and the actual request matched a 3xx then we do not check require redirect-ok but it will have some additional considerations to account for some of the security concerns. I will also likely rip out the regex support in favor of a comma separated list as this highlights some ux issues. Hopefully I will have some time over the weekend to take a crack at it.

[majormoses]

I started a refactor of this logic and I am not sure we have a good/safe path forward without dropping support for regex, or I should say without full regex support that is. We could potentially still support automatically matching the last 2 values so you could say specify 3 which would match 3([0-9]{2}) for you. The other option is to inspect the regex first in the case of a 3xx response code and try to interpolate what they mean with their regex but that seems like a bad idea as regexp is almost a language in its own right and the number of ways someone could defeat this are way too high. Anyways here is my initial stab (leaving regexp support for the time being to illustrate why its dangerous to get rid of redirect ok while we have regexp matching): https://github.com/sensu-plugins/sensu-plugins-http/compare/feature/139?expand=1 would love to hear your thoughts on if this seems like it matches what you want. If so I will check with a few people and see if they agree if there is another sane way to handle this without ripping out regexp support.

[majormoses]

@ydkn any chance you have had a chance to look at what I have and see if this solves your issue?

[ydkn]

@majormoses sorry for the late response. It took me a while to test your changes.

First of all, thank you very much for the work you put into this!

I found a few problems:

• check-http.rb -u http://foo.bar fails if the server returns 200 - this worked before (master)
• specifying a 4xx or 5xx as response code was not correctly checked against the config option

I also changed the safe_redirect_regex? method to be more reliable (it is still ugly)

Now the response_code config option is always checked and to be backward compatible I changed the default regexp to include 3xx codes.

Another thought would be to check pattern, negpattern and sha256checksum regardless of the response code - currently this is only checked against 2xx codes... why not do it for all responses? But this a completely different topic.

You can take a look at my proposed changes here:
https://github.com/sensu-plugins/sensu-plugins-http/compare/feature/139...SICSoftwareGmbH:feature/139_modified?expand=1

[majormoses]

@ydkn sorry I must have missed the notification and it fell off my radar. If this is still something you want to work on, I will put aside some time to help. If thats the case I will rebase my branch and take a closer look at the changes to see why the two issues you raised are happening. I do like your changes on top of the work I started, there were definitely some ugly bits there.

Another thought would be to check pattern, negpattern and sha256checksum regardless of the response code - currently this is only checked against 2xx codes... why not do it for all responses? But this a completely different topic.

Ya that makes sense, if you can raise an issue we can discuss it further. Please ping me on the issue.