goss-org / goss

Quick and Easy server testing/validation
https://goss.rocks
Apache License 2.0
5.62k stars 472 forks source link

incomprehensible behavior when testing with different versions of goss #845

Closed sysop200 closed 6 months ago

sysop200 commented 1 year ago

Describe the error I carry out testing as described in this repository: https://github.com/ansible-lockdown/RHEL8-CIS-Audit When using version 0.3.23, the report turns out good: "expected": [ "/^MaxAuthTries [1-4]/", "!/^MaxAuthTries [5-9]/" ], "found": [ "!/^MaxAuthTries [5-9]/" ],

When using version >0.4.0, the field does not contain the correct queries: "matcher-result": { *### _"Actual": "object: bytes.Reader",_** "Expected": [ "/^MaxAuthTries [1-4]/", "!/^MaxAuthTries [5-9]/" ], "ExtraElements": null, "Message": "to have patterns", "MissingElements": [ "/^MaxAuthTries [1-4]/"

How ​​to play Download the repository and check your Linux computer with standard settings

aelsabbahy commented 1 year ago

Hello, thank you for opening this.

Can you give a simple reproducible Goss.yaml example?

Also, what's the expectation vs incorrect result? At a quick glance of this issue text it appears both are saying the same thing, but in a different way? Seems v0.3.x, says what it expected and found. While version 0.4.x says what it expected and missing/failed?

Also, I assume this is using --format json?

aelsabbahy commented 1 year ago

Upon rereading the message, I realized it's the "Actual": "object: *bytes.Reader" that is causing confusion.

I'll explain the problem first and perhaps there's a suggestion for a better message.

Some tests return an Reader by default. This allows goss to do verification on say a 2GB file without consuming 2GB of memory. This format is efficient. However, because it reads content line by line, it has no storage of "actual."

One option would be to not include actual whenever that is the case, rather than showing the confusion *bytes.Reader. Another option, is putting a message that alludes to this behavior, perhaps:

{
  "Actual": "Not Available - Content loaded line by line"
}

I'm leaning towards having the message, seems that would be much clearer to the user.

sysop200 commented 1 year ago

Then I don’t understand how to work with the new version of goss. this problem appeared starting from version 0.4.x, before this version there was no Actual field and this problem did not exist. Below is a task that has not changed for a long time.

{{ if .Vars.rhel8cis_rule_4_1_2_3 }}
command:
  auditd_space_actions:
    title: 4.1.2.3 | Ensure system is disabled when audit logs are full
    exec: grep -E "action" /etc/audit/auditd.conf
    exit-status: 0
    stdout:
    - space_left_action = email
    - action_mail_acct = root
    - '/^admin_space_left_action = {{ .Vars.rhel8cis_auditd.admin_space_left_action }}/'
    meta:
      server: 2
      workstation: 2
      CIS_ID:
      - 4.1.2.3
      CISv8: 
      - 8.2
      - 8.3
      CISv8_IG1: true
      CISv8_IG2: true
      CISv8_IG3: true
{{ end }}

The output result of this task in different versions: 0.4.х

"meta": {
                "CIS_ID": [
                    "4.1.2.3"
                ],
                "CISv8": [
                    8.2,
                    8.3
                ],
                "CISv8_IG1": true,
                "CISv8_IG2": true,
                "CISv8_IG3": true,
                "server": 2,
                "workstation": 2
            },
            "property": "exit-status",
            "resource-id": "auditd_space_actions",
            "resource-type": "Command",
            "result": 0,
            "skipped": false,
            "start-time": "2023-09-27T20:35:05.035683192+03:00",
            "successful": true,
            "summary-line": "Command: auditd_space_actions: exit-status: matches expectation: 0",
            "summary-line-compact": "Command: auditd_space_actions: exit-status: matches expectation: 0",
            "title": "4.1.2.3 | Ensure system is disabled when audit logs are full"
        },
        {
            "duration": 48718,
            "end-time": "2023-09-27T20:35:05.03808896+03:00",
            "err": null,
            "matcher-result": {
                "Actual": "object: *bytes.Reader",
                "Expected": [
                    "space_left_action = email",
                    "action_mail_acct = root",
                    "/^admin_space_left_action = halt/"
                ],
                "ExtraElements": null,
                "Message": "to have patterns",
                "MissingElements": [
                    "space_left_action = email",
                    "/^admin_space_left_action = halt/"
                ],
                "TransformerChain": null,
                "UntransformedValue": null
            },

<=0.3.23

 "meta": {
                "CIS_ID": [
                    "4.1.2.3"
                ],
                "CISv8": [
                    8.2,
                    8.3
                ],
                "CISv8_IG1": true,
                "CISv8_IG2": true,
                "CISv8_IG3": true,
                "server": 2,
                "workstation": 2
            },
            "property": "exit-status",
            "resource-id": "auditd_space_actions",
            "resource-type": "Command",
            "result": 0,
            "skipped": false,
            "successful": true,
            "summary-line": "Command: auditd_space_actions: exit-status: matches expectation: [0]",
            "test-type": 0,
            "title": "4.1.2.3 | Ensure system is disabled when audit logs are full"
        },
        {
            "duration": 21961,
            "err": null,
            "expected": [
                "space_left_action = email",
                "action_mail_acct = root",
                "/^admin_space_left_action = halt/"
            ],
            "found": [
                "action_mail_acct = root"
            ],
            "human": "",
            "meta": {
                "CIS_ID": [
                    "4.1.2.3"
                ],
                "CISv8": [
                    8.2,
                    8.3
                ],
                "CISv8_IG1": true,
                "CISv8_IG2": true,
                "CISv8_IG3": true,
                "server": 2,
                "workstation": 2
            },
            "property": "stdout",
            "resource-id": "auditd_space_actions",
            "resource-type": "Command",
            "result": 1,
            "skipped": false,
            "successful": false,
            "summary-line": "Command: auditd_space_actions: stdout: patterns not found: [space_left_action = email, /^admin_space_left_action = halt/]",
            "test-type": 2,
            "title": "4.1.2.3 | Ensure system is disabled when audit logs are full"
        },
aelsabbahy commented 1 year ago

Right, the matcher logic changed quite a bit between goss 0.3.x and 0.4.x.

If you don't mind, can you expand a bit on how you were using "found" before?

For example:

yaml:

command:
  echo "hip":
    exit-status: 1
    stdout:
      - hi
      - bye
    timeout: 10000

Goss v0.3.xx:

$ goss.3.18 v -f json | jq '.results[] | {property, expected, found}' -c
{"property":"exit-status","expected":["1"],"found":["0"]}
{"property":"stdout","expected":["hi","bye"],"found":["hi"]}

Goss v0.4.xx:

$ goss v -f json | jq '.results[] ."matcher-result" | {Expected, Actual, MissingElements}' -c
{"Expected":1,"Actual":0,"MissingElements":null}
{"Expected":["hi","bye"],"Actual":"object: *bytes.Reader","MissingElements":["bye"]}

Where the differences start to show more is with goss v0.4.xx the command output can be treated as a string, so for example:

command:
  echo "hip":
    exit-status: 1
    stdout: "hi"
    timeout: 10000

Goss v0.3.xxx:

Error/Not supported

Goss v0.4.xxx:

$ goss v -f json | jq '.results[] ."matcher-result" | {Expected, Actual, MissingElements}' -c
{"Expected":1,"Actual":0,"MissingElements":null}
{"Expected":"hi","Actual":"hip\n","MissingElements":null}

Hopefully, the above helps explain why the API was changed, I'm open for changing it based on feedback. Just trying to better understand how you're using the fields from goss v0.3.xx and the bug that this is introducing.

Somewhat related note: The other goss outputs are pretty much derived from matcher-result object.

sysop200 commented 1 year ago

above is my task He should run the grep command and look for the action line in the audit.conf file and compare it with the stdout output. and display the result auditfile.yml :

command:
  auditd_space_actions:
    title: 4.1.2.3 | Ensure system is disabled when audit logs are full
    exec: grep -E "action" /etc/audit/auditd.conf
    exit-status: 0
    stdout:
    - space_left_action = email
    - action_mail_acct = root
    - '/^admin_space_left_action = halt/'
    meta:
      server: 2
      workstation: 2
      CIS_ID:
      - 4.1.2.3
      CISv8: 
      - 8.2
      - 8.3
      CISv8_IG1: true
      CISv8_IG2: true
      CISv8_IG3: true

goss -g auditfile.yml --vars varsfile_patch v -f json -o pretty >report.json

aelsabbahy commented 1 year ago

For the above, what would you like to see in report.json?

is the preference to have it be null when reading line by line?

            "matcher-result": {
                "Actual": null,
aelsabbahy commented 1 year ago

Tagging @uk-bolly since I noticed you're tagged in a linked issue. Would love to understand this better and collaborate on best path forward.

sysop200 commented 1 year ago

As far as I understand, Actual is what is found from the Expected field, and MissingElement is what was not found during the search process. I'm generally confused by the conclusion that the new version offers. In the old version it was simply Expected and Found. And now I can’t figure out how I can parse a new format, for example in Python, and get a beautiful test report - What I wanted to find - What I found - What’s missing - And my result Successfull

aelsabbahy commented 1 year ago

Gotcha, so for clarity. actual is just what the underlying system responded with.

You're right that found is no longer there, I can re-introduce it. I didn't realize it was being used and it was removed in v0.4.xx since it wasn't needed by goss to render other output formats.

In goss v0.3.xx, I assume these questions were answered as follows:

  1. What I wanted to find - expected
  2. What I found - found
  3. What’s missing: expected - found , subtract 1-2 (correct me if I'm wrong, but seems this had to be calculated from the other two before)

In goss v0.4.xx, the data that's presented inversely:

  1. What I wanted to find: matcher-result/Expected
  2. What’s missing: matcher-result/MissingElements
  3. What I found: subtract 1-2

So found is the one that has to be calculated from the other two. I can enrich the matcher-result object to contain both missing-elements and found-elements this way both data points are available.

Some boring details

Before goss would track "found" and when rendering would subtract expected-found to display messages when running other (non-json) outputs:

https://github.com/goss-org/goss/blob/v0.3.23/outputs/outputs.go#L92

strings.Join(subtractSlice(r.Expected, r.Found), ", ")

There was nowhere in goss code that was using r.Found aside from it being rendered in the json payload. I didn't realize others were using it. In goss v0.4.xx it tracks MissingElements since that's the only thing goss cared about. I should have communicated this better.

Path forward / Questions

Thank you for your patience and clarifications on this, feedback is what makes goss better for everyone. Appreciate you taking the time to file this.

sysop200 commented 1 year ago

I think that adding the Found-elements attribute will solve the problem in terms of converting json to html report - it will be beautiful! I'll have to rewrite my json parser - but it'll be worth it. Attribute "Actual" - unfortunately, I don’t know what to use it for.

aelsabbahy commented 1 year ago

Sounds good, marked issue as approved. If your html report generator is open source I would love to see it. If not, I understand.

sysop200 commented 1 year ago

I'm ready to share - the code is not a trade secret. If you help improve it or rewrite it in GO, I don’t mind... I’ll post it on github and give you a link

sysop200 commented 1 year ago

Wellcome https://github.com/sysop200/CIS-reporting/

aelsabbahy commented 1 year ago

I'll release a new version soon that includes this enhancement.

I would love your feedback once it's released.

aelsabbahy commented 1 year ago

Cut a new release just now.

sysop200 commented 1 year ago

Glad to hear it! I'll test it soon.

sysop200 commented 1 year ago

Works! But it’s not quite as it should be yet. It is necessary to check the tasks and results expected to be obtained. In my opinion, the issue can be closed.

aelsabbahy commented 1 year ago

Before we close our the issue, can you explain the last message. This way I can improve on this in a future release.

sysop200 commented 1 year ago

I can not explain. It seems to me that the verification logic needs to be changed. I am attaching json and html results. audit_sysop.4check.ru_1700413601.json audit_sysop.4check.ru_1700413601_ren_to_html.txt Parsing script json2html attached above.

aelsabbahy commented 6 months ago

Works! But it’s not quite as it should be yet. It is necessary to check the tasks and results expected to be obtained. In my opinion, the issue can be closed.

Forgot to close this. Closing, thanks for filing this issue.

MichaelThamm commented 2 months ago

Goss version: v0.4.8

TLDR; This issue is already closed, but in my opinion, goss validate providing object: *bytes.Reader is still supported in v0.4.X and is hurting UX.

Like @sysop200, I am confused at the decision to switch the actual result to object: *bytes.Reader in Goss v0.4.xxx because it makes the validation results much harder to sift through quickly (which is often what we want for incident response). After reading the comments, I understand it has no storage of "actual" due to content being read line by line.

Using your example, imagine I am an incident responder and I run:

goss -g goss/goss.yaml --vars goss/vars.yaml v -f json | jq '.results[] ."matcher-result"':

for a test:

command:
  test:
    exec: echo "I need to know what this is"
    exit-status: 1
    stdout:
      - hi
      - test

with result:

{
  "actual": "object: *bytes.Reader",
  "expected": [
    "hi",
    "test"
  ],
  "extra-elements": null,
  "found-elements": [
    "hi"
  ],
  "message": "to have patterns",
  "missing-elements": [
    "test"
  ],
  "transform-chain": null,
  "untransformed-value": null
}

Great, I got some information about what was expected and what was missing, but not having the result of the failed operation creates extra investigative efforts for the responder to now have to check:

Only after all these steps will they know what the cause for failure could be. I would like to implement Goss at my company but this is truly a blocking issue. If I am missing something that would make the validation results more actionable, I would love to hear it!

aelsabbahy commented 2 months ago

Hello, thanks for reaching out. If you don't care for the value to be read line by line, you can use the have-patterns matcher.. which I just realized I forgot to document :facepalm:

command:
  test:
    exec: echo "I need to know what this is"
    exit-status: 1
    stdout:
      have-patterns:
      - hi
      - test

default output:

 goss v
FF

Failures/Skipped:

Command: test: exit-status:
Expected
    0
to be numerically eq
    1
Command: test: stdout:
Expected
    "I need to know what this is\n"
to have patterns
    ["hi","test"]
the missing elements were
    ["test"]

Total Duration: 0.002s
Count: 2, Failed: 2, Skipped: 0

json:

goss v -f json| jq '.results[] ."matcher-result"'
{
  "actual": 0,
  "expected": 1,
  "extra-elements": null,
  "found-elements": null,
  "message": "to be numerically eq",
  "missing-elements": null,
  "transform-chain": null,
  "untransformed-value": 0
}
{
  "actual": "I need to know what this is\n",
  "expected": [
    "hi",
    "test"
  ],
  "extra-elements": null,
  "found-elements": [
    "hi"
  ],
  "message": "to have patterns",
  "missing-elements": [
    "test"
  ],
  "transform-chain": null,
  "untransformed-value": "I need to know what this is\n"
}

I'll update the docs over the weekend or next week to document that matcher. It's basically the same as the default one, except it doesn't read line-by-line. So if it's a giant input, it'll use more memory.

MichaelThamm commented 2 months ago

This is great @aelsabbahy, thanks for the timely reply! I will have to test to see the difference in performance for matcher vs. default as you mentioned, but looks like it will work at first glance.