Page MenuHomeFreeBSD

Bring our pam_login_access to support the same features as Linux's pam_access
ClosedPublic

Authored by cy on Jan 16 2020, 6:28 AM.
Tags
None
Referenced Files
F80195801: D23198.id.diff
Fri, Mar 29, 2:45 AM
Unknown Object (File)
Tue, Mar 19, 2:08 PM
Unknown Object (File)
Tue, Mar 19, 3:36 AM
Unknown Object (File)
Mon, Mar 18, 7:09 PM
Unknown Object (File)
Mon, Mar 18, 5:14 PM
Unknown Object (File)
Feb 21 2024, 2:38 AM
Unknown Object (File)
Feb 21 2024, 2:38 AM
Unknown Object (File)
Feb 21 2024, 2:38 AM
Subscribers

Details

Summary

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.

Test Plan

This code, along with its two prerequisites, is already running on my my infrastructure.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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.
s/However/However,/

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.

This fixes the man page errors introduced by my first cut of this diff.

In D23198#512364, @cy wrote:

This fixes the man page errors introduced by my first cut of this diff.

Does it? ;-)

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.

cy marked 6 inline comments as done.Jan 28 2020, 4:32 AM

Great, OK for the man page side of things.

bjk requested changes to this revision.Jan 30 2020, 3:51 AM

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.

This revision now requires changes to proceed.Jan 30 2020, 3:51 AM
cy marked 6 inline comments as done.

Hopefully I haven't missed anything.

The previous upload had two build errors.

cy marked an inline comment as done.

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.

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
Approved by: des (blanket, implicit)
MFC after: 3 days

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.

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.
I assume this made more sense to me the first time I looked but I confused myself by waiting too long to look at the updates.

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".
It degrades gracefully, though, so most people wouldn't complain.

This revision is now accepted and ready to land.Feb 11 2020, 3:29 AM
cy planned changes to this revision.Feb 15 2020, 2:08 AM
cy marked 5 inline comments as done.

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.

cy marked 2 inline comments as done.

It's probably best to discuss strsep() with des@ first and leave strtok() for now.

This revision is now accepted and ready to land.Feb 16 2020, 2:18 AM