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.

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

incomprehensible behavior when testing with different versions of goss #845

Closed
sysop200 opened this issue Oct 2, 2023 · 20 comments · Fixed by #852
Closed

incomprehensible behavior when testing with different versions of goss #845

sysop200 opened this issue Oct 2, 2023 · 20 comments · Fixed by #852

Comments

@sysop200
Copy link

sysop200 commented Oct 2, 2023

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

@sysop200 sysop200 added the bug label Oct 2, 2023
@aelsabbahy
Copy link
Member

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
Copy link
Member

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
Copy link
Author

sysop200 commented Oct 3, 2023

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"
        },

@sysop200 sysop200 closed this as completed Oct 3, 2023
@sysop200 sysop200 reopened this Oct 3, 2023
@aelsabbahy
Copy link
Member

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:

  • expected: inconsistent - array of strings, even when numeric test (e.g. exit-status)
  • found: inconsistent behavior.
    • For exit-status: it was reporting "found" as what was actually found, but as a one element array of type string
    • For stdout: "found" is the subset of matchers that were found (notice it's "hi" and not "hip")
$ 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:

  • expected: test
  • Actual: what goss found on the system (after any transformations)
  • MissingElements: missing elements (if it's a have-patterns type matcher)
$ 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:

  • expected: test
  • Actual = what goss found on the system (after any transformations), since this is a string matcher now instead of the memory efficient pattern matcher it can display the full content under actual.
  • MissingElements: n/a since it's not a have-patterns matcher
$ 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
Copy link
Author

sysop200 commented Oct 4, 2023

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
Copy link
Member

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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

  • I can enrich the matcher object to contain both found and missing, would that resolve this issue?
  • For actual, when it's content that was read line-by-line do you prefer null in that case? It seems that it would communicate that the "actual" system result is not available better than *bytes.Reader which is confusing.

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Author

@aelsabbahy
Copy link
Member

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

I would love your feedback once it's released.

@aelsabbahy
Copy link
Member

Cut a new release just now.

@sysop200
Copy link
Author

sysop200 commented Nov 8, 2023

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

@sysop200
Copy link
Author

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
Copy link
Member

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

@sysop200
Copy link
Author

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants