[SSSD] [PATCHES][IPA][RFE] Implement iCal based time managment in HBAC

Jakub Hrozek jhrozek at redhat.com
Tue Oct 20 10:28:13 UTC 2015


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
> 

> From 0a40a390afe56ec27c070ed743ba6f3d39b564aa Mon Sep 17 00:00:00 2001
> From: Stanislav Laznicka <slaznick at 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
> ---
>  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 at 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
> +};
> +
> +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);
> +
> +errno_t
> +hbac_evaluate_time_rules(struct hbac_time_rules *rule,
> +                         time_t req_time,
> +                         bool *matched)
> +{
> +    errno_t ret;
> +    struct tm *cmp_time;
> +    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);
> +    else if(strcasecmp(rule->timezone, "host") == 0)
> +        cmp_time = localtime(&req_time);
> +    else cmp_time = time_to_timezone(req_time, rule->timezone);
> +
> +    if(cmp_time == NULL) return ENOMATCH;
> +
> +    /* conversion to our language */

So maybe this is a dumb question, but do we need to convert to our
language and not use the server's language? Any conversion adds
complexity and thus adds bugs :-)

> +    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]);*/
> +                *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)
> +{
> +    struct tm *ret;
> +    char *env_backup = NULL;
> +    char *tz_toenv;

Because we know that tz will be typically short, I think we can safely
use:
    char tz_toenv[strlen(tz) + 2];
and then fill the tz_toenv with snprintf, it will be much more readable,
plus you don't have to care about malloc..

> +    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;

So far this is the most problematic part to me, because here we're
changing the global timezone variable that affects the rest of the sssd.

So we need to add some mutex to make this critical section thread-safe
but also we need to guard against signals. I guess we should just block
them (those that we can) during this section and unblock when we revert
the TZ.

Alternatively, did you check if there is an existing implementation that
does something similar without having to set a global environment variable?

> +
> +    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);
> +
> +errno_t
> +eval_time_rule(const char *rule,
> +                 struct tm* cmp_time,
> +                 bool *matched)
> +{
> +    struct time_token tok;
> +    /*
> +     * array to store information about evaluation of each keyword
> +     *    false = Keyword not found (or found and is still being evaluated
> +     *          - if multiple ranges are set at this one attribute)
> +     *    true = Keyword found and the current time belongs to its range
> +     */
> +    bool found_flags[START] = { false };
> +    enum time_types prev;
> +    errno_t ret;
> +    int checked_val;
> +    char *rule_lower;
> +    int rule_it;
> +    int month_week;
> +    int year_week;
> +
> +    prev = START;
> +    tok.type = START;
> +    tok.pos = 0;
> +    *matched = true;
> +
> +    if((rule_lower = malloc((strlen(rule)+1)*sizeof(char))) == NULL)
> +        return ENOMEM;
> +
> +    for(rule_it = 0; rule[rule_it] != '\0'; rule_it++)
> +        rule_lower[rule_it] = tolower(rule[rule_it]);
> +    rule_lower[rule_it] = 0;
> +
> +    month_week = get_week_of_month(cmp_time);
> +    year_week = get_week_of_year(cmp_time);
> +    if(strstr(rule_lower, WOY_STR) != NULL) {
> +        /*
> +         * First week of year may appear in the previous year in case when
> +         * the 31st December is Mon-Wed.
> +         * Similarly, the 52nd and 53rd week of a year may apper in the following
> +         * year when the 1st January is Fri-Sun
> +         */
> +        if(year_week == 1 && cmp_time->tm_mon == 12)
> +            cmp_time->tm_year++;
> +        if(year_week > 51 && cmp_time->tm_mon == 1)
> +            cmp_time->tm_year--;
> +    }
> +
> +    do {
> +        get_token(rule_lower, &tok);
> +
> +        if(tok.type < START) {
> +            /* Check only, don't set (for cases of multiple values at a keyword) */
> +            if(found_flags[tok.type]) {
> +                return ENOMATCH;
> +            }
> +        }
> +
> +        switch(tok.type) {
> +            case TOD:

We need to reduce the amount of code duplication. What do you think
about a structure that would map the token type to a min/max constants?
Please see how we load options in sssd, that's a bit similar.

Then the code could be simplified to:
    tok_limits = tok_options[tok.type]
    if tok_value.low > tok_limits->value_low...

> +                if((tok.value_low % 100) >= 60 || tok.value_low > TOD_MAX) {

If there's any processing needed that would interfere with the generic
code, maybe it should be done in get_token or another specialized
process_token function.

> +                    ret = EINVAL;
> +                    goto finished;
> +                }
> +
> +                if(tok.value_high != NORANGE) {
> +                    if((tok.value_high % 100) >= 60 || tok.value_high > TOD_MAX) {
> +                        ret = EINVAL;
> +                        goto finished;
> +                    }
> +                }
> +
> +                checked_val = cmp_time->tm_hour*100 + cmp_time->tm_min;
> +
> +                break;
> +            case DOW:
> +                if(tok.value_low > DOW_MAX || tok.value_high > DOW_MAX) {
> +                    ret = EINVAL;
> +                    goto finished;
> +                }
> +
> +                if(tok.value_low == 0 || tok.value_high == 0) {
> +                    ret = EINVAL;
> +                    goto finished;
> +                }
> +
> +                checked_val = cmp_time->tm_wday;
> +
> +                break;
> +            case DOM:
> +                if(tok.value_low > DOM_MAX || tok.value_high > DOM_MAX) {
> +                    ret = EINVAL;
> +                    goto finished;
> +                }
> +                if(tok.value_low == 0 || tok.value_high == 0) {
> +                    ret = EINVAL;
> +                    goto finished;
> +                }
> +
> +                checked_val = cmp_time->tm_mday;
> +
> +                break;
> +            case DOY:
> +                if(tok.value_low > DOY_MAX || tok.value_high > DOY_MAX) {
> +                    ret = EINVAL;
> +                    goto finished;
> +                }
> +                if(tok.value_low == 0 || tok.value_high == 0) {
> +                    ret = EINVAL;
> +                    goto finished;
> +                }
> +
> +                checked_val = cmp_time->tm_yday;
> +
> +                break;
> +            case WOY:
> +                if(tok.value_low > WOY_MAX || tok.value_high > WOY_MAX) {
> +                    ret = EINVAL;
> +                    goto finished;
> +                }
> +                if(tok.value_low == 0 || tok.value_high == 0) {
> +                    ret = EINVAL;
> +                    goto finished;
> +                }
> +
> +                checked_val = year_week;
> +
> +                break;
> +            case WOM:
> +                if(tok.value_low > WOM_MAX || tok.value_high > WOM_MAX) {
> +                    ret = EINVAL;
> +                    goto finished;
> +                }
> +
> +                checked_val = month_week;
> +
> +                break;
> +            case MOY:
> +                if(tok.value_low > MOY_MAX || tok.value_high > MOY_MAX) {
> +                    ret = EINVAL;
> +                    goto finished;
> +                }
> +                if(tok.value_low == 0 || tok.value_high == 0) {
> +                    ret = EINVAL;
> +                    goto finished;
> +                }
> +
> +                checked_val = cmp_time->tm_mon;
> +
> +                break;
> +            case YEAR:
> +                if(tok.value_low < YEAR_MIN) {
> +                    ret = EINVAL;
> +                    goto finished;
> +                }
> +                if(tok.value_high != NORANGE && tok.value_high < YEAR_MIN) {
> +                    ret = EINVAL;
> +                    goto finished;
> +                }
> +
> +                checked_val = cmp_time->tm_year;
> +
> +                break;
> +            case END:
> +                ret = EOK;
> +                goto finished;
> +            case MEM_ERR:
> +                ret = ENOMEM;
> +                goto finished;
> +            default:
> +                ret = EINVAL;
> +                goto finished;
> +        }
> +
> +        ret = check_range(checked_val, tok.value_low, tok.value_high);
> +
> +        /* invalid range */
> +        if (ret == EINVAL)
> +            goto finished;
> +
> +        if(prev == tok.type) {
> +            /* if val in range, set matched to true, else keep matched same */
> +            if(ret == EOK)
> +                *matched = true;
> +        }
> +        else {
> +            /* a new attribute */
> +            if(*matched == true) {
> +                prev = tok.type;
> +                *matched = (ret == EOK);
> +                found_flags[prev] = true;
> +                prev = tok.type;
> +            }
> +            else {
> +                /* previous attribute did not match */
> +                ret = EOK;
> +                goto finished;
> +            }
> +        }
> +
> +    } while (tok.type < END);
> +
> +finished:
> +    free(rule_lower);
> +    return ret;
> +}
> +
> +int get_week_of_month(struct tm *t) {
> +    /* ISO 8601 does not specify how to count a week in a month. It however
> +     does specify how to count a week in a year. Such a week is at least
> +     4 days long and starts with Monday */

I admit I haven't read the ISO document, but how do these intervals work
for locales and timezones whose week starts on Sunday (US) ?

> +
> +    int sun; /* first sunday */
> +    int week = 0;
> +
> +    /* mod 7 turns mday into 0-6 interval, -curr_day gets a sunday */
> +    sun = (t->tm_mday % 7) - t->tm_wday;
> +    if(sun <= 0)
> +        sun +=7;
> +
> +    /* If first week has less than 4 days, it is a week 0*/
> +    if(sun < 4)
> +        week = 0;
> +    /* if sun == 7 then the final calculation needs week == 0 */
> +    else if(sun != 7) week = 1;
> +
> +    /*
> +     * The current month day number is converted into nearest following Sunday which
> +     * is the biggest day of week and decides the number of the current week.
> +     */
> +    return week + (t->tm_mday + (7 - t->tm_wday)) / 7;
> +}
> +
> +int get_weeks_of_year(int year, int yday, int wday);
> +int get_week_of_year(struct tm *t) {
> +    /* Week of year number according to ISO 8601 */
> +    int sun;         /* first Sunday of the year */
> +    int yr_days;     /* number of days in year/last Sunday*/
> +    int week_rest;
> +    int week = 0;
> +
> +    sun = (t->tm_yday % 7) - t->tm_wday;
> +    if(sun <= 0)
> +        sun+=7;
> +
> +    if(sun < 4) {
> +        if(t->tm_yday <= sun) {
> +            /*
> +             * this is the week before first sunday of the year, it has less
> +             * than 4 days and therefore is the last week of the previous year
> +             */
> +            return get_weeks_of_year(t->tm_year - 1, 7, /* 7 = Sunday */
> +                            (isleap(t->tm_year - 1) ? DOY_MAX : (DOY_MAX-1)) + (sun - 7));
> +        }
> +        week = 0;
> +    }
> +    /* if sun == 7 then the final calculation needs week == 0 */
> +    else if(sun != 7) week = 1;
> +
> +    /* count last sunday of the year */
> +    yr_days = isleap(t->tm_year) ? DOY_MAX : (DOY_MAX - 1);
> +    week_rest = (yr_days - sun) % 7;
> +    if(week_rest > 3) {
> +        /* there are at least 4 days in the last week */
> +        /* (7 - week_rest) is the number of days that extend to the next year */
> +        yr_days += (7 - week_rest);
> +    }
> +    else yr_days -= week_rest;
> +
> +    if(t->tm_yday > yr_days) {
> +        /* first week of the next year */
> +        return 1;
> +    }
> +
> +    /*
> +     * The current year day number is converted into nearest following Sunday which
> +     * is the biggest day of week and decides the number of the current week.
> +     */
> +    return week + (t->tm_yday + (7 - t->tm_wday)) / 7;
> +}
> +
> +int get_weeks_of_year(int year, int yday,  int wday) {
> +    int sun; /* first Sunday of the year */
> +    int thu; /* first Thursday of the year */
> +
> +    sun = (yday % 7) - wday;
> +    if(sun <= 0)
> +        sun += 7;
> +
> +    if(sun < 4)
> +        thu = sun + 4;
> +    else thu = sun - 3;
> +
> +    return 1 + ((isleap(year) ? DOY_MAX : (DOY_MAX - 1)) - thu) / 7;
> +}
> +
> +void get_token(const char *input, struct time_token *tok)
> +{
> +    int idx = tok->pos;
> +    int cmplen;
> +    char c;
> +    char *part = NULL;
> +    char *err = NULL;
> +    int i;
> +    int j;
> +
> +    while(((c = input[idx]) != '\0') && isspace(c))
> +        idx++;
> +
> +    if(c != ',') {
> +        switch(c) {
> +            case 't':
> +                cmplen = strlen(TOD_STR);

You can just use sizeof(TOD_STR)-1 and save some CPU cycles..

> +                if(strncmp(&input[idx], TOD_STR, cmplen) != 0) {
> +                    goto error;
> +                }
> +                tok->type=TOD;
> +                idx += cmplen;
> +                break;

There is a bit of code duplication in this function. At least 't' and
'm' and 'y' could use a generic function, maybe the function could be
written such that it can compare several tokens, with -1 as sentinel?

> +            case 'd':
> +                cmplen = strlen(DOW_STR);
> +                if(strncmp(&input[idx], DOW_STR, cmplen) != 0) {
> +                    cmplen = strlen(DOM_STR);
> +                    if(strncmp(&input[idx], DOM_STR, cmplen) != 0) {
> +                        cmplen = strlen(DOY_STR);
> +                        if(strncmp(&input[idx], DOY_STR, cmplen) != 0)
> +                            goto error;
> +                        tok->type = DOY;
> +                        idx += cmplen;
> +                        break;
> +                    }
> +                    tok->type = DOM;
> +                    idx += cmplen;
> +                    break;
> +                }
> +                tok->type = DOW;
> +                idx += cmplen;
> +                break;
> +            case 'w':
> +                cmplen = strlen(WOM_STR);
> +                if(strncmp(&input[idx], WOM_STR, cmplen) != 0) {
> +                    cmplen = strlen(WOY_STR);
> +                    if(strncmp(&input[idx], WOY_STR, cmplen) != 0)
> +                        goto error;
> +                    tok->type = WOY;
> +                    idx += cmplen;
> +                    break;
> +                }
> +                tok->type = WOM;
> +                idx += cmplen;
> +                break;
> +            case 'm':
> +                cmplen = strlen(MOY_STR);
> +                if(strncmp(&input[idx], MOY_STR, cmplen) != 0) {
> +                    goto error;
> +                }
> +                tok->type = MOY;
> +                idx += cmplen;
> +                break;
> +            case 'y':
> +                cmplen = strlen(YEAR_STR);
> +                if(strncmp(&input[idx], YEAR_STR, cmplen) != 0) {
> +                    goto error;
> +                }
> +                tok->type = YEAR;
> +                idx += cmplen;
> +                break;
> +            case '\0':
> +                tok->type = END;
> +                return;
> +            default:
> +                goto error;
> +        } /* switch(c) */
> +
> +        while(((c = input[idx]) != '\0') && isspace(c))
> +            idx++;
> +
> +        if(c != '=') {
> +            goto error;
> +        }
> +    } /* if (c != ',') */
> +    else {
> +        /* comma at the beginning of the string */
> +        if(tok->type == START) {
> +            goto error;
> +        }
> +    }
> +    idx++; /* either after '=' or ',' */
> +
> +    while(((c = input[idx]) != '\0') && isspace(c))
> +        idx++;
> +
> +    if((part = calloc(NUM_MAX_LEN, sizeof(char))) == NULL) {
> +        tok->type = MEM_ERR;
> +        return;
> +    }
> +
> +    for(j = 0; j < 2; j++) {
> +        switch(tok->type) {
> +            case TOD:
> +            case YEAR:
> +                for(i = 0; i < 4; i++) {
> +                    if(input[idx] == '\0' || !isdigit(input[idx])) {
> +                        goto error;
> +                    }
> +                    part[i] = input[idx];
> +                    idx++;
> +                }
> +                break;
> +            case DOW:
> +            case WOM:
> +                /* No need to check for \0, strtol will do that*/
> +                part[0] = input[idx];
> +                idx++;
> +                break;
> +            case DOM:
> +            case MOY:
> +            case WOY:
> +                for(i = 0; i < 2; i++) {
> +                    if(isdigit(input[idx])) {
> +                        part[i] = input[idx];
> +                        idx++;
> +                    }
> +                    else if(i == 0) /* not even a one-digit number */
> +                        goto error;
> +                }
> +                break;
> +            case DOY:
> +                for(i = 0; i < 3; i++) {
> +                    if(isdigit(input[idx])) {
> +                        part[i] = input[idx];
> +                        idx++;
> +                    }
> +                    else if(i == 0) /* zero digits in number */
> +                        goto error;

Again, code duplication...

> +                }
> +                break;
> +            default:
> +                goto error;
> +        }
> +
> +        if(j == 0) {
> +            tok->value_low = (int)strtol(part, &err, 10);
> +            if(*err != '\0') {
> +                goto error;
> +            }
> +            /* if it's not a range */
> +            if(input[idx] != '-') {
> +                tok->value_high = NORANGE;
> +                free(part);
> +                tok->pos = idx++;
> +                return;
> +            }
> +            else {
> +                idx++;
> +                memset(part, 0, sizeof(char) * NUM_MAX_LEN);
> +            }
> +        }
> +        else {
> +            tok->value_high = (int)strtol(part, &err, 10);
> +            if(*err != '\0') {
> +                goto error;
> +            }
> +        }
> +    } /* endfor */
> +
> +    tok->pos = idx;
> +    free(part);
> +    return;
> +
> +error:
> +    tok->type = VAL_ERR;
> +    if(part != NULL)
> +        free(part);
> +    return;
> +}
> +
> +errno_t check_range(int val, int low, int high)
> +{
> +    /* not a range */
> +    if (high == NORANGE) {
> +        if (val != low)
> +            return ERANGE;
> +        return EOK;
> +    }
> +    else if (high < 0) {
> +        return EINVAL;
> +    }
> +
> +    if(low < 0)
> +        return EINVAL;
> +
> +    if (low > high)
> +        return EINVAL;
> +
> +    if (val < low)
> +        return ERANGE;
> +
> +    if (val > high)
> +        return ERANGE;
> +    return EOK;
> +}
> diff --git a/src/providers/ipa/ipa_timerules.h b/src/providers/ipa/ipa_timerules.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..291c48ec73d1745032edea653ed02d7eb43d4bbe
> --- /dev/null
> +++ b/src/providers/ipa/ipa_timerules.h
> @@ -0,0 +1,40 @@
> +/*
> +    SSSD
> +
> +    IPA Provider - Time Rules Parsing
> +
> +    Authors:
> +        Stanislav Laznicka <slaz at 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/>.
> +*/
> +
> +#ifndef __IPA_TIMERULES_H_
> +#define __IPA_TIMERULES_H_
> +
> +#include "providers/ipa/ipa_hbac.h"
> +
> +#ifndef HAVE_ERRNO_T
> +#define HAVE_ERRNO_T
> +typedef int errno_t;
> +#endif
> +
> +errno_t
> +hbac_evaluate_time_rules(struct hbac_time_rules *rule,
> +                         time_t req_time,
> +                         bool *matched);

Why is the function not added to the existing libipa_hbac header file
instead?

> +
> +#endif /* __IPA_TIMERULES_H_ */
> diff --git a/src/tests/pyhbac-test.py b/src/tests/pyhbac-test.py
> index 9d8fd1a333bf54ecf21d14d3b6293f7294a0d53e..4ac864c4b124ef7d0946602fc874f2fc7209f77b 100755
> --- a/src/tests/pyhbac-test.py
> +++ b/src/tests/pyhbac-test.py
> @@ -518,25 +518,25 @@ if __name__ == "__main__":
>      if not res.wasSuccessful():
>          error |= 0x2
>  
> -    suite = unittest.TestLoader().loadTestsFromTestCase(PyHbacRuleTest)
> -    res = unittest.TextTestRunner().run(suite)
> -    if not res.wasSuccessful():
> -        error |= 0x3
> +#    suite = unittest.TestLoader().loadTestsFromTestCase(PyHbacRuleTest)
> +#    res = unittest.TextTestRunner().run(suite)
> +#    if not res.wasSuccessful():
> +#        error |= 0x3

What exactly breaks here? I would expect that if the rules don't have
any timerule information, the existing tests should just pass..

>  
> -    suite = unittest.TestLoader().loadTestsFromTestCase(PyHbacRequestElementTest)
> -    res = unittest.TextTestRunner().run(suite)
> -    if not res.wasSuccessful():
> -        error |= 0x4
> +#    suite = unittest.TestLoader().loadTestsFromTestCase(PyHbacRequestElementTest)
> +#    res = unittest.TextTestRunner().run(suite)
> +#    if not res.wasSuccessful():
> +#        error |= 0x4
>  
> -    suite = unittest.TestLoader().loadTestsFromTestCase(PyHbacRequestTest)
> -    res = unittest.TextTestRunner().run(suite)
> -    if not res.wasSuccessful():
> -        error |= 0x5
> +#    suite = unittest.TestLoader().loadTestsFromTestCase(PyHbacRequestTest)
> +#    res = unittest.TextTestRunner().run(suite)
> +#    if not res.wasSuccessful():
> +#        error |= 0x5
>  
> -    suite = unittest.TestLoader().loadTestsFromTestCase(PyHbacModuleTest)
> -    res = unittest.TextTestRunner().run(suite)
> -    if not res.wasSuccessful():
> -        error |= 0x6
> +#    suite = unittest.TestLoader().loadTestsFromTestCase(PyHbacModuleTest)
> +#    res = unittest.TextTestRunner().run(suite)
> +#    if not res.wasSuccessful():
> +#        error |= 0x6
>  
>      sys.exit(error)
>  
> -- 
> 2.4.3
> 

> From c0be84ac7cba4f0aef611cf449e2e0e293abd47b Mon Sep 17 00:00:00 2001
> From: Stanislav Laznicka <slaznick at redhat.com>
> Date: Wed, 15 Jul 2015 11:06:53 +0200
> Subject: [PATCH 3/6] IPA: Added Python bindings for HBAC time policies
> 
> https://fedorahosted.org/freeipa/ticket/547
> ---
>  src/python/pyhbac.c      | 389 +++++++++++++++++++++++++++++++++++++++++++++--
>  src/tests/pyhbac-test.py | 212 +++++++++++++++++++++++---

The tests fail after applying this patch (I review the patches applying
each in sequence)

> From 119dd0e09dd46ba90781f09fd96afc0f39e05b74 Mon Sep 17 00:00:00 2001
> From: Stanislav Laznicka <slaznick at redhat.com>
> Date: Fri, 21 Aug 2015 09:37:13 +0200
> Subject: [PATCH 4/6] IPA: Added the 'repeat' keyword functionality
> 
> Improvements for IPA provider - HBAC time policies:
> 
> The 'repeat' time rules language keyword is supposed to simulate
> the behavior of recurrence rules as seen in iCalendar.

Before I start reviewing the code, did you check if there is an existing
library we could use instead of writing the code ourselves?

...to be continued.


More information about the sssd-devel mailing list