Typos and false positives

[exploide]

This PR contains few commits that solve the following:

• a few minor typos
• avoid common false positives in "Can we write to any paths present in cron jobs" by excluding /dev/null, /dev/zero, /dev/random and /dev/urandom
• avoid false positive in "User systemd timers" by checking for timers without the headline being counted
• avoid false positive in "Are there private keys in ssh-agent" by excluding the message that there are no identities

Just let me know if you require any changes. :)

[exploide]

I pushed a further commit fixing the "Are there other users in administrative groups" test.

Before this patch, it did never match groups with more than one user in it due to a missing ,.

Furthermore this test excludes the current user, since the question is about other users.

[diego-treitos]

Hi @exploide, thank you for your PR and sorry for the huge delay in my response. I've been quite busy.

May I ask you to make some amends?

• line 586: Here you added $lse_user in the second grep. However, if you do that, it will not show other users that are in administrative groups if the current user is also in that administrative group. For this reason I believe the line should be:

  'grep $lse_grep_opts -E "^(adm|admin|root|sudo|wheel)" /etc/group | grep -Ev ":$" | grep $lse_grep_opts -Ei ":[,a-z_-]+\$"'

• line 963: I think it would be better this line to be

  'for p in `grep --color=never -hERoi "/[a-z0-9_/\.\-]+" /etc/cron* | grep -Ev "/dev/(null|zero|random|urandom)" | sort -u`; do [ -w "$p" ] && echo "$p"; done' \

• line 983: I see your point here, but I think this simpler line should do the trick (I tested it)

  systemctl --user list-timers --all --no-legend | grep -Ev "(^$|timers listed)"

• line 1190: I am not sure about this change. Actually there are other messages different from "agent has no identities", although in my tests it worked fine (no false positives). Maybe it would be better to have something like ssh-add -l | grep -Ei 'RSA|DSA', but I am not sure if it will cause false negatives. I'd rather have false positives than false negatives.

Sorry for being picky but I really want the code to be optimal (at least in my opinion). However if you think I am wrong in any point, please feel free to discuss.

Thanks again for taking your time doing this and sorry again for the delay on looking into this.

[exploide]

Hi @exploide, thank you for your PR and sorry for the huge delay in my response. Sorry for being picky but I really want the code to be optimal (at least in my opinion).

Hi @diego-treitos, no problem. Glad you found the time to review my PR. Actually, I appreciate when maintainers tend to be picky. Code quality is important. :)

line 586: Here you added $lse_user in the second grep. However, if you do that, it will not show other users that are in administrative groups if the current user is also in that administrative group.

I think I already took care of this case. The second grep excludes :$lse_user$, that is the separator, followed by the current user, end of line. This would not match if there is another user before or after the current user. Hence, should be good.

line 963: I think it would be better this line to be ...grep -Ev "/dev/(null|zero|random|urandom)"...

Totally agree. Done.

line 983: I see your point here, but I think this simpler line should do the trick (I tested it) ...systemctl --user list-timers --all --no-legend...

The idea was to have the legend header included in the final output. This way it is omitted. But if you prefer it this way, that's fine. Done that.

line 1190: I am not sure about this change. Actually there are other messages different from "agent has no identities", although in my tests it worked fine (no false positives). Maybe it would be better to have something like ssh-add -l | grep -Ei 'RSA|DSA', but I am not sure if it will cause false negatives. I'd rather have false positives than false negatives.

To be honest I don't see the problem here. The line "agent has no identities" is probably never a success. And if we do it the other way around, i.e. ssh-add -l | grep -Ei 'RSA|DSA' then we need to mention each and every algorithm explicitly. Especially your example would not find ED25519. So I agree that false negatives are much worse. And that's the reason why I implemented it as is.

[diego-treitos]

Hi @exploide thanks for your feedback.

I think you are right in... well, mostly all the points :D

Regarding the systemd timers test, I agree that having the headers is really useful. Maybe we can use this?

systemctl --user list-timers --all | grep -iq '\.timer' && systemctl --user list-timers --all

It checks for the .timer string that should appear in any timer in the unit name.

Regarding the SSH agent test I am not sure yet. For example one of the messages I get is "Could not open a connection to your authentication agent.", althought it is true that in this case the command exits with an error code so the test does not pass... I guess that at least for now your proposal is a good workaround, so we can leave it like that for now.

Summary

1. The :$lse_user$ thing: You are right. Please leave it like that.

2. The /dev regex: Thank you for changing that.

3. The systemd timers: I'd rather use a simpler test like the one I proposed, but feel free to improve it or let me know if my proposal is flawed.

4. The SSH agent test: Your proposal seems reasonable. At least it will work better. Maybe one minor change: add the -i flag to the grep just in case some casing changes in the future? I use to do that as I think it is more robust.

Again, thank you for contributing

[exploide]

Yes, sounds good. I incorporated your proposal concerning the systemd timers. Also grep -i is now used when checking the SSH agent, it really is more robust.