Re: [Freeipa-devel] [RANT] --setattr validation is a minefield.
On 05/14/2012 04:00 PM, Rob Crittenden wrote:
Martin Kosek wrote:
On Thu, 2012-05-10 at 15:19 +0200, Petr Viktorin wrote:
On 04/10/2012 07:53 PM, Martin Kosek wrote:
On Tue, 2012-04-10 at 19:25 +0200, 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:
[snip]
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.
That makes some sense, I could agree with that.
Now that I have a ticket on this
(https://fedorahosted.org/freeipa/ticket/2580), I would like to get some
wider agreement here.
The no_update/no_create attributes are mainly enabled flags
(ipaenabledflag, nsaccountlock, idnszoneactive), administrative
(krbprincipalname, ipauniqueid, ipacertificatesubjectbase), DNS record
type and data, and various virtual attributes.
If setattr etc. is disabled for all of these, it will mainly matter for
the enabled flags. To be honest I don't know why we only allow
modifying those through special commands.
If there's some security reason for that, then setattr etc. should be
disabled for them; otherwise I think they should be changeable through
xyz-mod.
I am not aware of any security reasons why we use special commands for
enabling/disabling objects. I assume this is to make it different from
standard object attribute changes and make sure that user does not
disable the object by accident when doing a mod operation. Rob, maybe
you remember the reason for this interface
But since we already have this approach, we should keep it and implement
missing xyz-enable and xyz-disable command so that user's using
*attr interface to play with enabled/disabled attributes can switch to
the specialized commands.
So far, I noticed that only DNS zone object misses the enable/disable
commands, I created a ticket to fix that:
https://fedorahosted.org/freeipa/ticket/2754
Either way, setattr etc. should honor the no_update flags. Any
objections?
Nope - as long as ticket 2754 is fixed.
Martin
I think those are there so they don't show up for the -mod command since
we have a separate interface for doing it.
rob
For that purpose, would a no_cli flag be better than no_create+no_update?
I found one more no_update attribute that might be useful with
--setattr: externalhost.
--
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 05/14/2012 04:00 PM, Rob Crittenden wrote:
Martin Kosek wrote:
On Thu, 2012-05-10 at 15:19 +0200, Petr Viktorin wrote:
On 04/10/2012 07:53 PM, Martin Kosek wrote:
On Tue, 2012-04-10 at 19:25 +0200, 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:
[snip]
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.
That makes some sense, I could agree with that.
Now that I have a ticket on this
(https://fedorahosted.org/freeipa/ticket/2580), I would like to get
some
wider agreement here.
The no_update/no_create attributes are mainly enabled flags
(ipaenabledflag, nsaccountlock, idnszoneactive), administrative
(krbprincipalname, ipauniqueid, ipacertificatesubjectbase), DNS record
type and data, and various virtual attributes.
If setattr etc. is disabled for all of these, it will mainly matter for
the enabled flags. To be honest I don't know why we only allow
modifying those through special commands.
If there's some security reason for that, then setattr etc. should be
disabled for them; otherwise I think they should be changeable through
xyz-mod.
I am not aware of any security reasons why we use special commands for
enabling/disabling objects. I assume this is to make it different from
standard object attribute changes and make sure that user does not
disable the object by accident when doing a mod operation. Rob, maybe
you remember the reason for this interface
But since we already have this approach, we should keep it and implement
missing xyz-enable and xyz-disable command so that user's using
*attr interface to play with enabled/disabled attributes can switch to
the specialized commands.
So far, I noticed that only DNS zone object misses the enable/disable
commands, I created a ticket to fix that:
https://fedorahosted.org/freeipa/ticket/2754
Either way, setattr etc. should honor the no_update flags. Any
objections?
Nope - as long as ticket 2754 is fixed.
Martin
I think those are there so they don't show up for the -mod command since
we have a separate interface for doing it.
rob
For that purpose, would a no_cli flag be better than no_create+no_update?
I found one more no_update attribute that might be useful with
--setattr: externalhost.
I think the flags are really only considered on the cli now. If a
different flag can make the intention clearer I'm ok with that. Having
two flags seems more flexible though.
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 05/18/2012 03:49 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 05/14/2012 04:00 PM, Rob Crittenden wrote:
Martin Kosek wrote:
On Thu, 2012-05-10 at 15:19 +0200, Petr Viktorin wrote:
On 04/10/2012 07:53 PM, Martin Kosek wrote:
On Tue, 2012-04-10 at 19:25 +0200, 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:
[snip]
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.
That makes some sense, I could agree with that.
Now that I have a ticket on this
(https://fedorahosted.org/freeipa/ticket/2580), I would like to get
some
wider agreement here.
The no_update/no_create attributes are mainly enabled flags
(ipaenabledflag, nsaccountlock, idnszoneactive), administrative
(krbprincipalname, ipauniqueid, ipacertificatesubjectbase), DNS record
type and data, and various virtual attributes.
If setattr etc. is disabled for all of these, it will mainly matter
for
the enabled flags. To be honest I don't know why we only allow
modifying those through special commands.
If there's some security reason for that, then setattr etc. should be
disabled for them; otherwise I think they should be changeable through
xyz-mod.
I am not aware of any security reasons why we use special commands for
enabling/disabling objects. I assume this is to make it different from
standard object attribute changes and make sure that user does not
disable the object by accident when doing a mod operation. Rob, maybe
you remember the reason for this interface
But since we already have this approach, we should keep it and
implement
missing xyz-enable and xyz-disable command so that user's using
*attr interface to play with enabled/disabled attributes can switch to
the specialized commands.
So far, I noticed that only DNS zone object misses the enable/disable
commands, I created a ticket to fix that:
https://fedorahosted.org/freeipa/ticket/2754
Either way, setattr etc. should honor the no_update flags. Any
objections?
Nope - as long as ticket 2754 is fixed.
Martin
I think those are there so they don't show up for the -mod command since
we have a separate interface for doing it.
rob
For that purpose, would a no_cli flag be better than no_create+no_update?
I found one more no_update attribute that might be useful with
--setattr: externalhost.
I think the flags are really only considered on the cli now. If a
different flag can make the intention clearer I'm ok with that. Having
two flags seems more flexible though.
rob
No. When the no_update flag is specified, the param doesn't get cloned
from the object to the -mod command at all.
(A lot of the getattr problems occured because this cloning also sets
important data, the attribute bit, so the uncloned params are
incomplete/unusable but the cloned ones aren't available.)
So, I want to have no_create/no_update for things we really don't want
the user to change (even through setattr), and hidden (a better name
for it than no_cli IMO) for things we just don't advertise because
there's a different way to do it.
It would also make sense to make the hidden attributes available in
-find – AFAIK, currently you can't search for disabled users for example.
As for two flags being more flexible, I can't find a case where we'd
want to hide an attribute in -mod but not in -add (or vice versa). I
think that'd just be an unnecessary inconsistency. (Remember this is
just hiding the param from the frontend; krbprincipalname is rightly
no_update only.)
Also, in the future, I think the enable/disable commands should call the
-mod command (or the other way around), so there's just one code path
for bugs to hide in.
--
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 Thu, 2012-05-10 at 15:19 +0200, Petr Viktorin wrote:
On 04/10/2012 07:53 PM, Martin Kosek wrote:
On Tue, 2012-04-10 at 19:25 +0200, 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:
[snip]
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.
That makes some sense, I could agree with that.
Now that I have a ticket on this
(https://fedorahosted.org/freeipa/ticket/2580), I would like to get some
wider agreement here.
The no_update/no_create attributes are mainly enabled flags
(ipaenabledflag, nsaccountlock, idnszoneactive), administrative
(krbprincipalname, ipauniqueid, ipacertificatesubjectbase), DNS record
type and data, and various virtual attributes.
If setattr etc. is disabled for all of these, it will mainly matter for
the enabled flags. To be honest I don't know why we only allow
modifying those through special commands.
If there's some security reason for that, then setattr etc. should be
disabled for them; otherwise I think they should be changeable through
xyz-mod.
I am not aware of any security reasons why we use special commands for
enabling/disabling objects. I assume this is to make it different from
standard object attribute changes and make sure that user does not
disable the object by accident when doing a mod operation. Rob, maybe
you remember the reason for this interface
But since we already have this approach, we should keep it and implement
missing xyz-enable and xyz-disable command so that user's using
*attr interface to play with enabled/disabled attributes can switch to
the specialized commands.
So far, I noticed that only DNS zone object misses the enable/disable
commands, I created a ticket to fix that:
https://fedorahosted.org/freeipa/ticket/2754
Either way, setattr etc. should honor the no_update flags. Any objections?
Nope - as long as ticket 2754 is fixed.
Martin
___
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 Mon, 2012-05-14 at 09:36 +0200, Martin Kosek wrote:
On Thu, 2012-05-10 at 15:19 +0200, Petr Viktorin wrote:
On 04/10/2012 07:53 PM, Martin Kosek wrote:
On Tue, 2012-04-10 at 19:25 +0200, 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:
[snip]
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.
That makes some sense, I could agree with that.
Now that I have a ticket on this
(https://fedorahosted.org/freeipa/ticket/2580), I would like to get some
wider agreement here.
The no_update/no_create attributes are mainly enabled flags
(ipaenabledflag, nsaccountlock, idnszoneactive), administrative
(krbprincipalname, ipauniqueid, ipacertificatesubjectbase), DNS record
type and data, and various virtual attributes.
If setattr etc. is disabled for all of these, it will mainly matter for
the enabled flags. To be honest I don't know why we only allow
modifying those through special commands.
If there's some security reason for that, then setattr etc. should be
disabled for them; otherwise I think they should be changeable through
xyz-mod.
I am not aware of any security reasons why we use special commands for
enabling/disabling objects. I assume this is to make it different from
standard object attribute changes and make sure that user does not
disable the object by accident when doing a mod operation. Rob, maybe
you remember the reason for this interface
But since we already have this approach, we should keep it and implement
missing xyz-enable and xyz-disable command so that user's using
*attr interface to play with enabled/disabled attributes can switch to
the specialized commands.
So far, I noticed that only DNS zone object misses the enable/disable
commands, I created a ticket to fix that:
https://fedorahosted.org/freeipa/ticket/2754
Either way, setattr etc. should honor the no_update flags. Any objections?
Nope - as long as ticket 2754 is fixed.
I just noticed we have the enable/disable command for DNS zone as well,
I somehow managed to overlook it... Sorry for noise, closing the ticket
2754.
Martin
___
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.
Martin Kosek wrote:
On Thu, 2012-05-10 at 15:19 +0200, Petr Viktorin wrote:
On 04/10/2012 07:53 PM, Martin Kosek wrote:
On Tue, 2012-04-10 at 19:25 +0200, 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:
[snip]
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.
That makes some sense, I could agree with that.
Now that I have a ticket on this
(https://fedorahosted.org/freeipa/ticket/2580), I would like to get some
wider agreement here.
The no_update/no_create attributes are mainly enabled flags
(ipaenabledflag, nsaccountlock, idnszoneactive), administrative
(krbprincipalname, ipauniqueid, ipacertificatesubjectbase), DNS record
type and data, and various virtual attributes.
If setattr etc. is disabled for all of these, it will mainly matter for
the enabled flags. To be honest I don't know why we only allow
modifying those through special commands.
If there's some security reason for that, then setattr etc. should be
disabled for them; otherwise I think they should be changeable through
xyz-mod.
I am not aware of any security reasons why we use special commands for
enabling/disabling objects. I assume this is to make it different from
standard object attribute changes and make sure that user does not
disable the object by accident when doing a mod operation. Rob, maybe
you remember the reason for this interface
But since we already have this approach, we should keep it and implement
missing xyz-enable and xyz-disable command so that user's using
*attr interface to play with enabled/disabled attributes can switch to
the specialized commands.
So far, I noticed that only DNS zone object misses the enable/disable
commands, I created a ticket to fix that:
https://fedorahosted.org/freeipa/ticket/2754
Either way, setattr etc. should honor the no_update flags. Any objections?
Nope - as long as ticket 2754 is fixed.
Martin
I think those are there so they don't show up for the -mod command since
we have a separate interface for doing it.
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 04/10/2012 07:53 PM, Martin Kosek wrote:
On Tue, 2012-04-10 at 19:25 +0200, 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:
[snip]
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.
That makes some sense, I could agree with that.
Now that I have a ticket on this
(https://fedorahosted.org/freeipa/ticket/2580), I would like to get some
wider agreement here.
The no_update/no_create attributes are mainly enabled flags
(ipaenabledflag, nsaccountlock, idnszoneactive), administrative
(krbprincipalname, ipauniqueid, ipacertificatesubjectbase), DNS record
type and data, and various virtual attributes.
If setattr etc. is disabled for all of these, it will mainly matter for
the enabled flags. To be honest I don't know why we only allow
modifying those through special commands.
If there's some security reason for that, then setattr etc. should be
disabled for them; otherwise I think they should be changeable through
xyz-mod.
Either way, setattr etc. should honor the no_update flags. Any objections?
--
Petr³
PS. For reference, our no_create/no_update params are:
automember
automemberdefaultgroup: no_create no_search no_update
key: no_create no_search no_update
automountkey
description: no_create no_output no_search no_update
config
ipacertificatesubjectbase: no_update
dnsrecord
dnsrecords: no_create no_search no_update
dnstype: no_create no_search no_update
dnsdata: no_create no_search no_update
a_extra_create_reverse: dnsrecord_extra no_update virtual_attribute
_extra_create_reverse: dnsrecord_extra no_update virtual_attribute
dnszone
idnszoneactive: no_create no_update
entitle
uuid: no_create no_update
ipaentitlementid: no_create no_update
hbacrule
ipaenabledflag: no_create no_search no_update
memberuser_user: no_create no_search no_update
memberuser_group: no_create no_search no_update
memberhost_host: no_create no_search no_update
memberhost_hostgroup: no_create no_search no_update
sourcehost_host: no_create no_search no_update
sourcehost_hostgroup: no_create no_search no_update
memberservice_hbacsvc: no_create no_search no_update
memberservice_hbacsvcgroup: no_create no_search no_update
host
randompassword: no_create no_search no_update virtual_attribute
krbprincipalname: no_create no_search no_update
sshpubkeyfp: no_create no_search no_update virtual_attribute
netgroup
ipauniqueid: no_create no_update
selinuxusermap
ipaenabledflag: no_create no_search no_update
memberuser_user: no_create no_search no_update
memberuser_group: no_create no_search no_update
memberhost_host: no_create no_search no_update
memberhost_hostgroup: no_create no_search no_update
sudocmdgroup
membercmd_sudocmd: no_create no_search no_update
membercmd_sudocmdgroup: no_create no_search no_update
sudorule
ipaenabledflag: no_create no_search no_update
memberuser_user: no_create no_search no_update
memberuser_group: no_create no_search no_update
memberhost_host: no_create no_search no_update
memberhost_hostgroup: no_create no_search no_update
memberallowcmd_sudocmd: no_create no_search no_update
memberdenycmd_sudocmd: no_create no_search no_update
memberallowcmd_sudocmdgroup: no_create no_search no_update
memberdenycmd_sudocmdgroup: no_create no_search no_update
ipasudorunas_user: no_create no_search no_update
ipasudorunas_group: no_create no_search no_update
ipasudoopt: no_create no_search no_update
ipasudorunasgroup_group: no_create no_search no_update
user
krbprincipalname: no_update
randompassword: no_create no_search no_update virtual_attribute
nsaccountlock: no_create no_search no_update
sshpubkeyfp: no_create no_search no_update virtual_attribute
___
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/11/2012 09:42 AM, Jan Cholasta wrote:
On 11.4.2012 09:27, Martin Kosek wrote:
On Wed, 2012-04-11 at 09:18 +0200, Jan Cholasta wrote:
On 10.4.2012 19:56, Dmitri Pal wrote:
...
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
Does this include refusing to modify no_update attributes?
Or do we treat those the same way as non-managed attributes (no
validation.conversion)?
Or is it okay as it's done now, validating and updating as if no_update
wasn't there?
2) If attribute is not managed it should not provide any guarantees and
act in the same way as via LDAP
I never had an issue with the non-managed attributes; addattr is perfect
for those.
Hope this helps.
This might be the best thing to do, but IMO it is still no good, because
the behavior of --{set,add,del}attr for a particular attribute might
change between API versions, when that attribute changes from unmanaged
to managed.
Honza
I think this is OK and expectable - user may use setattr option to set
an attribute before it is officially supported by IPA and still have it
working when he upgrades.
There's no guarantee the user will still have it working. User
application might break if it depends on the unmanaged behavior and we
are too strict in validation of the managed attribute, for example. I
don't known how much of an issue is this actually, but this kind of
unpredictability is not a good thing IMHO.
Since our validation now allows working with bad attributes, just not
adding new ones, I think this is not much of a problem.
Though we should make sure that we describe
this API well in our documentation to make sure this expectation is
shared with users.
Martin
Honza
--
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 Tue, 2012-04-10 at 13:56 -0400, Dmitri Pal wrote: On 04/10/2012 01:48 PM, Rob Crittenden wrote: [snip] 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. I agree with your points, that's what I was trying to say in my previous mail. I think that all the grief is caused by expectation 1) which is broken with current setattr options. If we fix that (preferably in 3.0), I would keep this API. Martin ___ 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 10.4.2012 19:56, Dmitri Pal wrote:
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.
This might be the best thing to do, but IMO it is still no good, because
the behavior of --{set,add,del}attr for a particular attribute might
change between API versions, when that attribute changes from unmanaged
to managed.
Honza
--
Jan Cholasta
Re: [Freeipa-devel] [RANT] --setattr validation is a minefield.
On 11.4.2012 09:27, Martin Kosek wrote:
On Wed, 2012-04-11 at 09:18 +0200, Jan Cholasta wrote:
On 10.4.2012 19:56, Dmitri Pal wrote:
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.
This might be the best thing to do, but IMO it is still no good, because
the behavior of --{set,add,del}attr for a particular attribute might
change between API versions,
Re: [Freeipa-devel] [RANT] --setattr validation is a minefield.
On 04/11/2012 03:42 AM, Jan Cholasta wrote:
On 11.4.2012 09:27, Martin Kosek wrote:
On Wed, 2012-04-11 at 09:18 +0200, Jan Cholasta wrote:
On 10.4.2012 19:56, Dmitri Pal wrote:
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.
This might be the best thing
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] [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] [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
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] [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.
