Re: [Freeipa-devel] [PATCH] 488-489 PermissionsV2 related winsync fixes

[Martin Kosek]

Adding freeipa-devel back.

On 01/14/2015 05:58 PM, Simo Sorce wrote:

On Wed, 14 Jan 2015 17:47:51 +0100
Martin Kosek  wrote:


-add:aci:'(targetfilter="(objectclass=nsContainer)")(version 3.0; acl
"Deny read access to replica configuration"; deny(read, search,
compare) userdn = "ldap:///anyone";;)'
+remove:aci:'(targetfilter="(objectclass=nsContainer)")(version 3.0;
acl "Deny read access to replica configuration"; deny(read, search,
compare) userdn = "ldap:///anyone";;)'


Why this removal ?


It is in the patch description. This container stores winsync "replicas". With 
this deny ACI, admin or anyone else besides Directory Manager can see the 
replicas as deny rules take precedence and this one is scoped for ldap://anyone.


My thinking was that this container is not too secret anyway, the only 
information that user get is name of the winsync'ed AD.



+dn: cn=config
+add:aci: '(version 3.0;acl "permission:Add Configuration
Sub-Entries";allow (add) groupdn = "ldap:///cn=Add Configuration
Sub-Entries,cn=permissions,cn=pbac,$SUFFIX";)'


Doesn't this allow REplication admin to add any object anywhere in
cn=config ?
This would be too broad.


It does. I wanted to narrow it with targetfilter '(targetfilter = 
"(cn=changelog5)")' but, it did not work for me, ADD was rejected. Not sure why 
though, when I used '(targetfilter = "(objectclass=extensibleobject)")', it 
worked fine.


I fear this is some problem in DS targetfilter evaluation during ADD operation, 
CCing Ludwig for reference.


Martin

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

Re: [Freeipa-devel] [PATCH 0002] Changing the token owner also changes its manager

[Nathaniel McCallum]

On Wed, 2015-01-14 at 17:49 +0100, Martin Babinsky wrote:
> On 01/14/2015 05:23 PM, Nathaniel McCallum wrote:
> > On Wed, 2015-01-14 at 16:49 +0100, Martin Babinsky wrote:
> >> Changing the owner of a token also implicitly sets the new owner as its
> >> manager if following conditions are met:
> >>
> >> 1.) The original token owner was also its manager
> >>
> >> 2.) The new manager is not set explicitly via CLI interface.
> >>
> >> If the owner is unset and the above conditions are met, then the manager
> >> of the token will also be unset.
> >>
> >> https://fedorahosted.org/freeipa/ticket/4681
> >
> > Nitpicks:
> > 1. The commit message summary line should not have a '.'
> > 2. The commit message is not properly wrapped.
> > 3. The newline before _normalize_owner() is undesirable.
> > 4. Shouldn't prev_managed_by be prev_managedby?
> >
> > The main body of the change looks good to me.
> >
> > Nathaniel
> >
> >
> >
> 
> Attaching updated patch.

ACK


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

Re: [Freeipa-devel] [PATCH 0039] Add test case for unsupported arg for ipa-advise

[Tomas Babej]

On 01/14/2015 06:13 PM, Gabe Alford wrote:
> On Wed, Jan 14, 2015 at 10:05 AM, Tomas Babej  > wrote:
>
>
> On 01/14/2015 06:00 PM, Tomas Babej wrote:
>>
>> On 01/14/2015 05:37 PM, Tomas Babej wrote:
>>>
>>> On 01/14/2015 02:55 PM, Gabe Alford wrote:
 Hello,

In looking into
 https://fedorahosted.org/freeipa/ticket/4029 I am wondering if
 there should be separate ipa-advise test, Yes/No? Could be
 handy in the future to test more ipa-advise output? Or should
 this test be added to the test_legacy_clients.py?

 Thanks,

 Gabe  

 On Tue, Dec 2, 2014 at 9:21 PM, Gabe Alford
 mailto:redhatri...@gmail.com>> wrote:

 Hello,

 I was going to try my hand at attempting a patch for
 ipa-tests. However in wanting to test my patch, I am not
 sure how to run ipa-tests to check if it works or not.
 Documentation is not really clear on what needs to be done
 to start a test and run a test. This is for
 https://fedorahosted.org/freeipa/ticket/4029

 I have attached the patch that I have yet to really test
 with ipa-test. Any help on how to test the patch running
 ipa-tests would be great. Of course, if one of the
 reviewers looks at the patch and looks good, then I would
 be happy with that as well.

 Thanks,

 Gabe




 ___
 Freeipa-devel mailing list
 Freeipa-devel@redhat.com 
 https://www.redhat.com/mailman/listinfo/freeipa-devel
>>>
>>> Hello,
>>>
>>> TL;DR: feel free to create a separate ipa-advise test file. Test
>>> requested in this ticket really does not belong to the legacy
>>> clients feature test.
>>>
>>> As for the any new tests that might come: I think tests for
>>> ipa-advise that are specific to that particular feature should
>>> be tested with that feature, more so, if they contain parts that
>>> are supposed to work copy-pasted. If a tests, however, tests a
>>> general behaviour of ipa-advise, it should live in the
>>> ipa-advise namespace, hence separate test file.
>>>
>>> HTH,
>>>
>>> -- 
>>> Tomas Babej
>>> Associate Software Engineer | Red Hat | Identity Management
>>> RHCE | Brno Site | IRC: tbabej | freeipa.org  
>>
>> The attached patch looks fine, although, please also test for a
>> non-zero return code number.
>>
>
> Upon hitting send I noticed you did not include raiseonerr=False
> into the run_command call. You need to do that, otherwise a
> exception will be raised, since ipa-advise exited with non-zero
> return code.
>
> Thanks Tomas.
>
> Which do you prefer: a test_advise.py or an update to the existing patch?

A new test file, as I pointed out in the second email :) sorry for
splitting.

However, it would be the best if you could spin up a positive test as
well (maybe listing out available advices), not just this negative one,
to justify the overhead reinstalling IPA for testing this feature.


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

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

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

Re: [Freeipa-devel] [PATCH 0039] Add test case for unsupported arg for ipa-advise

[Tomas Babej]

On 01/14/2015 06:00 PM, Tomas Babej wrote:
>
> On 01/14/2015 05:37 PM, Tomas Babej wrote:
>>
>> On 01/14/2015 02:55 PM, Gabe Alford wrote:
>>> Hello,
>>>
>>>In looking into https://fedorahosted.org/freeipa/ticket/4029
>>> I am wondering if there should be separate ipa-advise test, Yes/No?
>>> Could be handy in the future to test more ipa-advise output? Or
>>> should this test be added to the test_legacy_clients.py?
>>>
>>> Thanks,
>>>
>>> Gabe  
>>>
>>> On Tue, Dec 2, 2014 at 9:21 PM, Gabe Alford >> > wrote:
>>>
>>> Hello,
>>>
>>> I was going to try my hand at attempting a patch for ipa-tests.
>>> However in wanting to test my patch, I am not sure how to run
>>> ipa-tests to check if it works or not. Documentation is not
>>> really clear on what needs to be done to start a test and run a
>>> test. This is for https://fedorahosted.org/freeipa/ticket/4029
>>>
>>> I have attached the patch that I have yet to really test with
>>> ipa-test. Any help on how to test the patch running ipa-tests
>>> would be great. Of course, if one of the reviewers looks at the
>>> patch and looks good, then I would be happy with that as well.
>>>
>>> Thanks,
>>>
>>> Gabe
>>>
>>>
>>>
>>>
>>> ___
>>> Freeipa-devel mailing list
>>> Freeipa-devel@redhat.com
>>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>>
>> Hello,
>>
>> TL;DR: feel free to create a separate ipa-advise test file. Test
>> requested in this ticket really does not belong to the legacy clients
>> feature test.
>>
>> As for the any new tests that might come: I think tests for
>> ipa-advise that are specific to that particular feature should be
>> tested with that feature, more so, if they contain parts that are
>> supposed to work copy-pasted. If a tests, however, tests a general
>> behaviour of ipa-advise, it should live in the ipa-advise namespace,
>> hence separate test file.
>>
>> HTH,
>>
>> -- 
>> Tomas Babej
>> Associate Software Engineer | Red Hat | Identity Management
>> RHCE | Brno Site | IRC: tbabej | freeipa.org 
>
> The attached patch looks fine, although, please also test for a
> non-zero return code number.
>

Upon hitting send I noticed you did not include raiseonerr=False into
the run_command call. You need to do that, otherwise a exception will be
raised, since ipa-advise exited with non-zero return code.

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

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

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

Re: [Freeipa-devel] [PATCH 0039] Add test case for unsupported arg for ipa-advise

[Gabe Alford]

On Wed, Jan 14, 2015 at 10:05 AM, Tomas Babej  wrote:

>
> On 01/14/2015 06:00 PM, Tomas Babej wrote:
>
>
> On 01/14/2015 05:37 PM, Tomas Babej wrote:
>
>
> On 01/14/2015 02:55 PM, Gabe Alford wrote:
>
>   Hello,
>
> In looking into https://fedorahosted.org/freeipa/ticket/4029 I am
> wondering if there should be separate ipa-advise test, Yes/No? Could be
> handy in the future to test more ipa-advise output? Or should this test be
> added to the test_legacy_clients.py?
>
>  Thanks,
>
>  Gabe
>
> On Tue, Dec 2, 2014 at 9:21 PM, Gabe Alford  wrote:
>
>>  Hello,
>>
>> I was going to try my hand at attempting a patch for ipa-tests. However
>> in wanting to test my patch, I am not sure how to run ipa-tests to check if
>> it works or not. Documentation is not really clear on what needs to be done
>> to start a test and run a test. This is for
>> https://fedorahosted.org/freeipa/ticket/4029
>>
>>  I have attached the patch that I have yet to really test with ipa-test.
>> Any help on how to test the patch running ipa-tests would be great. Of
>> course, if one of the reviewers looks at the patch and looks good, then I
>> would be happy with that as well.
>>
>>  Thanks,
>>
>> Gabe
>>
>
>
>
> ___
> Freeipa-devel mailing 
> listFreeipa-devel@redhat.comhttps://www.redhat.com/mailman/listinfo/freeipa-devel
>
>
> Hello,
>
> TL;DR: feel free to create a separate ipa-advise test file. Test requested
> in this ticket really does not belong to the legacy clients feature test.
>
> As for the any new tests that might come: I think tests for ipa-advise
> that are specific to that particular feature should be tested with that
> feature, more so, if they contain parts that are supposed to work
> copy-pasted. If a tests, however, tests a general behaviour of ipa-advise,
> it should live in the ipa-advise namespace, hence separate test file.
>
> HTH,
>
> --
> Tomas Babej
> Associate Software Engineer | Red Hat | Identity Management
> RHCE | Brno Site | IRC: tbabej | freeipa.org
>
>
> The attached patch looks fine, although, please also test for a non-zero
> return code number.
>
>
> Upon hitting send I noticed you did not include raiseonerr=False into the
> run_command call. You need to do that, otherwise a exception will be
> raised, since ipa-advise exited with non-zero return code.
>
Thanks Tomas.

Which do you prefer: a test_advise.py or an update to the existing patch?

>
> --
> Tomas Babej
> Associate Software Engineer | Red Hat | Identity Management
> RHCE | Brno Site | IRC: tbabej | freeipa.org
>
>
> --
> Tomas Babej
> Associate Software Engineer | Red Hat | Identity Management
> RHCE | Brno Site | IRC: tbabej | freeipa.org
>
>
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH 0039] Add test case for unsupported arg for ipa-advise

[Tomas Babej]

On 01/14/2015 05:37 PM, Tomas Babej wrote:
>
> On 01/14/2015 02:55 PM, Gabe Alford wrote:
>> Hello,
>>
>>In looking into https://fedorahosted.org/freeipa/ticket/4029 I
>> am wondering if there should be separate ipa-advise test, Yes/No?
>> Could be handy in the future to test more ipa-advise output? Or
>> should this test be added to the test_legacy_clients.py?
>>
>> Thanks,
>>
>> Gabe  
>>
>> On Tue, Dec 2, 2014 at 9:21 PM, Gabe Alford > > wrote:
>>
>> Hello,
>>
>> I was going to try my hand at attempting a patch for ipa-tests.
>> However in wanting to test my patch, I am not sure how to run
>> ipa-tests to check if it works or not. Documentation is not
>> really clear on what needs to be done to start a test and run a
>> test. This is for https://fedorahosted.org/freeipa/ticket/4029
>>
>> I have attached the patch that I have yet to really test with
>> ipa-test. Any help on how to test the patch running ipa-tests
>> would be great. Of course, if one of the reviewers looks at the
>> patch and looks good, then I would be happy with that as well.
>>
>> Thanks,
>>
>> Gabe
>>
>>
>>
>>
>> ___
>> Freeipa-devel mailing list
>> Freeipa-devel@redhat.com
>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>
> Hello,
>
> TL;DR: feel free to create a separate ipa-advise test file. Test
> requested in this ticket really does not belong to the legacy clients
> feature test.
>
> As for the any new tests that might come: I think tests for ipa-advise
> that are specific to that particular feature should be tested with
> that feature, more so, if they contain parts that are supposed to work
> copy-pasted. If a tests, however, tests a general behaviour of
> ipa-advise, it should live in the ipa-advise namespace, hence separate
> test file.
>
> HTH,
>
> -- 
> Tomas Babej
> Associate Software Engineer | Red Hat | Identity Management
> RHCE | Brno Site | IRC: tbabej | freeipa.org 

The attached patch looks fine, although, please also test for a non-zero
return code number.


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

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

Re: [Freeipa-devel] [PATCH 0001] ipa-client-install: attempt to get host TGT several times before aborting client installation

[Martin Babinsky]

On 01/14/2015 04:53 PM, Martin Babinsky wrote:

On 01/13/2015 04:48 PM, Martin Babinsky wrote:

On 01/13/2015 09:46 AM, Jan Cholasta wrote:

Dne 13.1.2015 v 09:22 Martin Kosek napsal(a):

On 01/12/2015 05:45 PM, Martin Babinsky wrote:

related to ticket https://fedorahosted.org/freeipa/ticket/4808

Patch attached.

Martin^3


I think the --tgt-kinit-attempts approach is good one. Couple comments
I have
when reading the patch:

1) Function
+def get_host_tgt(options, keytab, host, realm, env):
should be made more general purpose and instead of whole "options", it
should
rather accept just "kinit_attemps". It will then enable future
generations to
reuse the function for something else. Just a generally good practice,
nothing
critical.

2) I think
+if returncode == 0:
+root_logger.info("Attempt %d succeeded." % n_attempts)
+break

can be just DEBUG level. People do not need to know we will try
multiple attempts.

3) It may be even better to print
"Attempt %d/%d failed." instead of just number. But this is up to you.

4) I see several C-isms in the code, as a programming practice, let us
remove
them :-) In Python, the OK/notOK status is generally passed via
exceptions, not
return codes unless you really need them for anything meaningful.

So, you can omit "raiseonerr=False" and have the handling code in an
Except
clause. When max number of attempts is breached, you then just raise
the
exception further (use bare "raise", to re-raise to keep the original
stack).


+1

Additionally:

5) I would prefer if the option was named --kinit-attempts instead of
--tgt-kinit-attempts (the "tgt" seems redundant).

6) Please do not use backslashes for line wrapping, unless it is
absolutely necessary. Instead, enclose the expression in parens for
implicit continuation:

+   help=("number of attempts to obtain host
TGT"
+ "if the first one fails (defaults to
%default)."))

7) Please follow PEP8 in new code:

ipa-client/ipa-install/ipa-client-install:151:80: E501 line too long (93
 > 79 characters)
ipa-client/ipa-install/ipa-client-install:1100:1: E302 expected 2 blank
lines, found 1
ipa-client/ipa-install/ipa-client-install:1107:29: E126 continuation
line over-indented for hanging indent
ipa-client/ipa-install/ipa-client-install:1107:41: E231 missing
whitespace after ','
ipa-client/ipa-install/ipa-client-install:1108:29: E128 continuation
line under-indented for visual indent
ipa-client/ipa-install/ipa-client-install:1116:51: E225 missing
whitespace around operator
ipa-client/ipa-install/ipa-client-install:2453:80: E501 line too long
(88 > 79 characters)
ipa-client/ipa-install/ipa-client-install:2454:80: E501 line too long
(89 > 79 characters)
ipa-client/ipa-install/ipa-client-install:2531:80: E501 line too long
(83 > 79 characters)
ipa-client/ipa-install/ipa-client-install:2532:80: E501 line too long
(81 > 79 characters)

Honza


Thank you for your comments. Attaching the updated patch (I have sent
the message much earlier, but only to Jan because I messed up the reply
addresses in Thunderbird. Sorry for that).

Martin


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



Attaching updated patch.

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

Martin^3



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



Forgot to update 'ipa-client-install' man page. The updated patch fixes 
this. Thanks to Rob for pointing it out.


Martin^3
From e7fc3c31840d5b14c29a027b418704a024d5db1d Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Fri, 9 Jan 2015 17:38:39 +0100
Subject: [PATCH] ipa-client-install: added new option '--kinit-attempts'

The option specifies a total number of attempts to obtain TGT from KDC before
giving up and aborting installation.

---
 ipa-client/ipa-install/ipa-client-install | 57 +--
 ipa-client/man/ipa-client-install.1   |  5 +++
 2 files changed, 51 insertions(+), 11 deletions(-)

diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install
index dfe0e3b7597c2ea63c299969b3a9d76cf8ecc273..55cb7e8356a441d9032f883e98b6cad059b2f1bf 100755
--- a/ipa-client/ipa-install/ipa-client-install
+++ b/ipa-client/ipa-install/ipa-client-install
@@ -91,6 +91,13 @@ def parse_options():
 
 parser.values.ca_cert_file = value
 
+def validate_kinit_attempts_option(option, opt, value, parser):
+if value < 1 or value > sys.maxint:
+raise OptionValueError(
+"%s option has invalid value %d" % (opt, value))
+
+parser.values.kinit_attempts = value
+
 parser = IPAOptionParser(version=version.VERSION)
 
 basic_group = OptionGroup(parser, "basic options")
@@ -144,6 +151,11 @@ def parse_options():

Re: [Freeipa-devel] [PATCH 0002] Changing the token owner also changes its manager

[Martin Babinsky]

On 01/14/2015 05:23 PM, Nathaniel McCallum wrote:

On Wed, 2015-01-14 at 16:49 +0100, Martin Babinsky wrote:

Changing the owner of a token also implicitly sets the new owner as its
manager if following conditions are met:

1.) The original token owner was also its manager

2.) The new manager is not set explicitly via CLI interface.

If the owner is unset and the above conditions are met, then the manager
of the token will also be unset.

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


Nitpicks:
1. The commit message summary line should not have a '.'
2. The commit message is not properly wrapped.
3. The newline before _normalize_owner() is undesirable.
4. Shouldn't prev_managed_by be prev_managedby?

The main body of the change looks good to me.

Nathaniel





Attaching updated patch.

Martin^3
From 9831945aa650bfe3509e89a9c1a2618ca49aaf6e Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Wed, 14 Jan 2015 15:57:45 +0100
Subject: [PATCH] Changing the token owner changes also the manager

This works if the change is made to a token which is owned and managed by the
same person. The new owner then automatically becomes token's manager unless
the attribute 'managedBy' is explicitly set otherwise.

https://fedorahosted.org/freeipa/ticket/4681
---
 ipalib/plugins/otptoken.py | 13 +
 1 file changed, 13 insertions(+)

diff --git a/ipalib/plugins/otptoken.py b/ipalib/plugins/otptoken.py
index 41a7f1087b783486704a066fe35e16a4db125bf2..b87145df80a3be9b16d596dd4072129c2290f40a 100644
--- a/ipalib/plugins/otptoken.py
+++ b/ipalib/plugins/otptoken.py
@@ -395,6 +395,19 @@ class otptoken_mod(LDAPUpdate):
   error='is after the validity end')
 _normalize_owner(self.api.Object.user, entry_attrs)
 
+# ticket #4681: if the owner of the token is changed and the
+# user also manages this token, then we should automatically
+# set the 'managedby' attribute to the new owner
+if 'ipatokenowner' in entry_attrs and 'managedby' not in entry_attrs:
+new_owner = entry_attrs.get('ipatokenowner', None)
+prev_entry = ldap.get_entry(dn, attrs_list=['ipatokenowner',
+'managedby'])
+prev_owner = prev_entry.get('ipatokenowner', None)
+prev_managedby = prev_entry.get('managedby', None)
+
+if (new_owner != prev_owner) and (prev_owner == prev_managedby):
+entry_attrs.setdefault('managedby', new_owner)
+
 attrs_list.append("objectclass")
 return dn
 
-- 
2.1.0

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

Re: [Freeipa-devel] [PATCH] 488-489 PermissionsV2 related winsync fixes

[Martin Kosek]

On 01/14/2015 03:58 PM, Martin Kosek wrote:
> On 01/14/2015 03:34 PM, Simo Sorce wrote:
>> On Wed, 14 Jan 2015 13:41:54 +0100
>> thierry bordaz  wrote:
>>
>>> On 01/14/2015 12:03 PM, Martin Kosek wrote:
 On 01/14/2015 10:58 AM, thierry bordaz wrote:
> On 01/14/2015 10:15 AM, Petr Viktorin wrote:
>> On 01/13/2015 10:52 PM, Martin Kosek wrote:
>>> On 01/13/2015 09:55 PM, Simo Sorce wrote:
 On Tue, 13 Jan 2015 18:16:11 +0100
 Martin Kosek  wrote:

> This is crude first version of the (working) fixes to fix
> Winsync/Passsync problems caused by the PermissionV2
> refactoring.
>
> Simo/Petr3 or others, any concerns?
>
 The first patch looks good
 the second looks .. broad ?

 Shouldn't you explicitly allow specific attributes ?
>>> You mean for:
>>>
>>> +'System: Read LDBM database config': {
>>> +'ipapermlocation': DN('cn=config'),
>>> +'ipapermtarget': DN('cn=config,cn=ldbm
>>> database,cn=plugins,cn=config'),
>>> +'ipapermbindruletype': 'permission',
>>> +'ipapermright': {'read', 'search', 'compare'},
>>> +'default_privileges': {'Replication Administrators'},
>>> +'ipapermdefaultattr': {'*'},
>>> +},
>>>
>>> ? I did that as my first try, but then the ACI was not accepted
>>> as the attribute I was looking for (nsslapd-changelogdir) is not
>>> in the schema as the config is just an extensibleObject. But as
>>> I was going through the attributes, I did not see anything
>>> super-secret.
>>>
>>> Petr, is there any way to make permission plugin accept unknown
>>> attribute in the permission attribute list, or do we need to use
>>> "*" in this case?
>> The ACL Syntax Error comes straight from the DS, so there's not
>> much IPA can do. The error suggests adding nsslapd-changelogdir
>> to the schema, but I'm not sure that's the right solution here.
>> Thierry, any comments? See the attached LDIF.
>>
> Actually this limitation was added with the bug
> https://bugzilla.redhat.com/show_bug.cgi?id=244229.
> I do not see in the bug, if the ability to define non schema
> attribute was creating a problem for IPA
 Not before, but with PermissionV2 and especially these patches, we
 may need to control access to unknown attributes in
 extensibleObject objects.
>>> One possibility is to revert that fix (with or without configuration 
>>> toggle). But then in a topology with mixed versions of DS, old DS
>>> will skipped those aci.
>>>
>>> Using '*' char is not nice but will guaranty a same evaluation on all 
>>> servers.
>>
>> We requested attribute validation when adding ACIs, w/o it it was very
>> simple to make typos, which would be fatal for DENY ACIs.
>>
>> The problem here is in using extensibleObject and not defining a
>> schema IMO.
>>
>> That said I am ok with the targetattr with appended asterisk to the
>> undefined attribute name.
>>
>> Simo.
> 
> After some thoughts, I agree with this path also. I will soon send the revised
> patches, with this and other improvements.

Attaching new, clean version of the patches, following this path. The ACIs are
now not as broad as before.

Originally, I added all new ACIs as V2 permissions, but then I realized it
would not work on replicas, as cn=config is not replicas and the ACIs stored
there would not be created as PermissionV2 object would already exist, when the
replica is being installed.

With attached patch set, "admin" user or "Replication Administrators" privilege
members should be able to create a winsync connection and PassSync user, e.g.:

[root@ipa ~]# ipa-replica-manage connect --winsync
--cacert=/home/mkosek/mkad2012.crt
--binddn='cn=Administrator,cn=users,dc=mkad2012,dc=test' --bindpw=Secret123
mkdc2012.mkad2012.test --passsync Secret123 -v
...
The user for the Windows PassSync service is
uid=passsync,cn=sysaccounts,cn=etc,dc=mkosek-f21,dc=test
Adding Windows PassSync system account
...
Connected 'ipa.mkosek-f21.test' to 'mkdc2012.mkad2012.test'


This should just complete and not crash. admin user should then also able to
list the winsync replica with

# ipa-replica-manage list

This fixes one bug. For testing the second ticket, PassSync one, either test
with PassSync software directly or verify that passsync system user can see NT
attribute and change user passwords:

# ldapsearch -D "uid=passsync,cn=sysaccounts,cn=etc,dc=mkosek-f21,dc=test" -x
-w Secret123 -b cn=users,cn=accounts,dc=mkosek-f21,dc=test
"(ntuserdomainid=testuser)" ntuserdomainid
# extended LDIF
#
# LDAPv3
# base  with scope subtree
# filter: (ntuserdomainid=testuser)
# requesting: ntuserdomainid
#

# testuser, users, accounts, mkosek-f21.test
dn: uid=testuser,cn=users,cn=accounts,dc=mkosek-f21,dc=test
ntuserdomainid: testuser

# search result
search: 2
result: 0 Success

# numResponses: 2
# nu

Re: [Freeipa-devel] [PATCH 0039] Add test case for unsupported arg for ipa-advise

[Tomas Babej]

On 01/14/2015 02:55 PM, Gabe Alford wrote:
> Hello,
>
>In looking into https://fedorahosted.org/freeipa/ticket/4029 I
> am wondering if there should be separate ipa-advise test, Yes/No?
> Could be handy in the future to test more ipa-advise output? Or should
> this test be added to the test_legacy_clients.py?
>
> Thanks,
>
> Gabe  
>
> On Tue, Dec 2, 2014 at 9:21 PM, Gabe Alford  > wrote:
>
> Hello,
>
> I was going to try my hand at attempting a patch for ipa-tests.
> However in wanting to test my patch, I am not sure how to run
> ipa-tests to check if it works or not. Documentation is not really
> clear on what needs to be done to start a test and run a test.
> This is for https://fedorahosted.org/freeipa/ticket/4029
>
> I have attached the patch that I have yet to really test with
> ipa-test. Any help on how to test the patch running ipa-tests
> would be great. Of course, if one of the reviewers looks at the
> patch and looks good, then I would be happy with that as well.
>
> Thanks,
>
> Gabe
>
>
>
>
> ___
> Freeipa-devel mailing list
> Freeipa-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel

Hello,

TL;DR: feel free to create a separate ipa-advise test file. Test
requested in this ticket really does not belong to the legacy clients
feature test.

As for the any new tests that might come: I think tests for ipa-advise
that are specific to that particular feature should be tested with that
feature, more so, if they contain parts that are supposed to work
copy-pasted. If a tests, however, tests a general behaviour of
ipa-advise, it should live in the ipa-advise namespace, hence separate
test file.

HTH,

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

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

Re: [Freeipa-devel] [PATCH 0002] Changing the token owner also changes its manager

[Nathaniel McCallum]

On Wed, 2015-01-14 at 16:49 +0100, Martin Babinsky wrote:
> Changing the owner of a token also implicitly sets the new owner as its 
> manager if following conditions are met:
> 
> 1.) The original token owner was also its manager
> 
> 2.) The new manager is not set explicitly via CLI interface.
> 
> If the owner is unset and the above conditions are met, then the manager 
> of the token will also be unset.
> 
> https://fedorahosted.org/freeipa/ticket/4681

Nitpicks:
1. The commit message summary line should not have a '.'
2. The commit message is not properly wrapped.
3. The newline before _normalize_owner() is undesirable.
4. Shouldn't prev_managed_by be prev_managedby?

The main body of the change looks good to me.

Nathaniel



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

Re: [Freeipa-devel] [PATCH 0170, 0183] Detect and warn about invalid forwardzone configuration

[Martin Basti]

On 12/12/14 13:52, Martin Basti wrote:

On 12/12/14 13:50, Martin Kosek wrote:

On 12/11/2014 05:44 PM, Petr Spacek wrote:

On 11.12.2014 16:50, Martin Basti wrote:

Updated aptch attached:


Nice work, ACK!



Can we also add some tests? This is a lot of new code that could 
break stuff.


We can, in week maybe or after christmas,  currently I do some work 
with tests and adding new tests required by QE, I will add forwardzone 
warnings  tests when finish this.




Added tests (required patch 0170).
Original and new patch attached.

--
Martin Basti

From f1d2f1b8b72934511e50fdd6f8fd72553c43dc58 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 14 Jan 2015 17:06:56 +0100
Subject: [PATCH] DNS tests: warning if forward zone is inactive

Ticket: https://fedorahosted.org/freeipa/ticket/4721
---
 ipatests/test_xmlrpc/test_dns_plugin.py | 468 
 1 file changed, 468 insertions(+)

diff --git a/ipatests/test_xmlrpc/test_dns_plugin.py b/ipatests/test_xmlrpc/test_dns_plugin.py
index dcefd2c6468d2af61dc6e8c5f196909fdc4bfdeb..986204afad6bfef8170aa850fc46d0df0797c699 100644
--- a/ipatests/test_xmlrpc/test_dns_plugin.py
+++ b/ipatests/test_xmlrpc/test_dns_plugin.py
@@ -59,6 +59,21 @@ zone1_permission_dn = DN(('cn',zone1_permission),
 api.env.container_permission,api.env.basedn)
 zone1_txtrec_dn = DN(('idnsname', '_kerberos'), zone1_dn)
 
+zone1_sub = u'sub.%s' % zone1_absolute
+zone1_sub_dnsname = DNSName(zone1_sub)
+zone1_sub_dn = DN(('idnsname', zone1_sub),
+  api.env.container_dns, api.env.basedn)
+
+zone1_sub_fw = u'fw.%s' % zone1_sub
+zone1_sub_fw_dnsname = DNSName(zone1_sub_fw)
+zone1_sub_fw_dn = DN(('idnsname', zone1_sub_fw),
+ api.env.container_dns, api.env.basedn)
+
+zone1_sub2_fw = u'fw.sub2.%s' % zone1_sub
+zone1_sub2_fw_dnsname = DNSName(zone1_sub2_fw)
+zone1_sub2_fw_dn = DN(('idnsname', zone1_sub2_fw),
+  api.env.container_dns, api.env.basedn)
+
 zone2 = u'zone2.test'
 zone2_dnsname = DNSName(zone2)
 zone2_absolute = u'%s.' % zone2
@@ -4662,3 +4677,456 @@ class test_forward_master_zones_mutual_exlusion(Declarative):
 ),
 
 ]
+
+
+class test_forwardzone_delegation_warnings(Declarative):
+
+@classmethod
+def setUpClass(cls):
+super(test_forwardzone_delegation_warnings, cls).setUpClass()
+
+if not api.Backend.rpcclient.isconnected():
+api.Backend.rpcclient.connect(fallback=False)
+
+if not have_ldap2:
+raise nose.SkipTest('server plugin not available')
+
+try:
+api.Command['dnszone_add'](zone1, idnssoarname=zone1_rname,)
+api.Command['dnszone_del'](zone1)
+except errors.NotFound:
+raise nose.SkipTest('DNS is not configured')
+except errors.DuplicateEntry:
+pass
+
+
+cleanup_commands = [
+('dnsforwardzone_del', [zone1_sub_fw, zone1_sub2_fw],
+{'continue': True}),
+('dnszone_del', [zone1, zone1_sub],
+{'continue': True}),
+]
+
+tests = [
+
+dict(
+desc='Create forward zone %r without forwarders with "none" '
+ 'policy' % zone1_sub_fw,
+command=(
+'dnsforwardzone_add', [zone1_sub_fw],
+{'idnsforwardpolicy': u'none'}
+),
+expected={
+'value': zone1_sub_fw_dnsname,
+'summary': None,
+'result': {
+'dn': zone1_sub_fw_dn,
+'idnsname': [zone1_sub_fw_dnsname],
+'idnszoneactive': [u'TRUE'],
+'idnsforwardpolicy': [u'none'],
+'objectclass': objectclasses.dnsforwardzone,
+},
+},
+),
+
+
+dict(
+desc='Create zone %r (expected warning for %r)' % (zone1,
+   zone1_sub_fw),
+command=(
+'dnszone_add', [zone1_absolute], {}
+),
+expected={
+'value': zone1_absolute_dnsname,
+'summary': None,
+'result': {
+'dn': zone1_dn,
+'idnsname': [zone1_absolute_dnsname],
+'idnszoneactive': [u'TRUE'],
+'idnssoamname': lambda x: True,  # don't care in this test
+'nsrecord': lambda x: True,  # don't care in this test
+'idnssoarname': lambda x: True,  # don't care in this test
+'idnssoaserial': [fuzzy_digits],
+'idnssoarefresh': [fuzzy_digits],
+'idnssoaretry': [fuzzy_digits],
+'idnssoaexpire': [fuzzy_digits],
+'idnssoaminimum': [fuzzy_digits],
+'idnsallowdynupdate': [u'FALSE'],
+'idnsupdatepolicy': lambda x: True,  # don't c

Re: [Freeipa-devel] [PATCH] 488-489 PermissionsV2 related winsync fixes

[Martin Kosek]

On 01/14/2015 03:42 PM, Alexander Bokovoy wrote:
> On Wed, 14 Jan 2015, Simo Sorce wrote:
>> On Wed, 14 Jan 2015 13:41:54 +0100
>> thierry bordaz  wrote:
>>
>>> On 01/14/2015 12:03 PM, Martin Kosek wrote:
>>> > On 01/14/2015 10:58 AM, thierry bordaz wrote:
>>> >> On 01/14/2015 10:15 AM, Petr Viktorin wrote:
>>> >>> On 01/13/2015 10:52 PM, Martin Kosek wrote:
>>>  On 01/13/2015 09:55 PM, Simo Sorce wrote:
>>> > On Tue, 13 Jan 2015 18:16:11 +0100
>>> > Martin Kosek  wrote:
>>> >
>>> >> This is crude first version of the (working) fixes to fix
>>> >> Winsync/Passsync problems caused by the PermissionV2
>>> >> refactoring.
>>> >>
>>> >> Simo/Petr3 or others, any concerns?
>>> >>
>>> > The first patch looks good
>>> > the second looks .. broad ?
>>> >
>>> > Shouldn't you explicitly allow specific attributes ?
>>>  You mean for:
>>> 
>>>  +'System: Read LDBM database config': {
>>>  +'ipapermlocation': DN('cn=config'),
>>>  +'ipapermtarget': DN('cn=config,cn=ldbm
>>>  database,cn=plugins,cn=config'),
>>>  +'ipapermbindruletype': 'permission',
>>>  +'ipapermright': {'read', 'search', 'compare'},
>>>  +'default_privileges': {'Replication Administrators'},
>>>  +'ipapermdefaultattr': {'*'},
>>>  +},
>>> 
>>>  ? I did that as my first try, but then the ACI was not accepted
>>>  as the attribute I was looking for (nsslapd-changelogdir) is not
>>>  in the schema as the config is just an extensibleObject. But as
>>>  I was going through the attributes, I did not see anything
>>>  super-secret.
>>> 
>>>  Petr, is there any way to make permission plugin accept unknown
>>>  attribute in the permission attribute list, or do we need to use
>>>  "*" in this case?
>>> >>> The ACL Syntax Error comes straight from the DS, so there's not
>>> >>> much IPA can do. The error suggests adding nsslapd-changelogdir
>>> >>> to the schema, but I'm not sure that's the right solution here.
>>> >>> Thierry, any comments? See the attached LDIF.
>>> >>>
>>> >> Actually this limitation was added with the bug
>>> >> https://bugzilla.redhat.com/show_bug.cgi?id=244229.
>>> >> I do not see in the bug, if the ability to define non schema
>>> >> attribute was creating a problem for IPA
>>> > Not before, but with PermissionV2 and especially these patches, we
>>> > may need to control access to unknown attributes in
>>> > extensibleObject objects.
>>> One possibility is to revert that fix (with or without configuration
>>> toggle). But then in a topology with mixed versions of DS, old DS
>>> will skipped those aci.
>>>
>>> Using '*' char is not nice but will guaranty a same evaluation on all
>>> servers.
>>
>> We requested attribute validation when adding ACIs, w/o it it was very
>> simple to make typos, which would be fatal for DENY ACIs.
>>
>> The problem here is in using extensibleObject and not defining a
>> schema IMO.
>>
>> That said I am ok with the targetattr with appended asterisk to the
>> undefined attribute name.
> +1. Another alternative is to use some symbol that could not be present
> in the attribute name at the beginning of the targetattr to switch off
> the schema checks, e.g. targetattr=">nssldapd-changelogdir".

Right, but the disadvantage is that ACIs following that approach would only
work on new servers...

Martin

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

Re: [Freeipa-devel] [PATCH 0001] ipa-client-install: attempt to get host TGT several times before aborting client installation

[Martin Babinsky]

On 01/13/2015 04:48 PM, Martin Babinsky wrote:

On 01/13/2015 09:46 AM, Jan Cholasta wrote:

Dne 13.1.2015 v 09:22 Martin Kosek napsal(a):

On 01/12/2015 05:45 PM, Martin Babinsky wrote:

related to ticket https://fedorahosted.org/freeipa/ticket/4808

Patch attached.

Martin^3


I think the --tgt-kinit-attempts approach is good one. Couple comments
I have
when reading the patch:

1) Function
+def get_host_tgt(options, keytab, host, realm, env):
should be made more general purpose and instead of whole "options", it
should
rather accept just "kinit_attemps". It will then enable future
generations to
reuse the function for something else. Just a generally good practice,
nothing
critical.

2) I think
+if returncode == 0:
+root_logger.info("Attempt %d succeeded." % n_attempts)
+break

can be just DEBUG level. People do not need to know we will try
multiple attempts.

3) It may be even better to print
"Attempt %d/%d failed." instead of just number. But this is up to you.

4) I see several C-isms in the code, as a programming practice, let us
remove
them :-) In Python, the OK/notOK status is generally passed via
exceptions, not
return codes unless you really need them for anything meaningful.

So, you can omit "raiseonerr=False" and have the handling code in an
Except
clause. When max number of attempts is breached, you then just raise the
exception further (use bare "raise", to re-raise to keep the original
stack).


+1

Additionally:

5) I would prefer if the option was named --kinit-attempts instead of
--tgt-kinit-attempts (the "tgt" seems redundant).

6) Please do not use backslashes for line wrapping, unless it is
absolutely necessary. Instead, enclose the expression in parens for
implicit continuation:

+   help=("number of attempts to obtain host TGT"
+ "if the first one fails (defaults to
%default)."))

7) Please follow PEP8 in new code:

ipa-client/ipa-install/ipa-client-install:151:80: E501 line too long (93
 > 79 characters)
ipa-client/ipa-install/ipa-client-install:1100:1: E302 expected 2 blank
lines, found 1
ipa-client/ipa-install/ipa-client-install:1107:29: E126 continuation
line over-indented for hanging indent
ipa-client/ipa-install/ipa-client-install:1107:41: E231 missing
whitespace after ','
ipa-client/ipa-install/ipa-client-install:1108:29: E128 continuation
line under-indented for visual indent
ipa-client/ipa-install/ipa-client-install:1116:51: E225 missing
whitespace around operator
ipa-client/ipa-install/ipa-client-install:2453:80: E501 line too long
(88 > 79 characters)
ipa-client/ipa-install/ipa-client-install:2454:80: E501 line too long
(89 > 79 characters)
ipa-client/ipa-install/ipa-client-install:2531:80: E501 line too long
(83 > 79 characters)
ipa-client/ipa-install/ipa-client-install:2532:80: E501 line too long
(81 > 79 characters)

Honza


Thank you for your comments. Attaching the updated patch (I have sent
the message much earlier, but only to Jan because I messed up the reply
addresses in Thunderbird. Sorry for that).

Martin


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



Attaching updated patch.

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

Martin^3

From a6ade537190215accaaf1bc534d25383ca7cc787 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Fri, 9 Jan 2015 17:38:39 +0100
Subject: [PATCH] ipa-client-install: added new option '--kinit-attempts'.

The option specifies a total number of attempts to obtain TGT from KDC before giving up and aborting installation.

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

---
 ipa-client/ipa-install/ipa-client-install | 57 +--
 1 file changed, 46 insertions(+), 11 deletions(-)

diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install
index dfe0e3b7597c2ea63c299969b3a9d76cf8ecc273..55cb7e8356a441d9032f883e98b6cad059b2f1bf 100755
--- a/ipa-client/ipa-install/ipa-client-install
+++ b/ipa-client/ipa-install/ipa-client-install
@@ -91,6 +91,13 @@ def parse_options():
 
 parser.values.ca_cert_file = value
 
+def validate_kinit_attempts_option(option, opt, value, parser):
+if value < 1 or value > sys.maxint:
+raise OptionValueError(
+"%s option has invalid value %d" % (opt, value))
+
+parser.values.kinit_attempts = value
+
 parser = IPAOptionParser(version=version.VERSION)
 
 basic_group = OptionGroup(parser, "basic options")
@@ -144,6 +151,11 @@ def parse_options():
   help="do not modify the nsswitch.conf and PAM configuration")
 basic_group.add_option("-f", "--force", dest="force", action="store_true",
   default=False, help="force setting of LDAP/Kerberos conf")
+basic_group.add_option('--kinit-attempts', dest='kinit_attempts',
+   action='call

[Freeipa-devel] [PATCH 0002] Changing the token owner also changes its manager

[Martin Babinsky]

Changing the owner of a token also implicitly sets the new owner as its 
manager if following conditions are met:


1.) The original token owner was also its manager

2.) The new manager is not set explicitly via CLI interface.

If the owner is unset and the above conditions are met, then the manager 
of the token will also be unset.


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

Martin^3
From c6d32af7359f29b53f518ce1c0b66e64e78f9566 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Wed, 14 Jan 2015 15:57:45 +0100
Subject: [PATCH] Changing the token owner changes also the manager.

This works if the change is made to a token which is owned and managed by the same person. The new owner then automatically becomes token's manager unless the attribute 'managedBy' is explicitly set otherwise.

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

---
 ipalib/plugins/otptoken.py | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/ipalib/plugins/otptoken.py b/ipalib/plugins/otptoken.py
index 41a7f1087b783486704a066fe35e16a4db125bf2..3b25ed35962e143a8bdb86b138d4143c6d98fab8 100644
--- a/ipalib/plugins/otptoken.py
+++ b/ipalib/plugins/otptoken.py
@@ -393,8 +393,22 @@ class otptoken_mod(LDAPUpdate):
 else:
 raise ValidationError(name='not_before',
   error='is after the validity end')
+
 _normalize_owner(self.api.Object.user, entry_attrs)
 
+# ticket #4681: if the owner of the token is changed and the
+# user also manages this token, then we should automatically
+# set the 'managedby' attribute to the new owner
+if 'ipatokenowner' in entry_attrs and 'managedby' not in entry_attrs:
+new_owner = entry_attrs.get('ipatokenowner', None)
+prev_entry = ldap.get_entry(dn, attrs_list=['ipatokenowner',
+'managedby'])
+prev_owner = prev_entry.get('ipatokenowner', None)
+prev_managed_by = prev_entry.get('managedby', None)
+
+if (new_owner != prev_owner) and (prev_owner == prev_managed_by):
+entry_attrs.setdefault('managedby', new_owner)
+
 attrs_list.append("objectclass")
 return dn
 
-- 
2.1.0

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

Re: [Freeipa-devel] [PATCH] 488-489 PermissionsV2 related winsync fixes

[Martin Kosek]

On 01/14/2015 03:34 PM, Simo Sorce wrote:
> On Wed, 14 Jan 2015 13:41:54 +0100
> thierry bordaz  wrote:
> 
>> On 01/14/2015 12:03 PM, Martin Kosek wrote:
>>> On 01/14/2015 10:58 AM, thierry bordaz wrote:
 On 01/14/2015 10:15 AM, Petr Viktorin wrote:
> On 01/13/2015 10:52 PM, Martin Kosek wrote:
>> On 01/13/2015 09:55 PM, Simo Sorce wrote:
>>> On Tue, 13 Jan 2015 18:16:11 +0100
>>> Martin Kosek  wrote:
>>>
 This is crude first version of the (working) fixes to fix
 Winsync/Passsync problems caused by the PermissionV2
 refactoring.

 Simo/Petr3 or others, any concerns?

>>> The first patch looks good
>>> the second looks .. broad ?
>>>
>>> Shouldn't you explicitly allow specific attributes ?
>> You mean for:
>>
>> +'System: Read LDBM database config': {
>> +'ipapermlocation': DN('cn=config'),
>> +'ipapermtarget': DN('cn=config,cn=ldbm
>> database,cn=plugins,cn=config'),
>> +'ipapermbindruletype': 'permission',
>> +'ipapermright': {'read', 'search', 'compare'},
>> +'default_privileges': {'Replication Administrators'},
>> +'ipapermdefaultattr': {'*'},
>> +},
>>
>> ? I did that as my first try, but then the ACI was not accepted
>> as the attribute I was looking for (nsslapd-changelogdir) is not
>> in the schema as the config is just an extensibleObject. But as
>> I was going through the attributes, I did not see anything
>> super-secret.
>>
>> Petr, is there any way to make permission plugin accept unknown
>> attribute in the permission attribute list, or do we need to use
>> "*" in this case?
> The ACL Syntax Error comes straight from the DS, so there's not
> much IPA can do. The error suggests adding nsslapd-changelogdir
> to the schema, but I'm not sure that's the right solution here.
> Thierry, any comments? See the attached LDIF.
>
 Actually this limitation was added with the bug
 https://bugzilla.redhat.com/show_bug.cgi?id=244229.
 I do not see in the bug, if the ability to define non schema
 attribute was creating a problem for IPA
>>> Not before, but with PermissionV2 and especially these patches, we
>>> may need to control access to unknown attributes in
>>> extensibleObject objects.
>> One possibility is to revert that fix (with or without configuration 
>> toggle). But then in a topology with mixed versions of DS, old DS
>> will skipped those aci.
>>
>> Using '*' char is not nice but will guaranty a same evaluation on all 
>> servers.
> 
> We requested attribute validation when adding ACIs, w/o it it was very
> simple to make typos, which would be fatal for DENY ACIs.
> 
> The problem here is in using extensibleObject and not defining a
> schema IMO.
> 
> That said I am ok with the targetattr with appended asterisk to the
> undefined attribute name.
> 
> Simo.

After some thoughts, I agree with this path also. I will soon send the revised
patches, with this and other improvements.

Martin

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

Re: [Freeipa-devel] [PATCH] 488-489 PermissionsV2 related winsync fixes

[Alexander Bokovoy]

On Wed, 14 Jan 2015, Simo Sorce wrote:

On Wed, 14 Jan 2015 13:41:54 +0100
thierry bordaz  wrote:


On 01/14/2015 12:03 PM, Martin Kosek wrote:
> On 01/14/2015 10:58 AM, thierry bordaz wrote:
>> On 01/14/2015 10:15 AM, Petr Viktorin wrote:
>>> On 01/13/2015 10:52 PM, Martin Kosek wrote:
 On 01/13/2015 09:55 PM, Simo Sorce wrote:
> On Tue, 13 Jan 2015 18:16:11 +0100
> Martin Kosek  wrote:
>
>> This is crude first version of the (working) fixes to fix
>> Winsync/Passsync problems caused by the PermissionV2
>> refactoring.
>>
>> Simo/Petr3 or others, any concerns?
>>
> The first patch looks good
> the second looks .. broad ?
>
> Shouldn't you explicitly allow specific attributes ?
 You mean for:

 +'System: Read LDBM database config': {
 +'ipapermlocation': DN('cn=config'),
 +'ipapermtarget': DN('cn=config,cn=ldbm
 database,cn=plugins,cn=config'),
 +'ipapermbindruletype': 'permission',
 +'ipapermright': {'read', 'search', 'compare'},
 +'default_privileges': {'Replication Administrators'},
 +'ipapermdefaultattr': {'*'},
 +},

 ? I did that as my first try, but then the ACI was not accepted
 as the attribute I was looking for (nsslapd-changelogdir) is not
 in the schema as the config is just an extensibleObject. But as
 I was going through the attributes, I did not see anything
 super-secret.

 Petr, is there any way to make permission plugin accept unknown
 attribute in the permission attribute list, or do we need to use
 "*" in this case?
>>> The ACL Syntax Error comes straight from the DS, so there's not
>>> much IPA can do. The error suggests adding nsslapd-changelogdir
>>> to the schema, but I'm not sure that's the right solution here.
>>> Thierry, any comments? See the attached LDIF.
>>>
>> Actually this limitation was added with the bug
>> https://bugzilla.redhat.com/show_bug.cgi?id=244229.
>> I do not see in the bug, if the ability to define non schema
>> attribute was creating a problem for IPA
> Not before, but with PermissionV2 and especially these patches, we
> may need to control access to unknown attributes in
> extensibleObject objects.
One possibility is to revert that fix (with or without configuration
toggle). But then in a topology with mixed versions of DS, old DS
will skipped those aci.

Using '*' char is not nice but will guaranty a same evaluation on all
servers.


We requested attribute validation when adding ACIs, w/o it it was very
simple to make typos, which would be fatal for DENY ACIs.

The problem here is in using extensibleObject and not defining a
schema IMO.

That said I am ok with the targetattr with appended asterisk to the
undefined attribute name.

+1. Another alternative is to use some symbol that could not be present
in the attribute name at the beginning of the targetattr to switch off
the schema checks, e.g. targetattr=">nssldapd-changelogdir".
--
/ Alexander Bokovoy

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

Re: [Freeipa-devel] [PATCH] 488-489 PermissionsV2 related winsync fixes

[Simo Sorce]

On Wed, 14 Jan 2015 13:41:54 +0100
thierry bordaz  wrote:

> On 01/14/2015 12:03 PM, Martin Kosek wrote:
> > On 01/14/2015 10:58 AM, thierry bordaz wrote:
> >> On 01/14/2015 10:15 AM, Petr Viktorin wrote:
> >>> On 01/13/2015 10:52 PM, Martin Kosek wrote:
>  On 01/13/2015 09:55 PM, Simo Sorce wrote:
> > On Tue, 13 Jan 2015 18:16:11 +0100
> > Martin Kosek  wrote:
> >
> >> This is crude first version of the (working) fixes to fix
> >> Winsync/Passsync problems caused by the PermissionV2
> >> refactoring.
> >>
> >> Simo/Petr3 or others, any concerns?
> >>
> > The first patch looks good
> > the second looks .. broad ?
> >
> > Shouldn't you explicitly allow specific attributes ?
>  You mean for:
> 
>  +'System: Read LDBM database config': {
>  +'ipapermlocation': DN('cn=config'),
>  +'ipapermtarget': DN('cn=config,cn=ldbm
>  database,cn=plugins,cn=config'),
>  +'ipapermbindruletype': 'permission',
>  +'ipapermright': {'read', 'search', 'compare'},
>  +'default_privileges': {'Replication Administrators'},
>  +'ipapermdefaultattr': {'*'},
>  +},
> 
>  ? I did that as my first try, but then the ACI was not accepted
>  as the attribute I was looking for (nsslapd-changelogdir) is not
>  in the schema as the config is just an extensibleObject. But as
>  I was going through the attributes, I did not see anything
>  super-secret.
> 
>  Petr, is there any way to make permission plugin accept unknown
>  attribute in the permission attribute list, or do we need to use
>  "*" in this case?
> >>> The ACL Syntax Error comes straight from the DS, so there's not
> >>> much IPA can do. The error suggests adding nsslapd-changelogdir
> >>> to the schema, but I'm not sure that's the right solution here.
> >>> Thierry, any comments? See the attached LDIF.
> >>>
> >> Actually this limitation was added with the bug
> >> https://bugzilla.redhat.com/show_bug.cgi?id=244229.
> >> I do not see in the bug, if the ability to define non schema
> >> attribute was creating a problem for IPA
> > Not before, but with PermissionV2 and especially these patches, we
> > may need to control access to unknown attributes in
> > extensibleObject objects.
> One possibility is to revert that fix (with or without configuration 
> toggle). But then in a topology with mixed versions of DS, old DS
> will skipped those aci.
> 
> Using '*' char is not nice but will guaranty a same evaluation on all 
> servers.

We requested attribute validation when adding ACIs, w/o it it was very
simple to make typos, which would be fatal for DENY ACIs.

The problem here is in using extensibleObject and not defining a
schema IMO.

That said I am ok with the targetattr with appended asterisk to the
undefined attribute name.

Simo.

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

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

Re: [Freeipa-devel] [PATCH 0039] Add test case for unsupported arg for ipa-advise

[Gabe Alford]

Hello,

   In looking into https://fedorahosted.org/freeipa/ticket/4029 I am
wondering if there should be separate ipa-advise test, Yes/No? Could be
handy in the future to test more ipa-advise output? Or should this test be
added to the test_legacy_clients.py?

Thanks,

Gabe

On Tue, Dec 2, 2014 at 9:21 PM, Gabe Alford  wrote:

> Hello,
>
> I was going to try my hand at attempting a patch for ipa-tests. However in
> wanting to test my patch, I am not sure how to run ipa-tests to check if it
> works or not. Documentation is not really clear on what needs to be done to
> start a test and run a test. This is for
> https://fedorahosted.org/freeipa/ticket/4029
>
> I have attached the patch that I have yet to really test with ipa-test.
> Any help on how to test the patch running ipa-tests would be great. Of
> course, if one of the reviewers looks at the patch and looks good, then I
> would be happy with that as well.
>
> Thanks,
>
> Gabe
>
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 488-489 PermissionsV2 related winsync fixes

[thierry bordaz]

On 01/14/2015 12:03 PM, Martin Kosek wrote:

On 01/14/2015 10:58 AM, thierry bordaz wrote:

On 01/14/2015 10:15 AM, Petr Viktorin wrote:

On 01/13/2015 10:52 PM, Martin Kosek wrote:

On 01/13/2015 09:55 PM, Simo Sorce wrote:

On Tue, 13 Jan 2015 18:16:11 +0100
Martin Kosek  wrote:


This is crude first version of the (working) fixes to fix
Winsync/Passsync problems caused by the PermissionV2 refactoring.

Simo/Petr3 or others, any concerns?


The first patch looks good
the second looks .. broad ?

Shouldn't you explicitly allow specific attributes ?

You mean for:

+'System: Read LDBM database config': {
+'ipapermlocation': DN('cn=config'),
+'ipapermtarget': DN('cn=config,cn=ldbm
database,cn=plugins,cn=config'),
+'ipapermbindruletype': 'permission',
+'ipapermright': {'read', 'search', 'compare'},
+'default_privileges': {'Replication Administrators'},
+'ipapermdefaultattr': {'*'},
+},

? I did that as my first try, but then the ACI was not accepted as the
attribute I was looking for (nsslapd-changelogdir) is not in the schema
as the config is just an extensibleObject. But as I was going through
the attributes, I did not see anything super-secret.

Petr, is there any way to make permission plugin accept unknown
attribute in the permission attribute list, or do we need to use "*" in
this case?

The ACL Syntax Error comes straight from the DS, so there's not much IPA can
do. The error suggests adding nsslapd-changelogdir to the schema, but I'm not
sure that's the right solution here.
Thierry, any comments? See the attached LDIF.


Actually this limitation was added with the bug
https://bugzilla.redhat.com/show_bug.cgi?id=244229.
I do not see in the bug, if the ability to define non schema attribute was
creating a problem for IPA

Not before, but with PermissionV2 and especially these patches, we may need to
control access to unknown attributes in extensibleObject objects.
One possibility is to revert that fix (with or without configuration 
toggle). But then in a topology with mixed versions of DS, old DS  will 
skipped those aci.


Using '*' char is not nice but will guaranty a same evaluation on all 
servers.
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCHES] 0684-0687 Use tracker fixture in host plugin tests

[Tomas Babej]

On 01/06/2015 03:49 PM, Petr Viktorin wrote:
> The first patch enables pylint to run on tests.
> Some of the exceptions I've added are pretty heavy-handed, but the
> main thing I wanted to enable is checking for duplicate test
> function/method names, since in that case the earlier test is silently
> dropped.
>
> I dropped support for old pylint while I was at it.
>
>
> The other three patches show how one might go around modernizing one
> of our Declarative test suites. There are some dead ends in the first
> patch that are removed subsequently; if you squash the patches
> together they might be easier to review.
> See the commit messages for what the patches do.
> The HostTracker should generalize well to other objects. With Service
> or DNS trackers some of the host tests can be simplified even more.
>
>
> To check that tests are independent, you can use the order-randomizing
> plugin for pytest (which is not in Fedora yet):
>
> pip install --user pytest-random
> ./make-test ipatests/test_xmlrpc/test_host_plugin.py -v --random
>
> (Note that this takes a lot of time as fixtures are created and
> re-created – independent tests are mainly useful for selecting
> *subsets* of tests.)
>

ACK, nice work!

Pushed to master: 07545569ecbdfdb1aeef6aa1878d827a7e5ddb38

Note: There was a build issue a fixed with attached patch, also pushed.

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

>From c50a56fef015a3aac06aa36eb8da8b39b6248261 Mon Sep 17 00:00:00 2001
From: Tomas Babej 
Date: Tue, 13 Jan 2015 22:11:19 +0100
Subject: [PATCH] spec: Add BuildRequires for python-pytest plugins

---
 freeipa.spec.in | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index 370d77440fe7c00fe85588a33d0a9356e0a6cda3..ef11c07b3956b20a1cd396636d2d26c7f9d679d1 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -92,6 +92,8 @@ BuildRequires:  softhsm-devel >= 2.0.0b1-3
 BuildRequires:  openssl-devel
 BuildRequires:  p11-kit-devel
 BuildRequires:  pki-base >= 10.2.1-0.1
+BuildRequires:  python-pytest-multihost >= 0.5
+BuildRequires:  python-pytest-sourceorder
 
 %description
 IPA is an integrated solution to provide centrally managed Identity (machine,
@@ -308,7 +310,7 @@ Requires: pytest >= 2.6
 Requires: python-paste
 Requires: python-coverage
 Requires: python-polib
-Requires: python-pytest-multihost >= 0.4
+Requires: python-pytest-multihost >= 0.5
 Requires: python-pytest-sourceorder
 
 Conflicts: %{alt_name}-tests
-- 
2.1.0

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

Re: [Freeipa-devel] [PATCH 0296] ipatests: Make descriptions of declarative tests sorted according to their order

[Tomas Babej]

On 01/14/2015 11:53 AM, Petr Viktorin wrote:
> On 12/19/2014 03:05 PM, Tomas Babej wrote:
>> Hi,
>>
>> this allows us to sort the descriptions and preserve the test order.
>>
>
> ACK
>
Pushed to master: 5416652f6f15de4cc2140a58466facdf07c70965

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

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

Re: [Freeipa-devel] [PATCH] 387 Fix validation of ipa-restore options

[Martin Kosek]

On 01/14/2015 12:35 PM, Petr Viktorin wrote:
> On 01/14/2015 09:14 AM, Martin Kosek wrote:
>> On 01/13/2015 06:02 PM, Jan Cholasta wrote:
> 
>>> Rebased again, patch attached.
>>
>> Given that Petr is not there today, I finished the review for him. I did not
>> find any other issues, all issues except (2) are fixed.
>>
>> ACK. Pushed to master (rebased) and ipa-4-1.
> 
> This broke master.
> 
> * Module ipaserver.install.ipa_restore
> ipaserver/install/ipa_restore.py:175: [E0602(undefined-variable),
> Restore.validate_options] Undefined variable 'BACKUP_DIR')
> ipaserver/install/ipa_restore.py:210: [E0602(undefined-variable), Restore.run]
> Undefined variable 'BACKUP_DIR')
> 

Grr... Sorry for the hickup, I fixed it with the attached patch and pushed it
to master as a one liner.
From 35c4fa2e36a83ad80c79a21ac8e495985f38dbde Mon Sep 17 00:00:00 2001
From: Martin Kosek 
Date: Wed, 14 Jan 2015 13:05:09 +0100
Subject: [PATCH] Fix IPA_BACKUP_DIR path name

Path name was not updated during patch rebase.

https://fedorahosted.org/freeipa/ticket/4797
---
 ipaserver/install/ipa_restore.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ipaserver/install/ipa_restore.py b/ipaserver/install/ipa_restore.py
index 9d308eff657e7b6223b9597d741af091268801ad..d93ddc79ecacdf660359d0db7831285ba7e0e3cb 100644
--- a/ipaserver/install/ipa_restore.py
+++ b/ipaserver/install/ipa_restore.py
@@ -172,7 +172,7 @@ def validate_options(self):
 
 dirname = self.args[0]
 if not os.path.isabs(dirname):
-dirname = os.path.join(BACKUP_DIR, dirname)
+dirname = os.path.join(paths.IPA_BACKUP_DIR, dirname)
 if not os.path.isdir(dirname):
 parser.error("must provide path to backup directory")
 
@@ -207,7 +207,7 @@ def run(self):
 
 self.backup_dir = self.args[0]
 if not os.path.isabs(self.backup_dir):
-self.backup_dir = os.path.join(BACKUP_DIR, self.backup_dir)
+self.backup_dir = os.path.join(paths.IPA_BACKUP_DIR, self.backup_dir)
 
 self.log.info("Preparing restore from %s on %s",
   self.backup_dir, api.env.host)
-- 
1.9.3

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

Re: [Freeipa-devel] [PATCH] 387 Fix validation of ipa-restore options

[Petr Viktorin]

On 01/14/2015 09:14 AM, Martin Kosek wrote:

On 01/13/2015 06:02 PM, Jan Cholasta wrote:



Rebased again, patch attached.


Given that Petr is not there today, I finished the review for him. I did not
find any other issues, all issues except (2) are fixed.

ACK. Pushed to master (rebased) and ipa-4-1.


This broke master.

* Module ipaserver.install.ipa_restore
ipaserver/install/ipa_restore.py:175: [E0602(undefined-variable), 
Restore.validate_options] Undefined variable 'BACKUP_DIR')
ipaserver/install/ipa_restore.py:210: [E0602(undefined-variable), 
Restore.run] Undefined variable 'BACKUP_DIR')


--
Petr³

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

Re: [Freeipa-devel] [PATCH] 488-489 PermissionsV2 related winsync fixes

[Martin Kosek]

On 01/14/2015 10:58 AM, thierry bordaz wrote:
> On 01/14/2015 10:15 AM, Petr Viktorin wrote:
>> On 01/13/2015 10:52 PM, Martin Kosek wrote:
>>> On 01/13/2015 09:55 PM, Simo Sorce wrote:
 On Tue, 13 Jan 2015 18:16:11 +0100
 Martin Kosek  wrote:

> This is crude first version of the (working) fixes to fix
> Winsync/Passsync problems caused by the PermissionV2 refactoring.
>
> Simo/Petr3 or others, any concerns?
>

 The first patch looks good
 the second looks .. broad ?

 Shouldn't you explicitly allow specific attributes ?
>>>
>>> You mean for:
>>>
>>> +'System: Read LDBM database config': {
>>> +'ipapermlocation': DN('cn=config'),
>>> +'ipapermtarget': DN('cn=config,cn=ldbm
>>> database,cn=plugins,cn=config'),
>>> +'ipapermbindruletype': 'permission',
>>> +'ipapermright': {'read', 'search', 'compare'},
>>> +'default_privileges': {'Replication Administrators'},
>>> +'ipapermdefaultattr': {'*'},
>>> +},
>>>
>>> ? I did that as my first try, but then the ACI was not accepted as the
>>> attribute I was looking for (nsslapd-changelogdir) is not in the schema
>>> as the config is just an extensibleObject. But as I was going through
>>> the attributes, I did not see anything super-secret.
>>>
>>> Petr, is there any way to make permission plugin accept unknown
>>> attribute in the permission attribute list, or do we need to use "*" in
>>> this case?
>>
>> The ACL Syntax Error comes straight from the DS, so there's not much IPA can
>> do. The error suggests adding nsslapd-changelogdir to the schema, but I'm not
>> sure that's the right solution here.
>> Thierry, any comments? See the attached LDIF.
>>
> Actually this limitation was added with the bug
> https://bugzilla.redhat.com/show_bug.cgi?id=244229.
> I do not see in the bug, if the ability to define non schema attribute was
> creating a problem for IPA

Not before, but with PermissionV2 and especially these patches, we may need to
control access to unknown attributes in extensibleObject objects.

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

Re: [Freeipa-devel] [PATCH 0296] ipatests: Make descriptions of declarative tests sorted according to their order

[Petr Viktorin]

On 12/19/2014 03:05 PM, Tomas Babej wrote:

Hi,

this allows us to sort the descriptions and preserve the test order.



ACK

--
Petr³

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

Re: [Freeipa-devel] [PATCH] 488-489 PermissionsV2 related winsync fixes

[Martin Kosek]

On 01/14/2015 10:37 AM, thierry bordaz wrote:
> On 01/14/2015 10:15 AM, Petr Viktorin wrote:
>> On 01/13/2015 10:52 PM, Martin Kosek wrote:
>>> On 01/13/2015 09:55 PM, Simo Sorce wrote:
 On Tue, 13 Jan 2015 18:16:11 +0100
 Martin Kosek  wrote:

> This is crude first version of the (working) fixes to fix
> Winsync/Passsync problems caused by the PermissionV2 refactoring.
>
> Simo/Petr3 or others, any concerns?
>

 The first patch looks good
 the second looks .. broad ?

 Shouldn't you explicitly allow specific attributes ?
>>>
>>> You mean for:
>>>
>>> +'System: Read LDBM database config': {
>>> +'ipapermlocation': DN('cn=config'),
>>> +'ipapermtarget': DN('cn=config,cn=ldbm
>>> database,cn=plugins,cn=config'),
>>> +'ipapermbindruletype': 'permission',
>>> +'ipapermright': {'read', 'search', 'compare'},
>>> +'default_privileges': {'Replication Administrators'},
>>> +'ipapermdefaultattr': {'*'},
>>> +},
>>>
>>> ? I did that as my first try, but then the ACI was not accepted as the
>>> attribute I was looking for (nsslapd-changelogdir) is not in the schema
>>> as the config is just an extensibleObject. But as I was going through
>>> the attributes, I did not see anything super-secret.
>>>
>>> Petr, is there any way to make permission plugin accept unknown
>>> attribute in the permission attribute list, or do we need to use "*" in
>>> this case?
>>
>> The ACL Syntax Error comes straight from the DS, so there's not much IPA can
>> do. The error suggests adding nsslapd-changelogdir to the schema, but I'm not
>> sure that's the right solution here.
>> Thierry, any comments? See the attached LDIF.
>>
> 
> 
> Yes DS acl plugin checks that the named attribute is in the schema. I do not
> see the benefit of this limitation I need to dig further.
> Now you may define the 'targetattr' with a '*'. Would it be possible to use an
> aci syntax like :
> 
> aci: (targetattr = "nsslapd-changelogdir*")(version 3.0;acl "test-aci";allow
> (compare,read,search) groupdn = "ldap:///all";;)
> 
> thanks
> theirry

Maybe, although it looks bit ugly. So far, I just used "*" given the ACIs were
quite focused and only for "Replication Administrators" privilege members.

Martin

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

Re: [Freeipa-devel] [PATCH] 488-489 PermissionsV2 related winsync fixes

[thierry bordaz]

On 01/14/2015 10:15 AM, Petr Viktorin wrote:

On 01/13/2015 10:52 PM, Martin Kosek wrote:

On 01/13/2015 09:55 PM, Simo Sorce wrote:

On Tue, 13 Jan 2015 18:16:11 +0100
Martin Kosek  wrote:


This is crude first version of the (working) fixes to fix
Winsync/Passsync problems caused by the PermissionV2 refactoring.

Simo/Petr3 or others, any concerns?



The first patch looks good
the second looks .. broad ?

Shouldn't you explicitly allow specific attributes ?


You mean for:

+'System: Read LDBM database config': {
+'ipapermlocation': DN('cn=config'),
+'ipapermtarget': DN('cn=config,cn=ldbm
database,cn=plugins,cn=config'),
+'ipapermbindruletype': 'permission',
+'ipapermright': {'read', 'search', 'compare'},
+'default_privileges': {'Replication Administrators'},
+'ipapermdefaultattr': {'*'},
+},

? I did that as my first try, but then the ACI was not accepted as the
attribute I was looking for (nsslapd-changelogdir) is not in the schema
as the config is just an extensibleObject. But as I was going through
the attributes, I did not see anything super-secret.

Petr, is there any way to make permission plugin accept unknown
attribute in the permission attribute list, or do we need to use "*" in
this case?


The ACL Syntax Error comes straight from the DS, so there's not much 
IPA can do. The error suggests adding nsslapd-changelogdir to the 
schema, but I'm not sure that's the right solution here.

Thierry, any comments? See the attached LDIF.

Actually this limitation was added with the bug 
https://bugzilla.redhat.com/show_bug.cgi?id=244229.
I do not see in the bug, if the ability to define non schema attribute 
was creating a problem for IPA


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

Re: [Freeipa-devel] [PATCH] 488-489 PermissionsV2 related winsync fixes

[thierry bordaz]

On 01/14/2015 10:15 AM, Petr Viktorin wrote:

On 01/13/2015 10:52 PM, Martin Kosek wrote:

On 01/13/2015 09:55 PM, Simo Sorce wrote:

On Tue, 13 Jan 2015 18:16:11 +0100
Martin Kosek  wrote:


This is crude first version of the (working) fixes to fix
Winsync/Passsync problems caused by the PermissionV2 refactoring.

Simo/Petr3 or others, any concerns?



The first patch looks good
the second looks .. broad ?

Shouldn't you explicitly allow specific attributes ?


You mean for:

+'System: Read LDBM database config': {
+'ipapermlocation': DN('cn=config'),
+'ipapermtarget': DN('cn=config,cn=ldbm
database,cn=plugins,cn=config'),
+'ipapermbindruletype': 'permission',
+'ipapermright': {'read', 'search', 'compare'},
+'default_privileges': {'Replication Administrators'},
+'ipapermdefaultattr': {'*'},
+},

? I did that as my first try, but then the ACI was not accepted as the
attribute I was looking for (nsslapd-changelogdir) is not in the schema
as the config is just an extensibleObject. But as I was going through
the attributes, I did not see anything super-secret.

Petr, is there any way to make permission plugin accept unknown
attribute in the permission attribute list, or do we need to use "*" in
this case?


The ACL Syntax Error comes straight from the DS, so there's not much 
IPA can do. The error suggests adding nsslapd-changelogdir to the 
schema, but I'm not sure that's the right solution here.

Thierry, any comments? See the attached LDIF.




Yes DS acl plugin checks that the named attribute is in the schema. I do 
not see the benefit of this limitation I need to dig further.
Now you may define the 'targetattr' with a '*'. Would it be possible to 
use an aci syntax like :


aci: (targetattr = "nsslapd-changelogdir*")(version 3.0;acl "test-aci";allow 
(compare,read,search) groupdn = "ldap:///all";;)

thanks
theirry


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

Re: [Freeipa-devel] [PATCH] 488-489 PermissionsV2 related winsync fixes

[Petr Viktorin]

On 01/13/2015 10:52 PM, Martin Kosek wrote:

On 01/13/2015 09:55 PM, Simo Sorce wrote:

On Tue, 13 Jan 2015 18:16:11 +0100
Martin Kosek  wrote:


This is crude first version of the (working) fixes to fix
Winsync/Passsync problems caused by the PermissionV2 refactoring.

Simo/Petr3 or others, any concerns?



The first patch looks good
the second looks .. broad ?

Shouldn't you explicitly allow specific attributes ?


You mean for:

+'System: Read LDBM database config': {
+'ipapermlocation': DN('cn=config'),
+'ipapermtarget': DN('cn=config,cn=ldbm
database,cn=plugins,cn=config'),
+'ipapermbindruletype': 'permission',
+'ipapermright': {'read', 'search', 'compare'},
+'default_privileges': {'Replication Administrators'},
+'ipapermdefaultattr': {'*'},
+},

? I did that as my first try, but then the ACI was not accepted as the
attribute I was looking for (nsslapd-changelogdir) is not in the schema
as the config is just an extensibleObject. But as I was going through
the attributes, I did not see anything super-secret.

Petr, is there any way to make permission plugin accept unknown
attribute in the permission attribute list, or do we need to use "*" in
this case?


The ACL Syntax Error comes straight from the DS, so there's not much IPA 
can do. The error suggests adding nsslapd-changelogdir to the schema, 
but I'm not sure that's the right solution here.

Thierry, any comments? See the attached LDIF.

--
Petr³

dn: cn=config
changetype: modify
add: aci
aci: (targetattr = \22nsslapd-changelogdir\22)(version 3.0;acl \22test-aci\22;allow (compare,read,search) groupdn = \22ldap:///all\22;)
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH] 387 Fix validation of ipa-restore options

[Martin Kosek]

On 01/13/2015 06:02 PM, Jan Cholasta wrote:
> Dne 13.1.2015 v 17:45 Jan Cholasta napsal(a):
>> Dne 13.1.2015 v 16:37 Petr Vobornik napsal(a):
>>> On 01/13/2015 02:26 PM, Jan Cholasta wrote:
 Dne 13.1.2015 v 13:01 Petr Vobornik napsal(a):
> On 01/12/2015 02:28 PM, Jan Cholasta wrote:
>> Hi,
>>
>> the attached patch fixes
>> .
>>
>> Note that --data with data-only backup and --logs-only with data-only
>> restore are deliberately ignored and considered no-op.
>>
>> Honza
>>
>>>
>
> 3. When #2 fixed, data backup with --no-logs doesn't raise warning as
> requested in ticket.

 IMO such a warning does not make sense. You request no logs, you get no
 logs, I don't see anything worth warning about here.
>>>
>>> ok, makes sense
>>>

>
> 5. Nitpick: imho option validation should be done before temp dir
> creation.

 Fixed.
>>>
>>> You've also moved
>>>self.header = os.path.join(self.backup_dir, 'header')
>>> below
>>>self.read_header()
>>>
>>> --> restore fails everytime
>>
>> Silly me. Sorry. Fixed.
>>
>> Rebased updated patch attached.
> 
> Rebased again, patch attached.

Given that Petr is not there today, I finished the review for him. I did not
find any other issues, all issues except (2) are fixed.

ACK. Pushed to master (rebased) and ipa-4-1.

Martin

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