Re: [Freeipa-devel] [PATCHES 0031-0032] Improve HBAC rule handling in selinuxusermap-add/mod/find

2013-02-27 Thread Martin Kosek
On 02/20/2013 12:31 PM, Tomas Babej wrote:
 On 02/19/2013 10:33 PM, Rob Crittenden wrote:
 Tomas Babej wrote:
 On 02/06/2013 07:57 PM, Rob Crittenden wrote:
 Tomas Babej wrote:
 Hi,

 this pair of patches improves HBAC rule handling in selinuxusermap
 commands.

 Patch 0031 deals with:
 https://fedorahosted.org/freeipa/ticket/3349

 Patch 0032 takes care of:
 https://fedorahosted.org/freeipa/ticket/3348

 and is to be applied on top of Patch 0031.

 See commit messages for detailed info.

 Tomas


 ACK for patch 0032.

 For patch 0031 we can't change the data type of an existing attribute.
 It will break backwards compatibility. Can you test with an older
 client to see if it cares (it may not care about the name of the
 type). If older clients will work then this is probably ok.

 I gather that seealso detected as a DN attribute and converted into a
 DN class and this is blowing up the Str validator?

 Yes, that was exactly the case.
 rob

 I added a workaround for older client versions, tested it with
 freeipa-client/admintools 2.2, works as expeceted.
 However, this only should be issue if there is older admintools package
 on the client than on the server.

 Outline is such as follows: I added a new flag for DNParam seelalso
 attribute, called 'allow_malformed' that allows any string to be passed
 to DNParam. Its value gets wrapped in 'malformed=yes,value=value'.
 This allows to parse out the string in selinuxusermap-add/mod
 pre_callback out of the DN and search for the rule with such name so
 that it's DN gets in LDAP instead.

 Updated patch attached.

 Tomas

 I like where you're going with this, just a couple of comments:

 1. Should we come up with a more universal name for allow_malformed? Is this
 something that we should allow at a higher level? I was thinking allow_raw,
 or allow_non_dn, or something like that.
 
 To me, allow_non_dn sounds is just as specific as allow_malformed,
 they both refer to DN specifically.
 
 I'd go with allow_raw, if need for such pattern may eventually arise for
 other parameter classes than DNParam.
 
 What do you mean by higher level, turning this hack into a feature
 Param class? I don't see how this would work, each parameter
 class that implements its own type validation as DNParam needs
 to override _convert_scalar(). And in every such class we would need
 to wrap our raw value so that it is represented in the type of this parameter,
 as we do with DN(('malformed','yes'),('value',value)) now.
 
 Maybe we could skip type validation in _convert_scalar default
 implementation or catch the error raised somehow, and let the type be
 invalid, but I'm not aware of the consenquences. I would need to investigate.
 Wouldn't it cause failure deeper in the framework?
 
 Or did you by higher level mean simply picking a more general name for the
 flag so it can be reused in other parameter classes with the same name?

 2. I think that if a bad dn is passed in as a Str the conversion into a DN
 won't be handled:

 +if 'allow_malformed' in self.flags:
 +dn = DN(('malformed','yes'),('value',value))

 Should this be wrapped in a try/except to raise a ConversionError if it 
 fails?
 Yes, thanks for that catch.

 rob
 Tomas

Is it just me, or does the 0031 look overengineered? I think this is a general
problem for each Str parameter which we then process/convert to DN in our
pre_callbacks.

selinuxusermap is one example where this does not work. This fix leaves other
examples not working:

# ipa trustconfig-mod --setattr ipantfallbackprimarygroup=cn=Default SMB
Group,cn=groups,cn=accounts,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com
ipa: ERROR: invalid 'ipantfallbackprimarygroup': must be Unicode text

I would rather propose to not automatically encode DN of known attributes set
by *attr:

diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index 1ebbe7a..e4b9834 100644
--- a/ipalib/plugins/baseldap.py
+++ b/ipalib/plugins/baseldap.py
@@ -768,12 +768,6 @@ last, after all sets and adds.),
 # None means delete this attribute
 value = None

-if ldap.has_dn_syntax(attr):
-try:
-value = DN(value)
-except ValueError:
-raise errors.InvalidSyntax(attr=attr)
-
 if attr in newdict:
 if type(value) in (tuple,):
 newdict[attr] += list(value)

I think this conversion is just done too early as this Str param is processed
and converted later in the pre_callback, when needed. The code above introduced
inconsistent processing of IPA attributes with DN syntax coming from regular
option and from *attr option - Str

When I did this change, both selinuxusermap-mod and trustconfig-mod started
working:

# ipa selinuxusermap-mod foo
--setattr=seealso=ipaUniqueID=70e42636-75db-11e2-9df6-001a4a104edc,cn=hbac,dc=rhel64,dc=ad,dc=test
---
Modified SELinux User Map foo

Re: [Freeipa-devel] [PATCH] 0006 Remove check for alphabetic only characters from domain name validation

2013-02-27 Thread Martin Kosek
On 02/22/2013 04:02 PM, Ana Krivokapic wrote:
 On 02/22/2013 10:19 AM, Petr Spacek wrote:
 On 20.2.2013 11:03, Ana Krivokapic wrote:
 On 02/18/2013 01:08 PM, Martin Kosek wrote:
 On 02/18/2013 12:47 PM, Sumit Bose wrote:
 On Mon, Feb 18, 2013 at 12:27:35PM +0100, Petr Spacek wrote:
 On 15.2.2013 15:22, Ana Krivokapic wrote:
 Hello,

 The .isalpha() check in validate_domain_name() was too strict,
 causing some commands like ipa dnsrecord-add to fail.

 https://fedorahosted.org/freeipa/ticket/3385
 I would add --force option rather than removing whole check, if
 it's possible.

 Would it be possible to mention RFC in the error message? Something
 like _('top level domain label must be alphabetic (RFC 1123 section
 2.1)')
 ?

 IMHO it is handy, because it educates users.
 The problem is that this check is always done on the last component of
 the domain_name even if it is just a sub-domain of the FreeIPA domain,
 where e.g. numbers are valid characters.

 At the beginning of validate_domain_name() a trailing '.' is stripped
 away. iirc the trailing '.' is an indication for a complete, fully
 qualified name. Would it work if the presence of the trailing '.' is
 saved and the check is only done if there was a '.'?

 bye,
 Sumit

 Sure. Though I am now not 100% sure that some IPA functions do not
 use this
 validator with a fqdn hostname without trailing dot. If not, I am
 for fixing
 this function as Sumit and Petr suggested.

 Martin

 ___
 Freeipa-devel mailing list
 Freeipa-devel@redhat.com
 https://www.redhat.com/mailman/listinfo/freeipa-devel
 After some thought, I decided to change the approach.

 As pointed out by Sumit, the problem was that the validate_domain_name()
 function did not distinguish between fqdn and non-fqdn domains
 (subdomains of the IPA domain). The trailing dot is not a clear
 indication either, because some IPA functions use this validator with an
 fqdn without the trailing dot.

 To fix this, I introduced an additional parameter to this function - a
 flag which indicates whether the domain name is an fqdn or not. The is
 .isalpha() check is then performed only in the case of an fqdn.

 I also improved the error message to mention the relevant RFC, as
 suggested by Petr.

 Please don't forget to add --force switch. It could be handy.

 I added the --force switch to ipa dnsrecord-add and opened a new ticket
 to handle the rest of the ipa commands that use domain name validation:
 https://fedorahosted.org/freeipa/ticket/3455
 
 Updated patch is attached.
 

This patch fixed validation only partially. The --force flag you made available
will not allow admin to for example add a zone example.zone1 which
technically will be resolvable, it is just not a good practice:

# ipa dnszone-add example.zone1 --name-server `hostname`. --force
ipa: ERROR: invalid 'name': top level domain label must be alphabetic (RFC 1123
section 2.1)

To enable this, I think you would need to not postpone the validation to DNS
zone pre_callback as you could not check --force flag presence right in the
idnsname parameter validator.

We may also want to change --force flag label, it now talks only about NS
record validation, but we now expanded it a bit, so the label would need to be
more general.

Martin

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


Re: [Freeipa-devel] [PATCH] 374 Remove ORDERING for IA5 attributeTypes

2013-02-27 Thread Petr Viktorin

On 02/26/2013 06:03 PM, Martin Kosek wrote:

IA5 string syntax does not have a compatible ORDERING matching rule.
Simply use default ORDERING for these attributeTypes as we already
do in other cases.

https://fedorahosted.org/freeipa/ticket/3398

-

This is a follow-up ticket for regression introduced by Rob's 1087. I did not
add any update rules for IPA servers that were already wrongly updated as we
did not release any IPA version with these bad attributeTypes and I want to
keep .update files small.

Martin


ACK

--
Petr³

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


Re: [Freeipa-devel] [PATCHES] 94-99 Read and use per-service PAC type

2013-02-27 Thread Sumit Bose
On Mon, Feb 25, 2013 at 04:35:20PM +0100, Martin Kosek wrote:
 On 02/21/2013 04:24 PM, Sumit Bose wrote:
  Hi,
  
  this series of patches fix https://fedorahosted.org/freeipa/ticket/2960
  The related design page is
  http://freeipa.org/page/V3/Read_and_use_per_service_pac_type .
  
  It was agreed while discussing the design page that the currently
  hardcoded value for the NFS service will be dropped completely, hence
  the first patch reverts to patch which added the hardcoded value.
  
  The second patch adds the needed attribute to compensate the now missing
  hardcoded value.
  
  In the following three patches the PAC type is read and used
  accordingly. The last patch adds a unit test for a new function.
  
  bye,
  Sumit
 
 I did only sanity testing to the C part of the RFE so far, but by reading it 
 it
 looks ok. It may be beneficial if Simo or Alexander checked if the ipa-kdb 
 part
 is OK.

Alexander asked in irc to change strcmp() to strcasecmp() so that
options like 'Ms-Pac' or 'none' are accepted as well.

 
 I will comment on the Python parts:
 
 1) Your update check testing if there is any NFS service with ipakrbauthzdata
 looks like a good heuristics to prevent admin frustration. Though an ideal
 solution would require
 https://fedorahosted.org/freeipa/ticket/2961
 to prevent multiple updates when admin purposefully removes this attribute. We
 may want to reference the ticket in a comment in the update plugin...

I added a comment in the code and in #2961.

 
 
 2) The upgrade plugin may get into infinite loop if ldap.find_entries returns
 truncated result. As you do not update entries directly but only return update
 instructions when you have no truncated results, you will loop.
 
 To simulate this, I just created 2 NFS principals and set size_limit=1 in your
 find_entries call.

Thanks for catching this, I removed the truncated logic and added a
messages if truncated was set.

 
 
 3) Ok, we set ipakrbauthzdata to NONE on upgrades. But what about new NFS
 service principals added by service-add? Shouldn't we set ipakrbauthzdata for
 new services too? As a default when no user ipakrhbauthzdata is set...
 Otherwise admins could be surprised why their new NFS services do not work
 while the others do.
 
 Also, I think we should have this NFS culprit documented somewhere (i.e. why 
 we
 set ipakrbauthzdata to NONE by default). At least as a small section in the
 online help for services plugin...

I added comments to the help and the doc string in patch #100. If you
think it is necessary to implicitly set PAC type to 'NONE' if NFS
services are added and no type is given explicitly, I would prefer if
you can open a new tickets so that it can be discussed independently.

Thank you for the review. New versions attached.

bye,
Sumit

 
 Martin
From 8eb34dd3190853bd3fa400434be4f7c720bf1354 Mon Sep 17 00:00:00 2001
From: Sumit Bose sb...@redhat.com
Date: Tue, 12 Feb 2013 09:59:00 +0100
Subject: [PATCH 094/100] Revert MS-PAC: Special case NFS services

This reverts commit 5269458f552380759c86018cd1f30b64761be92e.

With the implementation of https://fedorahosted.org/freeipa/ticket/2960
a special hardcoded handling of NFS service tickets is not needed
anymore.
---
 daemons/ipa-kdb/ipa_kdb_mspac.c | 36 +---
 1 file changed, 1 insertion(+), 35 deletions(-)

diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
index 
95349702038e1d55bd257816f7f61e9557a5..f05c552fc18eb9b15ee3e39b513184589bc8c468
 100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -743,24 +743,6 @@ static bool is_cross_realm_krbtgt(krb5_const_principal 
princ)
 return true;
 }
 
-static bool is_service_of_type(krb5_const_principal princ, const char *type)
-{
-size_t len;
-
-if (princ-length  2) {
-return false;
-}
-
-len = strlen(type);
-
-if ((princ-data[0].length == len) ||
-(strncasecmp(princ-data[0].data, type, len) == 0)) {
-return true;
-}
-
-return false;
-}
-
 static char *gen_sid_string(TALLOC_CTX *memctx, struct dom_sid *dom_sid,
 uint32_t rid)
 {
@@ -1555,7 +1537,6 @@ krb5_error_code ipadb_sign_authdata(krb5_context context,
 krb5_error_code kerr;
 krb5_pac pac = NULL;
 krb5_data pac_data;
-bool is_nfs = false;
 
 /* When using s4u2proxy client_princ actually refers to the proxied user
  * while client-princ to the proxy service asking for the TGS on behalf
@@ -1566,32 +1547,17 @@ krb5_error_code ipadb_sign_authdata(krb5_context 
context,
 ks_client_princ = client-princ;
 }
 
-/* NFS Server on Linux is limited and will choke on big tickets.
- * So avoid attachnig the PAC to nfs/ tickets for now.
- * FIXME: remove this when we have interface to support disabling
- * PACs on arbitrary services */
-if (is_service_of_type(ks_client_princ, nfs) ||
-is_service_of_type(server-princ, nfs)) 

[Freeipa-devel] [PATCHES] 101-107 Fixes for various Coverity issues

2013-02-27 Thread Sumit Bose
Hi,

the attached patches 102-107 fix issues found by Coverity which are
tracked by tickets #3422-#3427 and remove an unused variable
(patch 101).

bye,
Sumit
From 97b3b7dedac28704d51e2fa4b4dc975a20d17ada Mon Sep 17 00:00:00 2001
From: Sumit Bose sb...@redhat.com
Date: Tue, 19 Feb 2013 12:48:58 +0100
Subject: [PATCH 101/107] ipa-kdb: remove unused variable

---
 daemons/ipa-kdb/ipa_kdb_principals.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/daemons/ipa-kdb/ipa_kdb_principals.c 
b/daemons/ipa-kdb/ipa_kdb_principals.c
index 
46a921cb2f267b977df4a0042d81a49beccb5a91..11c155e64e20d16da98158eb4d7b8034803bf1de
 100644
--- a/daemons/ipa-kdb/ipa_kdb_principals.c
+++ b/daemons/ipa-kdb/ipa_kdb_principals.c
@@ -535,7 +535,7 @@ static krb5_error_code ipadb_fetch_principals(struct 
ipadb_context *ipactx,
 krb5_error_code kerr;
 char *src_filter = NULL;
 char *esc_original_princ = NULL;
-int ret, i;
+int ret;
 
 if (!ipactx-lcontext) {
 ret = ipadb_get_connection(ipactx);
-- 
1.8.1.2

From d5a64eced4800190221395d19341918ae417f118 Mon Sep 17 00:00:00 2001
From: Sumit Bose sb...@redhat.com
Date: Tue, 19 Feb 2013 12:49:25 +0100
Subject: [PATCH 102/107] ipa-kdb: Uninitialized scalar variable in
 ipadb_reinit_mspac()

There was a code path where ret was used instead of kerr to save a
return value.

Fixes https://fedorahosted.org/freeipa/ticket/3422
---
 daemons/ipa-kdb/ipa_kdb_mspac.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
index 
c72e762ca4c680def9ed97e492f6edfcc5e0677f..d022bd9a958f2ec591a6a338753b510492b2d400
 100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -1986,12 +1986,11 @@ krb5_error_code ipadb_reinit_mspac(struct ipadb_context 
*ipactx)
 if (ipactx-mspac  ipactx-mspac-num_trusts == 0) {
 /* Check if there is any trust configured. If not, just return
  * and do not re-initialize the MS-PAC structure. */
-ret = ipadb_mspac_check_trusted_domains(ipactx);
-if (ret == KRB5_KDB_NOENTRY) {
-ret = 0;
+kerr = ipadb_mspac_check_trusted_domains(ipactx);
+if (kerr == KRB5_KDB_NOENTRY) {
+kerr = 0;
 goto done;
-} else if (ret != 0) {
-ret = EIO;
+} else if (kerr != 0) {
 goto done;
 }
 }
-- 
1.8.1.2

From c36fc476b52315012d2d17994d0337761d25cb5e Mon Sep 17 00:00:00 2001
From: Sumit Bose sb...@redhat.com
Date: Wed, 27 Feb 2013 12:12:45 +0100
Subject: [PATCH 103/107] ipa-sam: Array compared against 0 in
 ipasam_set_trusted_domain()

ipa_mspac_well_known_sids is a globally defined array so the check was
always true.

Fixes https://fedorahosted.org/freeipa/ticket/3423
---
 daemons/ipa-sam/ipa_sam.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/daemons/ipa-sam/ipa_sam.c b/daemons/ipa-sam/ipa_sam.c
index 
0d4a27cf6def737412f6ab3ca48df8d1339c15b6..b9fc00c8d08821e09b3242a82b3673a88ace0a51
 100644
--- a/daemons/ipa-sam/ipa_sam.c
+++ b/daemons/ipa-sam/ipa_sam.c
@@ -2293,7 +2293,7 @@ static NTSTATUS ipasam_set_trusted_domain(struct 
pdb_methods *methods,
  td-trust_forest_trust_info);
}
 
-   for (i = 0; ipa_mspac_well_known_sids  ipa_mspac_well_known_sids[i]; 
i++) {
+   for (i = 0; ipa_mspac_well_known_sids[i]; i++) {
smbldap_make_mod(priv2ld(ldap_state), entry, mods,
  LDAP_ATTRIBUTE_SID_BLACKLIST_INCOMING,
  ipa_mspac_well_known_sids[i]);
-- 
1.8.1.2

From bffd1ec51ea697887d012ec1eda4171f4affafaf Mon Sep 17 00:00:00 2001
From: Sumit Bose sb...@redhat.com
Date: Fri, 22 Feb 2013 13:30:30 +0100
Subject: [PATCH 104/107] ipa-kdb: Dereference after null check in
 ipa_kdb_mspac.c

A wrong logic was used to check ipactx.

Fixes https://fedorahosted.org/freeipa/ticket/3424
---
 daemons/ipa-kdb/ipa_kdb_mspac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
index 
d022bd9a958f2ec591a6a338753b510492b2d400..2c07f4cffe38eb127a7b3f901dc7700e88d6af2a
 100644
--- a/daemons/ipa-kdb/ipa_kdb_mspac.c
+++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
@@ -1217,7 +1217,7 @@ static krb5_error_code filter_logon_info(krb5_context 
context,
  * */
 if (info-info-info3.sidcount != 0) {
 ipactx = ipadb_get_context(context);
-if (!ipactx  !ipactx-mspac) {
+if (!ipactx || !ipactx-mspac) {
 return KRB5_KDB_DBNOTINITED;
 }
 count = info-info-info3.sidcount;
-- 
1.8.1.2

From e56a47f82f4600ebcb26c012c382c6686125c14a Mon Sep 17 00:00:00 2001
From: Sumit Bose sb...@redhat.com
Date: Fri, 22 Feb 2013 13:40:08 +0100
Subject: [PATCH 105/107] ipa-lockout: Wrong sizeof argument in ipa_lockout.c

Fixes https://fedorahosted.org/freeipa/ticket/3425
---
 

[Freeipa-devel] Using the new LDAP code

2013-02-27 Thread Petr Viktorin

Hello,
A big refactoring of our LDAP code should be merged soon-ish now. Here's 
a summary for developers.



If you see these outside ipaldap.py, you're looking at deprecated API:
- methods with camelCaseNames
- methods with _s and _ext postfixes (modify_s, search_ext, ...)

The exception is client code and places where we don't want to read the 
schema (migration, AD). These are still limited to raw python-ldap for now.




The LDAPEntry class represents LDAP entries. It behaves like a 
dictionary of lists: entry.get(attrname) or entry[attrname] should 
always give you a list.


LDAPEntry.dn will give you the entry's DN.

Single-value attributes are represented as lists with a single value. 
(For now, some code still puts bare values instead of lists in entries, 
in that case you'll still get a bare value from get()/__getitem__. That 
should be fixed eventually.)
The single_value method gets a single value, with checking. Always use 
`entry.single_value('abc')` instead of `entry.get('abc')[0]`.

Also, single_value allows a default: `entry.single_value('abc', None)`.

LDAPEntry is case-insensitive and handles attributes with multiple 
names: entry['cn'] and entry['CN'] and entry['CommonName'] are equivalent.


IPA plugins traditionally use (dn, entry_attrs) pairs to represent 
entries. To make that work, iterating over an LDAPEntry will, for now, 
yield the DN and the entry itself. Always use keys() or values() when 
iterating over an entry.


LDAPEntry objects are tied to a connection. Do not create them directly, 
use a connection method like make_entry() or get_entry().



Speaking of connections, there still are two classes for those: ldap2 
and IPAdmin.
ldap2 is an API plugin created using the IPA settings. It works in 
Apache (per-thread connections). It also applies the default IPA 
settings (time  records limit).
Use IPAdmin if IPA is not installed yet (or when it's being 
uninstalled), or if you need to connect to a non-default server or bind 
as a user like the DM.


Besides the connecting code, both of these (will ideally) have the same 
API, based on the old ldap2. A rough summary:


- make_entry(dn, attrs)
- get_entry(dn)
- add_entry(entry)
- update_entry(entry)
- delete_entry(entry_or_dn)

- get_entries(base_dn, [scope, [filter, [attrs_list]]]): simple search
- find_entries: more powerful (and backwards-compatible) search
- make_filter  friends, unchanged from ldap2


ldap2's DN normalization – appending the suffix to DNs that don't end 
with it ­– is gone, you need to do that manually now.




That should be it, if you don't intend to hack on ipaldap itself or the 
ldapupdater. If you have any questions, ask! (Or look at the code :)


--
Petr³

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

Re: [Freeipa-devel] [PATCH] 374 Remove ORDERING for IA5 attributeTypes

2013-02-27 Thread Martin Kosek
On 02/27/2013 11:58 AM, Petr Viktorin wrote:
 On 02/26/2013 06:03 PM, Martin Kosek wrote:
 IA5 string syntax does not have a compatible ORDERING matching rule.
 Simply use default ORDERING for these attributeTypes as we already
 do in other cases.

 https://fedorahosted.org/freeipa/ticket/3398

 -

 This is a follow-up ticket for regression introduced by Rob's 1087. I did not
 add any update rules for IPA servers that were already wrongly updated as we
 did not release any IPA version with these bad attributeTypes and I want to
 keep .update files small.

 Martin
 
 ACK
 

Pushed to master, ipa-3-1, ipa-3-0.

Martin

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


Re: [Freeipa-devel] [PATCHES] 101-107 Fixes for various Coverity issues

2013-02-27 Thread Martin Kosek
On 02/27/2013 01:39 PM, Martin Kosek wrote:
 On 02/27/2013 12:35 PM, Sumit Bose wrote:
 Hi,

 the attached patches 102-107 fix issues found by Coverity which are
 tracked by tickets #3422-#3427 and remove an unused variable
 (patch 101).

 bye,
 Sumit

 
 I see just one issue. In patch 0105:
 
 -global_ipactx = (struct ipa_context *)malloc(sizeof(global_ipactx));
 +global_ipactx = (struct ipa_context *)malloc(sizeof(*global_ipactx));
 
 I do not think this will work right. *global_ipactx will just de-reference
 global_ipactxt and run sizeof on the result, right?
 
 I would prefer this change:
 
 global_ipactx = (struct ipa_context *)malloc(sizeof(struct ipa_context *));
 
 Martin
 

We just discussed this in Brno - in fact your change should be right as you
allocate memory for the whole structure and not just a pointer to the structure
(though I personally would prefer sizeof(struct ipa_context) as it is clearer).

Hmrph, C language 101 revisited :-)

Martin

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


[Freeipa-devel] CA name constrains

2013-02-27 Thread Petr Spacek

Hello list,

during our last meeting with Simo we discussed support for name constraint 
extension in CA certificates and clients.


The Name Constraints Extensions is defined here:
http://tools.ietf.org/html/rfc5280#section-4.2.1.10

Following article could be interesting for you if you like longer stories:
Mozilla changes policy to limit risk of subordinate CA certificate abuse
Author: Lucian Constantin 19.02.2013 kl 21:50
http://news.idg.no/cw/art.cfm?id=8C9E7CFA-0E65-24B0-1539C891C8F4C09B

If I remember correctly, questions were mainly about support on client side 
and about implications for older clients.


--
Petr^2 Spacek

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


Re: [Freeipa-devel] [PATCHES] 146-164 LDAP code refactoring (Part 4)

2013-02-27 Thread Jan Cholasta

On 26.2.2013 11:03, Petr Viktorin wrote:

Thanks. I think you should also add a tearDown method to test_LDAPEntry
which disconnects self.conn if it is connected (the same thing test_ldap
does).


Thanks for the catch, added.



ACK.

--
Jan Cholasta

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


Re: [Freeipa-devel] CA name constrains

2013-02-27 Thread Simo Sorce
On Wed, 2013-02-27 at 13:55 +0100, Petr Spacek wrote:
 Hello list,
 
 during our last meeting with Simo we discussed support for name constraint 
 extension in CA certificates and clients.
 
 The Name Constraints Extensions is defined here:
 http://tools.ietf.org/html/rfc5280#section-4.2.1.10
 
 Following article could be interesting for you if you like longer stories:
 Mozilla changes policy to limit risk of subordinate CA certificate abuse
 Author: Lucian Constantin 19.02.2013 kl 21:50
 http://news.idg.no/cw/art.cfm?id=8C9E7CFA-0E65-24B0-1539C891C8F4C09B
 
 If I remember correctly, questions were mainly about support on client side 
 and about implications for older clients.

I had a chat with Kai Engert (in CC) at DevConf.cz about this, we'll try
to work on this as time permits.
NSS seem to support this extension but so far we do not have tests
covering it apparently.

Simo.

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

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


Re: [Freeipa-devel] What about desktop policies?

2013-02-27 Thread Loris Santamaria
El mar, 26-02-2013 a las 15:11 -0500, Dmitri Pal escribió:
 On 02/25/2013 02:15 PM, Loris Santamaria wrote: 
  Hi all, 
  
  some customers of ours are interested in managing desktop policies for
  their linux workstations, really nothing fancy, corporate background and
  proxy settings are the most common requests.
  
  In the past I created Gnome desktop profiles using Sabayon, distributed
  them using puppet and associated them to user accounts with a Sabayon
  specific LDAP attribute, a process a bit convoluted, and no longer
  possible since sabayon is no longer developed. Also it was really buggy,
  and very gnome specific.
  
  I was thinking in how integrate desktop policies in freeIPA in a general
  manner and I wanted to share my ideas with you. Hopefully some of this
  may be incorporated in IPA at some point in the future.
  
  Properties of a policy:
  
* is a collection of settings
* can be associated with users or groups (desktop policy) or with
  hosts or hostgroups (system policy)
* is associated with a consumer, the client software that
  interprets and applies the policy. This way one could define
  policies for dconf, policies for kde, policies for WBEM.
  
  Properties of a setting
* is a key-value pair
* must conform to a schema
* may be mandatory
  
  The schema:
* indicates which attributes a policy may consist of
* indicates which kind of value may take an attribute. Bool,
  string, etc.
* There may be more than one schema for a given consumer. For
  example for dconf you may have an evolution schema, a
  gnome-games schema, etc.
   
  Sample policy creation process:
   1. The admin creates a new schema in IPA, with a command like ipa
  schema-add --consumer=dconf gnomeSettingsSchema
   2. The admin adds some definition to the schema: ipa
  schema-add-setting gnomeSettingsSchema
  --name=/schemas/desktop/gnome/background/picture_filename
  --type=string --description='File to use for the background
  image.'
   3. He creates a new policy: ipa policy-add corporateBackground
  --type=desktop --consumer=dconf
   4. He adds a setting to the policy: ipa policy-add-setting
  corporateBackground
  --name=/schemas/desktop/gnome/background/picture_filename
  --value=file:///san/wp/wallpaper.jpg --mandatory. Ipa would
  check that the key is defined in one of the dconf related
  schemas and the value is acceptable for that key.
   5. He associates the policy with users: ipa-policy-add-user
  corporateBackground --groups=ipausers
  
  How should the policy be applied? On the workstation, on startup, an ipa
  related utility should check if there are any policies related to the
  workstation, if there are any it should call a helper capable of
  applying a specific type of policy. Then on user logon another ipa
  related utility should check if there are any policies associated with
  the user and call the appropriate helper, if available.
  
  For the policy created in the above example, on logon the ipa policy
  utility would find that there is a policy of type dconf associated with
  the user. It would check if there is a dconf policy helper installed and
  if positive it would call the helper passing it the parameters defined
  in the policy.
  
  Hope this is useful at least as a starting point in defining desktop
  policies in IPA.
 
 This is great!
 Thank you for sharing some ideas.
 We sort of stayed away from centralized policy management for quite
 some time.
 Originally we thought that IPA will do policy management and did a lot
 of design around it.
 However at some point we realized that there is an overlap with the
 system management and content management for which things like puppet
 are more suitable. We said then that IdM would focus on managing
 identity related policies that are traditionally served via LDAP.
 The things that you are talking about resemble to some extent MSFT GPO
 and we felt that IdM might not be the right place for it. May be it is
 time to reassess it.
 I would however not go that route at least yet.

Hey puppet is great and we use it a lot to install packages and to
distribute configuration files, yet it is not so great to set these
key=value kind of settings of which more and more modern software
consists of. When you take into consideration gconf settings for gnome
2.x, dconf settings for gnome 3.x, mozilla settings, thunderbird
settings it quickly becomes a PITA to manage them distributing around
files with puppet.

Having those key=value pairs in an ldap would allow a helper on the
client to pull only the keys it understands and to merge them in the
configuration database in the appropriate way.

Of course i took inspiration from AD GPOs, yet I think that IPA should
manage these key=value kind of policies in a more 

Re: [Freeipa-devel] [PATCHES] 94-99 Read and use per-service PAC type

2013-02-27 Thread Simo Sorce
On Wed, 2013-02-27 at 11:58 +0100, Sumit Bose wrote:
 On Mon, Feb 25, 2013 at 04:35:20PM +0100, Martin Kosek wrote:
  On 02/21/2013 04:24 PM, Sumit Bose wrote:
   Hi,
   
   this series of patches fix https://fedorahosted.org/freeipa/ticket/2960
   The related design page is
   http://freeipa.org/page/V3/Read_and_use_per_service_pac_type .
   
   It was agreed while discussing the design page that the currently
   hardcoded value for the NFS service will be dropped completely, hence
   the first patch reverts to patch which added the hardcoded value.
   
   The second patch adds the needed attribute to compensate the now missing
   hardcoded value.
   
   In the following three patches the PAC type is read and used
   accordingly. The last patch adds a unit test for a new function.
   
   bye,
   Sumit
  
  I did only sanity testing to the C part of the RFE so far, but by reading 
  it it
  looks ok. It may be beneficial if Simo or Alexander checked if the ipa-kdb 
  part
  is OK.
 
 Alexander asked in irc to change strcmp() to strcasecmp() so that
 options like 'Ms-Pac' or 'none' are accepted as well.

Is there a good reason to allow insensitive matching ?

in LDAP you can't use true or false either for booleans, you have to use
TRUE and FALSE, so I am not so thrilled to allow insensitive matching
for this option as well.

  I will comment on the Python parts:
  
  1) Your update check testing if there is any NFS service with 
  ipakrbauthzdata
  looks like a good heuristics to prevent admin frustration. Though an ideal
  solution would require
  https://fedorahosted.org/freeipa/ticket/2961
  to prevent multiple updates when admin purposefully removes this attribute. 
  We
  may want to reference the ticket in a comment in the update plugin...
 
 I added a comment in the code and in #2961.
 
  
  
  2) The upgrade plugin may get into infinite loop if ldap.find_entries 
  returns
  truncated result. As you do not update entries directly but only return 
  update
  instructions when you have no truncated results, you will loop.
  
  To simulate this, I just created 2 NFS principals and set size_limit=1 in 
  your
  find_entries call.
 
 Thanks for catching this, I removed the truncated logic and added a
 messages if truncated was set.
 
  
  
  3) Ok, we set ipakrbauthzdata to NONE on upgrades. But what about new NFS
  service principals added by service-add? Shouldn't we set ipakrbauthzdata 
  for
  new services too? As a default when no user ipakrhbauthzdata is set...
  Otherwise admins could be surprised why their new NFS services do not work
  while the others do.
  
  Also, I think we should have this NFS culprit documented somewhere (i.e. 
  why we
  set ipakrbauthzdata to NONE by default). At least as a small section in the
  online help for services plugin...
 
 I added comments to the help and the doc string in patch #100. If you
 think it is necessary to implicitly set PAC type to 'NONE' if NFS
 services are added and no type is given explicitly, I would prefer if
 you can open a new tickets so that it can be discussed independently.
 
 Thank you for the review. New versions attached.

I do not think we should set the policy to NONE by default, OTOH I see
the problem with upgrades. Maybe we should have a way to express
additional defaults, per service type.

Ie add additional values like:

ipakrbAuthsData: nfs:NONE

This would be a default but only for services that have the form nfs/*@*

This would allow us to avoid magical special casing in the code and
allow admins to change the overall default for specific services as
needed.

Maybe we should add a new ticket for this ?

Simo.

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

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


Re: [Freeipa-devel] CA name constrains

2013-02-27 Thread Simo Sorce
On Wed, 2013-02-27 at 08:16 -0500, Simo Sorce wrote:
 On Wed, 2013-02-27 at 13:55 +0100, Petr Spacek wrote:
  Hello list,
  
  during our last meeting with Simo we discussed support for name constraint 
  extension in CA certificates and clients.
  
  The Name Constraints Extensions is defined here:
  http://tools.ietf.org/html/rfc5280#section-4.2.1.10
  
  Following article could be interesting for you if you like longer stories:
  Mozilla changes policy to limit risk of subordinate CA certificate abuse
  Author: Lucian Constantin 19.02.2013 kl 21:50
  http://news.idg.no/cw/art.cfm?id=8C9E7CFA-0E65-24B0-1539C891C8F4C09B
  
  If I remember correctly, questions were mainly about support on client side 
  and about implications for older clients.
 
 I had a chat with Kai Engert (in CC) at DevConf.cz about this, we'll try
 to work on this as time permits.
 NSS seem to support this extension but so far we do not have tests
 covering it apparently.
 
 Simo.
 

Btw I opened ticket https://fedorahosted.org/freeipa/ticket/3466 to
track this.

Simo.

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

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


[Freeipa-devel] [PATCH] 105 Fix remove while iterating in suppress_netgroup_memberof

2013-02-27 Thread Jan Cholasta

Hi,

this patch fixes https://fedorahosted.org/freeipa/ticket/3464.

Honza

--
Jan Cholasta
From c40f1f123b905fdd0ee4d05d32f3d86e6ffdccc0 Mon Sep 17 00:00:00 2001
From: Jan Cholasta jchol...@redhat.com
Date: Wed, 27 Feb 2013 14:14:33 +0100
Subject: [PATCH] Fix remove while iterating in suppress_netgroup_memberof.

https://fedorahosted.org/freeipa/ticket/3464
---
 ipalib/plugins/host.py| 2 +-
 ipalib/plugins/hostgroup.py   | 2 +-
 tests/test_xmlrpc/test_nesting.py | 1 -
 3 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/ipalib/plugins/host.py b/ipalib/plugins/host.py
index f464127..4a47f50 100644
--- a/ipalib/plugins/host.py
+++ b/ipalib/plugins/host.py
@@ -364,7 +364,7 @@ class host(LDAPObject):
 
 ng_container = DN(api.env.container_netgroup, api.env.basedn)
 if 'memberofindirect' in entry_attrs:
-for member in entry_attrs['memberofindirect']:
+for member in list(entry_attrs['memberofindirect']):
 memberdn = DN(member)
 if memberdn.endswith(ng_container):
 try:
diff --git a/ipalib/plugins/hostgroup.py b/ipalib/plugins/hostgroup.py
index 7ae438c..9fb1029 100644
--- a/ipalib/plugins/hostgroup.py
+++ b/ipalib/plugins/hostgroup.py
@@ -99,7 +99,7 @@ class hostgroup(LDAPObject):
 
 if 'memberof' in entry_attrs:
 hgdn = DN(dn)
-for member in entry_attrs['memberof']:
+for member in list(entry_attrs['memberof']):
 ngdn = DN(member)
 if ngdn['cn'] == hgdn['cn']:
 try:
diff --git a/tests/test_xmlrpc/test_nesting.py b/tests/test_xmlrpc/test_nesting.py
index a09a798..e1b41b2 100644
--- a/tests/test_xmlrpc/test_nesting.py
+++ b/tests/test_xmlrpc/test_nesting.py
@@ -790,7 +790,6 @@ class test_nesting(Declarative):
 managedby_host=[fqdn1],
 memberof_hostgroup = [u'testhostgroup2'],
 memberofindirect_hostgroup = [u'testhostgroup1'],
-memberofindirect_netgroup = [u'testhostgroup2'],
 ),
 ),
 ),
-- 
1.8.1

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

Re: [Freeipa-devel] DESIGN: Recover DNA Ranges

2013-02-27 Thread Rob Crittenden

Sumit Bose wrote:

On Mon, Feb 25, 2013 at 03:12:19PM +0100, Martin Kosek wrote:

On 02/25/2013 03:09 PM, Rob Crittenden wrote:

Martin Kosek wrote:

...

4) What does NOTE: We will need to be clear that this range has nothing to do
with Trust ranges. actually mean? AFAIU, IPA should have all local ranges
covered with a local idrange range(s).


IPA ranges is completely separate from DNA ranges. You can set/modify all the
local ranges you want and it won't affect the UIDs getting assigned.


If it does not have it covered, it could happen that for example a new trust
would overlap with this user-defined local range and we would have colliding
POSIX IDs...


Hmm, that's a good point.


IMO, dnarange-set and dnanextrange-set should at first check if the range is
covered with some local idrange and only then allowed setting the new range.


I can do that as well, but again, the local ranges don't really affect the ids
we hand out via DNA.

rob


You are right, that DNA plugin is really not aware of the idranges we set in
IPA. But the idrange is still a safeguard for our POSIX IDs to not overlap with
trust ranges and I think we should respect that with ipa-replica-manage.

I wonder if there was not even a plan to increase cooperation between our
idranges and DNA plugin, maybe Sumit or Alexander knows more.


If I understand the use case and design properly, it is about
re-arranging the sub-ranges each replica can use from the original
range, which was given/created at installation time and which is also
stored as idrange in
DOMAIN.NAME_id_range,cn=ranges,cn=etc,dc=domain,dc=name with
objectclass=ipaDomainIDRange.

If the re-arrangement does not result in IDs which are outside of this
range give by the ipaDomainIDRange object, no conflicts with idranges
used by trusted domain will occur, because it is one of the task of the
idrange objects to avoid those conflicts.

If the original given range is exhausted completely and a completely new
DNA range must be created, it should be checked with ipa idrange-find
that the new range is not used and a new ipaDomainIDRange object which
reserves the local range should be added.

There are https://fedorahosted.org/freeipa/ticket/591 which can be used
to track the coordinated creation of DNA and id-range.



Honestly, I was going to skip the range stuff altogether and just focus 
on the DNA ranges.


I'm primarily interested in saving any range from a server that is going 
to be deleted. I can save that in the on-base attribute of any existing 
servers. Where I run into trouble is if all the on-base are populated, 
then we'd lose range. So I need to alert the user somehow.


This leads to wanting to be able to add that back in so I came up with 
the idea of letting users manage the DNA range directly. It may be 
possible to check the DOMAIN ranges as well but that bumps the 
complexity up considerably.


rob

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


Re: [Freeipa-devel] 0165-0174 LDAP code refactoring (Part 5)

2013-02-27 Thread Jan Cholasta

On 31.1.2013 11:03, Petr Viktorin wrote:

And hee is another batch of patches. This one is about converting the
legacy IPAdmin and raw python-ldap calls to the new wrappers.



Patch 165:

I have noticed two things that are not really related to your work, but 
here they are nonetheless:


+if self.admin_conn.get_entries(
+DN(api.env.container_ranges, self.suffix),
+ldap.SCOPE_ONELEVEL,
+objectclass=ipaDomainIDRange):

Is that a valid filter?

+if self.admin_conn.get_entries(cn=accounts, + self.suffix,
+   ldap.SCOPE_SUBTREE, id_filter):

This doesn't seem right as well, why is the DN class not used here?

Patch 167:

-conn.sasl_interactive_bind_s(None, sasl_auth)
+conn.do_sasl_gssapi_bind()

sasl_auth is unused after this change, can you please remove it as well 
(and cb_info too)?


-self.sasl_interactive_bind_s, timeout, None, SASL_AUTH)
+self.conn.sasl_interactive_bind_s, timeout, None, SASL_AUTH)

Again, this is not related to your work, but can we please rename 
SASL_AUTH to SASL_GSSAPI?


Patch 173:

-res = con.search_st(str(base),
-ldap.SCOPE_SUBTREE,
-filterstr=srcfilter,
-attrlist=attrs,
-timeout=10)
+res = con.get_entries(base, con.SCOPE_SUBTREE, srcfilter, attrs)

I assume the timeout is there for a reason, can you please keep it?

Patch 174:

-conn.modify_s(
-def_dn,
-[(ldap.MOD_REPLACE,
-'originfilter',
-disable_attr)]
-)
+entry['originfilter'] = [disable_attr]

I think you forgot to call update_entry here.

Honza

--
Jan Cholasta

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


[Freeipa-devel] [PATCH] 0007 Web UI: Realm Domains page

2013-02-27 Thread Ana Krivokapic
Add support for Realm Domains to web UI.

https://fedorahosted.org/freeipa/ticket/3407

-- 
Regards,

Ana Krivokapic
Associate Software Engineer
FreeIPA team
Red Hat Inc.

From a44867fdcd4fda8cce531d689f94f466ca6bb52d Mon Sep 17 00:00:00 2001
From: Ana Krivokapic akriv...@redhat.com
Date: Wed, 27 Feb 2013 14:49:21 +0100
Subject: [PATCH] Realm Domains page

Add support for Realm Domains to web UI.

https://fedorahosted.org/freeipa/ticket/3407
---
 install/ui/src/freeipa/app.js   |  1 +
 install/ui/src/freeipa/realmdomains.js  | 56 +
 install/ui/src/freeipa/webui.js |  3 +-
 install/ui/test/data/ipa_init.json  |  3 ++
 install/ui/test/data/ipa_init_objects.json  | 42 ++
 install/ui/test/data/realmdomains_show.json | 24 +
 ipalib/plugins/internal.py  |  3 ++
 7 files changed, 131 insertions(+), 1 deletion(-)
 create mode 100644 install/ui/src/freeipa/realmdomains.js
 create mode 100644 install/ui/test/data/realmdomains_show.json

diff --git a/install/ui/src/freeipa/app.js b/install/ui/src/freeipa/app.js
index 9d89c1aede857ddfc27ebffa306c41172ed56bca..3dcb10f493824923254636c06b715164e419cce5 100644
--- a/install/ui/src/freeipa/app.js
+++ b/install/ui/src/freeipa/app.js
@@ -41,6 +41,7 @@ define([
 './idrange',
 './netgroup',
 './policy',
+'./realmdomains',
 './rule',
 './selinux',
 './serverconfig',
diff --git a/install/ui/src/freeipa/realmdomains.js b/install/ui/src/freeipa/realmdomains.js
new file mode 100644
index ..f1dbe506d6931b72f174fc637ab567184e0b0293
--- /dev/null
+++ b/install/ui/src/freeipa/realmdomains.js
@@ -0,0 +1,56 @@
+/*  Authors:
+ *Ana Krivokapic akriv...@redhat.com
+ *
+ * Copyright (C) 2013 Red Hat
+ * see file 'COPYING' for use and warranty information
+ *
+ * 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/.
+ */
+
+define(['./ipa', './jquery', './details', './search', './association',
+'./entity'], function (IPA, $) {
+
+IPA.realmdomains = {};
+
+IPA.realmdomains.entity = function (spec) {
+
+var that = IPA.entity(spec);
+
+that.init = function () {
+that.entity_init();
+
+that.builder.details_facet({
+title: IPA.metadata.objects.realmdomains.label,
+sections: [
+{
+name: 'identity',
+label: IPA.messages.objects.realmdomains.identity,
+fields: [
+{
+name: 'associateddomain',
+type: 'multivalued'
+}
+]
+}
+],
+needs_update: true
+});
+};
+return that;
+};
+
+IPA.register('realmdomains', IPA.realmdomains.entity);
+
+return {};
+});
diff --git a/install/ui/src/freeipa/webui.js b/install/ui/src/freeipa/webui.js
index f6c3339ec4b5d3fb8a4cb547407eebf2a19b45af..04b255d8e99f59a777fce6eea230d01b48f52a9f 100644
--- a/install/ui/src/freeipa/webui.js
+++ b/install/ui/src/freeipa/webui.js
@@ -42,7 +42,8 @@ IPA.admin_navigation = function(spec) {
  {entity: 'dnsconfig'},
  {entity: 'dnsrecord', hidden:true}
  ]
-}
+},
+{entity: 'realmdomains'}
 ]},
 {name: 'policy', label: IPA.messages.tabs.policy, children: [
 {name: 'hbac', label: IPA.messages.tabs.hbac, children: [
diff --git a/install/ui/test/data/ipa_init.json b/install/ui/test/data/ipa_init.json
index c16bc992e437e6a5e1be918d46ea5dda33b97562..84663c9cb6c996ba9cf9428548a0cc860fdcd323 100644
--- a/install/ui/test/data/ipa_init.json
+++ b/install/ui/test/data/ipa_init.json
@@ -380,6 +380,9 @@
 type_ad: Active Directory domain,
 type_local: Local domain
 },
+realmdomains: {
+identity: Realm Domains
+},
 role: {
 identity: Role Settings
 },
diff --git a/install/ui/test/data/ipa_init_objects.json b/install/ui/test/data/ipa_init_objects.json

Re: [Freeipa-devel] [PATCH 0112] Make log messages related to Kerberos more verbose

2013-02-27 Thread Petr Spacek

On 12.2.2013 13:58, Petr Spacek wrote:

Hello,

 Make log messages related to Kerberos more verbose.

This change should help people supporting bind-dyndb-ldap to figure out what
is happening under covers.


Added explanatory error message for case where Kerberos context initialization 
failed.


--
Petr^2 Spacek
From 467a5d405f57e2277808c0b33b22480a3167abe4 Mon Sep 17 00:00:00 2001
From: Petr Spacek pspa...@redhat.com
Date: Tue, 12 Feb 2013 13:49:32 +0100
Subject: [PATCH] Make log messages related to Kerberos more verbose.

Signed-off-by: Petr Spacek pspa...@redhat.com
---
 src/krb5_helper.c | 38 +++---
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/src/krb5_helper.c b/src/krb5_helper.c
index ffa6938d08a37d3470dd9184be2d8ab5c604afdf..25de7f80ee56a6a2c6c6591266edf621914a10b9 100644
--- a/src/krb5_helper.c
+++ b/src/krb5_helper.c
@@ -60,15 +60,15 @@ check_credentials(krb5_context context,
 	krberr = krb5_build_principal(context, mcreds.server,
   strlen(realm), realm,
   krbtgt, realm, NULL);
-	CHECK_KRB5(context, krberr, Failed to build tgt principal);
+	CHECK_KRB5(context, krberr, Failed to build 'krbtgt/REALM' principal);
 
 	/* krb5_cc_retrieve_cred filters on both server and client */
 	mcreds.client = service;
 
 	krberr = krb5_cc_retrieve_cred(context, ccache, 0, mcreds, creds);
 	if (krberr) {
 		const char * errmsg = krb5_get_error_message(context, krberr);
-		log_debug(2, Principal not found in cred cache (%s),
+		log_debug(2, Credentials are not present in cache (%s),
 			  errmsg);
 		krb5_free_error_message(context, errmsg);
 		result = ISC_R_FAILURE;
@@ -79,7 +79,7 @@ check_credentials(krb5_context context,
 	CHECK_KRB5(context, krberr, Failed to get timeofday);
 
 	if (now  (creds.times.endtime + MIN_TIME)) {
-		log_debug(2, Credentials expired);
+		log_debug(2, Credentials in cache expired);
 		result = ISC_R_FAILURE;
 		goto cleanup;
 	}
@@ -123,42 +123,46 @@ get_krb5_tgt(isc_mem_t *mctx, const char *principal, const char *keyfile)
 	}
 
 	krberr = krb5_init_context(context);
-	if (krberr) {
-		log_error(Failed to init kerberos context);
-		return ISC_R_FAILURE;
-	}
+	/* This will blow up with older versions of Heimdal Kerberos, but
+	 * this kind of errors are not debuggable without any error message.
+	 * http://mailman.mit.edu/pipermail/kerberos/2013-February/018720.html */
+	CHECK_KRB5(NULL, krberr, Kerberos context initialization failed);
 
 	/* get credentials cache */
 	CHECK(str_new(mctx, ccname));
 	CHECK(str_sprintf(ccname, MEMORY:_ld_krb5_cc_%s, principal));
 
 	ret = setenv(KRB5CCNAME, str_buf(ccname), 1);
 	if (ret == -1) {
-		log_error(Failed to set KRB5CCNAME environment variable);
+		log_error(Failed to set KRB5CCNAME environment variable to 
+			  '%s', str_buf(ccname));
 		result = ISC_R_FAILURE;
 		goto cleanup;
 	}
 
 	krberr = krb5_cc_resolve(context, str_buf(ccname), ccache);
 	CHECK_KRB5(context, krberr,
-		   Failed to resolve ccache name %s, str_buf(ccname));
+		   Failed to resolve credentials cache name '%s',
+		   str_buf(ccname));
 
 	/* get krb5_principal from string */
 	krberr = krb5_parse_name(context, principal, kprincpw);
 	CHECK_KRB5(context, krberr,
-		   Failed to parse the principal name %s, principal);
+		   Failed to parse the principal name '%s', principal);
 
 	/* check if we already have valid credentials */
 	result = check_credentials(context, ccache, kprincpw);
 	if (result == ISC_R_SUCCESS) {
-		log_debug(2, Found valid cached credentials);
+		log_debug(2, Found valid Kerberos credentials in cache);
 		goto cleanup;
+	} else {
+		log_debug(2, Attempting to acquire new Kerberos credentials);
 	}
 
 	/* open keytab */
 	krberr = krb5_kt_resolve(context, keyfile, keytab);
 	CHECK_KRB5(context, krberr,
-		   Failed to resolve keytab file %s, keyfile);
+		   Failed to resolve keytab file '%s', keyfile);
 
 	memset(my_creds, 0, sizeof(my_creds));
 	memset(options, 0, sizeof(options));
@@ -170,15 +174,19 @@ get_krb5_tgt(isc_mem_t *mctx, const char *principal, const char *keyfile)
 	/* get tgt */
 	krberr = krb5_get_init_creds_keytab(context, my_creds, kprincpw,
 	keytab, 0, NULL, options);
-	CHECK_KRB5(context, krberr, Failed to init credentials);
+	CHECK_KRB5(context, krberr, Failed to get initial credentials (TGT) 
+using principal '%s' and keytab '%s',
+principal, keyfile);
 	my_creds_ptr = my_creds;
 
 	/* store credentials in cache */
 	krberr = krb5_cc_initialize(context, ccache, kprincpw);
-	CHECK_KRB5(context, krberr, Failed to initialize ccache);
+	CHECK_KRB5(context, krberr, Failed to initialize credentials cache 
+'%s', str_buf(ccname));
 
 	krberr = krb5_cc_store_cred(context, ccache, my_creds);
-	CHECK_KRB5(context, krberr, Failed to store ccache);
+	CHECK_KRB5(context, krberr, Failed to store credentials 
+in credentials cache '%s', str_buf(ccname));
 
 	result = ISC_R_SUCCESS;
 
-- 
1.7.11.7


[Freeipa-devel] [PATCHES] 106-113 Access raw LDAP values directly from LDAPEntry

2013-02-27 Thread Jan Cholasta

Hi,

these patches add the ability to access and manipulate raw attribute 
values as they are returned from python-ldap to the LDAPEntry class. 
This is useful for comparing entries, computing modlists for the modify 
operation, deleting values that don't match the syntax of an attribute, 
etc., because we don't need to care what syntax does a particular 
attribute use, or what Python type is used for it in the framework (raw 
values are always list of str). It should also make interaction with 
non-389 DS LDAP servers easier in the framework.


(It might be too late for this kind of changes to get into 3.2 now, I'm 
posting these patches mainly so that you are aware that they exist.)


Honza

--
Jan Cholasta
From b365ef78e5f784661261cba1c51f24703d5a3437 Mon Sep 17 00:00:00 2001
From: Jan Cholasta jchol...@redhat.com
Date: Tue, 26 Feb 2013 11:27:55 +0100
Subject: [PATCH 1/8] Make LDAPEntry a wrapper around dict rather than a dict
 subclass.

---
 ipaserver/ipaldap.py | 100 ---
 1 file changed, 64 insertions(+), 36 deletions(-)

diff --git a/ipaserver/ipaldap.py b/ipaserver/ipaldap.py
index bdc2d44..da9a60d 100644
--- a/ipaserver/ipaldap.py
+++ b/ipaserver/ipaldap.py
@@ -582,8 +582,8 @@ class IPASimpleLDAPObject(object):
 # r = result[0]
 # r[0] == r.dn
 # r[1] == r.data
-class LDAPEntry(dict):
-__slots__ = ('_conn', '_dn', '_names', '_orig')
+class LDAPEntry(object):
+__slots__ = ('_conn', '_dn', '_names', '_data', '_orig')
 
 def __init__(self, _conn, _dn=None, _obj=None, **kwargs):
 
@@ -601,8 +601,6 @@ class LDAPEntry(dict):
 
 Keyword arguments can be used to override values of specific attributes.
 
-super(LDAPEntry, self).__init__()
-
 if isinstance(_conn, LDAPEntry):
 assert _dn is None
 _dn = _conn
@@ -611,13 +609,15 @@ class LDAPEntry(dict):
 if isinstance(_dn, LDAPEntry):
 assert _obj is None
 _obj = _dn
-_dn = DN(_dn._dn)
+_dn = _dn._dn
 
 if isinstance(_obj, LDAPEntry):
+data = dict(_obj._data)
 orig = _obj._orig
 else:
 if _obj is None:
 _obj = {}
+data = {}
 orig = self
 
 assert isinstance(_conn, IPASimpleLDAPObject)
@@ -625,8 +625,9 @@ class LDAPEntry(dict):
 
 self._conn = _conn
 self._dn = _dn
-self._orig = orig
 self._names = CIDict()
+self._data = data
+self._orig = orig
 
 self.update(_obj, **kwargs)
 
@@ -655,8 +656,7 @@ class LDAPEntry(dict):
 return self._orig
 
 def __repr__(self):
-return '%s(%r, %s)' % (type(self).__name__, self._dn,
-super(LDAPEntry, self).__repr__())
+return '%s(%r, %r)' % (type(self).__name__, self._dn, self._data)
 
 def copy(self):
 return LDAPEntry(self)
@@ -664,11 +664,8 @@ class LDAPEntry(dict):
 def clone(self):
 result = LDAPEntry(self._conn, self._dn)
 
-for name in self.iterkeys():
-super(LDAPEntry, result).__setitem__(
-name, deepcopy(super(LDAPEntry, self).__getitem__(name)))
-
 result._names = deepcopy(self._names)
+result._data = deepcopy(self._data)
 if self._orig is not self:
 result._orig = self._orig.clone()
 
@@ -704,7 +701,7 @@ class LDAPEntry(dict):
 if keyname == oldname:
 self._names[altname] = name
 
-super(LDAPEntry, self).__delitem__(oldname)
+del self._data[oldname]
 else:
 self._names[name] = name
 
@@ -720,7 +717,7 @@ class LDAPEntry(dict):
 altname = altname.decode('utf-8')
 self._names[altname] = name
 
-super(LDAPEntry, self).__setitem__(name, value)
+self._data[name] = value
 
 def setdefault(self, name, default):
 if name not in self:
@@ -737,6 +734,9 @@ class LDAPEntry(dict):
 name = self._names[name]
 return name
 
+def _get(self, name):
+return self._data[self._get_attr_name(name)]
+
 def __getitem__(self, name):
 # for python-ldap tuple compatibility
 if name == 0:
@@ -744,16 +744,14 @@ class LDAPEntry(dict):
 elif name == 1:
 return self
 
-return super(LDAPEntry, self).__getitem__(self._get_attr_name(name))
+return self._get(name)
 
 def get(self, name, default=None):
 try:
-name = self._get_attr_name(name)
+return self._get(name)
 except KeyError:
 return default
 
-return super(LDAPEntry, self).get(name, default)
-
 def single_value(self, name, default=_missing):
 Return a single attribute value
 
@@ -763,8 +761,7 @@ class LDAPEntry(dict):
 If the entry is missing and default is not given, raise KeyError.
 
   

Re: [Freeipa-devel] Using the new LDAP code

2013-02-27 Thread John Dennis

On 02/27/2013 06:46 AM, Petr Viktorin wrote:

Hello,
A big refactoring of our LDAP code should be merged soon-ish now. Here's
a summary for developers.


Great, that's fabulous news and thanks for the good work!


IPA plugins traditionally use (dn, entry_attrs) pairs to represent
entries. To make that work, iterating over an LDAPEntry will, for now,
yield the DN and the entry itself. Always use keys() or values() when
iterating over an entry.


I'm trying parse what the above means, it seems to be one of the two:

1) There is still a bunch of code that continues to use 2-tuples in the 
plugins and additional work remains to converge on a single API.


2) All the code that used 2-tuples has been cleaned up and expunged, 
however if the old 2-tuple methodology was used it would still work.



--
John Dennis jden...@redhat.com

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/

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


Re: [Freeipa-devel] [PATCH] 0007 Web UI: Realm Domains page

2013-02-27 Thread Petr Vobornik

On 02/27/2013 04:20 PM, Ana Krivokapic wrote:

Add support for Realm Domains to web UI.

https://fedorahosted.org/freeipa/ticket/3407


The patch looks good, but there is a issue we don't have a precedence for.

The mod command is doing dns check for new domains. Currently we can't 
specify --force option to bypass the check.


I see two possible implementations:
1) On update, when user adds or modifies the values, a dialog would pop 
up and ask user whether he wants to force it.


2) Another option is to disable edit on the list(deletion would be still 
allowed) and move the add operation to separate action in action list.


I prefer the former. Latter might have issues with two modifications 
(delete and add) at the same time at two different places (facet and add 
dialog).

--
Petr Vobornik

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


Re: [Freeipa-devel] Using the new LDAP code

2013-02-27 Thread Jan Cholasta

Hi,

On 27.2.2013 17:09, John Dennis wrote:

IPA plugins traditionally use (dn, entry_attrs) pairs to represent
entries. To make that work, iterating over an LDAPEntry will, for now,
yield the DN and the entry itself. Always use keys() or values() when
iterating over an entry.


I'm trying parse what the above means, it seems to be one of the two:

1) There is still a bunch of code that continues to use 2-tuples in the
plugins and additional work remains to converge on a single API.

2) All the code that used 2-tuples has been cleaned up and expunged,
however if the old 2-tuple methodology was used it would still work.



it's the former, there is still code that uses 2-tuples.

Honza

--
Jan Cholasta

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


Re: [Freeipa-devel] Using the new LDAP code

2013-02-27 Thread John Dennis

On 02/27/2013 11:23 AM, Jan Cholasta wrote:

Hi,

On 27.2.2013 17:09, John Dennis wrote:

IPA plugins traditionally use (dn, entry_attrs) pairs to represent
entries. To make that work, iterating over an LDAPEntry will, for now,
yield the DN and the entry itself. Always use keys() or values() when
iterating over an entry.


I'm trying parse what the above means, it seems to be one of the two:

1) There is still a bunch of code that continues to use 2-tuples in the
plugins and additional work remains to converge on a single API.

2) All the code that used 2-tuples has been cleaned up and expunged,
however if the old 2-tuple methodology was used it would still work.



it's the former, there is still code that uses 2-tuples.


O.K. so we're still a ways away from completing the task. Would this be 
a correct summary?


Phase 1 is completed, a consistent API has been defined and implemented. 
Phase 2 is converting all the code to use the API defined in Phase 1, a 
task yet to be done.



--
John Dennis jden...@redhat.com

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/

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


Re: [Freeipa-devel] Using the new LDAP code

2013-02-27 Thread Jan Cholasta

On 27.2.2013 18:14, John Dennis wrote:

On 02/27/2013 11:23 AM, Jan Cholasta wrote:

Hi,

On 27.2.2013 17:09, John Dennis wrote:

IPA plugins traditionally use (dn, entry_attrs) pairs to represent
entries. To make that work, iterating over an LDAPEntry will, for now,
yield the DN and the entry itself. Always use keys() or values() when
iterating over an entry.


I'm trying parse what the above means, it seems to be one of the two:

1) There is still a bunch of code that continues to use 2-tuples in the
plugins and additional work remains to converge on a single API.

2) All the code that used 2-tuples has been cleaned up and expunged,
however if the old 2-tuple methodology was used it would still work.



it's the former, there is still code that uses 2-tuples.


O.K. so we're still a ways away from completing the task. Would this be
a correct summary?

Phase 1 is completed, a consistent API has been defined and implemented.
Phase 2 is converting all the code to use the API defined in Phase 1, a
task yet to be done.



Correct. But I don't think converting 2-tuples to the new API will be a 
huge task.


Honza

--
Jan Cholasta

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


Re: [Freeipa-devel] Using the new LDAP code

2013-02-27 Thread John Dennis

On 02/27/2013 12:22 PM, Jan Cholasta wrote:

On 27.2.2013 18:14, John Dennis wrote:

On 02/27/2013 11:23 AM, Jan Cholasta wrote:

Hi,

On 27.2.2013 17:09, John Dennis wrote:

IPA plugins traditionally use (dn, entry_attrs) pairs to represent
entries. To make that work, iterating over an LDAPEntry will, for now,
yield the DN and the entry itself. Always use keys() or values() when
iterating over an entry.


I'm trying parse what the above means, it seems to be one of the two:

1) There is still a bunch of code that continues to use 2-tuples in the
plugins and additional work remains to converge on a single API.

2) All the code that used 2-tuples has been cleaned up and expunged,
however if the old 2-tuple methodology was used it would still work.



it's the former, there is still code that uses 2-tuples.


O.K. so we're still a ways away from completing the task. Would this be
a correct summary?

Phase 1 is completed, a consistent API has been defined and implemented.
Phase 2 is converting all the code to use the API defined in Phase 1, a
task yet to be done.



Correct. But I don't think converting 2-tuples to the new API will be a
huge task.


Cool!


--
John Dennis jden...@redhat.com

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/

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


Re: [Freeipa-devel] DESIGN: Recover DNA Ranges

2013-02-27 Thread Sumit Bose
On Wed, Feb 27, 2013 at 09:50:21AM -0500, Rob Crittenden wrote:
 Sumit Bose wrote:
 On Mon, Feb 25, 2013 at 03:12:19PM +0100, Martin Kosek wrote:
 On 02/25/2013 03:09 PM, Rob Crittenden wrote:
 Martin Kosek wrote:
 ...
 4) What does NOTE: We will need to be clear that this range has nothing 
 to do
 with Trust ranges. actually mean? AFAIU, IPA should have all local ranges
 covered with a local idrange range(s).
 
 IPA ranges is completely separate from DNA ranges. You can set/modify all 
 the
 local ranges you want and it won't affect the UIDs getting assigned.
 
 If it does not have it covered, it could happen that for example a new 
 trust
 would overlap with this user-defined local range and we would have 
 colliding
 POSIX IDs...
 
 Hmm, that's a good point.
 
 IMO, dnarange-set and dnanextrange-set should at first check if the range 
 is
 covered with some local idrange and only then allowed setting the new 
 range.
 
 I can do that as well, but again, the local ranges don't really affect the 
 ids
 we hand out via DNA.
 
 rob
 
 You are right, that DNA plugin is really not aware of the idranges we set in
 IPA. But the idrange is still a safeguard for our POSIX IDs to not overlap 
 with
 trust ranges and I think we should respect that with ipa-replica-manage.
 
 I wonder if there was not even a plan to increase cooperation between our
 idranges and DNA plugin, maybe Sumit or Alexander knows more.
 
 If I understand the use case and design properly, it is about
 re-arranging the sub-ranges each replica can use from the original
 range, which was given/created at installation time and which is also
 stored as idrange in
 DOMAIN.NAME_id_range,cn=ranges,cn=etc,dc=domain,dc=name with
 objectclass=ipaDomainIDRange.
 
 If the re-arrangement does not result in IDs which are outside of this
 range give by the ipaDomainIDRange object, no conflicts with idranges
 used by trusted domain will occur, because it is one of the task of the
 idrange objects to avoid those conflicts.
 
 If the original given range is exhausted completely and a completely new
 DNA range must be created, it should be checked with ipa idrange-find
 that the new range is not used and a new ipaDomainIDRange object which
 reserves the local range should be added.
 
 There are https://fedorahosted.org/freeipa/ticket/591 which can be used
 to track the coordinated creation of DNA and id-range.
 
 
 Honestly, I was going to skip the range stuff altogether and just
 focus on the DNA ranges.
 
 I'm primarily interested in saving any range from a server that is
 going to be deleted. I can save that in the on-base attribute of any
 existing servers. Where I run into trouble is if all the on-base are
 populated, then we'd lose range. So I need to alert the user
 somehow.

If it is only about moving existing DNA range it is fine, because this
means that DNA will not give ID which are outside of the idrange of the
IPA domain which is created at installation or during upgrade to v3.
 
 This leads to wanting to be able to add that back in so I came up
 with the idea of letting users manage the DNA range directly. It may
 be possible to check the DOMAIN ranges as well but that bumps the
 complexity up considerably.

But it looks like dnarange-set and dnanextrange-set can also be used to
not only move existing DNA ranges but to create new DNA ranges which
will lead to ID which are not in the idrange of the IPA domain but might
be in an idrange which is used by a trusted domain. So I think a check
and a warning are desirable.

bye,
Sumit

 
 rob

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


Re: [Freeipa-devel] DESIGN: Recover DNA Ranges

2013-02-27 Thread Rob Crittenden

Sumit Bose wrote:

On Wed, Feb 27, 2013 at 09:50:21AM -0500, Rob Crittenden wrote:

Sumit Bose wrote:

On Mon, Feb 25, 2013 at 03:12:19PM +0100, Martin Kosek wrote:

On 02/25/2013 03:09 PM, Rob Crittenden wrote:

Martin Kosek wrote:

...

4) What does NOTE: We will need to be clear that this range has nothing to do
with Trust ranges. actually mean? AFAIU, IPA should have all local ranges
covered with a local idrange range(s).


IPA ranges is completely separate from DNA ranges. You can set/modify all the
local ranges you want and it won't affect the UIDs getting assigned.


If it does not have it covered, it could happen that for example a new trust
would overlap with this user-defined local range and we would have colliding
POSIX IDs...


Hmm, that's a good point.


IMO, dnarange-set and dnanextrange-set should at first check if the range is
covered with some local idrange and only then allowed setting the new range.


I can do that as well, but again, the local ranges don't really affect the ids
we hand out via DNA.

rob


You are right, that DNA plugin is really not aware of the idranges we set in
IPA. But the idrange is still a safeguard for our POSIX IDs to not overlap with
trust ranges and I think we should respect that with ipa-replica-manage.

I wonder if there was not even a plan to increase cooperation between our
idranges and DNA plugin, maybe Sumit or Alexander knows more.


If I understand the use case and design properly, it is about
re-arranging the sub-ranges each replica can use from the original
range, which was given/created at installation time and which is also
stored as idrange in
DOMAIN.NAME_id_range,cn=ranges,cn=etc,dc=domain,dc=name with
objectclass=ipaDomainIDRange.

If the re-arrangement does not result in IDs which are outside of this
range give by the ipaDomainIDRange object, no conflicts with idranges
used by trusted domain will occur, because it is one of the task of the
idrange objects to avoid those conflicts.

If the original given range is exhausted completely and a completely new
DNA range must be created, it should be checked with ipa idrange-find
that the new range is not used and a new ipaDomainIDRange object which
reserves the local range should be added.

There are https://fedorahosted.org/freeipa/ticket/591 which can be used
to track the coordinated creation of DNA and id-range.



Honestly, I was going to skip the range stuff altogether and just
focus on the DNA ranges.

I'm primarily interested in saving any range from a server that is
going to be deleted. I can save that in the on-base attribute of any
existing servers. Where I run into trouble is if all the on-base are
populated, then we'd lose range. So I need to alert the user
somehow.


If it is only about moving existing DNA range it is fine, because this
means that DNA will not give ID which are outside of the idrange of the
IPA domain which is created at installation or during upgrade to v3.


This leads to wanting to be able to add that back in so I came up
with the idea of letting users manage the DNA range directly. It may
be possible to check the DOMAIN ranges as well but that bumps the
complexity up considerably.


But it looks like dnarange-set and dnanextrange-set can also be used to
not only move existing DNA ranges but to create new DNA ranges which
will lead to ID which are not in the idrange of the IPA domain but might
be in an idrange which is used by a trusted domain. So I think a check
and a warning are desirable.


That is an excellent point. I've updated the design.

rob

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


Re: [Freeipa-devel] [PATCHES] 94-99 Read and use per-service PAC type

2013-02-27 Thread Sumit Bose
On Wed, Feb 27, 2013 at 06:48:27PM +0100, Sumit Bose wrote:
 On Wed, Feb 27, 2013 at 08:37:18AM -0500, Simo Sorce wrote:
  On Wed, 2013-02-27 at 11:58 +0100, Sumit Bose wrote:
   On Mon, Feb 25, 2013 at 04:35:20PM +0100, Martin Kosek wrote:
On 02/21/2013 04:24 PM, Sumit Bose wrote:
 Hi,
 
 this series of patches fix 
 https://fedorahosted.org/freeipa/ticket/2960
 The related design page is
 http://freeipa.org/page/V3/Read_and_use_per_service_pac_type .
 
 It was agreed while discussing the design page that the currently
 hardcoded value for the NFS service will be dropped completely, hence
 the first patch reverts to patch which added the hardcoded value.
 
 The second patch adds the needed attribute to compensate the now 
 missing
 hardcoded value.
 
 In the following three patches the PAC type is read and used
 accordingly. The last patch adds a unit test for a new function.
 
 bye,
 Sumit

I did only sanity testing to the C part of the RFE so far, but by 
reading it it
looks ok. It may be beneficial if Simo or Alexander checked if the 
ipa-kdb part
is OK.
   
   Alexander asked in irc to change strcmp() to strcasecmp() so that
   options like 'Ms-Pac' or 'none' are accepted as well.
  
  Is there a good reason to allow insensitive matching ?
  
  in LDAP you can't use true or false either for booleans, you have to use
  TRUE and FALSE, so I am not so thrilled to allow insensitive matching
  for this option as well.
 
 I'm fine with this. Alexander, any comments.

Alexander said on irc that he is fine with the strict case-sensitive
handling of the values, as long as the user input is automatically
upper-cased by the framework as it is currently done for booleans. Since
the WebUI does not allow user entered values this will only affect the
CLI.

bye,
Sumit

 
  
I will comment on the Python parts:

1) Your update check testing if there is any NFS service with 
ipakrbauthzdata
looks like a good heuristics to prevent admin frustration. Though an 
ideal
solution would require
https://fedorahosted.org/freeipa/ticket/2961
to prevent multiple updates when admin purposefully removes this 
attribute. We
may want to reference the ticket in a comment in the update plugin...
   
   I added a comment in the code and in #2961.
   


2) The upgrade plugin may get into infinite loop if ldap.find_entries 
returns
truncated result. As you do not update entries directly but only return 
update
instructions when you have no truncated results, you will loop.

To simulate this, I just created 2 NFS principals and set size_limit=1 
in your
find_entries call.
   
   Thanks for catching this, I removed the truncated logic and added a
   messages if truncated was set.
   


3) Ok, we set ipakrbauthzdata to NONE on upgrades. But what about new 
NFS
service principals added by service-add? Shouldn't we set 
ipakrbauthzdata for
new services too? As a default when no user ipakrhbauthzdata is set...
Otherwise admins could be surprised why their new NFS services do not 
work
while the others do.

Also, I think we should have this NFS culprit documented somewhere 
(i.e. why we
set ipakrbauthzdata to NONE by default). At least as a small section in 
the
online help for services plugin...
   
   I added comments to the help and the doc string in patch #100. If you
   think it is necessary to implicitly set PAC type to 'NONE' if NFS
   services are added and no type is given explicitly, I would prefer if
   you can open a new tickets so that it can be discussed independently.
   
   Thank you for the review. New versions attached.
  
  I do not think we should set the policy to NONE by default, OTOH I see
  the problem with upgrades. Maybe we should have a way to express
  additional defaults, per service type.
  
  Ie add additional values like:
  
  ipakrbAuthsData: nfs:NONE
  
  This would be a default but only for services that have the form nfs/*@*
  
  This would allow us to avoid magical special casing in the code and
  allow admins to change the overall default for specific services as
  needed.
  
  Maybe we should add a new ticket for this ?
 
 This sounds like a good compromise and will make updating much easier.
 If we agree to do it this way I can add ipakrbAuthsData: nfs:NONE and
 fix the code with this ticket. But we would need tickets for the CLI and
 WebUI to handle this new case.
 
 For the WebUI there already is
 https://fedorahosted.org/freeipa/ticket/3404 , maybe it can be extended
 to handle this new case as well?
 
 As for design documents I think I can added the needed information to
 http://freeipa.org/page/V3/Read_and_use_per_service_pac_type .
 
 bye,
 Sumit
  
  Simo.
  
  -- 
  Simo Sorce * Red Hat, Inc * New York
  
 
 

Re: [Freeipa-devel] DESIGN: Recover DNA Ranges

2013-02-27 Thread Rob Crittenden

Sumit Bose wrote:

On Wed, Feb 27, 2013 at 02:03:24PM -0500, Rob Crittenden wrote:

Sumit Bose wrote:

But it looks like dnarange-set and dnanextrange-set can also be used to
not only move existing DNA ranges but to create new DNA ranges which
will lead to ID which are not in the idrange of the IPA domain but might
be in an idrange which is used by a trusted domain. So I think a check
and a warning are desirable.


That is an excellent point. I've updated the design.


Thank you. I would like to suggest a slight edit. Instead of

That doesn't remove our responsibility to not test for overlaps in the
idranges though. We will need to verify that the manual configuration
changes do not overlap with all ranges in cn=ranges,cn=etc,$SUFFIX.

I would say

That doesn't remove our responsibility to not test for overlaps in the
idranges though. We will need to verify that the manual configuration
changes do
* not overlap with ranges from trusted domains
   (objectclass=ipatrustedaddomainrange) in cn=ranges,cn=etc,$SUFFIX, or
   reject the change otherwise.
* overlap completely with ranges from the local IPA domain
   (objectclass=ipaDomainIDRange) in cn=ranges,cn=etc,$SUFFIX, or give a
   warning otherwise which asks the user to add a new suitable idrange.

Codewise the logic could be:
- check if the new range is a subset of a local idrange, if yes, all is
   fine
- if not, check if it overlaps with an idrange of a trusted domain, if
   yes, reject
- if not, give a warning and ask to add a new idrange for the local
   domain


I took this almost verbatim but I'm more harsh. If the range is outside 
the local range and not overlapping anything I will still reject it. If 
we're going to go through the trouble of keeping them in sync we should 
be consistent.


thanks

rob

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


Re: [Freeipa-devel] DESIGN: Recover DNA Ranges

2013-02-27 Thread Simo Sorce
On Wed, 2013-02-27 at 15:00 -0500, Rob Crittenden wrote:
 Sumit Bose wrote:
  On Wed, Feb 27, 2013 at 02:03:24PM -0500, Rob Crittenden wrote:
  Sumit Bose wrote:
  But it looks like dnarange-set and dnanextrange-set can also be used to
  not only move existing DNA ranges but to create new DNA ranges which
  will lead to ID which are not in the idrange of the IPA domain but might
  be in an idrange which is used by a trusted domain. So I think a check
  and a warning are desirable.
 
  That is an excellent point. I've updated the design.
 
  Thank you. I would like to suggest a slight edit. Instead of
 
  That doesn't remove our responsibility to not test for overlaps in the
  idranges though. We will need to verify that the manual configuration
  changes do not overlap with all ranges in cn=ranges,cn=etc,$SUFFIX.
 
  I would say
 
  That doesn't remove our responsibility to not test for overlaps in the
  idranges though. We will need to verify that the manual configuration
  changes do
  * not overlap with ranges from trusted domains
 (objectclass=ipatrustedaddomainrange) in cn=ranges,cn=etc,$SUFFIX, or
 reject the change otherwise.
  * overlap completely with ranges from the local IPA domain
 (objectclass=ipaDomainIDRange) in cn=ranges,cn=etc,$SUFFIX, or give a
 warning otherwise which asks the user to add a new suitable idrange.
 
  Codewise the logic could be:
  - check if the new range is a subset of a local idrange, if yes, all is
 fine
  - if not, check if it overlaps with an idrange of a trusted domain, if
 yes, reject
  - if not, give a warning and ask to add a new idrange for the local
 domain
 
 I took this almost verbatim but I'm more harsh. If the range is outside 
 the local range and not overlapping anything I will still reject it. If 

the local ranges ^^ the admin may have already added additional ones, so
it needs to be plural.

 we're going to go through the trouble of keeping them in sync we should 
 be consistent.

Otherwise good plan.

Simo.

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

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


Re: [Freeipa-devel] DESIGN: Recover DNA Ranges

2013-02-27 Thread Sumit Bose
On Wed, Feb 27, 2013 at 03:00:10PM -0500, Rob Crittenden wrote:
 Sumit Bose wrote:
 On Wed, Feb 27, 2013 at 02:03:24PM -0500, Rob Crittenden wrote:
 Sumit Bose wrote:
 But it looks like dnarange-set and dnanextrange-set can also be used to
 not only move existing DNA ranges but to create new DNA ranges which
 will lead to ID which are not in the idrange of the IPA domain but might
 be in an idrange which is used by a trusted domain. So I think a check
 and a warning are desirable.
 
 That is an excellent point. I've updated the design.
 
 Thank you. I would like to suggest a slight edit. Instead of
 
 That doesn't remove our responsibility to not test for overlaps in the
 idranges though. We will need to verify that the manual configuration
 changes do not overlap with all ranges in cn=ranges,cn=etc,$SUFFIX.
 
 I would say
 
 That doesn't remove our responsibility to not test for overlaps in the
 idranges though. We will need to verify that the manual configuration
 changes do
 * not overlap with ranges from trusted domains
(objectclass=ipatrustedaddomainrange) in cn=ranges,cn=etc,$SUFFIX, or
reject the change otherwise.
 * overlap completely with ranges from the local IPA domain
(objectclass=ipaDomainIDRange) in cn=ranges,cn=etc,$SUFFIX, or give a
warning otherwise which asks the user to add a new suitable idrange.
 
 Codewise the logic could be:
 - check if the new range is a subset of a local idrange, if yes, all is
fine
 - if not, check if it overlaps with an idrange of a trusted domain, if
yes, reject
 - if not, give a warning and ask to add a new idrange for the local
domain
 
 I took this almost verbatim but I'm more harsh. If the range is
 outside the local range and not overlapping anything I will still
 reject it. If we're going to go through the trouble of keeping them
 in sync we should be consistent.

even better, thank you.

bye,
Sumit

 
 thanks
 
 rob
 

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


Re: [Freeipa-devel] [PATCHES] 94-99 Read and use per-service PAC type

2013-02-27 Thread Martin Kosek
On 02/27/2013 06:48 PM, Sumit Bose wrote:
 On Wed, Feb 27, 2013 at 08:37:18AM -0500, Simo Sorce wrote:
 On Wed, 2013-02-27 at 11:58 +0100, Sumit Bose wrote:
 On Mon, Feb 25, 2013 at 04:35:20PM +0100, Martin Kosek wrote:
 On 02/21/2013 04:24 PM, Sumit Bose wrote:
 Hi,

 this series of patches fix https://fedorahosted.org/freeipa/ticket/2960
 The related design page is
 http://freeipa.org/page/V3/Read_and_use_per_service_pac_type .

 It was agreed while discussing the design page that the currently
 hardcoded value for the NFS service will be dropped completely, hence
 the first patch reverts to patch which added the hardcoded value.

 The second patch adds the needed attribute to compensate the now missing
 hardcoded value.

 In the following three patches the PAC type is read and used
 accordingly. The last patch adds a unit test for a new function.

 bye,
 Sumit

 I did only sanity testing to the C part of the RFE so far, but by reading 
 it it
 looks ok. It may be beneficial if Simo or Alexander checked if the ipa-kdb 
 part
 is OK.

 Alexander asked in irc to change strcmp() to strcasecmp() so that
 options like 'Ms-Pac' or 'none' are accepted as well.

 Is there a good reason to allow insensitive matching ?

 in LDAP you can't use true or false either for booleans, you have to use
 TRUE and FALSE, so I am not so thrilled to allow insensitive matching
 for this option as well.
 
 I'm fine with this. Alexander, any comments.
 

 I will comment on the Python parts:

 1) Your update check testing if there is any NFS service with 
 ipakrbauthzdata
 looks like a good heuristics to prevent admin frustration. Though an ideal
 solution would require
 https://fedorahosted.org/freeipa/ticket/2961
 to prevent multiple updates when admin purposefully removes this 
 attribute. We
 may want to reference the ticket in a comment in the update plugin...

 I added a comment in the code and in #2961.



 2) The upgrade plugin may get into infinite loop if ldap.find_entries 
 returns
 truncated result. As you do not update entries directly but only return 
 update
 instructions when you have no truncated results, you will loop.

 To simulate this, I just created 2 NFS principals and set size_limit=1 in 
 your
 find_entries call.

 Thanks for catching this, I removed the truncated logic and added a
 messages if truncated was set.



 3) Ok, we set ipakrbauthzdata to NONE on upgrades. But what about new NFS
 service principals added by service-add? Shouldn't we set ipakrbauthzdata 
 for
 new services too? As a default when no user ipakrhbauthzdata is set...
 Otherwise admins could be surprised why their new NFS services do not work
 while the others do.

 Also, I think we should have this NFS culprit documented somewhere (i.e. 
 why we
 set ipakrbauthzdata to NONE by default). At least as a small section in the
 online help for services plugin...

 I added comments to the help and the doc string in patch #100. If you
 think it is necessary to implicitly set PAC type to 'NONE' if NFS
 services are added and no type is given explicitly, I would prefer if
 you can open a new tickets so that it can be discussed independently.

 Thank you for the review. New versions attached.

 I do not think we should set the policy to NONE by default, OTOH I see
 the problem with upgrades. Maybe we should have a way to express
 additional defaults, per service type.

 Ie add additional values like:

 ipakrbAuthsData: nfs:NONE

 This would be a default but only for services that have the form nfs/*@*

 This would allow us to avoid magical special casing in the code and
 allow admins to change the overall default for specific services as
 needed.

 Maybe we should add a new ticket for this ?
 
 This sounds like a good compromise and will make updating much easier.
 If we agree to do it this way I can add ipakrbAuthsData: nfs:NONE and
 fix the code with this ticket. But we would need tickets for the CLI and
 WebUI to handle this new case.
 
 For the WebUI there already is
 https://fedorahosted.org/freeipa/ticket/3404 , maybe it can be extended
 to handle this new case as well?
 
 As for design documents I think I can added the needed information to
 http://freeipa.org/page/V3/Read_and_use_per_service_pac_type .
 
 bye,
 Sumit

Hi Sumit,

This looks like a good idea and would prevent the magic default PAC type, yes.
Though I would not add this service-specific setting to global IPA config 
object.

I would rather like to see that in the service tree, for example as a
configuration option of the service root which could be controlled with
serviceconfig-* commands (we already have dnsconfig, trustconfig), e.g:

# ipa serviceconfig-add-pacmap --service=nfs --pac-type=NONE
# ipa serviceconfig-add-pacmap --service=cifs --pac-type=PAD
# ipa serviceconfig-show
  Default PAC Map: nfs:NONE, cifs:PAD

Martin

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com