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

SSL Certificate Validation #1152

Closed
wants to merge 7 commits into from
Closed

SSL Certificate Validation #1152

wants to merge 7 commits into from

Conversation

whatarthurcodes
Copy link
Contributor

-checks if the certificate is expired or not
-warning can occur based on user settings

-checks if the certificate is expired or not
-warning can occur based on user settings
@whatarthurcodes
Copy link
Contributor Author

@LeoCavaille Please Review

@LeoCavaille LeoCavaille self-assigned this Oct 7, 2014
@@ -174,3 +181,38 @@ def report_as_service_check(self, name, status, instance, msg=None):
message=msg
)

def report_ssl(self, host, warning_days):
if host.startswith('https://'):
url = host[8:]
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use urlparse instead.

@LeoCavaille
Copy link
Member

@whatarthurcodes thanks, it is I think a cool thing to have in the agent!!
Also, could you write a simple test to see that a certificate is OK (e.g. say github.com) and another one that is expired (https://testssl-expire.disig.sk/index.en.html).

@remh
Copy link
Contributor

remh commented Oct 7, 2014

+1 it's going to be great to have that in the agent.
It looks like it's not as straight forward as i thought.

The self contained agent bundles openssl (and pyopenssl). Maybe you should use that instead ? It would probably be easier.

@remh
Copy link
Contributor

remh commented Oct 7, 2014

You could basically run the same command as the following:

echo | openssl s_client -connect google.com:443 2>/dev/null | openssl x509 -noout -dates

with pyopenssl

@whatarthurcodes
Copy link
Contributor Author

Are you aware of a particular advantages that pyopenssl can offer, if not I think the standard ssl library serves the purpose we want it to.

@LeoCavaille LeoCavaille removed their assignment Oct 23, 2014
@LeoCavaille LeoCavaille added this to the 5.1.0 milestone Oct 23, 2014
# ssl certificate. Allow for a warning to occur when x days are
# left in the certificate.

check_certificate_expiration: true
Copy link
Contributor

Choose a reason for hiding this comment

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

The common practice in the example configuration is to comment all the optional parameters and to put the default value.

It's already the good value so it just need to be commented.

-used urlparse
-remove ciphers
-fixed example yaml
@@ -124,6 +125,19 @@ def _process(self, instance):
result = (FAILURE, FAILURE, FAILURE, FAILURE)
self.resultsq.put(result)

ssl_expire = instance.get('check_certificate_expiration', False)
if ssl_expire:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LeoCavaille if the config defines, this section will queue another service check regarding the expiration

@@ -174,3 +180,37 @@ def report_as_service_check(self, name, status, instance, msg=None):
message=msg
)

def report_ssl(self, instance):
warning_days = instance.get('days_warning', 14)
host = instance.get('url', None)
Copy link
Member

Choose a reason for hiding this comment

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

I am a bit confused on why this is called host and there is url below. I feel like they should be swapped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay i can switch that if it makes more sense, there was no strong reasoning for either or

return Status.WARNING, "%s" % (str(e))

exp_date = datetime.strptime(cert['notAfter'], "%b %d %H:%M:%S %Y %Z")
days_left = exp_date - datetime.now()
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't use datetime.now. Is the date specified in the certificate in UTC or is there a timezone specified ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Format will be like such:
'notAfter': 'Feb 16 16:54:50 2013 GMT'

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it always GMT ?
If yes then use datetime.utcnow instead, if no then specify the correct timezone in datetime.now

@LeoCavaille LeoCavaille modified the milestones: 5.2.0, 5.1.0 Nov 12, 2014
@LeoCavaille
Copy link
Member

Closing this PR in favor of new rebased and improved PR #1259

@remh remh deleted the arthur/http_ssl branch March 23, 2015 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants