[Freeipa-devel] [RFC] Reload trust data in ipadb

[Simo Sorce]

On Fri, 2012-10-05 at 13:32 +0200, Sumit Bose wrote:
> From f8726fe1c4a2ab71ada1297003e3dbe6068e4207 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Fri, 5 Oct 2012 12:06:24 +0200
> Subject: [PATCH] ipadb: reload trust information if domain is not know
> 
> Currently the data about trusted domains is read once at startup. If a
> new trust is added the KDC must be restarted to know about the new
> trust. This patch reloads the trust data if there is a request from an
> unknown domain. To make DOS attacks a bit harder the data can be
> updated
> only once in a minute.
> ---
>  daemons/ipa-kdb/ipa_kdb_mspac.c | 44
> ++++++++++++++++++++++++++++++++---------
>  1 Datei geändert, 35 Zeilen hinzugefügt(+), 9 Zeilen entfernt(-)
> 
> diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c
> b/daemons/ipa-kdb/ipa_kdb_mspac.c
> index
> b5346fed1230d02a88c94ab913507112990a1651..f0bd3bfe913705abd44efb08325f54521533637e 100644
> --- a/daemons/ipa-kdb/ipa_kdb_mspac.c
> +++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
> @@ -40,6 +40,7 @@ struct ipadb_mspac {
>  
>      int num_trusts;
>      struct ipadb_adtrusts *trusts;
> +    time_t last_update;
>  };
>  
>  
> @@ -983,6 +984,8 @@ static struct ipadb_adtrusts
> *get_domain_from_realm(krb5_context context,
>      struct ipadb_context *ipactx;
>      struct ipadb_adtrusts *domain;
>      int i;
> +    krb5_error_code kerr;
> +    bool updated = false;

The name of this variable sounds backwards, should be called need_update
or similar.

>      ipactx = ipadb_get_context(context);
>      if (!ipactx) {
> @@ -993,17 +996,28 @@ static struct ipadb_adtrusts
> *get_domain_from_realm(krb5_context context,
>          return NULL;
>      }
>  
> -    for (i = 0; i < ipactx->mspac->num_trusts; i++) {
> -        domain = &ipactx->mspac->trusts[i];
> -        if (strlen(domain->domain_name) != realm.length) {
> -            continue;
> +    do {
> +        for (i = 0; i < ipactx->mspac->num_trusts; i++) {
> +            domain = &ipactx->mspac->trusts[i];
> +            if (strlen(domain->domain_name) != realm.length) {
> +                continue;
> +            }
> +            if (strncasecmp(domain->domain_name, realm.data,
> +                            realm.length) == 0) {
> +                return domain;
> +            }
>          }
> -        if (strncasecmp(domain->domain_name, realm.data,
> realm.length) == 0) {
> -            return domain;
> -        }
> -    }
>  
> -    return NULL;
> +        if (updated) {
> +            return NULL;
> +        } else {
> +            kerr = ipadb_reinit_mspac(ipactx);
> +            if (kerr != 0) {
> +                return NULL;
> +            }
> +            updated = true;
> +        }
> +    } while(1);
>  }

I do not think I like the do/while loop here, I would rather have
another function that wraps the current get_domain_from_realm() like:

static struct ipadb_adtrusts *get_domain_from_realm_update()
{
	dom = get_domain_from_realm();
	if (dom == NULL) {
		/* no dom, try to see if a new trust appeared */
		kerr = ipadb_reinit_mspac(ipactx);
		dom = get_domain_from_realm();
	}
	return dom;
}

This is clearer imo, and allows you to decide if you really want to
update or if you want to call the non-updating function later on, should
we need it.

>  static krb5_error_code filter_logon_info(krb5_context context,
> @@ -1550,6 +1564,16 @@ krb5_error_code ipadb_reinit_mspac(struct
> ipadb_context *ipactx)
>      struct dom_sid gsid;
>      char *resstr;
>      int ret;
> +    time_t now;
> +
> +    /* Do not update the mspac struct more than once a minute. This
> would
> +     * avoid heavy load on the directory server if there are lots of
> requests
> +     * from domains which we do not trust. */
> +    now = time(NULL);
> +    if (ipactx->mspac != NULL && now > ipactx->mspac->last_update &&
> +        (now - ipactx->mspac->last_update) < 60) {
> +        return 0;
> +    }
>  
>      /* clean up in case we had old values around */
>      ipadb_mspac_struct_free(&ipactx->mspac);
> @@ -1560,6 +1584,8 @@ krb5_error_code ipadb_reinit_mspac(struct
> ipadb_context *ipactx)
>          goto done;
>      }
>  
> +    ipactx->mspac->last_update = now;
> +
>      kerr = ipadb_simple_search(ipactx, ipactx->base,
> LDAP_SCOPE_SUBTREE,
>                                 "(objectclass=ipaNTDomainAttrs)",
> dom_attrs,
>                                  &result);

This part look fine, I wonder if we shouldn't make it even longer than 1
min.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York