Support TLS renegotiation

[britcey]

I have some Windows web hosts I'm trying to monitor that seem to insist on renegotiation - currently I get ssl_probe_success of 0 with these, although all other metrics are present and correct. Log reads:

local error: tls: no renegotiation

This just allow client renegotiation;

Upstream Prometheus consensus is "renegotiation is deprecated, don't use it" but I think for the purposes of checking cert expiration it is low risk.

With this change, the only difference I see is ssl_probe_success is now 1 for those hosts (and the log is quiet).

[ribbybibby]

Thanks for the PR!

Rather than hardcoding this for every case, I think it should be configurable.

Something like this in the config file:

modules:
  https:
    prober: https
    tls_config:
      renegotiation: <string> # could be never, onceasclient, freelyasclient. Defaults to never.

[ribbybibby]

Upstream Prometheus consensus is "renegotiation is deprecated, don't use it"

I guess you're referring to this issue: https://github.com/prometheus/prometheus/issues/1998? Putting the link here for the context.

[britcey]

I had looked at that - TLSConfig is coming from upstream github.com/prometheus/common/config & doesn't support such a thing, so it'll need to be overridden, somehow?

Still new to Go but I'll try to dig in a bit.

Argument against - renegotiation has nothing to do with certificates themselves but just the server config, so having renegotiation support be as liberal as possible would be ideal.

[britcey]

Semi-hackish middle-ground - have insecure_skip_verify also enable renegotiation; you're already signaling that you're willing to be lax about the security of the endpoint per se, you just want the cert information.

[britcey]

I guess you're referring to this issue: prometheus/prometheus#1998? Putting the link here for the context.

Yes, that's it. They're not wrong re: renegotiation (it was dropped for a reason) but "just use a proxy if you need it" isn't all that helpful.

[ribbybibby]

Semi-hackish middle-ground - have insecure_skip_verify also enable renegotiation; you're already signaling that you're willing to be lax about the security of the endpoint per se, you just want the cert information.

That field specifically represents the one in tls.Config so we shouldn't be adding extra meaning to it.

I had looked at that - TLSConfig is coming from upstream github.com/prometheus/common/config & doesn't support such a thing, so it'll need to be overridden, somehow?

Yep, that's my thinking. Something along the lines of creating our own TLSConfig type that embeds config.TLSConfig alongside the extra field:

type TLSConfig struct {
  config.TLSConfig
  Renegotiation string `yaml:"renegotiation"`
}

That might not work with how yaml is unmarshalled but basically we should be able to extend config.TLSConfig in some way.

This exporter is specifically targeted at TLS, so it makes sense that we might offer more options around that than upstream are willing to implement, even if that means departing from the common lib.

Still new to Go but I'll try to dig in a bit.

I can have a look at it, although my time is a bit limited at the moment so it won't be right away.

[ribbybibby]

We can borrow very liberally from the abandoned PR: https://github.com/prometheus/common/pull/221 in the common lib here.

[britcey]

We can borrow very liberally from the abandoned PR: prometheus/common#221 in the common lib here.

I think I should be able to handle incorporating the relevant bits of that code as well as extending TLSConfig.

I'll update this PR when those changes are ready to go.

Thanks.

[britcey]

I think I've hit the limits of my current Go abilities - I went with this approach:

type Module struct {
    Prober     string            `yaml:"prober,omitempty"`
    Timeout    time.Duration     `yaml:"timeout,omitempty"`
    ETLSConfig ExtendedTLSConfig `yaml:"tls_config,omitempty"`
    HTTPS      HTTPSProbe        `yaml:"https,omitempty"`
    TCP        TCPProbe          `yaml:"tcp,omitempty"`
    Kubernetes KubernetesProbe   `yaml:"kubernetes,omitempty"`
}

// Extend Prometheus TLS Config to support
// renegotiation option
type ExtendedTLSConfig struct {
    config.TLSConfig `yaml:",inline"`
    Renegotiation    string `yaml:"renegotiation,omitempty"`
}

but I can't get it to unmarshal the YAML with a 'renegotiation' item in the tls_config - I think because there's a UnmarshalYAML defined for TLSConfig and I can't figure out how to override that (https://pkg.go.dev/github.com/prometheus/common/config?utm_source=gopls#TLSConfig.UnmarshalYAML).

It'd be straight-forward to add a 'renegotiation' argument to the https & tcp probers, but I'm stumped in terms of extending TLSClient itself.

[ribbybibby]

but I can't get it to unmarshal the YAML with a 'renegotiation' item in the tls_config

What errors/issues are you running into specifically?

I just gave it a go and got:

error parsing config file: yaml: unmarshal errors:\n  line 8: field renegotiation not found in type config.plain

I think this is down to the fact we're doing strict unmarshalling, which will reject unknown fields: https://github.com/ribbybibby/ssl_exporter/blob/master/config/config.go#L50. This must not be playing nicely with the embedded config.TLSConfig.

I don't think disabling the strict marshalling is a good option. I wouldn't want to lose that functionality.

Perhaps we could just copy config.TLSConfig and make it an explicit goal to maintain compatibility with the common prometheus config:

// TLSConfig is a superset of config.TLSConfig that supports TLS renegotiation
type TLSConfig struct {
    CAFile             string `yaml:"ca_file,omitempty"`
    CertFile           string `yaml:"cert_file,omitempty"`
    KeyFile            string `yaml:"key_file,omitempty"`
    ServerName         string `yaml:"server_name,omitempty"`
    InsecureSkipVerify bool   `yaml:"insecure_skip_verify"`
    Renegotiation      string `yaml:"renegotiation,omitempty"`
}

Or, leave config.TLSConfig alone and put our extensions under another field:

modules:
  https:
    prober: https
    tls_config:
      server_name: foobar.com
    tls_config_ext:
      renegotiation: never

I think I like the former.

[britcey]

I don't think disabling the strict marshalling is a good option. I wouldn't want to lose that functionality.

That doesn't help anyway, AFAICT - it parses the file but you don't get any value for 'renegotiation' (I reduced it to a bare-bones example in playground and it never would fill in any value for a non-standard field)

I'll try re-implementing TLSConfig.

[britcey]

Pretty happy with this iteration - uses jinzhu/copier to copy the fields from our TLSConfig to a Prometheus TLSConfig & setting Renegotiation when we get a TLSConfig back from NewTLSConfig.

Tests pass.

[britcey]

Incorporated your suggestions, thanks for the feedback.

[britcey]

@ribbybibby are there any other changes you'd like to this PR?

[ribbybibby]

This looks good. Just needs a go mod tidy I think.

[britcey]

Whoops, missed tidying up. Done.

[britcey]

@ribbybibby, thanks much - may I request a new release?

[ribbybibby]

@britcey https://github.com/ribbybibby/ssl_exporter/releases/tag/v2.4.0