On 28.5.2014 22:44, Nathaniel McCallum wrote:
On Mon, 2014-05-26 at 16:57 +0200, Jan Cholasta wrote:
On 13.5.2014 19:12, Nathaniel McCallum wrote:
On Tue, 2014-05-13 at 16:33 +0200, Jan Cholasta wrote:
On 12.5.2014 21:02, Nathaniel McCallum wrote:
On Thu, 2014-05-08 at 13:51 -0400, Simo Sorce wrote:
On Thu, 2014-05-08 at 12:26 -0400, Nathaniel McCallum wrote:
On Wed, 2014-05-07 at 11:17 -0400, Simo Sorce wrote:
On Wed, 2014-05-07 at 09:54 -0400, Dmitri Pal wrote:
On 05/07/2014 09:05 AM, Nathaniel McCallum wrote:
On Wed, 2014-05-07 at 11:42 +0200, Jan Cholasta wrote:
Hi,
On 6.5.2014 17:08, Nathaniel McCallum wrote:
On Tue, 2014-05-06 at 09:49 -0400, Nathaniel McCallum wrote:
On Mon, 2014-05-05 at 12:42 -0400, Nathaniel McCallum wrote:
This also constitutes a rethinking of the token ACIs after the
introduction of SELFDN support.
Admins, as before, have full access to all token permissions.
Normal users have read/search/compare access to all of the non-secret
data for tokens assigned to them, whether protected or non-protected.
Users can add or delete non-protected tokens and modify most of their
metadata. However they cannot create, delete or modify protected tokens.
Regardless of whether the token is protected or not, users cannot change
a token's ownership or unique identity.
In contrast, admins can create protected tokens. This protects the token
from deletion or modification when assigned to users. Additionally, when
a user account is deleted, the assigned non-protected tokens are deleted
but the protected tokens are merely orphaned. This permits the token to
be reassigned without having to recreate it. This last point is
particularly useful in the case of hardware tokens.
https://fedorahosted.org/freeipa/ticket/4228
NOTE: This patch depends on my patch 0048.
This new version makes ipatokenDisabled visible for token owners. It is
also writable if the token is non-protected. This additionally fixes:
https://fedorahosted.org/freeipa/ticket/4259
This new version changes the way the default value of protected is setup
in accordance with the changes made for the review of my patch 0048.2.
Nathaniel
Is using the ipatokenprotected attribute the final design?
No. Alternate designs are welcome. The code is easy enough to modify.
I did not dig too deep into this, but I think it might be better to
instead use the managedby attribute on a token to limit who can delete
(or administer in other way) the token. On otptoken-add, managedby would
be set to the "whoami" user DN, unless run with --protected, in which
case managedby would be left empty. Then, when deleting a user, the
token would be deleted only if the user manages the token.
It seems to me that the mechanics of this are roughly the same as
protected, just with a different syntax. The cost of this is more
complex ACIs. In particular, we'd have to use two userdn clauses (is
this possible?) instead of a simple filter. If there is a clear benefit,
we can justify the more obtuse syntax.
We usually try not to create new attributes until it is fully justified.
I would like Simo to chime in on this.
I would also prefer to reuse existing attributes and mechanism if
possible and if it will not preclude future work.
In this case, it "sounds" like managed-by has the appropriate meaning:
"who manages the token ?" -> myself, admin, other, none ?
Nathaniel can you send 2 lines showing the difference in ACIs between
using managed-by vs a new attribute ?
These are the ACIs using the protected mechanism:
aci: (targetfilter = "(objectClass=ipaToken)")(targetattrs =
"objectclass || description || ipatokenUniqueID || ipatokenDisabled ||
ipatokenNotBefore || ipatokenNotAfter || ipatokenVendor || ipatokenModel
|| ipatokenSerial || ipatokenOwner || ipatokenProtected")(version 3.0;
acl "Users can read basic token info"; allow (read, search, compare)
userattr = "ipatokenOwner#USERDN";)
aci: (targetfilter = "(objectClass=ipatokenTOTP)")(targetattrs =
"ipatokenOTPalgorithm || ipatokenOTPdigits ||
ipatokenTOTPtimeStep")(version 3.0; acl "Users can see TOTP details";
allow (read, search, compare) userattr = "ipatokenOwner#USERDN";)
aci: (targetfilter = "(objectClass=ipatokenHOTP)")(targetattrs =
"ipatokenOTPalgorithm || ipatokenOTPdigits")(version 3.0; acl "Users can
see HOTP details"; allow (read, search, compare) userattr =
"ipatokenOwner#USERDN";)
aci: (targetfilter =
"(&(objectClass=ipaToken)(!(ipatokenProtected=TRUE)))")(targetattrs =
"description || ipatokenDisabled || ipatokenNotBefore ||
ipatokenNotAfter || ipatokenVendor || ipatokenModel ||
ipatokenSerial")(version 3.0; acl "Users can write basic token info";
allow (write) userattr = "ipatokenOwner#USERDN";)
aci: (target = "ldap:///ipatokenuniqueid=*,cn=otp,$SUFFIX";)(targetfilter
= "(&(objectClass=ipaToken)(!(ipatokenProtected=TRUE))))")(version 3.0;
acl "Users can create and delete tokens"; allow (add, delete) userattr =
"ipatokenOwner#SELFDN";)
This is what they look like using managedBy (I have not tested this):
aci: (targetfilter = "(objectClass=ipaToken)")(targetattrs =
"objectclass || description || ipatokenUniqueID || ipatokenDisabled ||
ipatokenNotBefore || ipatokenNotAfter || ipatokenVendor || ipatokenModel
|| ipatokenSerial || ipatokenOwner || ipatokenProtected")(version 3.0;
acl "Users can read basic token info"; allow (read, search, compare)
userattr = "ipatokenOwner#USERDN"; allow (read, search, compare)
userattr = "managedBy#USERDN";)
aci: (targetfilter = "(objectClass=ipatokenTOTP)")(targetattrs =
"ipatokenOTPalgorithm || ipatokenOTPdigits ||
ipatokenTOTPtimeStep")(version 3.0; acl "Users can see TOTP details";
allow (read, search, compare) userattr = "ipatokenOwner#USERDN"; allow
(read, search, compare) userattr = "managedBy#USERDN";)
aci: (targetfilter = "(objectClass=ipatokenHOTP)")(targetattrs =
"ipatokenOTPalgorithm || ipatokenOTPdigits")(version 3.0; acl "Users can
see HOTP details"; allow (read, search, compare) userattr =
"ipatokenOwner#USERDN"; allow (read, search, compare) userattr =
"managedBy#USERDN";)
aci: (targetfilter = "(objectClass=ipaToken)")(targetattrs =
"description || ipatokenDisabled || ipatokenNotBefore ||
ipatokenNotAfter || ipatokenVendor || ipatokenModel ||
ipatokenSerial")(version 3.0; acl "Managers can write basic token info";
allow (write) userattr = "managedBy#USERDN";)
aci: (targetfilter = "(objectClass=ipaToken)")(version 3.0; acl
"Managers can delete tokens"; allow (delete) userattr =
"managedBy#USERDN";)
aci: (target = "ldap:///ipatokenuniqueid=*,cn=otp,$SUFFIX";)(targetfilter
= "(objectClass=ipaToken)")(version 3.0; acl "Users can create
self-managed tokens"; allow (add) userattr = "ipatokenOwner#SELFDN" and
userattr = "managedBy#SELFDN";)
In short:
1. Owner and manager get read, search and compare.
2. Manager gets write (to select attributes) and delete.
3. Users can create self-managed tokens for themselves only.
The otptoken-add command should gain the following defaults:
1. The owner defaults to the user adding the token.
2. If owner == user adding token, managedBy defaults to owner.
3. Otherwise, managedBy defaults to None.
This means that if neither owner nor managedBy are specified, the
default is a self-owned, self-managed token. Likewise, if a different
owner is specified, no manager is assumed.
rcrit expresses worry that ipalib's ACI parser may not handle the above
syntax. This will become clear during testing if we want this approach.
Does this look sane?
I am not entirely sure your ACI syntax is always right for the second
set. and perhaps we want to duplicate ACIs in some cases (once for owner
once for manager).
I think the read ACIs do not need to list managedby ? Do we plan to have
a manager that is another regular user but not the owner nor an admin ?
In any case I prefer the sytnax that uses managedby, as it has more
potential.
Attached is a new version of the patch which implements the feature
using managedBy instead of ipatokenProtected. One important thing needs
to be said about this patch. I am not exposing managedBy in either the
UI, the CLI or LDAP (ACI). Do we care about this? If yes, should I
expose this similar to owner or as a relationship?
I would expose it, as a relationship. (Note that ipatokenowner should
ideally be represented as a relationship too, but the framework does not
support 1-to-many relationships ATM.)
So since this is a 1-to-many relationship we shouldn't expose it?
Or should I do it like owner is currently done?
Do it like managedby is done in the host plugin (see
"attribute_members", "relationships", etc.)
Just curious, why are the ACIs divided like this:
aci: (targetfilter = "(objectClass=ipaToken)")(targetattrs =
"objectclass || description || ipatokenUniqueID || ipatokenDisabled ||
ipatokenNotBefore || ipatokenNotAfter || ipatokenVendor || ipatokenModel
|| ipatokenSerial || ipatokenOwner")(version 3.0; acl "Users/managers
can read basic token info"; allow (read, search, compare) userattr =
"ipatokenOwner#USERDN" or userattr = "managedBy#USERDN";)
aci: (targetfilter = "(objectClass=ipatokenTOTP)")(targetattrs =
"ipatokenOTPalgorithm || ipatokenOTPdigits ||
ipatokenTOTPtimeStep")(version 3.0; acl "Users/managers can see TOTP
details"; allow (read, search, compare) userattr =
"ipatokenOwner#USERDN" or userattr = "managedBy#USERDN";)
aci: (targetfilter = "(objectClass=ipatokenHOTP)")(targetattrs =
"ipatokenOTPalgorithm || ipatokenOTPdigits")(version 3.0; acl
"Users/managers can see HOTP details"; allow (read, search, compare)
userattr = "ipatokenOwner#USERDN" or userattr = "managedBy#USERDN";)
IMHO you could make them less complex by dividing them like this:
aci: (targetfilter = "(objectClass=ipaToken)")(targetattrs =
"objectclass || description || ipatokenUniqueID || ipatokenDisabled ||
ipatokenNotBefore || ipatokenNotAfter || ipatokenVendor || ipatokenModel
|| ipatokenSerial || ipatokenOwner || ipatokenOTPalgorithm ||
ipatokenOTPdigits || ipatokenTOTPtimeStep")(version 3.0; acl "Owner can
read token details"; allow (read, search, compare) userattr =
"ipatokenOwner#USERDN";)
aci: (targetfilter = "(objectClass=ipaToken)")(targetattrs =
"objectclass || description || ipatokenUniqueID || ipatokenDisabled ||
ipatokenNotBefore || ipatokenNotAfter || ipatokenVendor || ipatokenModel
|| ipatokenSerial || ipatokenOwner || ipatokenOTPalgorithm ||
ipatokenOTPdigits || ipatokenTOTPtimeStep")(version 3.0; acl "Managers
can read token details"; allow (read, search, compare) userattr =
"managedBy#USERDN";)
The first set is organized by objectClass. The second by userattr. I
have no strong opinion on this matter, though performance is probably a
consideration. Do any DS guys want to chime in?
I would still like to know someone else's opinion on this, but if
there's none, let's keep it your way.
Would it make sense to keep --protected as a flag in otptoken-add as a
shortcut for "entry_attrs['managedby'] = None"?
I can't think of a use case for this. The only use case I *can* think of
is an admin creating a non-protected token for a user.
OK.
Would it make sense to default managedby to the current bind DN in
otptoken-add, even if it's not a user DN? (Do we want to allow running
otptoken-add by hosts/services/other non-users?)
No idea. Dmitri?
We can add this later if necessary.
Is orphaning a token when a user is deleted only if it is not managed by
any other users the intended behavior? It just seems sort of strange to
me, since it changes the token from unprotected to protected.
I don't think that is the behavior. We orphan the token if the owner is
not listed as a manager. If the owner is listed as a manager, we delete
the token.
Put another way, protected tokens are orphaned and unprotected tokens
are deleted.
If I didn't implement that, please point out my bug.
Sorry, my bad, your code is right. You can make it simpler, though:
orphan = [x for x in token.get('managedby', []) if x == dn]
(The "len() == 0" check is not necessary and using list comprehensions
makes the code more readable than using filter.)
The attached version fixes all the above issues. Two issues that may
remain:
1. There is no option to set managedBy during otptoken-add or
otptoken-mod. This is probably okay.
Yes. I guess this bit is not needed anymore:
# If owner was not specified, default to the person adding
this token.
- if 'ipatokenowner' not in entry_attrs:
+ # If managedby was not specified, attempt a sensible default.
+ if 'ipatokenowner' not in entry_attrs or 'managedby' not in
entry_attrs:
result = self.api.Command.user_find(whoami=True)['result']
if result:
cur_uid = result[0]['uid'][0]
- entry_attrs.setdefault('ipatokenowner', cur_uid)
+ prev_uid = entry_attrs.setdefault('ipatokenowner', cur_uid)
+ if cur_uid == prev_uid:
+ entry_attrs.setdefault('managedby', result[0]['dn'])
2. I can't figure out how to get the framework to actually show
managedBy in command output (like otptoken-show). This means you can
add/remove relationships using otptoken-add-managedby and
otptoken-remove-managedby, but you can't actually see the list of
managers. What am I missing?
In the hbacrule or selinuxusermap plugins it is done by adding an
"invisible" param to the object plugin, like this:
Str('managedby_user?',
label=_('Manager'),
flags=['no_create', 'no_update', 'no_search'],
),
Also, it would be helpful if someone with DS expertise could answer the
question about the performance of the ACI structure options as listed
above.
+1
Nathaniel
You should update the code in user_del to use managedby_user instead of
managedby, otherwise it won't really work.
--
Jan Cholasta
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
