[sssd PR#260][opened] Update sss_override.c

[amitkumar50]

URL: https://github.com/SSSD/sssd/pull/260
Author: amitkumar50
 Title: #260: Update sss_override.c
Action: opened

PR body:
"""
This is Fix for https://pagure.io/SSSD/sssd/issue/2834
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/260/head:pr260
git checkout pr260

[sssd PR#260][comment] Update sss_override.c

[centos-ci]

URL: https://github.com/SSSD/sssd/pull/260
Title: #260: Update sss_override.c

centos-ci commented:
"""
Can one of the admins verify this patch?
"""

See the full comment at https://github.com/SSSD/sssd/pull/260#issuecomment-298910666

[sssd PR#260][comment] Update sss_override.c

[centos-ci]

URL: https://github.com/SSSD/sssd/pull/260
Title: #260: Update sss_override.c

centos-ci commented:
"""
Can one of the admins verify this patch?
"""

See the full comment at https://github.com/SSSD/sssd/pull/260#issuecomment-298910674

[sssd PR#260][+Changes requested] Update sss_override.c

[fidencio]

URL: https://github.com/SSSD/sssd/pull/260
Title: #260: Update sss_override.c

Label: +Changes requested

[sssd PR#260][synchronized] Update sss_override.c

[amitkumar50]

URL: https://github.com/SSSD/sssd/pull/260
Author: amitkumar50
 Title: #260: Update sss_override.c
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/260/head:pr260
git checkout pr260

[sssd PR#260][comment] Update sss_override.c

[amitkumar50]

URL: https://github.com/SSSD/sssd/pull/260
Title: #260: Update sss_override.c

amitkumar50 commented:
"""
Dear Fabiano,

Huge thanks for response & consideration.

Frankly speaking I feel my short log+commit message are more to the
point & explanatory.
Your commit message does not explain complete problem=> Errorout+override.

Will add empty line between Commit Message & Explanation.

Finally its your call.

Thanks
Amit
On 05/04/2017 12:04 PM, fidencio wrote:
> *@fidencio* commented on this pull request.
>
> Amit,
>
> Firstly, thanks for quickly wrapping up the changes.
>
> I do have a few comments related to the commit short log and commit
> message.
>
> For the short log, I'd like to suggest something like:
> "TOOLS/OVERRIDE: Error out when setting uid/gid as 0"
>
> For the commit message, what do you think about:
> "Currently sss_override silently ignores when uid/gid is set as 0. In
> order to inform the user that setting uid/gid to zero is not allowed,
> let's error out those cases."
>
> May I ask what do you think about those commit short log and message?
> Please, let me know because may be the case yours is cleaner (and then
> we want to discuss till we have a consensus).
>
> Also, please, add an empty line between the explanation in the commit
> message and the "Resolves:" part.
>
> Thanks a lot for the contribution and I'll follow up actually testing
> your patches later Today/Tomorrow.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> https://github.com/SSSD/sssd/pull/260#pullrequestreview-36194074, or
> mute the thread
> https://github.com/notifications/unsubscribe-auth/AWA8SE3FUlDlGZ-JRyLtjg63gQu3CNG1ks5r2XGRgaJpZM4NPZfq.

[sssd PR#260][comment] Update sss_override.c

[fidencio]

URL: https://github.com/SSSD/sssd/pull/260
Title: #260: Update sss_override.c

fidencio commented:
"""
So, I've tried to do some simple test here and the proposed patch doesn't work as expected.

When trying to sss_override user-add john -u 1001 the tool returns an error complaining that gid is 0.

From a really quick look at the code I have the impression that the uid/gid is filled with the ones from the user passed as input, so it shouldn't happen.

I don't have the time right now to deeply dive into this. Amit, could you check you have the same issue?
Are you going to dive deeply into the issue found?
"""

See the full comment at https://github.com/SSSD/sssd/pull/260#issuecomment-299434151

[sssd PR#260][synchronized] Update sss_override.c

[amitkumar50]

URL: https://github.com/SSSD/sssd/pull/260
Author: amitkumar50
 Title: #260: Update sss_override.c
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/260/head:pr260
git checkout pr260

[sssd PR#260][synchronized] Update sss_override.c

[amitkumar50]

URL: https://github.com/SSSD/sssd/pull/260
Author: amitkumar50
 Title: #260: Update sss_override.c
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/260/head:pr260
git checkout pr260

[sssd PR#260][-Changes requested] Update sss_override.c

[lslebodn]

URL: https://github.com/SSSD/sssd/pull/260
Title: #260: Update sss_override.c

Label: -Changes requested

[sssd PR#260][comment] Update sss_override.c

[fidencio]

URL: https://github.com/SSSD/sssd/pull/260
Title: #260: Update sss_override.c

fidencio commented:
"""
Amit,

Sorry for the long time taken for reviewing this patch.

I didn't go through the code but I still can see some functional issues with your patches. (same one mentioned in the [in the comment above](https://github.com/SSSD/sssd/pull/260#issuecomment-299434151).

Let me show you what happens when I have your patches applied:
```
[root@client sssd]# sss_override user-add [e-mail] -u 11111
SSSD needs to be restarted for the changes to take effect.
Setting uid or gid as 0 is not allowed
```

This is *not* the expected behaviour. For instance, if the admin sets a user's id override without passing the gid ... it's expected that the gid will be kept as the same. With your patch, what happens is we fail the operation because the gid was not set.

The reason why it happens is because when parsing the user passed in the command line (please, take a look on `parse_cmdlinser_user_add` function the default value taken for uid/gid is 0 (when none is passed).

Here's what the struct override_user looks like:
 ```
struct override_user {                                                                                                             
     const char *input_name;
     const char *orig_name;
     const char *sysdb_name;
     struct sss_domain_info *domain;

const char *name;
     uid_t uid;
     gid_t gid;
     const char *home;
     const char *shell;struct override_user {                                                                                                             
     const char *input_name;
     const char *orig_name;
     const char *sysdb_name;
     struct sss_domain_info *domain;

const char *name;
     uid_t uid;
     gid_t gid;
     const char *home;
     const char *shell;
     const char *gecos;
     const char *cert;
 };
```

So, uid and gid cannot be set to a negative number (like -1) in order to differentiate whether the 0 was intentionally set just was not set at all (and the admin wants to keep the current user's uid/gid).

As suggested in the previous comment, you'll have to dig a bit more in the code in order to propose a better solution.

Maybe a better solution would be to populate the override_user structure before hand and then go and set it in the sysdb only in case we are sure a 0 wasn't passed in the command line? I'm not even sure it would work, this is just coming from the top of my head and I haven't any experience with this code (yet).

Please, let me know if you're interested on doing such changes/digging deeper into the issue.
"""

See the full comment at https://github.com/SSSD/sssd/pull/260#issuecomment-306409624

[sssd PR#260][+Changes requested] Update sss_override.c

[fidencio]

URL: https://github.com/SSSD/sssd/pull/260
Title: #260: Update sss_override.c

Label: +Changes requested

[sssd PR#260][comment] Update sss_override.c

[fidencio]

URL: https://github.com/SSSD/sssd/pull/260
Title: #260: Update sss_override.c

fidencio commented:
"""
Amit,

Sorry for the long time taken for reviewing this patch.

I didn't go through the code but I still can see some functional issues with your patches. (same one mentioned in the [in the comment above](https://github.com/SSSD/sssd/pull/260#issuecomment-299434151).

Let me show you what happens when I have your patches applied:
```
[root@client sssd]# sss_override user-add [e-mail] -u 11111
SSSD needs to be restarted for the changes to take effect.
Setting uid or gid as 0 is not allowed
```

This is *not* the expected behaviour. For instance, if the admin sets a user's id override without passing the gid ... it's expected that the gid will be kept as the same. With your patch, what happens is we fail the operation because the gid was not set.

The reason why it happens is because when parsing the user passed in the command line (please, take a look on `parse_cmdlinser_user_add` function the default value taken for uid/gid is 0 (when none is passed).

Here's what the struct override_user looks like:
 ```
struct override_user {                                                                                                             
     const char *input_name;
     const char *orig_name;
     const char *sysdb_name;
     struct sss_domain_info *domain;

const char *name;
     uid_t uid;
     gid_t gid;
     const char *home;
     const char *shell;struct override_user {                                                                                                             
     const char *input_name;
     const char *orig_name;
     const char *sysdb_name;
     struct sss_domain_info *domain;

const char *name;
     uid_t uid;
     gid_t gid;
     const char *home;
     const char *shell;
     const char *gecos;
     const char *cert;
 };
```

So, uid and gid cannot be set to a negative number (like -1) in order to differentiate whether the 0 was intentionally set just was not set at all (and the admin wants to keep the current user's uid/gid).

As suggested in the previous comment, you'll have to dig a bit more in the code in order to propose a better solution.

Maybe a better solution would be to populate the override_user structure before hand and then go and set it in the sysdb only in case we are sure a 0 wasn't passed in the command line? I'm not even sure it would work, this is just coming from the top of my head and I haven't any experience with this code (yet).

Please, let me know if you're interested on doing such changes/digging deeper into the issue.
"""

See the full comment at https://github.com/SSSD/sssd/pull/260#issuecomment-306409624

[sssd PR#260][comment] Update sss_override.c

[fidencio]

URL: https://github.com/SSSD/sssd/pull/260
Title: #260: Update sss_override.c

fidencio commented:
"""
Amit,

Sorry for the long time taken for reviewing this patch.

I didn't go through the code but I still can see some functional issues with your patches. (same one mentioned in the [in the comment above](https://github.com/SSSD/sssd/pull/260#issuecomment-299434151).

Let me show you what happens when I have your patches applied:
```
[root@client sssd]# sss_override user-add [e-mail] -u 11111
SSSD needs to be restarted for the changes to take effect.
Setting uid or gid as 0 is not allowed
```

This is *not* the expected behaviour. For instance, if the admin sets a user's id override without passing the gid ... it's expected that the gid will be kept as the same. With your patch, what happens is we fail the operation because the gid was not set.

The reason why it happens is because when parsing the user passed in the command line (please, take a look on `parse_cmdlinser_user_add` function the default value taken for uid/gid is 0 (when none is passed).

Here's what the struct override_user looks like:
 ```
struct override_user {                                                                                                             
     const char *input_name;
     const char *orig_name;
     const char *sysdb_name;
     struct sss_domain_info *domain;

const char *name;
     uid_t uid;
     gid_t gid;
     const char *home;
     const char *shell;struct override_user {                                                                                                             
     const char *input_name;
     const char *orig_name;
     const char *sysdb_name;
     struct sss_domain_info *domain;

const char *name;
     uid_t uid;
     gid_t gid;
     const char *home;
     const char *shell;
     const char *gecos;
     const char *cert;
 };
```

So, uid and gid cannot be set to a negative number (like -1) in order to differentiate whether the 0 was intentionally set or just was not set at all (and the admin wants to keep the current user's uid/gid).

As suggested in the previous comment, you'll have to dig a bit more in the code in order to propose a better solution.

Maybe a better solution would be to populate the override_user structure before hand and then go and set it in the sysdb only in case we are sure a 0 wasn't passed in the command line? I'm not even sure it would work, this is just coming from the top of my head and I haven't any experience with this code (yet).

Please, let me know if you're interested on doing such changes/digging deeper into the issue.
"""

See the full comment at https://github.com/SSSD/sssd/pull/260#issuecomment-306409624

[sssd PR#260][comment] Update sss_override.c

[lslebodn]

URL: https://github.com/SSSD/sssd/pull/260
Title: #260: Update sss_override.c

lslebodn commented:
"""
@amitkumar50 Any progress here? Or should we close this PR? 
"""

See the full comment at https://github.com/SSSD/sssd/pull/260#issuecomment-317389674

[sssd PR#260][comment] Update sss_override.c

[amitkumar50]

URL: https://github.com/SSSD/sssd/pull/260
Title: #260: Update sss_override.c

amitkumar50 commented:
"""
@lslebodn Don't close it I will think fresh and submit changes soon.
Thanks for reminder..
"""

See the full comment at https://github.com/SSSD/sssd/pull/260#issuecomment-317390329

[sssd PR#260][synchronized] Update sss_override.c

[amitkumar50]

URL: https://github.com/SSSD/sssd/pull/260
Author: amitkumar50
 Title: #260: Update sss_override.c
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/260/head:pr260
git checkout pr260

[sssd PR#260][comment] Update sss_override.c

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/260
Title: #260: Update sss_override.c

jhrozek commented:
"""
@amitkumar50 do you need any help with this PR?
"""

See the full comment at https://github.com/SSSD/sssd/pull/260#issuecomment-330496175

[sssd PR#260][comment] Update sss_override.c

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/260
Title: #260: Update sss_override.c

jhrozek commented:
"""
@amitkumar50 I'm going to close this PR for now because it hasn't received any update for a long time. But please feel free to either reopen it or open a new one with a patch update.
"""

See the full comment at https://github.com/SSSD/sssd/pull/260#issuecomment-334488518

[sssd PR#260][closed] Update sss_override.c

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/260
Author: amitkumar50
 Title: #260: Update sss_override.c
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/260/head:pr260
git checkout pr260