Active Check for remaining validity of Certificate Revocation Lists by OguzhanCicek · Pull Request #331 · Checkmk/checkmk

OguzhanCicek · 2021-03-02T14:40:58Z

This PR adds a active check to monitor the remaining hours of validity of a given CRL. To check this the following nagios plugin is being used:
https://raymii.org/s/software/Nagios_plugin_to_check_CRL_expiry.html

OguzhanCicek · 2021-03-02T14:41:55Z

One of the unit tests fail because it cant find the manpage for this check. Im not sure why its not being found.

jherbel · 2021-03-03T06:34:16Z

Hi,
thank you for this PR. I am not sure if this PR is actually needed. Checkmk has the capability to use Nagios plugins, see
https://docs.checkmk.com/master/en/active_checks.html (2.3. Integrating other Nagios-compatible plug-ins). Please check if this already fits your needs. I also took a brief look at the description of the Nagios plugin, you can provide WARN and CRIT levels via the command line.
Best
Jörg

jherbel · 2021-03-03T10:58:17Z

We just had an internal discussion about this and it turned out that we should definitely consider merging this. I will take a closer look at the code itself soon.

jherbel · 2021-03-03T11:01:19Z

One thing: I think the version of the plugin itself which you uploaded here is not the latest one, this one seems to be newer: https://github.com/RaymiiOrg/nagios/blob/master/check_crl/check_crl.py
Could you check this please?

…cts the remaining time in minutes. Had to add a addition try-except block in line 59 to make it work. -now works with remaining minutes and not hours.

jherbel · 2021-03-08T14:24:51Z

+
+
+def check_crl_arguments(params):
+    return "-u {} -w {} -c {}".format(params.get("url"), params.get("minutes")[0], params.get("minutes")[1])


This line is incorrect: if "minutes" is not in params, you will end up with the expression None[0], which will of course crash. This should never happen, since the valuespec below has no optional keys, so please use params["url"] etc.

jherbel · 2021-03-08T14:30:53Z

+              help=_("These levels make the check go warning or critical whenever the "
+                     "remaining validity of the monitored CRL is too low."),
+              elements=[
+                  Integer(title=_("warning at"), unit=u"Minutes", default_value=2880),


Please use the valuespec Age here. In checks/check_crl, the actual value will then be seconds, so you have to transform there to what the Nagios plugin expects.

jherbel · 2021-03-08T14:33:54Z

+    try:
+        inform = 'DER'
+        crlfile = open(tmpcrl, "r")
+        try:


Do you know why and when this is necessary? Probably, this PR is related:
RaymiiOrg/nagios#6
I am not saying that you should include this PR here, I am just trying to understand under what conditions this will fail and if this could be avoided by using for example open(..., mode="rb") or something.

Here, we still need some clarification

jherbel · 2021-03-08T14:57:41Z

@@ -0,0 +1,141 @@
+#!/usr/bin/python3


Our fileheader please

jherbel · 2021-03-08T15:09:42Z

+
+def main():
+    try:
+        opts, args = getopt.getopt(sys.argv[1:], "hu:w:c:", ["help", "url=", "warning=", "critical="])


Please transform this to argparse

jherbel · 2021-03-08T15:10:53Z

+import tempfile
+import urllib.request, urllib.parse, urllib.error
+
+def check_crl(url, warn, crit):


Please add typization

jherbel · 2021-03-08T15:13:23Z

+    minutes = (eol - today) / 60
+    if abs(minutes) < 4 * 60:
+        expires = minutes
+        unit = "minutes"


Please take a look at cmk.utils.render.Age for this

Please check cmk.utils.render.Age

Please check cmk.utils.render.Age

jherbel

The Nagios plugin still needs some work. First of all, please move it to active_checks/check_crl. Next, we need to bring it to a maintainable state. I know that you did not write the plugin, however, since we port it into our codebase, we will have to maintain it in the future.

OguzhanCicek · 2021-03-21T11:19:45Z

The Nagios plugin still needs some work. First of all, please move it to active_checks/check_crl. Next, we need to bring it to a maintainable state. I know that you did not write the plugin, however, since we port it into our codebase, we will have to maintain it in the future.

ok. I will try to update this PR within the coming week. Please dont close this for inactivity.

-uses valuespec Age now -uses now checkmk file headers -changes the main() call -uses argpase now

jherbel

Already much better. Please also add a unit test similar to tests/unit/checks/test_check_sftp.py

jherbel · 2021-04-06T07:24:34Z

+
+def main():
+    parser = argparse.ArgumentParser()
+    parser.add_argument("--url", "-u", required=True)


Please add type=str here for clarity

jherbel · 2021-04-06T07:27:22Z

+import urllib.request, urllib.parse, urllib.error
+
+def output_check_result(rc, s):
+    stxt = ['OK', 'WARN', 'CRIT', 'UNKNOWN'][rc]


I don't think you need stxt, since in Checkmk, you anyway see the check result in a separate column, so there is no need to add it here as a text.

jherbel · 2021-04-06T07:27:53Z

@@ -0,0 +1,140 @@
+#!/usr/bin/env python3
+# -*- coding: utf-8 -*-
+# Copyright (C) 2013 - Remy van Elst


Adjust file header please

jherbel · 2021-04-06T07:29:08Z

+    output = '%s - %s' % (stxt, s)
+    sys.stdout.write('%s\n' % output)
+
+def check_crl(url: str, warn: int, crit: int):


add return type

jherbel · 2021-04-06T07:32:21Z

+    minutes = (eol - today) / 60
+    if abs(minutes) < 4 * 60:
+        expires = minutes
+        unit = "minutes"


Please check cmk.utils.render.Age

jherbel · 2021-04-06T07:34:29Z

+
+
+def check_crl_description(params):
+    return "CRL: {}".format(params["url"])


Please use an f-string here

jherbel · 2021-04-06T07:34:32Z

+
+
+def check_crl_arguments(params):
+    return "-u {} -w {} -c {}".format(params["url"], params["time"][0], params["time"][1])


Please use an f-string here

jherbel · 2021-04-06T07:38:02Z

+    return "CRL: {}".format(params["url"])
+
+
+active_check_info["check_crl"] = {


active_check_info["crl"]

jherbel · 2021-04-06T07:38:17Z

+    HostRulespec(
+        group=RulespecGroupActiveChecks,
+        match_type="all",
+        name="active_checks:check_crl",


name="active_checks:crl",

jherbel · 2021-04-06T07:39:05Z

    ))
+
+
+def _valuespec_active_checks_crl():


typization (return type)

-renames the active check from check_crl to crl

jherbel

Still some issues + the new unit tests of course needs to pass.

jherbel · 2021-04-19T06:08:14Z

@@ -0,0 +1,140 @@
+#!/usr/bin/env python3
+# -*- coding: utf-8 -*-
+# Copyright (C) 2013 - Remy van Elst


Adjust file header please

jherbel · 2021-04-19T06:08:23Z

+    try:
+        inform = 'DER'
+        crlfile = open(tmpcrl, "r")
+        try:


Here, we still need some clarification

jherbel · 2021-04-19T06:08:32Z

+    minutes = (eol - today) / 60
+    if abs(minutes) < 4 * 60:
+        expires = minutes
+        unit = "minutes"


Please check cmk.utils.render.Age

jherbel · 2021-04-19T06:17:11Z

+
+
+@pytest.mark.parametrize("params,expected_args", [
+    (("foo", 222, 111, "bar", {}), ["--url=foo", "--warn=222", "--crit=111"]),


According to the valuespec, you expect a dict here, not a tuple. Accordingly, this test failed in the last travis run.

jherbel · 2021-05-18T09:26:15Z

Closed this due to inactivity.

adds active check to monitor to remaining validity of a given CRL URL

f9be378

-replaces the old check_crl script with a newer one. The new one dete…

1be3a33

…cts the remaining time in minutes. Had to add a addition try-except block in line 59 to make it work. -now works with remaining minutes and not hours.

jherbel reviewed Mar 8, 2021

View reviewed changes

Comment thread checks/check_crl

jherbel reviewed Mar 8, 2021

View reviewed changes

Comment thread omd/packages/nagios/skel/local/lib/nagios/plugins/check_crl

jherbel reviewed Mar 8, 2021

View reviewed changes

jherbel requested changes Mar 8, 2021

View reviewed changes

-fixes bug in getting variables from "params" in case they dont exist

53c152a

-uses valuespec Age now -uses now checkmk file headers -changes the main() call -uses argpase now

jherbel requested changes Apr 6, 2021

View reviewed changes

-adds a unit test for check_crl

c046ed3

-renames the active check from check_crl to crl

jherbel requested changes Apr 19, 2021

View reviewed changes

jherbel closed this May 18, 2021



		def check_crl_arguments(params):
		return "-u {} -w {} -c {}".format(params.get("url"), params.get("minutes")[0], params.get("minutes")[1])



		def check_crl_description(params):
		return "CRL: {}".format(params["url"])

		return "CRL: {}".format(params["url"])


		active_check_info["check_crl"] = {



		@pytest.mark.parametrize("params,expected_args", [
		(("foo", 222, 111, "bar", {}), ["--url=foo", "--warn=222", "--crit=111"]),

Conversation

OguzhanCicek commented Mar 2, 2021

Uh oh!

OguzhanCicek commented Mar 2, 2021

Uh oh!

jherbel commented Mar 3, 2021

Uh oh!

jherbel commented Mar 3, 2021

Uh oh!

jherbel commented Mar 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jherbel left a comment

Choose a reason for hiding this comment

Uh oh!

OguzhanCicek commented Mar 21, 2021

Uh oh!

jherbel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jherbel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jherbel commented May 18, 2021

Uh oh!

Reviewers

Assignees