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.
>
>> + } else if (strncmp(keyend, "gs", 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 = wbcSidToGid(&sid, &gid);
>> + if (rc)
>> + syslog(LOG_DEBUG, "SID %s to gid wbc error: %d",
>> + keyend, rc);
>> + }
>> + if (rc) {
>> + gr = getgrnam("nogroup");
>> + if (!gr)
>> + syslog(LOG_DEBUG, "SID %s to gid pw error: %d",
>> + keyend, rc);
>> + else {
>> + gid = gr->gr_gid;
>> + rc = 0;
>> + }
>> + }
>> + if (!rc) {
>> + rc = keyctl_instantiate(key, &gid, sizeof(gid_t), 0);
>> + if (rc)
>> + syslog(LOG_ERR, "%s: key inst: %s",
>> + __func__, strerror(errno));
>> + }
>> + } else
>> + syslog(LOG_DEBUG, "Invalid SID");
> ^^^^^^^^^^^^^^
> Might be helpful to know what that SID is, don't you
> think?
yes, will fix this.
>> +
>> + return rc;
>> +}
>> +
>> /*
>> * Older kernels sent IPv6 addresses without colons. Well, at least
>> * they're fixed-length strings. Convert these addresses to have colon
>> @@ -833,6 +925,12 @@ int main(const int argc, char *const argv[])
>> goto out;
>> }
>>
>> + if ((strncmp(buf, "cifs.cifs_idmap", sizeof("cifs.cifs_idmap") - 1)
>> + == 0)) {
>> + rc = cifs_sid_resolver(key, buf);
>> + goto out;
>> + }
>> +
>> have = decode_key_description(buf, &arg);
>> SAFE_FREE(buf);
>> if ((have & DKD_MUSTHAVE_SET) != DKD_MUSTHAVE_SET) {
>
>
> --
> 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
