This the third of three planned commits, which I may break up into two to implement Linux's pam_access features into our pam_login_access. des@ has already given summary approval however I would like to have a review of the man pages. Code reviews are also welcome.
Details
Details
- Reviewers
bjk bcr - Group Reviewers
manpages - Commits
- rS359117: MFC r358070:
rS358070: This commit makes significant changes to pam_login_access(8) to bring it
This code, along with its two prerequisites, is already running on my my infrastructure.
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
I checked the man pages. You can install textproc/igor and run it over your man page to get some useful hints. Another check for syntax and such can be done by running "mandoc -Tlint".
Thanks for these PAM changes.
| lib/libpam/modules/pam_login_access/login.access.5 | ||
|---|---|---|
| 34 ↗ | (On Diff #66816) | You need to make a line break after a sentence stop in man pages. |
| lib/libpam/modules/pam_login_access/pam_login_access.8 | ||
| 70 ↗ | (On Diff #66816) | Another line break is needed here after the sentence stop. |
| 72 ↗ | (On Diff #66816) | Another line break needed here. |
| 78 ↗ | (On Diff #66816) | Sentence stop = break the line. |
| 82 ↗ | (On Diff #66816) | Line break needed here after the sentence stop. |
| 83 ↗ | (On Diff #66816) | ... and one more time here. |
Comment Actions
Sorry about that. There are commits in my pam_login_access git branch that were prerequisites to this but really outside of the scope of this revision. I've included everything here. This includes the following [to be] commits:
Improve the legibility of the login.access.5 man page by separating each argument into its own paragraph. The words ALL, LOCAL, and EXCEPT have special meaning and are documented as in the login.access(5) man page. However strcasecmp() is used to compare for these special strings. Because of this User accounts and groups with the corresponding lowercase names are misintrepreted to have special whereas they should not. This commit fixes this, conforming to the man page and to how the Linux pam_access(8) handles these special words. When pam_login_access(5) fails to match a username it attempts to match the primary group a user belongs to. This commit extends the match to secondary groups a user belongs to as well, just as the Linux pam_access(5) does.
And finally what this review addresses: bringing our pam_login_access up to the same support as the Linux pam_access.
Comment Actions
Sorry this is later than expected.
-1 just for indentation, "Xr syslog 3", and nodefgroup description; the rest is optional.
| lib/libpam/modules/pam_login_access/login_access.c | ||
|---|---|---|
| 89 ↗ | (On Diff #67386) | Would it be worth caching these in locals to avoid repeated dereferences (and make the code easier to read)? |
| 138 ↗ | (On Diff #67386) | The indentation is off here. |
| 194 ↗ | (On Diff #67386) | I'm not sure I understand why "string" is the right thing after "grouplist:" here; "grouplist of" would make more sense, as would "grouplist for user" or "getgrouplist" |
| 208 ↗ | (On Diff #67386) | Three instances of free(grouplist) is perhaps on the boundary of moving to a cleanup handler. |
| 230 ↗ | (On Diff #67386) | I'm not super-keen on this construction but can proffer no alternative. |
| 232 ↗ | (On Diff #67386) | Do we need to check errno to know that strndup failed to allocate memory? |
| 275 ↗ | (On Diff #67386) | Not quite already being touched, but strchr returns a pointer not an integer. |
| lib/libpam/modules/pam_login_access/pam_login_access.8 | ||
| 77 ↗ | (On Diff #67386) | There are Po and Pc macros for open- and close-parenthesis, though style takes no stance on their use versus the literal characters. |
| 79 ↗ | (On Diff #67386) | I think this could be misinterpreted. Perhaps "makes tokens not enclosed in parentheses only match users, requiring groups to be specified in parentheses. This is not [...], which intermingle user and group names, with user entries taking precedence over group entries." is better, at the risk of being verbose. |
| 86 ↗ | (On Diff #67386) | I'm not sure what this is doing here. |
Comment Actions
I agree there is no other way to do this:
tok[(stringlen = strlen(&tok[1]))
I think I've addressed everything except for .Po and .Pc (which are optional) and should be discussed with des@ too.
Comment Actions
I agree there is no other way to do this:
tok[(stringlen = strlen(&tok[1]))
I think I've addressed everything except for .Po and .Pc (which are optional) and should be discussed with des@ too.
| lib/libpam/modules/pam_login_access/login_access.c | ||
|---|---|---|
| 89 ↗ | (On Diff #67386) | That makes sense. |
| 138 ↗ | (On Diff #67386) | Thanks. Good catch. |
| 194 ↗ | (On Diff #67386) | "string" came from its sister function user_match() which doesn't know if "string" is a user or a group. group_match() does. I'll change this to "groupname". |
| 230 ↗ | (On Diff #67386) | It needs to be enclosed in parentheses. The only alternative would be nested if statements. I'm not enamored with this: } else if (tok[0] == '(') { /* group */
stringlen = strlen(&tok[1];
if (tok[stringlen] == ')') {
if ((grpstr = strndup(&tok[1], stringlen - 1)) == NULL) {
syslog(LOG_ERR, "cannot allocate memory for %s", string);
return (NO);
}
} |
| 232 ↗ | (On Diff #67386) | Probably not. |
| 275 ↗ | (On Diff #67386) | This is original code. I'll fix this in another commit. strchr() returns a pointer not an int. Reported by: bjk |
| lib/libpam/modules/pam_login_access/pam_login_access.8 | ||
| 77 ↗ | (On Diff #67386) | There are a number of parentheses in both man pages. If these are to be changed I should have a commit that converts the existing parentheses before this commit. I'm leaning toward not changing them. |
| 86 ↗ | (On Diff #67386) | That was due to a git mismerge. Thanks for catching this. This as originally one large commit but I've separated it out into smaller commits. |
Comment Actions
There are probably still some changes to make here, but not really anything that affects functionality, I think.
Sorry to have spotted some things this time that I apparently missed the first time around :(
| lib/libpam/modules/pam_login_access/login.access.5 | ||
|---|---|---|
| 43 ↗ | (On Diff #67830) | A comma after "Otherwise" might be helpful though it's debatable whether it's strictly needed for correctness. |
| lib/libpam/modules/pam_login_access/login_access.c | ||
| 52 ↗ | (On Diff #67830) | It looks like this line is 99 columns? |
| 128 ↗ | (On Diff #67830) | Thinking about strtok_r() is clearly a matter for a different change. |
| 183 ↗ | (On Diff #67830) | It feels weird to call getpwnam() with a "groupname" (vs. getgrnam()), though IIUC the second parameter here is the username we're trying to determine access for so the code is okay. |
| 225 ↗ | (On Diff #67830) | How long is this line? |
| 236 ↗ | (On Diff #67830) | Shouldn't we just return the result of group_match(), which is already YES or NO? |
| lib/libpam/modules/pam_login_access/pam_login_access.8 | ||
| 71 ↗ | (On Diff #67830) | The width is supposed to be of the widest element, which is now IIUC "accessfile=pathname". |
Comment Actions
It passes a build. I'll upload.
strtok() should remain as strtok_r() isn't needed, It's not threaded. However, all occurrences of strtok() should be replaced with strsep(). I should probably do that before committing this. Give me a few days.
| lib/libpam/modules/pam_login_access/login.access.5 | ||
|---|---|---|
| 43 ↗ | (On Diff #67830) | I don't think a comma is needed but I'll put one in there. |
| lib/libpam/modules/pam_login_access/login_access.c | ||
| 128 ↗ | (On Diff #67830) | strtok_r() isn't needed in this module. It's only used during account management. Only one will run. Ideally this should be replaced with strsep(), but that's for another project. |
| 183 ↗ | (On Diff #67830) | This string because we don't know if this is a user or group. In this context we consider it user so renaming string to groupname was wrong. Username is correct. Thank you for pointing this out. |
| 225 ↗ | (On Diff #67830) | 76. |