[Freeipa-devel] [PATCH] 0100 Enumerate UPN suffixes in ipasam
Sumit Bose
sbose at redhat.com
Wed Mar 27 09:50:22 UTC 2013
On Mon, Mar 25, 2013 at 08:07:44PM +0200, Alexander Bokovoy wrote:
> Hi,
>
> following patch allows to enumerate UPN suffixes associated with IPA
> domain and make them available to AD domain we trust.
>
> The patch relies on PASSDB API expansion I'm working on and as such
> requires Samba built with the change. You can find F18 scratch build at
> http://koji.fedoraproject.org/koji/taskinfo?taskID=5168969, these
> patches will be submitted to Samba upstream this week.
>
> In the patch I'm filtering out our own DNS domain since its value will
> be added by Samba by default -- if PASSDB module does not provide the
> function, its absence will be ignored in the new API. Filtering out is
> done by freeing the string and moving empty item to last position in the
> array, reducing the array size but not resizing it -- talloc will hardly
> win anything from resizing one (char *) pointer and actual lifetime of
> the array is until the packet is sent so it is acceptable.
>
> In order to test the patch, you need updated Samba, then rebuild FreeIPA
> packages. Once installed and configured, UPN suffixes can be managed via
> 'ipa realmdomains-mod --{add,dell}-domain' commands. These domains will
> be exposed to Windows.
>
> When you added realm domains, an attempt to establish trust will cause
> Windows to ask for name suffixes and Samba will serve expanded list of
> them via netr_GetTrustInformation (opnum 44). In Samba code I've also
> implemented partially opnum 43 which is giving out the same information
> via DsRGetTrustInformation but so far Windows AD DC haven't actually
> tried to use opnum 43.
>
> Additionally, you can request Windows to update list of name suffixes
> via UI. Here is how it looks in Windows 2012 Server:
> http://abbra.fedorapeople.org/.paste/win2012-multiple-suffixes.png
>
> Part of ticket https://fedorahosted.org/freeipa/ticket/2848
I've tested the attached patch with the samba packages mentioned above
and everything works as expect.
As can be seen in the figure Alexander linked above the new suffixes are
disabled by default on the Windows side. This is expected and exactly
the same behaviour can be found in AD-AD trusts. Nevertheless it would
be good if you can make sure this behaviour is explicitly mentioned e.g.
in the design page or other documents to avoid confusion when this
feature is tested by others.
Review comments are in-line.
bye,
Sumit
>
>
> --
> / Alexander Bokovoy
> >From f400d55eaec99b3e5440e90b6a6d055e26529e7e Mon Sep 17 00:00:00 2001
> From: Alexander Bokovoy <abokovoy at redhat.com>
> Date: Fri, 22 Mar 2013 17:30:41 +0200
> Subject: [PATCH 2/2] ipasam: add enumeration of UPN suffixes based on the
> realm domains
>
> PASSDB API in Samba adds support for specifying UPN suffixes. The change
> in ipasam will allow to pass through list of realm domains as UPN suffixes
> so that Active Directory domain controller will be able to recognize
> non-primary UPN suffixes as belonging to IPA and properly find our KDC
> for cross-realm TGT.
>
> Since Samba already returns primary DNS domain separately, filter it out
> from list of UPN suffixes.
>
> Also enclose provider of UPN suffixes into #ifdef to support both
> Samba with and without pdb_enum_upn_suffixes().
>
> Part of https://fedorahosted.org/freeipa/ticket/2848
> ---
> daemons/configure.ac | 10 +++
> daemons/ipa-sam/ipa_sam.c | 172 ++++++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 177 insertions(+), 5 deletions(-)
>
> diff --git a/daemons/configure.ac b/daemons/configure.ac
> index d3b6b19..14dc04e 100644
> --- a/daemons/configure.ac
> +++ b/daemons/configure.ac
> @@ -252,6 +252,16 @@ AC_CHECK_LIB([wbclient],
> [$SAMBA40EXTRA_LIBPATH])
> AC_SUBST(WBCLIENT_LIBS)
>
> +AC_CHECK_LIB([pdb],
> + [make_pdb_method],
> + [HAVE_LIBPDB=1],
> + [AC_MSG_ERROR([libpdb does not have make_pdb_method])],
> + [$SAMBA40EXTRA_LIBPATH])
> +AC_CHECK_LIB([pdb],[pdb_enum_upn_suffixes],
> + [AC_DEFINE([HAVE_PDB_ENUM_UPN_SUFFIXES], [1], [Ability to enumerate UPN suffixes])],
> + [AC_MSG_WARN([libpdb does not have pdb_enum_upn_suffixes, no support for realm domains in ipasam])],
> + [$SAMBA40EXTRA_LIBPATH])
any reason for the extra space in the indentation?
> +
> dnl ---------------------------------------------------------------------------
> dnl - Check for check unit test framework http://check.sourceforge.net/
> dnl ---------------------------------------------------------------------------
...
> +static char **get_attribute_values(TALLOC_CTX *mem_ctx, LDAP *ldap_struct,
> + LDAPMessage *entry, const char *attribute, int *num_values)
> +{
> + struct berval **values;
> + int count, i;
> + char **result = NULL;
> + size_t conv_size;
> +
> + if (attribute == NULL || entry == NULL) {
> + return NULL;
> + }
> +
> + values = ldap_get_values_len(ldap_struct, entry, attribute);
> + if (values == NULL) {
> + DEBUG(10, ("Attribute [%s] not found.\n", attribute));
> + return NULL;
> + }
if ldap_get_values_len() returns NULL in the case of an error (according
to the man page), if there are no attributes, an empty array is
returned. This also means that count can be 0 below.
> +
> + /* Note that if we've got any values, count would be > 0 */
> + count = ldap_count_values_len(values);
> + result = talloc_array(mem_ctx, char *, count);
> + if (result == NULL) {
> + goto done;
> + }
> +
> + *num_values = count;
> + for (i = 0; i < count; i++) {
> + if (!convert_string_talloc(result, CH_UTF8, CH_UNIX,
> + values[i]->bv_val, values[i]->bv_len,
> + &result[i], &conv_size)) {
> + DEBUG(10, ("Failed to convert %dth value of [%s] out of %d.\n",
> + i, attribute, count));
> + talloc_free(result);
> + result = NULL;
maybe set *num_values = 0 as a precaution as well?
> + goto done;
> + }
> + }
> +
> +done:
> + ldap_value_free_len(values);
> + return result;
> +}
> +
>
...
> +#ifdef HAVE_PDB_ENUM_UPN_SUFFIXES
> +static NTSTATUS ipasam_enum_upn_suffixes(struct pdb_methods *pdb_methods,
> + TALLOC_CTX *mem_ctx,
> + uint32_t *num_suffixes,
> + char ***suffixes)
> +{
> + int ret;
> + LDAPMessage *result;
> + LDAPMessage *entry = NULL;
> + int count, i;
> + char *realmdomains_dn = NULL;
> + char **domains = NULL;
> + struct ldapsam_privates *ldap_state;
> + struct smbldap_state *smbldap_state;
> + const char *attr_list[] = {
> + "associatedDomain",
would you mind to #define attribute names and directory paths at the
beginning of ipa_sam.c?
> + NULL
> + };
> +
> + if ((suffixes == NULL) || (num_suffixes == NULL)) {
> + return NT_STATUS_UNSUCCESSFUL;
> + }
> +
> + ldap_state = (struct ldapsam_privates *)pdb_methods->private_data;
> + smbldap_state = ldap_state->smbldap_state;
> +
> + realmdomains_dn = talloc_asprintf(mem_ctx, "cn=Realm Domains,cn=ipa,cn=etc,%s",
> + ldap_state->ipasam_privates->base_dn);
> + if (realmdomains_dn == NULL) {
> + return NT_STATUS_NO_MEMORY;
> + }
> +
> + ret = smbldap_search(smbldap_state,
> + realmdomains_dn,
> + LDAP_SCOPE_BASE,
> + "objectclass=domainRelatedObject", attr_list, 0,
> + &result);
> + if (ret != LDAP_SUCCESS) {
> + DEBUG(1, ("Failed to get list of realm domains: %s\n",
> + ldap_err2string (ret)));
> + return NT_STATUS_UNSUCCESSFUL;
> + }
> +
> + count = ldap_count_entries(smbldap_state->ldap_struct, result);
> + if (count != 1) {
> + DEBUG(1, ("Unexpected number of results [%d] for realm domains "
> + "search.\n", count));
> + ldap_msgfree(result);
> + return NT_STATUS_OK;
> + }
Is there a reason to return NT_STATUS_OK for this particular error
condition?
> +
> + entry = ldap_first_entry(smbldap_state->ldap_struct, result);
> + if (entry == NULL) {
> + DEBUG(0, ("Could not get domainRelatedObject entry\n"));
> + ldap_msgfree(result);
> + return NT_STATUS_UNSUCCESSFUL;
> + }
> +
> + domains = get_attribute_values(mem_ctx, smbldap_state->ldap_struct, entry,
> + "associatedDomain", &count);
> + if (domains == NULL) {
> + ldap_msgfree(result);
> + return NT_STATUS_UNSUCCESSFUL;
> + }
> +
> + /* Since associatedDomain has attributeType MUST, there must be at least one domain */
> + for (i = 0; i < count ; i++) {
> + if (strcmp(ldap_state->domain_name, domains[i]) == 0) {
> + break;
> + }
> + }
Since we area handling DNS domain names here strcasecmp() would be more
fault tolerant? OTOH I think mixed cases here can only happen if some
modifies IPA LDAP object manually.
> +
> + if (i < count) {
> + /* If we found our primary domain in the list and it is alone, exit with empty list */
> + if (count == 1) {
> + ldap_msgfree(result);
> + talloc_free(domains);
> + return NT_STATUS_UNSUCCESSFUL;
> + }
> +
> + talloc_free(domains[i]);
> +
> + /* if i is not last element, move everything down */
> + if (i != (count - 1)) {
> + memcpy(domains + i, domains + i + 1, sizeof(char *) * (count - i - 1));
> + }
I think you meant memmove() according to your comment, because memcpy()
should not be used for overlapping areas.
Maybe add domains[count - 1] = NULL; as a precaution measure?
> +
> + /* we don't resize whole list, only reduce number of elements in it
> + * since sizing down a single pointer will not reduce memory usage in talloc
> + */
> + *suffixes = domains;
> + *num_suffixes = count - 1;
> + } else {
> + /* There is no our primary domain in the list */
> + *suffixes = domains;
> + *num_suffixes = count;
> + }
> +
> + ldap_msgfree(result);
> + return NT_STATUS_OK;
> +}
> +#endif /* HAVE_PDB_ENUM_UPN_SUFFIXES */
> +
> +
> #define SECRETS_DOMAIN_SID "SECRETS/SID"
> static char *sec_key(TALLOC_CTX *mem_ctx, const char *d)
> {
> @@ -4030,7 +4187,7 @@ static NTSTATUS pdb_init_ipasam(struct pdb_methods **pdb_method,
> }
>
> status = ipasam_get_domain_name(ldap_state, ldap_state,
> - &ldap_state->domain_name);
> + (char**) &ldap_state->domain_name);
> if (!NT_STATUS_IS_OK(status)) {
> DEBUG(0, ("Failed to get domain name.\n"));
> return status;
> @@ -4150,6 +4307,11 @@ static NTSTATUS pdb_init_ipasam(struct pdb_methods **pdb_method,
> (*pdb_method)->set_trusted_domain = ipasam_set_trusted_domain;
> (*pdb_method)->del_trusted_domain = ipasam_del_trusted_domain;
> (*pdb_method)->enum_trusted_domains = ipasam_enum_trusted_domains;
> +#ifdef HAVE_PDB_ENUM_UPN_SUFFIXES
> + (*pdb_method)->enum_upn_suffixes = ipasam_enum_upn_suffixes;
> + DEBUG(1, ("pdb_init_ipasam: support for pdb_enum_upn_suffixes "
> + "enabled for domain %s\n", ldap_state->domain_name));
> +#endif
>
> return NT_STATUS_OK;
> }
> --
> 1.8.1.4
>
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel
More information about the Freeipa-devel
mailing list