[PATCH 1/2] Add a confdb_get_uint32_t convenience function A convenience function to fetch the gid override from confdb
[PATCH 2/2] Add options to override GID, homedir and shell The functionality to override the shell is a little different than described in the ticket -- it was adjusted after a discussion on #sssd on IRC.
https://fedorahosted.org/sssd/ticket/742 https://fedorahosted.org/sssd/ticket/551
On Fri, 2011-05-06 at 13:56 +0200, Jakub Hrozek wrote:
[PATCH 1/2] Add a confdb_get_uint32_t convenience function A convenience function to fetch the gid override from confdb
Ack
[PATCH 2/2] Add options to override GID, homedir and shell The functionality to override the shell is a little different than described in the ticket -- it was adjusted after a discussion on #sssd on IRC.
Nack.
In the future, please provide separate patches for logically different features (this patch contains three different changes).
The home directory template should also support UID (I mentioned this in ticket 742). We should cover all our bases.
Please reorder the manpage entry for allowed_shells. It should be clear that the order of evaluation is: 1. If it's in /etc/shells, use it. 2. If it's NOT in /etc/shells, but it is in allowed_shells, use the value of shell_fallback. 3. If it's NOT in /etc/shells and it's not in allowed_shells, it becomes nologin
We should also make it clear that having an empty string for shell is passed as-is to glibc for it to make the decision.
In expand_homedir_template(), you allocate memory for "copy" that you never free. Please create a tmp_ctx for copy and free it on all exits (success or failure). It would be preferable to also allocate "result" on tmp_ctx and only talloc_steal() it onto mem_ctx at the end of successful execution.
get_shell_override() should not shortcut out if there are no allowed shells. It should still check /etc/shells because if it's not listed, we want to convert it to nologin.
Also, get_shell_override() isn't handling the empty shell special case, which should be passed back as-is.
Finally, getusershell() is an expensive system call. I'd prefer if we only did so once and cached it in the nss_ctx, rather than invoking it for every user request. I think it's reasonable to require the SSSD to be restarted if /etc/shells changes. (Might want to put that in the manpage though.)
On 05/06/2011 04:05 PM, Stephen Gallagher wrote:
On Fri, 2011-05-06 at 13:56 +0200, Jakub Hrozek wrote:
[PATCH 1/2] Add a confdb_get_uint32_t convenience function A convenience function to fetch the gid override from confdb
Ack
[PATCH 2/2] Add options to override GID, homedir and shell The functionality to override the shell is a little different than described in the ticket -- it was adjusted after a discussion on #sssd on IRC.
Nack.
In the future, please provide separate patches for logically different features (this patch contains three different changes).
The home directory template should also support UID (I mentioned this in ticket 742). We should cover all our bases.
Done, sorry, I implemented this particular part based on the discussion in ticket #551 that does not list UID.
Please reorder the manpage entry for allowed_shells. It should be clear that the order of evaluation is:
- If it's in /etc/shells, use it.
- If it's NOT in /etc/shells, but it is in allowed_shells, use the
value of shell_fallback. 3. If it's NOT in /etc/shells and it's not in allowed_shells, it becomes nologin
We should also make it clear that having an empty string for shell is passed as-is to glibc for it to make the decision.
Done.
In expand_homedir_template(), you allocate memory for "copy" that you never free. Please create a tmp_ctx for copy and free it on all exits (success or failure). It would be preferable to also allocate "result" on tmp_ctx and only talloc_steal() it onto mem_ctx at the end of successful execution.
Done. I copied this bug when I stole the code from expand_ccname_template(). I'll send a separate patch to address the same issue there, too.
get_shell_override() should not shortcut out if there are no allowed shells. It should still check /etc/shells because if it's not listed, we want to convert it to nologin.
This is done in the attached patch, but I don't agree with the change.
It differs from the current behaviour and moreover allowed_shells is unset in the default setting, so the change would affect anyone running with defaults.
Also, get_shell_override() isn't handling the empty shell special case, which should be passed back as-is.
Done.
Finally, getusershell() is an expensive system call. I'd prefer if we only did so once and cached it in the nss_ctx, rather than invoking it for every user request. I think it's reasonable to require the SSSD to be restarted if /etc/shells changes. (Might want to put that in the manpage though.)
Done. I was relying on VFS, I guess, but I agree that restarting SSSD is acceptable.
On Mon, 2011-05-09 at 10:45 +0200, Jakub Hrozek wrote:
On 05/06/2011 04:05 PM, Stephen Gallagher wrote:
get_shell_override() should not shortcut out if there are no allowed shells. It should still check /etc/shells because if it's not listed, we want to convert it to nologin.
This is done in the attached patch, but I don't agree with the change.
It differs from the current behaviour and moreover allowed_shells is unset in the default setting, so the change would affect anyone running with defaults.
Well, that wouldn't really have been a change in behavior, since if the shell didn't exist on the system, the login program or SSH would still be denying them. I guess we're still mixed up on the edge case of 'shell is installed but not listed in /etc/shells', but I think that's more of a configuration error.
The alternative would be to treat a nonexistent allowed_shells as 'all shells are allowed'. Thus we would force them into the fallback shell if they hit this case. What do you think?
On all other changes: ack.
On 05/09/2011 07:42 PM, Stephen Gallagher wrote:
On Mon, 2011-05-09 at 10:45 +0200, Jakub Hrozek wrote:
On 05/06/2011 04:05 PM, Stephen Gallagher wrote:
get_shell_override() should not shortcut out if there are no allowed shells. It should still check /etc/shells because if it's not listed, we want to convert it to nologin.
This is done in the attached patch, but I don't agree with the change.
It differs from the current behaviour and moreover allowed_shells is unset in the default setting, so the change would affect anyone running with defaults.
Well, that wouldn't really have been a change in behavior, since if the shell didn't exist on the system, the login program or SSH would still be denying them.
Oh, I though the user would get /bin/sh..
In this case I agree with you..thank you for explaining.
On Tue, 2011-05-10 at 10:58 +0200, Jakub Hrozek wrote:
On 05/09/2011 07:42 PM, Stephen Gallagher wrote:
On Mon, 2011-05-09 at 10:45 +0200, Jakub Hrozek wrote:
On 05/06/2011 04:05 PM, Stephen Gallagher wrote:
get_shell_override() should not shortcut out if there are no allowed shells. It should still check /etc/shells because if it's not listed, we want to convert it to nologin.
This is done in the attached patch, but I don't agree with the change.
It differs from the current behaviour and moreover allowed_shells is unset in the default setting, so the change would affect anyone running with defaults.
Well, that wouldn't really have been a change in behavior, since if the shell didn't exist on the system, the login program or SSH would still be denying them.
Oh, I though the user would get /bin/sh..
In this case I agree with you..thank you for explaining.
I was thinking about this more last night, and I think in the end that you were probably right with how you did this the first time. While it's true that both 'login' and 'sshd' will just reject the user (actually, they'll try to exec the shell, get an error from exec and then bail out), it's possible that other mechanisms like rlogin or telnet or similar might have their own ways of behaving (implementing their own fallback, for example).
So I think that we probably need to just punt if we don't have an allowed_shells option and return the LDAP value.
Sorry for going back and forth on this so many times.
On 05/10/2011 01:35 PM, Stephen Gallagher wrote:
On Tue, 2011-05-10 at 10:58 +0200, Jakub Hrozek wrote:
On 05/09/2011 07:42 PM, Stephen Gallagher wrote:
On Mon, 2011-05-09 at 10:45 +0200, Jakub Hrozek wrote:
On 05/06/2011 04:05 PM, Stephen Gallagher wrote:
get_shell_override() should not shortcut out if there are no allowed shells. It should still check /etc/shells because if it's not listed, we want to convert it to nologin.
This is done in the attached patch, but I don't agree with the change.
It differs from the current behaviour and moreover allowed_shells is unset in the default setting, so the change would affect anyone running with defaults.
Well, that wouldn't really have been a change in behavior, since if the shell didn't exist on the system, the login program or SSH would still be denying them.
Oh, I though the user would get /bin/sh..
In this case I agree with you..thank you for explaining.
I was thinking about this more last night, and I think in the end that you were probably right with how you did this the first time. While it's true that both 'login' and 'sshd' will just reject the user (actually, they'll try to exec the shell, get an error from exec and then bail out), it's possible that other mechanisms like rlogin or telnet or similar might have their own ways of behaving (implementing their own fallback, for example).
So I think that we probably need to just punt if we don't have an allowed_shells option and return the LDAP value.
Sorry for going back and forth on this so many times.
No problem :-)
New patches attached. They also change the label "fail" to "done" in the two functions that expand templates.
On Fri, 2011-05-13 at 11:14 +0200, Jakub Hrozek wrote:
On 05/10/2011 01:35 PM, Stephen Gallagher wrote:
On Tue, 2011-05-10 at 10:58 +0200, Jakub Hrozek wrote:
On 05/09/2011 07:42 PM, Stephen Gallagher wrote:
On Mon, 2011-05-09 at 10:45 +0200, Jakub Hrozek wrote:
On 05/06/2011 04:05 PM, Stephen Gallagher wrote:
get_shell_override() should not shortcut out if there are no allowed shells. It should still check /etc/shells because if it's not listed, we want to convert it to nologin.
This is done in the attached patch, but I don't agree with the change.
It differs from the current behaviour and moreover allowed_shells is unset in the default setting, so the change would affect anyone running with defaults.
Well, that wouldn't really have been a change in behavior, since if the shell didn't exist on the system, the login program or SSH would still be denying them.
Oh, I though the user would get /bin/sh..
In this case I agree with you..thank you for explaining.
I was thinking about this more last night, and I think in the end that you were probably right with how you did this the first time. While it's true that both 'login' and 'sshd' will just reject the user (actually, they'll try to exec the shell, get an error from exec and then bail out), it's possible that other mechanisms like rlogin or telnet or similar might have their own ways of behaving (implementing their own fallback, for example).
So I think that we probably need to just punt if we don't have an allowed_shells option and return the LDAP value.
Sorry for going back and forth on this so many times.
No problem :-)
New patches attached. They also change the label "fail" to "done" in the two functions that expand templates.
Please don't squash unrelated changes into patches. The label change should have been separate. Much the same way that this patch should have been three, one for shells, one for homedir and one for GID.
I just realized that I have one more nack for this. While I think the shell overrides are fine, homedir and GID need to be configurable per-domain (especially GID). Configuring GID globally for all domains just doesn't make sense. Homedir I think should probably be settable in either place (similar to how we do filter_users in either [NSS] or per-domain)
Nack.
On 05/13/2011 02:54 PM, Stephen Gallagher wrote:
On Fri, 2011-05-13 at 11:14 +0200, Jakub Hrozek wrote:
On 05/10/2011 01:35 PM, Stephen Gallagher wrote:
On Tue, 2011-05-10 at 10:58 +0200, Jakub Hrozek wrote:
On 05/09/2011 07:42 PM, Stephen Gallagher wrote:
On Mon, 2011-05-09 at 10:45 +0200, Jakub Hrozek wrote:
On 05/06/2011 04:05 PM, Stephen Gallagher wrote: > get_shell_override() should not shortcut out if there are no allowed > shells. It should still check /etc/shells because if it's not listed, we > want to convert it to nologin. >
This is done in the attached patch, but I don't agree with the change.
It differs from the current behaviour and moreover allowed_shells is unset in the default setting, so the change would affect anyone running with defaults.
Well, that wouldn't really have been a change in behavior, since if the shell didn't exist on the system, the login program or SSH would still be denying them.
Oh, I though the user would get /bin/sh..
In this case I agree with you..thank you for explaining.
I was thinking about this more last night, and I think in the end that you were probably right with how you did this the first time. While it's true that both 'login' and 'sshd' will just reject the user (actually, they'll try to exec the shell, get an error from exec and then bail out), it's possible that other mechanisms like rlogin or telnet or similar might have their own ways of behaving (implementing their own fallback, for example).
So I think that we probably need to just punt if we don't have an allowed_shells option and return the LDAP value.
Sorry for going back and forth on this so many times.
No problem :-)
New patches attached. They also change the label "fail" to "done" in the two functions that expand templates.
Please don't squash unrelated changes into patches. The label change should have been separate.
Didn't we use to say that trivial changes such as fixing typos can be squashed? Or does that only apply when we're changing code around that area?
Much the same way that this patch should have been three, one for shells, one for homedir and one for GID.
I just realized that I have one more nack for this. While I think the shell overrides are fine, homedir and GID need to be configurable per-domain (especially GID). Configuring GID globally for all domains just doesn't make sense. Homedir I think should probably be settable in either place (similar to how we do filter_users in either [NSS] or per-domain)
Nack.
OK, I've changed the patches and also split them into three. I think it actually makes sense now that the options are quite different, previously it was just three new NSS options.
On Wed, 2011-05-18 at 11:22 +0200, Jakub Hrozek wrote:
On 05/13/2011 02:54 PM, Stephen Gallagher wrote:
On Fri, 2011-05-13 at 11:14 +0200, Jakub Hrozek wrote:
On 05/10/2011 01:35 PM, Stephen Gallagher wrote:
On Tue, 2011-05-10 at 10:58 +0200, Jakub Hrozek wrote:
On 05/09/2011 07:42 PM, Stephen Gallagher wrote:
On Mon, 2011-05-09 at 10:45 +0200, Jakub Hrozek wrote: > On 05/06/2011 04:05 PM, Stephen Gallagher wrote: >> get_shell_override() should not shortcut out if there are no allowed >> shells. It should still check /etc/shells because if it's not listed, we >> want to convert it to nologin. >> > > This is done in the attached patch, but I don't agree with the change. > > It differs from the current behaviour and moreover allowed_shells is > unset in the default setting, so the change would affect anyone running > with defaults. >
Well, that wouldn't really have been a change in behavior, since if the shell didn't exist on the system, the login program or SSH would still be denying them.
Oh, I though the user would get /bin/sh..
In this case I agree with you..thank you for explaining.
I was thinking about this more last night, and I think in the end that you were probably right with how you did this the first time. While it's true that both 'login' and 'sshd' will just reject the user (actually, they'll try to exec the shell, get an error from exec and then bail out), it's possible that other mechanisms like rlogin or telnet or similar might have their own ways of behaving (implementing their own fallback, for example).
So I think that we probably need to just punt if we don't have an allowed_shells option and return the LDAP value.
Sorry for going back and forth on this so many times.
No problem :-)
New patches attached. They also change the label "fail" to "done" in the two functions that expand templates.
Please don't squash unrelated changes into patches. The label change should have been separate.
Didn't we use to say that trivial changes such as fixing typos can be squashed? Or does that only apply when we're changing code around that area?
This is a bit of an edge case. Fixing a typo in a piece of code you're working with is fine to squash in. Making the same change to a half-dozen similar places is probably grounds for separating into its own patch, just for ease of code review.
The general rule I try to follow is: one patch per logical change. This way a reviewer can pick up a patch and know that, while there may be a five patch series, each individual patch should be relatively easy to review and lead logically to the next.
Much the same way that this patch should have been three, one for shells, one for homedir and one for GID.
I just realized that I have one more nack for this. While I think the shell overrides are fine, homedir and GID need to be configurable per-domain (especially GID). Configuring GID globally for all domains just doesn't make sense. Homedir I think should probably be settable in either place (similar to how we do filter_users in either [NSS] or per-domain)
Nack.
OK, I've changed the patches and also split them into three. I think it actually makes sense now that the options are quite different, previously it was just three new NSS options.
Patch 0001: Ack
Patch 0002: Ack
Patch 0003: Nack. One last minor change. I realized that nss_get_etc_shells() could be vulnerable to a race-condition attack. If an administrator accidentally had the wrong permissions set on /etc/shells, then it would be possible for an attacker to monitor that file for accesses with inotify and replace its contents between the initial read for length and the second read for contents. Thus, a buffer overrun could be exploited.
I think it would be wiser for us to do buffered talloc_realloc()'s (say, five at a time) and run through the getusershell() loop only once. Probably putting an upper limit of, say 50 shells just to be safe.
On 05/18/2011 02:47 PM, Stephen Gallagher wrote:
On Wed, 2011-05-18 at 11:22 +0200, Jakub Hrozek wrote:
On 05/13/2011 02:54 PM, Stephen Gallagher wrote:
On Fri, 2011-05-13 at 11:14 +0200, Jakub Hrozek wrote:
On 05/10/2011 01:35 PM, Stephen Gallagher wrote:
On Tue, 2011-05-10 at 10:58 +0200, Jakub Hrozek wrote:
On 05/09/2011 07:42 PM, Stephen Gallagher wrote: > On Mon, 2011-05-09 at 10:45 +0200, Jakub Hrozek wrote: >> On 05/06/2011 04:05 PM, Stephen Gallagher wrote: >>> get_shell_override() should not shortcut out if there are no allowed >>> shells. It should still check /etc/shells because if it's not listed, we >>> want to convert it to nologin. >>> >> >> This is done in the attached patch, but I don't agree with the change. >> >> It differs from the current behaviour and moreover allowed_shells is >> unset in the default setting, so the change would affect anyone running >> with defaults. >> > > Well, that wouldn't really have been a change in behavior, since if the > shell didn't exist on the system, the login program or SSH would still > be denying them.
Oh, I though the user would get /bin/sh..
In this case I agree with you..thank you for explaining.
I was thinking about this more last night, and I think in the end that you were probably right with how you did this the first time. While it's true that both 'login' and 'sshd' will just reject the user (actually, they'll try to exec the shell, get an error from exec and then bail out), it's possible that other mechanisms like rlogin or telnet or similar might have their own ways of behaving (implementing their own fallback, for example).
So I think that we probably need to just punt if we don't have an allowed_shells option and return the LDAP value.
Sorry for going back and forth on this so many times.
No problem :-)
New patches attached. They also change the label "fail" to "done" in the two functions that expand templates.
Please don't squash unrelated changes into patches. The label change should have been separate.
Didn't we use to say that trivial changes such as fixing typos can be squashed? Or does that only apply when we're changing code around that area?
This is a bit of an edge case. Fixing a typo in a piece of code you're working with is fine to squash in. Making the same change to a half-dozen similar places is probably grounds for separating into its own patch, just for ease of code review.
The general rule I try to follow is: one patch per logical change. This way a reviewer can pick up a patch and know that, while there may be a five patch series, each individual patch should be relatively easy to review and lead logically to the next.
Much the same way that this patch should have been three, one for shells, one for homedir and one for GID.
I just realized that I have one more nack for this. While I think the shell overrides are fine, homedir and GID need to be configurable per-domain (especially GID). Configuring GID globally for all domains just doesn't make sense. Homedir I think should probably be settable in either place (similar to how we do filter_users in either [NSS] or per-domain)
Nack.
OK, I've changed the patches and also split them into three. I think it actually makes sense now that the options are quite different, previously it was just three new NSS options.
Patch 0001: Ack
Patch 0002: Ack
Patch 0003: Nack. One last minor change. I realized that nss_get_etc_shells() could be vulnerable to a race-condition attack. If an administrator accidentally had the wrong permissions set on /etc/shells, then it would be possible for an attacker to monitor that file for accesses with inotify and replace its contents between the initial read for length and the second read for contents. Thus, a buffer overrun could be exploited.
Nice catch..
I think it would be wiser for us to do buffered talloc_realloc()'s (say, five at a time) and run through the getusershell() loop only once. Probably putting an upper limit of, say 50 shells just to be safe.
Done. I love working around terrible legacy interfaces.
On Wed, 2011-05-18 at 17:48 +0200, Jakub Hrozek wrote:
Patch 0003: Nack. One last minor change. I realized that nss_get_etc_shells() could be vulnerable to a race-condition attack. If an administrator accidentally had the wrong permissions set on /etc/shells, then it would be possible for an attacker to monitor that file for accesses with inotify and replace its contents between the initial read for length and the second read for contents. Thus, a buffer overrun could be exploited.
Nice catch..
I think it would be wiser for us to do buffered talloc_realloc()'s (say, five at a time) and run through the getusershell() loop only once. Probably putting an upper limit of, say 50 shells just to be safe.
Done. I love working around terrible legacy interfaces.
Nack.
Please do the check for SHELL_REALLOC_MAX before actually performing the realloc. Right now it happens to work okay because SHELL_REALLOC_MAX is a multiple of SHELL_REALLOC_INCREMENT, but if we ever changed one or the other, it could cause problems. Also, if we DO have more than 50 shells, you've just allocated space unnecessarily for 55 shells.
Also, minor nitpick, could you please reduce the "Found shell" debug level to 6 to match the level at which we print other options?
One of these days we really need to write down a formal debug level specification. Right now it's pretty much unregulated.
On 05/18/2011 06:02 PM, Stephen Gallagher wrote:
On Wed, 2011-05-18 at 17:48 +0200, Jakub Hrozek wrote:
Patch 0003: Nack. One last minor change. I realized that nss_get_etc_shells() could be vulnerable to a race-condition attack. If an administrator accidentally had the wrong permissions set on /etc/shells, then it would be possible for an attacker to monitor that file for accesses with inotify and replace its contents between the initial read for length and the second read for contents. Thus, a buffer overrun could be exploited.
Nice catch..
I think it would be wiser for us to do buffered talloc_realloc()'s (say, five at a time) and run through the getusershell() loop only once. Probably putting an upper limit of, say 50 shells just to be safe.
Done. I love working around terrible legacy interfaces.
Nack.
Please do the check for SHELL_REALLOC_MAX before actually performing the realloc. Right now it happens to work okay because SHELL_REALLOC_MAX is a multiple of SHELL_REALLOC_INCREMENT, but if we ever changed one or the other, it could cause problems. Also, if we DO have more than 50 shells, you've just allocated space unnecessarily for 55 shells.
Also, minor nitpick, could you please reduce the "Found shell" debug level to 6 to match the level at which we print other options?
One of these days we really need to write down a formal debug level specification. Right now it's pretty much unregulated.
attached
On Thu, 2011-05-19 at 10:11 +0200, Jakub Hrozek wrote:
On 05/18/2011 06:02 PM, Stephen Gallagher wrote:
On Wed, 2011-05-18 at 17:48 +0200, Jakub Hrozek wrote:
Patch 0003: Nack. One last minor change. I realized that nss_get_etc_shells() could be vulnerable to a race-condition attack. If an administrator accidentally had the wrong permissions set on /etc/shells, then it would be possible for an attacker to monitor that file for accesses with inotify and replace its contents between the initial read for length and the second read for contents. Thus, a buffer overrun could be exploited.
Nice catch..
I think it would be wiser for us to do buffered talloc_realloc()'s (say, five at a time) and run through the getusershell() loop only once. Probably putting an upper limit of, say 50 shells just to be safe.
Done. I love working around terrible legacy interfaces.
Nack.
Please do the check for SHELL_REALLOC_MAX before actually performing the realloc. Right now it happens to work okay because SHELL_REALLOC_MAX is a multiple of SHELL_REALLOC_INCREMENT, but if we ever changed one or the other, it could cause problems. Also, if we DO have more than 50 shells, you've just allocated space unnecessarily for 55 shells.
Also, minor nitpick, could you please reduce the "Found shell" debug level to 6 to match the level at which we print other options?
One of these days we really need to write down a formal debug level specification. Right now it's pretty much unregulated.
attached
Ack
On Fri, 2011-05-20 at 05:48 -0400, Stephen Gallagher wrote:
On Thu, 2011-05-19 at 10:11 +0200, Jakub Hrozek wrote:
On 05/18/2011 06:02 PM, Stephen Gallagher wrote:
On Wed, 2011-05-18 at 17:48 +0200, Jakub Hrozek wrote:
Patch 0003: Nack. One last minor change. I realized that nss_get_etc_shells() could be vulnerable to a race-condition attack. If an administrator accidentally had the wrong permissions set on /etc/shells, then it would be possible for an attacker to monitor that file for accesses with inotify and replace its contents between the initial read for length and the second read for contents. Thus, a buffer overrun could be exploited.
Nice catch..
I think it would be wiser for us to do buffered talloc_realloc()'s (say, five at a time) and run through the getusershell() loop only once. Probably putting an upper limit of, say 50 shells just to be safe.
Done. I love working around terrible legacy interfaces.
Nack.
Please do the check for SHELL_REALLOC_MAX before actually performing the realloc. Right now it happens to work okay because SHELL_REALLOC_MAX is a multiple of SHELL_REALLOC_INCREMENT, but if we ever changed one or the other, it could cause problems. Also, if we DO have more than 50 shells, you've just allocated space unnecessarily for 55 shells.
Also, minor nitpick, could you please reduce the "Found shell" debug level to 6 to match the level at which we print other options?
One of these days we really need to write down a formal debug level specification. Right now it's pretty much unregulated.
attached
Ack
Pushed to master
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Stephen Gallagher