On Wed, Oct 07, 2015 at 10:20:03AM +0200, Stanislav Laznicka wrote:
Hi,
Like I promised on FreeIPA mailing list, I am submitting my patches for your
kindly review. The code includes the evaluation of time policies in FreeIPA
HBAC rules. For better understanding of what these patches are all about,
please see the design page
http://www.freeipa.org/page/V4/Time-Based_Account_Policies.
Let me also share two links to the FreeIPA mailing list archive for
description of the major changes that followed my latest post on this
mailing list:
- the 'repeat' keyword to compensate iCalendar recurrence rules:
https://www.redhat.com/archives/freeipa-devel/2015-August/msg00451.html
- negative values:
https://www.redhat.com/archives/freeipa-devel/2015-October/msg00086.html
Hopefully, the code will meet your standards. To me, it is rather complex so
I tried adding as much comments as possible, which I hope will help you
better orient yourself in it.
Also, to help you better test it, there is a copr repo set up for this
project -
https://copr.fedoraproject.org/coprs/stlaz/freeipa-sssd-timerules/. You will
probably also need the mkosek/freeipa-master copr repo for some
dependencies.
Many thanks,
Standa
Hi,
thanks a lot for the patches. I didn't do a careful review yet, but I
wanted to start. So far I only read the diffs of the first two patches
and ran all the patches through static analysis.
I filed a tracking ticket for SSSD side of this feature:
https://fedorahosted.org/sssd/ticket/2836
and assigned it to you.
Alexander, I know you were involved in the upstream design of this feature,
are you interested in SSSD review as well? (conceptual review would be fine,
too...to make sure the patches match the design)
There are some Coverity issues in the patchset:
Error: FORWARD_NULL (CWE-476):
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:125: var_compare_op: Comparing
"rule->accesstimes" to null implies that "rule->accesstimes"
might be null.
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:171: var_deref_op: Dereferencing null
pointer "rule->accesstimes".
# 169| }
# 170|
# 171|-> if(!rule->accesstimes[0]) {
# 172| /* No match in exceptions and no accessTimes set -> the rule should
apply */
# 173| free(cmp_time);
Error: REVERSE_INULL (CWE-476):
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:171: deref_ptr: Directly dereferencing
pointer "rule->accesstimes".
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:178: check_after_deref: Null-checking
"rule->accesstimes" suggests that it may be null, but it has already been
dereferenced on all paths leading to the check.
# 176| }
# 177|
# 178|-> if(rule->accesstimes) {
# 179| for(i = 0; rule->accesstimes[i]; i++) {
# 180| ret = eval_time_rule(rule->accesstimes[i], cmp_time,
Error: RESOURCE_LEAK (CWE-772):
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:213: alloc_fn: Storage is returned from
allocation function "strdup".
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:213: var_assign: Assigning:
"env_backup" = storage returned from "strdup(tmp)".
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:220: leaked_storage: Variable
"env_backup" going out of scope leaks the storage it points to.
# 218|
# 219| if(setenv("TZ", tz_toenv, 1) != 0)
# 220|-> return NULL;
# 221|
# 222| tzset();
Error: RESOURCE_LEAK (CWE-772):
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:208: alloc_fn: Storage is returned from
allocation function "malloc".
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:208: var_assign: Assigning:
"tz_toenv" = storage returned from "malloc((tzlen + 2) * 1UL)".
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:217: noescape: Resource "tz_toenv +
1" is not freed or pointed-to in "memcpy". [Note: The source code
implementation of the function has been overridden by a builtin model.]
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:219: noescape: Resource
"tz_toenv" is not freed or pointed-to in "setenv".
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:220: leaked_storage: Variable
"tz_toenv" going out of scope leaks the storage it points to.
# 218|
# 219| if(setenv("TZ", tz_toenv, 1) != 0)
# 220|-> return NULL;
# 221|
# 222| tzset();
Error: CHECKED_RETURN (CWE-252):
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:226: check_return: Calling
"setenv" without checking return value (as is done elsewhere 13 out of 15
times).
sssd-1.13.90/src/monitor/monitor.c:2314: example_assign: Example 1: Assigning:
"ret" = return value from "setenv("KRB5RCACHEDIR", rcachedir,
1)".
sssd-1.13.90/src/monitor/monitor.c:2315: example_checked: Example 1 (cont.):
"ret" has its value checked in "ret < 0".
sssd-1.13.90/src/providers/data_provider_be.c:2967: example_assign: Example 2: Assigning:
"ret" = return value from "setenv("_SSS_DOM", be_domain,
1)".
sssd-1.13.90/src/providers/data_provider_be.c:2968: example_checked: Example 2 (cont.):
"ret" has its value checked in "ret != 0".
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:1303: example_checked: Example 3:
"setenv("TZ", tz_toenv, 1)" has its value checked in
"setenv("TZ", tz_toenv, 1) != 0".
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:219: example_checked: Example 4:
"setenv("TZ", tz_toenv, 1)" has its value checked in
"setenv("TZ", tz_toenv, 1) != 0".
sssd-1.13.90/src/providers/krb5/krb5_common.c:77: example_assign: Example 5: Assigning:
"ret" = return value from "setenv(env_name, str, 1)".
sssd-1.13.90/src/providers/krb5/krb5_common.c:78: example_checked: Example 5 (cont.):
"ret" has its value checked in "ret != 0".
# 224|
# 225| if(env_backup != NULL)
# 226|-> setenv("TZ", env_backup, 1);
# 227| else unsetenv("TZ");
# 228|
Error: RESOURCE_LEAK (CWE-772):
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:213: alloc_fn: Storage is returned from
allocation function "strdup".
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:213: var_assign: Assigning:
"env_backup" = storage returned from "strdup(tmp)".
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:226: noescape: Resource
"env_backup" is not freed or pointed-to in "setenv".
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:232: leaked_storage: Variable
"env_backup" going out of scope leaks the storage it points to.
# 230| free(tz_toenv);
# 231|
# 232|-> return ret;
# 233| }
# 234|
Error: OVERRUN (CWE-119):
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:274: assignment: Assigning:
"prev" = "START".
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:546: overrun-local: Overrunning array
"found_flags" of 9 bytes at byte offset 9 using index "prev" (which
evaluates to 9).
# 544| if(*matched == true) {
# 545| *matched = false;
# 546|-> found_flags[prev] = true;
# 547| prev = tok.type;
# 548| }
Error: REVERSE_INULL (CWE-476):
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:1275: deref_ptr_in_call: Dereferencing
pointer "tz".
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:1285: check_after_deref: Null-checking
"tz" suggests that it may be null, but it has already been dereferenced on all
paths leading to the check.
# 1283|
# 1284| /* Timezone is either unset, set to UTC or set to a time zone name */
# 1285|-> if(!tz) {
# 1286| /* length of the 'utc' string */
# 1287| tzlen = 3;
Error: RESOURCE_LEAK (CWE-772):
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:1288: alloc_fn: Storage is returned from
allocation function "strdup".
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:1288: var_assign: Assigning: "tz"
= storage returned from "strdup("utc")".
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:1294: leaked_storage: Returning without
freeing "tz" leaks the storage that it points to.
# 1292| tz_toenv = malloc((tzlen + 2) * sizeof(char));
# 1293| if(tz_toenv == NULL)
# 1294|-> return ENOMEM;
# 1295| /* TZ variable is set */
# 1296| if((tmp = getenv("TZ")) != NULL)
Error: RESOURCE_LEAK (CWE-772):
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:1288: alloc_fn: Storage is returned from
allocation function "strdup".
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:1288: var_assign: Assigning: "tz"
= storage returned from "strdup("utc")".
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:1301: noescape: Resource "tz" is
not freed or pointed-to in "memcpy". [Note: The source code implementation of
the function has been overridden by a builtin model.]
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:1305: leaked_storage: Returning without
freeing "tz" leaks the storage that it points to.
# 1303| if(setenv("TZ", tz_toenv, 1) != 0) {
# 1304| free(tz_toenv);
# 1305|-> return ENOMEM;
# 1306| }
# 1307|
Error: RESOURCE_LEAK (CWE-772):
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:1297: alloc_fn: Storage is returned from
allocation function "strdup".
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:1297: var_assign: Assigning:
"env_backup" = storage returned from "strdup(tmp)".
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:1305: leaked_storage: Variable
"env_backup" going out of scope leaks the storage it points to.
# 1303| if(setenv("TZ", tz_toenv, 1) != 0) {
# 1304| free(tz_toenv);
# 1305|-> return ENOMEM;
# 1306| }
# 1307|
Error: CHECKED_RETURN (CWE-252):
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:1317: check_return: Calling
"setenv" without checking return value (as is done elsewhere 13 out of 15
times).
sssd-1.13.90/src/monitor/monitor.c:2314: example_assign: Example 1: Assigning:
"ret" = return value from "setenv("KRB5RCACHEDIR", rcachedir,
1)".
sssd-1.13.90/src/monitor/monitor.c:2315: example_checked: Example 1 (cont.):
"ret" has its value checked in "ret < 0".
sssd-1.13.90/src/providers/data_provider_be.c:2967: example_assign: Example 2: Assigning:
"ret" = return value from "setenv("_SSS_DOM", be_domain,
1)".
sssd-1.13.90/src/providers/data_provider_be.c:2968: example_checked: Example 2 (cont.):
"ret" has its value checked in "ret != 0".
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:1303: example_checked: Example 3:
"setenv("TZ", tz_toenv, 1)" has its value checked in
"setenv("TZ", tz_toenv, 1) != 0".
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:219: example_checked: Example 4:
"setenv("TZ", tz_toenv, 1)" has its value checked in
"setenv("TZ", tz_toenv, 1) != 0".
sssd-1.13.90/src/providers/krb5/krb5_common.c:77: example_assign: Example 5: Assigning:
"ret" = return value from "setenv(env_name, str, 1)".
sssd-1.13.90/src/providers/krb5/krb5_common.c:78: example_checked: Example 5 (cont.):
"ret" has its value checked in "ret != 0".
# 1315|
# 1316| if(env_backup != NULL)
# 1317|-> setenv("TZ", env_backup, 1);
# 1318| else unsetenv("TZ");
# 1319|
Error: RESOURCE_LEAK (CWE-772):
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:1288: alloc_fn: Storage is returned from
allocation function "strdup".
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:1288: var_assign: Assigning: "tz"
= storage returned from "strdup("utc")".
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:1301: noescape: Resource "tz" is
not freed or pointed-to in "memcpy". [Note: The source code implementation of
the function has been overridden by a builtin model.]
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:1323: leaked_storage: Returning without
freeing "tz" leaks the storage that it points to.
# 1321| free(tz_toenv);
# 1322|
# 1323|-> return EOK;
# 1324| }
Error: RESOURCE_LEAK (CWE-772):
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:1297: alloc_fn: Storage is returned from
allocation function "strdup".
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:1297: var_assign: Assigning:
"env_backup" = storage returned from "strdup(tmp)".
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:1317: noescape: Resource
"env_backup" is not freed or pointed-to in "setenv".
sssd-1.13.90/src/providers/ipa/ipa_timerules.c:1323: leaked_storage: Variable
"env_backup" going out of scope leaks the storage it points to.
# 1321| free(tz_toenv);
# 1322|
# 1323|-> return EOK;
# 1324| }
Error: NULL_RETURNS (CWE-476):
sssd-1.13.90/src/python/pyhbac.c:930: returned_null: "get_utf8_string" returns
null (checked 4 out of 5 times).
sssd-1.13.90/src/python/pyhbac.c:86:13: return_null: Explicitly returning null.
sssd-1.13.90/src/python/pyhbac.c:140: example_assign: Example 1: Assigning:
"utf_item" = return value from "get_utf8_string(item, p)".
sssd-1.13.90/src/python/pyhbac.c:141: example_checked: Example 1 (cont.):
"utf_item" has its value checked in "utf_item == NULL".
sssd-1.13.90/src/python/pyhbac.c:1741: example_assign: Example 2: Assigning:
"utf_name" = return value from "get_utf8_string(pyel->name,
"name")".
sssd-1.13.90/src/python/pyhbac.c:1742: example_checked: Example 2 (cont.):
"utf_name" has its value checked in "utf_name == NULL".
sssd-1.13.90/src/python/pyhbac.c:1109: example_assign: Example 3: Assigning:
"utf8_str" = return value from "get_utf8_string(enabled,
"enabled")".
sssd-1.13.90/src/python/pyhbac.c:1110: example_checked: Example 3 (cont.):
"utf8_str" has its value checked in "utf8_str".
sssd-1.13.90/src/python/pyhbac.c:1454: example_assign: Example 4: Assigning:
"utf_name" = return value from "get_utf8_string(pyrule->name,
"name")".
sssd-1.13.90/src/python/pyhbac.c:1455: example_checked: Example 4 (cont.):
"utf_name" has its value checked in "utf_name == NULL".
sssd-1.13.90/src/python/pyhbac.c:930: var_assigned: Assigning: "tmp" = null
return value from "get_utf8_string".
sssd-1.13.90/src/python/pyhbac.c:935: alias: Assigning: "_py_decref_tmp" =
"tmp". Both pointers are now null.
sssd-1.13.90/src/python/pyhbac.c:935: dereference: Dereferencing a null pointer
"_py_decref_tmp".
# 933| tr->exceptions = sequence_as_string_list(pyel->exceptions,
"exceptions");
# 934|
# 935|-> Py_DECREF(tmp);
# 936| if(!tr->timezone || !tr->accesstimes || !tr->exceptions) {
# 937| goto fail;
> From 56ca98abf7e785c86464e45a5386d25c47cb54b1 Mon Sep 17 00:00:00 2001
> From: Stanislav Laznicka <slaznick(a)redhat.com>
> Date: Mon, 13 Jul 2015 09:53:10 +0200
> Subject: [PATCH 1/6] IPA: Added caching of time policies for HBAC rules
> The time policies which are part of the FreeIPA HBAC Rule
object
> are now being cached along with other HBAC Rule objects' attributes.
> Also, the cached time policies are transformed into hbac_time_rules
> structure representation.
>
https://fedorahosted.org/freeipa/ticket/547
> ---
> src/providers/ipa/hbac_evaluator.c | 20 +++---
> src/providers/ipa/ipa_access.c | 3 +
> src/providers/ipa/ipa_hbac.h | 14 ++++-
> src/providers/ipa/ipa_hbac_common.c | 119 +++++++++++++++++++++++++++++++++++
> src/providers/ipa/ipa_hbac_private.h | 10 +++
> src/providers/ipa/ipa_hbac_rules.c | 7 ++-
> src/tests/ipa_hbac-tests.c | 6 ++
> 7 files changed, 166 insertions(+), 13 deletions(-)
> diff --git a/src/providers/ipa/hbac_evaluator.c
b/src/providers/ipa/hbac_evaluator.c
> index
28d802bc1d3729eee98cfe66ea420043cbd9f40a..45c90cc9d46dc90254d47e89936fc4c62c164a8b 100644
> --- a/src/providers/ipa/hbac_evaluator.c
> +++ b/src/providers/ipa/hbac_evaluator.c
> @@ -70,14 +70,6 @@ static void hbac_rule_element_debug_print(struct hbac_rule_element
*el,
> /* 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
> - */
> -struct hbac_time_rules {
> - int not_yet_implemented;
> -};
> -
> enum hbac_eval_result_int {
> HBAC_EVAL_MATCH_ERROR = -1,
> HBAC_EVAL_MATCHED,
> @@ -100,6 +92,13 @@ static bool hbac_rule_element_is_complete(struct
hbac_rule_element *el)
> return false;
> }
>
> +static bool hbac_rule_timerules_elem_is_complete(struct hbac_time_rules *el) {
For functions we normally open the curly brace on a newline
+ if (el == NULL) return false;
+
+ /** Currently nothing has to be set */
+ return true;
+}
+
bool hbac_rule_is_complete(struct hbac_rule *rule, uint32_t *missing_attrs)
{
bool complete = true;
@@ -132,6 +131,11 @@ bool hbac_rule_is_complete(struct hbac_rule *rule, uint32_t
*missing_attrs)
*missing_attrs |= HBAC_RULE_ELEMENT_SOURCEHOSTS;
}
+ if (!hbac_rule_timerules_elem_is_complete(rule->timerules)) {
+ complete = false;
+ *missing_attrs |= HBAC_RULE_TIMERULES;
+ }
+
return complete;
}
diff --git a/src/providers/ipa/ipa_access.c b/src/providers/ipa/ipa_access.c
index 65a791c3fe4b56d0f50d6e501a69d6d4b13f1d9a..379860aa1b80d34c241446f6d442d216b5588033
100644
--- a/src/providers/ipa/ipa_access.c
+++ b/src/providers/ipa/ipa_access.c
@@ -736,6 +736,9 @@ errno_t hbac_get_cached_rules(TALLOC_CTX *mem_ctx,
IPA_EXTERNAL_HOST,
IPA_MEMBER_HOST,
IPA_HOST_CATEGORY,
+ IPA_TIMEZONE,
+ IPA_ACCESSTIME,
+ IPA_ACCESSTIME_EXCLUDE,
NULL };
tmp_ctx = talloc_new(NULL);
diff --git a/src/providers/ipa/ipa_hbac.h b/src/providers/ipa/ipa_hbac.h
index 9e85890e78c5cf0cda759f6132ab09290e637c1d..421d65dd9c4506a27ec92ec59dc9a96e7a04b738
100644
--- a/src/providers/ipa/ipa_hbac.h
+++ b/src/providers/ipa/ipa_hbac.h
@@ -95,7 +95,11 @@ enum hbac_eval_result {
/**
* Opaque type contained in hbac_evaluator.c
*/
-struct hbac_time_rules;
+ struct hbac_time_rules {
+ const char *timezone;
+ const char **accesstimes;
+ const char **exceptions;
+ };
/**
* Component of an HBAC rule
@@ -314,6 +318,9 @@ void hbac_free_info(struct hbac_info *info);
/** Source host element */
#define HBAC_RULE_ELEMENT_SOURCEHOSTS 0x08
+/** Time rules */
+#define HBAC_RULE_TIMERULES 0x10
+
/**
* @brief Evaluate whether an HBAC rule contains all necessary elements
*
@@ -322,8 +329,9 @@ void hbac_free_info(struct hbac_info *info);
* This is a bitmask that may contain one or more
* of #HBAC_RULE_ELEMENT_USERS,
* #HBAC_RULE_ELEMENT_SERVICES,
- * #HBAC_RULE_ELEMENT_TARGETHOSTS and
- * #HBAC_RULE_ELEMENT_SOURCEHOSTS
+ * #HBAC_RULE_ELEMENT_TARGETHOSTS,
+ * #HBAC_RULE_ELEMENT_SOURCEHOSTS and
+ * #HBAC_RULE_TIMERULES
*
* @return True if the rule contains all mandatory attributes
*
diff --git a/src/providers/ipa/ipa_hbac_common.c b/src/providers/ipa/ipa_hbac_common.c
index 72a620ef0971a8bc657bd7bda3f61b4abdd614ee..f90bd393b14d34c26c3efbe8ca0b0707c1251b65
100644
--- a/src/providers/ipa/ipa_hbac_common.c
+++ b/src/providers/ipa/ipa_hbac_common.c
@@ -354,6 +354,16 @@ hbac_attrs_to_rule(TALLOC_CTX *mem_ctx,
goto done;
}
+ ret = hbac_time_attrs_to_rule(new_rule, new_rule->name,
+ hbac_ctx->rules[idx],
+ &new_rule->timerules);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "Could not parse time rules for rule [%s]\n",
+ new_rule->name);
+ goto done;
+ }
+
*rule = new_rule;
ret = EOK;
@@ -363,6 +373,115 @@ done:
}
errno_t
+hbac_time_attrs_to_rule(TALLOC_CTX *mem_ctx,
+ const char *rule_name,
+ struct sysdb_attrs *rule_attrs,
+ struct hbac_time_rules **times)
As a convention, we start names of output parameters with an underscore:
struct hbac_time_rules **_times)
+{
+ errno_t ret;
+ TALLOC_CTX *tmp_ctx = NULL;
+ struct ldb_message_element *el;
+ const char *timezone;
+ struct hbac_time_rules *new_times = NULL;
+ int i;
+
+ tmp_ctx = talloc_new(mem_ctx);
+ if (tmp_ctx == NULL) return ENOMEM;
+
+ new_times = talloc_zero(tmp_ctx, struct hbac_time_rules);
+ if (new_times == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ DEBUG(SSSDBG_TRACE_LIBS, "Processing time policies for rule [%s]\n",
+ rule_name);
+
+ ret = sysdb_attrs_get_string(rule_attrs, IPA_TIMEZONE, &timezone);
+ if (ret != EOK && ret != ENOENT) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "sysdb_attrs_get_el failed.\n");
I agree we don't have a good explanation of the debug levels (and a lot
of our codebase was mass-converted from old decimal levels, so we're
not consistent either..), but I would prefer OP_FAILURE if some utility
function fails. Feel free to use CRIT_FAILURE in function that handles
hbac_time_attrs_to_rule.
+ goto done;
+ }
+ if (ret == ENOENT) {
+ /* No value = default behavior (utc) */
+ new_times->timezone = talloc_strdup(new_times, "utc");
I would prefer if utc was #defined somewhere with a comment stating that
it's the default.
+ }
+ else {
+ new_times->timezone = talloc_strdup(new_times,
+ timezone);
+ }
Normally we format the multi-branch if-else statements like this:
if (ret == ENOENT) {
/* Handle ENOENT */
} else if (ret != EOK) {
/* Handle ERROR */
} else {
/* The else/EOK branch can me omitted if it makes the code more
* readable and just continue in the main block */
}
Please also check the new_times->timezone pointer for NULL.
+
+ ret = sysdb_attrs_get_el(rule_attrs, IPA_ACCESSTIME, &el);
+ if(ret != EOK && ret != ENOENT) {
~~~~
(And elsewhere in the patchset)
Please put a space after keywords like if or for.
+ DEBUG(SSSDBG_CRIT_FAILURE, "sysdb_attrs_get_el
failed");
+ goto done;
+ }
+ if(ret == ENOENT || el->num_values == 0) {
Are you sure sysdb_attrs_get_el() can return ENOENT? IIRC it would
return element with zero values (so the second part of the condition is
correct..)
+ el->num_values = 0;
+ DEBUG(SSSDBG_CONF_SETTINGS,
+ "No access time specified.\n");
+ }
+
+ new_times->accesstimes = talloc_array(new_times,
+ const char *,
+ el->num_values + 1);
If you use talloc_zero_array, all the pointers will be initialized to
NULL and you won't have to bother with setting the sentinel..
> + if (new_times->accesstimes == NULL) {
> + ret = ENOMEM;
> + goto done;
> + }
> +
> + for(i = 0; i < el->num_values; i++) {
> + new_times->accesstimes[i] = talloc_strdup(new_times->accesstimes,
> + (const char *)el->values[i].data);
> + if(new_times->accesstimes[i] == NULL) {
> + ret = ENOMEM;
> + goto done;
> + }
> + }
> + new_times->accesstimes[i] = NULL;
> +
> + ret = sysdb_attrs_get_el(rule_attrs, IPA_ACCESSTIME_EXCLUDE, &el);
> + if(ret != EOK && ret != ENOENT) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "sysdb_attrs_get_el
failed");
+ goto done;
+ }
+ if(ret == ENOENT || el->num_values == 0) {
> + el->num_values
= 0;
> + DEBUG(SSSDBG_CONF_SETTINGS,
> + "No access time exceptions specified.\n");
> + }
> +
> + new_times->exceptions = talloc_array(new_times,
> + const char *,
> + el->num_values + 1);
> + if (new_times->exceptions == NULL) {
> + ret = ENOMEM;
> + goto done;
> + }
> +
> + for(i = 0; i < el->num_values; i++) {
> + new_times->exceptions[i] = talloc_strdup(new_times->exceptions,
> + (const char *)el->values[i].data);
> + if(new_times->exceptions[i] == NULL) {
> + ret = ENOMEM;
> + goto done;
> + }
> + }
> + new_times->exceptions[i] = NULL;
Looks like this is a copy of the block above, can you extract the
copying to a utility function? Or even better, use
sss_ldb_el_to_string_list().
+
+ ret = EOK;
+
+done:
+ if (ret == EOK) {
It's not really needed here, but to be on the safe side, we also tend to
check the validity of the output pointer. Since we also can't jump to
done with EOK, you can move the steal right before the done handler.
> + *times = talloc_steal(mem_ctx, new_times);
> + }
> + talloc_free(tmp_ctx);
> +
> + return ret;
> +}
> +
> +
> +errno_t
> hbac_get_category(struct sysdb_attrs *attrs,
> const char *category_attr,
> uint32_t *_categories)
> diff --git a/src/providers/ipa/ipa_hbac_private.h
b/src/providers/ipa/ipa_hbac_private.h
> index
c831cd5c6dd2ed1ff2bc0d649a25ae1212548dda..f36d4ee1ef78c20a5c52b375d07cd5a426fe2275 100644
> --- a/src/providers/ipa/ipa_hbac_private.h
> +++ b/src/providers/ipa/ipa_hbac_private.h
> @@ -53,6 +53,9 @@
> #define IPA_CN "cn"
> #define IPA_MEMBER_SERVICE "memberService"
> #define IPA_SERVICE_CATEGORY "serviceCategory"
> +#define IPA_TIMEZONE "ipaTimeZone"
> +#define IPA_ACCESSTIME "accessTime"
> +#define IPA_ACCESSTIME_EXCLUDE "accessTimeExclude"
> #define IPA_TRUE_VALUE "TRUE"
>
> #define IPA_HBAC_BASE_TMPL "cn=hbac,%s"
> @@ -129,6 +132,13 @@ hbac_service_attrs_to_rule(TALLOC_CTX *mem_ctx,
> const char *rule_name,
> struct sysdb_attrs *rule_attrs,
> struct hbac_rule_element **services);
> +
> +errno_t
> +hbac_time_attrs_to_rule(TALLOC_CTX *mem_ctx,
> + const char *rule_name,
> + struct sysdb_attrs *rule_attrs,
> + struct hbac_time_rules **times);
> +
> errno_t
> get_ipa_servicegroupname(TALLOC_CTX *mem_ctx,
> struct sysdb_ctx *sysdb,
> diff --git a/src/providers/ipa/ipa_hbac_rules.c b/src/providers/ipa/ipa_hbac_rules.c
> index
6ce1f76bb656352ec61332007a6fb62568374203..50514c5979d943bc854398fe7d03652afe9ae8cb 100644
> --- a/src/providers/ipa/ipa_hbac_rules.c
> +++ b/src/providers/ipa/ipa_hbac_rules.c
> @@ -94,7 +94,7 @@ ipa_hbac_rule_info_send(TALLOC_CTX *mem_ctx,
> state->opts = opts;
> state->search_bases = search_bases;
> state->search_base_iter = 0;
> - state->attrs = talloc_zero_array(state, const char *, 15);
> + state->attrs = talloc_zero_array(state, const char *, 18);
> if (state->attrs == NULL) {
> ret = ENOMEM;
> goto immediate;
> @@ -113,7 +113,10 @@ ipa_hbac_rule_info_send(TALLOC_CTX *mem_ctx,
> state->attrs[11] = IPA_EXTERNAL_HOST;
> state->attrs[12] = IPA_MEMBER_HOST;
> state->attrs[13] = IPA_HOST_CATEGORY;
> - state->attrs[14] = NULL;
> + state->attrs[14] = IPA_TIMEZONE;
> + state->attrs[15] = IPA_ACCESSTIME;
> + state->attrs[16] = IPA_ACCESSTIME_EXCLUDE;
> + state->attrs[17] = NULL;
>
> rule_filter = talloc_asprintf(tmp_ctx,
> "(&(objectclass=%s)"
> diff --git a/src/tests/ipa_hbac-tests.c b/src/tests/ipa_hbac-tests.c
> index
f2192a6fbc5188a7a7f6b204e03ca5961bb53f75..cf4788ac23c24b05b0085f2266029d5ded0b57c0 100644
> --- a/src/tests/ipa_hbac-tests.c
> +++ b/src/tests/ipa_hbac-tests.c
> @@ -103,6 +103,12 @@ static void get_allow_all_rule(TALLOC_CTX *mem_ctx,
> rule->srchosts->names = NULL;
> rule->srchosts->groups = NULL;
>
> + rule->timerules = talloc_zero(rule, struct hbac_time_rules);
> + fail_if(rule->timerules == NULL);
> + rule->timerules->timezone = NULL;
> + rule->timerules->accesstimes = NULL;
> + rule->timerules->exceptions = NULL;
> +
> *allow_rule = rule;
> }
>
> --
> 2.4.3
> From 0a40a390afe56ec27c070ed743ba6f3d39b564aa Mon Sep 17 00:00:00 2001
> From: Stanislav Laznicka <slaznick(a)redhat.com>
> Date: Mon, 13 Jul 2015 10:00:42 +0200
> Subject: [PATCH 2/6] IPA: Added evaluation of time policies in HBAC
> The time policies in FreeIPA HBAC objects are now
evaluated in
> the libipa_hbac module.
> TODO: Had to disable the pyhbac tests until proper
python
> bindings are implemented.
>
https://fedorahosted.org/freeipa/ticket/547
It would be better to include the SSSD ticket in the commit message.
It would also be nice to add a link to the design page into the commit
message.
You added new functions to the API, so the version info of libipa_hbac
must be incremented:
https://www.gnu.org/software/libtool/manual/libtool.html#Updating-version...
and the src/providers/ipa/ipa_hbac.exports must be amended as well.
> ---
> Makefile.am | 2 +
> src/providers/ipa/hbac_evaluator.c | 16 +-
> src/providers/ipa/ipa_timerules.c | 701 +++++++++++++++++++++++++++++++++++++
> src/providers/ipa/ipa_timerules.h | 40 +++
> src/tests/pyhbac-test.py | 32 +-
> 5 files changed, 774 insertions(+), 17 deletions(-)
> create mode 100644 src/providers/ipa/ipa_timerules.c
> create mode 100644 src/providers/ipa/ipa_timerules.h
> diff --git a/Makefile.am b/Makefile.am
> index
ce3166716eab20c7b98005faf938f32458dbd673..2196b672be770e307a86e97136ba48b5f92df3db 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -623,6 +623,7 @@ dist_noinst_HEADERS = \
> src/providers/ldap/sdap_dyndns.h \
> src/providers/ldap/sdap_async_enum.h \
> src/providers/ipa/ipa_common.h \
> + src/providers/ipa/ipa_timerules.h \
> src/providers/ipa/ipa_config.h \
> src/providers/ipa/ipa_access.h \
> src/providers/ipa/ipa_selinux.h \
> @@ -927,6 +928,7 @@ lib_LTLIBRARIES = libipa_hbac.la \
> pkgconfig_DATA += src/providers/ipa/ipa_hbac.pc
> libipa_hbac_la_DEPENDENCIES = src/providers/ipa/ipa_hbac.exports
> libipa_hbac_la_SOURCES = \
> + src/providers/ipa/ipa_timerules.c \
> src/providers/ipa/hbac_evaluator.c \
> src/util/sss_utf8.c
> libipa_hbac_la_LIBADD = \
> diff --git a/src/providers/ipa/hbac_evaluator.c b/src/providers/ipa/hbac_evaluator.c
> index
45c90cc9d46dc90254d47e89936fc4c62c164a8b..db4a47e67b0ae2183edb1feb427d327c6bc11f9f 100644
> --- a/src/providers/ipa/hbac_evaluator.c
> +++ b/src/providers/ipa/hbac_evaluator.c
> @@ -27,6 +27,7 @@
> #include <string.h>
> #include <errno.h>
> #include "providers/ipa/ipa_hbac.h"
> +#include "providers/ipa/ipa_timerules.h"
> #include "util/sss_utf8.h"
>
> #ifndef HAVE_ERRNO_T
> @@ -232,7 +233,8 @@ enum hbac_eval_result_int hbac_evaluate_rule(struct hbac_rule
*rule,
> if (!rule->users
> || !rule->services
> || !rule->targethosts
> - || !rule->srchosts) {
> + || !rule->srchosts
> + || !rule->timerules) {
> *error = HBAC_ERROR_UNPARSEABLE_RULE;
> return HBAC_EVAL_MATCH_ERROR;
> }
> @@ -280,6 +282,18 @@ enum hbac_eval_result_int hbac_evaluate_rule(struct hbac_rule
*rule,
> } else if (!matched) {
> return HBAC_EVAL_UNMATCHED;
> }
> +
> + /* Check time policies */
> + ret = hbac_evaluate_time_rules(rule->timerules,
> + hbac_req->request_time,
> + &matched);
> + if(ret != EOK) {
> + *error = HBAC_ERROR_UNPARSEABLE_RULE;
> + return HBAC_EVAL_MATCH_ERROR;
> + } else if (!matched) {
> + return HBAC_EVAL_UNMATCHED;
> + }
> +
> return HBAC_EVAL_MATCHED;
> }
>
> diff --git a/src/providers/ipa/ipa_timerules.c b/src/providers/ipa/ipa_timerules.c
> new file mode 100644
> index
0000000000000000000000000000000000000000..44766c063b142dbc69bfac875a04ad948e6ae5f2
> --- /dev/null
> +++ b/src/providers/ipa/ipa_timerules.c
> @@ -0,0 +1,701 @@
> +/*
> + SSSD
> +
> + IPA Provider - Time Rules Parsing
> +
> + Authors:
> + Stanislav Laznicka <slaz(a)seznam.cz>
> +
> + Copyright (C) 2015 Red Hat
> +
> + This program is free software; you can redistribute it and/or modify
> + it under the terms of the GNU General Public License as published by
> + the Free Software Foundation; either version 3 of the License, or
> + (at your option) any later version.
> +
> + This program is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + GNU General Public License for more details.
> +
> + You should have received a copy of the GNU General Public License
> + along with this program. If not, see <
http://www.gnu.org/licenses/>.
> +*/
> +
> +#include <errno.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include "util/sss_utf8.h"
> +#include "providers/ipa/ipa_hbac_private.h"
> +#include "providers/ipa/ipa_hbac.h"
> +#include "providers/ipa/ipa_common.h"
> +
> +#define NORANGE -10001
> +
> +#define TOD_STR "timeofday"
> +#define DOW_STR "dayofweek"
> +#define DOM_STR "dayofmonth"
> +#define DOY_STR "dayofyear"
> +#define WOM_STR "weekofmonth"
> +#define WOY_STR "weekofyear"
> +#define MOY_STR "monthofyear"
> +#define YEAR_STR "year"
> +
> +#define isleap(y) ((((y % 4 == 0) && (y % 100 != 0)) || (y % 400 == 0)) ?
true : false)
> +
> +#define TOD_MAX 2359
> +#define DOW_MAX 7
> +#define DOM_MAX 31
> +#define DOY_MAX 366
> +#define WOM_MAX 5
> +#define WOY_MAX 53
> +#define MOY_MAX 12
> +#define YEAR_MAX 9999
> +#define YEAR_MIN 1970
> +
> +/* max length of number string - 4 + 1 for '\0' */
> +#define NUM_MAX_LEN 5
> +
> +typedef int errno_t;
> +
> +enum time_types {
> + TOD = 0,
> + DOW,
> + DOM,
> + DOY,
> + WOM,
> + WOY,
> + MOY,
> + YEAR,
> + START,
> + END,
> + VAL_ERR,
> + MEM_ERR
We already have HBAC_EVAL_OOM, can we use it?
+};
+
+struct time_token {
+ enum time_types type;
+ int value_low;
+ int value_high;
+ int pos;
+};
+
+errno_t
+eval_time_rule(const char *rule,
+ struct tm* cmp_time,
+ bool *matched);
+
+struct tm *time_to_timezone(time_t t, const char *tz);
These two functions should be static..
+
+errno_t
+hbac_evaluate_time_rules(struct hbac_time_rules *rule,
+ time_t req_time,
+ bool *matched)
+{
+ errno_t ret;
+ struct tm *cmp_time;
Can't we just use a structure on stack and avoid allocating it?
+ int i;
+
+ /* No access time policies = always match */
+ if(!(rule->accesstimes && rule->accesstimes[0]) &&
+ !(rule->exceptions && rule->exceptions[0])) {
+ *matched = true;
+ return EOK;
+ }
+
+ if(!rule->timezone || strcasecmp(rule->timezone, "utc") == 0)
+ cmp_time = gmtime(&req_time);
We prefer to use opening and closing curly braces for one-line
statements as well.
+ else if(strcasecmp(rule->timezone, "host") == 0)
+ cmp_time = localtime(&req_time);
+ else cmp_time = time_to_timezone(req_time, rule->timezone);
time_to_timezone returns a malloc-ed pointer, but nobody frees it.
In general I wonder if time_to_timezone just accepted a pointer to
struct tm and filled it?
+
+ if(cmp_time == NULL) return ENOMATCH;
+
+ /* conversion to our language */
+ if(cmp_time->tm_wday == 0)
+ cmp_time->tm_wday = 7;
+ cmp_time->tm_mon += 1;
+ cmp_time->tm_year += 1900;
+ cmp_time->tm_yday += 1;
+
+ if(rule->exceptions) {
+ for(i = 0; rule->exceptions[i]; i++) {
+ ret = eval_time_rule(rule->exceptions[i], cmp_time, matched);
+ if(ret != EOK) {
+ /*DEBUG(SSSDBG_CONF_SETTINGS,
+ "Invalid value found in accessTimeExclude rule [%s]\n",
+ rule->exceptions[i]);*/
In commit 65ce66c43141f7e5c8482a8f8e7e217a23791588 Petr added libhbac
debugging, you might want to use it.
+ *matched = false;
+ return ENOMATCH;
+ }
+ /* exception matched, rule can't be applied */
+ if(*matched == true) {
+ *matched = false;
+ return EOK;
+ }
+ }
+ }
+
+ if(!rule->accesstimes[0]) {
+ /* No match in exceptions and no accessTimes set -> the rule should apply */
+ *matched = true;
+ return EOK;
+ }
+
+ if(rule->accesstimes) {
+ for(i = 0; rule->accesstimes[i]; i++) {
+ ret = eval_time_rule(rule->accesstimes[i], cmp_time, matched);
+ if(ret != EOK) {
+ *matched = false;
+ return ENOMATCH;
+ }
+ else if(*matched == true) {
+ return EOK;
+ }
+ }
+ }
+ return ret;
+}
+
+struct tm *time_to_timezone(time_t t, const char *tz)
This function should be static.
+{
+ struct tm *ret;
+ char *env_backup = NULL;
+ char *tz_toenv;
+ char *tmp = NULL;
+ int tzlen;
+
+ tzlen = strlen(tz);
+ /* + 2 because - prepending string with ':'; end is a null sign */
+ tz_toenv = malloc((tzlen + 2) * sizeof(char));
+ if(tz_toenv == NULL)
+ return NULL;
+ /* TZ variable is set */
+ if((tmp = getenv("TZ")) != NULL)
+ env_backup = strdup(tmp);
+
+ tz_toenv[0] = ':';
+ tz_toenv[tzlen+1] = '\0';
+ memcpy(&tz_toenv[1], tz, tzlen*sizeof(char));
+
+ if(setenv("TZ", tz_toenv, 1) != 0)
+ return NULL;
Since libipa_hbac is only used by sssd_be which is a single-threaded process,
then so far we don't care, but should we stay on the safe side and guard
modifying the process environment with some mutexes to avoid issues with
multi-threaded applications?
Can you also use localtime_r instead?
(I really wish there was a version of localtime that would just accept
the TZ as parameter...)
+
+ tzset();
+ ret = localtime(&t);
+
+ if(env_backup != NULL)
+ setenv("TZ", env_backup, 1);
+ else unsetenv("TZ");
+ free(tz_toenv);
+
+ return ret;
+}
+
+void get_token(const char *input, struct time_token *tok);
+errno_t check_range(int val, int low, int high);
+int get_week_of_month(struct tm *t);
+int get_week_of_year(struct tm *t);
Make these functions static, please..
+
+errno_t
+eval_time_rule(const char *rule,
+ struct tm* cmp_time,
+ bool *matched)
I stopped here, I need more brainpower to review the rest :-) But I was
wondering if the function could be split into a couple of smaller
functions.