From 84e2041d3b6d9a240ddab3f39c7baed8e718bfab Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jan=20=C4=8Cern=C3=BD?= <jcerny@redhat.com>
Date: Tue, 20 Nov 2018 12:00:48 +0100
Subject: [PATCH 1/4] Prevent duplicate variables in Ansible Playbooks
The same XCCDF value can appear in multiple rules, which means
there can be same variable in multiple Ansible task. But when
we generate a Playbook we should put the variable only once.
Ansible produces a warning if there are multiple definitions
of the same variable.
---
src/XCCDF_POLICY/xccdf_policy_remediate.c | 180 ++++++++++++----------
1 file changed, 95 insertions(+), 85 deletions(-)
diff --git a/src/XCCDF_POLICY/xccdf_policy_remediate.c b/src/XCCDF_POLICY/xccdf_policy_remediate.c
index 65544c7f74..6ac566225c 100644
--- a/src/XCCDF_POLICY/xccdf_policy_remediate.c
+++ b/src/XCCDF_POLICY/xccdf_policy_remediate.c
@@ -134,11 +134,11 @@ static int _write_remediation_to_fd_and_free(int output_fd, const char* template
free(text);
return 1;
}
- }
- if (_write_text_to_fd(output_fd, "\n") != 0) {
- free(text);
- return 1;
+ if (_write_text_to_fd(output_fd, "\n") != 0) {
+ free(text);
+ return 1;
+ }
}
if (next_delim != NULL) {
@@ -610,7 +610,7 @@ static int _write_fix_missing_warning_to_fd(const char *sys, int output_fd, stru
}
}
-static inline int _xccdf_policy_rule_generate_fix_ansible(const char *template, int output_fd, const char *fix_text, bool ansible_variable_mode)
+static inline int _parse_ansible_fix(const char *fix_text, struct oscap_list *variables, struct oscap_list *tasks)
{
#if defined USE_REGEX_PCRE
// TODO: Tolerate different indentation styles in this regex
@@ -649,69 +649,53 @@ static inline int _xccdf_policy_rule_generate_fix_ansible(const char *template,
return 1;
}
- if (ansible_variable_mode) {
- // ovector[0] and [1] hold the start and end of the whole needle match
- // ovector[2] and [3] hold the start and end of the first capture group
- // ovector[4] and [5] hold the start and end of the second capture group
- char *variable_name = malloc((ovector[3] - ovector[2] + 1) * sizeof(char));
- memcpy(variable_name, &fix_text[ovector[2]], ovector[3] - ovector[2]);
- variable_name[ovector[3] - ovector[2]] = '\0';
+ // ovector[0] and [1] hold the start and end of the whole needle match
+ // ovector[2] and [3] hold the start and end of the first capture group
+ // ovector[4] and [5] hold the start and end of the second capture group
+ char *variable_name = malloc((ovector[3] - ovector[2] + 1) * sizeof(char));
+ memcpy(variable_name, &fix_text[ovector[2]], ovector[3] - ovector[2]);
+ variable_name[ovector[3] - ovector[2]] = '\0';
- char *variable_value = malloc((ovector[5] - ovector[4] + 1) * sizeof(char));
- memcpy(variable_value, &fix_text[ovector[4]], ovector[5] - ovector[4]);
- variable_value[ovector[5] - ovector[4]] = '\0';
+ char *variable_value = malloc((ovector[5] - ovector[4] + 1) * sizeof(char));
+ memcpy(variable_value, &fix_text[ovector[4]], ovector[5] - ovector[4]);
+ variable_value[ovector[5] - ovector[4]] = '\0';
- char *var_line = oscap_sprintf(" %s: %s", variable_name, variable_value);
+ char *var_line = oscap_sprintf(" %s: %s\n", variable_name, variable_value);
- free(variable_name);
- free(variable_value);
+ free(variable_name);
+ free(variable_value);
- if (_write_remediation_to_fd_and_free(output_fd, template, var_line) != 0) {
- pcre_free(re);
- return 1;
- }
- }
- else {
- // Remarks: ovector doesn't contain values relative to start_offset, it contains
- // absolute indices of fix_text.
- const int length_between_matches = ovector[0] - start_offset;
- char *remediation_part = malloc((length_between_matches + 1) * sizeof(char));
- memcpy(remediation_part, &fix_text[start_offset], length_between_matches);
- remediation_part[length_between_matches] = '\0';
- if (_write_remediation_to_fd_and_free(output_fd, template, remediation_part) != 0) {
- pcre_free(re);
- return 1;
- }
+ if (!oscap_list_contains(variables, var_line, (oscap_cmp_func) oscap_streq)) {
+ oscap_list_add(variables, var_line);
}
+ // Remarks: ovector doesn't contain values relative to start_offset, it contains
+ // absolute indices of fix_text.
+ const int length_between_matches = ovector[0] - start_offset;
+ char *remediation_part = malloc((length_between_matches + 1) * sizeof(char));
+ memcpy(remediation_part, &fix_text[start_offset], length_between_matches);
+ remediation_part[length_between_matches] = '\0';
+ oscap_list_add(tasks, remediation_part);
+
start_offset = ovector[1]; // next time start after the entire pattern
}
- if (!ansible_variable_mode && fix_text_len - start_offset > 0) {
+ if (fix_text_len - start_offset > 0) {
char *remediation_part = malloc((fix_text_len - start_offset + 1) * sizeof(char));
memcpy(remediation_part, &fix_text[start_offset], fix_text_len - start_offset);
remediation_part[fix_text_len - start_offset] = '\0';
-
- if (_write_remediation_to_fd_and_free(output_fd, template, remediation_part) != 0) {
- pcre_free(re);
- return 1;
- }
+ oscap_list_add(tasks, remediation_part);
}
pcre_free(re);
return 0;
#else
// TODO: Implement the post-process for posix regex as well
- if (ansible_variable_mode) {
- // this is not implemented so we don't write anything out for variables
- return 0;
- }
- else
- return _write_remediation_to_fd_and_free(output_fd, template, fix_text);
+ oscap_list_add(tasks, strdup(fix_text));
#endif
}
-static inline int _xccdf_policy_rule_generate_fix(struct xccdf_policy *policy, struct xccdf_rule *rule, const char *template, int output_fd, unsigned int current, unsigned int total, bool ansible_variable_mode)
+static int _xccdf_policy_rule_get_fix_text(struct xccdf_policy *policy, struct xccdf_rule *rule, const char *template, char **fix_text)
{
// Ensure that given Rule is selected and applicable (CPE).
const bool is_selected = xccdf_policy_is_item_selected(policy, xccdf_rule_get_id(rule));
@@ -722,15 +706,8 @@ static inline int _xccdf_policy_rule_generate_fix(struct xccdf_policy *policy, s
// Find the most suitable fix.
const struct xccdf_fix *fix = _find_fix_for_template(policy, rule, template);
if (fix == NULL) {
- int ret = _write_fix_header_to_fd(template, output_fd, rule, current, total);
- if (ret != 0)
- return ret;
- ret = _write_fix_missing_warning_to_fd(template, output_fd, rule);
- if (ret != 0)
- return ret;
- ret = _write_fix_footer_to_fd(template, output_fd, rule);
dI("No fix element was found for Rule/@id=\"%s\"", xccdf_rule_get_id(rule));
- return ret;
+ return 0;
}
dI("Processing a fix for Rule/@id=\"%s\"", xccdf_rule_get_id(rule));
@@ -744,37 +721,55 @@ static inline int _xccdf_policy_rule_generate_fix(struct xccdf_policy *policy, s
return res == 1; // Value 2 indicates warning.
}
// Refine. Resolve XML comments, CDATA and remaining elements
- char *fix_text = NULL;
- if (_xccdf_fix_decode_xml(cfix, &fix_text) != 0) {
+ if (_xccdf_fix_decode_xml(cfix, fix_text) != 0) {
oscap_seterr(OSCAP_EFAMILY_OSCAP, "A fix element for Rule/@id=\"%s\" contains unresolved child elements.",
xccdf_rule_get_id(rule));
xccdf_fix_free(cfix);
return 1;
}
xccdf_fix_free(cfix);
+ return 0;
+}
- int ret = _write_fix_header_to_fd(template, output_fd, rule, current, total);
+static int _xccdf_policy_rule_generate_fix(struct xccdf_policy *policy, struct xccdf_rule *rule, const char *template, int output_fd, unsigned int current, unsigned int total, bool ansible_variable_mode)
+{
+ char *fix_text = NULL;
+ int ret = _xccdf_policy_rule_get_fix_text(policy, rule, template, &fix_text);
+ if (fix_text == NULL || ret != 0) {
+ ret = _write_fix_header_to_fd(template, output_fd, rule, current, total);
+ if (ret != 0) {
+ return ret;
+ }
+ ret = _write_fix_missing_warning_to_fd(template, output_fd, rule);
+ if (ret != 0) {
+ return ret;
+ }
+ ret = _write_fix_footer_to_fd(template, output_fd, rule);
+ return ret;
+ }
+ ret = _write_fix_header_to_fd(template, output_fd, rule, current, total);
if (ret != 0) {
free(fix_text);
return ret;
}
+ ret = _write_remediation_to_fd_and_free(output_fd, template, fix_text);
+ if (ret != 0) {
+ return ret;
+ }
+ ret = _write_fix_footer_to_fd(template, output_fd, rule);
+ return ret;
+}
- if (oscap_streq(template, "urn:xccdf:fix:script:ansible")) {
- // Ansible is special because we have two output modes, variable and
- // task mode. In both cases we have to do post processing.
- // We can't recover the return value (it is rewritten by a later call) to _write_fix_footer_to_fd
- _xccdf_policy_rule_generate_fix_ansible(template, output_fd, fix_text, ansible_variable_mode);
- free(fix_text);
- }
- else {
- ret = _write_remediation_to_fd_and_free(output_fd, template, fix_text);
- if (ret != 0) {
- return ret;
- }
+static int _xccdf_policy_rule_generate_ansible_fix(struct xccdf_policy *policy, struct xccdf_rule *rule, const char *template, struct oscap_list *variables, struct oscap_list *tasks)
+{
+ char *fix_text = NULL;
+ int ret = _xccdf_policy_rule_get_fix_text(policy, rule, template, &fix_text);
+ if (fix_text == NULL) {
+ return ret;
}
-
- ret = _write_fix_footer_to_fd(template, output_fd, rule);
+ ret = _parse_ansible_fix(fix_text, variables, tasks);
+ free(fix_text);
return ret;
}
@@ -988,30 +983,45 @@ int xccdf_policy_generate_fix(struct xccdf_policy *policy, struct xccdf_result *
// In Ansible we have to generate variables first and then tasks
if (strcmp(sys, "urn:xccdf:fix:script:ansible") == 0) {
+ struct oscap_list *variables = oscap_list_new();
+ struct oscap_list *tasks = oscap_list_new();
+ struct oscap_iterator *rules_to_fix_it = oscap_iterator_new(rules_to_fix);
+ while (oscap_iterator_has_more(rules_to_fix_it)) {
+ struct xccdf_rule *rule = (struct xccdf_rule*)oscap_iterator_next(rules_to_fix_it);
+ ret = _xccdf_policy_rule_generate_ansible_fix(policy, rule, sys, variables, tasks);
+ if (ret != 0)
+ break;
+ }
+ oscap_iterator_free(rules_to_fix_it);
+
_write_text_to_fd(output_fd, " vars:\n");
+ struct oscap_iterator *variables_it = oscap_iterator_new(variables);
+ while(oscap_iterator_has_more(variables_it)) {
+ char *var_line = (char *) oscap_iterator_next(variables_it);
+ _write_text_to_fd(output_fd, var_line);
+ }
+ oscap_iterator_free(variables_it);
+ oscap_list_free(variables, free);
+ _write_text_to_fd(output_fd, " tasks:\n");
+ struct oscap_iterator *tasks_it = oscap_iterator_new(tasks);
+ while(oscap_iterator_has_more(tasks_it)) {
+ char *var_line = strdup((char *) oscap_iterator_next(tasks_it));
+ _write_remediation_to_fd_and_free(output_fd, sys, var_line);
+ }
+ oscap_iterator_free(tasks_it);
+ oscap_list_free(tasks, free);
+ } else {
unsigned int current = 1;
struct oscap_iterator *rules_to_fix_it = oscap_iterator_new(rules_to_fix);
while (oscap_iterator_has_more(rules_to_fix_it)) {
struct xccdf_rule *rule = (struct xccdf_rule*)oscap_iterator_next(rules_to_fix_it);
- ret = _xccdf_policy_rule_generate_fix(policy, rule, sys, output_fd, current++, total, true);
+ ret = _xccdf_policy_rule_generate_fix(policy, rule, sys, output_fd, current++, total, false);
if (ret != 0)
break;
}
oscap_iterator_free(rules_to_fix_it);
-
- _write_text_to_fd(output_fd, " tasks:\n");
- }
-
- unsigned int current = 1;
- struct oscap_iterator *rules_to_fix_it = oscap_iterator_new(rules_to_fix);
- while (oscap_iterator_has_more(rules_to_fix_it)) {
- struct xccdf_rule *rule = (struct xccdf_rule*)oscap_iterator_next(rules_to_fix_it);
- ret = _xccdf_policy_rule_generate_fix(policy, rule, sys, output_fd, current++, total, false);
- if (ret != 0)
- break;
}
- oscap_iterator_free(rules_to_fix_it);
oscap_list_free(rules_to_fix, NULL);
From 8badfbf30f423ca2f9452ab3ac6daf8bb8d850e3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jan=20=C4=8Cern=C3=BD?= <jcerny@redhat.com>
Date: Thu, 22 Nov 2018 13:43:40 +0100
Subject: [PATCH 2/4] Remove unused parameter
This parameter is not used in this function. The function is static
and it is used only at one place, so we can safely omit this parameter.
---
src/XCCDF_POLICY/xccdf_policy_remediate.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/XCCDF_POLICY/xccdf_policy_remediate.c b/src/XCCDF_POLICY/xccdf_policy_remediate.c
index 6ac566225c..520e9ff4de 100644
--- a/src/XCCDF_POLICY/xccdf_policy_remediate.c
+++ b/src/XCCDF_POLICY/xccdf_policy_remediate.c
@@ -731,7 +731,7 @@ static int _xccdf_policy_rule_get_fix_text(struct xccdf_policy *policy, struct x
return 0;
}
-static int _xccdf_policy_rule_generate_fix(struct xccdf_policy *policy, struct xccdf_rule *rule, const char *template, int output_fd, unsigned int current, unsigned int total, bool ansible_variable_mode)
+static int _xccdf_policy_rule_generate_fix(struct xccdf_policy *policy, struct xccdf_rule *rule, const char *template, int output_fd, unsigned int current, unsigned int total)
{
char *fix_text = NULL;
int ret = _xccdf_policy_rule_get_fix_text(policy, rule, template, &fix_text);
@@ -1016,7 +1016,7 @@ int xccdf_policy_generate_fix(struct xccdf_policy *policy, struct xccdf_result *
struct oscap_iterator *rules_to_fix_it = oscap_iterator_new(rules_to_fix);
while (oscap_iterator_has_more(rules_to_fix_it)) {
struct xccdf_rule *rule = (struct xccdf_rule*)oscap_iterator_next(rules_to_fix_it);
- ret = _xccdf_policy_rule_generate_fix(policy, rule, sys, output_fd, current++, total, false);
+ ret = _xccdf_policy_rule_generate_fix(policy, rule, sys, output_fd, current++, total);
if (ret != 0)
break;
}
From 90e0354c3eef165d8b467e6d843e9dfe1d264416 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jan=20=C4=8Cern=C3=BD?= <jcerny@redhat.com>
Date: Mon, 26 Nov 2018 18:18:47 +0100
Subject: [PATCH 3/4] Refactor: Extract functions
To improve the readability of the code we can refactor the code
that is specific to Ansible to a separate method. We can also make
a similar change in the `else` branch.
---
src/XCCDF_POLICY/xccdf_policy_remediate.c | 94 +++++++++++++----------
1 file changed, 54 insertions(+), 40 deletions(-)
diff --git a/src/XCCDF_POLICY/xccdf_policy_remediate.c b/src/XCCDF_POLICY/xccdf_policy_remediate.c
index 520e9ff4de..bf36523dfd 100644
--- a/src/XCCDF_POLICY/xccdf_policy_remediate.c
+++ b/src/XCCDF_POLICY/xccdf_policy_remediate.c
@@ -929,6 +929,56 @@ static int _write_script_header_to_fd(struct xccdf_policy *policy, struct xccdf_
}
}
+static int _xccdf_policy_generate_fix_ansible(struct oscap_list *rules_to_fix, struct xccdf_policy *policy, const char *sys, int output_fd)
+{
+ int ret = 0;
+ struct oscap_list *variables = oscap_list_new();
+ struct oscap_list *tasks = oscap_list_new();
+ struct oscap_iterator *rules_to_fix_it = oscap_iterator_new(rules_to_fix);
+ while (oscap_iterator_has_more(rules_to_fix_it)) {
+ struct xccdf_rule *rule = (struct xccdf_rule*)oscap_iterator_next(rules_to_fix_it);
+ ret = _xccdf_policy_rule_generate_ansible_fix(policy, rule, sys, variables, tasks);
+ if (ret != 0)
+ break;
+ }
+ oscap_iterator_free(rules_to_fix_it);
+
+ _write_text_to_fd(output_fd, " vars:\n");
+ struct oscap_iterator *variables_it = oscap_iterator_new(variables);
+ while(oscap_iterator_has_more(variables_it)) {
+ char *var_line = (char *) oscap_iterator_next(variables_it);
+ _write_text_to_fd(output_fd, var_line);
+ }
+ oscap_iterator_free(variables_it);
+ oscap_list_free(variables, free);
+
+ _write_text_to_fd(output_fd, " tasks:\n");
+ struct oscap_iterator *tasks_it = oscap_iterator_new(tasks);
+ while(oscap_iterator_has_more(tasks_it)) {
+ char *var_line = strdup((char *) oscap_iterator_next(tasks_it));
+ _write_remediation_to_fd_and_free(output_fd, sys, var_line);
+ }
+ oscap_iterator_free(tasks_it);
+ oscap_list_free(tasks, free);
+ return ret;
+}
+
+static int _xccdf_policy_generate_fix_other(struct oscap_list *rules_to_fix, struct xccdf_policy *policy, const char *sys, int output_fd)
+{
+ int ret = 0;
+ const unsigned int total = oscap_list_get_itemcount(rules_to_fix);
+ unsigned int current = 1;
+ struct oscap_iterator *rules_to_fix_it = oscap_iterator_new(rules_to_fix);
+ while (oscap_iterator_has_more(rules_to_fix_it)) {
+ struct xccdf_rule *rule = (struct xccdf_rule *) oscap_iterator_next(rules_to_fix_it);
+ ret = _xccdf_policy_rule_generate_fix(policy, rule, sys, output_fd, current++, total);
+ if (ret != 0)
+ break;
+ }
+ oscap_iterator_free(rules_to_fix_it);
+ return ret;
+}
+
int xccdf_policy_generate_fix(struct xccdf_policy *policy, struct xccdf_result *result, const char *sys, int output_fd)
{
__attribute__nonnull__(policy);
@@ -979,48 +1029,12 @@ int xccdf_policy_generate_fix(struct xccdf_policy *policy, struct xccdf_result *
xccdf_rule_result_iterator_free(rr_it);
}
- const unsigned int total = oscap_list_get_itemcount(rules_to_fix);
-
- // In Ansible we have to generate variables first and then tasks
+ // Ansible Playbooks are generated using a different function because
+ // in Ansible we have to generate variables first and then tasks
if (strcmp(sys, "urn:xccdf:fix:script:ansible") == 0) {
- struct oscap_list *variables = oscap_list_new();
- struct oscap_list *tasks = oscap_list_new();
- struct oscap_iterator *rules_to_fix_it = oscap_iterator_new(rules_to_fix);
- while (oscap_iterator_has_more(rules_to_fix_it)) {
- struct xccdf_rule *rule = (struct xccdf_rule*)oscap_iterator_next(rules_to_fix_it);
- ret = _xccdf_policy_rule_generate_ansible_fix(policy, rule, sys, variables, tasks);
- if (ret != 0)
- break;
- }
- oscap_iterator_free(rules_to_fix_it);
-
- _write_text_to_fd(output_fd, " vars:\n");
- struct oscap_iterator *variables_it = oscap_iterator_new(variables);
- while(oscap_iterator_has_more(variables_it)) {
- char *var_line = (char *) oscap_iterator_next(variables_it);
- _write_text_to_fd(output_fd, var_line);
- }
- oscap_iterator_free(variables_it);
- oscap_list_free(variables, free);
-
- _write_text_to_fd(output_fd, " tasks:\n");
- struct oscap_iterator *tasks_it = oscap_iterator_new(tasks);
- while(oscap_iterator_has_more(tasks_it)) {
- char *var_line = strdup((char *) oscap_iterator_next(tasks_it));
- _write_remediation_to_fd_and_free(output_fd, sys, var_line);
- }
- oscap_iterator_free(tasks_it);
- oscap_list_free(tasks, free);
+ ret = _xccdf_policy_generate_fix_ansible(rules_to_fix, policy, sys, output_fd);
} else {
- unsigned int current = 1;
- struct oscap_iterator *rules_to_fix_it = oscap_iterator_new(rules_to_fix);
- while (oscap_iterator_has_more(rules_to_fix_it)) {
- struct xccdf_rule *rule = (struct xccdf_rule*)oscap_iterator_next(rules_to_fix_it);
- ret = _xccdf_policy_rule_generate_fix(policy, rule, sys, output_fd, current++, total);
- if (ret != 0)
- break;
- }
- oscap_iterator_free(rules_to_fix_it);
+ ret = _xccdf_policy_generate_fix_other(rules_to_fix, policy, sys, output_fd);
}
oscap_list_free(rules_to_fix, NULL);
From 2a11da0a6a1643669caadb72f1a217fafcfea6d5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jan=20=C4=8Cern=C3=BD?= <jcerny@redhat.com>
Date: Wed, 28 Nov 2018 14:19:12 +0100
Subject: [PATCH 4/4] Refactor: Remove duplicate code
---
src/XCCDF_POLICY/xccdf_policy_remediate.c | 23 +++++++----------------
1 file changed, 7 insertions(+), 16 deletions(-)
diff --git a/src/XCCDF_POLICY/xccdf_policy_remediate.c b/src/XCCDF_POLICY/xccdf_policy_remediate.c
index bf36523dfd..15f9453938 100644
--- a/src/XCCDF_POLICY/xccdf_policy_remediate.c
+++ b/src/XCCDF_POLICY/xccdf_policy_remediate.c
@@ -733,26 +733,17 @@ static int _xccdf_policy_rule_get_fix_text(struct xccdf_policy *policy, struct x
static int _xccdf_policy_rule_generate_fix(struct xccdf_policy *policy, struct xccdf_rule *rule, const char *template, int output_fd, unsigned int current, unsigned int total)
{
+ int ret = _write_fix_header_to_fd(template, output_fd, rule, current, total);
+ if (ret != 0) {
+ return ret;
+ }
char *fix_text = NULL;
- int ret = _xccdf_policy_rule_get_fix_text(policy, rule, template, &fix_text);
+ ret = _xccdf_policy_rule_get_fix_text(policy, rule, template, &fix_text);
if (fix_text == NULL || ret != 0) {
- ret = _write_fix_header_to_fd(template, output_fd, rule, current, total);
- if (ret != 0) {
- return ret;
- }
ret = _write_fix_missing_warning_to_fd(template, output_fd, rule);
- if (ret != 0) {
- return ret;
- }
- ret = _write_fix_footer_to_fd(template, output_fd, rule);
- return ret;
- }
- ret = _write_fix_header_to_fd(template, output_fd, rule, current, total);
- if (ret != 0) {
- free(fix_text);
- return ret;
+ } else {
+ ret = _write_remediation_to_fd_and_free(output_fd, template, fix_text);
}
- ret = _write_remediation_to_fd_and_free(output_fd, template, fix_text);
if (ret != 0) {
return ret;
}