On Tue, Jul 29, 2014 at 04:15:16PM +0200, Daniel Gollub wrote: > Fixes: > https://fedorahosted.org/sssd/ticket/1021
Now that we have a design document on the list, let me include a couple of nitpicks, which might be nice to fix in the next iteration.. > > Signed-off-by: Daniel Gollub <[email protected]> > Reviewed-by: Sven-Thorsten Dietrich <[email protected]> > --- > src/providers/data_provider.h | 1 + > src/responder/pam/pamsrv_cmd.c | 44 > +++++++++++++++++++++++++++++++++++++++++- > src/sss_client/pam_sss.c | 29 ++++++++++++++++++++++++++-- > src/sss_client/sss_cli.h | 1 + > 4 files changed, 72 insertions(+), 3 deletions(-) > > diff --git a/src/providers/data_provider.h b/src/providers/data_provider.h > index ebb4fad..0abfd0f 100644 > --- a/src/providers/data_provider.h > +++ b/src/providers/data_provider.h > @@ -170,6 +170,7 @@ struct pam_data { > char *tty; > char *ruser; > char *rhost; > + char **requested_domains; > struct sss_auth_token *authtok; > struct sss_auth_token *newauthtok; > uint32_t cli_pid; > diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c > index 140d541..c6539e5 100644 > --- a/src/responder/pam/pamsrv_cmd.c > +++ b/src/responder/pam/pamsrv_cmd.c > @@ -44,6 +44,26 @@ enum pam_verbosity { > > static void pam_reply(struct pam_auth_req *preq); > > +static bool is_domain_requested(struct pam_data *pd, const char *domain_name) > +{ > + int i; > + > + /* If none specific domains got requested via pam, all domains are > allowed. > + * Which mimics the default/original behaviour. > + */ > + if (!pd->requested_domains) > + return true; We prefer to use curly brackets around one-line statements, too: if (!pd->requested_domains) { return true; } It's mostly defensive coding so that when the single-line statement becomes multi-line, we don't forget to add the brackets as well.. > + > + for (i = 0; pd->requested_domains[i]; i++) { > + if (strcmp(domain_name, pd->requested_domains[i])) same here. > + continue; > + > + return true; > + } > + > + return false; > +} > + > static int extract_authtok_v2(struct sss_auth_token *tok, > size_t data_size, uint8_t *body, size_t blen, > size_t *c) > @@ -146,6 +166,7 @@ static int pam_parse_in_data_v2(struct sss_domain_info > *domains, > int ret; > uint32_t start; > uint32_t terminator; > + char *requested_domains; > > if (blen < 4*sizeof(uint32_t)+2) { > DEBUG(SSSDBG_CRIT_FAILURE, "Received data is invalid.\n"); > @@ -202,6 +223,16 @@ static int pam_parse_in_data_v2(struct sss_domain_info > *domains, > ret = extract_string(&pd->rhost, size, body, blen, &c); > if (ret != EOK) return ret; > break; > + case SSS_PAM_ITEM_REQUESTED_DOMAINS: > + ret = extract_string(&requested_domains, size, body, > blen, &c); > + if (ret != EOK) return ret; This case is I think OK, (return on the same line as if) I would just put a newline after the if, but that's more of a personal preference. > + ret = split_on_separator(pd, requested_domains, ',', > true, true, > + &pd->requested_domains, NULL); > + if (ret != EOK) { > + DEBUG(1, ("Failed to parse requested_domains > list!\n")); We have converted our DEBUG macros to a new SSSDBG_ constants instead of numbers and no longer need to wrap the DEBUG body in (). It would be nice to use the same pattern in your patch. > + return ret; > + } > + break; > case SSS_PAM_ITEM_CLI_PID: > ret = extract_uint32_t(&pd->cli_pid, size, > body, blen, &c); > @@ -836,12 +867,22 @@ static int pam_forwarder(struct cli_ctx *cctx, int > pam_cmd) > ret = ENOENT; > goto done; > } > + > + /* skip this domain if not requested */ > + if (!is_domain_requested(pd, dom->name)) { > + ret = ENOENT; > + goto done; > + } > } else { > for (dom = preq->cctx->rctx->domains; > dom; > dom = get_next_domain(dom, false)) { > if (dom->fqnames) continue; > > + /* skip this domain if not requested */ > + if (!is_domain_requested(pd, dom->name)) > + continue; Single-line statement again. > + > ncret = sss_ncache_check_user(pctx->ncache, pctx->neg_timeout, > dom, pd->user); > if (ncret == ENOENT) { > @@ -856,7 +897,8 @@ static int pam_forwarder(struct cli_ctx *cctx, int > pam_cmd) > "User [%s@%s] filtered out (negative cache). " > "Trying next domain.\n", pd->user, dom->name); > } > - if (!dom) { > + > + if (!dom || !is_domain_requested(pd, dom->name)) { > ret = ENOENT; > goto done; > } > diff --git a/src/sss_client/pam_sss.c b/src/sss_client/pam_sss.c > index d2502d1..4d76bd2 100644 > --- a/src/sss_client/pam_sss.c > +++ b/src/sss_client/pam_sss.c > @@ -58,6 +58,7 @@ > #define PW_RESET_MSG_MAX_SIZE 4096 > > #define OPT_RETRY_KEY "retry=" > +#define OPT_DOMAINS_KEY "domains=" > > struct pam_items { > const char* pam_service; > @@ -81,6 +82,8 @@ struct pam_items { > pid_t cli_pid; > const char *login_name; > char *domain_name; > + const char *requested_domains; > + size_t requested_domains_size; > }; > > #define DEBUG_MGS_LEN 1024 > @@ -246,6 +249,9 @@ static int pack_message_v3(struct pam_items *pi, size_t > *size, > len += pi->pam_newauthtok != NULL ? > 3*sizeof(uint32_t) + pi->pam_newauthtok_size : 0; > len += 3*sizeof(uint32_t); /* cli_pid */ > + len += *pi->requested_domains != '\0' ? > + 2*sizeof(uint32_t) + pi->requested_domains_size : 0; > + > > buf = malloc(len); > if (buf == NULL) { > @@ -271,6 +277,9 @@ static int pack_message_v3(struct pam_items *pi, size_t > *size, > rp += add_string_item(SSS_PAM_ITEM_RHOST, pi->pam_rhost, > pi->pam_rhost_size, > &buf[rp]); > > + rp += add_string_item(SSS_PAM_ITEM_REQUESTED_DOMAINS, > pi->requested_domains, pi->requested_domains_size, > + &buf[rp]); > + > rp += add_uint32_t_item(SSS_PAM_ITEM_CLI_PID, (uint32_t) pi->cli_pid, > &buf[rp]); > > @@ -1061,6 +1070,9 @@ static int get_pam_items(pam_handle_t *pamh, struct > pam_items *pi) > > pi->domain_name = NULL; > > + if (pi->requested_domains == NULL) pi->requested_domains=""; > + pi->requested_domains_size=strlen(pi->requested_domains)+1; Can you put a space before and after the equals sign? Like: pi->requested_domains_size = strlen(pi->requested_domains) + 1; The rest looks good. Thanks again for the patch. _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
