Created attachment 1762 [details] Patch for src/auth.c to process AllowUsers/DenyGroups config directives correctly Details: When logging into the sshd server, if the server's sshd_config configuration file contains both "AllowUsers joe" and "DenyGroups joe", a user "joe" belonging to group "joe" will be denied access based on his group. However, the sshd_config man page states that AllowUsers should be processed before DenyGroups, thereby allowing joe to log in: "... The allow/deny directives are processed in the following order: DenyUsers, AllowUsers, DenyGroups, and finally AllowGroups." To reproduce: 1) Create a user 'test' and give him a password. 2) Add these lines in sshd_config: AllowUsers test DenyGroups test 3) Restart sshd. 4) Attempt to SSH in as user 'test'. 5) Check /var/log/auth.log. The attempt will be reported as denied because the user is in a denied group. Solution: The solution depends on what the problem actually is: If the way it is currently working is the desired functionality, the man page just needs to be updated. However, if the order currently listed in the man page is desired, auth.c needs a quick patch. In that case, I have attached a patch for review. Basically, it tells allowed_user() to return true if the username is indeed present in the AllowUsers list. This falls in the correct order between DenyUsers and DenyGroups according to the man page. There was some ifdef'd auth check for AIX (?) that I also moved ahead so it would be checked before the user was authorized.
The problem is that the way these things work is that they only ever provide a way to deny a login, not allow it. That is to say if that if a given login would be denied by any one of the directives then it'll be denied. It's neither first-match nor last-match. Changing this would require changing the semantics of the directives, which would change the behaviour of existing configurations. We could maybe do this, but it would need to be well documented in the release notes, and it's almost inevitable that someone somewhere wants the current behaviour. Instead, I think we should (a) improve the documentation, and (b) add a new directive that can work with the Match directive which would allow the rules to be expressed as first-match in whichever order makes sense for your purpose. You would be able to express your rules as something like: Match User joe AllowLogin yes Match Group joe AllowLogin no Match rules are processed first-match per directive, so this should do what you want, and also allows easier use of Address rules and suchlike.
I have to agree that this is a bug. The logical order of DenyUsers, AllowUsers, DenyGroups, AllowGroups that is noted in the documentation makes sense. The implementation does not. First off, DenyUsers works properly and can be ignored for this discussion. However, ask this: if an administrator SPECIFICALLY grants access to a user by listing their ID in AllowUsers, why should they be subject to further restrictions? In any case, here are the Allow/Deny tables for the existing and patched systems. The cases that would be changed are noted with an "*" AllowUsers DenyGroups AllowGroups Result F F F Deny F F T Allow F T F Deny F T T Deny T F F Deny T F T Allow T T F Deny T T T Deny New AllowUsers DenyGroups AllowGroups Result F F F Deny F F T Allow F T F Deny F T T Deny T F F *Allow* Case 1 T F T Allow T T F *Allow* Case 2 T T T *Allow* Case 3 Take each of the three cases: 1. User is AllowUser but not in DenyGroups or AllowGroups. This adds valuable functionality. Say you have users A and B in group 1 and users C, D, and E in group 2. You want to allow users A, C, D, E. If you define AllowUsers A and AllowGroups 2, user A will be rejected because they are not part of group 2. The only alternative is to AllowUsers A, C, D, E and not define AllowGroups. That is illogical. 2,3. The change in result from Deny to Allow where a user is an AllowedUser yet is a member of a DenyGroup is not as controversial a change as it may appear. Currently, the system is totally ignoring the AllowUser configuration when the user is a member of a DenyGroup. So the AllowUser configuration shouldn't even be in the configuration. Assuming most admins are smart enough to understand that, and don't have AllowedUsers that are also members of DenyGroups there should't be any significant repercussions.
I've committed a change to the manual pages to emphasise that the users and groups controls are independent. This will be in OpenSSH 8.2
close bugs that were resolved in OpenSSH 8.5 release cycle