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

x509_cert: allow specifying target hostname #6809

Closed
hackery opened this issue Dec 17, 2019 · 10 comments · Fixed by #6917
Closed

x509_cert: allow specifying target hostname #6809

hackery opened this issue Dec 17, 2019 · 10 comments · Fixed by #6917
Labels
feature request Requests for new plugin and for new features to existing plugins
Milestone

Comments

@hackery
Copy link
Contributor

hackery commented Dec 17, 2019

The current configuration syntax provides no way to override the host name (or IP address) given in a sources entry and used for CN validation. This would be useful for testing hosts with multiple vhosts, service groups behind load balancers, and staging hosts which don't resolve from the domain name being tested.

[[inputs.x509_cert]]
  sources = [
    "https://example.org:443",
    # staging hosts offer certs for "example.org"
    { source = "https://staging-01.example.org:443", hostname="example.org" }
    { source = "https://staging-02.example.org:443", hostname="example.org" }
    # qa hosts offer certs for qa.example.org
    { source = "https://qa-01.example.org:443", hostname="qa.example.org" }
    # dev hosts offer certs for dev.example.org
    { source = "https://127.0.0.1:443", hostname="dev.example.org" }
    # do not want to share certs with SAN for *.example.org or IPs
  ]
@danielnelson
Copy link
Contributor

My first instinct is that we shouldn't do this, we are essentially asking the plugin to report an invalid certificate as valid. It seems to me that the system offering the certificate should serve it on the correct CN or SAN. Workarounds could be modifying dns lookup (for example /etc/hosts), adding a SAN to the certificate, or collecting the certificate from the server system using a file path.

I'll leave this open for further discussion.

@danielnelson danielnelson added discussion Topics for discussion feature request Requests for new plugin and for new features to existing plugins labels Dec 17, 2019
@ruatag
Copy link

ruatag commented Dec 24, 2019

openssl s_client can do this. This check valid certificate on specified host and no reason to add individual host names to SAN as they never used. There can be many different names identified by SNI. If hosts balanced you can't connect exactly to one of them. If SNI checked you won't ever connect
echo | openssl s_client -connect servername.com:443 -servername realserver.com -CAfile ca.crt

@danielnelson
Copy link
Contributor

It sounds like we need more control for SNI. Something like this would be backwards compatibile with the current settings, does this have the expected control?

[[inputs.x509_cert]]
  sources = ["example.org"]

  ## Method for handling Server Name Indication.  Can be set to "auto",
  ## "manual", or "disable".
  ##
  ## In "auto" mode the source hostname will be used on certificates retrieved
  ## over the network.  When set to "manual" the contents of "server_name" will
  ## be sent.  If set to "disable" then the server name extension will not be
  ## sent.
  # sni = "auto"

  ## Server name to send when "sni" is set to "manual".
  # server_name = ""

@hackery
Copy link
Contributor Author

hackery commented Jan 3, 2020

That looks good to me, the section could be repeated for each target server name, which would be more straightforward than having a large map of URLs<->servernames in the sources setting.

@danielnelson danielnelson added ready and removed discussion Topics for discussion labels Jan 10, 2020
@hackery
Copy link
Contributor Author

hackery commented Jan 10, 2020

Also cf.:

The Prometheus blackbox exporter, which includes a server_name parameter.

curl and other clients will spot if you include a -H 'Host: ...' option and automatically propagate that to the TLS context for the connection to send SNI.

@hackery
Copy link
Contributor Author

hackery commented Jan 20, 2020

I've not implemented the sni parameter from above in the version I've PR'd. I'm struggling to see the use case for sni = disabled; if a server does not support SNI, the hostname from Sources will be used for connection, and whatever we set in the ClientHello ignored. To treat a server which does support SNI as if it doesn't, one would set the same hostname value in the ClientHello which is what happens by default, i.e. sni = manual.

I also can't see a documented way to do so in crypto/tls — setting Config.ServerName="" seems to omit the SNI extension from the ClientHello, but I don't know if the behaviour is defined.

The other two cases don't need the sni parameter of course — if server_name is present it behaves as "manual", absent like "auto".

@danielnelson danielnelson added this to the 1.14.0 milestone Jan 22, 2020
@hackery
Copy link
Contributor Author

hackery commented Jan 23, 2020

Might have missed something here - should the requested server_name become part of the tags? When a check returns a "valid" verification we will have the cert common_name in the tags, but if the server doesn't recognise the server_name and serves the default site, we see the common_name of the default site - so if there are checks with different server_names against the same host, failing results become indistinguishable. A workaround (or maybe the Right Answer) is to include the tag choice in the configuration - I suppose one might want it given a different identifier than the actual domain name.

Actually, this might be a degenerative case of an existing problem in the tag schema for this input - successful and failing results have different tag values, so they form different series; I'd be inclined to move some of these tags into values in the plugin itself.

@danielnelson
Copy link
Contributor

if the server doesn't recognise the server_name and serves the default site, we see the common_name of the default site

How about we just add a server_name tag to the measurement when we send one, only on the network certificate paths?

successful and failing results have different tag values, so they form different series

Are you referring to the verification tag? Strictly speaking this is true, there is a series for valid lookups and one for invalid. This isn't really desired, but is required because tags have a dual purpose and meaning: not only do they form the identity of the series but since only tags are indexed they are also required for quick lookups and group by.

@hackery
Copy link
Contributor Author

hackery commented Jan 24, 2020

How about we just add a server_name tag

Yup, probably good. Should it be optional? Would it be sensible to have the plugin add that by default, but allow the value to be overridden by a [inputs.x509_cert.tags] choice? (oh, except that doesn't work yet #3362)

the verification tag?

Yes, but not uniquely: I'm not sure there's a use for GROUP BY serial_number for example, which would change when a cert is renewed. Anyway, on second thoughts I think I'm seeing a problem where there isn't one - it's not like the cardinality of ongoing measurements grows unreasonably.

@danielnelson
Copy link
Contributor

One way you could override it is by adding tags like that with the override processor. I think we should always set the tag since we are always setting the server name for verification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Requests for new plugin and for new features to existing plugins
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants