Re: [Freeipa-devel] [PATCH] 0100 Enumerate UPN suffixes in ipasam

2013-03-29 Thread Martin Kosek
On 03/27/2013 12:46 PM, Sumit Bose wrote:
 On Wed, Mar 27, 2013 at 12:53:18PM +0200, Alexander Bokovoy wrote:
 Hi,

 On Wed, 27 Mar 2013, Sumit Bose wrote:
 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.
 I'll add that, thanks for reminding.


 Review comments are in-line.

 bye,
 +
 +  /* 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.
 Technically it should be something that does utf-8 caseless lookups. We
 can go with strcasecmp as it is for time being, I'll add TODO there for
 future IDN handling.
 
 yes, good idea
 


 New patch attached. Survives Windows 2012 testing.
 
 Survives my testing with 2008 as well. ACK.
 
 bye,
 Sumit

Also survived my testing without support of PASSDB API in samba (i.e. did not
crash or disable existing functionality).

Pushed to master.

Martin

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCH] 0100 Enumerate UPN suffixes in ipasam

2013-03-27 Thread Sumit Bose
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 aboko...@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) {
 + 

Re: [Freeipa-devel] [PATCH] 0100 Enumerate UPN suffixes in ipasam

2013-03-27 Thread Alexander Bokovoy

Hi,

On Wed, 27 Mar 2013, Sumit Bose wrote:

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.

I'll add that, thanks for reminding.



Review comments are in-line.

bye,
Sumit




--
/ Alexander Bokovoy



From f400d55eaec99b3e5440e90b6a6d055e26529e7e Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy aboko...@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?

No :)



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

I ended up specifically checking count to zero and returning before
allocating values.


+#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?

Yes, I was wondering about that before. We have another places where we
use associatedDomain now. I've moved all of them to defines.


+   NULL
+ };
+
+   if ((suffixes == NULL) || (num_suffixes == NULL)) {
+   return NT_STATUS_UNSUCCESSFUL;
+   }
+
+   ldap_state = (struct ldapsam_privates *)pdb_methods-private_data;
+   

Re: [Freeipa-devel] [PATCH] 0100 Enumerate UPN suffixes in ipasam

2013-03-27 Thread Sumit Bose
On Wed, Mar 27, 2013 at 12:53:18PM +0200, Alexander Bokovoy wrote:
 Hi,
 
 On Wed, 27 Mar 2013, Sumit Bose wrote:
 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.
 I'll add that, thanks for reminding.
 
 
 Review comments are in-line.
 
 bye,
 +
 +   /* 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.
 Technically it should be something that does utf-8 caseless lookups. We
 can go with strcasecmp as it is for time being, I'll add TODO there for
 future IDN handling.

yes, good idea

 
 
 New patch attached. Survives Windows 2012 testing.

Survives my testing with 2008 as well. ACK.

bye,
Sumit

 
 
 
 -- 
 / Alexander Bokovoy

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel