[Freeipa-devel] Compat tree permissions

[Martin Kosek]

On 09/03/2014 07:55 AM, Alexander Bokovoy wrote:
> Switching to freeipa-devel@ since it is an important issue.
> 
> On Tue, 02 Sep 2014, Rob Crittenden wrote:
>> Chris Whittle wrote:
>>> If I do this
>>>
>>> ldapsearch -LLL -H ldaps://DOMAIN:636 -x -D
>>> "uid=mac_slave,cn=users,cn=accounts,dc=domain,dc=com" -w 'nachopassword'
>>> -b "uid=awesomeuser,cn=users,cn=accounts,dc=domain,dc=com"
>>>
>>> It works fine
>>
>> AFAICT there currently isn't a permission for the compat tree. The admin
>> user can do it via 'Admin can manage any entry" and of course DM can do
>> it because it can do anything.
>>
>> A temporary workaround would be to add an aci manually:
>>
>> dn: dc=example,dc=com
>> changetype: modify
>> add: aci
>> aci: (targetattr = "*")(target =
>> "ldap:///uid=*,cn=canlogin,cn=compat,dc=example,dc=com";)(version 3.0;acl
>> "Read canlogin compat tree";allow (compare,read,search) userdn =
>> "ldap:///all";;)
>>
>> This won't show up as a permission and will grant all authenticated
>> users read access to the canlogin compat tree. I'm assuming here this
>> contains entries keyed on uid.
> We have several use-cases for compat tree and I wonder what to do with
> completely unauthenticated case? Do we still want to support that?

Wouldn't hiding the compat tree only to authenticated users limit our Legacy
Client feature? See "ipa-advise config-redhat-nss-ldap", this advise would stop
working after this change, right?

We already show selected subset of attributes to anonymous, I think we should
continue with that:

# ipa permission-show "System: Read User Standard Attributes"
  Permission name: System: Read User Standard Attributes
  Granted rights: read, compare, search
  Effective attributes: cn, description, displayname, gecos, gidnumber,
givenname, homedirectory,
initials, ipantsecurityidentifier, loginshell, manager,
objectclass, sn, title,
uid, uidnumber
  Default attributes: displayname, description, title, objectclass, loginshell,
ipantsecurityidentifier,
  uidnumber, gidnumber, initials, manager, gecos, sn,
homedirectory, givenname, cn,
  uid
  Bind rule type: anonymous
  Subtree: cn=users,cn=accounts,dc=mkosek-fedora20,dc=test
  Type: user

> Exposing the same data anonymously over compat tree when it is available
> only for authenticated users over primary tree isn't secure.

If you check
cn=users,cn=Schema Compatibility,cn=plugins,cn=config
you would see that we only allow attributes we already expose to anonymous as
in the basic permission. So it is not that bad.

But maybe we should add a new internal "link" between standard and compat tree
permissions and issue a warning when visibility of one is changed...

Regarding missing compat permissions, I would personally add these:

System: Read User Compat Tree
System: Read Group Compat Tree
System: Read Host Compat Tree
System: Read Netgroup Compat Tree

so that they are close to their standard tree alternatives.

Martin

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

Re: [Freeipa-devel] Compat tree permissions

[Petr Viktorin]

On 09/03/2014 10:17 AM, Martin Kosek wrote:
[...]

Exposing the same data anonymously over compat tree when it is available
only for authenticated users over primary tree isn't secure.


If you check
cn=users,cn=Schema Compatibility,cn=plugins,cn=config
you would see that we only allow attributes we already expose to anonymous as
in the basic permission. So it is not that bad.


For users, yes. I assume we want the others to be authenticated only?


But maybe we should add a new internal "link" between standard and compat tree
permissions and issue a warning when visibility of one is changed...

Regarding missing compat permissions, I would personally add these:

System: Read User Compat Tree
System: Read Group Compat Tree
System: Read Host Compat Tree
System: Read Netgroup Compat Tree

so that they are close to their standard tree alternatives.



--
Petr³

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

Re: [Freeipa-devel] [PATCHES 0114-0115] DNS: allow to add root zone '.'

[Martin Basti]

On 02/09/14 17:46, Petr Spacek wrote:

On 25.8.2014 14:52, Martin Basti wrote:

Patches attached.

Ticket: https://fedorahosted.org/freeipa/ticket/4149

There is a bug in bind-dyndb-ldap (or worse in dirsrv), which cause 
the named

service is stopped after deleting zone.
Bug ticket: https://fedorahosted.org/bind-dyndb-ldap/ticket/138


Functional ACK, it works for me. It can be pushed if Python gurus are 
okay with the code.


# ipa dnszone-add .
Authoritative nameserver: @
Administrator e-mail address [hostmaster.]:
>>> Administrator e-mail address: missing address domain
Administrator e-mail address [hostmaster.]: hostmaster.
>>> Administrator e-mail address: missing address domain
Administrator e-mail address [hostmaster.]: hostmaster.test.
Nameserver IP address: 127.0.0.1
  Zone name: .
  Active zone: TRUE
  Authoritative nameserver: @
  Administrator e-mail address: hostmaster.test.
  SOA serial: 1409672572
  SOA refresh: 3600
  SOA retry: 900
  SOA expire: 1209600
  SOA minimum: 3600
  BIND update policy: grant IPA.EXAMPLE krb5-self * A; grant 
IPA.EXAMPLE krb5-self * ; grant IPA.EXAMPLE krb5-self * SSHFP;

  Dynamic update: FALSE
  Allow query: any;
  Allow transfer: none;

# ipa dnszone-mod . --expire=555
  Zone name: .
  Active zone: TRUE
  Authoritative nameserver: @
  Administrator e-mail address: hostmaster.test.
  SOA serial: 1409672710
  SOA refresh: 3600
  SOA retry: 900
  SOA expire: 555
  SOA minimum: 3600
  Allow query: any;
  Allow transfer: none;

# ipa dnszone-del .

Deleted DNS zone "."


"Administrator e-mail address: missing address domain" failure is IMHO 
acceptable in this case. It seems unlikely that root domain will have 
MX records :-)



I noticed that during patch creation, the behavior seems good to me.

--
Martin Basti

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

[Freeipa-devel] [PATCH[ 0639 permission plugin: Make --target available in the CLI

[Petr Viktorin]

This fixes https://fedorahosted.org/freeipa/ticket/4522. The API is 
already tested and the attribute is available in the UI.


Pushed as one-liner to:
ipa-4-0: 1044d09333114058bf38df501acc12708329af73
ipa-4-1: c01c61618d5e768fde0376b2f46b4887308f7a86
master: 4fbba3f7b86e4e7c8890911da650a3b55cc9a046


--
Petr³
From 91eb0650e39d1342a39321d3e5b6f81287f8e7f2 Mon Sep 17 00:00:00 2001
From: Petr Viktorin 
Date: Wed, 3 Sep 2014 10:06:58 +0200
Subject: [PATCH] permission plugin: Make --target available in the CLI

This was left out by mistake when permissions were refactored.
The API is already tested.

https://fedorahosted.org/freeipa/ticket/4522
---
 ipalib/plugins/permission.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/ipalib/plugins/permission.py b/ipalib/plugins/permission.py
index edd316be6446cd5561729e1502a837ddcf1a3831..6cacd55e1600dc85d3002125419311ce6781de0c 100644
--- a/ipalib/plugins/permission.py
+++ b/ipalib/plugins/permission.py
@@ -297,7 +297,6 @@ class permission(baseldap.LDAPObject):
 'ipapermtarget?',
 cli_name='target',
 label=_('ACI target DN'),
-flags={'no_option'}
 ),
 
 Str('memberof*',
-- 
1.9.3

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

Re: [Freeipa-devel] [PATCH[ 0639 permission plugin: Make --target available in the CLI

[Martin Kosek]

On 09/03/2014 12:17 PM, Petr Viktorin wrote:
> This fixes https://fedorahosted.org/freeipa/ticket/4522. The API is already
> tested and the attribute is available in the UI.
> 
> Pushed as one-liner to:
> ipa-4-0: 1044d09333114058bf38df501acc12708329af73
> ipa-4-1: c01c61618d5e768fde0376b2f46b4887308f7a86
> master: 4fbba3f7b86e4e7c8890911da650a3b55cc9a046

Given that we now publish this attribute in CLI, shouldn't we improve it's
label and docs? I am not sure if just plain "ACI target DN" is useful for
anyone not very familiar with ACI structure.

Maybe we should label it as just "Target DN" with docs something like "Optional
DN to apply permissions to (can be used on top of Subtree)"

Martin

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

Re: [Freeipa-devel] [PATCHES 0114-0115] DNS: allow to add root zone '.'

[Martin Kosek]

On 09/02/2014 05:46 PM, Petr Spacek wrote:
> On 25.8.2014 14:52, Martin Basti wrote:
>> Patches attached.
>>
>> Ticket: https://fedorahosted.org/freeipa/ticket/4149
>>
>> There is a bug in bind-dyndb-ldap (or worse in dirsrv), which cause the named
>> service is stopped after deleting zone.
>> Bug ticket: https://fedorahosted.org/bind-dyndb-ldap/ticket/138
> 
> Functional ACK, it works for me. It can be pushed if Python gurus are okay 
> with
> the code.

Is it safe to commit the change given that bind-dyndb-ldap still crash when "."
is removed? Wouldn't it break our CI tests?

Maybe we should wait until fixed bind-dydnb-ldap is released. Hopefully it
would be soon.

Martin

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

Re: [Freeipa-devel] [PATCHES 0111-0113] Fix NS record coexistence validation

[Martin Kosek]

On 09/02/2014 05:38 PM, Petr Spacek wrote:
> On 21.8.2014 19:21, Martin Basti wrote:
>> During work on DNSSEC we found a wrong validation of NS records
>> Patch 0113 fixes an error in tests caused by bind-dyndb-ldap bug
>> https://fedorahosted.org/bind-dyndb-ldap/ticket/123
>> Patches attached.
> 
> Functional ACK. It can be pushed if Python gurus don't see any problem.
> 

I think the patches will need a rebase before push, I cannot apply them to my
tree. The Python part itself looked good to me.

Martin

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

Re: [Freeipa-devel] Compat tree permissions

[Petr Viktorin]

On 09/03/2014 10:45 AM, Petr Viktorin wrote:

On 09/03/2014 10:17 AM, Martin Kosek wrote:
[...]

Exposing the same data anonymously over compat tree when it is available
only for authenticated users over primary tree isn't secure.


If you check
cn=users,cn=Schema Compatibility,cn=plugins,cn=config
you would see that we only allow attributes we already expose to
anonymous as
in the basic permission. So it is not that bad.


For users, yes. I assume we want the others to be authenticated only?


But maybe we should add a new internal "link" between standard and
compat tree
permissions and issue a warning when visibility of one is changed...

Regarding missing compat permissions, I would personally add these:

System: Read User Compat Tree
System: Read Group Compat Tree
System: Read Host Compat Tree
System: Read Netgroup Compat Tree


Also, what about sudoers?


--
Petr³

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

Re: [Freeipa-devel] Compat tree permissions

[Martin Kosek]

On 09/03/2014 12:32 PM, Petr Viktorin wrote:
> On 09/03/2014 10:45 AM, Petr Viktorin wrote:
>> On 09/03/2014 10:17 AM, Martin Kosek wrote:
>> [...]
 Exposing the same data anonymously over compat tree when it is available
 only for authenticated users over primary tree isn't secure.
>>>
>>> If you check
>>> cn=users,cn=Schema Compatibility,cn=plugins,cn=config
>>> you would see that we only allow attributes we already expose to
>>> anonymous as
>>> in the basic permission. So it is not that bad.
>>
>> For users, yes. I assume we want the others to be authenticated only?
>>
>>> But maybe we should add a new internal "link" between standard and
>>> compat tree
>>> permissions and issue a warning when visibility of one is changed...
>>>
>>> Regarding missing compat permissions, I would personally add these:
>>>
>>> System: Read User Compat Tree
>>> System: Read Group Compat Tree
>>> System: Read Host Compat Tree
>>> System: Read Netgroup Compat Tree
> 
> Also, what about sudoers?

What about them? I thought we have that part resolved already:

# ipa permission-find sudoers

1 permission matched

  Permission name: System: Read Sudoers compat tree
  Granted rights: read, compare, search
  Effective attributes: cn, description, objectclass, ou, sudocommand,
sudohost, sudonotafter,
sudonotbefore, sudooption, sudoorder, sudorunas,
sudorunasgroup, sudorunasuser,
sudouser
  Default attributes: sudonotafter, description, sudouser, cn, objectclass,
sudooption, sudocommand,
  sudonotbefore, sudorunas, sudorunasuser, sudohost, ou,
sudoorder, sudorunasgroup
  Bind rule type: all
  Subtree: dc=mkosek-fedora20,dc=test
  ACI target DN: ou=sudoers,dc=mkosek-fedora20,dc=test

Number of entries returned 1


Martin

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

Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.

[David Kupka]

On 09/02/2014 01:56 PM, Jan Cholasta wrote:

Dne 29.8.2014 v 14:34 David Kupka napsal(a):

Hope, I've addressed all the issues (except 9 and 11, inline). Let's go
for another round :-)

On 08/27/2014 11:05 AM, Jan Cholasta wrote:

Hi,

Dne 25.8.2014 v 15:39 David Kupka napsal(a):

On 08/19/2014 05:44 PM, Rob Crittenden wrote:

David Kupka wrote:

On 08/19/2014 09:58 AM, Martin Kosek wrote:

On 08/19/2014 09:05 AM, David Kupka wrote:

FreeIPA will use certmonger D-Bus API as discussed in this thread
https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html



This change should prevent hard-to-reproduce bugs like
https://fedorahosted.org/freeipa/ticket/4280


Thanks for this effort, the updated certmonger module looks much
better! This
will help us get rid of the non-standard communication with
certmonger.

Just couple initial comments from me by reading the code:

1) Testing needs fixed version of certmonger, right? This needs
to be
spelled
out right with the patch.

Yes, certmonger 0.75.13 and above should be fine according ticket
https://fedorahosted.org/certmonger/ticket/36. Added to patch
description.


You should update the spec to set the minimum version as well.

Sure, thanks.




2) Description text in patches is cheap, do not be afraid to use it
and
describe what you did and why. Link to the ticket is missing in the
description
as well:

Ok, increased verbosity a bit :-)



Subject: [PATCH] Use certmonger D-Bus API instead of messing with
its
files.

---


3) get_request_id API:


   criteria = (
-('cert_storage_location', dogtag_constants.ALIAS_DIR,
- certmonger.NPATH),
-('cert_nickname', nickname, None),
+('cert_storage_location', dogtag_constants.ALIAS_DIR),
+('cert_nickname', nickname),
   )
   request_id = certmonger.get_request_id(criteria)


Do we want to continue using the "criteria" object or should we
rather
switch
to normal function options? I.e. rather using

request_id = certmonger.get_request_id(cert_nickname=nickname,
cert_storage_location=dogtag_constants.ALIAS_DIR)

? It would look more consistent with other calls. I am just asking,
not insisting.

I've no preference here. It seems to be a very small change. Has
anyone
a reason to do it one way and not the other?


I think I used this criteria thing to avoid having a bazillion
optional
parameters and for future-proofing. I think at this point the list is
probably pretty stable, so I'd base it on whether you care about
having
a whole ton of optional parameters or not (it has the advantage of
self-documenting itself).


The list is probably stable but also really excessive. I don't think it
would help to have more than dozen optional parameters. So I prefer to
leave as-is and change it in future if it is wanted.


3) Starting function:


+try:
+ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
skip_output=True)
+except Exception, e:
+root_logger.error('Failed to start certmonger: %s' % e)
+raise e


I see 2 issues related to this code:
a) Do not call SYSTEMCTL directly. To be platform independent,
rather use
services.knownservices.messagebus.start() that is overridable by
someone else
porting to non-systemd platforms.

Is there anything that can't be done using
ipalib/ipapython/ipaplatform?


It can't make coffee (yet).


b) In this case, do not use "raise e", but just "raise" to keep the
exception
stack trace intact for better debugging.

Every day there's something new to learn about python or FreeIPA.


Both a) and b) should be fixed in other occasions and places.

I found only one occurence of a) issue. Is there some hidden or are
you
talking about the whole FreeIPA project?


4) Feel free to add yourself to Authors section of this module. You
refactored
it greatly to earn it :-)

Done.


You already import dbus, why also separately import DBusException?


Removed, thanks for noticing.

rob



1) The patch needs to be rebased.


I didn't notice the patch is targeted for 4.0. Can you please provide
patches for both ipa-4-0 and ipa-4-1/master?



Attached, 0008-5 works on master/ipa-4-1 and 0008-5-ipa40 works on ipa-4-0.




2) Please try to follow PEP8, at least in certmonger.py.


3) In certificate_renewal_update() in ipa-upgradeconfig you removed
ca_name from criteria.


4) IMO renaming nickname to cert_nickname in dogtag_start_tracking() and
stop_tracking() is unnecessary. We can keep calling request nicknames
"request_id" and certificate nicknames "nickname" in our APIs.


5) I think it would be better to add a docstring to
_cm_dbus_object.__init__() instead of doing this:

 # object is accesible over this DBus bus instance
 bus = None
 # DBus object path
 path = None
 # the actual DBus object
 obj = None
 # object interface name
 obj_dbus_if = None
 # object parent interface name
 parent_dbus_if = None
 # object inteface
 obj_if = None
 # prop

Re: [Freeipa-devel] Compat tree permissions

[Alexander Bokovoy]

On Wed, 03 Sep 2014, Petr Viktorin wrote:

On 09/03/2014 10:17 AM, Martin Kosek wrote:
[...]

Exposing the same data anonymously over compat tree when it is available
only for authenticated users over primary tree isn't secure.


If you check
cn=users,cn=Schema Compatibility,cn=plugins,cn=config
you would see that we only allow attributes we already expose to anonymous as
in the basic permission. So it is not that bad.


For users, yes. I assume we want the others to be authenticated only?

My point was that if we are hiding from anonymous access even the fact
that certain user or group exists, compat tree is the one where we were
leaking this information. Do we want to continue giving it out for
unauthenticated?

It is not about specific attributes but rather just the fact that
certain user or group exists.

Finally, sudo compat tree shouldn't be an issue as SSSD does use
authenticated access and native sudo.ldap plugin supports using bind DN.

The only issue is switching from unauthenticated 3.3 to authenticated
4.0.x where your existing clients using non-bound version will stop
authorizing sudo commands. And this issue is huge.

--
/ Alexander Bokovoy

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

Re: [Freeipa-devel] [PATCH[ 0639 permission plugin: Make --target available in the CLI

[Petr Viktorin]

On 09/03/2014 12:26 PM, Martin Kosek wrote:

On 09/03/2014 12:17 PM, Petr Viktorin wrote:

This fixes https://fedorahosted.org/freeipa/ticket/4522. The API is already
tested and the attribute is available in the UI.

Pushed as one-liner to:
ipa-4-0: 1044d09333114058bf38df501acc12708329af73
ipa-4-1: c01c61618d5e768fde0376b2f46b4887308f7a86
master: 4fbba3f7b86e4e7c8890911da650a3b55cc9a046


Given that we now publish this attribute in CLI, shouldn't we improve it's
label and docs? I am not sure if just plain "ACI target DN" is useful for
anyone not very familiar with ACI structure.


You're right, as always


Maybe we should label it as just "Target DN" with docs something like "Optional
DN to apply permissions to (can be used on top of Subtree)"


Instead I parenthesized the major limitation (specifying a target 
outside of subtree will give an ACI error) and the usual use case.

Does this look OK?


--
Petr³

From 2aa229ccb91eea3b2cd7638e9324ad9daa1fc157 Mon Sep 17 00:00:00 2001
From: Petr Viktorin 
Date: Wed, 3 Sep 2014 12:39:57 +0200
Subject: [PATCH] permission plugin: Improve description of the target option

https://fedorahosted.org/freeipa/ticket/4521
---
 ipalib/plugins/permission.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ipalib/plugins/permission.py b/ipalib/plugins/permission.py
index 6cacd55e1600dc85d3002125419311ce6781de0c..9e254a99b78d0475930282ccbd624708ccf704f7 100644
--- a/ipalib/plugins/permission.py
+++ b/ipalib/plugins/permission.py
@@ -296,7 +296,9 @@ class permission(baseldap.LDAPObject):
 DNParam(
 'ipapermtarget?',
 cli_name='target',
-label=_('ACI target DN'),
+label=_('Target DN'),
+doc=_('Optional DN to apply the permission to '
+  '(must be in the subtree, but may not yet exist)'),
 ),
 
 Str('memberof*',
-- 
1.9.3

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

Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.

[Martin Kosek]

On 09/03/2014 12:37 PM, David Kupka wrote:
> On 09/02/2014 01:56 PM, Jan Cholasta wrote:
>> Dne 29.8.2014 v 14:34 David Kupka napsal(a):
>>> Hope, I've addressed all the issues (except 9 and 11, inline). Let's go
>>> for another round :-)
>>>
>>> On 08/27/2014 11:05 AM, Jan Cholasta wrote:
 Hi,

 Dne 25.8.2014 v 15:39 David Kupka napsal(a):
> On 08/19/2014 05:44 PM, Rob Crittenden wrote:
>> David Kupka wrote:
>>> On 08/19/2014 09:58 AM, Martin Kosek wrote:
 On 08/19/2014 09:05 AM, David Kupka wrote:
> FreeIPA will use certmonger D-Bus API as discussed in this thread
> https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html
>
>
>
> This change should prevent hard-to-reproduce bugs like
> https://fedorahosted.org/freeipa/ticket/4280

 Thanks for this effort, the updated certmonger module looks much
 better! This
 will help us get rid of the non-standard communication with
 certmonger.

 Just couple initial comments from me by reading the code:

 1) Testing needs fixed version of certmonger, right? This needs
 to be
 spelled
 out right with the patch.
>>> Yes, certmonger 0.75.13 and above should be fine according ticket
>>> https://fedorahosted.org/certmonger/ticket/36. Added to patch
>>> description.
>>
>> You should update the spec to set the minimum version as well.
> Sure, thanks.
>>

 2) Description text in patches is cheap, do not be afraid to use it
 and
 describe what you did and why. Link to the ticket is missing in the
 description
 as well:
>>> Ok, increased verbosity a bit :-)

> Subject: [PATCH] Use certmonger D-Bus API instead of messing with
> its
> files.
>
> ---

 3) get_request_id API:

>criteria = (
> -('cert_storage_location', dogtag_constants.ALIAS_DIR,
> - certmonger.NPATH),
> -('cert_nickname', nickname, None),
> +('cert_storage_location', dogtag_constants.ALIAS_DIR),
> +('cert_nickname', nickname),
>)
>request_id = certmonger.get_request_id(criteria)

 Do we want to continue using the "criteria" object or should we
 rather
 switch
 to normal function options? I.e. rather using

 request_id = certmonger.get_request_id(cert_nickname=nickname,
 cert_storage_location=dogtag_constants.ALIAS_DIR)

 ? It would look more consistent with other calls. I am just asking,
 not insisting.
>>> I've no preference here. It seems to be a very small change. Has
>>> anyone
>>> a reason to do it one way and not the other?
>>
>> I think I used this criteria thing to avoid having a bazillion
>> optional
>> parameters and for future-proofing. I think at this point the list is
>> probably pretty stable, so I'd base it on whether you care about
>> having
>> a whole ton of optional parameters or not (it has the advantage of
>> self-documenting itself).
>>
> The list is probably stable but also really excessive. I don't think it
> would help to have more than dozen optional parameters. So I prefer to
> leave as-is and change it in future if it is wanted.

 3) Starting function:

> +try:
> +ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
> skip_output=True)
> +except Exception, e:
> +root_logger.error('Failed to start certmonger: %s' % e)
> +raise e

 I see 2 issues related to this code:
 a) Do not call SYSTEMCTL directly. To be platform independent,
 rather use
 services.knownservices.messagebus.start() that is overridable by
 someone else
 porting to non-systemd platforms.
>>> Is there anything that can't be done using
>>> ipalib/ipapython/ipaplatform?
>>
>> It can't make coffee (yet).
>>
 b) In this case, do not use "raise e", but just "raise" to keep the
 exception
 stack trace intact for better debugging.
>>> Every day there's something new to learn about python or FreeIPA.

 Both a) and b) should be fixed in other occasions and places.
>>> I found only one occurence of a) issue. Is there some hidden or are
>>> you
>>> talking about the whole FreeIPA project?

 4) Feel free to add yourself to Authors section of this module. You
 refactored
 it greatly to earn it :-)
>>> Done.
>>
>> You already import dbus, why also separately import DBusException?
>>
> Removed, thanks for not

Re: [Freeipa-devel] [PATCHES 0114-0115] DNS: allow to add root zone '.'

[Martin Basti]

On 03/09/14 12:27, Martin Kosek wrote:

On 09/02/2014 05:46 PM, Petr Spacek wrote:

On 25.8.2014 14:52, Martin Basti wrote:

Patches attached.

Ticket: https://fedorahosted.org/freeipa/ticket/4149

There is a bug in bind-dyndb-ldap (or worse in dirsrv), which cause the named
service is stopped after deleting zone.
Bug ticket: https://fedorahosted.org/bind-dyndb-ldap/ticket/138

Functional ACK, it works for me. It can be pushed if Python gurus are okay with
the code.

Is it safe to commit the change given that bind-dyndb-ldap still crash when "."
is removed? Wouldn't it break our CI tests?

Maybe we should wait until fixed bind-dydnb-ldap is released. Hopefully it
would be soon.

Martin

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

It will broke tests, don't push it until bind-dyndb-ldap is fixed.
Currently I'm testing bind-dyndb-ldap related patch.

--
Martin Basti

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

Re: [Freeipa-devel] Compat tree permissions

[Martin Kosek]

On 09/03/2014 12:39 PM, Alexander Bokovoy wrote:
> On Wed, 03 Sep 2014, Petr Viktorin wrote:
>> On 09/03/2014 10:17 AM, Martin Kosek wrote:
>> [...]
 Exposing the same data anonymously over compat tree when it is available
 only for authenticated users over primary tree isn't secure.
>>>
>>> If you check
>>> cn=users,cn=Schema Compatibility,cn=plugins,cn=config
>>> you would see that we only allow attributes we already expose to anonymous 
>>> as
>>> in the basic permission. So it is not that bad.
>>
>> For users, yes. I assume we want the others to be authenticated only?
> My point was that if we are hiding from anonymous access even the fact
> that certain user or group exists

Are we?

# ipa permission-find standard

1 permission matched

  Permission name: System: Read User Standard Attributes
  Granted rights: read, compare, search
  Effective attributes: cn, description, displayname, gecos, gidnumber,
givenname, homedirectory,
initials, ipantsecurityidentifier, loginshell, manager,
objectclass, sn, title,
uid, uidnumber
  Default attributes: displayname, description, title, objectclass, loginshell,
ipantsecurityidentifier,
  uidnumber, gidnumber, initials, manager, gecos, sn,
homedirectory, givenname, cn,
  uid
  Bind rule type: *anonymous*
  Subtree: cn=users,cn=accounts,dc=mkosek-fedora20,dc=test
  Type: user

Number of entries returned 1


# ipa permission-show "System: Read Groups"
  Permission name: System: Read Groups
  Granted rights: read, compare, search
  Effective attributes: businesscategory, cn, description, gidnumber,
ipaexternalmember,
ipantsecurityidentifier, ipauniqueid, mepmanagedby, o,
objectclass, ou, owner,
seealso
  Default attributes: businesscategory, cn, ipaexternalmember, objectclass,
ipantsecurityidentifier,
  description, gidnumber, o, mepmanagedby, ipauniqueid,
owner, ou, seealso
  Bind rule type: *anonymous*
  Subtree: cn=groups,cn=accounts,dc=mkosek-fedora20,dc=test
  Type: group

> compat tree is the one where we were
> leaking this information. Do we want to continue giving it out for
> unauthenticated?
> 
> It is not about specific attributes but rather just the fact that
> certain user or group exists.
> 
> Finally, sudo compat tree shouldn't be an issue as SSSD does use
> authenticated access and native sudo.ldap plugin supports using bind DN.
> 
> The only issue is switching from unauthenticated 3.3 to authenticated
> 4.0.x where your existing clients using non-bound version will stop
> authorizing sudo commands. And this issue is huge.

Right, this affects Legacy clients feature, makes our ipa-advise insufficient.

Martin

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

Re: [Freeipa-devel] [PATCH[ 0639 permission plugin: Make --target available in the CLI

[Martin Kosek]

On 09/03/2014 12:43 PM, Petr Viktorin wrote:
> On 09/03/2014 12:26 PM, Martin Kosek wrote:
>> On 09/03/2014 12:17 PM, Petr Viktorin wrote:
>>> This fixes https://fedorahosted.org/freeipa/ticket/4522. The API is already
>>> tested and the attribute is available in the UI.
>>>
>>> Pushed as one-liner to:
>>> ipa-4-0: 1044d09333114058bf38df501acc12708329af73
>>> ipa-4-1: c01c61618d5e768fde0376b2f46b4887308f7a86
>>> master: 4fbba3f7b86e4e7c8890911da650a3b55cc9a046
>>
>> Given that we now publish this attribute in CLI, shouldn't we improve it's
>> label and docs? I am not sure if just plain "ACI target DN" is useful for
>> anyone not very familiar with ACI structure.
> 
> You're right, as always

Heh, I have my moments :)

>> Maybe we should label it as just "Target DN" with docs something like 
>> "Optional
>> DN to apply permissions to (can be used on top of Subtree)"
> 
> Instead I parenthesized the major limitation (specifying a target outside of
> subtree will give an ACI error) and the usual use case.
> Does this look OK?

It does - ACK.

Thanks,
Martin

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

Re: [Freeipa-devel] Compat tree permissions

[Alexander Bokovoy]

On Wed, 03 Sep 2014, Martin Kosek wrote:

On 09/03/2014 12:39 PM, Alexander Bokovoy wrote:

On Wed, 03 Sep 2014, Petr Viktorin wrote:

On 09/03/2014 10:17 AM, Martin Kosek wrote:
[...]

Exposing the same data anonymously over compat tree when it is available
only for authenticated users over primary tree isn't secure.


If you check
cn=users,cn=Schema Compatibility,cn=plugins,cn=config
you would see that we only allow attributes we already expose to anonymous as
in the basic permission. So it is not that bad.


For users, yes. I assume we want the others to be authenticated only?

My point was that if we are hiding from anonymous access even the fact
that certain user or group exists


Are we?

I was under impression we've followed the change requested by some our
users to knock down anonymous access completely but I still see

# FIXME: We need to allow truly anonymous access only to NIS data for
# older clients. We need to allow broad access to most attributes only
# to authenticated users

in install/share/default-aci.ldif

Maybe it is time to do so?


4.0.x where your existing clients using non-bound version will stop
authorizing sudo commands. And this issue is huge.


Right, this affects Legacy clients feature, makes our ipa-advise insufficient.

Means we should improve the advices.
--
/ Alexander Bokovoy

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

Re: [Freeipa-devel] [PATCH 0061] Ensure ipaUserAuthTypeClass when needed on user creation

[Petr Vobornik]

On 2.9.2014 17:22, Nathaniel McCallum wrote:

On Tue, 2014-09-02 at 13:49 +0200, Petr Vobornik wrote:

On 28.8.2014 20:14, Nathaniel McCallum wrote:

On Tue, 2014-08-19 at 16:46 -0400, Nathaniel McCallum wrote:

Also, remove the attempt to load the objectClasses when absent. This
never makes sense during an add operation.

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


I still need a review for this. We are trying to get this in 4.0.2.

Nathaniel



ACK if comment below doesn't need any change:

Maybe I'm missing something, but why do we do following check: `if
'objectclass' in entry_attrs:`? Shouldn't it be always True? Since
the objectclass is set in LDAPCreate.execute. A pre-callback in an third
party plugin can remove it, but I don't think we should care.


I also thought that was odd, but I cargo-culted it to retain backwards
compatibility. Attached is a version of the patch which doesn't retain
this. I don't care which gets merged.

Nathaniel



ACK

Pushed to:
master: e26b3e14eb07baa4868109307d9d064bab4c5e0b
ipa-4-1: 480512f6db4d4b487e3376e7bca1b658d76c6f86
ipa-4-0: 4200af9b7a8c254b02034b067b29a3e66532daa2
--
Petr Vobornik

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

[Freeipa-devel] [PATCH] 0640 Add managed read permissions for compat tree

[Petr Viktorin]

Hello,
This adds managed read permissions to the compat tree.

For users it grants anonymous access; authenticated users can read 
groups, hosts and netgroups.


I'm unsure if this is what we want to do for groups, but "Read Group 
Membership" is only granted to authenticated users by default, and the 
compat tree exposes memberuid.


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

--
Petr³
From 542fa0c63840198099cf554601ebd7d6d2f30afb Mon Sep 17 00:00:00 2001
From: Petr Viktorin 
Date: Wed, 3 Sep 2014 10:54:50 +0200
Subject: [PATCH] Add managed read permissions for compat tree

https://fedorahosted.org/freeipa/ticket/4521
---
 ACI.txt|  8 
 ipalib/plugins/group.py| 10 ++
 ipalib/plugins/host.py | 10 ++
 ipalib/plugins/netgroup.py | 10 ++
 ipalib/plugins/user.py | 11 +++
 5 files changed, 49 insertions(+)

diff --git a/ACI.txt b/ACI.txt
index 6e286d8e445a757284daec30ca2ee5cc450b0db2..50fd9df9b189c83951afd64ac283b21d32ff57fe 100644
--- a/ACI.txt
+++ b/ACI.txt
@@ -50,6 +50,8 @@ dn: cn=groups,cn=accounts,dc=ipa,dc=example
 aci: (targetattr = "member")(targetfilter = "(&(!(cn=admins))(objectclass=ipausergroup))")(version 3.0;acl "permission:System: Modify Group Membership";allow (write) groupdn = "ldap:///cn=System: Modify Group Membership,cn=permissions,cn=pbac,dc=ipa,dc=example";)
 dn: cn=groups,cn=accounts,dc=ipa,dc=example
 aci: (targetattr = "cn || description || gidnumber || ipauniqueid || mepmanagedby || objectclass")(targetfilter = "(|(objectclass=ipausergroup)(objectclass=posixgroup))")(version 3.0;acl "permission:System: Modify Groups";allow (write) groupdn = "ldap:///cn=System: Modify Groups,cn=permissions,cn=pbac,dc=ipa,dc=example";)
+dn: dc=ipa,dc=example
+aci: (targetattr = "cn || memberuid || objectclass")(target = "ldap:///cn=groups,cn=compat,dc=ipa,dc=example";)(version 3.0;acl "permission:System: Read Group Compat Tree";allow (compare,read,search) userdn = "ldap:///all";;)
 dn: cn=groups,cn=accounts,dc=ipa,dc=example
 aci: (targetattr = "member || memberhost || memberof || memberuid || memberuser")(targetfilter = "(|(objectclass=ipausergroup)(objectclass=posixgroup))")(version 3.0;acl "permission:System: Read Group Membership";allow (compare,read,search) userdn = "ldap:///all";;)
 dn: cn=groups,cn=accounts,dc=ipa,dc=example
@@ -96,6 +98,8 @@ dn: cn=computers,cn=accounts,dc=ipa,dc=example
 aci: (targetattr = "ipasshpubkey")(targetfilter = "(objectclass=ipahost)")(version 3.0;acl "permission:System: Manage Host SSH Public Keys";allow (write) groupdn = "ldap:///cn=System: Manage Host SSH Public Keys,cn=permissions,cn=pbac,dc=ipa,dc=example";)
 dn: cn=computers,cn=accounts,dc=ipa,dc=example
 aci: (targetattr = "description || l || macaddress || nshardwareplatform || nshostlocation || nsosversion || userclass")(targetfilter = "(objectclass=ipahost)")(version 3.0;acl "permission:System: Modify Hosts";allow (write) groupdn = "ldap:///cn=System: Modify Hosts,cn=permissions,cn=pbac,dc=ipa,dc=example";)
+dn: dc=ipa,dc=example
+aci: (targetattr = "cn || macaddress || objectclass")(target = "ldap:///cn=computers,cn=compat,dc=ipa,dc=example";)(version 3.0;acl "permission:System: Read Host Compat Tree";allow (compare,read,search) userdn = "ldap:///all";;)
 dn: cn=computers,cn=accounts,dc=ipa,dc=example
 aci: (targetattr = "memberof")(targetfilter = "(objectclass=ipahost)")(version 3.0;acl "permission:System: Read Host Membership";allow (compare,read,search) userdn = "ldap:///all";;)
 dn: cn=computers,cn=accounts,dc=ipa,dc=example
@@ -126,6 +130,8 @@ dn: cn=ng,cn=alt,dc=ipa,dc=example
 aci: (targetattr = "externalhost || member || memberhost || memberuser")(targetfilter = "(objectclass=ipanisnetgroup)")(version 3.0;acl "permission:System: Modify Netgroup Membership";allow (write) groupdn = "ldap:///cn=System: Modify Netgroup Membership,cn=permissions,cn=pbac,dc=ipa,dc=example";)
 dn: cn=ng,cn=alt,dc=ipa,dc=example
 aci: (targetattr = "description")(targetfilter = "(objectclass=ipanisnetgroup)")(version 3.0;acl "permission:System: Modify Netgroups";allow (write) groupdn = "ldap:///cn=System: Modify Netgroups,cn=permissions,cn=pbac,dc=ipa,dc=example";)
+dn: dc=ipa,dc=example
+aci: (targetattr = "cn || mambernisnetgroup || nisnetgrouptriple || objectclass")(target = "ldap:///cn=ng,cn=compat,dc=ipa,dc=example";)(version 3.0;acl "permission:System: Read Netgroup Compat Tree";allow (compare,read,search) userdn = "ldap:///all";;)
 dn: cn=ng,cn=alt,dc=ipa,dc=example
 aci: (targetattr = "externalhost || member || memberhost || memberof || memberuser || objectclass")(targetfilter = "(objectclass=ipanisnetgroup)")(version 3.0;acl "permission:System: Read Netgroup Membership";allow (compare,read,search) userdn = "ldap:///all";;)
 dn: cn=ng,cn=alt,dc=ipa,dc=example
@@ -232,6 +238,8 @@ dn: cn=UPG Definition,cn=Definitions,cn=Managed Entries,cn=etc,dc=ipa,dc=example
 aci: (targetattr = "*")(target = "ldap:///cn=UPG Definition,cn=Definitions,cn=Managed 

Re: [Freeipa-devel] Compat tree permissions

[Martin Kosek]

On 09/03/2014 01:02 PM, Alexander Bokovoy wrote:
> On Wed, 03 Sep 2014, Martin Kosek wrote:
>> On 09/03/2014 12:39 PM, Alexander Bokovoy wrote:
>>> On Wed, 03 Sep 2014, Petr Viktorin wrote:
 On 09/03/2014 10:17 AM, Martin Kosek wrote:
 [...]
>> Exposing the same data anonymously over compat tree when it is available
>> only for authenticated users over primary tree isn't secure.
>
> If you check
> cn=users,cn=Schema Compatibility,cn=plugins,cn=config
> you would see that we only allow attributes we already expose to 
> anonymous as
> in the basic permission. So it is not that bad.

 For users, yes. I assume we want the others to be authenticated only?
>>> My point was that if we are hiding from anonymous access even the fact
>>> that certain user or group exists
>>
>> Are we?
> I was under impression we've followed the change requested by some our
> users to knock down anonymous access completely but I still see
> 
> # FIXME: We need to allow truly anonymous access only to NIS data for
> # older clients. We need to allow broad access to most attributes only
> # to authenticated users
> 
> in install/share/default-aci.ldif
> 
> Maybe it is time to do so?

Not sure about this FIXME comment, we already show most attributes to
authenticated users only, we only show the very basic POSIX attributes. You can
check yourself with

# ipa permission-find "Read User"

>>> 4.0.x where your existing clients using non-bound version will stop
>>> authorizing sudo commands. And this issue is huge.
>>
>> Right, this affects Legacy clients feature, makes our ipa-advise 
>> insufficient.
> Means we should improve the advices.

ipa-advise would then need to refer to some common system account + it's
password it would bind with. Should we file RFE? Is this a right move?

Martin

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

Re: [Freeipa-devel] Compat tree permissions

[Alexander Bokovoy]

On Wed, 03 Sep 2014, Martin Kosek wrote:

On 09/03/2014 01:02 PM, Alexander Bokovoy wrote:

On Wed, 03 Sep 2014, Martin Kosek wrote:

On 09/03/2014 12:39 PM, Alexander Bokovoy wrote:

On Wed, 03 Sep 2014, Petr Viktorin wrote:

On 09/03/2014 10:17 AM, Martin Kosek wrote:
[...]

Exposing the same data anonymously over compat tree when it is available
only for authenticated users over primary tree isn't secure.


If you check
cn=users,cn=Schema Compatibility,cn=plugins,cn=config
you would see that we only allow attributes we already expose to anonymous as
in the basic permission. So it is not that bad.


For users, yes. I assume we want the others to be authenticated only?

My point was that if we are hiding from anonymous access even the fact
that certain user or group exists


Are we?

I was under impression we've followed the change requested by some our
users to knock down anonymous access completely but I still see

# FIXME: We need to allow truly anonymous access only to NIS data for
# older clients. We need to allow broad access to most attributes only
# to authenticated users

in install/share/default-aci.ldif

Maybe it is time to do so?


Not sure about this FIXME comment, we already show most attributes to
authenticated users only, we only show the very basic POSIX attributes. You can
check yourself with

# ipa permission-find "Read User"

No, my question is not about that. We had this discussion in January
where people were asking about shutting anonymous binds to non-rootDSE.
Do we want to follow that now that we have easy way to manage
permissions and remove anonymous binds by default?

And another request was repeated to me lately at a local security meetup
by one of heavy IPA users: can we make an option to prevent
authenticated users from seeing details of other users? This is a
lock down most welcomed by governmental contractors, I've heard.
With our existing permission scheme we seem to be able to implement
that, isn't it?


4.0.x where your existing clients using non-bound version will stop
authorizing sudo commands. And this issue is huge.


Right, this affects Legacy clients feature, makes our ipa-advise insufficient.

Means we should improve the advices.


ipa-advise would then need to refer to some common system account + it's
password it would bind with. Should we file RFE? Is this a right move?

Yes, we need to file RFE and make recommendations to always have
BINDDN/BINDPW or GSSAPI_SIGN/GSSAPI_ENCRYPT/SASL_AUTH_ID/KRB5_CCNAME/USE_SASL
(see sudoers.ldap and ldap.conf manpages).
--
/ Alexander Bokovoy

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

Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.

[Jan Cholasta]

Dne 3.9.2014 v 12:45 Martin Kosek napsal(a):

On 09/03/2014 12:37 PM, David Kupka wrote:

On 09/02/2014 01:56 PM, Jan Cholasta wrote:

Dne 29.8.2014 v 14:34 David Kupka napsal(a):

Hope, I've addressed all the issues (except 9 and 11, inline). Let's go
for another round :-)

On 08/27/2014 11:05 AM, Jan Cholasta wrote:

Hi,

Dne 25.8.2014 v 15:39 David Kupka napsal(a):

On 08/19/2014 05:44 PM, Rob Crittenden wrote:

David Kupka wrote:

On 08/19/2014 09:58 AM, Martin Kosek wrote:

On 08/19/2014 09:05 AM, David Kupka wrote:

FreeIPA will use certmonger D-Bus API as discussed in this thread
https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html



This change should prevent hard-to-reproduce bugs like
https://fedorahosted.org/freeipa/ticket/4280


Thanks for this effort, the updated certmonger module looks much
better! This
will help us get rid of the non-standard communication with
certmonger.

Just couple initial comments from me by reading the code:

1) Testing needs fixed version of certmonger, right? This needs
to be
spelled
out right with the patch.

Yes, certmonger 0.75.13 and above should be fine according ticket
https://fedorahosted.org/certmonger/ticket/36. Added to patch
description.


You should update the spec to set the minimum version as well.

Sure, thanks.




2) Description text in patches is cheap, do not be afraid to use it
and
describe what you did and why. Link to the ticket is missing in the
description
as well:

Ok, increased verbosity a bit :-)



Subject: [PATCH] Use certmonger D-Bus API instead of messing with
its
files.

---


3) get_request_id API:


criteria = (
-('cert_storage_location', dogtag_constants.ALIAS_DIR,
- certmonger.NPATH),
-('cert_nickname', nickname, None),
+('cert_storage_location', dogtag_constants.ALIAS_DIR),
+('cert_nickname', nickname),
)
request_id = certmonger.get_request_id(criteria)


Do we want to continue using the "criteria" object or should we
rather
switch
to normal function options? I.e. rather using

request_id = certmonger.get_request_id(cert_nickname=nickname,
cert_storage_location=dogtag_constants.ALIAS_DIR)

? It would look more consistent with other calls. I am just asking,
not insisting.

I've no preference here. It seems to be a very small change. Has
anyone
a reason to do it one way and not the other?


I think I used this criteria thing to avoid having a bazillion
optional
parameters and for future-proofing. I think at this point the list is
probably pretty stable, so I'd base it on whether you care about
having
a whole ton of optional parameters or not (it has the advantage of
self-documenting itself).


The list is probably stable but also really excessive. I don't think it
would help to have more than dozen optional parameters. So I prefer to
leave as-is and change it in future if it is wanted.


3) Starting function:


+try:
+ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
skip_output=True)
+except Exception, e:
+root_logger.error('Failed to start certmonger: %s' % e)
+raise e


I see 2 issues related to this code:
a) Do not call SYSTEMCTL directly. To be platform independent,
rather use
services.knownservices.messagebus.start() that is overridable by
someone else
porting to non-systemd platforms.

Is there anything that can't be done using
ipalib/ipapython/ipaplatform?


It can't make coffee (yet).


b) In this case, do not use "raise e", but just "raise" to keep the
exception
stack trace intact for better debugging.

Every day there's something new to learn about python or FreeIPA.


Both a) and b) should be fixed in other occasions and places.

I found only one occurence of a) issue. Is there some hidden or are
you
talking about the whole FreeIPA project?


4) Feel free to add yourself to Authors section of this module. You
refactored
it greatly to earn it :-)

Done.


You already import dbus, why also separately import DBusException?


Removed, thanks for noticing.

rob



1) The patch needs to be rebased.


I didn't notice the patch is targeted for 4.0. Can you please provide
patches for both ipa-4-0 and ipa-4-1/master?



Attached, 0008-5 works on master/ipa-4-1 and 0008-5-ipa40 works on ipa-4-0.




2) Please try to follow PEP8, at least in certmonger.py.


3) In certificate_renewal_update() in ipa-upgradeconfig you removed
ca_name from criteria.


4) IMO renaming nickname to cert_nickname in dogtag_start_tracking() and
stop_tracking() is unnecessary. We can keep calling request nicknames
"request_id" and certificate nicknames "nickname" in our APIs.


5) I think it would be better to add a docstring to
_cm_dbus_object.__init__() instead of doing this:

  # object is accesible over this DBus bus instance
  bus = None
  # DBus object path
  path = None
  # the actual DBus object
  obj = None
  # object interface name
  obj_dbus_if = None
  # object 

Re: [Freeipa-devel] [PATCH 0279] Always use task associated ISC event instead of global inst->task

[Martin Basti]

On 02/09/14 18:54, Petr Spacek wrote:

Hello,

Always use task associated with ISC event instead of global inst->task.

This is necessary to prevent random crashes like:
REQUIRE(task->state == task_state_running) failed

https://fedorahosted.org/bind-dyndb-ldap/ticket/138



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

Functional ACK

--
Martin Basti

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

Re: [Freeipa-devel] [PATCH] 0640 Add managed read permissions for compat tree

[Petr Viktorin]

On 09/03/2014 01:27 PM, Petr Viktorin wrote:

Hello,
This adds managed read permissions to the compat tree.

For users it grants anonymous access; authenticated users can read
groups, hosts and netgroups.

I'm unsure if this is what we want to do for groups, but "Read Group
Membership" is only granted to authenticated users by default, and the
compat tree exposes memberuid.

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


Self-NACK, there's a typo (though I could swear I tested this :/)


--
Petr³

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

Re: [Freeipa-devel] Compat tree permissions

[Martin Kosek]

On 09/03/2014 02:04 PM, Alexander Bokovoy wrote:
> On Wed, 03 Sep 2014, Martin Kosek wrote:
>> On 09/03/2014 01:02 PM, Alexander Bokovoy wrote:
>>> On Wed, 03 Sep 2014, Martin Kosek wrote:
 On 09/03/2014 12:39 PM, Alexander Bokovoy wrote:
> On Wed, 03 Sep 2014, Petr Viktorin wrote:
>> On 09/03/2014 10:17 AM, Martin Kosek wrote:
>> [...]
 Exposing the same data anonymously over compat tree when it is 
 available
 only for authenticated users over primary tree isn't secure.
>>>
>>> If you check
>>> cn=users,cn=Schema Compatibility,cn=plugins,cn=config
>>> you would see that we only allow attributes we already expose to
>>> anonymous as
>>> in the basic permission. So it is not that bad.
>>
>> For users, yes. I assume we want the others to be authenticated only?
> My point was that if we are hiding from anonymous access even the fact
> that certain user or group exists

 Are we?
>>> I was under impression we've followed the change requested by some our
>>> users to knock down anonymous access completely but I still see
>>>
>>> # FIXME: We need to allow truly anonymous access only to NIS data for
>>> # older clients. We need to allow broad access to most attributes only
>>> # to authenticated users
>>>
>>> in install/share/default-aci.ldif
>>>
>>> Maybe it is time to do so?
>>
>> Not sure about this FIXME comment, we already show most attributes to
>> authenticated users only, we only show the very basic POSIX attributes. You 
>> can
>> check yourself with
>>
>> # ipa permission-find "Read User"
> No, my question is not about that. We had this discussion in January
> where people were asking about shutting anonymous binds to non-rootDSE.
> Do we want to follow that now that we have easy way to manage
> permissions and remove anonymous binds by default?

Now it is easy to change different parts of your DIT to different view levels.
You can easily turn off anonymous access to users without having to disable
anonymous access on DS cn=config level.

> And another request was repeated to me lately at a local security meetup
> by one of heavy IPA users: can we make an option to prevent
> authenticated users from seeing details of other users? This is a
> lock down most welcomed by governmental contractors, I've heard.
> With our existing permission scheme we seem to be able to implement
> that, isn't it?

Hm, it would be difficult. SSSD still needs to be able to read all that, so all
host/FQDN principals need to access that. So you would first need to have a
hostgroup with all hosts to be able to point permissions at them.

> 4.0.x where your existing clients using non-bound version will stop
> authorizing sudo commands. And this issue is huge.

 Right, this affects Legacy clients feature, makes our ipa-advise 
 insufficient.
>>> Means we should improve the advices.
>>
>> ipa-advise would then need to refer to some common system account + it's
>> password it would bind with. Should we file RFE? Is this a right move?
> Yes, we need to file RFE and make recommendations to always have
> BINDDN/BINDPW or GSSAPI_SIGN/GSSAPI_ENCRYPT/SASL_AUTH_ID/KRB5_CCNAME/USE_SASL
> (see sudoers.ldap and ldap.conf manpages).

Ok, please file the ticket then.

Martin

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

Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.

[Martin Kosek]

On 09/03/2014 02:07 PM, Jan Cholasta wrote:
> Dne 3.9.2014 v 12:45 Martin Kosek napsal(a):
>> On 09/03/2014 12:37 PM, David Kupka wrote:
>>> On 09/02/2014 01:56 PM, Jan Cholasta wrote:
 Dne 29.8.2014 v 14:34 David Kupka napsal(a):
> Hope, I've addressed all the issues (except 9 and 11, inline). Let's go
> for another round :-)
>
> On 08/27/2014 11:05 AM, Jan Cholasta wrote:
>> Hi,
>>
>> Dne 25.8.2014 v 15:39 David Kupka napsal(a):
>>> On 08/19/2014 05:44 PM, Rob Crittenden wrote:
 David Kupka wrote:
> On 08/19/2014 09:58 AM, Martin Kosek wrote:
>> On 08/19/2014 09:05 AM, David Kupka wrote:
>>> FreeIPA will use certmonger D-Bus API as discussed in this thread
>>> https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html
>>>
>>>
>>>
>>> This change should prevent hard-to-reproduce bugs like
>>> https://fedorahosted.org/freeipa/ticket/4280
>>
>> Thanks for this effort, the updated certmonger module looks much
>> better! This
>> will help us get rid of the non-standard communication with
>> certmonger.
>>
>> Just couple initial comments from me by reading the code:
>>
>> 1) Testing needs fixed version of certmonger, right? This needs
>> to be
>> spelled
>> out right with the patch.
> Yes, certmonger 0.75.13 and above should be fine according ticket
> https://fedorahosted.org/certmonger/ticket/36. Added to patch
> description.

 You should update the spec to set the minimum version as well.
>>> Sure, thanks.

>>
>> 2) Description text in patches is cheap, do not be afraid to use it
>> and
>> describe what you did and why. Link to the ticket is missing in the
>> description
>> as well:
> Ok, increased verbosity a bit :-)
>>
>>> Subject: [PATCH] Use certmonger D-Bus API instead of messing with
>>> its
>>> files.
>>>
>>> ---
>>
>> 3) get_request_id API:
>>
>>> criteria = (
>>> -('cert_storage_location', dogtag_constants.ALIAS_DIR,
>>> - certmonger.NPATH),
>>> -('cert_nickname', nickname, None),
>>> +('cert_storage_location', dogtag_constants.ALIAS_DIR),
>>> +('cert_nickname', nickname),
>>> )
>>> request_id = certmonger.get_request_id(criteria)
>>
>> Do we want to continue using the "criteria" object or should we
>> rather
>> switch
>> to normal function options? I.e. rather using
>>
>> request_id = certmonger.get_request_id(cert_nickname=nickname,
>> cert_storage_location=dogtag_constants.ALIAS_DIR)
>>
>> ? It would look more consistent with other calls. I am just asking,
>> not insisting.
> I've no preference here. It seems to be a very small change. Has
> anyone
> a reason to do it one way and not the other?

 I think I used this criteria thing to avoid having a bazillion
 optional
 parameters and for future-proofing. I think at this point the list is
 probably pretty stable, so I'd base it on whether you care about
 having
 a whole ton of optional parameters or not (it has the advantage of
 self-documenting itself).

>>> The list is probably stable but also really excessive. I don't think it
>>> would help to have more than dozen optional parameters. So I prefer to
>>> leave as-is and change it in future if it is wanted.
>>
>> 3) Starting function:
>>
>>> +try:
>>> +ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
>>> skip_output=True)
>>> +except Exception, e:
>>> +root_logger.error('Failed to start certmonger: %s' % e)
>>> +raise e
>>
>> I see 2 issues related to this code:
>> a) Do not call SYSTEMCTL directly. To be platform independent,
>> rather use
>> services.knownservices.messagebus.start() that is overridable by
>> someone else
>> porting to non-systemd platforms.
> Is there anything that can't be done using
> ipalib/ipapython/ipaplatform?

 It can't make coffee (yet).

>> b) In this case, do not use "raise e", but just "raise" to keep the
>> exception
>> stack trace intact for better debugging.
> Every day there's something new to learn about python or FreeIPA.
>>
>> Both a) and b) should be fixed in other occasions and places.
> I found only one occurence of a) issue. Is there some hidden or are
>>

Re: [Freeipa-devel] [PATCH 0107-0108] Fix DNS wildcard validation

[Martin Basti]

On 02/09/14 17:33, Petr Spacek wrote:

On 21.8.2014 10:58, Martin Basti wrote:

On 21/08/14 08:43, Petr Spacek wrote:

On 20.8.2014 17:37, Martin Basti wrote:

+# dissallowed wildcard (RFC 4592)
+no_wildcard_rtypes = ['CNAME', 'DNAME', 'DS', 'NS']

NACK

http://tools.ietf.org/html/rfc4592#section-4.3 doesn't forbid CNAME 
with
wildcard owner name. This subsection is is just a "note" for 
implementers

about proper wildcard handling.

Sorry :-)


Thank you!

Updated patches attached.



# ipa dnsrecord-add  ipa.example. '*' --ns-rec='ns'
ipa: ERROR: invalid 'idnsname': owner of DNAME, DS, NS records should 
not be a wildcard domain name (RFC 4592)


It would be nice to have more specific reference to RFC: 'RFC 4592 
section 4'.


CondACK: It can be pushed if you amend the error message.


Updated patch attached.
Please push to branches: ipa 4.0.x, 4.1, master

--
Martin Basti

From 4911277dc84f29359f308f3df4a7a9aa29600bc9 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 20 Aug 2014 15:14:12 +0200
Subject: [PATCH 1/2] FIX DNS wildcard records (RFC4592)

Make validation more strict

* DS, NS, DNAME owners should not be a wildcard domanin name
* zone name should not be a wildcard domain name

Ticket: https://fedorahosted.org/freeipa/ticket/4488
---
 ipalib/plugins/dns.py | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
index fdcccb0b74a2b044a1ad917d22d2fe9696d7584c..aba30dd3f3ca2f06058a05f5c0350e1a3e8eb2e5 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -489,6 +489,14 @@ def _hostname_validator(ugettext, value):
 
 return None
 
+def _no_wildcard_validator(ugettext, value):
+"""Disallow usage of wildcards as RFC 4592 section 4 recommends
+"""
+assert isinstance(value, DNSName)
+if value.is_wild():
+return _('should not be a wildcard domain name (RFC 4592 section 4)')
+return None
+
 def is_forward_record(zone, str_address):
 addr = netaddr.IPAddress(str_address)
 if addr.version == 4:
@@ -1731,6 +1739,7 @@ class DNSZoneBase(LDAPObject):
 
 takes_params = (
 DNSNameParam('idnsname',
+_no_wildcard_validator,  # RFC 4592 section 4
 only_absolute=True,
 cli_name='name',
 label=_('Zone name'),
@@ -2619,6 +2628,19 @@ class dnsrecord(LDAPObject):
 error=unicode(_('out-of-zone data: record name must '
 'be a subdomain of the zone or a '
 'relative name')))
+# dissallowed wildcard (RFC 4592 section 4)
+no_wildcard_rtypes = ['DNAME', 'DS', 'NS']
+if (keys[-1].is_wild() and
+any(entry_attrs.get('%srecord' % r.lower())
+for r in no_wildcard_rtypes)
+):
+raise errors.ValidationError(
+name='idnsname',
+error=(_('owner of %(types)s records '
+'should not be a wildcard domain name (RFC 4592 section 4)') %
+{'types': ', '.join(no_wildcard_rtypes)}
+)
+)
 
 def _ptrrecord_pre_callback(self, ldap, dn, entry_attrs, *keys, **options):
 assert isinstance(dn, DN)
-- 
1.8.3.1

From b896636140c320b93d87f86578a8381fd3fd9091 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 20 Aug 2014 17:26:34 +0200
Subject: [PATCH 2/2] Tests: DNS wildcard records

Ticket: https://fedorahosted.org/freeipa/ticket/4488
---
 ipatests/test_xmlrpc/test_dns_plugin.py | 47 -
 1 file changed, 46 insertions(+), 1 deletion(-)

diff --git a/ipatests/test_xmlrpc/test_dns_plugin.py b/ipatests/test_xmlrpc/test_dns_plugin.py
index 50b4d2ec7bf4d55f7d138f45993184f1bf7790bd..1876e1440eda6b357810765fe92712d750ce36f3 100644
--- a/ipatests/test_xmlrpc/test_dns_plugin.py
+++ b/ipatests/test_xmlrpc/test_dns_plugin.py
@@ -263,6 +263,7 @@ zone_findtest_forward = u'forward.find.test.'
 zone_findtest_forward_dnsname = DNSName(zone_findtest_forward)
 zone_findtest_forward_dn = DN(('idnsname', zone_findtest_forward), api.env.container_dns, api.env.basedn)
 
+zone_fw_wildcard = u'*.wildcardforwardzone.test.'
 
 class test_dns(Declarative):
 
@@ -289,7 +290,8 @@ class test_dns(Declarative):
  revzone3_classless1, revzone3_classless2,
  idnzone1, revidnzone1, zone_findtest_master],
 {'continue': True}),
-('dnsforwardzone_del', [fwzone1, zone_findtest_forward],
+('dnsforwardzone_del', [fwzone1, zone_findtest_forward,
+zone_fw_wildcard],
 {'continue': True}),
 ('dnsconfig_mod', [], {'idnsforwarders' : None,
'idnsforwardpolicy' : None,
@@ -2736,6 +2738,39 @@ class test_dns(Declarative):
 
 
 dict(
+desc='Try to add NS record to wildcard owner %r in zone %r' % (wildcard_rec1, zone1),

Re: [Freeipa-devel] Compat tree permissions

[Alexander Bokovoy]

On Wed, 03 Sep 2014, Martin Kosek wrote:

On 09/03/2014 02:04 PM, Alexander Bokovoy wrote:

On Wed, 03 Sep 2014, Martin Kosek wrote:

On 09/03/2014 01:02 PM, Alexander Bokovoy wrote:

On Wed, 03 Sep 2014, Martin Kosek wrote:

On 09/03/2014 12:39 PM, Alexander Bokovoy wrote:

On Wed, 03 Sep 2014, Petr Viktorin wrote:

On 09/03/2014 10:17 AM, Martin Kosek wrote:
[...]

Exposing the same data anonymously over compat tree when it is available
only for authenticated users over primary tree isn't secure.


If you check
cn=users,cn=Schema Compatibility,cn=plugins,cn=config
you would see that we only allow attributes we already expose to
anonymous as
in the basic permission. So it is not that bad.


For users, yes. I assume we want the others to be authenticated only?

My point was that if we are hiding from anonymous access even the fact
that certain user or group exists


Are we?

I was under impression we've followed the change requested by some our
users to knock down anonymous access completely but I still see

# FIXME: We need to allow truly anonymous access only to NIS data for
# older clients. We need to allow broad access to most attributes only
# to authenticated users

in install/share/default-aci.ldif

Maybe it is time to do so?


Not sure about this FIXME comment, we already show most attributes to
authenticated users only, we only show the very basic POSIX attributes. You can
check yourself with

# ipa permission-find "Read User"

No, my question is not about that. We had this discussion in January
where people were asking about shutting anonymous binds to non-rootDSE.
Do we want to follow that now that we have easy way to manage
permissions and remove anonymous binds by default?


Now it is easy to change different parts of your DIT to different view levels.
You can easily turn off anonymous access to users without having to disable
anonymous access on DS cn=config level.

Yep, that's good and need to be documented, we don't have to push people
to cn=config changes like documentation says now.




And another request was repeated to me lately at a local security meetup
by one of heavy IPA users: can we make an option to prevent
authenticated users from seeing details of other users? This is a
lock down most welcomed by governmental contractors, I've heard.
With our existing permission scheme we seem to be able to implement
that, isn't it?


Hm, it would be difficult. SSSD still needs to be able to read all that, so all
host/FQDN principals need to access that. So you would first need to have a
hostgroup with all hosts to be able to point permissions at them.

Can we find a better solution here? I.e. make a filter that considers
selfdn and denies access to other user and group accounts unless selfdn
is memberof those groups?

If current ACI syntax doesn't allow this, what can we do to extend the
syntax? I see value in such changes as they will allow also to proceed
to proper containerized setups (multiple trees in the same directory,
invisible to each other).



4.0.x where your existing clients using non-bound version will stop
authorizing sudo commands. And this issue is huge.


Right, this affects Legacy clients feature, makes our ipa-advise insufficient.

Means we should improve the advices.


ipa-advise would then need to refer to some common system account + it's
password it would bind with. Should we file RFE? Is this a right move?

Yes, we need to file RFE and make recommendations to always have
BINDDN/BINDPW or GSSAPI_SIGN/GSSAPI_ENCRYPT/SASL_AUTH_ID/KRB5_CCNAME/USE_SASL
(see sudoers.ldap and ldap.conf manpages).


Ok, please file the ticket then.

Will do.

--
/ Alexander Bokovoy

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

Re: [Freeipa-devel] [PATCH] 0640 Add managed read permissions for compat tree

[Petr Viktorin]

On 09/03/2014 02:27 PM, Petr Viktorin wrote:

On 09/03/2014 01:27 PM, Petr Viktorin wrote:

Hello,
This adds managed read permissions to the compat tree.

For users it grants anonymous access; authenticated users can read
groups, hosts and netgroups.

I'm unsure if this is what we want to do for groups, but "Read Group
Membership" is only granted to authenticated users by default, and the
compat tree exposes memberuid.

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


Self-NACK, there's a typo (though I could swear I tested this :/)




Fixed patch attached.

--
Petr³
From a283e18e050f4b6d2a2d9ea4909d8f75a994b42b Mon Sep 17 00:00:00 2001
From: Petr Viktorin 
Date: Wed, 3 Sep 2014 10:54:50 +0200
Subject: [PATCH] Add managed read permissions for compat tree

https://fedorahosted.org/freeipa/ticket/4521
---
 ACI.txt|  8 
 ipalib/plugins/group.py| 10 ++
 ipalib/plugins/host.py | 10 ++
 ipalib/plugins/netgroup.py | 10 ++
 ipalib/plugins/user.py | 11 +++
 5 files changed, 49 insertions(+)

diff --git a/ACI.txt b/ACI.txt
index 6e286d8e445a757284daec30ca2ee5cc450b0db2..ea34e5e7e105c34b30a2a9c2d44bb360938b48d6 100644
--- a/ACI.txt
+++ b/ACI.txt
@@ -50,6 +50,8 @@ dn: cn=groups,cn=accounts,dc=ipa,dc=example
 aci: (targetattr = "member")(targetfilter = "(&(!(cn=admins))(objectclass=ipausergroup))")(version 3.0;acl "permission:System: Modify Group Membership";allow (write) groupdn = "ldap:///cn=System: Modify Group Membership,cn=permissions,cn=pbac,dc=ipa,dc=example";)
 dn: cn=groups,cn=accounts,dc=ipa,dc=example
 aci: (targetattr = "cn || description || gidnumber || ipauniqueid || mepmanagedby || objectclass")(targetfilter = "(|(objectclass=ipausergroup)(objectclass=posixgroup))")(version 3.0;acl "permission:System: Modify Groups";allow (write) groupdn = "ldap:///cn=System: Modify Groups,cn=permissions,cn=pbac,dc=ipa,dc=example";)
+dn: dc=ipa,dc=example
+aci: (targetattr = "cn || memberuid || objectclass")(target = "ldap:///cn=groups,cn=compat,dc=ipa,dc=example";)(version 3.0;acl "permission:System: Read Group Compat Tree";allow (compare,read,search) userdn = "ldap:///all";;)
 dn: cn=groups,cn=accounts,dc=ipa,dc=example
 aci: (targetattr = "member || memberhost || memberof || memberuid || memberuser")(targetfilter = "(|(objectclass=ipausergroup)(objectclass=posixgroup))")(version 3.0;acl "permission:System: Read Group Membership";allow (compare,read,search) userdn = "ldap:///all";;)
 dn: cn=groups,cn=accounts,dc=ipa,dc=example
@@ -96,6 +98,8 @@ dn: cn=computers,cn=accounts,dc=ipa,dc=example
 aci: (targetattr = "ipasshpubkey")(targetfilter = "(objectclass=ipahost)")(version 3.0;acl "permission:System: Manage Host SSH Public Keys";allow (write) groupdn = "ldap:///cn=System: Manage Host SSH Public Keys,cn=permissions,cn=pbac,dc=ipa,dc=example";)
 dn: cn=computers,cn=accounts,dc=ipa,dc=example
 aci: (targetattr = "description || l || macaddress || nshardwareplatform || nshostlocation || nsosversion || userclass")(targetfilter = "(objectclass=ipahost)")(version 3.0;acl "permission:System: Modify Hosts";allow (write) groupdn = "ldap:///cn=System: Modify Hosts,cn=permissions,cn=pbac,dc=ipa,dc=example";)
+dn: dc=ipa,dc=example
+aci: (targetattr = "cn || macaddress || objectclass")(target = "ldap:///cn=computers,cn=compat,dc=ipa,dc=example";)(version 3.0;acl "permission:System: Read Host Compat Tree";allow (compare,read,search) userdn = "ldap:///all";;)
 dn: cn=computers,cn=accounts,dc=ipa,dc=example
 aci: (targetattr = "memberof")(targetfilter = "(objectclass=ipahost)")(version 3.0;acl "permission:System: Read Host Membership";allow (compare,read,search) userdn = "ldap:///all";;)
 dn: cn=computers,cn=accounts,dc=ipa,dc=example
@@ -126,6 +130,8 @@ dn: cn=ng,cn=alt,dc=ipa,dc=example
 aci: (targetattr = "externalhost || member || memberhost || memberuser")(targetfilter = "(objectclass=ipanisnetgroup)")(version 3.0;acl "permission:System: Modify Netgroup Membership";allow (write) groupdn = "ldap:///cn=System: Modify Netgroup Membership,cn=permissions,cn=pbac,dc=ipa,dc=example";)
 dn: cn=ng,cn=alt,dc=ipa,dc=example
 aci: (targetattr = "description")(targetfilter = "(objectclass=ipanisnetgroup)")(version 3.0;acl "permission:System: Modify Netgroups";allow (write) groupdn = "ldap:///cn=System: Modify Netgroups,cn=permissions,cn=pbac,dc=ipa,dc=example";)
+dn: dc=ipa,dc=example
+aci: (targetattr = "cn || membernisnetgroup || nisnetgrouptriple || objectclass")(target = "ldap:///cn=ng,cn=compat,dc=ipa,dc=example";)(version 3.0;acl "permission:System: Read Netgroup Compat Tree";allow (compare,read,search) userdn = "ldap:///all";;)
 dn: cn=ng,cn=alt,dc=ipa,dc=example
 aci: (targetattr = "externalhost || member || memberhost || memberof || memberuser || objectclass")(targetfilter = "(objectclass=ipanisnetgroup)")(version 3.0;acl "permission:System: Read Netgroup Membership";allow (compare,read,search) userdn = "ldap:///all";;)
 dn: cn=ng,cn=alt,dc=ipa,dc=example
@@ -232

Re: [Freeipa-devel] Compat tree permissions

[Rob Crittenden]

Alexander Bokovoy wrote:
> On Wed, 03 Sep 2014, Martin Kosek wrote:
>> On 09/03/2014 02:04 PM, Alexander Bokovoy wrote:
>>> On Wed, 03 Sep 2014, Martin Kosek wrote:
 On 09/03/2014 01:02 PM, Alexander Bokovoy wrote:
> On Wed, 03 Sep 2014, Martin Kosek wrote:
>> On 09/03/2014 12:39 PM, Alexander Bokovoy wrote:
>>> On Wed, 03 Sep 2014, Petr Viktorin wrote:
 On 09/03/2014 10:17 AM, Martin Kosek wrote:
 [...]
>> Exposing the same data anonymously over compat tree when it is
>> available
>> only for authenticated users over primary tree isn't secure.
>
> If you check
> cn=users,cn=Schema Compatibility,cn=plugins,cn=config
> you would see that we only allow attributes we already expose to
> anonymous as
> in the basic permission. So it is not that bad.

 For users, yes. I assume we want the others to be authenticated
 only?
>>> My point was that if we are hiding from anonymous access even the
>>> fact
>>> that certain user or group exists
>>
>> Are we?
> I was under impression we've followed the change requested by some our
> users to knock down anonymous access completely but I still see
>
> # FIXME: We need to allow truly anonymous access only to NIS data for
> # older clients. We need to allow broad access to most attributes only
> # to authenticated users
>
> in install/share/default-aci.ldif
>
> Maybe it is time to do so?

 Not sure about this FIXME comment, we already show most attributes to
 authenticated users only, we only show the very basic POSIX
 attributes. You can
 check yourself with

 # ipa permission-find "Read User"
>>> No, my question is not about that. We had this discussion in January
>>> where people were asking about shutting anonymous binds to non-rootDSE.
>>> Do we want to follow that now that we have easy way to manage
>>> permissions and remove anonymous binds by default?
>>
>> Now it is easy to change different parts of your DIT to different view
>> levels.
>> You can easily turn off anonymous access to users without having to
>> disable
>> anonymous access on DS cn=config level.
> Yep, that's good and need to be documented, we don't have to push people
> to cn=config changes like documentation says now.
> 
>>
>>> And another request was repeated to me lately at a local security meetup
>>> by one of heavy IPA users: can we make an option to prevent
>>> authenticated users from seeing details of other users? This is a
>>> lock down most welcomed by governmental contractors, I've heard.
>>> With our existing permission scheme we seem to be able to implement
>>> that, isn't it?
>>
>> Hm, it would be difficult. SSSD still needs to be able to read all
>> that, so all
>> host/FQDN principals need to access that. So you would first need to
>> have a
>> hostgroup with all hosts to be able to point permissions at them.
> Can we find a better solution here? I.e. make a filter that considers
> selfdn and denies access to other user and group accounts unless selfdn
> is memberof those groups?
> 
> If current ACI syntax doesn't allow this, what can we do to extend the
> syntax? I see value in such changes as they will allow also to proceed
> to proper containerized setups (multiple trees in the same directory,
> invisible to each other).
> 
> 
>>> 4.0.x where your existing clients using non-bound version will stop
>>> authorizing sudo commands. And this issue is huge.
>>
>> Right, this affects Legacy clients feature, makes our ipa-advise
>> insufficient.
> Means we should improve the advices.

 ipa-advise would then need to refer to some common system account +
 it's
 password it would bind with. Should we file RFE? Is this a right move?
>>> Yes, we need to file RFE and make recommendations to always have
>>> BINDDN/BINDPW or
>>> GSSAPI_SIGN/GSSAPI_ENCRYPT/SASL_AUTH_ID/KRB5_CCNAME/USE_SASL
>>> (see sudoers.ldap and ldap.conf manpages).
>>
>> Ok, please file the ticket then.
> Will do.
> 

Remember that most of the NIS/legacy systems that would actually use
this are non-Linux so keep that in mind as you tighten things up.
ipa-advise doesn't cover the cases of AIX, Solaris and HP/ux.

rob

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

Re: [Freeipa-devel] [PATCH] 0010 Add 'host' setting into default.conf configuration file

[Nalin Dahyabhai]

On Tue, Sep 02, 2014 at 10:18:12AM +0200, Jan Cholasta wrote:
> Dne 27.8.2014 v 16:49 David Kupka napsal(a):
> >On 08/27/2014 11:22 AM, Jan Cholasta wrote:
> >>Dne 26.8.2014 v 15:55 Rob Crittenden napsal(a):
> >>>David Kupka wrote:
> On 08/26/2014 03:08 PM, Jan Cholasta wrote:
> >Hi,
> >
> >Dne 26.8.2014 v 13:01 David Kupka napsal(a):
> >>https://fedorahosted.org/freeipa/ticket/4481
> >
> >Doing this will break ipa-client-automount and ipa-certupdate, because
> >they assume that api.env.host contains the hostname of the local
> >system
> >(which is the default value).
> 
> It looked suspiciously simple so I could expect that there is some
> catch.
> >
> >There is obviously some confusion about what the option should
> >represent
> >(documentation says server hostname, code does client hostname),
> >IMO we
> >should resolve that first.
> 
> Ok, are there any suggestions? What is the desired state?
> >>>
> >>>AIUI the server option is deprecated because it wasn't being used, not
> >>>that it needed to be replaced. I believe that in most cases the server
> >>>name is pulled from the xmlrpc_uri.
> >>
> >>Yes, that's what the ticket says:
> >>.
> >
> >Ok, adding 'host' entry with local host name.
> >>>
> >>>host has always meant the local host name.
> >>>
> >>>I think the man page is wrong.
> >>
> >>+1
> >>
> >Fixing the line in man page.
> >>>
> >>>rob
> 
> ACK as long as this works for Nalin.

The other half of this was cases where there's no ldap_uri set.  Just so
there's no confusion, if ldap_uri and/or server_uri are not set, what
are the recommended fallback settings that should be used for
constructing them?  I suspect it's "server", then "host", which is the
reverse of the order that they're currently being consulted, but I
figured I'd ask while we're all here.

Thanks,

Nalin

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

Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.

[Nalin Dahyabhai]

On Wed, Sep 03, 2014 at 02:34:44PM +0200, Martin Kosek wrote:
> On 09/03/2014 02:07 PM, Jan Cholasta wrote:
> > I was about to ask the same. Another option is to ask Nalin to update
> > certmonger in F20.
> 
> CCing Nalin. What is your take on this, do you plan to release it to F20.
> AFAIK, it is just stabilization/bugfixing release so it should fit there 
> nicely.

Assuming you don't hit any new bugs, yeah, that makes sense to me, too.
The current F21 candidate build (not in bodhi yet... I should get to
that) is probably what you'll see pop up for F20 as well.

HTH,

Nalin

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

Re: [Freeipa-devel] Compat tree permissions

[Alexander Bokovoy]

On Wed, 03 Sep 2014, Rob Crittenden wrote:

ipa-advise would then need to refer to some common system account +
it's
password it would bind with. Should we file RFE? Is this a right move?

Yes, we need to file RFE and make recommendations to always have
BINDDN/BINDPW or
GSSAPI_SIGN/GSSAPI_ENCRYPT/SASL_AUTH_ID/KRB5_CCNAME/USE_SASL
(see sudoers.ldap and ldap.conf manpages).


Ok, please file the ticket then.

Will do.



Remember that most of the NIS/legacy systems that would actually use
this are non-Linux so keep that in mind as you tighten things up.
ipa-advise doesn't cover the cases of AIX, Solaris and HP/ux.

Yep. However:

- NIS doesn't require LDAP access from client side and nis plugin will
 work fine as it uses slapi_*_internal_*() calls which are not subject
 to ACI evaluation.

- LDAP with any non-anonymous bind will work, including simple bind over
 SSL.

 I've fixed recently cyrus-sasl bug with GSSAPI mech that was
 preventing GSSAPI authentication from AIX. Unfortunately, cyrus-sasl
 developers are not responsive, no answers from upstream for a month.
 Fedora/RHEL packages are not yet updated but I'm going to do that
 soon. This affects only server-side, so by fixing it we'll get GSSAPI
 working for old LDAP clients that support it.

--
/ Alexander Bokovoy

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

Re: [Freeipa-devel] [PATCH] 0010 Add 'host' setting into default.conf configuration file

[Jan Cholasta]

Dne 3.9.2014 v 15:29 Nalin Dahyabhai napsal(a):

On Tue, Sep 02, 2014 at 10:18:12AM +0200, Jan Cholasta wrote:

Dne 27.8.2014 v 16:49 David Kupka napsal(a):

On 08/27/2014 11:22 AM, Jan Cholasta wrote:

Dne 26.8.2014 v 15:55 Rob Crittenden napsal(a):

David Kupka wrote:

On 08/26/2014 03:08 PM, Jan Cholasta wrote:

Hi,

Dne 26.8.2014 v 13:01 David Kupka napsal(a):

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


Doing this will break ipa-client-automount and ipa-certupdate, because
they assume that api.env.host contains the hostname of the local
system
(which is the default value).


It looked suspiciously simple so I could expect that there is some
catch.


There is obviously some confusion about what the option should
represent
(documentation says server hostname, code does client hostname),
IMO we
should resolve that first.


Ok, are there any suggestions? What is the desired state?


AIUI the server option is deprecated because it wasn't being used, not
that it needed to be replaced. I believe that in most cases the server
name is pulled from the xmlrpc_uri.


Yes, that's what the ticket says:
.


Ok, adding 'host' entry with local host name.


host has always meant the local host name.

I think the man page is wrong.


+1


Fixing the line in man page.


rob


ACK as long as this works for Nalin.


The other half of this was cases where there's no ldap_uri set.  Just so
there's no confusion, if ldap_uri and/or server_uri are not set, what
are the recommended fallback settings that should be used for
constructing them?  I suspect it's "server", then "host", which is the
reverse of the order that they're currently being consulted, but I
figured I'd ask while we're all here.


"ldap_uri" is set only on servers, on clients you should use "server" 
(we should probably un-deprecate it). You could use "host" as a 
fallback, but it will only work on servers, as it points to the local 
host. IMO the right order is "server", then "ldap_uri", then maybe "host".




Thanks,

Nalin




--
Jan Cholasta

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

Re: [Freeipa-devel] [PATCH] 0012 Add record(s) to /etc/host when IPA is configured as DNS server.

[Jan Cholasta]

Hi,

Dne 2.9.2014 v 16:51 David Kupka napsal(a):

Ok, the patch no longer depends on 0009. The reason is that 0012 is
going to ipa-4.0 and 0009 to ipa-4.1.

On 09/02/2014 12:13 PM, David Kupka wrote:

This patch depends on freeipa-dkupka-0009 as it modifies the same part
of code.

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


Works for me, ACK.

Honza

--
Jan Cholasta

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

[Freeipa-devel] [PATCH 0260] ipa-client-install: Do not add already configured sources to

[Tomas Babej]

Hi,

Makes sure that any new sources added are not already present
in the entry.

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

-- 
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org 


>From 6cd6f5d523e11a70cd51788dd669cbd2e628eab6 Mon Sep 17 00:00:00 2001
From: Tomas Babej 
Date: Wed, 27 Aug 2014 09:10:59 +0200
Subject: [PATCH] ipa-client-install: Do not add already configured sources to
 nsswitch.conf entries

Makes sure that any new sources added are not already present
in the entry.

https://fedorahosted.org/freeipa/ticket/4508
---
 ipa-client/ipa-install/ipa-client-install | 22 ++
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install
index 08fefc86d31392e9abf66ee4f8fff54a88179795..de466db526e5177b7906038cdb43ed4acef1da7c 100755
--- a/ipa-client/ipa-install/ipa-client-install
+++ b/ipa-client/ipa-install/ipa-client-install
@@ -430,14 +430,20 @@ def configure_nsswitch_database(fstore, database, services, preserve=True,
 opts = conf.parse(f)
 raw_database_entry = conf.findOpts(opts, 'option', database)[1]
 
-if not raw_database_entry:
-# If there is no database entry, database is not present in
-# the nsswitch.conf. Set the list of services to the
-# default list, if passed.
-configured_services = ' '.join(default_value or [])
-else:
-configured_services = raw_database_entry['value'].strip()
+# Detect the list of already configured services
+if not raw_database_entry:
+# If there is no database entry, database is not present in
+# the nsswitch.conf. Set the list of services to the
+# default list, if passed.
+configured_services = ' '.join(default_value or [])
+else:
+configured_services = raw_database_entry['value'].strip()
 
+# Make sure no service is added if already mentioned in the list
+services = [s for s in services
+if s not in configured_services.split()]
+
+# Prepend / append the list of new services
 if append:
 new_services = ' ' + configured_services + ' ' + ' '.join(services)
 else:
@@ -451,7 +457,7 @@ def configure_nsswitch_database(fstore, database, services, preserve=True,
 opts = [{'name': database,
  'type':'option',
  'action':'set',
- 'value': new_services
+ 'value': new_services.strip()
 },
 {'name':'empty',
  'type':'empty'
-- 
1.9.3

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

Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.

[Jan Cholasta]

Dne 3.9.2014 v 12:37 David Kupka napsal(a):

On 09/02/2014 01:56 PM, Jan Cholasta wrote:

Dne 29.8.2014 v 14:34 David Kupka napsal(a):

Hope, I've addressed all the issues (except 9 and 11, inline). Let's go
for another round :-)

On 08/27/2014 11:05 AM, Jan Cholasta wrote:

Hi,

Dne 25.8.2014 v 15:39 David Kupka napsal(a):

On 08/19/2014 05:44 PM, Rob Crittenden wrote:

David Kupka wrote:

On 08/19/2014 09:58 AM, Martin Kosek wrote:

On 08/19/2014 09:05 AM, David Kupka wrote:

FreeIPA will use certmonger D-Bus API as discussed in this thread
https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html




This change should prevent hard-to-reproduce bugs like
https://fedorahosted.org/freeipa/ticket/4280


Thanks for this effort, the updated certmonger module looks much
better! This
will help us get rid of the non-standard communication with
certmonger.

Just couple initial comments from me by reading the code:

1) Testing needs fixed version of certmonger, right? This needs
to be
spelled
out right with the patch.

Yes, certmonger 0.75.13 and above should be fine according ticket
https://fedorahosted.org/certmonger/ticket/36. Added to patch
description.


You should update the spec to set the minimum version as well.

Sure, thanks.




2) Description text in patches is cheap, do not be afraid to use it
and
describe what you did and why. Link to the ticket is missing in the
description
as well:

Ok, increased verbosity a bit :-)



Subject: [PATCH] Use certmonger D-Bus API instead of messing with
its
files.

---


3) get_request_id API:


   criteria = (
-('cert_storage_location', dogtag_constants.ALIAS_DIR,
- certmonger.NPATH),
-('cert_nickname', nickname, None),
+('cert_storage_location',
dogtag_constants.ALIAS_DIR),
+('cert_nickname', nickname),
   )
   request_id = certmonger.get_request_id(criteria)


Do we want to continue using the "criteria" object or should we
rather
switch
to normal function options? I.e. rather using

request_id = certmonger.get_request_id(cert_nickname=nickname,
cert_storage_location=dogtag_constants.ALIAS_DIR)

? It would look more consistent with other calls. I am just asking,
not insisting.

I've no preference here. It seems to be a very small change. Has
anyone
a reason to do it one way and not the other?


I think I used this criteria thing to avoid having a bazillion
optional
parameters and for future-proofing. I think at this point the list is
probably pretty stable, so I'd base it on whether you care about
having
a whole ton of optional parameters or not (it has the advantage of
self-documenting itself).


The list is probably stable but also really excessive. I don't
think it
would help to have more than dozen optional parameters. So I prefer to
leave as-is and change it in future if it is wanted.


3) Starting function:


+try:
+ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
skip_output=True)
+except Exception, e:
+root_logger.error('Failed to start certmonger: %s' % e)
+raise e


I see 2 issues related to this code:
a) Do not call SYSTEMCTL directly. To be platform independent,
rather use
services.knownservices.messagebus.start() that is overridable by
someone else
porting to non-systemd platforms.

Is there anything that can't be done using
ipalib/ipapython/ipaplatform?


It can't make coffee (yet).


b) In this case, do not use "raise e", but just "raise" to keep the
exception
stack trace intact for better debugging.

Every day there's something new to learn about python or FreeIPA.


Both a) and b) should be fixed in other occasions and places.

I found only one occurence of a) issue. Is there some hidden or are
you
talking about the whole FreeIPA project?


4) Feel free to add yourself to Authors section of this module. You
refactored
it greatly to earn it :-)

Done.


You already import dbus, why also separately import DBusException?


Removed, thanks for noticing.

rob



1) The patch needs to be rebased.


I didn't notice the patch is targeted for 4.0. Can you please provide
patches for both ipa-4-0 and ipa-4-1/master?



Attached, 0008-5 works on master/ipa-4-1 and 0008-5-ipa40 works on ipa-4-0.


There is a little bug in ipa-upgradeconfig in the 4.0 version of the 
patch. This is wrong:


for request in requests:
nss_dir, nickname, ca_name, pre_command, post_command, profile 
= request

criteria = {
'cert-database': nss_dir,
'cert-nickname': nickname,
'ca-name': ca_name,
'template-profile': profile,
}

It should be:

 for nss_dir, nickname, ca_name, pre_command, post_command in requests:
criteria = {
'cert-database': nss_dir,
'cert-nickname': nickname,
'ca-name': ca_name,
}






2) Please try to follow PEP8, at least in certmonger.py.


3) In certificate_renewal_update() in ipa-upgradeconfig you re

Re: [Freeipa-devel] [PATCH] 0012 Add record(s) to /etc/host when IPA is configured as DNS server.

[Petr Viktorin]

On 09/03/2014 03:43 PM, Jan Cholasta wrote:

Hi,

Dne 2.9.2014 v 16:51 David Kupka napsal(a):

Ok, the patch no longer depends on 0009. The reason is that 0012 is
going to ipa-4.0 and 0009 to ipa-4.1.

On 09/02/2014 12:13 PM, David Kupka wrote:

This patch depends on freeipa-dkupka-0009 as it modifies the same part
of code.

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


Works for me, ACK.




Pushed to:
master: 8aa01e24a1664f5f523732f79ae8d842fb4417a8
ipa-4-1: 7baf8fecd410e5a8b64767d27a6a2848c4a00803
ipa-4-0: c1b680c54ec2bc0846c4e8a0f5d3260864d645b9


--
Petr³

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

Re: [Freeipa-devel] [PATCH] 0010 Add 'host' setting into default.conf configuration file

[Martin Kosek]

On 09/03/2014 03:41 PM, Jan Cholasta wrote:
> Dne 3.9.2014 v 15:29 Nalin Dahyabhai napsal(a):
>> On Tue, Sep 02, 2014 at 10:18:12AM +0200, Jan Cholasta wrote:
>>> Dne 27.8.2014 v 16:49 David Kupka napsal(a):
 On 08/27/2014 11:22 AM, Jan Cholasta wrote:
> Dne 26.8.2014 v 15:55 Rob Crittenden napsal(a):
>> David Kupka wrote:
>>> On 08/26/2014 03:08 PM, Jan Cholasta wrote:
 Hi,

 Dne 26.8.2014 v 13:01 David Kupka napsal(a):
> https://fedorahosted.org/freeipa/ticket/4481

 Doing this will break ipa-client-automount and ipa-certupdate, because
 they assume that api.env.host contains the hostname of the local
 system
 (which is the default value).
>>>
>>> It looked suspiciously simple so I could expect that there is some
>>> catch.

 There is obviously some confusion about what the option should
 represent
 (documentation says server hostname, code does client hostname),
 IMO we
 should resolve that first.
>>>
>>> Ok, are there any suggestions? What is the desired state?
>>
>> AIUI the server option is deprecated because it wasn't being used, not
>> that it needed to be replaced. I believe that in most cases the server
>> name is pulled from the xmlrpc_uri.
>
> Yes, that's what the ticket says:
> .

 Ok, adding 'host' entry with local host name.
>>
>> host has always meant the local host name.
>>
>> I think the man page is wrong.
>
> +1
>
 Fixing the line in man page.
>>
>> rob
>>>
>>> ACK as long as this works for Nalin.
>>
>> The other half of this was cases where there's no ldap_uri set.  Just so
>> there's no confusion, if ldap_uri and/or server_uri are not set, what
>> are the recommended fallback settings that should be used for
>> constructing them?  I suspect it's "server", then "host", which is the
>> reverse of the order that they're currently being consulted, but I
>> figured I'd ask while we're all here.
> 
> "ldap_uri" is set only on servers, on clients you should use "server" (we
> should probably un-deprecate it). You could use "host" as a fallback, but it
> will only work on servers, as it points to the local host. IMO the right order
> is "server", then "ldap_uri", then maybe "host".

BTW what happens when original server that the client enrolled with no longer
exist and was replaced by some other server with other FQDN. Will certmonger
fail in this case or will it fall back and do DNS SRV record to find
alternative server like "ipa" command does?

Martin

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

Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.

[David Kupka]

On 09/03/2014 04:05 PM, Jan Cholasta wrote:

Dne 3.9.2014 v 12:37 David Kupka napsal(a):

On 09/02/2014 01:56 PM, Jan Cholasta wrote:

Dne 29.8.2014 v 14:34 David Kupka napsal(a):

Hope, I've addressed all the issues (except 9 and 11, inline). Let's go
for another round :-)

On 08/27/2014 11:05 AM, Jan Cholasta wrote:

Hi,

Dne 25.8.2014 v 15:39 David Kupka napsal(a):

On 08/19/2014 05:44 PM, Rob Crittenden wrote:

David Kupka wrote:

On 08/19/2014 09:58 AM, Martin Kosek wrote:

On 08/19/2014 09:05 AM, David Kupka wrote:

FreeIPA will use certmonger D-Bus API as discussed in this thread
https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html





This change should prevent hard-to-reproduce bugs like
https://fedorahosted.org/freeipa/ticket/4280


Thanks for this effort, the updated certmonger module looks much
better! This
will help us get rid of the non-standard communication with
certmonger.

Just couple initial comments from me by reading the code:

1) Testing needs fixed version of certmonger, right? This needs
to be
spelled
out right with the patch.

Yes, certmonger 0.75.13 and above should be fine according ticket
https://fedorahosted.org/certmonger/ticket/36. Added to patch
description.


You should update the spec to set the minimum version as well.

Sure, thanks.




2) Description text in patches is cheap, do not be afraid to
use it
and
describe what you did and why. Link to the ticket is missing in
the
description
as well:

Ok, increased verbosity a bit :-)



Subject: [PATCH] Use certmonger D-Bus API instead of messing with
its
files.

---


3) get_request_id API:


   criteria = (
-('cert_storage_location',
dogtag_constants.ALIAS_DIR,
- certmonger.NPATH),
-('cert_nickname', nickname, None),
+('cert_storage_location',
dogtag_constants.ALIAS_DIR),
+('cert_nickname', nickname),
   )
   request_id = certmonger.get_request_id(criteria)


Do we want to continue using the "criteria" object or should we
rather
switch
to normal function options? I.e. rather using

request_id = certmonger.get_request_id(cert_nickname=nickname,
cert_storage_location=dogtag_constants.ALIAS_DIR)

? It would look more consistent with other calls. I am just
asking,
not insisting.

I've no preference here. It seems to be a very small change. Has
anyone
a reason to do it one way and not the other?


I think I used this criteria thing to avoid having a bazillion
optional
parameters and for future-proofing. I think at this point the
list is
probably pretty stable, so I'd base it on whether you care about
having
a whole ton of optional parameters or not (it has the advantage of
self-documenting itself).


The list is probably stable but also really excessive. I don't
think it
would help to have more than dozen optional parameters. So I
prefer to
leave as-is and change it in future if it is wanted.


3) Starting function:


+try:
+ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
skip_output=True)
+except Exception, e:
+root_logger.error('Failed to start certmonger: %s' % e)
+raise e


I see 2 issues related to this code:
a) Do not call SYSTEMCTL directly. To be platform independent,
rather use
services.knownservices.messagebus.start() that is overridable by
someone else
porting to non-systemd platforms.

Is there anything that can't be done using
ipalib/ipapython/ipaplatform?


It can't make coffee (yet).


b) In this case, do not use "raise e", but just "raise" to keep
the
exception
stack trace intact for better debugging.

Every day there's something new to learn about python or FreeIPA.


Both a) and b) should be fixed in other occasions and places.

I found only one occurence of a) issue. Is there some hidden or are
you
talking about the whole FreeIPA project?


4) Feel free to add yourself to Authors section of this module.
You
refactored
it greatly to earn it :-)

Done.


You already import dbus, why also separately import DBusException?


Removed, thanks for noticing.

rob



1) The patch needs to be rebased.


I didn't notice the patch is targeted for 4.0. Can you please provide
patches for both ipa-4-0 and ipa-4-1/master?



Attached, 0008-5 works on master/ipa-4-1 and 0008-5-ipa40 works on
ipa-4-0.


There is a little bug in ipa-upgradeconfig in the 4.0 version of the
patch. This is wrong:

 for request in requests:
 nss_dir, nickname, ca_name, pre_command, post_command, profile
= request
 criteria = {
 'cert-database': nss_dir,
 'cert-nickname': nickname,
 'ca-name': ca_name,
 'template-profile': profile,
 }

It should be:

  for nss_dir, nickname, ca_name, pre_command, post_command in
requests:
 criteria = {
 'cert-database': nss_dir,
 'cert-nickname': nickname,
 'ca-name': ca_name,
 }






2) Please try to follow PEP8, at least in certmonger.py.


3) In 

Re: [Freeipa-devel] [PATCH] 0010 Add 'host' setting into default.conf configuration file

[Nalin Dahyabhai]

On Wed, Sep 03, 2014 at 04:25:00PM +0200, Martin Kosek wrote:
> On 09/03/2014 03:41 PM, Jan Cholasta wrote:
> > "ldap_uri" is set only on servers, on clients you should use "server" (we
> > should probably un-deprecate it). You could use "host" as a fallback, but it
> > will only work on servers, as it points to the local host. IMO the right 
> > order
> > is "server", then "ldap_uri", then maybe "host".
> 
> BTW what happens when original server that the client enrolled with no longer
> exist and was replaced by some other server with other FQDN. Will certmonger
> fail in this case or will it fall back and do DNS SRV record to find
> alternative server like "ipa" command does?

It doesn't currently, but that certainly sounds like a reasonable thing
to ask for in a trac ticket or bugzilla.

Cheers,

Nalin

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

Re: [Freeipa-devel] [PATCHES 0109-0110] DNS: fix DS record validation

[Martin Basti]

On 02/09/14 17:16, Petr Spacek wrote:

On 20.8.2014 19:26, Martin Basti wrote:

Part of DNSSEC
Patches attached.


NACK

# ipa dnsrecord-add ipa.example. ds '--ds-rec=1 2 3 4'
ipa: ERROR: invalid 'dsrecord': DS record requires to coexist with an 
NS record (RFC 4529, section 4.6)


RFC number is incorrect. IMHO it should also reference 'RFC 4035 
section 2.4'.


Also, there is one hole:
Current code allows you to add DS RR to existing NS and then to remove 
NS.


Let me know if adding a check to -del is too hard, maybe we can live 
without it...



dnsrecord-del validation added

Updated patch attached

Required in ipa 4.1 but this could be pushed to 4.0.x  too

--
Martin Basti

From 072206c859f52b801926fab6a75f4c4f574040ea Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 20 Aug 2014 18:51:25 +0200
Subject: [PATCH 1/2] DNSSEC: fix DS record validation

Part of: https://fedorahosted.org/freeipa/ticket/3801
---
 ipalib/plugins/dns.py | 99 ---
 1 file changed, 63 insertions(+), 36 deletions(-)

diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
index aba30dd3f3ca2f06058a05f5c0350e1a3e8eb2e5..e4822003fd8ca2b2caa9c4db9b812488b2023bdc 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -2610,6 +2610,14 @@ class dnsrecord(LDAPObject):
doc=_('Parse all raw DNS records and return them in a structured way'),
)
 
+def _dsrecord_pre_callback(self, ldap, dn, entry_attrs, *keys, **options):
+assert isinstance(dn, DN)
+dsrecords = entry_attrs.get('dsrecord')
+if dsrecords and self.is_pkey_zone_record(*keys):
+raise errors.ValidationError(
+name='dsrecord',
+error=unicode(_('DS record must not be in zone apex (RFC 4035 section 2.4)')))
+
 def _nsrecord_pre_callback(self, ldap, dn, entry_attrs, *keys, **options):
 assert isinstance(dn, DN)
 nsrecords = entry_attrs.get('nsrecord')
@@ -2868,8 +2876,9 @@ class dnsrecord(LDAPObject):
 processed.append(rrparam.name)
 yield rrparam
 
-def check_record_type_collisions(self, keys, old_entry, entry_attrs):
-# Test that only allowed combination of record types was created
+def updated_rrattrs(self, old_entry, entry_attrs):
+"""Returns updated RR attributes
+"""
 rrattrs = {}
 if old_entry is not None:
 old_rrattrs = dict((key, value) for key, value in old_entry.iteritems()
@@ -2880,42 +2889,53 @@ class dnsrecord(LDAPObject):
 if key in self.params and
 isinstance(self.params[key], DNSRecord))
 rrattrs.update(new_rrattrs)
+return rrattrs
+
+def check_record_type_collisions(self, keys, rrattrs):
+# Test that only allowed combination of record types was created
 
 # CNAME record validation
-try:
-cnames = rrattrs['cnamerecord']
-except KeyError:
-pass
-else:
-if cnames is not None:
-if len(cnames) > 1:
-raise errors.ValidationError(name='cnamerecord',
-error=_('only one CNAME record is allowed per name '
-'(RFC 2136, section 1.1.5)'))
-if any(rrvalue is not None
-   and rrattr != 'cnamerecord'
-   for rrattr, rrvalue in rrattrs.iteritems()):
-raise errors.ValidationError(name='cnamerecord',
-  error=_('CNAME record is not allowed to coexist '
-  'with any other record (RFC 1034, section 3.6.2)'))
+cnames = rrattrs.get('cnamerecord')
+if cnames is not None:
+if len(cnames) > 1:
+raise errors.ValidationError(name='cnamerecord',
+error=_('only one CNAME record is allowed per name '
+'(RFC 2136, section 1.1.5)'))
+if any(rrvalue is not None
+   and rrattr != 'cnamerecord'
+   for rrattr, rrvalue in rrattrs.iteritems()):
+raise errors.ValidationError(name='cnamerecord',
+error=_('CNAME record is not allowed to coexist '
+  'with any other record (RFC 1034, section 3.6.2)'))
 
 # DNAME record validation
-try:
-dnames = rrattrs['dnamerecord']
-except KeyError:
-pass
-else:
-if dnames is not None:
-if len(dnames) > 1:
-raise errors.ValidationError(name='dnamerecord',
-error=_('only one DNAME record is allowed per name '
-'(RFC 6672, section 2.4)'))
-# DNAME must not coexist with CNAME, but this is already checked earlier
-if rrattrs

Re: [Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.

[Jan Cholasta]

Dne 3.9.2014 v 16:25 David Kupka napsal(a):

On 09/03/2014 04:05 PM, Jan Cholasta wrote:

Dne 3.9.2014 v 12:37 David Kupka napsal(a):

On 09/02/2014 01:56 PM, Jan Cholasta wrote:

Dne 29.8.2014 v 14:34 David Kupka napsal(a):

Hope, I've addressed all the issues (except 9 and 11, inline).
Let's go
for another round :-)

On 08/27/2014 11:05 AM, Jan Cholasta wrote:

Hi,

Dne 25.8.2014 v 15:39 David Kupka napsal(a):

On 08/19/2014 05:44 PM, Rob Crittenden wrote:

David Kupka wrote:

On 08/19/2014 09:58 AM, Martin Kosek wrote:

On 08/19/2014 09:05 AM, David Kupka wrote:

FreeIPA will use certmonger D-Bus API as discussed in this
thread
https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html






This change should prevent hard-to-reproduce bugs like
https://fedorahosted.org/freeipa/ticket/4280


Thanks for this effort, the updated certmonger module looks much
better! This
will help us get rid of the non-standard communication with
certmonger.

Just couple initial comments from me by reading the code:

1) Testing needs fixed version of certmonger, right? This needs
to be
spelled
out right with the patch.

Yes, certmonger 0.75.13 and above should be fine according ticket
https://fedorahosted.org/certmonger/ticket/36. Added to patch
description.


You should update the spec to set the minimum version as well.

Sure, thanks.




2) Description text in patches is cheap, do not be afraid to
use it
and
describe what you did and why. Link to the ticket is missing in
the
description
as well:

Ok, increased verbosity a bit :-)



Subject: [PATCH] Use certmonger D-Bus API instead of messing
with
its
files.

---


3) get_request_id API:


   criteria = (
-('cert_storage_location',
dogtag_constants.ALIAS_DIR,
- certmonger.NPATH),
-('cert_nickname', nickname, None),
+('cert_storage_location',
dogtag_constants.ALIAS_DIR),
+('cert_nickname', nickname),
   )
   request_id = certmonger.get_request_id(criteria)


Do we want to continue using the "criteria" object or should we
rather
switch
to normal function options? I.e. rather using

request_id = certmonger.get_request_id(cert_nickname=nickname,
cert_storage_location=dogtag_constants.ALIAS_DIR)

? It would look more consistent with other calls. I am just
asking,
not insisting.

I've no preference here. It seems to be a very small change. Has
anyone
a reason to do it one way and not the other?


I think I used this criteria thing to avoid having a bazillion
optional
parameters and for future-proofing. I think at this point the
list is
probably pretty stable, so I'd base it on whether you care about
having
a whole ton of optional parameters or not (it has the advantage of
self-documenting itself).


The list is probably stable but also really excessive. I don't
think it
would help to have more than dozen optional parameters. So I
prefer to
leave as-is and change it in future if it is wanted.


3) Starting function:


+try:
+ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
skip_output=True)
+except Exception, e:
+root_logger.error('Failed to start certmonger: %s' % e)
+raise e


I see 2 issues related to this code:
a) Do not call SYSTEMCTL directly. To be platform independent,
rather use
services.knownservices.messagebus.start() that is overridable by
someone else
porting to non-systemd platforms.

Is there anything that can't be done using
ipalib/ipapython/ipaplatform?


It can't make coffee (yet).


b) In this case, do not use "raise e", but just "raise" to keep
the
exception
stack trace intact for better debugging.

Every day there's something new to learn about python or FreeIPA.


Both a) and b) should be fixed in other occasions and places.

I found only one occurence of a) issue. Is there some hidden or
are
you
talking about the whole FreeIPA project?


4) Feel free to add yourself to Authors section of this module.
You
refactored
it greatly to earn it :-)

Done.


You already import dbus, why also separately import DBusException?


Removed, thanks for noticing.

rob



1) The patch needs to be rebased.


I didn't notice the patch is targeted for 4.0. Can you please provide
patches for both ipa-4-0 and ipa-4-1/master?



Attached, 0008-5 works on master/ipa-4-1 and 0008-5-ipa40 works on
ipa-4-0.


There is a little bug in ipa-upgradeconfig in the 4.0 version of the
patch. This is wrong:

 for request in requests:
 nss_dir, nickname, ca_name, pre_command, post_command, profile
= request
 criteria = {
 'cert-database': nss_dir,
 'cert-nickname': nickname,
 'ca-name': ca_name,
 'template-profile': profile,
 }

It should be:

  for nss_dir, nickname, ca_name, pre_command, post_command in
requests:
 criteria = {
 'cert-database': nss_dir,
 'cert-nickname': nickname,
 'ca-name': ca_name,
 }






2) Please try to fo

Re: [Freeipa-devel] [PATCH] 0010 Add 'host' setting into default.conf configuration file

[Martin Kosek]

On 09/03/2014 04:33 PM, Nalin Dahyabhai wrote:
> On Wed, Sep 03, 2014 at 04:25:00PM +0200, Martin Kosek wrote:
>> On 09/03/2014 03:41 PM, Jan Cholasta wrote:
>>> "ldap_uri" is set only on servers, on clients you should use "server" (we
>>> should probably un-deprecate it). You could use "host" as a fallback, but it
>>> will only work on servers, as it points to the local host. IMO the right 
>>> order
>>> is "server", then "ldap_uri", then maybe "host".
>>
>> BTW what happens when original server that the client enrolled with no longer
>> exist and was replaced by some other server with other FQDN. Will certmonger
>> fail in this case or will it fall back and do DNS SRV record to find
>> alternative server like "ipa" command does?
> 
> It doesn't currently, but that certainly sounds like a reasonable thing
> to ask for in a trac ticket or bugzilla.
> 
> Cheers,
> 
> Nalin
> 

Ok, bug filed: https://bugzilla.redhat.com/show_bug.cgi?id=1136900

Martin

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

Re: [Freeipa-devel] [PATCHES 0111-0113] Fix NS record coexistence validation

[Martin Basti]

On 03/09/14 12:30, Martin Kosek wrote:

On 09/02/2014 05:38 PM, Petr Spacek wrote:

On 21.8.2014 19:21, Martin Basti wrote:

During work on DNSSEC we found a wrong validation of NS records
Patch 0113 fixes an error in tests caused by bind-dyndb-ldap bug
https://fedorahosted.org/bind-dyndb-ldap/ticket/123
Patches attached.

Functional ACK. It can be pushed if Python gurus don't see any problem.


I think the patches will need a rebase before push, I cannot apply them to my
tree. The Python part itself looked good to me.

Martin

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

Rebased patch attached, due changes in freeipa-mbasti-0109,
patches mbasti-0109.2, mbasti-0110.2 are required.

--
Martin Basti

From be5dd565252ccf089e52a378656f070e889b3eaa Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Thu, 21 Aug 2014 18:08:10 +0200
Subject: [PATCH 1/3] DNS fix NS record coexistence validator

NS can coexistent only with A, , DS, NS record
---
 ipalib/plugins/dns.py | 23 +--
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
index e4822003fd8ca2b2caa9c4db9b812488b2023bdc..c7039bbf9c4c52a5a12ba6b806d3a0d414144fb8 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -2916,11 +2916,23 @@ class dnsrecord(LDAPObject):
 error=_('only one DNAME record is allowed per name '
 '(RFC 6672, section 2.4)'))
 # DNAME must not coexist with CNAME, but this is already checked earlier
-if rrattrs.get('nsrecord') and not keys[1].is_empty():
-raise errors.ValidationError(name='dnamerecord',
-  error=_('DNAME record is not allowed to coexist with an '
-  'NS record except when located in a zone root '
-  'record (RFC 6672, section 2.3)'))
+
+# NS record validation
+# NS record can coexist only with A, , DS, and other NS records (except zone apex)
+# RFC 2181 section 6.1,
+allowed_records = ['', 'A', 'DS', 'NS']
+nsrecords = rrattrs.get('nsrecord')
+if nsrecords and not self.is_pkey_zone_record(*keys):
+for r_type in _record_types:
+if (r_type not in allowed_records
+and rrattrs.get('%srecord' % r_type.lower())
+):
+raise errors.ValidationError(
+name='nsrecord',
+error=_('NS record is not allowed to coexist with an '
+'%(type)s record except when located in a '
+'zone root record (RFC 2181, section 6.1)') %
+{'type': r_type})
 
 def check_record_type_dependencies(self, keys, rrattrs):
 # Test that all record type dependencies are satisfied
@@ -2936,7 +2948,6 @@ class dnsrecord(LDAPObject):
 error=_('DS record requires to coexist with an '
  'NS record (RFC 4592 section 4.6, RFC 4035 section 2.4)'))
 
-
 def _entry2rrsets(self, entry_attrs, dns_name, dns_domain):
 '''Convert entry_attrs to a dictionary {rdtype: rrset}.
 
-- 
1.8.3.1

From 02b3613c6cc42ad741293442f50fc7a9a8425a09 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Thu, 21 Aug 2014 18:09:22 +0200
Subject: [PATCH 2/3] Test: DNS NS validation

---
 ipatests/test_xmlrpc/test_dns_plugin.py | 33 +
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/ipatests/test_xmlrpc/test_dns_plugin.py b/ipatests/test_xmlrpc/test_dns_plugin.py
index 4b4595580b0058b25bb0552bd4f8f72a3d606d03..1784e0b17e87cffc037184c1ab06d2dfe0454ac1 100644
--- a/ipatests/test_xmlrpc/test_dns_plugin.py
+++ b/ipatests/test_xmlrpc/test_dns_plugin.py
@@ -123,6 +123,10 @@ name1_dn = DN(('idnsname',name1), zone1_dn)
 name1_renamed = u'testdnsres-renamed'
 name1_renamed_dnsname = DNSName(name1_renamed)
 
+name_ns = u'testdnsres-ns'
+name_ns_dnsname = DNSName(name_ns)
+name_ns_dn = DN(('idnsname',name_ns), zone1_dn)
+
 revname1 = u'80'
 revname1_dnsname = DNSName(revname1)
 revname1_ip = revzone1_ipprefix + revname1
@@ -1168,9 +1172,10 @@ class test_dns(Declarative):
 desc='Try to add NS record to %r using dnsrecord_add' % (dname),
 command=('dnsrecord_add', [zone1, dname],
 {'nsrecord': u'%s.%s.' % (name1, zone1)}),
-expected=errors.ValidationError(name='dnamerecord',
-error=u'DNAME record is not allowed to coexist with an NS '
-  u'record except when located in a zone root record (RFC 6672, section 2.3)'),
+expected=errors.ValidationError(name='nsrecord',
+error=u'NS record is not allowed to coexist with an DNAME '
+  u'record except

Re: [Freeipa-devel] [PATCH] 0640 Add managed read permissions for compat tree

[Simo Sorce]

On Wed, 2014-09-03 at 13:27 +0200, Petr Viktorin wrote:
> Hello,
> This adds managed read permissions to the compat tree.
> 
> For users it grants anonymous access; authenticated users can read 
> groups, hosts and netgroups.
> 
> I'm unsure if this is what we want to do for groups, but "Read Group 
> Membership" is only granted to authenticated users by default, and the 
> compat tree exposes memberuid.

The reason we restrict member is because it exposes also hbac, sudo and
other sensible groupings. memberuid does not have those groups in, so I
think it is safe (and necessary for legacy clients) to allow anonymous
to read it, just like for users.

Simo.

> https://fedorahosted.org/freeipa/ticket/4521
> 


-- 
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] [PATCH 0242] Set the default attributes for RootDSE

[Tomas Babej]

On 07/28/2014 03:03 PM, Petr Viktorin wrote:
> On 07/15/2014 09:13 AM, Tomas Babej wrote:
>> Hi,
>>
>> With 389 DS 1.3.3 upwards we can leverage the
>> nsslapd-return-default-opattr
>> attribute to enumerate the list of attributes that should be returned
>> even if not specified explicitly. Use the behaviour to get the same
>> attributes
>> returned from searches on rootDSE as in 1.3.1.
>>
>> https://fedorahosted.org/freeipa/ticket/4288
>
> This fails with an older DS version.
>
> Running transaction (shutdown inhibited)
>   Updating   : freeipa-python-4.0.0GITa2b91d7-0.fc20.x86_64   
> 1/14
>   Updating   : freeipa-client-4.0.0GITa2b91d7-0.fc20.x86_64   
> 2/14
> Could not load host key: /etc/ssh/ssh_host_dsa_key
>   Updating   : freeipa-admintools-4.0.0GITa2b91d7-0.fc20.x86_64
>3/14
>   Updating   : freeipa-server-4.0.0GITa2b91d7-0.fc20.x86_64   
> 4/14
>   Updating   : freeipa-server-trust-ad-4.0.0GITa2b91d7-0.fc20.x86_64
>5/14
>   Updating   : freeipa-tests-4.0.0GITa2b91d7-0.fc20.x86_64   
> 6/14
>   Updating   : freeipa-debuginfo-4.0.0GITa2b91d7-0.fc20.x86_64
>7/14
>   Cleanup: freeipa-tests-4.0.0GIT06aa522-0.fc20.x86_64   
> 8/14
>   Cleanup: freeipa-debuginfo-4.0.0GIT06aa522-0.fc20.x86_64
>9/14
>   Cleanup: freeipa-server-trust-ad-4.0.0GIT06aa522-0.fc20.x86_64
>   10/14
>   Cleanup: freeipa-server-4.0.0GIT06aa522-0.fc20.x86_64  
> 11/14
>   Cleanup: freeipa-admintools-4.0.0GIT06aa522-0.fc20.x86_64
>   12/14
>   Cleanup: freeipa-client-4.0.0GIT06aa522-0.fc20.x86_64  
> 13/14
>   Cleanup: freeipa-python-4.0.0GIT06aa522-0.fc20.x86_64  
> 14/14
> Upgrade failed with attribute "nsslapd-return-default-opattr" not allowed
> IPA upgrade failed.
>
> You'll need to update the spec file too, at least.
>

Sure, spec file updated.

We might want to wait with pushing this, since 1.3.3 is not available yet.

-- 
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org 

>From 8c90173e40468406b69ad9ed57c8cb2bb7d39070 Mon Sep 17 00:00:00 2001
From: Tomas Babej 
Date: Wed, 2 Jul 2014 02:55:01 +0200
Subject: [PATCH] Set the default attributes for RootDSE

With 389 DS 1.3.3 upwards we can leverage the nsslapd-return-default-opattr
attribute to enumerate the list of attributes that should be returned
even if not specified explicitly. Use the behaviour to get the same attributes
returned from searches on rootDSE as in 1.3.1.

https://fedorahosted.org/freeipa/ticket/4288
---
 freeipa.spec.in   | 2 +-
 install/updates/10-rootdse.update | 9 +
 install/updates/Makefile.am   | 1 +
 3 files changed, 11 insertions(+), 1 deletion(-)
 create mode 100644 install/updates/10-rootdse.update

diff --git a/freeipa.spec.in b/freeipa.spec.in
index 24771ac8eea0390d3cc3db201ca9bc986e48dc53..90d4596e7230a877f0cde061db75ffbde9bed9ac 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -87,7 +87,7 @@ Group: System Environment/Base
 Requires: %{name}-python = %{version}-%{release}
 Requires: %{name}-client = %{version}-%{release}
 Requires: %{name}-admintools = %{version}-%{release}
-Requires: 389-ds-base >= 1.3.2.20
+Requires: 389-ds-base >= 1.3.3
 Requires: openldap-clients > 2.4.35-4
 Requires: nss >= 3.14.3-12.0
 Requires: nss-tools >= 3.14.3-12.0
diff --git a/install/updates/10-rootdse.update b/install/updates/10-rootdse.update
new file mode 100644
index ..f44992a5d9cc0ad58eaed485f9793e1b07f06b6a
--- /dev/null
+++ b/install/updates/10-rootdse.update
@@ -0,0 +1,9 @@
+# Set the default attributes to be returned by RootDSE
+dn:
+add:nsslapd-return-default-opattr:namingContexts
+add:nsslapd-return-default-opattr:supportedControl
+add:nsslapd-return-default-opattr:supportedExtension
+add:nsslapd-return-default-opattr:supportedLDAPVersion
+add:nsslapd-return-default-opattr:supportedSASLMechanisms
+add:nsslapd-return-default-opattr:vendorName
+add:nsslapd-return-default-opattr:vendorVersion
diff --git a/install/updates/Makefile.am b/install/updates/Makefile.am
index 1d912a7d29552000d082aca58d345924ab84e11c..82acaca70b0d0712cd074eca97c543d1cfb0bbb8 100644
--- a/install/updates/Makefile.am
+++ b/install/updates/Makefile.am
@@ -5,6 +5,7 @@ app_DATA =\
 	10-config.update		\
 	10-enable-betxn.update		\
 	10-selinuxusermap.update	\
+	10-rootdse.update		\
 	10-uniqueness.update		\
 	10-schema_compat.update		\
 	19-managed-entries.update	\
-- 
1.9.3

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

Re: [Freeipa-devel] [PATCH 0260] ipa-client-install: Do not add already configured sources to

[Petr Viktorin]

On 09/03/2014 03:53 PM, Tomas Babej wrote:

Hi,

Makes sure that any new sources added are not already present
in the entry.

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


It works fine, ACK.

I do have some comments, but 4.0.x is a stabilization release, so they'd 
probably be better in a 4.1 patch:


The way you first join default_value to make configured_services, and
then repeatedly split it, looks quite wasteful. Wouldn't 
configured_services be better as a list?
Also I wonder if configure_nsswitch_database needs those unused 
preserve/append options.


Should I push now?

--
Petr³

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

Re: [Freeipa-devel] [PATCH] 0640 Add managed read permissions for compat tree

[Petr Viktorin]

On 09/03/2014 04:51 PM, Simo Sorce wrote:

On Wed, 2014-09-03 at 13:27 +0200, Petr Viktorin wrote:

Hello,
This adds managed read permissions to the compat tree.

For users it grants anonymous access; authenticated users can read
groups, hosts and netgroups.

I'm unsure if this is what we want to do for groups, but "Read Group
Membership" is only granted to authenticated users by default, and the
compat tree exposes memberuid.


The reason we restrict member is because it exposes also hbac, sudo and
other sensible groupings. memberuid does not have those groups in, so I
think it is safe (and necessary for legacy clients) to allow anonymous
to read it, just like for users.

Simo.


In that case, I'd also add memberuid to 'Read Groups', to make it clear 
what our default policy is.


--
Petr³

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

Re: [Freeipa-devel] [PATCH] 0011 Allow user to force Kerberos realm during installation

[Jan Cholasta]

Hi,

Dne 27.8.2014 v 13:56 David Kupka napsal(a):

Usually it isn't wise to allow something like this. But in environment
with broken DNS (described in ticket) there is probably not many
alternatives.

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


1) I think you can log realm in search() as part of the "Starting IPA 
discovery ..." message instead of a separate message.



2) Also, no need to log the realm twice in search().


3) It looks like you forgot to un-indent some code in ipadnssearchkrbkdc().


Honza

--
Jan Cholasta

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

Re: [Freeipa-devel] [PATCH] 0640 Add managed read permissions for compat tree

[Martin Kosek]

On 09/03/2014 03:15 PM, Petr Viktorin wrote:
> On 09/03/2014 02:27 PM, Petr Viktorin wrote:
>> On 09/03/2014 01:27 PM, Petr Viktorin wrote:
>>> Hello,
>>> This adds managed read permissions to the compat tree.
>>>
>>> For users it grants anonymous access; authenticated users can read
>>> groups, hosts and netgroups.
>>>
>>> I'm unsure if this is what we want to do for groups, but "Read Group
>>> Membership" is only granted to authenticated users by default, and the
>>> compat tree exposes memberuid.
>>>
>>> https://fedorahosted.org/freeipa/ticket/4521
>>
>> Self-NACK, there's a typo (though I could swear I tested this :/)
>>
>>
> 
> Fixed patch attached.
> 

I tested and it looks and works OK, ACK from me. We can wait till tomorrow to
see if there are no reservations from Alexander or Rob.

Martin

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

[Freeipa-devel] [PATCH] 318 Backup CS.cfg before modifying it

[Jan Cholasta]

Hi,

the attached patch fixes .

Honza

--
Jan Cholasta
>From cdf6dcd447b762c47bf1f46a53a0127e265e6983 Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Wed, 3 Sep 2014 15:04:35 +0200
Subject: [PATCH] Backup CS.cfg before modifying it

https://fedorahosted.org/freeipa/ticket/4166
---
 install/tools/ipa-upgradeconfig |  1 +
 ipaserver/install/cainstance.py | 21 +
 2 files changed, 22 insertions(+)

diff --git a/install/tools/ipa-upgradeconfig b/install/tools/ipa-upgradeconfig
index 9c9de03..c0cd0e8 100644
--- a/install/tools/ipa-upgradeconfig
+++ b/install/tools/ipa-upgradeconfig
@@ -1085,6 +1085,7 @@ def main():
 sub_dict['SUBJECT_BASE'] = subject_base
 
 ca = cainstance.CAInstance(api.env.realm, certs.NSS_DIR)
+ca.backup_config()
 
 # migrate CRL publish dir before the location in ipa.conf is updated
 ca_restart = migrate_crl_publish_dir(ca)
diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index e8bb7d7..8f7fdd5 100644
--- a/ipaserver/install/cainstance.py
+++ b/ipaserver/install/cainstance.py
@@ -404,6 +404,7 @@ class CAInstance(DogtagInstance):
 self.step("creating pki-ca instance", self.create_instance)
 self.step("configuring certificate server instance", self.__configure_instance)
 self.step("stopping certificate server instance to update CS.cfg", self.stop_instance)
+self.step("backing up CS.cfg", self.backup_config)
 self.step("disabling nonces", self.__disable_nonce)
 self.step("set up CRL publishing", self.__enable_crl_publish)
 self.step("enable PKIX certificate path discovery and validation", self.enable_pkix)
@@ -723,6 +724,12 @@ class CAInstance(DogtagInstance):
 
 self.log.debug("completed creating ca instance")
 
+def backup_config(self):
+try:
+backup_config(self.dogtag_constants)
+except Exception, e:
+root_logger.warning("Failed to backup CS.cfg: %s", e)
+
 def __disable_nonce(self):
 # Turn off Nonces
 update_result = installutils.update_file(
@@ -1577,6 +1584,11 @@ class CAInstance(DogtagInstance):
   'subsystemCert cert-pki-ca': 'ca.subsystem.cert',
   'Server-Cert cert-pki-ca': 'ca.sslserver.cert'}
 
+try:
+backup_config(dogtag_constants)
+except Exception, e:
+syslog.syslog(syslog.LOG_ERR, "Failed to backup CS.cfg: %s" % e)
+
 DogtagInstance.update_cert_cs_cfg(
 nickname, cert, directives,
 dogtag.configured_constants().CS_CFG_PATH,
@@ -1705,6 +1717,15 @@ def install_replica_ca(config, postinstall=False):
 
 return ca
 
+def backup_config(dogtag_constants=None):
+"""
+Create a backup copy of CS.cfg
+"""
+if dogtag_constants is None:
+dogtag_constants = dogtag.configured_constants()
+
+shutil.copy(dogtag_constants.CS_CFG_PATH,
+dogtag_constants.CS_CFG_PATH + '.ipabkp')
 
 def update_people_entry(dercert):
 """
-- 
1.9.3

>From 3abeba94882e2e62f190fd0441be9bdfc0ce84ec Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Wed, 3 Sep 2014 15:04:35 +0200
Subject: [PATCH] Backup CS.cfg before modifying it

https://fedorahosted.org/freeipa/ticket/4166
---
 install/tools/ipa-upgradeconfig |  1 +
 ipaserver/install/cainstance.py | 21 +
 2 files changed, 22 insertions(+)

diff --git a/install/tools/ipa-upgradeconfig b/install/tools/ipa-upgradeconfig
index 54193e9..5e74b39 100644
--- a/install/tools/ipa-upgradeconfig
+++ b/install/tools/ipa-upgradeconfig
@@ -1079,6 +1079,7 @@ def main():
 sub_dict['SUBJECT_BASE'] = subject_base
 
 ca = cainstance.CAInstance(api.env.realm, certs.NSS_DIR)
+ca.backup_config()
 
 # migrate CRL publish dir before the location in ipa.conf is updated
 ca_restart = migrate_crl_publish_dir(ca)
diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index 531b930..c39ab85 100644
--- a/ipaserver/install/cainstance.py
+++ b/ipaserver/install/cainstance.py
@@ -449,6 +449,7 @@ class CAInstance(service.Service):
 self.step("creating pki-ca instance", self.create_instance)
 self.step("configuring certificate server instance", self.__configure_instance)
 self.step("stopping certificate server instance to update CS.cfg", self.__stop)
+self.step("backing up CS.cfg", self.backup_config)
 self.step("disabling nonces", self.__disable_nonce)
 self.step("set up CRL publishing", self.__enable_crl_publish)
 self.step("starting certificate server instance", self.__start)
@@ -801,6 +802,12 @@ class CAInstance(service.Service):
 root_logger.debug(traceback.format_exc())
 root_logger.critical("Failed to restart the certificate server. See the installation log for details.")
 
+def backup_config(self):
+try:

[Freeipa-devel] [PATCH] 1109 No client machine cert

[Rob Crittenden]

No longer request and install a cert for the IPA client machine.

rob
>From 0468e18bb949e9dd8fc60c5f20581c1aea72be29 Mon Sep 17 00:00:00 2001
From: Rob Crittenden 
Date: Wed, 3 Sep 2014 15:14:45 -0400
Subject: [PATCH] No longer generate a machine certificate on client installs

https://fedorahosted.org/freeipa/ticket/4449
---
 ipa-client/ipa-install/ipa-client-install | 68 +--
 1 file changed, 2 insertions(+), 66 deletions(-)

diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install
index 08fefc86d31392e9abf66ee4f8fff54a88179795..94921bdae087e00115371e4d454f8d74c005d7fe 100755
--- a/ipa-client/ipa-install/ipa-client-install
+++ b/ipa-client/ipa-install/ipa-client-install
@@ -523,7 +523,8 @@ def uninstall(options, env):
 client_nss_nickname = client_nss_nickname_format % hostname
 
 # Always start certmonger. We can't untrack something if it isn't
-# running
+# running. Note that this is legacy code to untrack any certificates
+# that were created by previous versions of this installer.
 messagebus = services.knownservices.messagebus
 try:
 messagebus.start()
@@ -1093,69 +1094,6 @@ def configure_krb5_conf(cli_realm, cli_domain, cli_server, cli_kdc, dnsok,
 
 return 0
 
-def configure_certmonger(fstore, subject_base, cli_realm, hostname, options,
- remote_env):
-started = True
-principal = 'host/%s@%s' % (hostname, cli_realm)
-
-messagebus = services.knownservices.messagebus
-try:
-messagebus.start()
-except Exception, e:
-log_service_error(messagebus.service_name, 'start', e)
-
-# Ensure that certmonger has been started at least once to generate the
-# cas files in /var/lib/certmonger/cas.
-cmonger = services.knownservices.certmonger
-try:
-cmonger.restart()
-except Exception, e:
-log_service_error(cmonger.service_name, 'restart', e)
-
-if options.hostname:
-# It needs to be stopped if we touch them
-try:
-cmonger.stop()
-except Exception, e:
-log_service_error(cmonger.service_name, 'stop', e)
-# If the hostname is explicitly set then we need to tell certmonger
-# which principal name to use when requesting certs.
-certmonger.add_principal_to_cas(principal)
-
-try:
-cmonger.restart()
-except Exception, e:
-log_service_error(cmonger.service_name, 'restart', e)
-root_logger.warning(
-"Automatic certificate management will not be available")
-started = False
-
-try:
-cmonger.enable()
-except Exception, e:
-root_logger.error(
-"Failed to configure automatic startup of the %s daemon: %s",
-cmonger.service_name, str(e))
-root_logger.warning(
-"Automatic certificate management will not be available")
-
-# Request our host cert
-if remote_env['enable_ra']:
-if started:
-client_nss_nickname = client_nss_nickname_format % hostname
-subject = DN(('CN', hostname), subject_base)
-try:
-run(["ipa-getcert", "request", "-d", paths.NSS_DB_DIR,
- "-n", client_nss_nickname, "-N", str(subject),
- "-K", principal])
-except Exception:
-root_logger.error("%s request for host certificate failed",
-  cmonger.service_name)
-else:
-root_logger.warning(
-"A RA is not configured on the server. "
-"Not requesting host certificate.")
-
 def configure_sssd_conf(fstore, cli_realm, cli_domain, cli_server, options, client_domain, client_hostname):
 try:
 sssdconfig = SSSDConfig.SSSDConfig()
@@ -2690,8 +2628,6 @@ def install(options, env, fstore, statestore):
 
 if not options.on_master:
 client_dns(cli_server[0], hostname, options.dns_updates)
-configure_certmonger(fstore, subject_base, cli_realm, hostname,
- options, remote_env)
 
 update_ssh_keys(cli_server[0], hostname, services.knownservices.sshd.get_config_dir(), options.create_sshfp)
 
-- 
1.9.3

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

Re: [Freeipa-devel] [PATCH] 720-729 OTP usability improvements

[Endi Sukma Dewata]

On 8/22/2014 3:31 AM, Petr Vobornik wrote:

On 12.8.2014 17:59, Endi Sukma Dewata wrote:

On 8/5/2014 6:31 AM, Petr Vobornik wrote:>>

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


snip (ACK of 720, 721) but patch 720 was replaced by a new version


ACK.


[PATCH] 724 webui: display fields based on otp token type

- in adder dialog


is it an ACK?


Sorry, missed this one. ACK.


[PATCH] 727 webui: hide empty fields and sections

Hide widgets without a value. Must be explicitly turned on. In widget by
`hidden_if_empty` flag. Or globally by `hide_empty_widgets` flag. Global
hiding can be individually turned off by `ignore_empty_hiding` flag.


In item #2 of ticket #4402 I was originally thinking the widget
visibility would be determined by the token type.


Originally I wrote it that way but with this patch it became redundant.


Any plan to add the
token type field in the future?


maybe, I don't have any strong feelings about it. Will users benefit
from adding it? If yes, it should be also added to CLI.


Can the users tell which type of token they have based on the existing 
fields? There is a model field which is populated with the type (e.g. 
totp or hotp). But since the value can be changed to anything it's not a 
reliable way to determine the type. I don't think it's very 
user-friendly to ask the user to see whether the type-specific fields 
are shown in order to determine the type.


I can't say there's a big benefit by adding the field, but maybe some 
admins might find it useful, and it can be used to sort/filter out 
search results.



Atm. token type is derived from object classes. It exists only in add
operation as a virtual attribute. We can check a presence of
ipatokenhotp or ipatokentotp object classes to simulate the field.


Yes, if people can add an attribute usually they will expect to be able 
to see what they added.



Will the "counter" field strictly have a
value with HOTP only and "clock offset & interval" fields strictly have
value with TOTP only? Do these fields contain the configured values or
the effective values? I was just thinking maybe in the future some of
these fields might be configured empty and they will use the default
values instead. If that's not a problem then ACK.


Respective fields are present only in corresponding object classes ->
there won't be an HTOP token with 'clock offset'. If they are present,
they have a default value. -> No false hiding -> Shouldn't be a problem.

Btw: Other of my other original intents was to hide it based on ACI
rights. The issues is that the rights are not present without
corresponding OC. Hiding in such case is dangerous - explicitly disabled
in patch 728.

What do you think about setting `hide_empty_widgets` global setting to
True?


On a read-only page I think it's OK to hide empty fields. But on edit 
page I don't think we should do that by default.


Does "empty" mean "unset", or "blank" like empty string/array? Does 
"unset" always mean "non-editable and invisible" and "blank" always mean 
"editable and visible but it's just empty"? If this definition isn't 
strictly followed there could be an editable field that accidentally 
gets hidden because it's empty.


Generally if a field is supposed to be hidden in an edit page because of 
other condition (e.g. token type), not because of the value, the code 
should also use that condition instead of relying on the value being 
unset. In this particular case, instead of:


  if (field !== undefined) {
  // display field
  }

we probably should do:

  var type = ...

  if (type == 'hotp') {
  // display hotp fields

  } else { // totp
  // display totp fields
  }

For now the type can be determined by the availability of certain 
type-specific fields, but in the future we may add the type itself.



3. The "Clock offset" field doesn't have a unit.


Fixed in patch 720-1, patch 729 was rebased


ACK.


4. If no "Owner" is specified when adding a token, it will default to
admin. Is this the intended behavior?


Sadly yes.


Maybe the adder dialog should show admin (or the current user?) as the 
default owner instead of empty?


--
Endi S. Dewata

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

Re: [Freeipa-devel] [PATCH] 748 webui: extract complex pkey on Add and Edit

[Endi Sukma Dewata]

On 9/2/2014 10:15 AM, Petr Vobornik wrote:

DNS zone 'Add and Edit' failed because of new DNS name encoding.

This patch makes sure that keys are extracted properly.

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


ACK.

--
Endi S. Dewata

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

Re: [Freeipa-devel] [PATCH] 742 webui: adjust behavior of bounce url

[Endi Sukma Dewata]

On 8/21/2014 11:06 AM, Petr Vobornik wrote:

based  on:
http://www.redhat.com/archives/freeipa-devel/2014-August/msg00073.html

- bounce url param was renamed from 'redirect' to 'url'
- support for 'delay' param added

Behavior:

- "Continue to next page" link is shown if 'url' is present
- page is no longer automatically redirected if 'url' is present
- automatic redirect is controlled by 'delay' param - it specifies
   number of seconds until redirection
- info message 'You will be redirected in Xs' is show to notify
   the user that something will happen. It's useful even if delay
   is 0 or negative because redirection might be slow.
- counter is decremented every second
- delay is ignored if parsed as NaN

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


ACK.

Just one thing, when the delay=0 and the direction happens quickly, the 
users might see the confirmation and the redirection messages displayed 
briefly on the screen but they cannot read it because it's too quick, 
which might leave them wondering what it was.


I think delay=0 is a special case where we want a seamless integration 
with 3rd party application. If the password reset is completed 
successfully, it should just display the next page in the 3rd party 
application. Users shouldn't see a 'redirection' message. To them it's 
all one application.


--
Endi S. Dewata

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

Re: [Freeipa-devel] [PATCH] 743 webui: do not show login error when switching back from otp sync screen

[Endi Sukma Dewata]

On 8/22/2014 6:51 AM, Petr Vobornik wrote:

Errors should reflect only a result of last operation.

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

Fixes issue found by Endi:


Try logging in with an incorrect password/OTP. After you get a login
error click Sync OTP Token. Once the sync is completed it will go
back to the login page with a "Token was synchronized" message that
disappears in a few seconds, but the old login error still appears
which is confusing. Error messages in the UI should only reflect the
last executed operation.


ACK.

--
Endi S. Dewata

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