Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update sss_override.c #260

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
5 participants
@amitkumar50
Copy link
Contributor

commented May 3, 2017

@centos-ci

This comment has been minimized.

Copy link
Collaborator

commented May 3, 2017

Can one of the admins verify this patch?

1 similar comment
@centos-ci

This comment has been minimized.

Copy link
Collaborator

commented May 3, 2017

Can one of the admins verify this patch?

@fidencio
Copy link
Contributor

left a comment

Amit,

Firstly thanks for the provided patch. I didn't go through the functionality of the patch yet, just giving you a quick review of the patch itself without any testing.

About the code you'll be able to find comments inline.

There are a few comments about the commit message that I'd like to share here:
We do follow some template for the commit messages:

COMPONENT: Subject                                                                                                                  
 
Explanation
 
Resolves:
https://pagure.io/SSSD/sssd/issue/XXXX

Please, try to keep the subject line within 52 chars ----|
Also please try to not exceed 72 characters of length for the body --|

It helps us to review the patch and to understand the reason we needed the patch when checking it any time in the future.

So, please, add a good explanation of why the changes are needed (the reason, not the description of the changes itself in the code) and a link to the pagure issue.

Would you mind on force updating this PR following our commit template and with the changes suggested in the inline review?

Best Regards,

@@ -333,13 +333,21 @@ static struct sysdb_attrs *build_attrs(TALLOC_CTX *mem_ctx,
if (ret != EOK) {
goto done;
}
}else {

This comment has been minimized.

Copy link
@fidencio

fidencio May 3, 2017

Contributor

Please, add an space between the "}" and the "else", like:```

...
} else {
...

This comment has been minimized.

Copy link
@amitkumar50

amitkumar50 May 4, 2017

Author Contributor

Hello Fabiano,
Greatly appreciate your comments. Will take care in future commits. Sincere Apologies for this time.
Extra space add: Done
Commit Id: f24f9fd

@@ -333,13 +333,21 @@ static struct sysdb_attrs *build_attrs(TALLOC_CTX *mem_ctx,
if (ret != EOK) {
goto done;
}
}else {
fprintf(stderr, ("Setting uid=0 is not allowed\n"));

This comment has been minimized.

Copy link
@fidencio

fidencio May 3, 2017

Contributor

I'm guessing here we want this message to be exposed to the user. So, it would be nice if it's also translated.
If that's the case, you should do something like:
fprintf(stderr, _("Setting uid as 0 is not allowed\n"));

If that's not the case, DEBUG macro would be more appropriate.

This comment has been minimized.

Copy link
@amitkumar50

amitkumar50 May 4, 2017

Author Contributor

Done

@@ -333,13 +333,21 @@ static struct sysdb_attrs *build_attrs(TALLOC_CTX *mem_ctx,
if (ret != EOK) {
goto done;
}
}else {
fprintf(stderr, ("Setting uid=0 is not allowed\n"));
ret = !EOK;

This comment has been minimized.

Copy link
@fidencio

fidencio May 3, 2017

Contributor

Setting the error code to !EOK is not exactly what we expect is this case.
For instance, we want to have the error code giving as much info as possible for whoever is receiving it.

In this specific case, we're returning an error because an invalid option (uid = 0) ... so, I do believe that receiving an error as EINVAL would be more appropriate.

This comment has been minimized.

Copy link
@amitkumar50

amitkumar50 May 4, 2017

Author Contributor

Done

}

if (gid != 0) {
ret = sysdb_attrs_add_uint32(attrs, SYSDB_GIDNUM, gid);
if (ret != EOK) {
goto done;
}
}else {

This comment has been minimized.

Copy link
@fidencio

fidencio May 3, 2017

Contributor

Same comment about the coding style applies here.

This comment has been minimized.

Copy link
@amitkumar50

amitkumar50 May 4, 2017

Author Contributor

Done

}

if (gid != 0) {
ret = sysdb_attrs_add_uint32(attrs, SYSDB_GIDNUM, gid);
if (ret != EOK) {
goto done;
}
}else {
fprintf(stderr, ("Setting gid=0 is not allowed\n"));

This comment has been minimized.

Copy link
@fidencio

fidencio May 3, 2017

Contributor

Same comment about the internationalization applies here.

This comment has been minimized.

Copy link
@amitkumar50

amitkumar50 May 4, 2017

Author Contributor

Done

}

if (gid != 0) {
ret = sysdb_attrs_add_uint32(attrs, SYSDB_GIDNUM, gid);
if (ret != EOK) {
goto done;
}
}else {
fprintf(stderr, ("Setting gid=0 is not allowed\n"));
ret = !EOK;

This comment has been minimized.

Copy link
@fidencio

fidencio May 3, 2017

Contributor

Same comment about the return value applies here.

This comment has been minimized.

Copy link
@amitkumar50

amitkumar50 May 4, 2017

Author Contributor

Done

@fidencio
Copy link
Contributor

left a comment

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.

@amitkumar50

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2017

@fidencio

This comment has been minimized.

Copy link
Contributor

commented May 5, 2017

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?

TOOLS/OVERRIDE: Error out when setting uid/gid as 0
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.

Resolves:
https://pagure.io/SSSD/sssd/issue/2834

@amitkumar50 amitkumar50 force-pushed the amitkumar50:3a-may-17 branch from 47c2581 to d1f28e0 May 16, 2017

@fidencio

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2017

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.

Let me show you what happens when I have your patches applied:

[root@client sssd]# sss_override user-add Administrator@fidencio.lan -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.

@amitkumar50

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2017

Thanks Fibencio, I am rethinking on the Solution..

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2017

@amitkumar50 Any progress here? Or should we close this PR?

@amitkumar50

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2017

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

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2017

@amitkumar50 do you need any help with this PR?

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2017

@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.

@jhrozek jhrozek closed this Oct 5, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.