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 certificate verification status to x509_cert input #6143

Merged
merged 3 commits into from
Jul 22, 2019
Merged

Conversation

glinton
Copy link
Contributor

@glinton glinton commented Jul 20, 2019

Resolves #4877

This always sets insecure_skip_verify as it verifies the cert once it's been collected/parsed.

To verify a self-signed cert (remote endpoint or file), set the tls_ca to the self-signing ca, or the self-signed cert itself for validation to be successful.

Alternate to #6139

@glinton glinton added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Jul 20, 2019
}

func (c *X509Cert) getCert(u *url.URL, timeout time.Duration) ([]*x509.Certificate, *tls.Config, error) {
tlsCfg, err := c.ClientConfig.TLSConfig()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would make sense to set this up once in an Init function. Since it is used in the parent function, I would have this function take the tls.Config instead of returning it.

fields := getFields(cert, now)
tags := getTags(cert.Subject, location)

opts := x509.VerifyOptions{}
if i == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here would be a great place for a comment:

// The first certificate is the leaf/end-entity certificate which needs DNS
// name validation against the URL hostname.

_, err = cert.Verify(opts)
if err == nil {
tags["validation"] = "success"
fields["validation"] = 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't name the tag and field with the same key since it is a bit tricky to query, instead use validation and validation_code.

I'm not totally on board with the validation key, the validation is always a success, its just that sometimes the cert is invalid. What do you think about verification=valid, verification=invalid. Sending the error is a bit of a new pattern, but I think it will work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do like verification and verification_code

@danielnelson danielnelson added this to the 1.12.0 milestone Jul 22, 2019
@glinton glinton requested a review from danielnelson July 22, 2019 22:25
} else {
tags["verification"] = "invalid"
fields["verification"] = 1
fields["validation_error"] = err.Error()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verification_error

- fields:
- validation (int)
- validation_error (string)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs updated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add certificate verification status to the x509_cert input
2 participants