- Download Diff

Summary:

9830 praudit should be able to map users and groups correctly

Review Request #1204 — Created Sept. 10, 2018 and submitted Aug. 13, 2019, 8:29 p.m.

Information
Owner:	ptribble
Repository:	illumos-gate
Branch:
Bugs:	9830
Depends On:
Reviewers
Groups:	general
People:

Description

It's common to aggregate audit logs on a central system. Currently, running praudit then resolves uids and gids back to names on the system where praudit runs, which may be completely wrong.

This fix allows the user to copy the group and passwd files to the aggregated system and point praudit at those files. It does this by preloading the uid and gid caches introduced in 9106.

Testing Done

Run praudit with the -p and -g flags, verified that it resolves uids and gids correctly. Verified that without the flags being used, we get the same (wrong) results as before.

Issues

0
0
7
1
All Issues: 8

Description	From	Last Updated
We should be checking for and handling failures from fgetpwent() here, so that the program doesn't silently accept an error ...	jclulow	Dec. 4, 2018, 4:42 p.m.
Check for failures of fgetgrent(), too.	jclulow	Dec. 4, 2018, 4:42 p.m.
It looks like these definitions should go in praudit.h, rather than directly here.	jclulow	Dec. 4, 2018, 4:42 p.m.
pf is a pointer, so we should compare against NULL; i.e., if (pf != NULL) {	jclulow	Dec. 4, 2018, 4:42 p.m.
We should check the return of fclose() here, even if just via VERIFY0(fclose(pf)). Same with fclose(gf) a couple of lines ...	jclulow	Dec. 4, 2018, 4:44 p.m.
if (gf != NULL) {	jclulow	Dec. 4, 2018, 4:42 p.m.
Is there a period missing on the front of the line here?	jclulow	Dec. 4, 2018, 4:44 p.m.
Is this behaviour desirable? If the audit file is from a foreign system, would it not be more correct to ...	jclulow	Dec. 4, 2018, 4:45 p.m.

Ship it!

```
Ship It!
```

usr/src/cmd/praudit/format.c (Diff revision 1)

Show all issues

We should be checking for and handling failures from fgetpwent() here, so that the program doesn't silently accept an error and move on.

ptribble Dec. 4, 2018, 4:42 p.m.

I've added this, although it turns out that the only failures that you catch are if the file is well-formed but has oversized entries. Input files that are just bad (or that have invalid lines) get accepted but skipped.

usr/src/cmd/praudit/format.c (Diff revision 1)
Show all issues
```
Check for failures of fgetgrent(), too.
```

usr/src/cmd/praudit/main.c (Diff revision 1)

Show all issues

It looks like these definitions should go in praudit.h, rather than directly here.

usr/src/cmd/praudit/main.c (Diff revision 1)

Show all issues

pf is a pointer, so we should compare against NULL; i.e., if (pf != NULL) {

usr/src/cmd/praudit/main.c (Diff revision 1)

Show all issues

We should check the return of fclose() here, even if just via VERIFY0(fclose(pf)).  Same with fclose(gf) a couple of lines down.

ptribble Dec. 4, 2018, 4:45 p.m.

Indirectly via the errno check on fgetpwent; looking at the gate very few instance of fclose are checked, and we don't have pending writes to worry about.

usr/src/cmd/praudit/main.c (Diff revision 1)
Show all issues
```
if (gf != NULL) {
```

usr/src/man/man1m/praudit.1m (Diff revision 1)

Show all issues

Is there a period missing on the front of the line here?

usr/src/man/man1m/praudit.1m (Diff revision 1)

Show all issues

Is this behaviour desirable? If the audit file is from a foreign system, would it not be more correct to use only the provided file and not fall back to IDs from the local system?

ptribble Dec. 4, 2018, 4:45 p.m.

I think this is correct. It's far better than what we have now, and also covers the case where you're importing a NIS passwd map (or the users from configuration management) and using the local passwd file for base system accounts.

Change Summary:

Address feedback: check fgetpwent (although that catches essentially nothing); also tidy up usage message and comments.

Diff:

Revision 2 (+111 -7)

Show changes

	usr/src/cmd/praudit/format.c
	usr/src/cmd/praudit/main.c
	usr/src/man/man1m/praudit.1m

Added Files:

Removed Files:

Change Summary:

2nd attempt at persuading RB to accpet the updated diff

Diff:

Revision 3 (+135 -12)

Show changes

	usr/src/cmd/praudit/format.c
	usr/src/cmd/praudit/main.c
	usr/src/cmd/praudit/praudit.h
	usr/src/man/man1m/praudit.1m

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

You have a pending review.

Review Board 3.0.18

9830 praudit should be able to map users and groups correctly

Change Summary:

Diff:

Added Files:

Removed Files:

Change Summary:

Diff:

Status: Closed (submitted)