[SSSD] [PATCH] [HBAC]: Better libhbac debuging

Lukas Slebodnik lslebodn at redhat.com
Fri Sep 11 15:24:33 UTC 2015


On (01/09/15 16:15), Petr Cech wrote:
>On 09/01/2015 11:00 AM, Pavel Reichl wrote:
>>> From 6b1c6cac7123e78a2c55c51019b66a6bcf97ec29 Mon Sep 17 00:00:00 2001
>>>From: Petr Cech<pcech at redhat.com>
>>>Date: Fri, 24 Jul 2015 10:56:49 -0400
>>>Subject: [PATCH 2/2] HBAC: Better libhbac debugging
>>>
>>>Added support for logging via external log function.
>>>Log provides information about rules evaluating (HBAC_DBG_INFO level)
>>>and additionally can describe rules (HBAC_DBG_TRACE level).
>>>
>>>Resolves:
>>>https://fedorahosted.org/sssd/ticket/2703
>>>---
>>>  src/providers/ipa/hbac_evaluator.c | 152 +++++++++++++++++++++++++++++++++++++
>>>  src/providers/ipa/ipa_access.c     |  49 ++++++++++++
>>>  src/providers/ipa/ipa_hbac.exports |   3 +-
>>>  src/providers/ipa/ipa_hbac.h       |  22 ++++++
>>>  4 files changed, 225 insertions(+), 1 deletion(-)
>>>
>>>diff --git a/src/providers/ipa/hbac_evaluator.c b/src/providers/ipa/hbac_evaluator.c
>>>index f40f9e0a7f16f5e012079c637b89c8e49ec5d15b..6f236058a4a9711cf9bfba1db1447789bbb2d4b5 100644
>>>--- a/src/providers/ipa/hbac_evaluator.c
>>>+++ b/src/providers/ipa/hbac_evaluator.c
>>>@@ -38,6 +38,39 @@ typedef int errno_t;
>>>  #define EOK 0
>>>  #endif
>>>
>>>+/* HBAC logging system */
>>>+
>>>+/* debug macro */
>>>+#define HBAC_DEBUG(level, format, ...) do { \
>>>+    if (hbac_debug_fn != NULL) { \
>>>+        hbac_debug_fn(__FILE__, __LINE__, level, format, ##__VA_ARGS__); \
>>>+    } \
>>>+} while (0)
>>>+
>>>+/* static pointer to external logging function */
>>>+static hbac_debug_fn_t hbac_debug_fn = NULL;
>>>+
>>>+/* setup function for external logging function */
>>>+void hbac_enable_debug(hbac_debug_fn_t external_debug_fn)
>>>+{
>>>+    hbac_debug_fn = external_debug_fn;
>>>+}
>>>+
>>>+/* auxiliary function for hbac_request_element logging */
>>>+static void hbac_request_element_debug_print(struct hbac_request_element *el,
>>>+                                             const char *label);
>>>+
>>>+/* auxiliary function for hbac_eval_req logging */
>>>+static void hbac_req_debug_print(struct hbac_eval_req *req);
>>>+
>>>+/* auxiliary function for hbac_rule_element logging */
>>>+static void hbac_rule_element_debug_print(struct hbac_rule_element *el,
>>>+                                          const char *label);
>>>+
>>>+/* auxiliary function for hbac_rule logging */
>>>+static void hbac_rule_debug_print(struct hbac_rule *rule);
>>>+
>>>+
>>>  /* Placeholder structure for future HBAC time-based
>>>   * evaluation rules
>>>   */
>>>@@ -114,9 +147,13 @@ enum hbac_eval_result hbac_evaluate(struct hbac_rule **rules,
>>>      enum hbac_eval_result result = HBAC_EVAL_DENY;
>>>      enum hbac_eval_result_int intermediate_result;
>>>
>>>+    HBAC_DEBUG(HBAC_DBG_INFO, "[< hbac_evaluate()\n");
>>>+    hbac_req_debug_print(hbac_req);
>>>+
>>>      if (info) {
>>>          *info = malloc(sizeof(struct hbac_info));
>>>          if (!*info) {
>>>+            HBAC_DEBUG(HBAC_DBG_ERROR, "Out of memory.\n");
>>>              return HBAC_EVAL_OOM;
>>>          }
>>>          (*info)->code = HBAC_ERROR_UNKNOWN;
>>>@@ -125,20 +162,25 @@ enum hbac_eval_result hbac_evaluate(struct hbac_rule **rules,
>>>      uint32_t i;
>>I know that you haven't changed this code, but could you move the
>>definition of i into for cycle or to he beginning of the block?
>Fixed.
>>>
>>>      for (i = 0; rules[i]; i++) {
>>>+        hbac_rule_debug_print(rules[i]);
>>>          intermediate_result = hbac_evaluate_rule(rules[i], hbac_req, &ret);
>>>          if (intermediate_result == HBAC_EVAL_UNMATCHED) {
>>>              /* This rule did not match at all. Skip it */
>>>+            HBAC_DEBUG(HBAC_DBG_INFO, "DISALLOWED by rule [%s].\n",
>>>+                       rules[i]->name);
>>I think this log message is wrong. Rule did not match - it was not
>>relevant for this user, host or service.  There are no deny rules. "The
>>rule [%s] did not match" is fine by me, do you agree?
>Fixed.
>>>              continue;
>>>          } else if (intermediate_result == HBAC_EVAL_MATCHED) {
>>>              /* This request matched an ALLOW rule
>>>               * Set the result to ALLOW but continue checking
>>>               * the other rules in case a DENY rule trumps it.
>>>               */
>>>+            HBAC_DEBUG(HBAC_DBG_INFO, "ALLOWED by rule [%s].\n", rules[i]->name);
>>>              result = HBAC_EVAL_ALLOW;
>>>              if (info) {
>>>                  (*info)->code = HBAC_SUCCESS;
>>>                  (*info)->rule_name = strdup(rules[i]->name);
>>>                  if (!(*info)->rule_name) {
>>>+                    HBAC_DEBUG(HBAC_DBG_ERROR, "Out of memory.\n");
>>>                      result = HBAC_EVAL_ERROR;
>>>                      (*info)->code = HBAC_ERROR_OUT_OF_MEMORY;
>>>                  }
>>>@@ -146,6 +188,9 @@ enum hbac_eval_result hbac_evaluate(struct hbac_rule **rules,
>>>              break;
>>>          } else {
>>>              /* An error occurred processing this rule */
>>>+            HBAC_DEBUG(HBAC_DBG_ERROR,
>>>+                       "Error occurred during evaluating of rule [%s].\n",
>>>+                       rules[i]->name);
>>>              result = HBAC_EVAL_ERROR;
>>>              if (info) {
>>>                  (*info)->code = ret;
>>>@@ -163,6 +208,7 @@ enum hbac_eval_result hbac_evaluate(struct hbac_rule **rules,
>>>       */
>>>  done:
>>>
>>>+    HBAC_DEBUG(HBAC_DBG_INFO, "hbac_evaluate() >]\n");
>>>      return result;
>>>  }
>>>
>>>@@ -333,3 +379,109 @@ const char *hbac_error_string(enum hbac_error_code code)
>>>          return "Unknown error code";
>>>      }
>>>  }
>>>+
>>>+static void hbac_request_element_debug_print(struct hbac_request_element *el,
>>>+                                             const char *label)
>>>+{
>>>+    if (el) {
>>>+        if (el->name) {
>>>+            HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t%s [%s]\n", label, el->name);
>>>+        }
>>add empty line
>Fixed.
>>>+        if (el->groups) {
>>>+            if (el->groups[0]) {
>>>+                HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t%s_group:\n", label);
>>>+                for (int i = 0; el->groups[i]; i++) {
>>>+                    HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t\t[%s]\n", el->groups[i]);
>>>+                }
>>>+            } else {
>>>+                HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t%s_group (none)\n", label);
>>>+            }
>>>+        }
>>>+    } else {
>>>+        HBAC_DEBUG(HBAC_DBG_TRACE, "\t%s (none)\n", label);
>>>+    }
>>>+}
>>>+
>>>+static void hbac_req_debug_print(struct hbac_eval_req *req)
>>>+{
>>>+    HBAC_DEBUG(HBAC_DBG_TRACE, "\tREQUEST:\n");
>>>+    if (req) {
>>>+        char time_buff[100];
>>IMO use const size_t size; or something like that here
>Fixed.
>>>+
>>>+        hbac_request_element_debug_print(req->service, "service");
>>>+        hbac_request_element_debug_print(req->user, "user");
>>>+        hbac_request_element_debug_print(req->targethost, "targethost");
>>>+        hbac_request_element_debug_print(req->srchost, "srchost");
>>>+        strftime(time_buff, sizeof time_buff, "%Y-%m-%d %H:%M:%S",
>>IMO use 'size' here. Please check return value of both strftime() and
>>localtime()
>Fixed.
>>>+                 localtime(&req->request_time));
>>>+        HBAC_DEBUG(HBAC_DBG_TRACE, "\t\trequest time %s\n", time_buff);
>>>+    } else {
>>>+        HBAC_DEBUG(HBAC_DBG_TRACE, "\tRequest is EMPTY.\n");
>>>+    }
>>>+}
>>>+
>>>+static void hbac_rule_element_debug_print(struct hbac_rule_element *el,
>>>+                                          const char *label)
>>>+{
>>>+    if (el) {
>>>+        HBAC_DEBUG(HBAC_DBG_TRACE, "\t\tcategory [%#x] [%s]\n", el->category,
>>>+                   (el->category == HBAC_CATEGORY_ALL) ? "ALL" : "NONE");
>>>+
>>>+        if (el->names) {
>>>+            if (el->names[0]) {
>>>+                HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t%s_names:\n", label);
>>>+                for (int i = 0; el->names[i]; i++) {
>>>+                    HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t\t[%s]\n", el->names[i]);
>>>+                }
>>>+            } else {
>>>+                HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t%s_names (none)\n", label);
>>>+            }
>>>+        }
>>>+
>>>+        if (el->groups) {
>>>+            if (el->groups[0]) {
>>>+                HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t%s_groups:\n", label);
>>>+                for (int i = 0; el->groups[i]; i++) {
>>>+                    HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t\t[%s]\n", el->groups[i]);
>>>+                }
>>>+            } else {
>>>+                HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t%s_groups (none)\n", label);
>>>+            }
>>>+        }
>>>+    }
>>>+}
>>>+
>>>+static void hbac_rule_debug_print(struct hbac_rule *rule)
>>>+{
>>>+    if (rule) {
>>>+        HBAC_DEBUG(HBAC_DBG_TRACE, "\tRULE [%s] [%s]:\n",
>>>+                   rule->name, (rule->enabled) ? "ENABLED" : "DISABLED");
>>>+        if (rule->services) {
>>>+            HBAC_DEBUG(HBAC_DBG_TRACE, "\tservices:\n");
>>>+            hbac_rule_element_debug_print(rule->services, "services");
>>>+        } else {
>>>+            HBAC_DEBUG(HBAC_DBG_TRACE, "\tservices (none)\n");
>>>+        }
>>>+
>>>+        if (rule->users) {
>>>+            HBAC_DEBUG(HBAC_DBG_TRACE, "\tusers:\n");
>>>+            hbac_rule_element_debug_print(rule->users, "users");
>>>+        } else {
>>>+            HBAC_DEBUG(HBAC_DBG_TRACE, "\tusers (none)\n");
>>>+        }
>>>+
>>>+        if (rule->targethosts) {
>>>+            HBAC_DEBUG(HBAC_DBG_TRACE, "\ttargethosts:\n");
>>>+            hbac_rule_element_debug_print(rule->targethosts, "targethosts");
>>>+        } else {
>>>+            HBAC_DEBUG(HBAC_DBG_TRACE, "\ttargethosts (none)\n");
>>>+        }
>>>+
>>>+        if (rule->srchosts) {
>>>+            HBAC_DEBUG(HBAC_DBG_TRACE, "\tsrchosts:\n");
>>>+            hbac_rule_element_debug_print(rule->srchosts, "srchosts");
>>>+        } else {
>>>+            HBAC_DEBUG(HBAC_DBG_TRACE, "\tsrchosts (none)\n");
>>>+        }
>>>+    }
>>>+}
>>>diff --git a/src/providers/ipa/ipa_access.c b/src/providers/ipa/ipa_access.c
>>>index 3198e2bd2a4c8355eeccc129c85ae3d7d67f61b0..77b5ffcb8e4f7ac15ab9aa1cbc503594a7ee760b 100644
>>>--- a/src/providers/ipa/ipa_access.c
>>>+++ b/src/providers/ipa/ipa_access.c
>>>@@ -35,6 +35,53 @@
>>>  #include "providers/ipa/ipa_hbac_private.h"
>>>  #include "providers/ipa/ipa_hbac_rules.h"
>>>
>>>+/* External logging function for HBAC. */
>>>+void hbac_debug_messages(const char *file, int line,
>>>+                         enum hbac_debug_level level,
>>>+                         const char *fmt, ...)
>>>+{
>>>+    int loglevel = SSSDBG_UNRESOLVED;
>>Would you move this assignment to the default clause in the switch
>>statement?
>Fixed.
>>>+
>>>+    switch(level) {
>>>+    case HBAC_DBG_FATAL:
>>>+        loglevel = SSSDBG_FATAL_FAILURE;
>>>+        break;
>>>+    case HBAC_DBG_ERROR:
>>>+        loglevel = SSSDBG_OP_FAILURE;
>>>+        break;
>>>+    case HBAC_DBG_WARNING:
>>>+        loglevel = SSSDBG_MINOR_FAILURE;
>>>+        break;
>>>+    case HBAC_DBG_INFO:
>>>+        loglevel = SSSDBG_CONF_SETTINGS;
>>>+        break;
>>>+    case HBAC_DBG_TRACE:
>>>+        loglevel = SSSDBG_TRACE_INTERNAL;
>>>+        break;
>>>+    default:
>>>+        break;
>>>+    }
>>>+
>>>+    if (DEBUG_IS_SET(loglevel)) {
>>>+        va_list ap;
>>>+        char *message = NULL;
>>>+        int ret;
>>>+
>>>+        va_start(ap, fmt);
>>>+        ret = vasprintf(&message, fmt, ap);
>>>+        va_end(ap);
>>>+        if (ret < 0) {
>>>+            /* ENOMEM */
>>>+            free(message);
>>>+            return;
>>>+        }
>>>+
>>>+        debug_fn(__FILE__, __LINE__, "hbac", loglevel, "[%s:%i] %s",
>>>+                 file, line, message);
>>>+        free(message);
>>>+    }
>>>+}
>>>+
>>>  static void ipa_access_reply(struct hbac_ctx *hbac_ctx, int pam_status)
>>>  {
>>>      struct be_req *be_req = hbac_ctx->be_req;
>>>@@ -635,6 +682,8 @@ void ipa_hbac_evaluate_rules(struct hbac_ctx *hbac_ctx)
>>>          return;
>>>      }
>>>
>>>+    hbac_enable_debug(hbac_debug_messages);
>>>+
>>>      result = hbac_evaluate(hbac_rules, eval_req, &info);
>>>      if (result == HBAC_EVAL_ALLOW) {
>>>          DEBUG(SSSDBG_MINOR_FAILURE, "Access granted by HBAC rule [%s]\n",
>>>diff --git a/src/providers/ipa/ipa_hbac.exports b/src/providers/ipa/ipa_hbac.exports
>>>index 0115084e2b3a66569f97c4e7c035dffdb6450b43..63b6a5cd673d7b7f3096794648483d280a6bb47f 100644
>>>--- a/src/providers/ipa/ipa_hbac.exports
>>>+++ b/src/providers/ipa/ipa_hbac.exports
>>>@@ -1,4 +1,4 @@
>>>-IPA_HBAC_0.0.1 {
>>>+IPA_HBAC_0.0.2 {
>>>
>>>      # public functions
>>>      global:
>>>@@ -8,6 +8,7 @@ IPA_HBAC_0.0.1 {
>>>          hbac_error_string;
>>>          hbac_free_info;
>>>          hbac_rule_is_complete;
>>>+        hbac_enable_debug;
>>>
>>>      # everything else is local
>>>      local:
>>>diff --git a/src/providers/ipa/ipa_hbac.h b/src/providers/ipa/ipa_hbac.h
>>>index f43611351c8a5dfb20ca3d075f0bcd7bb71798c9..57947e7bb8509a8280e43259bbaf1b75599e0484 100644
>>>--- a/src/providers/ipa/ipa_hbac.h
>>>+++ b/src/providers/ipa/ipa_hbac.h
>>>@@ -41,6 +41,28 @@
>>>  #include <stdbool.h>
>>>  #include <time.h>
>>>
>>>+/** Debug levels for HBAC. */
>>>+enum hbac_debug_level {
>>>+    HBAC_DBG_FATAL,     /** Fatal failure (not used). */
>>>+    HBAC_DBG_ERROR,     /** Serious failure (out of memory, for example). */
>>>+    HBAC_DBG_WARNING,   /** Warnings (not used). */
>>>+    HBAC_DBG_INFO,      /** HBAC allow/disallow info. */
>>>+    HBAC_DBG_TRACE      /** Vesrbose description of rules. */
>>typo s/Vesrbose/Verbose/
>>Generally - why 2 asterisks at the beginning of the comment?  But as I
>>see we use it at hundreds of other places, so no need to change it.
>Fixed. 2 asterisks come from doxygen format.
>>>+};
>>>+
>>>+/**
>>>+ * Function pointer to HBAC external debugging function.
>>>+ */
>>>+typedef void (*hbac_debug_fn_t)(const char *file, int line,
>>>+                                enum hbac_debug_level, const char *format,
>>>+                                ...);
>>>+
>>>+/**
>>>+ *  HBAC uses external_debug_fn for logging messages.
>>>+ *  @param[in|out] external_debug_void Pointer to external logging function.
>>>+ */
>>>+void hbac_enable_debug(hbac_debug_fn_t external_debug_fn);
>>>+
>>>  /** Result of HBAC evaluation */
>>>  enum hbac_eval_result {
>>>      /** An error occurred
>>>-- 2.4.3
>Petr
>
>PS: I attached both patches. The 0001-TESTS-Fixing... was ACK in past.

>From d8d12580c3f1c047e51a4e149c9351bcf03ee0bd Mon Sep 17 00:00:00 2001
>From: Petr Cech <pcech at redhat.com>
>Date: Wed, 26 Aug 2015 02:50:26 -0400
>Subject: [PATCH 1/2] TESTS: Fixing of uninitialized pointer.
>
>There was a bug with uninitialized pointer during solving ticket 2703.
>
>More details:
>rules[0]->services->names[1] is initialized on line 361, but
>initializing of rules[0]->srchosts->names[1] was missing.
>
>Resolves:
>https://fedorahosted.org/sssd/ticket/2703
>---
> src/tests/ipa_hbac-tests.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/src/tests/ipa_hbac-tests.c b/src/tests/ipa_hbac-tests.c
>index bd56c8f107b05f07b1ba8913fc14a03419d679f7..f2192a6fbc5188a7a7f6b204e03ca5961bb53f75 100644
>--- a/src/tests/ipa_hbac-tests.c
>+++ b/src/tests/ipa_hbac-tests.c
>@@ -367,7 +367,7 @@ START_TEST(ipa_hbac_test_allow_utf8)
>     fail_if(rules[0]->services->names == NULL);
> 
>     rules[0]->srchosts->names[0] = (const char *) &srchost_utf8_upcase;
>-    rules[0]->services->names[1] = NULL;
>+    rules[0]->srchosts->names[1] = NULL;
> 
>     rules[1] = NULL;
> 
>-- 
>2.4.3
>

>From 5043aea0f1ce6968d3c06245205fa666f2f96ab9 Mon Sep 17 00:00:00 2001
>From: Petr Cech <pcech at redhat.com>
>Date: Fri, 24 Jul 2015 10:56:49 -0400
>Subject: [PATCH 2/2] HBAC: Better libhbac debugging
>
>Added support for logging via external log function.
>Log provides information about rules evaluating (HBAC_DBG_INFO level)
>and additionally can describe rules (HBAC_DBG_TRACE level).
>
>Resolves:
>https://fedorahosted.org/sssd/ticket/2703
>---
> src/providers/ipa/hbac_evaluator.c | 168 ++++++++++++++++++++++++++++++++++++-
> src/providers/ipa/ipa_access.c     |  50 +++++++++++
> src/providers/ipa/ipa_hbac.exports |   3 +-
> src/providers/ipa/ipa_hbac.h       |  22 +++++
> 4 files changed, 240 insertions(+), 3 deletions(-)
>
>diff --git a/src/providers/ipa/hbac_evaluator.c b/src/providers/ipa/hbac_evaluator.c
>index f40f9e0a7f16f5e012079c637b89c8e49ec5d15b..28d802bc1d3729eee98cfe66ea420043cbd9f40a 100644
>--- a/src/providers/ipa/hbac_evaluator.c
>+++ b/src/providers/ipa/hbac_evaluator.c
>@@ -38,6 +38,39 @@ typedef int errno_t;
> #define EOK 0
> #endif
> 
>+/* HBAC logging system */
>+
>+/* debug macro */
>+#define HBAC_DEBUG(level, format, ...) do { \
>+    if (hbac_debug_fn != NULL) { \
>+        hbac_debug_fn(__FILE__, __LINE__, level, format, ##__VA_ARGS__); \
>+    } \
>+} while (0)
>+
>+/* static pointer to external logging function */
>+static hbac_debug_fn_t hbac_debug_fn = NULL;
>+
>+/* setup function for external logging function */
>+void hbac_enable_debug(hbac_debug_fn_t external_debug_fn)
>+{
>+    hbac_debug_fn = external_debug_fn;
>+}
>+
>+/* auxiliary function for hbac_request_element logging */
>+static void hbac_request_element_debug_print(struct hbac_request_element *el,
>+                                             const char *label);
>+
>+/* auxiliary function for hbac_eval_req logging */
>+static void hbac_req_debug_print(struct hbac_eval_req *req);
>+
>+/* auxiliary function for hbac_rule_element logging */
>+static void hbac_rule_element_debug_print(struct hbac_rule_element *el,
>+                                          const char *label);
>+
>+/* auxiliary function for hbac_rule logging */
>+static void hbac_rule_debug_print(struct hbac_rule *rule);
>+
>+
> /* Placeholder structure for future HBAC time-based
>  * evaluation rules
>  */
>@@ -114,31 +147,39 @@ enum hbac_eval_result hbac_evaluate(struct hbac_rule **rules,
>     enum hbac_eval_result result = HBAC_EVAL_DENY;
>     enum hbac_eval_result_int intermediate_result;
> 
>+    HBAC_DEBUG(HBAC_DBG_INFO, "[< hbac_evaluate()\n");
>+    hbac_req_debug_print(hbac_req);
>+
>     if (info) {
>         *info = malloc(sizeof(struct hbac_info));
>         if (!*info) {
>+            HBAC_DEBUG(HBAC_DBG_ERROR, "Out of memory.\n");
>             return HBAC_EVAL_OOM;
>         }
>         (*info)->code = HBAC_ERROR_UNKNOWN;
>         (*info)->rule_name = NULL;
>     }
>-    uint32_t i;
> 
>-    for (i = 0; rules[i]; i++) {
>+    for (uint32_t i = 0; rules[i]; i++) {
>+        hbac_rule_debug_print(rules[i]);
>         intermediate_result = hbac_evaluate_rule(rules[i], hbac_req, &ret);
>         if (intermediate_result == HBAC_EVAL_UNMATCHED) {
>             /* This rule did not match at all. Skip it */
>+            HBAC_DEBUG(HBAC_DBG_INFO, "The rule [%s] did not match.\n",
>+                       rules[i]->name);
>             continue;
>         } else if (intermediate_result == HBAC_EVAL_MATCHED) {
>             /* This request matched an ALLOW rule
>              * Set the result to ALLOW but continue checking
>              * the other rules in case a DENY rule trumps it.
>              */
>+            HBAC_DEBUG(HBAC_DBG_INFO, "ALLOWED by rule [%s].\n", rules[i]->name);
>             result = HBAC_EVAL_ALLOW;
>             if (info) {
>                 (*info)->code = HBAC_SUCCESS;
>                 (*info)->rule_name = strdup(rules[i]->name);
>                 if (!(*info)->rule_name) {
>+                    HBAC_DEBUG(HBAC_DBG_ERROR, "Out of memory.\n");
>                     result = HBAC_EVAL_ERROR;
>                     (*info)->code = HBAC_ERROR_OUT_OF_MEMORY;
>                 }
>@@ -146,6 +187,9 @@ enum hbac_eval_result hbac_evaluate(struct hbac_rule **rules,
>             break;
>         } else {
>             /* An error occurred processing this rule */
>+            HBAC_DEBUG(HBAC_DBG_ERROR,
>+                       "Error occurred during evaluating of rule [%s].\n",
>+                       rules[i]->name);
>             result = HBAC_EVAL_ERROR;
>             if (info) {
>                 (*info)->code = ret;
>@@ -163,6 +207,7 @@ enum hbac_eval_result hbac_evaluate(struct hbac_rule **rules,
>      */
> done:
> 
>+    HBAC_DEBUG(HBAC_DBG_INFO, "hbac_evaluate() >]\n");
>     return result;
> }
> 
>@@ -333,3 +378,122 @@ const char *hbac_error_string(enum hbac_error_code code)
>         return "Unknown error code";
>     }
> }
>+
>+static void hbac_request_element_debug_print(struct hbac_request_element *el,
>+                                             const char *label)
>+{
>+    if (el) {
>+        if (el->name) {
>+            HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t%s [%s]\n", label, el->name);
>+        }
>+
>+        if (el->groups) {
>+            if (el->groups[0]) {
>+                HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t%s_group:\n", label);
>+                for (int i = 0; el->groups[i]; i++) {
>+                    HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t\t[%s]\n", el->groups[i]);
>+                }
>+            } else {
>+                HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t%s_group (none)\n", label);
>+            }
>+        }
>+    } else {
>+        HBAC_DEBUG(HBAC_DBG_TRACE, "\t%s (none)\n", label);
>+    }
>+}
>+
>+static void hbac_req_debug_print(struct hbac_eval_req *req)
>+{
>+    HBAC_DEBUG(HBAC_DBG_TRACE, "\tREQUEST:\n");
>+    if (req) {
>+        struct tm *local_time = NULL;
>+        size_t ret;
>+        const size_t buff_size = 100;
>+        char time_buff[buff_size];
>+
>+        hbac_request_element_debug_print(req->service, "service");
>+        hbac_request_element_debug_print(req->user, "user");
>+        hbac_request_element_debug_print(req->targethost, "targethost");
>+        hbac_request_element_debug_print(req->srchost, "srchost");
>+
>+        local_time = localtime(&req->request_time);
>+        if (local_time == NULL) {
>+            return;
>+        }
>+
>+        ret = strftime(time_buff, buff_size, "%Y-%m-%d %H:%M:%S", local_time);
>+        if (ret <= 0) {
>+            return;
>+        }
>+
>+        HBAC_DEBUG(HBAC_DBG_TRACE, "\t\trequest time %s\n", time_buff);
>+    } else {
>+        HBAC_DEBUG(HBAC_DBG_TRACE, "\tRequest is EMPTY.\n");
>+    }
>+}
>+
>+static void hbac_rule_element_debug_print(struct hbac_rule_element *el,
>+                                          const char *label)
>+{
>+    if (el) {
>+        HBAC_DEBUG(HBAC_DBG_TRACE, "\t\tcategory [%#x] [%s]\n", el->category,
>+                   (el->category == HBAC_CATEGORY_ALL) ? "ALL" : "NONE");
>+
>+        if (el->names) {
>+            if (el->names[0]) {
>+                HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t%s_names:\n", label);
>+                for (int i = 0; el->names[i]; i++) {
>+                    HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t\t[%s]\n", el->names[i]);
>+                }
>+            } else {
>+                HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t%s_names (none)\n", label);
>+            }
>+        }
>+
>+        if (el->groups) {
>+            if (el->groups[0]) {
>+                HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t%s_groups:\n", label);
>+                for (int i = 0; el->groups[i]; i++) {
>+                    HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t\t[%s]\n", el->groups[i]);
>+                }
>+            } else {
>+                HBAC_DEBUG(HBAC_DBG_TRACE, "\t\t%s_groups (none)\n", label);
>+            }
>+        }
>+    }
>+}
>+
>+static void hbac_rule_debug_print(struct hbac_rule *rule)
>+{
>+    if (rule) {
>+        HBAC_DEBUG(HBAC_DBG_TRACE, "\tRULE [%s] [%s]:\n",
>+                   rule->name, (rule->enabled) ? "ENABLED" : "DISABLED");
>+        if (rule->services) {
>+            HBAC_DEBUG(HBAC_DBG_TRACE, "\tservices:\n");
>+            hbac_rule_element_debug_print(rule->services, "services");
>+        } else {
>+            HBAC_DEBUG(HBAC_DBG_TRACE, "\tservices (none)\n");
>+        }
>+
>+        if (rule->users) {
>+            HBAC_DEBUG(HBAC_DBG_TRACE, "\tusers:\n");
>+            hbac_rule_element_debug_print(rule->users, "users");
>+        } else {
>+            HBAC_DEBUG(HBAC_DBG_TRACE, "\tusers (none)\n");
>+        }
>+
>+        if (rule->targethosts) {
>+            HBAC_DEBUG(HBAC_DBG_TRACE, "\ttargethosts:\n");
>+            hbac_rule_element_debug_print(rule->targethosts, "targethosts");
>+        } else {
>+            HBAC_DEBUG(HBAC_DBG_TRACE, "\ttargethosts (none)\n");
>+        }
>+
>+        if (rule->srchosts) {
>+            HBAC_DEBUG(HBAC_DBG_TRACE, "\tsrchosts:\n");
>+            hbac_rule_element_debug_print(rule->srchosts, "srchosts");
>+        } else {
>+            HBAC_DEBUG(HBAC_DBG_TRACE, "\tsrchosts (none)\n");
>+        }
>+    }
>+}
>diff --git a/src/providers/ipa/ipa_access.c b/src/providers/ipa/ipa_access.c
>index 3198e2bd2a4c8355eeccc129c85ae3d7d67f61b0..65a791c3fe4b56d0f50d6e501a69d6d4b13f1d9a 100644
>--- a/src/providers/ipa/ipa_access.c
>+++ b/src/providers/ipa/ipa_access.c
>@@ -35,6 +35,54 @@
> #include "providers/ipa/ipa_hbac_private.h"
> #include "providers/ipa/ipa_hbac_rules.h"
> 
>+/* External logging function for HBAC. */
>+void hbac_debug_messages(const char *file, int line,
>+                         enum hbac_debug_level level,
>+                         const char *fmt, ...)
>+{
>+    int loglevel;
>+
>+    switch(level) {
>+    case HBAC_DBG_FATAL:
>+        loglevel = SSSDBG_FATAL_FAILURE;
>+        break;
>+    case HBAC_DBG_ERROR:
>+        loglevel = SSSDBG_OP_FAILURE;
>+        break;
>+    case HBAC_DBG_WARNING:
>+        loglevel = SSSDBG_MINOR_FAILURE;
>+        break;
>+    case HBAC_DBG_INFO:
>+        loglevel = SSSDBG_CONF_SETTINGS;
>+        break;
>+    case HBAC_DBG_TRACE:
>+        loglevel = SSSDBG_TRACE_INTERNAL;
>+        break;
>+    default:
>+        loglevel = SSSDBG_UNRESOLVED;
>+        break;
>+    }
>+
>+    if (DEBUG_IS_SET(loglevel)) {
>+        va_list ap;
>+        char *message = NULL;
>+        int ret;
>+
>+        va_start(ap, fmt);
>+        ret = vasprintf(&message, fmt, ap);
>+        va_end(ap);
>+        if (ret < 0) {
>+            /* ENOMEM */
>+            free(message);
>+            return;
>+        }
>+
>+        debug_fn(__FILE__, __LINE__, "hbac", loglevel, "[%s:%i] %s",
>+                 file, line, message);
>+        free(message);
>+    }
>+}
>+
> static void ipa_access_reply(struct hbac_ctx *hbac_ctx, int pam_status)
> {
>     struct be_req *be_req = hbac_ctx->be_req;
>@@ -635,6 +683,8 @@ void ipa_hbac_evaluate_rules(struct hbac_ctx *hbac_ctx)
>         return;
>     }
> 
>+    hbac_enable_debug(hbac_debug_messages);
>+
>     result = hbac_evaluate(hbac_rules, eval_req, &info);
>     if (result == HBAC_EVAL_ALLOW) {
>         DEBUG(SSSDBG_MINOR_FAILURE, "Access granted by HBAC rule [%s]\n",
>diff --git a/src/providers/ipa/ipa_hbac.exports b/src/providers/ipa/ipa_hbac.exports
>index 0115084e2b3a66569f97c4e7c035dffdb6450b43..63b6a5cd673d7b7f3096794648483d280a6bb47f 100644
>--- a/src/providers/ipa/ipa_hbac.exports
>+++ b/src/providers/ipa/ipa_hbac.exports
>@@ -1,4 +1,4 @@
>-IPA_HBAC_0.0.1 {
>+IPA_HBAC_0.0.2 {
> 
>     # public functions
>     global:
>@@ -8,6 +8,7 @@ IPA_HBAC_0.0.1 {
>         hbac_error_string;
>         hbac_free_info;
>         hbac_rule_is_complete;
>+        hbac_enable_debug;

This change is not correct.
new functions should not be added to the the existing version
which was released.

You also forgot to update version-info for library.

@see more details about version script files in the thread which
introduced them to sssd
https://lists.fedorahosted.org/pipermail/sssd-devel/2014-July/019693.html

On Thu, Jun 26, 2014 at 10:31:27AM +0200, Lukas Slebodnik wrote:
> ehlo,
> 
> attached patch fixes ticket #2194.
> 
> If you wan to know more about version script (version maps) here are links:
> 
> http://people.redhat.com/drepper/dsohowto.pdf
>     (sections 2.2.5 .. 2.2.7, 3.4, 3.5)
> https://www.gnu.org/software/gnulib/manual/html_node/LD-Version-Scripts.html
> ftp://ftp.gnu.org/old-gnu/Manuals/ld-2.9.1/html_node/ld_25.html


BTW all these links were provided off the lists few weeks ago.
and for symplification attached is a patch which shoudl be squashed
to your 2nd patch :-)


LS
-------------- next part --------------
>From 4246d5cd91c4c34b8524be5bfce38c57163a6e2b Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Fri, 11 Sep 2015 17:04:58 +0200
Subject: [PATCH] squash_me

---
 Makefile.am                        | 2 +-
 src/providers/ipa/ipa_hbac.exports | 8 ++++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 851f943a4c57b70cee4f4f34e83457e7d204aff1..a2a868455f91fac212fcfa7b41681086145c06f9 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -928,7 +928,7 @@ libipa_hbac_la_LIBADD = \
     $(UNICODE_LIBS)
 libipa_hbac_la_LDFLAGS = \
     -Wl,--version-script,$(srcdir)/src/providers/ipa/ipa_hbac.exports \
-    -version-info 0:1:0
+    -version-info 1:0:1
 
 dist_noinst_DATA += src/providers/ipa/ipa_hbac.exports
 
diff --git a/src/providers/ipa/ipa_hbac.exports b/src/providers/ipa/ipa_hbac.exports
index 63b6a5cd673d7b7f3096794648483d280a6bb47f..b7945e139b9ab81b7c1d68eb707acaaff7163a2e 100644
--- a/src/providers/ipa/ipa_hbac.exports
+++ b/src/providers/ipa/ipa_hbac.exports
@@ -1,4 +1,4 @@
-IPA_HBAC_0.0.2 {
+IPA_HBAC_0.0.1 {
 
     # public functions
     global:
@@ -8,9 +8,13 @@ IPA_HBAC_0.0.2 {
         hbac_error_string;
         hbac_free_info;
         hbac_rule_is_complete;
-        hbac_enable_debug;
 
     # everything else is local
     local:
         *;
 };
+
+IPA_HBAC_0.1.0 {
+    global:
+        hbac_evaluate;
+} IPA_HBAC_0.0.1;
-- 
2.5.0



More information about the sssd-devel mailing list