-
Notifications
You must be signed in to change notification settings - Fork 581
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
Added the --dane
option to the command definition ssl_cert
#10196
base: master
Are you sure you want to change the base?
Conversation
75ca700
to
76d1b70
Compare
I don't have the slightest idea why the windows tests fail ... very unlikely to have anything to do with the code change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your Pull Request!
I am a bit uncertain about the failing Windows tests at the moment, but these are not related to your change. Please remove the unnecessary repeat_key
, otherwise it looks good to me. Thanks!
You are totally right. I missed something up, sorry. Please keep it as it is. Regarding the failing Windows Jobs, it seems the access permissions for the Windows packaging repository were changed. This, however, has nothing to do with your PR. |
doc/10-icinga-template-library.md
Outdated
@@ -5939,6 +5939,7 @@ ssl_cert_ignore_ocsp_errors | **Optional.** Continue if the OCSP status cannot | |||
ssl_cert_ignore_ocsp_timeout | **Optional.** Ignore OCSP result when timeout occurs while checking. | |||
ssl_cert_ignore_sct | **Optional.** Do not check for signed certificate timestamps. | |||
ssl_cert_ignore_tls_renegotiation | **Optional.** Do not check for renegotiation. | |||
ssl_cert_dane | **Optional.** Verify that valid DANE records exist {211,301,302,311 or empty string}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ssl_cert_dane | **Optional.** Verify that valid DANE records exist {211,301,302,311 or empty string}. | |
ssl_cert_dane | **Optional.** Verify that valid DANE records exist (211, 301, 302, 311 or empty string). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aesthetically, I'm with you.
Same file, a couple of lines further up:
ssl_cert_ssl_version | **Optional.** Force specific SSL version out of {ssl2,ssl3,tls1,tls1_1,tls1_2}.
ssl_cert_disable_ssl_versions | **Optional.** Disable specific SSL versions out of {ssl2,ssl3,tls1,tls1_1,tls1_2}. Multiple versions can be given as array.
I tried to follow suit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it's
ssl_cert_dane | **Optional.** Verify that valid DANE records exist {211,301,302,311 or empty string}. | |
ssl_cert_dane | **Optional.** Verify that valid DANE records exist ({211,301,302,311} or empty string). |
IMAO
76d1b70
to
b63ecfe
Compare
doc/10-icinga-template-library.md
Outdated
@@ -5939,6 +5939,7 @@ ssl_cert_ignore_ocsp_errors | **Optional.** Continue if the OCSP status cannot | |||
ssl_cert_ignore_ocsp_timeout | **Optional.** Ignore OCSP result when timeout occurs while checking. | |||
ssl_cert_ignore_sct | **Optional.** Do not check for signed certificate timestamps. | |||
ssl_cert_ignore_tls_renegotiation | **Optional.** Do not check for renegotiation. | |||
ssl_cert_dane | **Optional.** Verify that valid DANE records exist ({211,301,302,311} or empty string). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pardon, but there seems also to be 312
, https://github.com/matteocorti/check_ssl_cert/blob/c3e6e3014e6ca4db24dbd65853d62d53a6a62efa/README.md?plain=1#L66.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, true. Wasn't there in my version:
--dane verify that valid DANE records exist (since OpenSSL 1.1.0)
--dane 211 verify that a valid DANE-TA(2) SPKI(1) SHA2-256(1) TLSA record exists
--dane 301 verify that a valid DANE-EE(3) Cert(0) SHA2-256(1) TLSA record exists
--dane 302 verify that a valid DANE-EE(3) Cert(0) SHA2-512(2) TLSA record exists
--dane 311 verify that a valid DANE-EE(3) SPKI(1) SHA2-256(1) TLSA record exists
-d,--debug produces debugging output
I'll prepare a documentation update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! There is also the part that the empty string for the --dane
flag results in another behavior as just passing the flag. I had this in my initial answer, but was unaware if this may have been the result of an incomplete install. On a fresh Debian, please compare:
root@ec5dfb2ea47b:/# ./check --dane -H example.com
SSL_CERT CRITICAL: No matching TLSA records found at _443._tcp.example.com
root@ec5dfb2ea47b:/# ./check --dane "" -H example.com
SSL_CERT OK - example.com:443, https, x509 certificate 'www.example.org' (example.com) from 'DigiCert Inc' valid until Mar 1 23:59:59 2025 GMT (expires in 108 days)|days_chain_elem1=108;20;15;; days_chain_elem2=2327;20;15;;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That raises an interesting question: Is there a way to make an Icinga CheckCommand
not put quotes around an empty argument?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently the whole option is dropped by the plugin for --dane ""
, as opposed to --dane
. Could be a bug in the plugin itself as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That raises an interesting question: Is there a way to make an Icinga
CheckCommand
not put quotes around an empty argument?
It's not like Icinga puts quotes there, but use its value - an empty string - as an argument for execve. Thus, without having verified this claim in the source, your check command should result in something like execve("/path/to/check_ssl_cert", ["--dane", ""], …)
.
If you want to skip the value, your check command's argument could leave out the value
and only have set_if
. However, in this case --dane
can either be a flag or carry a value. One can work around this limitation as shown in this comment at a similar PR.
Apparently the whole option is dropped by the plugin for
--dane ""
, as opposed to--dane
. Could be a bug in the plugin itself as well.
Taking a quick look at the implementation, I would say it works as intended: in this case there is a value after --dane
and it is not a flag (does not start with "-"), thus $DANE
will be set to this value, effectively being the empty string. This, however, results in disabling the dane feature 🙃
Btw, the project itself also ships an Icinga check command, addressing the dane flags a bit more straight forward. However, I prefer your approach as otherwise one ends up with multiple variables/flags for mutual exclusive things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HI @oxzi, the workaround you linked to seems to be the most sensible solution, thanks a lot! I'll update my test command as soon as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your feedback and no hurry. I just took a second look at the check command's implementation and it should be a trivial fix. I may report it upstream and let they decide if they want more flexibility for the argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As promised, I gave it a shot: matteocorti/check_ssl_cert#526
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. Let's try that first, it's definitely the more elegant and straightforward solution.
The "--dane" option can be used both as a flag and with an argument. In its current implementation, it is even a special case for flags with variable numbers of arguments. At an Icinga 2 ITL PR by GitHub user @peteeckel, an unexpected behavior was seen when calling check_ssl_cert with "--dane" followed by an empty argument[0], as so: $ ./check_ssl_cert --dane "" If the empty argument was used, the --dane option was effectively useless. This is due to the argument counting/checking code, not expecting an empty second argument, setting DANE="", which disables it. This change allows an empty second argument, which will then be swallowed. For the other options with variable numbers of arguments, this does not seem to apply. [0]: Icinga/icinga2#10196 (comment)
fixes #10195
Added the
ssl_cert_date
option to thessl_cert
command definition. Values can be an empty string or a specification of the TLSA record type to check (201, 301, 302, or 311).