mpatton at inforelay.com
2012-Aug-10 02:32 UTC
AllowUsers "logic" and failure to indicate bad configuration
I smacked into this previously reported bug today whereby an invalid keyword in the Match{} stanza did not throw an error on configuration reload. Are there any plans to fix this? Likewise the penchant for some fields to be comma separated and others to be spaces is just asking for mistakes. Why not support both and be done with it? There was no response (that I saw in the archives) to this post http://marc.info/?l=openssh-unix-dev&m=132311628508429&w=2 Like him, I'm using 5.3p1 as packaged in CentOS 6.3. Secondly the Allow/Deny logic is downright tortured. I looked back and again didn't come across any good discussion as to why it was written that way. It should not be necessary for AllowUsers to be the superset of AllowGroups. As Spock would say "it is illogical." If you had to write PF rules like that you'd go crazy. That's why most people use first-match logic. Per the manpage, if the logic is DenyUsers > AllowUsers > DenyGroups > AllowGroups, then there has to be a immediate stop to the logic chain at each stage. if $user ~= %DenyUsers; then ( deny; return ) if $user ~= %AllowUsers; then { allow; return ) if $user member of %DenyGroups; then (deny; return ) if $user member of %AllowGroups; then (allow; return ) if (%AllowUsers != null || %AllowGroups != null); then deny Anything else makes no sense. Can we expect a change in the future? Or if I were to submit a patch to fix this, would it be accepted? The other flaw I see here is the treatment of 'root'. There are already special keywords controlling that UID, so it makes no sense to have to tag 'root' into this logic either. PermitRootLogin is supposed to be sufficient to dictate that particular outcome.
mpatton at inforelay.com
2012-Aug-10 03:38 UTC
AllowUsers "logic" and failure to indicate bad configuration
> way. It should not be necessary for AllowUsers to be the superset of > AllowGroups.Sorry, I meant to say AllowGroups should not have to include at least one GID from each of AllowUsers. I forgot to add that a case could be made for the Allow/Deny directives to be valid within all Match{} stanzas as well. Right now there is only one global rule. However, it makes perfect sense to want to selectively override it. At first blush I thought that maybe the rules specified in Match{} would just be used to "flip" the logic arrived at from the main body; eg. AllowUsers could effectively negate any rejection reached by the main body logic. Then I thought that maybe it would be better to treat it as a complete substitution of the matching global rule directive. To me, this seems to be the most elegant but will have to be clearly spelled out in documentation. The 3rd option would call for ignoring the entire global rule in favor of building a new one based strictly off the directives contained within Match{}. Yes, in most cases they'll be a lot of cut/pasting from the global rule and tweaking them to taste, but the intent of the stanza is unambiguous. If we were to implement option 2, specifying all 4 directives would achieve the same result as option 3. Thoughts?
Possibly Parallel Threads
- [Bug 1690] New: AllowUsers and DenyGroups directives are not parsed in the order specified
- ((AllowUsers || AllowGroups) && !(AllowUsers && AllowGroups))
- [Bug 2292] New: sshd_config(5): DenyUsers, AllowUsers, DenyGroups, AllowGroups should actually tell how the evaluation order matters
- AllowUsers Change
- [Bug 3193] New: Add separate section in sshd_config man page on Access Control