close
Skip to content

Added the --dane option to the command definition ssl_cert#10196

Merged
oxzi merged 3 commits intoIcinga:masterfrom
peteeckel:fix/add-dane-to-ssl-cert
Jan 8, 2025
Merged

Added the --dane option to the command definition ssl_cert#10196
oxzi merged 3 commits intoIcinga:masterfrom
peteeckel:fix/add-dane-to-ssl-cert

Conversation

@peteeckel
Copy link
Copy Markdown
Contributor

@peteeckel peteeckel commented Oct 22, 2024

fixes #10195

Added the ssl_cert_date option to the ssl_cert command definition. Values can be an empty string or a specification of the TLSA record type to check (201, 301, 302, or 311).

@cla-bot cla-bot Bot added the cla/signed label Oct 22, 2024
@peteeckel peteeckel force-pushed the fix/add-dane-to-ssl-cert branch 2 times, most recently from 75ca700 to 76d1b70 Compare October 22, 2024 15:58
@peteeckel
Copy link
Copy Markdown
Contributor Author

peteeckel commented Oct 22, 2024

I don't have the slightest idea why the windows tests fail ... very unlikely to have anything to do with the code change.

@oxzi oxzi added the area/itl Template Library CheckCommands label Oct 23, 2024
oxzi
oxzi requested changes Oct 23, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@oxzi oxzi left a 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!

Comment thread itl/plugins-contrib.d/web.conf
@peteeckel
Copy link
Copy Markdown
Contributor Author

peteeckel commented Oct 23, 2024 •
edited
Loading

Hi,

thanks. repeat_key = false actually ins't unnecessary, as the default value is true and the option --dane is not repeatable - do you still want it removed?

image

@oxzi
Copy link
Copy Markdown
Member

oxzi commented Oct 23, 2024

repeat_key = false actually ins't unnecessary, as the default value is true and the option --dane is not repeatable - do you still want it removed?

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.

oxzi
oxzi previously approved these changes Oct 23, 2024
View reviewed changes
Al2Klimov
Al2Klimov reviewed Oct 23, 2024
View reviewed changes
Comment thread doc/10-icinga-template-library.md Outdated
@Al2Klimov Al2Klimov requested a review from oxzi October 23, 2024 14:48
@peteeckel peteeckel force-pushed the fix/add-dane-to-ssl-cert branch from 76d1b70 to b63ecfe Compare October 23, 2024 14:52
@yhabteab yhabteab added this to the 2.15.0 milestone Nov 13, 2024
@yhabteab yhabteab added the enhancement New feature or request label Nov 13, 2024
@yhabteab yhabteab requested review from Al2Klimov and oxzi and removed request for oxzi November 13, 2024 08:53
oxzi
oxzi reviewed Nov 13, 2024
View reviewed changes
Comment thread doc/10-icinga-template-library.md Outdated
@Al2Klimov Al2Klimov requested a review from oxzi November 13, 2024 17:00
oxzi added a commit to oxzi/check_ssl_cert that referenced this pull request Nov 14, 2024
@oxzi
Allow empty argument for --dane
1f11fc7 
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)
@oxzi oxzi enabled auto-merge (squash) January 8, 2025 08:48
@oxzi oxzi force-pushed the fix/add-dane-to-ssl-cert branch from f94aa81 to ec9e468 Compare January 8, 2025 08:51
@oxzi
Copy link
Copy Markdown
Member

oxzi commented Jan 8, 2025

I have rebased your PR against the current master to contain all necessary checks to satisfy the auto-merge.

@peteeckel
Copy link
Copy Markdown
Contributor Author

peteeckel commented Jan 8, 2025

I have rebased your PR against the current master to contain all necessary checks to satisfy the auto-merge.

Perfect, thanks!

@oxzi oxzi merged commit 920ba0b into Icinga:master Jan 8, 2025
@peteeckel peteeckel deleted the fix/add-dane-to-ssl-cert branch January 8, 2025 10:48
Al2Klimov pushed a commit that referenced this pull request Jan 21, 2025
@peteeckel @Al2Klimov
Added the --dane option to the command definition ssl_cert (#10196)
ba3363c
@yhabteab yhabteab removed the request for review from Al2Klimov June 11, 2025 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Al2Klimov Al2Klimov Al2Klimov left review comments

@oxzi oxzi oxzi approved these changes

Assignees

No one assigned

Labels

area/itl Template Library CheckCommands cla/signed enhancement New feature or request

Projects

None yet

Milestone

2.15.0

Development

Successfully merging this pull request may close these issues.

ssl_cert check does not have the option to check DANE

4 participants

@peteeckel @oxzi @Al2Klimov @yhabteab