Add a single test-program for praudit(1) utility
ClosedPublic

Authored by aniketp on Jun 10 2018, 11:36 PM.

Details

Summary

The revision introduces a new test-program for an audit viewer application: praudit.
The tests have been created for checking the proper functioning of each mode of execution.

-d del  Specifies the delimiter.  The default delimiter is the comma.

-l      Prints the entire record on the same line.  If this option is not
        specified, every token is displayed on a different line.

-n      Do not convert user and group IDs to their names but leave in
        their numeric forms.

-p      Specify this option if input to praudit is piped from the tail(1)
        utility.  This causes praudit to sync to the start of the next
        record.

-r      Prints the records in their raw, numeric form.  This option is
        exclusive from -s.

-s      Prints the tokens in their short form.  Short text
        representations for record and event type are displayed.  This
        option is exclusive from -r.

-x      Print audit records in the XML output format.
NOTE: The above snippet is taken from praudit.1
Test Plan

Execute make install from usr.sbin/praudit/tests.
Execute kyua test from /usr/test/usr.sbin/praudit. The test case should succeed.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 17222
Build 17067: arc lint + arc unit
aniketp created this revision.Jun 10 2018, 11:36 PM
asomers requested changes to this revision.Jun 11 2018, 12:16 AM

What about -p? And how about asserting the exclusivity of -s and -r in a negative test?

usr.sbin/praudit/tests/Makefile
19

Remove the final backslash

usr.sbin/praudit/tests/praudit_test.sh
34

As these assertions are all independent, you should split them into separate test cases.

38

Don't assert that the exit status is 1. That's a bug. Fix the bug instead.

This revision now requires changes to proceed.Jun 11 2018, 12:16 AM
aniketp updated this revision to Diff 43580.Jun 11 2018, 2:25 AM
  • Add a test for praudit exclusivity for 'r' and 's' arguments
  • Separate the test cases from one another
aniketp updated this revision to Diff 43581.Jun 11 2018, 2:29 AM

Update the praudit_raw_short_exclusive's description

aniketp updated this revision to Diff 43592.Jun 11 2018, 10:32 AM
  • Replace the audit startup trail with socket(2) trail
  • Improvements in the raw_short_exclusive description

I still think you should try to test "-p". That's the only thing standing between you and full coverage of the praudit command line.

aniketp updated this revision to Diff 43691.Jun 13 2018, 6:07 AM

Add a test case for -p option which is used with tail(1)

aniketp updated this revision to Diff 43692.Jun 13 2018, 6:08 AM

Add a test case for -p option which is used with tail(1)

  • I had missed the corrupted record earlier
aniketp updated this revision to Diff 43694.Jun 13 2018, 6:48 AM

Update praudit's flag '-p': Earlier the tests were failing

aniketp updated this revision to Diff 43695.Jun 13 2018, 6:56 AM

Update praudit_sync_to_next_record test case

asomers added inline comments.Jun 13 2018, 3:25 PM
usr.sbin/praudit/tests/praudit_test.sh
149

Is it a requirement that praudit _can't_ work with a truncated record without -p? I don't think it is. So you should probably remove the first assertion. Also, could you somewhere include a textual description of the difference between the normal and the truncated records?

aniketp marked 3 inline comments as done.Jun 13 2018, 3:40 PM
aniketp added inline comments.
usr.sbin/praudit/tests/praudit_test.sh
149

@asomers I had added extra random texts in the trail (before correct audit record) and verified that
praudit trail resulted in empty STDOUT
while praudit -p trail gave only the correct (later) part of the stdout.

aniketp updated this revision to Diff 43741.Jun 14 2018, 3:19 AM

Remove the normal praudit atf_check assertion in the praudit_sync_to_next_record test-case

I would still like to see a comment describing the contents of the corrupted file.

aniketp updated this revision to Diff 43769.Jun 14 2018, 4:29 PM

Add a description on the intent of testing the -p option

asomers added inline comments.Jun 15 2018, 2:27 PM
usr.sbin/praudit/tests/praudit_test.sh
150

I doesn't read from the end. It still reads from the front; but it syncs to the start of the first whole record before it does anything else.

aniketp updated this revision to Diff 43829.Jun 15 2018, 4:42 PM

Update the test case description

aniketp retitled this revision from Add a single test-case for praudit(1) utility to Add a single test-program for praudit(1) utility.Jun 15 2018, 4:44 PM
aniketp set the repository for this revision to rS FreeBSD src repository.

The praudit_test:praudit_sync_to_next_record spins the CPU and never returns. Does it do that for you too?

No @asomers ,
In its current form it passes smoothly:

praudit_test:praudit_sync_to_next_record  ->  passed  [0.027s]
asomers requested changes to this revision.Jun 16 2018, 2:18 PM

Since praudit -p occasionally spins the cpu (as confirmed by Aniket on IRC) you should set a shorter timeout. Apart from that this review looks good. However, it does depend on the change to praudit's return code, which hasn't been committed yet.

This revision now requires changes to proceed.Jun 16 2018, 2:18 PM

No @asomers, praudit -p never spins the CPU in normal invocation. (Apologies if my earlier statement was unclear)
This is the observation:

Case 1) For the tail(1) usage:

	atf_check -o file:$(atf_get_srcdir)/no_args \
		tail $(atf_get_srcdir)/corrupted | praudit -p

CPU usage shoots up and test case never returns.

Case 2) For the normal usage:

	atf_check -o file:$(atf_get_srcdir)/no_args \
		praudit -p $(atf_get_srcdir)/corrupted

Result is successful everytime.

aniketp updated this revision to Diff 43902.Jun 16 2018, 3:47 PM
  • Reduce the timeout to 2 seconds
asomers accepted this revision.Jun 17 2018, 5:01 PM

I figured out why it works for you but not for me; it's arcanist's fault. Arcanist apparently doesn't like binary files. When I do "arc patch" it creates corrupted and trail as 0-length files. That means that a repro case for the bug is "touch /tmp/empty; praudit -p /tmp/empty".

I'm accepting this revision, but it still depends on fixing praudit's exit status.

This revision is now accepted and ready to land.Jun 17 2018, 5:01 PM
Closed by commit rS335290: praudit(1): add tests (authored by asomers, committed by ). · Explain WhyJun 17 2018, 5:31 PM
This revision was automatically updated to reflect the committed changes.