[Freeipa-devel] [PATCH] 248 Raise proper exception when LDAP limits are exceeded
Few test hints are attached to the ticket. --- ldap2 plugin returns NotFound error for find_entries/get_entry queries when the server did not manage to return an entry due to time limits. This may be confusing for user when the entry he searches actually exists. This patch fixes the behavior in ldap2 plugin to return LimitsExceeded exception instead. This way, user would know that his time/size limits are set too low and can amend them to get correct results. https://fedorahosted.org/freeipa/ticket/2606 From ee78c421a942cdfb7baaa0e21b9c39a4e5cac272 Mon Sep 17 00:00:00 2001 From: Martin Kosek mko...@redhat.com Date: Tue, 10 Apr 2012 10:50:51 +0200 Subject: [PATCH] Raise proper exception when LDAP limits are exceeded ldap2 plugin returns NotFound error for find_entries/get_entry queries when the server did not manage to return an entry due to time limits. This may be confusing for user when the entry he searches actually exists. This patch fixes the behavior in ldap2 plugin to return LimitsExceeded exception instead. This way, user would know that his time/size limits are set too low and can amend them to get correct results. https://fedorahosted.org/freeipa/ticket/2606 --- ipaserver/plugins/ldap2.py |4 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/ipaserver/plugins/ldap2.py b/ipaserver/plugins/ldap2.py index 61341b082b834fb3cc2ab5e3452a563be37cebef..a96abe0c9bfbc1a098a12a65ac500764b9031c23 100644 --- a/ipaserver/plugins/ldap2.py +++ b/ipaserver/plugins/ldap2.py @@ -750,6 +750,10 @@ class ldap2(CrudBackend, Encoder): res.append(res_list[0]) except (_ldap.ADMINLIMIT_EXCEEDED, _ldap.TIMELIMIT_EXCEEDED, _ldap.SIZELIMIT_EXCEEDED), e: +if not res: +# server returned no result +# return rather LimitsExceeded error than NotFound error +raise errors.LimitsExceeded() truncated = True except _ldap.LDAPError, e: _handle_errors(e) -- 1.7.7.6 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0034 Limit permission and selfservice names
On 04/09/2012 03:55 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
https://fedorahosted.org/freeipa/ticket/2585: ipa permission-add throws
internal server error when name contains '', '' or other special
characters.
The problem is, of course, proper escaping; not only in DNs but also in
ACIs. Right now we don't really do either.
This patch is just a simple workaround: disallow anything except
known-good characters. It's just names, so no functionality is lost.
All tickets for April are now taken, so unless a new one comes my way,
I'll take a dive into the code and fix it properly. This could take some
time and would mean somewhat larger changes.
Is there a reason you didn't use pattern/pattern_errmsg instead?
You'd need to change the regex as patterns use re.match rather than
re.search.
rob
Right, that makes more sense.
It changes API.txt though. Do I need to bump VERSION in this case?
Also, is there a reason pattern_errmsg is included in API.txt?
--
Petr³
From ece4a28d2b6dc3c35e7fbc6fc3ef80a063312846 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Fri, 6 Apr 2012 04:56:46 -0400
Subject: [PATCH] Limit permission and selfservice names to alphanumerics, -,
_, space
The DN and ACI code doesn't always escape special characters properly.
Rather than trying to fix it, this patch takes the easy way out and
enforces that the names are safe.
https://fedorahosted.org/freeipa/ticket/2585
---
API.txt | 26 +-
ipalib/plugins/permission.py |4
ipalib/plugins/selfservice.py|4
tests/test_xmlrpc/test_permission_plugin.py | 11 +++
tests/test_xmlrpc/test_selfservice_plugin.py | 13 +
5 files changed, 45 insertions(+), 13 deletions(-)
diff --git a/API.txt b/API.txt
index c1b2ba05c40576abf6328f834eadb11e2d030b49..a49376817daf06b504e8148783e50091a62b6363 100644
--- a/API.txt
+++ b/API.txt
@@ -2039,7 +2039,7 @@ output: Output('result', type 'bool', None)
output: Output('value', type 'unicode', None)
command: permission_add
args: 1,12,3
-arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, required=True)
+arg: Str('cn', attribute=True, cli_name='name', multivalue=False, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, required=True)
option: Str('permissions', attribute=True, cli_name='permissions', csv=True, multivalue=True, required=True)
option: Str('attrs', alwaysask=True, attribute=True, autofill=False, cli_name='attrs', csv=True, multivalue=True, query=False, required=False)
option: StrEnum('type', alwaysask=True, attribute=True, autofill=False, cli_name='type', multivalue=False, query=False, required=False, values=(u'user', u'group', u'host', u'service', u'hostgroup', u'netgroup', u'dnsrecord'))
@@ -2057,25 +2057,25 @@ output: Entry('result', type 'dict', Gettext('A dictionary representing an LDA
output: Output('value', type 'unicode', None)
command: permission_add_member
args: 1,4,3
-arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, query=True, required=True)
+arg: Str('cn', attribute=True, cli_name='name', multivalue=False, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, query=True, required=True)
option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
option: Str('version?', exclude='webui')
option: Str('privilege*', alwaysask=True, cli_name='privileges', csv=True)
output: Entry('result', type 'dict', Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
output: Output('failed', type 'dict', None)
output: Output('completed', type 'int', None)
command: permission_del
args: 1,1,3
-arg: Str('cn', attribute=True, cli_name='name', multivalue=True, primary_key=True, query=True, required=True)
+arg: Str('cn', attribute=True, cli_name='name', multivalue=True, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, query=True, required=True)
option: Flag('continue', autofill=True, cli_name='continue', default=False)
output: Output('summary', (type 'unicode', type 'NoneType'), None)
output: Output('result', type 'dict', None)
output: Output('value', type 'unicode', None)
command: permission_find
args: 1,14,4
arg: Str('criteria?', noextrawhitespace=False)
-option: Str('cn', attribute=True, autofill=False, cli_name='name', multivalue=False, primary_key=True, query=True, required=False)
+option: Str('cn', attribute=True, autofill=False, cli_name='name', multivalue=False, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, query=True, required=False)
option: Str('permissions',
Re: [Freeipa-devel] [PATCH] 115 Reworked netgroup Web UI to allow setting user/host category
On 04/05/2012 04:54 PM, Endi Sukma Dewata wrote: On 3/29/2012 7:46 AM, Petr Vobornik wrote: This patch is changing netgroup web ui to look more like hbac or sudo rule UI. This change allows to define and display user category, host category and external host. The core of the change is changing member attributes (user, group, host, hostgroup) to use rule_details_widget instead of separate association facets. In host case it also allows to display and add external hosts. https://fedorahosted.org/freeipa/ticket/2578 Note: compare to other plugins (HBAC, Sudo) netgroup plugins doesn't have member attrs in takes_param therefore labels for columns have to be explicitly set. ACK. Pushed to master, ipa-2-2. Just one thing, the label for user/host category says User/host category the rule applies to. Netgroup is not a rule, so it might be better to say something like User/host category of netgroup members or Member user/host category. This is an existing server issue. -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 116 Fixed: permission attrs table didn't update its available options on load
On 04/05/2012 04:55 PM, Endi Sukma Dewata wrote: On 4/4/2012 2:18 AM, Petr Vobornik wrote: It could lead to state where attributes from other object type were displayed instead of the correct ones. https://fedorahosted.org/freeipa/ticket/2590 ACK. Pushed to master, ipa-2-2. -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 118-119 DNS forward policy: checkboxes changed to radio buttons
On 4/5/2012 10:58 AM, Petr Vobornik wrote: Revised patch 118 attached. I used: * Forward first * Forward only and set 'default_value' to 'first'. So there would be always some value checked, which indicates what is actually used. There is a little issue with undo button if policy is not set '' because default_value !== ''. I did this kinda in the hurry, so I hope I didn't missed anything crucial. Will be back on Tuesday. ACK on both patches. -- Endi S. Dewata ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0034 Limit permission and selfservice names
Petr Viktorin wrote: On 04/09/2012 03:55 PM, Rob Crittenden wrote: Petr Viktorin wrote: https://fedorahosted.org/freeipa/ticket/2585: ipa permission-add throws internal server error when name contains '', '' or other special characters. The problem is, of course, proper escaping; not only in DNs but also in ACIs. Right now we don't really do either. This patch is just a simple workaround: disallow anything except known-good characters. It's just names, so no functionality is lost. All tickets for April are now taken, so unless a new one comes my way, I'll take a dive into the code and fix it properly. This could take some time and would mean somewhat larger changes. Is there a reason you didn't use pattern/pattern_errmsg instead? You'd need to change the regex as patterns use re.match rather than re.search. rob Right, that makes more sense. It changes API.txt though. Do I need to bump VERSION in this case? Also, is there a reason pattern_errmsg is included in API.txt? Yes, please bump VERSION. pattern_errmsg should probably be removed from API.txt. We've been paring back the amount of data to validate slowly as we've run into these questionable items. Please open a ticket for this. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 118-119 DNS forward policy: checkboxes changed to radio buttons
On 04/10/2012 03:39 PM, Endi Sukma Dewata wrote: On 4/5/2012 10:58 AM, Petr Vobornik wrote: Revised patch 118 attached. I used: * Forward first * Forward only and set 'default_value' to 'first'. So there would be always some value checked, which indicates what is actually used. There is a little issue with undo button if policy is not set '' because default_value !== ''. I did this kinda in the hurry, so I hope I didn't missed anything crucial. Will be back on Tuesday. ACK on both patches. Pushed to master, ipa-2-2. -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [RANT] --setattr validation is a minefield.
I'm aware that we have backwards compatibility requirements so we have
to stick with unfortunate decisions, but I wanted you to know what I
think. Please tell me I'm wrong!
It is not clear what --{set,add,del}attr and friends should do. On the
one hand they should be powerful -- presumably as powerful as
ldapmodify. On the other hand they should be checked to ensure they
can't be used to break the system. These requirements are contradictory.
And in either case we're doing it wrong:
- If they should be all-powerful, we shouldn't validate them.
- If they shouldn't break the system we can just disable them for
IPA-managed attributes. My understanding is that they were originally
only added for attributes IPA doesn't know about. People can still use
ldapmodify to bypass validation if they want.
- If somewhere in between, we need to clearly define what they should
do, instead of drawing the line ad-hoc based on individual details we
forgot about, as tickets come from QE.
I would hope people won't use --setattr for IPA-managed attributes.
Which would however mean we won't get much community testing for all
this extra code.
Then, there's an unfortunate detail in IPA implementation: attribute
Params need to be cloned to method objects (Create, Update, etc.) to
work properly (e.g. get the `attribute` flag set). If they are marked
no_update, they don't get cloned, so they don't work properly.
Yet --setattr apparently needs to be able to update and validate
attributes marked no_update (this ties to the confusing requirements on
--setattr I already mentioned). This leads to doing the same work again,
slightly differently.
tl;dr: --setattr work on IPA-managed attributes (with validation) is a
mistake. It adds no functionality, only complexity. We don't want people
to use it. It will cost us a lot of maintenance work to support.
Thank you for listening. A patch for the newest regression is coming up.
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0034 Limit permission and selfservice names
On 04/10/2012 03:46 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 04/09/2012 03:55 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
https://fedorahosted.org/freeipa/ticket/2585: ipa permission-add throws
internal server error when name contains '', '' or other special
characters.
The problem is, of course, proper escaping; not only in DNs but also in
ACIs. Right now we don't really do either.
This patch is just a simple workaround: disallow anything except
known-good characters. It's just names, so no functionality is lost.
All tickets for April are now taken, so unless a new one comes my way,
I'll take a dive into the code and fix it properly. This could take
some
time and would mean somewhat larger changes.
Is there a reason you didn't use pattern/pattern_errmsg instead?
You'd need to change the regex as patterns use re.match rather than
re.search.
rob
Right, that makes more sense.
It changes API.txt though. Do I need to bump VERSION in this case?
Also, is there a reason pattern_errmsg is included in API.txt?
Yes, please bump VERSION.
Attaching updated patch.
pattern_errmsg should probably be removed from API.txt. We've been
paring back the amount of data to validate slowly as we've run into
these questionable items. Please open a ticket for this.
Done: https://fedorahosted.org/freeipa/ticket/2619
--
Petr³
From 0454790c2c037f70ad268e6bdd4a25ca8927ce6a Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Fri, 6 Apr 2012 04:56:46 -0400
Subject: [PATCH] Limit permission and selfservice names to alphanumerics, -,
_, space
The DN and ACI code doesn't always escape special characters properly.
Rather than trying to fix it, this patch takes the easy way out and
enforces that the names are safe.
https://fedorahosted.org/freeipa/ticket/2585
---
API.txt | 26 +-
VERSION |4 ++--
ipalib/plugins/permission.py |4
ipalib/plugins/selfservice.py|4
tests/test_xmlrpc/test_permission_plugin.py | 11 +++
tests/test_xmlrpc/test_selfservice_plugin.py | 13 +
6 files changed, 47 insertions(+), 15 deletions(-)
diff --git a/API.txt b/API.txt
index c1b2ba05c40576abf6328f834eadb11e2d030b49..a49376817daf06b504e8148783e50091a62b6363 100644
--- a/API.txt
+++ b/API.txt
@@ -2039,7 +2039,7 @@ output: Output('result', type 'bool', None)
output: Output('value', type 'unicode', None)
command: permission_add
args: 1,12,3
-arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, required=True)
+arg: Str('cn', attribute=True, cli_name='name', multivalue=False, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, required=True)
option: Str('permissions', attribute=True, cli_name='permissions', csv=True, multivalue=True, required=True)
option: Str('attrs', alwaysask=True, attribute=True, autofill=False, cli_name='attrs', csv=True, multivalue=True, query=False, required=False)
option: StrEnum('type', alwaysask=True, attribute=True, autofill=False, cli_name='type', multivalue=False, query=False, required=False, values=(u'user', u'group', u'host', u'service', u'hostgroup', u'netgroup', u'dnsrecord'))
@@ -2057,25 +2057,25 @@ output: Entry('result', type 'dict', Gettext('A dictionary representing an LDA
output: Output('value', type 'unicode', None)
command: permission_add_member
args: 1,4,3
-arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, query=True, required=True)
+arg: Str('cn', attribute=True, cli_name='name', multivalue=False, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, query=True, required=True)
option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
option: Str('version?', exclude='webui')
option: Str('privilege*', alwaysask=True, cli_name='privileges', csv=True)
output: Entry('result', type 'dict', Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
output: Output('failed', type 'dict', None)
output: Output('completed', type 'int', None)
command: permission_del
args: 1,1,3
-arg: Str('cn', attribute=True, cli_name='name', multivalue=True, primary_key=True, query=True, required=True)
+arg: Str('cn', attribute=True, cli_name='name', multivalue=True, pattern='^[-_ a-zA-Z0-9]+$', pattern_errmsg='May only contain letters, numbers, -, _, and space', primary_key=True, query=True, required=True)
option: Flag('continue', autofill=True, cli_name='continue', default=False)
output: Output('summary', (type 'unicode', type 'NoneType'), None)
output: Output('result', type 'dict', None)
output: Output('value', type 'unicode', None)
command: permission_find
args: 1,14,4
arg:
Re: [Freeipa-devel] [RANT] --setattr validation is a minefield.
On 10.4.2012 16:00, Petr Viktorin wrote:
I'm aware that we have backwards compatibility requirements so we have
to stick with unfortunate decisions, but I wanted you to know what I
think. Please tell me I'm wrong!
It is not clear what --{set,add,del}attr and friends should do. On the
one hand they should be powerful -- presumably as powerful as
ldapmodify. On the other hand they should be checked to ensure they
can't be used to break the system. These requirements are contradictory.
And in either case we're doing it wrong:
- If they should be all-powerful, we shouldn't validate them.
- If they shouldn't break the system we can just disable them for
IPA-managed attributes. My understanding is that they were originally
only added for attributes IPA doesn't know about. People can still use
ldapmodify to bypass validation if they want.
- If somewhere in between, we need to clearly define what they should
do, instead of drawing the line ad-hoc based on individual details we
forgot about, as tickets come from QE.
I would hope people won't use --setattr for IPA-managed attributes.
Which would however mean we won't get much community testing for all
this extra code.
Then, there's an unfortunate detail in IPA implementation: attribute
Params need to be cloned to method objects (Create, Update, etc.) to
work properly (e.g. get the `attribute` flag set). If they are marked
no_update, they don't get cloned, so they don't work properly.
Yet --setattr apparently needs to be able to update and validate
attributes marked no_update (this ties to the confusing requirements on
--setattr I already mentioned). This leads to doing the same work again,
slightly differently.
tl;dr: --setattr work on IPA-managed attributes (with validation) is a
mistake. It adds no functionality, only complexity. We don't want people
to use it. It will cost us a lot of maintenance work to support.
Thank you for listening. A patch for the newest regression is coming up.
I wholeheartedly agree.
Like you said above, we should either not validate --{set,add,del}attr
or don't allow them on known attributes.
To be functionally complete, we should also add validated equivalents of
--{add,del}attr to *-mod commands for all multivalue params (think
--add-param and --del-param for each --param).
Honza
--
Jan Cholasta
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 72] Validate DN RDN parameters for migrate command
On 04/06/2012 10:11 AM, John Dennis wrote:
On 04/06/2012 04:40 AM, Martin Kosek wrote:
1) We still crash when the parameter is empty. We may want to make it
required (the same fix Rob did for cert rejection reason):
# echo secret123 | ipa migrate-ds ldap://vm-054.idm.lab.bos.redhat.com
--with-compat --base-dn=dc=greyoak,dc=com --user-container=
ipa: ERROR: cannot connect to
u'http://vm-022.idm.lab.bos.redhat.com/ipa/xml': Internal Server Error
Good point, will fix.
2) Do you think it would make sense to create a special Param for DN?
Its quite general type and I bet there are other Params that could use
DN instead of Str. It could look like that:
DN('usercontainer?',
rdn=True, can be RDN, not DN
Yes, I considered introducing a new DN parameter type as well. I think
this is a good approach and will have payoff down the road. I will make
that change. However I'm inclined to introduce both a DN parameter and a
RDN parameter, they really are different entities, if you use a flag to
indicate you require a RDN then you lose the type of the parameter, it
could be either, and it really should be one or the other and knowing
which it is has value.
3) We should not restrict users from passing a user/group container with
more than one RDN:
Yeah, I wasn't too sure about that. The parameter distinctly called for
an RDN, but it seemed to me it should support any container below the
base, which would make it a DN not a RDN.
FWIW, a DN is an ordered sequence of 1 or more RDN's. DN's do not have
to be absolute, they can be any subset of a path sequence. A DN
with exactly one RDN is equivalent to an RDN, a special case).
I will change the parameter to be a DN since that is what was the
original intent.
All good suggestions, a revised patch will follow shortly.
Hmm... I'm rethinking this. The suggestion of adding a new DN parameter
is the right thing to do and I tried to implement it, but as I was
working I realized I needed to touch a fair amount of code to support
the new parameter type and to modify multiple places in the migration
code to work with the new type. That's more code changes at the very
last minute for this release than I'm comfortable with, we're too close
the freeze date for invasive modifications.
We have a work item open to introduce DN types throughout the code
including as a new Param type. I think it's best to make just a small
tweak to fix the traceback today and wait till the 3.0 work to do it
right. Therefore I'm no longer in favor of the new Param approach. I
will fix the problem you reported when the parameter is empty, but merge
that into the original patch.
--
John Dennis jden...@redhat.com
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 72] Validate DN RDN parameters for migrate command
On 10.4.2012 17:03, John Dennis wrote:
On 04/06/2012 10:11 AM, John Dennis wrote:
On 04/06/2012 04:40 AM, Martin Kosek wrote:
1) We still crash when the parameter is empty. We may want to make it
required (the same fix Rob did for cert rejection reason):
# echo secret123 | ipa migrate-ds ldap://vm-054.idm.lab.bos.redhat.com
--with-compat --base-dn=dc=greyoak,dc=com --user-container=
ipa: ERROR: cannot connect to
u'http://vm-022.idm.lab.bos.redhat.com/ipa/xml': Internal Server Error
Good point, will fix.
2) Do you think it would make sense to create a special Param for DN?
Its quite general type and I bet there are other Params that could use
DN instead of Str. It could look like that:
DN('usercontainer?',
rdn=True, can be RDN, not DN
Yes, I considered introducing a new DN parameter type as well. I think
this is a good approach and will have payoff down the road. I will make
that change. However I'm inclined to introduce both a DN parameter and a
RDN parameter, they really are different entities, if you use a flag to
indicate you require a RDN then you lose the type of the parameter, it
could be either, and it really should be one or the other and knowing
which it is has value.
3) We should not restrict users from passing a user/group container with
more than one RDN:
Yeah, I wasn't too sure about that. The parameter distinctly called for
an RDN, but it seemed to me it should support any container below the
base, which would make it a DN not a RDN.
FWIW, a DN is an ordered sequence of 1 or more RDN's. DN's do not have
to be absolute, they can be any subset of a path sequence. A DN
with exactly one RDN is equivalent to an RDN, a special case).
I will change the parameter to be a DN since that is what was the
original intent.
All good suggestions, a revised patch will follow shortly.
Hmm... I'm rethinking this. The suggestion of adding a new DN parameter
is the right thing to do and I tried to implement it, but as I was
working I realized I needed to touch a fair amount of code to support
the new parameter type and to modify multiple places in the migration
code to work with the new type. That's more code changes at the very
last minute for this release than I'm comfortable with, we're too close
the freeze date for invasive modifications.
We have a work item open to introduce DN types throughout the code
including as a new Param type. I think it's best to make just a small
tweak to fix the traceback today and wait till the 3.0 work to do it
right. Therefore I'm no longer in favor of the new Param approach. I
will fix the problem you reported when the parameter is empty, but merge
that into the original patch.
Just for the record, there are some related tickets:
https://fedorahosted.org/freeipa/ticket/2033,
https://fedorahosted.org/freeipa/ticket/2265.
Honza
--
Jan Cholasta
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [RANT] --setattr validation is a minefield.
On 04/10/2012 05:03 PM, Jan Cholasta wrote:
To be functionally complete, we should also add validated equivalents of
--{add,del}attr to *-mod commands for all multivalue params (think
--add-param and --del-param for each --param).
We need something like that anyway. Requiring users to learn raw LDAP
attribute names and value encodings, which they'd need for --setattr, is
suboptimal to say the least.
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 1005 fix password history
On Mon, 2012-04-09 at 23:54 -0400, Rob Crittenden wrote: Password history wasn't working because the qsort comparison function was comparing pointers, not data. This resulted in a random element being removed from the history on overflow rather than the oldest. We sort in reverse so we don't have to move elements inside the list when removing to make more room. We just pop off the top then shove on the new password. The history includes a time to make comparisons straightforward (and LDAP doesn't guarantee order). I've attached a test script to exercise things. I don't see a way to easily include this into our current framework at the moment. We'd need a way to switch users in the middle of a test. rob Thanks. The new line looks quite scary, but it is OK and works fine (explanation in man qsort). ACK. Pushed to master, ipa-2-2. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0035 Convert --setattr values for attributes marked no_update
Fix --setattr to work on no_update params. https://fedorahosted.org/freeipa/ticket/2616 -- Petr³ From b22c159e4f4c3d411850b30267fce61e56100acd Mon Sep 17 00:00:00 2001 From: Petr Viktorin pvikt...@redhat.com Date: Tue, 10 Apr 2012 07:44:21 -0400 Subject: [PATCH] Convert --setattr values for attributes marked no_update Attribute Patrams marked no_update never get cloned to Update commands, and thus never receive the `attribute` flag. This makes their `encode` method a no-op, which meant they don't get properly encoded when used with --setattr, making the --setattr fail. Introduce a `force` argument to encode, which overrides checking for the attribute flag. Use this in set/add/delattr normalization, where we know we are dealing with attributes. https://fedorahosted.org/freeipa/ticket/2616 --- ipalib/parameters.py |6 -- ipalib/plugins/baseldap.py|7 ++- tests/test_xmlrpc/test_attr.py|2 +- tests/test_xmlrpc/test_hbac_plugin.py | 21 + 4 files changed, 32 insertions(+), 4 deletions(-) diff --git a/ipalib/parameters.py b/ipalib/parameters.py index 60fb502365fecb02c30a1081255e8db22ec5481e..5c55d8bcca03607c77be588ce7edb0a27400538d 100644 --- a/ipalib/parameters.py +++ b/ipalib/parameters.py @@ -895,7 +895,7 @@ def _validate_scalar(self, value, index=None): rule=rule, ) -def encode(self, value): +def encode(self, value, force=False): Encode Python native type value to chosen backend format. Encoding is applied for parameters representing actual attributes (attribute=True). @@ -909,8 +909,10 @@ def encode(self, value): `Param._encode()`. :param value: Encoded value +:param force: If set to true, encoding takes place even for Params +not marked as attribute -if not self.attribute: #pylint: disable=E1101 +if not self.attribute and not force: #pylint: disable=E1101 return value if self.encoder is not None: #pylint: disable=E1101 return self.encoder(value) #pylint: disable=E1101 diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py index daf1b07fb60307cf52b593390be4523e70b51d2d..3e7923479519fac937f6e63dd3a0fa44a97d1ee4 100644 --- a/ipalib/plugins/baseldap.py +++ b/ipalib/plugins/baseldap.py @@ -937,7 +937,12 @@ def process_attr_options(self, entry_attrs, dn, keys, options): raise errors.ValidationError(name=attr, error=err.error) except errors.ConversionError, err: raise errors.ConversionError(name=attr, error=err.error) -value = param.encode(value) +# FIXME: We use `force` when encoding because we know this is +# an attribute, even if it does not have the `attribute` flag +# set. This happens with no_update attributes, which are +# not cloned to Update commands. This cloning is where the flag +# gets set. +value = param.encode(value, force=True) entry_attrs[attr] = value else: # unknown attribute: remove duplicite and invalid values diff --git a/tests/test_xmlrpc/test_attr.py b/tests/test_xmlrpc/test_attr.py index e6872a67a1fcfa3f782a2415bb79c416d2f75283..c19a6948c5e4976cbcb4c342e95cf5f7e48d201f 100644 --- a/tests/test_xmlrpc/test_attr.py +++ b/tests/test_xmlrpc/test_attr.py @@ -433,7 +433,7 @@ class test_attr(Declarative): command=( 'user_mod', [user1], dict( addattr=u'nsaccountlock=FaLsE', -delattr=u'nsaccountlock=True') +delattr=u'nsaccountlock=TRUE') ), expected=dict( result=dict( diff --git a/tests/test_xmlrpc/test_hbac_plugin.py b/tests/test_xmlrpc/test_hbac_plugin.py index 78c4973c95d0cc4b49eb7d6b50828931c31b2825..c7cb55bad4309f05fc0d9651f9e97d37ffe866ae 100644 --- a/tests/test_xmlrpc/test_hbac_plugin.py +++ b/tests/test_xmlrpc/test_hbac_plugin.py @@ -430,6 +430,27 @@ def test_e_hbacrule_enabled(self): # FIXME: Should this be 'enabled' or 'TRUE'? assert_attr_equal(entry, 'ipaenabledflag', 'TRUE') +def test_ea_hbacrule_disable_setattr(self): + +Test disabling HBAC rule using setattr + +command_result = api.Command['hbacrule_mod']( +self.rule_name, setattr=u'ipaenabledflag=false') +assert command_result['result']['ipaenabledflag'] == (u'FALSE',) +entry = api.Command['hbacrule_show'](self.rule_name)['result'] +assert_attr_equal(entry, 'ipaenabledflag', 'FALSE') + +def test_eb_hbacrule_enable_setattr(self): + +Test enabling HBAC rule using setattr + +command_result = api.Command['hbacrule_mod']( +self.rule_name, setattr=u'ipaenabledflag=1') +
Re: [Freeipa-devel] [RANT] --setattr validation is a minefield.
On 04/10/2012 05:31 PM, Petr Viktorin wrote:
On 04/10/2012 05:03 PM, Jan Cholasta wrote:
On 04/10/2012 05:31 PM, Petr Viktorin wrote:
tl;dr: --setattr work on IPA-managed attributes (with validation) is a
mistake.
+1
It adds no functionality, only complexity. We don't want people
to use it. It will cost us a lot of maintenance work to support.
I wholeheartedly agree.
I absolutely agree. Why we should write validation code twice?
But things are worse than only code duplicity:
Small differences between two versions of code lead to problems with
code maintenance and potentially add a lot of bugs.
Petr^2 Spacek
To be functionally complete, we should also add validated equivalents of
--{add,del}attr to *-mod commands for all multivalue params (think
--add-param and --del-param for each --param).
We need something like that anyway. Requiring users to learn raw LDAP
attribute names and value encodings, which they'd need for --setattr, is
suboptimal to say the least.
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 120 Removal of memberofindirect_permissons from privileges
Problem:
In the Privilege page, can list Permissions. This Shows Results for
Direct
Membership. But there is an option to list this for Indirect Membership
also.
There isn't a way to nest permissions, so this option is not needed.
Solution:
This patch removes the memberofindirect_persmission definition from
server plugin. It fixes the problem in Web UI.
https://fedorahosted.org/freeipa/ticket/2611
--
Petr Vobornik
From 8db7bc4e665c7bbd8baaba4e6d4f6925e1936139 Mon Sep 17 00:00:00 2001
From: Petr Vobornik pvobo...@redhat.com
Date: Tue, 10 Apr 2012 14:40:44 +0200
Subject: [PATCH] Removal of memberofindirect_permissons from privileges
Problem:
In the Privilege page, can list Permissions. This Shows Results for Direct
Membership. But there is an option to list this for Indirect Membership
also.
There isn't a way to nest permissions, so this option is not needed.
Solution:
This patch removes the memberofindirect_persmission definition from server plugin. It fixes the problem in Web UI.
https://fedorahosted.org/freeipa/ticket/2611
---
ipalib/plugins/privilege.py |5 +
1 files changed, 1 insertions(+), 4 deletions(-)
diff --git a/ipalib/plugins/privilege.py b/ipalib/plugins/privilege.py
index 53e1de223a2a8018b942255b9911488f762cb338..ba2428736c9cdd022680fc69e97d0b583a16a2d1 100644
--- a/ipalib/plugins/privilege.py
+++ b/ipalib/plugins/privilege.py
@@ -49,13 +49,10 @@ class privilege(LDAPObject):
object_name = _('privilege')
object_name_plural = _('privileges')
object_class = ['nestedgroup', 'groupofnames']
-default_attributes = ['cn', 'description', 'member', 'memberof',
-'memberindirect', 'memberofindirect',
-]
+default_attributes = ['cn', 'description', 'member', 'memberof']
attribute_members = {
'member': ['role'],
'memberof': ['permission'],
-'memberofindirect': ['permission'],
}
reverse_members = {
'member': ['permission'],
--
1.7.7.6
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 72] Validate DN RDN parameters for migrate command
On Tue, 2012-04-10 at 11:03 -0400, John Dennis wrote:
On 04/06/2012 10:11 AM, John Dennis wrote:
On 04/06/2012 04:40 AM, Martin Kosek wrote:
1) We still crash when the parameter is empty. We may want to make it
required (the same fix Rob did for cert rejection reason):
# echo secret123 | ipa migrate-ds ldap://vm-054.idm.lab.bos.redhat.com
--with-compat --base-dn=dc=greyoak,dc=com --user-container=
ipa: ERROR: cannot connect to
u'http://vm-022.idm.lab.bos.redhat.com/ipa/xml': Internal Server Error
Good point, will fix.
2) Do you think it would make sense to create a special Param for DN?
Its quite general type and I bet there are other Params that could use
DN instead of Str. It could look like that:
DN('usercontainer?',
rdn=True, can be RDN, not DN
Yes, I considered introducing a new DN parameter type as well. I think
this is a good approach and will have payoff down the road. I will make
that change. However I'm inclined to introduce both a DN parameter and a
RDN parameter, they really are different entities, if you use a flag to
indicate you require a RDN then you lose the type of the parameter, it
could be either, and it really should be one or the other and knowing
which it is has value.
3) We should not restrict users from passing a user/group container with
more than one RDN:
Yeah, I wasn't too sure about that. The parameter distinctly called for
an RDN, but it seemed to me it should support any container below the
base, which would make it a DN not a RDN.
FWIW, a DN is an ordered sequence of 1 or more RDN's. DN's do not have
to be absolute, they can be any subset of a path sequence. A DN
with exactly one RDN is equivalent to an RDN, a special case).
I will change the parameter to be a DN since that is what was the
original intent.
All good suggestions, a revised patch will follow shortly.
Hmm... I'm rethinking this. The suggestion of adding a new DN parameter
is the right thing to do and I tried to implement it, but as I was
working I realized I needed to touch a fair amount of code to support
the new parameter type and to modify multiple places in the migration
code to work with the new type. That's more code changes at the very
last minute for this release than I'm comfortable with, we're too close
the freeze date for invasive modifications.
Ok, I agree with this approach. Lets fix just points 1) and 3) in my
intial review.
Martin
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 21 Unable to rename permission object
https://fedorahosted.org/freeipa/ticket/2571 The update was failing because of the case insensitivity of permission object DN. -- Regards, Ondrej Hamada FreeIPA team jabber: oh...@jabbim.cz IRC: ohamada From 75772d91024d961fc4193654a8ca128664b2d4d5 Mon Sep 17 00:00:00 2001 From: Ondrej Hamada oham...@redhat.com Date: Tue, 10 Apr 2012 16:21:07 +0200 Subject: [PATCH] Unable to rename permission object The update was failing because of the case insensitivity of permission object DN. https://fedorahosted.org/freeipa/ticket/2571 --- ipalib/plugins/permission.py | 19 +++ 1 files changed, 11 insertions(+), 8 deletions(-) diff --git a/ipalib/plugins/permission.py b/ipalib/plugins/permission.py index ce2536d9921ede73d2c26468f5d99609552e1881..05bd9901da82eea393a67255ff3d091b6fb02fd0 100644 --- a/ipalib/plugins/permission.py +++ b/ipalib/plugins/permission.py @@ -331,14 +331,17 @@ class permission_mod(LDAPUpdate): # when renaming permission, check if the target permission does not # exists already. Then, make changes to underlying ACI if 'rename' in options: -try: -new_dn = dn.replace(keys[-1], options['rename'], 1) -(new_dn, attrs) = ldap.get_entry( -new_dn, attrs_list, normalize=self.obj.normalize_dn -) -raise errors.DuplicateEntry() -except errors.NotFound: -pass# permission may be renamed, continue +if options['rename']: +try: +new_dn = dn.replace(keys[-1].lower(), options['rename'], 1) +(new_dn, attrs) = ldap.get_entry( +new_dn, attrs_list, normalize=self.obj.normalize_dn +) +raise errors.DuplicateEntry() +except errors.NotFound: +pass# permission may be renamed, continue +else: +raise errors.ValidationError(name='rename',error='New name can not be empty') opts = copy.copy(options) for o in ['all', 'raw', 'rights', 'rename']: -- 1.7.6.5 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [RANT] --setattr validation is a minefield.
On Tue, 2012-04-10 at 17:03 +0200, Jan Cholasta wrote:
On 10.4.2012 16:00, Petr Viktorin wrote:
I'm aware that we have backwards compatibility requirements so we have
to stick with unfortunate decisions, but I wanted you to know what I
think. Please tell me I'm wrong!
It is not clear what --{set,add,del}attr and friends should do. On the
one hand they should be powerful -- presumably as powerful as
ldapmodify. On the other hand they should be checked to ensure they
can't be used to break the system. These requirements are contradictory.
And in either case we're doing it wrong:
- If they should be all-powerful, we shouldn't validate them.
- If they shouldn't break the system we can just disable them for
IPA-managed attributes. My understanding is that they were originally
only added for attributes IPA doesn't know about. People can still use
ldapmodify to bypass validation if they want.
- If somewhere in between, we need to clearly define what they should
do, instead of drawing the line ad-hoc based on individual details we
forgot about, as tickets come from QE.
I would hope people won't use --setattr for IPA-managed attributes.
Which would however mean we won't get much community testing for all
this extra code.
Then, there's an unfortunate detail in IPA implementation: attribute
Params need to be cloned to method objects (Create, Update, etc.) to
work properly (e.g. get the `attribute` flag set). If they are marked
no_update, they don't get cloned, so they don't work properly.
Yet --setattr apparently needs to be able to update and validate
attributes marked no_update (this ties to the confusing requirements on
--setattr I already mentioned). This leads to doing the same work again,
slightly differently.
tl;dr: --setattr work on IPA-managed attributes (with validation) is a
mistake. It adds no functionality, only complexity. We don't want people
to use it. It will cost us a lot of maintenance work to support.
Thank you for listening. A patch for the newest regression is coming up.
I wholeheartedly agree.
This is indeed a mine field and we need to make a look from at the issue
from all sides before accepting a decision.
Like you said above, we should either not validate --{set,add,del}attr
or don't allow them on known attributes.
IMHO, validating attributes we manage in the same way for both --setattr
and standard attrs is not that wrong. It is a good precaution, because
if we let an unvalidated value in, it can make even a bigger mess later
in our pre_callbacks or post_callbacks where we assume that at this
point everything is valid.
If somebody wants to modify attributes in an uncontrolled, unvalidated
way, he is free to use ldapmodify or other tool to play with raw LDAP
values. Without our guarantee of course.
But if he chooses to use our --{set,add,del}attr we should at least help
him to not shoot himself to the leg and validate/normalize/encode the
value. I don't know how many users use this API, but removing a support
for all managed attributes seems as a big compatibility break to me.
Martin
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 68] text unit test should validate using installed mo file
On 04/09/2012 05:24 PM, John Dennis wrote: On 03/30/2012 08:57 AM, Petr Viktorin wrote: On 03/30/2012 02:41 AM, John Dennis wrote: On 03/28/2012 04:40 AM, Petr Viktorin wrote: Can install/po/Makefile just call test_i18n.py from the tests/ tree? It doesn't import any IPA code so there's no need to set sys.path in this case (though there'd have to be a comment saying we depend on this). In the other case, unit tests, the path is already set by Nose. Also the file would have to be renamed so nose doesn't pick it up as a test module. Good idea. I moved test_i18n.py to tests/i18n.py. I was reluctant about moving the file, but that was without merit, it works better this way. The downside is that the file now looks like a test utility module. It could use a comment near the imports saying that it's also called as a script, and that it shouldn't import IPA code (this could silently use the system-installed version of IPA, or crash if it's not there). Alternatively, set PYTHONPATH in the Makefile. I also removed the superfluous comment in Makefile.in you pointed out. When I was exercising the code I noticed the validation code was not treating msgid's from C code correctly (we do have some C code in the client area). That required a much more nuanced parsing the format conversion specifiers to correctly identify what was a positional format specifier vs. an indexed format specifier. The new version of the i18n.py includes the function parse_printf_fmt() and get_prog_langs() to identify the source programming language. More non-trivial code without tests. This makes me worry. But, tests for this can be added later I guess. Two more patches will follow shortly, one which adds validation when make lint is run and a patch to correct the problems it found in the C code strings which did not used indexed format specifiers. revised patch with comment about imports attached. ACK -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [RANT] --setattr validation is a minefield.
On 04/10/2012 07:07 PM, Martin Kosek wrote:
On Tue, 2012-04-10 at 17:03 +0200, Jan Cholasta wrote:
On 10.4.2012 16:00, Petr Viktorin wrote:
I'm aware that we have backwards compatibility requirements so we have
to stick with unfortunate decisions, but I wanted you to know what I
think. Please tell me I'm wrong!
It is not clear what --{set,add,del}attr and friends should do. On the
one hand they should be powerful -- presumably as powerful as
ldapmodify. On the other hand they should be checked to ensure they
can't be used to break the system. These requirements are contradictory.
And in either case we're doing it wrong:
- If they should be all-powerful, we shouldn't validate them.
- If they shouldn't break the system we can just disable them for
IPA-managed attributes. My understanding is that they were originally
only added for attributes IPA doesn't know about. People can still use
ldapmodify to bypass validation if they want.
- If somewhere in between, we need to clearly define what they should
do, instead of drawing the line ad-hoc based on individual details we
forgot about, as tickets come from QE.
I would hope people won't use --setattr for IPA-managed attributes.
Which would however mean we won't get much community testing for all
this extra code.
Then, there's an unfortunate detail in IPA implementation: attribute
Params need to be cloned to method objects (Create, Update, etc.) to
work properly (e.g. get the `attribute` flag set). If they are marked
no_update, they don't get cloned, so they don't work properly.
Yet --setattr apparently needs to be able to update and validate
attributes marked no_update (this ties to the confusing requirements on
--setattr I already mentioned). This leads to doing the same work again,
slightly differently.
tl;dr: --setattr work on IPA-managed attributes (with validation) is a
mistake. It adds no functionality, only complexity. We don't want people
to use it. It will cost us a lot of maintenance work to support.
Thank you for listening. A patch for the newest regression is coming up.
I wholeheartedly agree.
This is indeed a mine field and we need to make a look from at the issue
from all sides before accepting a decision.
Yes.
Like you said above, we should either not validate --{set,add,del}attr
or don't allow them on known attributes.
IMHO, validating attributes we manage in the same way for both --setattr
and standard attrs is not that wrong. It is a good precaution, because
if we let an unvalidated value in, it can make even a bigger mess later
in our pre_callbacks or post_callbacks where we assume that at this
point everything is valid.
Then we should validate *exactly* the same way, including not allowing
no_update attributes to be updated.
If somebody wants to modify attributes in an uncontrolled, unvalidated
way, he is free to use ldapmodify or other tool to play with raw LDAP
values. Without our guarantee of course.
That's clear.
But if he chooses to use our --{set,add,del}attr we should at least help
him to not shoot himself to the leg and validate/normalize/encode the
value. I don't know how many users use this API, but removing a support
for all managed attributes seems as a big compatibility break to me.
Well, it was broken (see https://fedorahosted.org/freeipa/ticket/2405,
2407, 2408), so I don't think many people used it.
Anyway, what's the use case? Why would the user want to use --setattr
for validated attributes? Is our regular API lacking something?
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [RANT] --setattr validation is a minefield.
Petr Viktorin wrote:
On 04/10/2012 07:07 PM, Martin Kosek wrote:
On Tue, 2012-04-10 at 17:03 +0200, Jan Cholasta wrote:
On 10.4.2012 16:00, Petr Viktorin wrote:
I'm aware that we have backwards compatibility requirements so we have
to stick with unfortunate decisions, but I wanted you to know what I
think. Please tell me I'm wrong!
It is not clear what --{set,add,del}attr and friends should do. On the
one hand they should be powerful -- presumably as powerful as
ldapmodify. On the other hand they should be checked to ensure they
can't be used to break the system. These requirements are
contradictory.
And in either case we're doing it wrong:
- If they should be all-powerful, we shouldn't validate them.
- If they shouldn't break the system we can just disable them for
IPA-managed attributes. My understanding is that they were originally
only added for attributes IPA doesn't know about. People can still use
ldapmodify to bypass validation if they want.
- If somewhere in between, we need to clearly define what they should
do, instead of drawing the line ad-hoc based on individual details we
forgot about, as tickets come from QE.
I would hope people won't use --setattr for IPA-managed attributes.
Which would however mean we won't get much community testing for all
this extra code.
Then, there's an unfortunate detail in IPA implementation: attribute
Params need to be cloned to method objects (Create, Update, etc.) to
work properly (e.g. get the `attribute` flag set). If they are marked
no_update, they don't get cloned, so they don't work properly.
Yet --setattr apparently needs to be able to update and validate
attributes marked no_update (this ties to the confusing requirements on
--setattr I already mentioned). This leads to doing the same work
again,
slightly differently.
tl;dr: --setattr work on IPA-managed attributes (with validation) is a
mistake. It adds no functionality, only complexity. We don't want
people
to use it. It will cost us a lot of maintenance work to support.
Thank you for listening. A patch for the newest regression is coming
up.
I wholeheartedly agree.
This is indeed a mine field and we need to make a look from at the issue
from all sides before accepting a decision.
Yes.
Like you said above, we should either not validate --{set,add,del}attr
or don't allow them on known attributes.
IMHO, validating attributes we manage in the same way for both --setattr
and standard attrs is not that wrong. It is a good precaution, because
if we let an unvalidated value in, it can make even a bigger mess later
in our pre_callbacks or post_callbacks where we assume that at this
point everything is valid.
Then we should validate *exactly* the same way, including not allowing
no_update attributes to be updated.
If somebody wants to modify attributes in an uncontrolled, unvalidated
way, he is free to use ldapmodify or other tool to play with raw LDAP
values. Without our guarantee of course.
That's clear.
But if he chooses to use our --{set,add,del}attr we should at least help
him to not shoot himself to the leg and validate/normalize/encode the
value. I don't know how many users use this API, but removing a support
for all managed attributes seems as a big compatibility break to me.
Well, it was broken (see https://fedorahosted.org/freeipa/ticket/2405,
2407, 2408), so I don't think many people used it.
Anyway, what's the use case? Why would the user want to use --setattr
for validated attributes? Is our regular API lacking something?
I think Honza had the best suggestion, enhance the standard API to
handle multi-valued attributes better and reduce the need for
set/add/delattr. At some future point we can consider dropping them when
updating something already covered by a Param. We're stuck with them for
now.
rob
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [RANT] --setattr validation is a minefield.
On Tue, Apr 10, 2012 at 10:25 AM, Petr Viktorin pvikt...@redhat.com wrote:
On 04/10/2012 07:07 PM, Martin Kosek wrote:
On Tue, 2012-04-10 at 17:03 +0200, Jan Cholasta wrote:
On 10.4.2012 16:00, Petr Viktorin wrote:
I'm aware that we have backwards compatibility requirements so we have
to stick with unfortunate decisions, but I wanted you to know what I
think. Please tell me I'm wrong!
It is not clear what --{set,add,del}attr and friends should do. On the
one hand they should be powerful -- presumably as powerful as
ldapmodify. On the other hand they should be checked to ensure they
can't be used to break the system. These requirements are contradictory.
And in either case we're doing it wrong:
- If they should be all-powerful, we shouldn't validate them.
- If they shouldn't break the system we can just disable them for
IPA-managed attributes. My understanding is that they were originally
only added for attributes IPA doesn't know about. People can still use
ldapmodify to bypass validation if they want.
- If somewhere in between, we need to clearly define what they should
do, instead of drawing the line ad-hoc based on individual details we
forgot about, as tickets come from QE.
I would hope people won't use --setattr for IPA-managed attributes.
Which would however mean we won't get much community testing for all
this extra code.
Then, there's an unfortunate detail in IPA implementation: attribute
Params need to be cloned to method objects (Create, Update, etc.) to
work properly (e.g. get the `attribute` flag set). If they are marked
no_update, they don't get cloned, so they don't work properly.
Yet --setattr apparently needs to be able to update and validate
attributes marked no_update (this ties to the confusing requirements on
--setattr I already mentioned). This leads to doing the same work again,
slightly differently.
tl;dr: --setattr work on IPA-managed attributes (with validation) is a
mistake. It adds no functionality, only complexity. We don't want people
to use it. It will cost us a lot of maintenance work to support.
Thank you for listening. A patch for the newest regression is coming up.
I wholeheartedly agree.
This is indeed a mine field and we need to make a look from at the issue
from all sides before accepting a decision.
Yes.
Like you said above, we should either not validate --{set,add,del}attr
or don't allow them on known attributes.
IMHO, validating attributes we manage in the same way for both --setattr
and standard attrs is not that wrong. It is a good precaution, because
if we let an unvalidated value in, it can make even a bigger mess later
in our pre_callbacks or post_callbacks where we assume that at this
point everything is valid.
Then we should validate *exactly* the same way, including not allowing
no_update attributes to be updated.
If somebody wants to modify attributes in an uncontrolled, unvalidated
way, he is free to use ldapmodify or other tool to play with raw LDAP
values. Without our guarantee of course.
That's clear.
But if he chooses to use our --{set,add,del}attr we should at least help
him to not shoot himself to the leg and validate/normalize/encode the
value. I don't know how many users use this API, but removing a support
for all managed attributes seems as a big compatibility break to me.
Well, it was broken (see https://fedorahosted.org/freeipa/ticket/2405, 2407,
2408), so I don't think many people used it.
Anyway, what's the use case? Why would the user want to use --setattr for
validated attributes? Is our regular API lacking something?
As a user of the IPA I thought I would throw in our use scenario. Let
me first say that I come to IPA having used Kerberos/LDAP solution
previously so I *really* appreciate what you've been able to do here.
That said, one of the things we are using IPA for is email
configuration. Although the mail attribute is available, we need to
separate by attribute the primary address from the aliases (for
searching the directory) so we need mailAlternateAddress. Luckily, it
is already in the schema. All we have to do is add the mailRecipient
objectclass. It's really nice to be able to add (and hopefully soon
remove) one objectclass without having to go back to ldap commands. I
certainly wouldn't anticipate changing or removing any IPA attributes,
but it sure is nice to have this feature for extra attributes.
Steve
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [RANT] --setattr validation is a minefield.
On 04/10/2012 01:48 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 04/10/2012 07:07 PM, Martin Kosek wrote:
On Tue, 2012-04-10 at 17:03 +0200, Jan Cholasta wrote:
On 10.4.2012 16:00, Petr Viktorin wrote:
I'm aware that we have backwards compatibility requirements so we
have
to stick with unfortunate decisions, but I wanted you to know what I
think. Please tell me I'm wrong!
It is not clear what --{set,add,del}attr and friends should do. On
the
one hand they should be powerful -- presumably as powerful as
ldapmodify. On the other hand they should be checked to ensure they
can't be used to break the system. These requirements are
contradictory.
And in either case we're doing it wrong:
- If they should be all-powerful, we shouldn't validate them.
- If they shouldn't break the system we can just disable them for
IPA-managed attributes. My understanding is that they were originally
only added for attributes IPA doesn't know about. People can still
use
ldapmodify to bypass validation if they want.
- If somewhere in between, we need to clearly define what they should
do, instead of drawing the line ad-hoc based on individual details we
forgot about, as tickets come from QE.
I would hope people won't use --setattr for IPA-managed attributes.
Which would however mean we won't get much community testing for all
this extra code.
Then, there's an unfortunate detail in IPA implementation: attribute
Params need to be cloned to method objects (Create, Update, etc.) to
work properly (e.g. get the `attribute` flag set). If they are marked
no_update, they don't get cloned, so they don't work properly.
Yet --setattr apparently needs to be able to update and validate
attributes marked no_update (this ties to the confusing
requirements on
--setattr I already mentioned). This leads to doing the same work
again,
slightly differently.
tl;dr: --setattr work on IPA-managed attributes (with validation)
is a
mistake. It adds no functionality, only complexity. We don't want
people
to use it. It will cost us a lot of maintenance work to support.
Thank you for listening. A patch for the newest regression is coming
up.
I wholeheartedly agree.
This is indeed a mine field and we need to make a look from at the
issue
from all sides before accepting a decision.
Yes.
Like you said above, we should either not validate --{set,add,del}attr
or don't allow them on known attributes.
IMHO, validating attributes we manage in the same way for both
--setattr
and standard attrs is not that wrong. It is a good precaution, because
if we let an unvalidated value in, it can make even a bigger mess later
in our pre_callbacks or post_callbacks where we assume that at this
point everything is valid.
Then we should validate *exactly* the same way, including not allowing
no_update attributes to be updated.
If somebody wants to modify attributes in an uncontrolled, unvalidated
way, he is free to use ldapmodify or other tool to play with raw LDAP
values. Without our guarantee of course.
That's clear.
But if he chooses to use our --{set,add,del}attr we should at least
help
him to not shoot himself to the leg and validate/normalize/encode the
value. I don't know how many users use this API, but removing a support
for all managed attributes seems as a big compatibility break to me.
Well, it was broken (see https://fedorahosted.org/freeipa/ticket/2405,
2407, 2408), so I don't think many people used it.
Anyway, what's the use case? Why would the user want to use --setattr
for validated attributes? Is our regular API lacking something?
I think Honza had the best suggestion, enhance the standard API to
handle multi-valued attributes better and reduce the need for
set/add/delattr. At some future point we can consider dropping them
when updating something already covered by a Param. We're stuck with
them for now.
The use case I would see is the extensibility. Say a customer wants to
extend a schema and add an attribute X to the user object. He would
still be able to manage users using CLI without writing a plugin for
the new attribute. Yes plugin is preferred but not everybody would go
for it. So in absence of the plugin we can't do validation but we still
should function and be able to deal with this attribute via CLI (and UI
if this attribute is enabled for UI via UI configuration).
I am generally against dropping this interface. But expectations IMO
should be:
1) If the attribute is managed by us with setattr and friends it should
behave in the same way as via the direct add/mod/del command
2) If attribute is not managed it should not provide any guarantees and
act in the same way as via LDAP
Hope this helps.
rob
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
--
Thank you,
Dmitri Pal
Sr. Engineering Manager IPA project,
Red Hat Inc.
Re: [Freeipa-devel] [PATCH] 0034 Limit permission and selfservice names
Petr Viktorin wrote: On 04/10/2012 03:46 PM, Rob Crittenden wrote: Petr Viktorin wrote: On 04/09/2012 03:55 PM, Rob Crittenden wrote: Petr Viktorin wrote: https://fedorahosted.org/freeipa/ticket/2585: ipa permission-add throws internal server error when name contains '', '' or other special characters. The problem is, of course, proper escaping; not only in DNs but also in ACIs. Right now we don't really do either. This patch is just a simple workaround: disallow anything except known-good characters. It's just names, so no functionality is lost. All tickets for April are now taken, so unless a new one comes my way, I'll take a dive into the code and fix it properly. This could take some time and would mean somewhat larger changes. Is there a reason you didn't use pattern/pattern_errmsg instead? You'd need to change the regex as patterns use re.match rather than re.search. rob Right, that makes more sense. It changes API.txt though. Do I need to bump VERSION in this case? Also, is there a reason pattern_errmsg is included in API.txt? Yes, please bump VERSION. Attaching updated patch. pattern_errmsg should probably be removed from API.txt. We've been paring back the amount of data to validate slowly as we've run into these questionable items. Please open a ticket for this. Done: https://fedorahosted.org/freeipa/ticket/2619 I made a minor change. VERSION shoudl just update the minor version number. I changed this, ACK, pushed to master and ipa-2-2 rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0035 Convert --setattr values for attributes marked no_update
Petr Viktorin wrote: Fix --setattr to work on no_update params. https://fedorahosted.org/freeipa/ticket/2616 ACK, pushed to master and ipa-2-2 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 120 Removal of memberofindirect_permissons from privileges
Petr Vobornik wrote: Problem: In the Privilege page, can list Permissions. This Shows Results for Direct Membership. But there is an option to list this for Indirect Membership also. There isn't a way to nest permissions, so this option is not needed. Solution: This patch removes the memberofindirect_persmission definition from server plugin. It fixes the problem in Web UI. https://fedorahosted.org/freeipa/ticket/2611 ACK, pushed to master and ipa-2-2 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 21 Unable to rename permission object
Ondrej Hamada wrote: https://fedorahosted.org/freeipa/ticket/2571 The update was failing because of the case insensitivity of permission object DN. Can you wrap the error in _() and add a couple of test cases for this, say one for the case insensitivity and one for empty rename attempt? rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0014 Add final debug message in installers
Petr Viktorin wrote:
On 03/30/2012 11:00 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 03/26/2012 05:35 PM, Petr Viktorin wrote:
On 03/26/2012 04:54 PM, Rob Crittenden wrote:
Some minor compliants.
Ideally, there would be a routine that sets up the logging and handles
command-line arguments in some uniform way (which is also needed before
logging setup to detect ipa-server-install --uninstall).
The original patch did the common logging setup, and I hacked around
the
install/uninstall problem too.
I guess I overdid it when I simplified the patch.
I'm somewhat confused about the scope, so bear with me as I clarify
what
you mean.
If you abort the installation you get this somewhat unnerving error:
Continue to configure the system with these values? [no]:
ipa : ERROR ipa-server-install failed, SystemExit: Installation
aborted
Installation aborted
ipa-ldap-updater is the same:
# ipa-ldap-updater
[2012-03-26T14:53:41Z ipa] ERROR: ipa-ldap-updater failed,
SystemExit:
IPA is not configured on this system.
IPA is not configured on this system.
and ipa-upgradeconfig
$ ipa-upgradeconfig
[2012-03-26T14:54:05Z ipa] ERROR: ipa-upgradeconfig failed,
SystemExit:
You must be root to run this script.
You must be root to run this script.
I'm guessing that the issue is that the log file isn't opened yet.
It would be nice if the logging would be confined to just the log.
If I understand you correctly, the code should check if logging has
been
configured already, and if not, skip displaying the message?
When uninstalling you get the message 'ipa-server-install successful'.
This is a little odd as well.
ipa-server-install is the name of the command. Wontfix for now, unless
you disagree strongly.
Updated patch: only log if logging has been configured (detected by
looking at the root logger's handlers), and changed the message to “The
ipa-server-install command has succeeded/failed”.
Works much better thanks. Just one request. When you created final_log()
you show less information than you did in earlier patches. It is nice
seeing the SystemExit failure. Can you do something like this (basically
cut-n-pasted from v05)?
diff --git a/ipaserver/install/installutils.py
b/ipaserver/install/installutils.
py
index 851b58d..ca82a1b 100644
--- a/ipaserver/install/installutils.py
+++ b/ipaserver/install/installutils.py
@@ -721,15 +721,15 @@ def script_context(operation_name):
# Only log if logging was already configured
# TODO: Do this properly (e.g. configure logging before the try/except)
if log_mgr.handlers.keys() != ['console']:
- root_logger.info(template, operation_name)
+ root_logger.info(template)
try:
yield
except BaseException, e:
if isinstance(e, SystemExit) and (e.code is None or e.code == 0):
# Not an error after all
- final_log('The %s command was successful')
+ final_log('The %s command was successful' % operation_name)
else:
- final_log('The %s command failed')
+ final_log('%s failed, %s: %s' % (operation_name, type(e).__name__,
e))
raise
else:
final_log('The %s command was successful')
This looks like:
2012-03-30T20:56:53Z INFO ipa-dns-install failed, SystemExit:
DNS is already configured in this IPA server.
rob
Fixed.
Hate to do this to you but I've found a few more issues. I basically
went down the list and ran all the commands in various conditions.
Some don't open any logs at all so the output gets written twice, like
ipa-replica-prepare and ipa-replica-manage:
# ipa-replica-manage del foo
Directory Manager password:
ipa: INFO: The ipa-replica-manage command failed, SystemExit:
'pony.greyoak.com' has no replication agreement for 'foo'
'pony.greyoak.com' has no replication agreement for 'foo'
Same with ipa-csreplica-manage.
# ipa-replica-prepare foo
Directory Manager (existing master) password:
ipa: INFO: The ipa-replica-prepare command failed, SystemExit:
The password provided is incorrect for LDAP server pony.greyoak.com
The password provided is incorrect for LDAP server pony.greyoak.com
rob
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 998 certmonger restarts services on renewal
On Fri, 2012-04-06 at 10:22 +0200, Martin Kosek wrote: On Thu, 2012-04-05 at 16:47 -0400, Rob Crittenden wrote: Rob Crittenden wrote: Martin Kosek wrote: On Tue, 2012-04-03 at 10:45 -0400, Rob Crittenden wrote: Rob Crittenden wrote: Martin Kosek wrote: On Mon, 2012-04-02 at 15:36 -0400, Rob Crittenden wrote: Rob Crittenden wrote: Martin Kosek wrote: On Tue, 2012-03-27 at 17:40 -0400, Rob Crittenden wrote: Certmonger will currently automatically renew server certificates but doesn't restart the services so you can still end up with expired certificates if you services never restart. This patch registers are restart command with certmonger so the IPA services will automatically be restarted to get the updated cert. Easy to test. Install IPA then resubmit the current server certs and watch the services restart: # ipa-getcert list Find the ID for either your dirsrv or httpd instance # ipa-getcert resubmit -iID Watch /var/log/httpd/error_log or /var/log/dirsrv/slapd-INSTANCE/errors to see the service restart. rob What about current instances - can we/do we want to update certmonger tracking so that their instances are restarted as well? Anyway, I found few issues SELinux issues with the patch: 1) # rpm -Uvh freeipa-* Preparing... ### [100%] 1:freeipa-python ### [ 20%] 2:freeipa-client ### [ 40%] 3:freeipa-admintools ### [ 60%] 4:freeipa-server ### [ 80%] /usr/bin/chcon: failed to change context of `/usr/lib64/ipa/certmonger' to `unconfined_u:object_r:certmonger_unconfined_exec_t:s0': Invalid argument /usr/bin/chcon: failed to change context of `/usr/lib64/ipa/certmonger/restart_dirsrv' to `unconfined_u:object_r:certmonger_unconfined_exec_t:s0': Invalid argument /usr/bin/chcon: failed to change context of `/usr/lib64/ipa/certmonger/restart_httpd' to `unconfined_u:object_r:certmonger_unconfined_exec_t:s0': Invalid argument warning: %post(freeipa-server-2.1.90GIT5b895af-0.fc16.x86_64) scriptlet failed, exit status 1 5:freeipa-server-selinux ### [100%] certmonger_unconfined_exec_t type was unknown with my selinux policy: selinux-policy-3.10.0-80.fc16.noarch selinux-policy-targeted-3.10.0-80.fc16.noarch If we need a higher SELinux version, we should bump the required package version spec file. Yeah, waiting on it to be backported. 2) Change of SELinux context with /usr/bin/chcon is temporary until restorecon or system relabel occurs. I think we should make it persistent and enforce this type in our SELinux policy and rather call restorecon instead of chcon That's a good idea, why didn't I think of that :-( Ah, now I remember, it will be handled by selinux-policy. I would have used restorecon here but since the policy isn't there yet this seemed like a good idea. I'm trying to find out the status of this new policy, it may only make it into F-17. rob Ok. But if this policy does not go in F-16 and if we want this fix in F16 release too, I guess we would have to implement both approaches in our spec file: 1) When on F16, include SELinux policy for restart scripts + run restorecon 2) When on F17, do not include the SELinux policy (+ run restorecon) Martin Won't work without updated selinux-policy. Without the permission for certmonger to execute the commands things will still fail (just in really non-obvious and far in the future ways). It looks like this is fixed in F-17 selinux-policy-3.10.0-107. rob Updated patch which works on F-17. rob What about F-16? The restart scripts won't work with enabled enforcing and will raise AVCs. Maybe we really need to deliver our own SELinux policy allowing it on F-16. Right, I don't see this working on F-16. I don't really want to carry this type of policy. It goes beyond marking a few files as certmonger_t, it is going to let certmonger execute arbitrary scripts. This is best left to the SELinux team who understand the consequences better. I also found an issue with the restart scripts: 1) restart_dirsrv: this won't work with systemd: # /sbin/service dirsrv restart Redirecting to /bin/systemctl restart dirsrv.service Failed to issue method call: Unit dirsrv.service failed to load: No such file or directory. See system logs and 'systemctl status dirsrv.service' for details. Wouldn't work so hot for sysV either as we'd be restarting all instances. I'll take a look. We would need to pass an instance of IPA dirsrv for this to
Re: [Freeipa-devel] [PATCH] 998 certmonger restarts services on renewal
On 04/10/2012 04:48 PM, Martin Kosek wrote: On Fri, 2012-04-06 at 10:22 +0200, Martin Kosek wrote: On Thu, 2012-04-05 at 16:47 -0400, Rob Crittenden wrote: Rob Crittenden wrote: Martin Kosek wrote: On Tue, 2012-04-03 at 10:45 -0400, Rob Crittenden wrote: Rob Crittenden wrote: Martin Kosek wrote: On Mon, 2012-04-02 at 15:36 -0400, Rob Crittenden wrote: Rob Crittenden wrote: Martin Kosek wrote: On Tue, 2012-03-27 at 17:40 -0400, Rob Crittenden wrote: Certmonger will currently automatically renew server certificates but doesn't restart the services so you can still end up with expired certificates if you services never restart. This patch registers are restart command with certmonger so the IPA services will automatically be restarted to get the updated cert. Easy to test. Install IPA then resubmit the current server certs and watch the services restart: # ipa-getcert list Find the ID for either your dirsrv or httpd instance # ipa-getcert resubmit -iID Watch /var/log/httpd/error_log or /var/log/dirsrv/slapd-INSTANCE/errors to see the service restart. rob What about current instances - can we/do we want to update certmonger tracking so that their instances are restarted as well? Anyway, I found few issues SELinux issues with the patch: 1) # rpm -Uvh freeipa-* Preparing... ### [100%] 1:freeipa-python ### [ 20%] 2:freeipa-client ### [ 40%] 3:freeipa-admintools ### [ 60%] 4:freeipa-server ### [ 80%] /usr/bin/chcon: failed to change context of `/usr/lib64/ipa/certmonger' to `unconfined_u:object_r:certmonger_unconfined_exec_t:s0': Invalid argument /usr/bin/chcon: failed to change context of `/usr/lib64/ipa/certmonger/restart_dirsrv' to `unconfined_u:object_r:certmonger_unconfined_exec_t:s0': Invalid argument /usr/bin/chcon: failed to change context of `/usr/lib64/ipa/certmonger/restart_httpd' to `unconfined_u:object_r:certmonger_unconfined_exec_t:s0': Invalid argument warning: %post(freeipa-server-2.1.90GIT5b895af-0.fc16.x86_64) scriptlet failed, exit status 1 5:freeipa-server-selinux ### [100%] certmonger_unconfined_exec_t type was unknown with my selinux policy: selinux-policy-3.10.0-80.fc16.noarch selinux-policy-targeted-3.10.0-80.fc16.noarch If we need a higher SELinux version, we should bump the required package version spec file. Yeah, waiting on it to be backported. 2) Change of SELinux context with /usr/bin/chcon is temporary until restorecon or system relabel occurs. I think we should make it persistent and enforce this type in our SELinux policy and rather call restorecon instead of chcon That's a good idea, why didn't I think of that :-( Ah, now I remember, it will be handled by selinux-policy. I would have used restorecon here but since the policy isn't there yet this seemed like a good idea. I'm trying to find out the status of this new policy, it may only make it into F-17. rob Ok. But if this policy does not go in F-16 and if we want this fix in F16 release too, I guess we would have to implement both approaches in our spec file: 1) When on F16, include SELinux policy for restart scripts + run restorecon 2) When on F17, do not include the SELinux policy (+ run restorecon) Martin Won't work without updated selinux-policy. Without the permission for certmonger to execute the commands things will still fail (just in really non-obvious and far in the future ways). It looks like this is fixed in F-17 selinux-policy-3.10.0-107. rob Updated patch which works on F-17. rob What about F-16? The restart scripts won't work with enabled enforcing and will raise AVCs. Maybe we really need to deliver our own SELinux policy allowing it on F-16. Right, I don't see this working on F-16. I don't really want to carry this type of policy. It goes beyond marking a few files as certmonger_t, it is going to let certmonger execute arbitrary scripts. This is best left to the SELinux team who understand the consequences better. I also found an issue with the restart scripts: 1) restart_dirsrv: this won't work with systemd: # /sbin/service dirsrv restart Redirecting to /bin/systemctl restart dirsrv.service Failed to issue method call: Unit dirsrv.service failed to load: No such file or directory. See system logs and 'systemctl status dirsrv.service' for details. Wouldn't work so hot for sysV either as we'd be restarting all instances. I'll take a look. We would need to pass an instance of IPA dirsrv for this to work. 2) restart_httpd: Is reload enough for httpd to pull a new certificate? Don't we need a full restart? If reload is enough, I think the command should be named reload_httpd Yes, it causes the modules to be reloaded which will reload the NSS
Re: [Freeipa-devel] [PATCH] 998 certmonger restarts services on renewal
Martin Kosek wrote: On Fri, 2012-04-06 at 10:22 +0200, Martin Kosek wrote: On Thu, 2012-04-05 at 16:47 -0400, Rob Crittenden wrote: Rob Crittenden wrote: Martin Kosek wrote: On Tue, 2012-04-03 at 10:45 -0400, Rob Crittenden wrote: Rob Crittenden wrote: Martin Kosek wrote: On Mon, 2012-04-02 at 15:36 -0400, Rob Crittenden wrote: Rob Crittenden wrote: Martin Kosek wrote: On Tue, 2012-03-27 at 17:40 -0400, Rob Crittenden wrote: Certmonger will currently automatically renew server certificates but doesn't restart the services so you can still end up with expired certificates if you services never restart. This patch registers are restart command with certmonger so the IPA services will automatically be restarted to get the updated cert. Easy to test. Install IPA then resubmit the current server certs and watch the services restart: # ipa-getcert list Find the ID for either your dirsrv or httpd instance # ipa-getcert resubmit -iID Watch /var/log/httpd/error_log or /var/log/dirsrv/slapd-INSTANCE/errors to see the service restart. rob What about current instances - can we/do we want to update certmonger tracking so that their instances are restarted as well? Anyway, I found few issues SELinux issues with the patch: 1) # rpm -Uvh freeipa-* Preparing... ### [100%] 1:freeipa-python ### [ 20%] 2:freeipa-client ### [ 40%] 3:freeipa-admintools ### [ 60%] 4:freeipa-server ### [ 80%] /usr/bin/chcon: failed to change context of `/usr/lib64/ipa/certmonger' to `unconfined_u:object_r:certmonger_unconfined_exec_t:s0': Invalid argument /usr/bin/chcon: failed to change context of `/usr/lib64/ipa/certmonger/restart_dirsrv' to `unconfined_u:object_r:certmonger_unconfined_exec_t:s0': Invalid argument /usr/bin/chcon: failed to change context of `/usr/lib64/ipa/certmonger/restart_httpd' to `unconfined_u:object_r:certmonger_unconfined_exec_t:s0': Invalid argument warning: %post(freeipa-server-2.1.90GIT5b895af-0.fc16.x86_64) scriptlet failed, exit status 1 5:freeipa-server-selinux ### [100%] certmonger_unconfined_exec_t type was unknown with my selinux policy: selinux-policy-3.10.0-80.fc16.noarch selinux-policy-targeted-3.10.0-80.fc16.noarch If we need a higher SELinux version, we should bump the required package version spec file. Yeah, waiting on it to be backported. 2) Change of SELinux context with /usr/bin/chcon is temporary until restorecon or system relabel occurs. I think we should make it persistent and enforce this type in our SELinux policy and rather call restorecon instead of chcon That's a good idea, why didn't I think of that :-( Ah, now I remember, it will be handled by selinux-policy. I would have used restorecon here but since the policy isn't there yet this seemed like a good idea. I'm trying to find out the status of this new policy, it may only make it into F-17. rob Ok. But if this policy does not go in F-16 and if we want this fix in F16 release too, I guess we would have to implement both approaches in our spec file: 1) When on F16, include SELinux policy for restart scripts + run restorecon 2) When on F17, do not include the SELinux policy (+ run restorecon) Martin Won't work without updated selinux-policy. Without the permission for certmonger to execute the commands things will still fail (just in really non-obvious and far in the future ways). It looks like this is fixed in F-17 selinux-policy-3.10.0-107. rob Updated patch which works on F-17. rob What about F-16? The restart scripts won't work with enabled enforcing and will raise AVCs. Maybe we really need to deliver our own SELinux policy allowing it on F-16. Right, I don't see this working on F-16. I don't really want to carry this type of policy. It goes beyond marking a few files as certmonger_t, it is going to let certmonger execute arbitrary scripts. This is best left to the SELinux team who understand the consequences better. I also found an issue with the restart scripts: 1) restart_dirsrv: this won't work with systemd: # /sbin/service dirsrv restart Redirecting to /bin/systemctl restart dirsrv.service Failed to issue method call: Unit dirsrv.service failed to load: No such file or directory. See system logs and 'systemctl status dirsrv.service' for details. Wouldn't work so hot for sysV either as we'd be restarting all instances. I'll take a look. We would need to pass an instance of IPA dirsrv for this to work. 2) restart_httpd: Is reload enough for httpd to pull a new certificate? Don't we need a full restart? If reload is enough, I think the command should be named reload_httpd Yes, it causes the modules to be reloaded which will reload the NSS database, that's all we need. I named it this way for consistency. I can rename it, though I doubt
