Page MenuHomeFreeBSD

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

Authored by aniketp on Jun 10 2018, 11:36 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 12, 1:40 AM
Unknown Object (File)
Tue, Mar 12, 1:40 AM
Unknown Object (File)
Tue, Mar 12, 1:40 AM
Unknown Object (File)
Tue, Mar 12, 1:40 AM
Unknown Object (File)
Tue, Mar 12, 1:40 AM
Unknown Object (File)
Tue, Mar 12, 1:40 AM
Unknown Object (File)
Tue, Mar 12, 1:40 AM
Unknown Object (File)
Tue, Mar 12, 1:40 AM
Subscribers

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 Passed
Unit
No Test Coverage
Build Status
Buildable 17225
Build 17070: arc lint + arc unit

Event Timeline

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
  • Add a test for praudit exclusivity for 'r' and 's' arguments
  • Separate the test cases from one another

Update the praudit_raw_short_exclusive's description

  • 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.

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

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

  • I had missed the corrupted record earlier

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

Update praudit_sync_to_next_record test case

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

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 added inline comments.
usr.sbin/praudit/tests/praudit_test.sh
148

@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.

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.

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

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.

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 - subversion.

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.

  • Reduce the timeout to 2 seconds

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
This revision was automatically updated to reflect the committed changes.