openshift / compliance-operator

Operator providing OpenShift cluster compliance checks
Apache License 2.0
110 stars 110 forks source link

Fix value-required handling + Add support for multi lines remediation(ex. NTP Servers) #700

Closed Vincent056 closed 3 years ago

Vincent056 commented 3 years ago

I previous used a wrong way to retrieve tailor profile config map, found this bug while writing contents for remediation templating

openshift-ci[bot] commented 3 years ago

Hi @Vincent056. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
JAORMX commented 3 years ago

/ok-to-test

Vincent056 commented 3 years ago

The code looks OK, but I wonder if we're at the point where we could start writing tests to catch issues like that?

Yeah I think I can start to write some cluster tests for that.

Vincent056 commented 3 years ago

For the second commit, I added support for remediation with multiple lines, this fix will help with NTP server remediation.

I changed the formate of remediation templating in the content.

In the CaC content machine config, before we url-encode the source data, we put everything needed as well as formatted variable string into source data, then the source data will get url-encoded in CaC with addition "{{ " at the beginning, and '" }}" at the end of source data. "{{ }}" is then identify in the compliance operator using regex match, we will then decode the source date in the compliance operator, and next we start use templating to find and replace variable. After the variable replacement with value, for simple variable substitution, we just use {{.var_name}} in CaC content, if the the variable is an array, we can use {{range $element:=.var_multiple_time_servers|toArrayByComma}}Server {{$element}} {{end}} to be able have multiple line for each values in that variable array in CaC.

To summary it up, everything as well as templating used in compliance operator will all get url-encoded in CaC, and added "{{ " at the beginning, and '" }}" at the end of url-encoded data

I am providing a example for multiple line remediation, for example to Specific NTP servers:

In CaC, the code for generating that remediation is here:

{{% macro kubernetes_machineconfig_chrony_config() -%}}
apiVersion: machineconfiguration.openshift.io/v1
kind: MachineConfig
metadata:
  annotations:
    complianceascode.io/value-input-required: var_multiple_time_servers
spec:
  config:
    ignition:
      version: 3.1.0
    storage:
      files:
      - contents:
          source: data:,{{ %23%20Allow%20for%20extra%20configuration%20files.%20This%20is%20useful%0A%23%20for%20admins%20specifying%20their%20own%20NTP%20servers%0Ainclude%20/etc/chrony.d/%2A.conf%0A%0A%23%20Set%20chronyd%20as%20client-only.%0Aport%200%0A%0A%23%20Disable%20chronyc%20from%20the%20network%0Acmdport%200%0A%0A%23%20Record%20the%20rate%20at%20which%20the%20system%20clock%20gains/losses%20time.%0Adriftfile%20/var/lib/chrony/drift%0A%0A%23%20Allow%20the%20system%20clock%20to%20be%20stepped%20in%20the%20first%20three%20updates%0A%23%20if%20its%20offset%20is%20larger%20than%201%20second.%0Amakestep%201.0%203%0A%0A%23%20Enable%20kernel%20synchronization%20of%20the%20real-time%20clock%20%28RTC%29.%0Artcsync%0A%0A%23%20Enable%20hardware%20timestamping%20on%20all%20interfaces%20that%20support%20it.%0A%23hwtimestamp%20%2A%0A%0A%23%20Increase%20the%20minimum%20number%20of%20selectable%20sources%20required%20to%20adjust%0A%23%20the%20system%20clock.%0A%23minsources%202%0A%0A%23%20Allow%20NTP%20client%20access%20from%20local%20network.%0A%23allow%20192.168.0.0/16%0A%0A%23%20Serve%20time%20even%20if%20not%20synchronized%20to%20a%20time%20source.%0A%23local%20stratum%2010%0A%0A%23%20Require%20authentication%20%28nts%20or%20key%20option%29%20for%20all%20NTP%20sources.%0A%23authselectmode%20require%0A%0A%23%20Specify%20file%20containing%20keys%20for%20NTP%20authentication.%0Akeyfile%20/etc/chrony.keys%0A%0A%23%20Insert/delete%20leap%20seconds%20by%20slewing%20instead%20of%20stepping.%0A%23leapsecmode%20slew%0A%0A%23%20Get%20TAI-UTC%20offset%20and%20leap%20seconds%20from%20the%20system%20tz%20database.%0Aleapsectz%20right/UTC%0A%0A%23%20Specify%20directory%20for%20log%20files.%0Alogdir%20/var/log/chrony%0A%0A%23%20Select%20which%20information%20is%20logged.%0A%23log%20measurements%20statistics%20tracking }}
        mode: 420
        overwrite: true
        path: /etc/chrony.conf
      - contents:
          source: data:,
        mode: 420
        overwrite: true
        path: /etc/chrony.d/.mco-keep
      - contents:
          source: data:,{{ {{{ url_encode(ntp_server()) }}} }}
        mode: 420
        overwrite: true
        path: /etc/chrony.d/ntp-server.conf
{{%- endmacro %}}

{{% macro ntp_server() -%}}
#
# This file controls the configuration of the ntp server
# {{.var_multiple_time_servers}} we have to put variable array name here
{{range $element:=.var_multiple_time_servers|toArrayByComma}}Server {{$element}}
{{end}}
{{%- endmacro %}}

We have var_multiple_time_servers which contains multiple values separated by comma. ex. pool1, pool2, pool3 And if we use the code above, in compliance operator var_multiple_time_servers will be converted into array separated by comma, and we can have remediation as:

server pool1
server pool2
server pool3

Because we are iterating through templating tree to get variable name, we will need to put {{.var_multiple_time_servers}} in the comma section for multi line remediations. For those normal single line remediation templating string, it is not needed.

For single line remediations templating are like following:

log_format = ENRICHED
flush = {{.var_auditd_flush}}
freq = 50
max_log_file = {{.var_auditd_max_log_file}}
num_logs = {{.var_auditd_num_logs}}
priority_boost = 4
name_format = hostname
##name = mydomain
max_log_file_action = {{.var_auditd_max_log_file_action}}
space_left = {{.var_auditd_space_left}}
space_left_action = {{.var_auditd_space_left_action}}
verify_email = yes
action_mail_acct = {{.var_auditd_action_mail_acct}}
admin_space_left = 50
admin_space_left_action = syslog
disk_full_action = {{.var_auditd_disk_full_action}}
disk_error_action = {{.var_auditd_disk_error_action}}
use_libwrap = yes
##tcp_listen_port = 60
tcp_listen_queue = 5
tcp_max_per_addr = 1
##tcp_client_ports = 1024-65535

Please let me know what you guys think when you have time @JAORMX @jhrozek @mrogers950, thanks!

JAORMX commented 3 years ago

This LGTM, I'll let @mrogers950 or @jhrozek merge if they see fit.

mrogers950 commented 3 years ago

Nice! /lgtm

openshift-ci[bot] commented 3 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrogers950, Vincent056

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/openshift/compliance-operator/blob/master/OWNERS)~~ [mrogers950] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment