On Tue, Jul 29, 2014 at 04:15:16PM +0200, Daniel Gollub wrote:
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 <dgollub(a)brocade.com>
Reviewed-by: Sven-Thorsten Dietrich <sven(a)brocade.com>
---
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.