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

Add HTTP response headers checker #62

Closed

Conversation

THE108
Copy link

@THE108 THE108 commented Sep 3, 2016

HTTP response headers checker

  • checks if required header exists
  • checks if required header matches or not matches regexp

@brian-brazil
Copy link
Contributor

I wonder if we'd be better having this as part of the existing body regexes.

@Andor
Copy link

Andor commented Sep 5, 2016

Brian, I think we need separate checks for headers and for a body, because they are different things HTTP protocol. For example, you can have a line Header: 123 in body but not in headers and it can match your check.

@THE108
Copy link
Author

THE108 commented Sep 5, 2016

Brian,

I think it would be better keeping them separate, otherwise complex rules could lead to complicated regexp.

@brian-brazil
Copy link
Contributor

We're already going to have complicated rules anyway, the config you're proposing is more complicated than what we have already and doesn't yet handle checking that a header isn't present.

@THE108
Copy link
Author

THE108 commented Sep 5, 2016

and doesn't yet handle checking that a header isn't present.

Actually it does check if header is not present. Header must be marked as Required: true in config.

@brian-brazil
Copy link
Contributor

That checks that a header is present, it doesn't check that a header isn't present.

@THE108
Copy link
Author

THE108 commented Sep 5, 2016

I think it is quite a rare case when the presence of a particular header is an error.

And it could be implemented as

fail_if_matches_regexp: ".*"

@brian-brazil
Copy link
Contributor

I wouldn't say it's rare at all, for this type of probe it's not an uncommon use case (e.g. checking internal headers aren't leaking). I think this would be better if a more compact way of configuring it were to be found.

@Andor
Copy link

Andor commented Sep 6, 2016

Actually yes, we can test it with fail_if_matches_regexp: [ '.*' ]

@THE108
Copy link
Author

THE108 commented Sep 6, 2016

I think this would be better if a more compact way of configuring it were to be found.

As I understood you suggest using fail_if_matches_regexp and fail_if_not_matches_regexp config fields for checking both response body and headers.

If so, let's say we want to check if header X-Server is AAA and body is BBB.

How would you suggest configuring this case?

Maybe

fail_if_not_matches_regexp:
  - "BBB"
  - "X-Server: AAA"

?

@brian-brazil
Copy link
Contributor

Yes, something like that is what I'm thinking as the dumbest possible solution.

@THE108
Copy link
Author

THE108 commented Sep 6, 2016

Yes, something like that is what I'm thinking as the dumbest possible solution.

Ok, but what if we need to check if header X-Server is AAA and body is (or isn't) AAA?

@brian-brazil
Copy link
Contributor

Usually there's enough to anchor off that this isn't a problem, especially as you tend to control both ends.

@THE108
Copy link
Author

THE108 commented Sep 6, 2016

Usually there's enough to anchor off that this isn't a problem, especially as you tend to control both ends.

Sorry, I didn't get you. Did you mean the case (header X-Server is AAA and body is (or isn't) AAA) I mentioned above is not real?

@brian-brazil
Copy link
Contributor

It's real, but I've never seen a case where it couldn't be handled.

@THE108
Copy link
Author

THE108 commented Sep 6, 2016

Could you please see Andor's comment? It is exactly that I'm talking about:

Brian, I think we need separate checks for headers and for a body, because they are different things HTTP protocol. For example, you can have a line Header: 123 in body but not in headers and it can match your check.

How would you suggest handling this case: response headers should contain X-Server: AAA and body shouldn't contain the same X-Server: AAA?

@brian-brazil
Copy link
Contributor

How often do you think it's likely to come up in a way where you can't control enough of the body to anchor the regex sufficiently? It's not normal to have things that look like headers at the start of a line in body, particularly when you're designing an end point to be probed.

@Andor
Copy link

Andor commented Sep 7, 2016

Brian, I think checks for body and checks for headers should be split because in a context of HTTP protocol this is different things.

 into http_response_headers_checker

# Conflicts:
#	README.md
#	http_test.go
#	main.go
@czerasz
Copy link

czerasz commented Feb 15, 2017

I also think it's better to separate checks for body and checks for headers.

What needs to be done to finish this feature? I think it's really useful!

@THE108
Copy link
Author

THE108 commented Feb 17, 2017

@czerasz You can use my fork if you need this feature.

achernov added 2 commits March 21, 2017 16:37
@xbglowx
Copy link

xbglowx commented Sep 15, 2017

What is blocking this, other than fixing the conflicts, to getting this merged?

@brian-brazil
Copy link
Contributor

How to configure this remains unresolved.

@cimnine
Copy link

cimnine commented Oct 26, 2017

I also suggest to keep the header and body checks separate. My suggestion for the configuration includes just two additional config items:

fail_if_header_matches_regexp:
- "^X-Internal-Secret-Header"
fail_if_header_not_matches_regexp:
- "^Strict-Transport-Security: max-age=[1-9][0-9]{3,}"
- "^X-Content-Type-Options: nosniff$"

The config above would check that the X-Internal-Secret-Header header is absent and that the Strict-Transport-Security and X-Content-Type-Options headers are present.

Hint: It would probably make sense to configure the regex machine to ignore character case when checking headers.

@maetthu
Copy link

maetthu commented Apr 6, 2018

Any news on this? @cimnine's proposition sounds reasonable to me.

@brian-brazil
Copy link
Contributor

Obsoleted by #419

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 this pull request may close these issues.

7 participants