On Thu, May 22, 2014 at 07:57:28PM +0200, Pavel Březina wrote:

On 05/21/2014 11:03 PM, Jakub Hrozek wrote:

...

Hi,

the attaches two patches implement DBus methods to retrieve SSSD domains using the system bus as well as two example getters that leverage the infrastructure to Get and GetAll the object properties.

Pavel, I know you have a large patchset in a private tree that implements the other getters and moves the domain methods into a separate file. I'm fine with merging your and mine patches, I just wanted to get the ball rolling on the list. Feel free to reply with your patchset.

Hi, see the attachment.

I squash all my patches to these five - mainly discarded the moving the code around.

I kept you as the author for the methods as I made only very minor changes there. However, I took the ownership for the properties since there only very little of your original code remains there - I hope you don't mind.

The next three patches implements the SSSD components interface required for the OpenLMI provider. I'm sending it to this thread since it depends on the domain patches anyway - there is enough dependency fuzz already.

From 7e55b23741ed749e95e9d33184b0fd702b86bb46 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 22 Apr 2014 15:38:08 +0200 Subject: [PATCH 1/5] IFP: Add ListDomains and FindDomainByName

This one was acked by Pavel separately.

From d3d93a3d9ae3d2818c6fd3b78662aab27a3ace36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 22 May 2014 19:01:35 +0200 Subject: [PATCH 2/5] IFP: Implement domain getters

See inline for some nitpicks. In general, the code seems to work well. As par of the review exercise, I started writing some unit tests, but I don't want to block your code from master any longer.

---

src/responder/ifp/ifp_domains.c | 348 ++++++++++++++++++++++++++ src/responder/ifp/ifp_domains.h | 60 +++++ src/responder/ifp/ifp_iface.xml | 19 ++ src/responder/ifp/ifp_iface_generated.c | 415 ++++++++++++++++++++++++++++++++ src/responder/ifp/ifp_iface_generated.h | 39 +++ src/responder/ifp/ifpsrv.c | 19 ++ 6 files changed, 900 insertions(+)

diff --git a/src/responder/ifp/ifp_domains.c b/src/responder/ifp/ifp_domains.c index a06ba471c43ac7111693aa0d22a14cf33d168caa..ad0c4132711d21a009c2abc9e21a417144936bcd 100644 --- a/src/responder/ifp/ifp_domains.c +++ b/src/responder/ifp/ifp_domains.c @@ -20,6 +20,8 @@ */

#include <talloc.h> +#include <tevent.h> +#include <string.h>

#include "db/sysdb.h" #include "util/util.h" @@ -232,3 +234,349 @@ static void ifp_find_domain_by_name_process(struct tevent_req *req) DEBUG(SSSDBG_OP_FAILURE, "Could not finish request!\n"); } }

+static struct sss_domain_info * +get_domain_info_from_req(struct sbus_request *req, void *data)

I would prefer a different name than 'req' for sbus_request. 'req' is used through the code as a tevent_req instance.

+{

• struct ifp_ctx *ctx = NULL;
• struct sss_domain_info *domains = NULL;
• struct sss_domain_info *iter = NULL;
• const char *path = dbus_message_get_path(req->message);
• const char *safe_name = NULL;
• char *name = NULL;
• ctx = talloc_get_type(data, struct ifp_ctx);
• if (ctx == NULL) {

•    DEBUG(SSSDBG_CRIT_FAILURE, "Invalid pointer!\n");
  

•    return NULL;
  

• }
• safe_name = ifp_path_strip_prefix(path, INFOPIPE_DOMAIN_PATH_PFX "/");

I think raw_name would be a better name. I would use safe_name for something that was unsafe and was escaped to be safe.

• if (safe_name == NULL) {

•    return NULL;
  

• }
• name = ifp_bus_path_unescape(NULL, safe_name);

Can you use sbus_req as the parent context here? I know you free the memory in this function, but allocating on the request would be future-proof when we refactor this code..

• if (name == NULL) {

•    return NULL;
  

• }
• DEBUG(SSSDBG_TRACE_INTERNAL, "Looking for domain %s\n", name);
• domains = ctx->rctx->domains;
• for (iter = domains; iter != NULL; iter = get_next_domain(iter, true)) {

•    if (strcasecmp(iter->name, name) == 0) {
  

•        break;
  

•    }
  

• }
• talloc_free(name);
• return iter;

+}

+static void get_server_list(struct sbus_request *dbus_req,

•                        void *data,
  

•                        const char ***_out,
  

•                        int *_out_len,
  

•                        bool backup)
  

+{

• static const char *srv[] = {"_srv_"};
• struct sss_domain_info *dom = NULL;
• struct ifp_ctx *ctx = NULL;
• const char *conf_path = NULL;
• const char *option = NULL;
• const char **out = NULL;
• char **servers = NULL;
• int num_servers;
• errno_t ret;
• int i;
• *_out = NULL;
• *_out_len = 0;
• dom = get_domain_info_from_req(dbus_req, data);
• if (dom == NULL) {

•    return;
  

• }
• if (dom->parent != NULL) {

•    /* subdomains are not present in configuration */
  

•    ret = ENOENT;
  

•    goto done;
  

• }
• ctx = talloc_get_type(data, struct ifp_ctx);
• if (ctx == NULL) {

•    DEBUG(SSSDBG_CRIT_FAILURE, "Invalid ifp context!\n");
  

•    ret = ENOMEM;
  

•    goto done;
  

• }
• conf_path = talloc_asprintf(dbus_req, CONFDB_DOMAIN_PATH_TMPL, dom->name);
• if (conf_path == NULL) {

•    ret = ENOMEM;
  

•    goto done;
  

• }
• /* TODO: replace hardcoded values with option names from the provider */

I don't like this at all, but I can't see another way with the current code. Maybe we should add a new ID target for getting info about domains? I think the performance would be a non-issue since all that would be needed is an sbus round trip to the DP that would just reply right away. But any DP changes should be discussed on the list first -- for now, just please file a ticket.

• if (strcasecmp(dom->provider, "ldap") == 0) {

•    option = backup == false ? "ldap_uri" : "ldap_backup_uri";
  

• } else if (strcasecmp(dom->provider, "ipa") == 0) {

•    option = backup == false ? "ipa_server" : "ipa_backup_server";
  

• } else if (strcasecmp(dom->provider, "ad") == 0) {

•    option = backup == false ? "ad_server" : "ad_backup_server";
  

• } else {

•    ret = EINVAL;
  

•    goto done;
  

• }
• ret = confdb_get_string_as_list(ctx->rctx->cdb, dbus_req, conf_path,

•                                option, &servers);
  

• if (ret != EOK) {

•    goto done;
  

• }
• for (num_servers = 0; servers[num_servers] != NULL; num_servers++);
• if (num_servers == 0) {

•    ret = ENOENT;
  

•    goto done;
  

• }
• out = talloc_zero_array(dbus_req, const char*, num_servers);
• if (out == NULL) {

•    ret = ENOMEM;
  

•    goto done;
  

• }
• for (i = 0; i < num_servers; i++) {

•    out[i] = talloc_steal(out, servers[i]);
  

• }
• *_out = out;
• *_out_len = num_servers;
• ret = EOK;

+done:

• if (ret == ENOENT) {

•    *_out = srv;
  

•    *_out_len = 1;
  

• }
• return;

+}

+void ifp_dom_get_name(struct sbus_request *dbus_req,

•                  void *data,
  

•                  const char **_out)
  

+{

• struct sss_domain_info *dom;
• *_out = NULL;
• dom = get_domain_info_from_req(dbus_req, data);
• if (dom == NULL) {

•    return;
  

• }
• *_out = dom->name;

+}

A general comment about the string getters -- could we have a macro that would wrap the repeated pattern? Maybe something like (pseudocode):

#define dom_get_str(dbus_req, data, _out, prop) \ struct sss_domain_info *dom; \ dom = get_domain_info_from_req(dbus_req, data); \ *_out = dom ? dom->prop : NULL;

Then you could reduce the getter to a single line: dom_get_str(dbus_req, data, _out, name);

[snip, the other getters generally look good to me]

+void ifp_dom_get_min_id(struct sbus_request *dbus_req,

•                    void *data,
  

•                    uint32_t *_out)
  

+{

• struct sss_domain_info *dom;
• *_out = 0;

The min_id default should be 1, I think. That's the default for min_id in confdb anyway. And our responders explicitly filter any request by ID for ID 0.

• dom = get_domain_info_from_req(dbus_req, data);
• if (dom == NULL) {

•    return;
  

• }
• *_out = dom->id_min;

+}

+void ifp_dom_get_max_id(struct sbus_request *dbus_req,

•                    void *data,
  

•                    uint32_t *_out)
  

+{

• struct sss_domain_info *dom;
• *_out = 0;

Here the default is OK, because 0 means 'unlimited'.

• dom = get_domain_info_from_req(dbus_req, data);
• if (dom == NULL) {

•    return;
  

• }
• *_out = dom->id_max;

+}

+void ifp_dom_get_realm(struct sbus_request *dbus_req,

•                   void *data,
  

•                   const char **_out)
  

+{

• struct sss_domain_info *dom;
• *_out = NULL;
• dom = get_domain_info_from_req(dbus_req, data);
• if (dom == NULL) {

•    return;
  

• }
• *_out = dom->realm;

+}

+void ifp_dom_get_forest(struct sbus_request *dbus_req,

•                    void *data,
  

•                    const char **_out)
  

+{

• struct sss_domain_info *dom;
• *_out = NULL;
• dom = get_domain_info_from_req(dbus_req, data);
• if (dom == NULL) {

•    return;
  

• }
• *_out = dom->forest;

+}

+void ifp_dom_get_login_format(struct sbus_request *dbus_req,

•                          void *data,
  

•                          const char **_out)
  

+{

• struct sss_domain_info *dom;
• *_out = NULL;
• dom = get_domain_info_from_req(dbus_req, data);
• if (dom == NULL) {

•    return;
  

• }
• *_out = dom->names->re_pattern;

+}

+void ifp_dom_get_fqdn_format(struct sbus_request *dbus_req,

•                         void *data,
  

•                         const char **_out)
  

+{

• struct sss_domain_info *dom;
• *_out = NULL;
• dom = get_domain_info_from_req(dbus_req, data);
• if (dom == NULL) {

•    return;
  

• }
• *_out = dom->names->fq_fmt;

+}

+void ifp_dom_get_enumerable(struct sbus_request *dbus_req,

•                        void *data,
  

•                        bool *_out)
  

+{

• struct sss_domain_info *dom;
• *_out = false;
• dom = get_domain_info_from_req(dbus_req, data);
• if (dom == NULL) {

•    return;
  

• }
• *_out = dom->enumerate;

+}

+void ifp_dom_get_use_fqdn(struct sbus_request *dbus_req,

•                      void *data,
  

•                      bool *_out)
  

+{

• struct sss_domain_info *dom;
• *_out = false;
• dom = get_domain_info_from_req(dbus_req, data);
• if (dom == NULL) {

•    return;
  

• }
• *_out = dom->fqnames;

+}

+void ifp_dom_get_subdomain(struct sbus_request *dbus_req,

•                       void *data,
  

•                       bool *_out)
  

+{

• struct sss_domain_info *dom;
• *_out = NULL;

Why are you setting a bool default to NULL? I guess you wanted to use false here?

• dom = get_domain_info_from_req(dbus_req, data);
• if (dom == NULL) {

•    return;
  

• }
• *_out = dom->parent ? true : false;

+}

+void ifp_dom_get_parent_domain(struct sbus_request *dbus_req,

•                          void *data,
  

•                          const char **_out)
  

+{

• struct sss_domain_info *dom;
• *_out = NULL;
• dom = get_domain_info_from_req(dbus_req, data);
• if (dom == NULL) {

•    return;
  

• }
• if (dom->parent == NULL) {

•    *_out = "/";
  

•    return;
  

• }
• *_out = ifp_reply_objpath(dbus_req, INFOPIPE_DOMAIN_PATH_PFX,

•                          dom->parent->name);
  

+}

The rest of the patch looks good to me.

I'll send review of other patches separately so you can work on addressing the issues in parallel.

On Mon, May 26, 2014 at 10:33:22PM +0200, Pavel Březina wrote:

On 05/26/2014 05:45 PM, Jakub Hrozek wrote:

...

On Thu, May 22, 2014 at 07:57:28PM +0200, Pavel Březina wrote:

...

On 05/21/2014 11:03 PM, Jakub Hrozek wrote:

...

Hi,

the attaches two patches implement DBus methods to retrieve SSSD domains using the system bus as well as two example getters that leverage the infrastructure to Get and GetAll the object properties.

Pavel, I know you have a large patchset in a private tree that implements the other getters and moves the domain methods into a separate file. I'm fine with merging your and mine patches, I just wanted to get the ball rolling on the list. Feel free to reply with your patchset.

Hi, see the attachment.

I squash all my patches to these five - mainly discarded the moving the code around.

I kept you as the author for the methods as I made only very minor changes there. However, I took the ownership for the properties since there only very little of your original code remains there - I hope you don't mind.

The next three patches implements the SSSD components interface required for the OpenLMI provider. I'm sending it to this thread since it depends on the domain patches anyway - there is enough dependency fuzz already.

...

From 7e55b23741ed749e95e9d33184b0fd702b86bb46 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek jhrozek@redhat.com Date: Tue, 22 Apr 2014 15:38:08 +0200 Subject: [PATCH 1/5] IFP: Add ListDomains and FindDomainByName

This one was acked by Pavel separately.

Hi, thank you for the review. I'm sending new patches.

...

...

• /* TODO: replace hardcoded values with option names from the provider */

I don't like this at all, but I can't see another way with the current

Me neither, thus todo.

...

code. Maybe we should add a new ID target for getting info about domains? I think the performance would be a non-issue since all that would be needed is an sbus round trip to the DP that would just reply right away. But any DP changes should be discussed on the list first -- for now, just please file a ticket.

My first idea was to do it as a part of future failover refactoring (to make the failover context per domain, not per backend). The idea was to provide a dbus interface for a failover that would retrieve a list of current servers, also with their status etc.

...

...

• if (strcasecmp(dom->provider, "ldap") == 0) {

•    option = backup == false ? "ldap_uri" : "ldap_backup_uri";
  

• } else if (strcasecmp(dom->provider, "ipa") == 0) {

•    option = backup == false ? "ipa_server" : "ipa_backup_server";
  

• } else if (strcasecmp(dom->provider, "ad") == 0) {

•    option = backup == false ? "ad_server" : "ad_backup_server";
  

• } else {

•    ret = EINVAL;
  

•    goto done;
  

• }

From 10e4b069bec51dab06be674cec4e881adda7bb4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Thu, 22 May 2014 19:01:35 +0200 Subject: [PATCH 1/4] IFP: Implement domain getters

---

src/responder/ifp/ifp_domains.c | 307 +++++++++++++++++++++++ src/responder/ifp/ifp_domains.h | 60 +++++ src/responder/ifp/ifp_iface.xml | 19 ++ src/responder/ifp/ifp_iface_generated.c | 415 ++++++++++++++++++++++++++++++++ src/responder/ifp/ifp_iface_generated.h | 39 +++ src/responder/ifp/ifpsrv.c | 19 ++ 6 files changed, 859 insertions(+)

diff --git a/src/responder/ifp/ifp_domains.c b/src/responder/ifp/ifp_domains.c index a06ba471c43ac7111693aa0d22a14cf33d168caa..5b34f96e9e6c4b45a1d775818fe15b328d335f18 100644 --- a/src/responder/ifp/ifp_domains.c +++ b/src/responder/ifp/ifp_domains.c @@ -20,6 +20,8 @@ */

#include <talloc.h> +#include <tevent.h> +#include <string.h>

#include "db/sysdb.h" #include "util/util.h" @@ -27,6 +29,19 @@ #include "responder/common/responder.h" #include "responder/ifp/ifp_domains.h"

+#define RETURN_DOM_PROP_AS_STRING(dbus_req, pvt_data, out, property) do { \

• struct sss_domain_info *dom; \

When you declare a variable inside a macro, it's better to give it some strange name to minimize shadowing other variables. Something like _pvt_dom maybe.

•                                                                      \
  

• *(out) = NULL; \

•                                                                      \
  

• dom = get_domain_info_from_req((dbus_req), (pvt_data)); \
• if (dom == NULL) { \

•    return;                                                           \
  

• } \

•                                                                      \
  

• *(out) = dom->realm; \

dom->property :-)

+} while (0)

The rest looks good to me. I'm still reviewing the latest patch, but I can't apply the one you sent previously to the thread, can you rebase (or rather regenerate the code, I think something had change in the last iteration of the GetAll patches that are luckily pushed now).

On Thu, May 22, 2014 at 07:57:28PM +0200, Pavel Březina wrote:

On 05/21/2014 11:03 PM, Jakub Hrozek wrote:

...

Hi,

the attaches two patches implement DBus methods to retrieve SSSD domains using the system bus as well as two example getters that leverage the infrastructure to Get and GetAll the object properties.

Pavel, I know you have a large patchset in a private tree that implements the other getters and moves the domain methods into a separate file. I'm fine with merging your and mine patches, I just wanted to get the ball rolling on the list. Feel free to reply with your patchset.

Hi, see the attachment.

I squash all my patches to these five - mainly discarded the moving the code around.

I kept you as the author for the methods as I made only very minor changes there. However, I took the ownership for the properties since there only very little of your original code remains there - I hope you don't mind.

The next three patches implements the SSSD components interface required for the OpenLMI provider. I'm sending it to this thread since it depends on the domain patches anyway - there is enough dependency fuzz already.

From a1a69124369eb50d37bc151802125b909fe7d1ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Tue, 6 May 2014 10:54:47 +0200 Subject: [PATCH 4/5] utils: add get_known_services()

---

src/monitor/monitor.c | 3 +-- src/util/util.c | 8 ++++++++ src/util/util.h | 2 ++ 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c index a164ede8a46f5b383c56a23ada693ba6d2ca2210..5e6eb37fdbf30c4079b309d98a1b05fdffdf65c7 100644 --- a/src/monitor/monitor.c +++ b/src/monitor/monitor.c @@ -888,8 +888,7 @@ done:

static char *check_services(char **services) {

• const char *known_services[] = { "nss", "pam", "sudo", "autofs", "ssh",

•                                 "pac", "ifp", NULL };
  

• const char * const *known_services = get_known_services(); int i; int ii;

diff --git a/src/util/util.c b/src/util/util.c index fc37aad229813f328c9af007a005569bb02c5091..ad93ca1a0715e3a5f5c6c7371eb142718c38afa3 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -806,3 +806,11 @@ bool check_ipv6_addr(struct in6_addr *addr, uint8_t flags)

 return true;

}

+const char * const * get_known_services(void) +{

• static const char *svc[] = {"nss", "pam", "sudo", "autofs",

•                            "ssh", "pac", "ifp", NULL };
  

• return svc;

+} diff --git a/src/util/util.h b/src/util/util.h index 54c6c6c78b87398da66e276d67a04f4558cec790..de7f9e553ef8f91df88fe7a71284538358e8798d 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -377,6 +377,8 @@ errno_t check_and_open_readonly(const char *filename, int *fd, bool check_ipv4_addr(struct in_addr *addr, uint8_t check); bool check_ipv6_addr(struct in6_addr *addr, uint8_t check);

+const char * const * get_known_services(void);

int split_on_separator(TALLOC_CTX *mem_ctx, const char *str, const char sep, bool trim, bool skip_empty, char ***_list, int *size); -- 1.7.11.7

ACK, see attached patch for what I tested with.