Skip to content
This repository has been archived by the owner on Aug 26, 2021. It is now read-only.

Checks for Host header when issuing selftest #168

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Checks for Host header when issuing selftest #168

wants to merge 1 commit into from

Conversation

lestrrat
Copy link
Contributor

  1. Upon request to verifyDomain(), we record the domain that
    we would like to test for reacheability
  2. The selftest handler does a check on the Host header value
    against this domain name. it returns 200 if the domain is present,
    and 404 otherwise
  3. Once the verifyDomain() test is done, this record is erased,
    ensuring that future requests to the selftest endpoing returns 404

fixes #163

1. Upon request to verifyDomain(), we record the domain that
we would like to test for reacheability
2. The selftest handler does a check on the Host header value
against this domain name. it returns 200 if the domain is present,
and 404 otherwise
3. Once the verifyDomain() test is done, this record is erased,
ensuring that future requests to the selftest endpoing returns 404
@znorris
Copy link

znorris commented Apr 24, 2017

Why only make the /_selftest endpoint available during domain authorization? For me it has been handy to use in manually testing and debugging.

@lestrrat
Copy link
Contributor Author

If the project owners give me the "Go!" signal, I certainly would go ahead and change this.

@znorris
Copy link

znorris commented Apr 25, 2017

I can see a reason for both versions. I was just curious what your thought process was. Thanks for submitting a PR for this so quickly. I wish I had a little GO experience.

@simonswine
Copy link
Contributor

Thanks @lestrrat for your PR. I think the best solution would be to always return something.

Maybe we just return a hash of ("$id:$host_header"). That would allow us to check @znorris use case with the host header and we would also make sure we are talking to the same kube-lego instance.

Can you take a look at the implementation @lestrrat?

@lestrrat
Copy link
Contributor Author

@simonswine Hmm, I don't follow. I thought the original purpose was to make the error (of not setting the proper Host header) more explicit? So if the upstream sent us a bogus Host, we would catch it.

if my above understanding is correct, my idea is to create a new PR where we change kubelego.KubeLego to register something in acme.Acme to note "These are the domains that you shall answer 200 OK with". @znorris Let me know if I'm missing something

@znorris
Copy link

znorris commented Apr 26, 2017

If the upstream sends a host header and that host is not in our ingress controller, we should not reply back with a 200 OK. In my opinion, in the event a unknown host header is received we should either still respond, just not with a 200. I haven't dug into the code so it's hard for me to be more specific than that.

@simonswine
Copy link
Contributor

I think the check is only important for Nginx's self test. I think it's not a good idea to maintain a valid host-headers slice in kube-lego. @znorris

Right now kube-lego returns on a call to the _selftest endpoint:

  • always 200 status code
  • and a random string instance_id (generate during kube-lego start-up). This makes sure that the exact same kube-lego instance is reached the self-test

In the future kube-lego should return on a call to the _selftest endpoint:

  • always 200 status code
  • a md5 hash of "instance_id:http_host_header"

This allows us to verify that the host header is forwarded to kube-lego during the self test.

Hope that makes it clearer @lestrrat

@lestrrat
Copy link
Contributor Author

@simonswine K, one last thing: whose instance ID are we talking about? kube-lego.Deployment.uid ? kube-lego.Pod.uid? or something else?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

../_selftest does not check Host header, but ../token does
3 participants