check_dig: add -E/--require-flags and -X/--forbid-flags #2165
Conversation
Introduced helper functions for flag parsing.
-E, --require-flags=LIST
Comma-separated dig flags that must be present (e.g. 'aa,qr')
-X, --forbid-flags=LIST
Comma-separated dig flags that must NOT be present
|
Hello @RincewindsHat, is there something I can do to accelerate the review process? Best regards, |
|
Hi @Decstasy, In general this looks sane to me, but then again I am no expert on DNS and that makes it harder for me to judge the general idea. For now, I have some change requests in a more general spirit. Please run You introduced some new functions here and from a quick glance they seem to receive multiple pointer arguments, where at least one of those is actually the output and that is a pattern I would like to avoid to make so I do not have read through the whole function to find out what goes where. Also some documentation for the functions (some lines of comments above the declaration would suffice) would be nice, a quick summarization what the function does (or should do) and what the Inputs and Outputs are. It is very considerate of you to implement the |
|
Offtopic note: Through this PR I learned that |
…tured return types and add documentation. This refactors all helper functions related to DNS flag extraction and validation: - Introduce a new `flag_list` struct used as unified return type for functions producing multiple output values - Replace all functions using output pointers (`char ***`, `size_t *`) with functions returning `flag_list` - Update callers in `main()` and the flag validation logic - Add documentation comments describing purpose, inputs and outputs for all new helper functions - Consolidate memory handling through a single `free_flag_list()` helper - Apply clang-format This brings more clarity, avoids hidden output and aligns with the review request for cleaner functions input/output.
|
Hey @RincewindsHat, A few energy drinks later, it’s done. I refactored all the helpers and the corresponding calls in main. I also added inline comments and proper function documentation. Check out the commit message for more details, and feel free to adjust smaller things if you think it’s necessary. |
|
Hey, that was quite quick. I did not expect an answer so soon :-) To try whether this works as I thought I wrote this patch: index 2db0f66b..9ea19e6a 100644
--- a/plugins/check_dig.c
+++ b/plugins/check_dig.c
@@ -58,10 +58,6 @@ void print_usage(void);
static int verbose = 0;
/* helpers for flag parsing */
-typedef struct {
- char **items;
- size_t count;
-} flag_list;
static flag_list parse_flags_line(const char *line);
static flag_list split_csv_trim(const char *csv);
static bool flag_list_contains(const flag_list *list, const char *needle);
@@ -208,49 +204,36 @@ int main(int argc, char **argv) {
}
/* Optional: evaluate dig flags only if -E/-X were provided */
- if ((config.require_flags && *config.require_flags) ||
- (config.forbid_flags && *config.forbid_flags)) {
-
+ if ((config.require_flags.count > 0) || (config.forbid_flags.count > 0)) {
if (dig_flags.count > 0) {
-
- if (config.require_flags && *config.require_flags) {
- flag_list req = split_csv_trim(config.require_flags);
-
- for (size_t r = 0; r < req.count; r++) {
- if (!flag_list_contains(&dig_flags, req.items[r])) {
- result = STATE_CRITICAL;
- if (!msg) {
- xasprintf(&msg, _("Missing required DNS flag: %s"), req.items[r]);
- } else {
- char *newmsg = NULL;
- xasprintf(&newmsg, _("%s; missing required DNS flag: %s"), msg,
- req.items[r]);
- msg = newmsg;
- }
+ for (size_t r = 0; r < config.require_flags.count; r++) {
+ if (!flag_list_contains(&dig_flags, config.require_flags.items[r])) {
+ result = STATE_CRITICAL;
+ if (!msg) {
+ xasprintf(&msg, _("Missing required DNS flag: %s"),
+ config.require_flags.items[r]);
+ } else {
+ char *newmsg = NULL;
+ xasprintf(&newmsg, _("%s; missing required DNS flag: %s"), msg,
+ config.require_flags.items[r]);
+ msg = newmsg;
}
}
-
- free_flag_list(&req);
}
- if (config.forbid_flags && *config.forbid_flags) {
- flag_list bad = split_csv_trim(config.forbid_flags);
-
- for (size_t r = 0; r < bad.count; r++) {
- if (flag_list_contains(&dig_flags, bad.items[r])) {
- result = STATE_CRITICAL;
- if (!msg) {
- xasprintf(&msg, _("Forbidden DNS flag present: %s"), bad.items[r]);
- } else {
- char *newmsg = NULL;
- xasprintf(&newmsg, _("%s; forbidden DNS flag present: %s"), msg,
- bad.items[r]);
- msg = newmsg;
- }
+ for (size_t r = 0; r < config.forbid_flags.count; r++) {
+ if (flag_list_contains(&dig_flags, config.forbid_flags.items[r])) {
+ result = STATE_CRITICAL;
+ if (!msg) {
+ xasprintf(&msg, _("Forbidden DNS flag present: %s"),
+ config.forbid_flags.items[r]);
+ } else {
+ char *newmsg = NULL;
+ xasprintf(&newmsg, _("%s; forbidden DNS flag present: %s"), msg,
+ config.forbid_flags.items[r]);
+ msg = newmsg;
}
}
-
- free_flag_list(&bad);
}
}
}
@@ -351,10 +334,10 @@ check_dig_config_wrapper process_arguments(int argc, char **argv) {
result.config.dig_args = strdup(optarg);
break;
case 'E': /* require flags */
- result.config.require_flags = strdup(optarg);
+ result.config.require_flags = split_csv_trim(optarg);
break;
case 'X': /* forbid flags */
- result.config.forbid_flags = strdup(optarg);
+ result.config.forbid_flags = split_csv_trim(optarg);
break;
case 'v': /* verbose */
verbose++;
diff --git a/plugins/check_dig.d/config.h b/plugins/check_dig.d/config.h
index 392848e5..dd1f58b5 100644
--- a/plugins/check_dig.d/config.h
+++ b/plugins/check_dig.d/config.h
@@ -7,6 +7,11 @@
#define DEFAULT_PORT 53
#define DEFAULT_TRIES 2
+typedef struct {
+ char **items;
+ size_t count;
+} flag_list;
+
typedef struct {
char *query_address;
char *record_type;
@@ -19,8 +24,8 @@ typedef struct {
double warning_interval;
double critical_interval;
- char *require_flags;
- char *forbid_flags;
+ flag_list require_flags;
+ flag_list forbid_flags;
} check_dig_config;
check_dig_config check_dig_config_init() {
@@ -36,9 +41,8 @@ check_dig_config check_dig_config_init() {
.warning_interval = UNDEFINED,
.critical_interval = UNDEFINED,
- .require_flags = NULL,
- .forbid_flags = NULL,
-
+ .require_flags = {.count = 0, .items = NULL},
+ .forbid_flags = {.count = 0, .items = NULL},
};
return tmp;
}feel free to apply it (I forgot how to add patches to a PR) or not, as you wish. Thanks for the work! |
- Moved flag_list definition to config.h for global use - Flags are parsed once during argument processing instead of repeatedly in the evaluation loop - Removed temporary allocations (req, bad) and their corresponding cleanup paths
|
That is a nice patch. I applied it, tested it, found no issues and pushed it with a new commit. Have a great weekend, |
|
Thanks again for the contribution and the cooperation |
3 participants
Hello maintainers,
this PR adds two optional switches to validate DNS header flags from
digoutput:-E, --require-flags=LIST— flags that must be present (e.g.aa,qr)-X, --forbid-flags=LIST— flags that must not be present (e.g.rd)Includes small helpers to parse the
flags:line and CSV lists. Help/usage updated. No behavior change if the options are not used.Why
Operational/SecOps cases often need to assert
aaon authoritative servers or ensurerdisn’t set. So I guess we need such a feature.Usage
Testing
digoptions (+norecurse, time/retry).rddepends on your+norecurse/recursion settings, as expected.Review
I don’t write C here every day, so please take a close look, especially around memory handling (e.g.,
realloc/strdup). I’m happy to adjust style/i18n/helper placement, or switch to aggregated error messages for multiple flags if preferred.Related / closes #1097
Thanks!
Dennis