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.

Sign up for GitHub

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

Jump to bottom

Added a new input plugin to check SSL certs #1762

Closed
wants to merge 4 commits into from
Closed

Added a new input plugin to check SSL certs #1762

wants to merge 4 commits into from

Conversation

egarbi
Copy link

@egarbi egarbi commented Sep 13, 2016 •
edited
Loading

Required for all PRs:

  • CHANGELOG.md updated (we recommend not updating this until the PR has been approved by a maintainer)
  • Sign CLA (if not already signed)
  • README.md updated (if adding a new plugin)

@nhaugo nhaugo added this to the 1.2.0 milestone Sep 21, 2016
@phemmer phemmer mentioned this pull request Oct 30, 2016
Closed
@sparrc sparrc mentioned this pull request Nov 2, 2016
Closed
3 tasks
@sparrc sparrc modified the milestones: Future Milestone, 1.2.0 Dec 16, 2016
@wcgcoder
Copy link

The README.md in this pull request for the plugin is incorrect. The example text for telegraf.conf should read:

# SSL request given a list of servers (server:port) and a timeout
[[inputs.check_ssl]]
  ## Servers ( Default [] )
  servers = ["github.com:443"]
  timeout = "5s"

... in order to fix the lack of quotes and the incorrect variable response_timeout.

I'd love to see this plugin in the main product! Please expedite! 👍

@egarbi
Copy link
Author

egarbi commented Jan 13, 2017

@wcgcoder Thanks for your input, Readme has been fixed however checks are failing for an unrelated issue.

@wcgcoder
Copy link

Thanks so much. Looks like a problem from outside your plugin. Not sure why as I'm rather new to this, but I was able to manually add your plugin to commit 31a4f03 and build it successfully.

@okv
Copy link

okv commented Mar 14, 2017

Hi there.
Any news about this? I also need this functionality.

@danielnelson
Copy link
Contributor

@okv We are using the github milestones to prioritize pull requests. When there is new information it will be reflected here.

@okv
Copy link

okv commented Mar 15, 2017

will be waiting for it, thanks for your information

@mcfedr
Copy link
Contributor

mcfedr commented Apr 11, 2017

Maybe a separate tool, but how nice would it be to integrate your SSL Labs score into this check? https://github.com/ssllabs/ssllabs-scan

@hardinw
Copy link

hardinw commented May 16, 2017

@danielnelson I know you said updates will be posted here, but i'm getting itchy to have this functionality in the official release. Any update on a potential release date?

@danielnelson danielnelson removed this from the Future Milestone milestone Jun 14, 2017
@swestcott
Copy link

I've patched the error handling issue on a new branch (fe7235c) and signed the CLA. Does someone want to cherry-pick this or should I raise a new PR? It could be great to get this released with v1.4.

@danielnelson
Copy link
Contributor

@egarbi Can you integrate the fix by @swestcott into your branch?

danielnelson
danielnelson suggested changes Jun 27, 2017
View reviewed changes
plugins/inputs/ssl/README.md
# SSL request given a list of servers (server:port) and a timeout
[[inputs.check_ssl]]
## Servers ( Default [] )
servers = ["github.com:443"]
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't use a real site as an example, since I'm sure they wouldn't appreciate being polled by Telegraf, you can use example.org instead. Please do this throughout the pull request.

What about other protocols, should this start with tcp://? Then we could support udp and tcp6

plugins/inputs/ssl/README.md
### Measurements & Fields:

- ssl_cert
- time_to_expire (int) # seconds left for the SSL cert to expire
Copy link
Contributor

Choose a reason for hiding this comment

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

Appears to be a float, you can format this like - time_to_expire (int, seconds).

What do you think about naming this expire_seconds or similar to indicate units?

plugins/inputs/ssl/check_ssl.go Outdated
// Gather gets all metric fields and tags and returns any errors it encounters
func (c *CheckExpire) Gather(acc telegraf.Accumulator) error {
errChan := errchan.New(len(c.Servers))
for _, server := range c.Servers {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should run these concurrently.

plugins/inputs/ssl/check_ssl.go Outdated
certs, err := c.checkHost(server)
errChan.C <- err
if err != nil {
timeToExpire = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this, if there is an error it does not necessarily mean the cert is expired.

plugins/inputs/ssl/README.md
# Check SSL Input Plugin

This input plugin will return how much time (in seconds) left for a SSL cert to expire.
Warning, this check doesnt verify if SSL is valid/secure or not.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should recommend that the user use a increased interval, since 10s default would be too small.

plugins/inputs/ssl/README.md
@@ -0,0 +1,32 @@
# Check SSL Input Plugin
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that ssl is the right name for this, maybe it should be x509 or cert?

Copy link
Author

@egarbi egarbi Jun 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@danielnelson So, what exactly should I change? Everything? the directory, file names and the references to them?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think we need to change all of this.

plugins/inputs/ssl/check_ssl.go
certs := conn.ConnectionState().PeerCertificates

if certs == nil || len(certs) < 1 {
return nil, errors.New("Could not get server's certificate from the TLS connection.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Include the name of the server that caused this.

@danielnelson danielnelson added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin and removed feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin request labels Aug 12, 2017
@antoinealb
Copy link

Since this PR is pretty stalled, what would be the best way to try to get it merged again ? Pick the patches, apply requested changes and open a new PR ?

@egarbi
Copy link
Author

egarbi commented Nov 21, 2017

@antoinealb if you want to start from where I left, be my guest. This was my first go project ever and I don't really now have much time to spend on this.
Thanks

@danielnelson danielnelson mentioned this pull request Feb 9, 2018
Merged
3 tasks
@russorat
Copy link
Contributor

sounds like we should close this in favor of #3768 . Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielnelson danielnelson danielnelson requested changes

Assignees

@dgnorton dgnorton

Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin new plugin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.