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 the --dane option to the command definition ssl_cert #10196

Merged
merged 3 commits into from
Jan 8, 2025
Merged

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

merged 3 commits into from
Jan 8, 2025

Conversation

peteeckel
Copy link
Contributor

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
Contributor Author

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
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!

itl/plugins-contrib.d/web.conf Show resolved Hide resolved
@peteeckel
Copy link
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
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 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 oxzi and Al2Klimov and removed request for oxzi November 13, 2024 08:53
@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
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
Contributor Author

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
21 checks passed
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oxzi oxzi oxzi approved these changes

@Al2Klimov Al2Klimov Awaiting requested review from Al2Klimov

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