On Wed, 16 Feb 2011 12:05:34 -0600
Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote:
> On Tue, Feb 15, 2011 at 2:45 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > On Fri, 11 Feb 2011 12:49:09 -0600
> > shirishpargaonkar@xxxxxxxxx wrote:
> >
> >> From: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
> >>
> >>
> >> Handle cifs_idmap type of key. Extract a SID string from the description
> >> and map it to either an uid or gid using winbind APIs.
> >> If that fails (e.g. because winbind is not installed/running or winbind returns
> >> an error), try to obtain uid of 'nobody' and gid of 'nogroup'.
> >> And if that fails, kernel assigns uid and gid (from mount superblock).
> >>
> >> An entry such as this
> >>
> >> create cifs.cifs_idmap * * /usr/sbin/cifs.upcall %k
> >>
> >> is needed in the file /etc/request-key.conf.
> >>
> >>
> >> Signed-off-by: Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx>
> >> ---
> >> Makefile.am | 2 +-
> >> cifs.upcall.c | 98 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> 2 files changed, 99 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/Makefile.am b/Makefile.am
> >> index 67a0190..c9018ae 100644
> >> --- a/Makefile.am
> >> +++ b/Makefile.am
> >> @@ -11,7 +11,7 @@ man_MANS = mount.cifs.8
> >> if CONFIG_CIFSUPCALL
> >> sbin_PROGRAMS = cifs.upcall
> >> cifs_upcall_SOURCES = cifs.upcall.c data_blob.c asn1.c spnego.c util.c
> >> -cifs_upcall_LDADD = -ltalloc -lkeyutils $(KRB5_LDADD)
> >> +cifs_upcall_LDADD = -ltalloc -lwbclient -lkeyutils $(KRB5_LDADD)
> >> man_MANS += cifs.upcall.8
> >>
> >> #
> >> diff --git a/cifs.upcall.c b/cifs.upcall.c
> >> index 479517c..fe8a6ad 100644
> >> --- a/cifs.upcall.c
> >> +++ b/cifs.upcall.c
> >> @@ -45,6 +45,13 @@
> >> #include <time.h>
> >> #include <netdb.h>
> >> #include <arpa/inet.h>
> >> +#include <stdint.h>
> >> +#include <stdbool.h>
> >> +#include <stdio.h>
> >> +#include <stdlib.h>
> >> +#include <errno.h>
> >> +#include <limits.h>
> >> +#include <wbclient.h>
> >>
> >> #include "util.h"
> >> #include "replace.h"
> >> @@ -695,6 +702,91 @@ static int cifs_resolver(const key_serial_t key, const char *key_descr)
> >> return 0;
> >> }
> >>
> >> +static int
> >> +cifs_sid_resolver(const key_serial_t key, const char *key_descr)
> >> +{
> >> + int i;
> >> + uid_t uid = 0;
> >> + gid_t gid = 0;;
> >> + wbcErr rc = 1;
> >> + const char *keyend = key_descr;
> >> + struct wbcDomainSid sid;
> >> + struct passwd *pw;
> >> + struct group *gr;
> >> +
> >> + /* skip next 4 ';' delimiters to get to description */
> >> + for (i = 1; i <= 4; ++i) {
> >> + keyend = index(keyend + 1, ';');
> >> + if (!keyend) {
> >> + syslog(LOG_ERR, "invalid key description: %s",
> >> + key_descr);
> >> + return 1;
> >> + }
> >> + }
> >> + keyend++;
> >> +
> >> + if (strncmp(keyend, "os", 2) == 0) {
> >> + keyend = index(keyend + 1, ':');
> >> + keyend++;
> >> + rc = wbcStringToSid(keyend, &sid);
> >> + if (rc)
> >> + syslog(LOG_DEBUG, "O strtosid: %s, rc: %d", keyend, rc);
> >> + else {
> >> + rc = wbcSidToUid(&sid, &uid);
> >> + if (rc)
> >> + syslog(LOG_DEBUG, "SID %s to uid wbc error: %d",
> >> + keyend, rc);
> >> + }
> >> + if (rc) {
> >> + pw = getpwnam("nobody");
> >> + if (!pw)
> >> + syslog(LOG_DEBUG, "SID %s to uid pw error: %d",
> >> + keyend, rc);
> >> + else {
> >> + uid = pw->pw_uid;
> >> + rc = 0;
> >> + }
> >> + }
> >> + if (!rc) {
> >> + rc = keyctl_instantiate(key, &uid, sizeof(uid_t), 0);
> >> + if (rc)
> >> + syslog(LOG_ERR, "%s: key inst: %s",
> >> + __func__, strerror(errno));
> >> + }
> >
> > Instead of all this if then nesting and continual checking for
> > the value of rc, I think it would be easier to follow the logic
> > and maintain this if it instead either did a "goto out" in the
> > error cases or just did a return when it hits an error.
>
> Will comments help? I thought of using function calls and goto calls
> but none of them will make readability and logic any easier.
> There are five different calls, and if either of first two fail, insetead of
> returning, third one gets called and all calls are different
> (e.g. if either of wbcStringToSid or wbcSidToUid, getpwnam should
> be called and if either of wbcStringToSid or wbcSidToGid fail,
> getgrnam should be called.
> keyctl_instantiate should be called if either wbcSidToUid or getpwnam
> succeed (in that order) or if either wbcSidToGid or getgrnam succeed
> (in that order).
> It would not be any clearer to follow this with goto statements.
>
Ok, I see your point. Comments probably would be helpful. At least
some that lay out the overall idea behind this code and why and how it
falls back when things fail.
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
