Re: [Freeipa-devel] [PATCH] 310 Exclude attributelevelrights from --raw result processing in baseldap

2014-07-29 Thread Jan Cholasta

Dne 28.7.2014 v 19:59 Petr Viktorin napsal(a):

On 07/24/2014 05:33 PM, Jan Cholasta wrote:

Hi,

the attached patch fixes https://fedorahosted.org/freeipa/ticket/4371.

Honza



NACK
If the value *is* a str, with this patch it ends up undefined.



Right, fixed.

--
Jan Cholasta
From 803c7cb55a12fba965876bb6a031cf0c7fbfd5c7 Mon Sep 17 00:00:00 2001
From: Jan Cholasta jchol...@redhat.com
Date: Thu, 24 Jul 2014 17:17:48 +0200
Subject: [PATCH] Exclude attributelevelrights from --raw result processing in
 baseldap.

https://fedorahosted.org/freeipa/ticket/4371
---
 ipalib/plugins/baseldap.py | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/ipalib/plugins/baseldap.py b/ipalib/plugins/baseldap.py
index 865a357..26b43b9 100644
--- a/ipalib/plugins/baseldap.py
+++ b/ipalib/plugins/baseldap.py
@@ -240,9 +240,13 @@ def entry_from_entry(entry, newentry):
 def entry_to_dict(entry, **options):
 if options.get('raw', False):
 result = {}
-for attr, value in entry.raw.iteritems():
-if entry.conn.get_type(attr) is not str:
-value = list(value)
+for attr in entry.iterkeys():
+if attr.lower() == 'attributelevelrights':
+value = entry[attr]
+elif entry.conn.get_type(attr) is str:
+value = entry.raw[attr]
+else:
+value = list(entry.raw[attr])
 for (i, v) in enumerate(value):
 try:
 value[i] = v.decode('utf-8')
-- 
1.9.3

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

Re: [Freeipa-devel] LDAP schema for DNSSEC keys

2014-07-29 Thread Jan Cholasta

Dne 28.7.2014 v 11:04 Simo Sorce napsal(a):

On Fri, 2014-07-25 at 19:26 +0200, Petr Spacek wrote:


I have updated design page and diagrams:
https://fedorahosted.org/bind-dyndb-ldap/wiki/BIND9/Design/DNSSEC/Keys/Shortterm#LDAPschema


Excellent page, I took a full read and it all seem reasonable.

However I would like a page like this with the detailed summary of key
material handling.

This is important to get right and have documented anyway so if someone
could summarize in detail all the key handling I would be happy to do a
detailed review and think carefully about the security stance of the
final solution we agreed on. If we can do this early it would be better
to avoid costly rewrites should we have forgotten/underestimated some
implementation detail that requires changes.

Simo.



Do you need more detail than 
https://fedorahosted.org/bind-dyndb-ldap/wiki/BIND9/Design/DNSSEC/Keys/Shortterm#Keydistribution?


--
Jan Cholasta

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


Re: [Freeipa-devel] LDAP schema for DNSSEC keys

2014-07-29 Thread Jan Cholasta

Dne 29.7.2014 v 08:56 Simo Sorce napsal(a):

On Tue, 2014-07-29 at 08:46 +0200, Jan Cholasta wrote:

Dne 28.7.2014 v 11:04 Simo Sorce napsal(a):

On Fri, 2014-07-25 at 19:26 +0200, Petr Spacek wrote:


I have updated design page and diagrams:
https://fedorahosted.org/bind-dyndb-ldap/wiki/BIND9/Design/DNSSEC/Keys/Shortterm#LDAPschema


Excellent page, I took a full read and it all seem reasonable.

However I would like a page like this with the detailed summary of key
material handling.

This is important to get right and have documented anyway so if someone
could summarize in detail all the key handling I would be happy to do a
detailed review and think carefully about the security stance of the
final solution we agreed on. If we can do this early it would be better
to avoid costly rewrites should we have forgotten/underestimated some
implementation detail that requires changes.

Simo.



Do you need more detail than
https://fedorahosted.org/bind-dyndb-ldap/wiki/BIND9/Design/DNSSEC/Keys/Shortterm#Keydistribution?


It's almost there but the wraping/unwrapping steps are a bit handwavy.

I would like more details on algorithms we are going to use and exactly
what parts do the wrapping and unwrapping. For example we say all these
operations happen in SoftHSM at the start, but then the steps that
describe how these keys are inserted into or extracted from SoftHSM are
vague enough they can be interpreted as these operations are being
performed outside of SoftHSM. It should be made much clearer exactly
what component on the system will perform each and any of the key
(un)wrapping operations and with which keys and algorithms.

Simo.



I don't think I'm authorized to edit bind-dyndb-ldap wiki, so I'm going 
to comment the steps from the link above here:



First server installation (replica1)

This operation needs to be detected and guarded so only one replica will become 
DNSSEC master. It can be done by writing who is DNSSEC master to cn=masters.

 1. Generate symmetric master key for DNSSEC


The master key is generated on the token by C_GenerateKey call using the 
CKM_AES_KEY_GEN mechanism, it is 128b AES, it can be used for wrapping 
and unwrapping keys, it can be extracted from the token by wrapping, but 
not in plaintext.




 2. Generate replica key pair:


The replica key pair is generated on the token by C_GenerateKeyPair call 
using the CKM_RSA_PKCS_KEY_PAIR_GEN mechanism, it is 2048b RSA, the 
public part can be used for wrapping keys and can be extracted from the 
token in plaintext, the private part can be used for unwrapping keys and 
can't be extracted from the token.




  * Store public key to LDAP: cn=DNS,cn=replica 
FQDN,cn=masters,cn=ipa,cn=etc,dc=example sub-tree in LDAP.

  * Store private key to SoftHSM (on disk).


This is already achieved above by generating the replica key pair on the 
token.




 3. Wrap master key with replica key:


The master key is wrapped on the token by C_WrapKey using the 
CKM_RSA_PKCS mechanism and the replica public key as the wrapping key.




  * Store resulting blob in cn=master, cn=keys, cn=sec, cn=dns, dc=example 
sub-tree in LDAP.


Replica installation (replica2)

This is done by ipa-replica-install:

 1. Generate replica key pair:


The replica key pair is generated in the same manner as in first server 
installation.




 2. Store public key to LDAP: cn=DNS,cn=replica 
FQDN,cn=masters,cn=ipa,cn=etc,dc=example sub-tree in LDAP.

 3. Store private key in SoftHSM (on disk).


This is already achieved above by generating the replica key pair on the 
token.




This is done by little daemon running on replica 1:

 1. DNSSEC master-replica will detect (SyncRepl) a new public key in cn=masters 
sub-tree and will use own private key (replica1's) to re-encrypt master key 
using public key of the new replica (replica2)


The master key is wrapped in the same manner as in first server 
installation.




 2. Store resulting master-key-blob to cn=master, cn=keys, cn=sec, cn=dns, 
dc=example sub-tree in LDAP.

This is done by little daemon running on replica 2:

 1. Download blob with master key from LDAP.


The master key is rotated when a replica is deleted, so there may be 
more than one master keys in LDAP, but there will be only one which is 
allowed to wrap keys. This particular master key is downloaded in this step.




 2. Unwrap blob with replica2's private key.


The master key is unwrapped on the token by C_UnwrapKey call using 
CKM_RSA_PKCS mechanism with the replica private key as the unwrapping 
key, it can be used for wrapping and unwrapping keys, it can be 
extracted from the token by wrapping, but not in plaintext.




 3. Store raw master key to local SoftHSM.


This is already achieved above by unwrapping the master key on the token.




Replica uninstallation (replica2)

NOTE: replica1 cannot be uninstalled at the moment. Support for this scenario 
requires more work.

NOTE: Old master keys remain in LDAP and old DNSSEC keys are 

Re: [Freeipa-devel] [PATCH] 310 Exclude attributelevelrights from --raw result processing in baseldap

2014-07-29 Thread Petr Viktorin

On 07/29/2014 08:27 AM, Jan Cholasta wrote:

Dne 28.7.2014 v 19:59 Petr Viktorin napsal(a):

On 07/24/2014 05:33 PM, Jan Cholasta wrote:

Hi,

the attached patch fixes https://fedorahosted.org/freeipa/ticket/4371.

Honza



NACK
If the value *is* a str, with this patch it ends up undefined.



Right, fixed.



ACK, pushed to:
master: 785e13dd1e16ad03d4ef03edcb672d6f9d8b457b
ipa-4-1: 785e13dd1e16ad03d4ef03edcb672d6f9d8b457b
ipa-4-0: 2eee6060f3237cc4cf23f4044e67e95d7fad195d

Could you also write a test for this, though?


--
Petr³

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


Re: [Freeipa-devel] [PATCH] 710 webui: review pending operation after expired session

2014-07-29 Thread Petr Vobornik

On 28.7.2014 19:01, Endi Sukma Dewata wrote:

On 7/28/2014 6:06 AM, Petr Vobornik wrote:

Right now suppose I'm trying to delete a user, I have the delete dialog
open and I let it sit until the session expires, then when I click
Delete it will show me a login screen. Once I re-login, the dialog box
is gone. It still has the user to be deleted selected, but there's no
indication what the operation I was trying to do before.

I was thinking the session expiration would work like desktop
screensaver lock. So when I re-login I would see same screen as I left
it, i.e. the delete dialog is still waiting for action.


Components have not been made with this feature in mind. Take for
example the delete issue. Deleter dialog is a subclass of confirm
dialog. Confirm dialog is closed right after confirmation/cancellation.
It doesn't wait for the result of the operation because it's handled by
other components. The behavior was OK so far because we showed error
dialog on normal error. On auth error, the command was re-executed.
Now we don't show any error on auth error nor we leave the dialog open.
Seems like that we should change all usages of confirm dialog to try to
do the operation first and close the dialog after the operation was
successful or canceled.





Not sure about that. If you're adding a user that already exists, you'll
get an error dialog, but then if you click Cancel you'll go back to the
same add dialog.


Yes because the adder dialog remained opened (it was hidden while the 
error dialog was open because only one dialog can be visible at the same 
time).



That allows you to revise the info you entered. Can we
use the same concept for session expiration? So instead of an error
dialog you'll get a login screen. If you relogin as the same person,
you'll go back to the same dialog with whatever info you already entered.


I think we are in agreement. By the previous text I wanted to point out 
that not every component behaves as adder dialogs and that they should 
be identified and fixed. I.e. it's not just about session expiration.




This is a low priority regardless.




--
Petr Vobornik

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


Re: [Freeipa-devel] [PATCH] 715 webui: add bounce url to reset_password.html

2014-07-29 Thread Petr Vobornik

On 28.7.2014 19:08, Endi Sukma Dewata wrote:

On 7/28/2014 3:58 AM, Petr Vobornik wrote:

Just one thing, there is no pause between clicking the Reset button
and the redirection, so the Password reset was successful.
confirmation message might only appear very briefly. A possible
alternative is to show a confirmation page/message, but the user will
have to click to continue to the next page.


I don't believe there is a universal solution. I would say that it
depends on personal preferences and a use case. I.e., if it's part of  a
login procedure I would prefer immediate redirection back to login page.
If it's invoked from a user action - just to change the password, some
delay might be good.

We might add a URL param(s) to configure the delay/link.


How about 2 URL params?
1. the link to the next page
2. an option whether to
a) redirect to the link immediately
b) show a confirmation page with the link

Just my preference, but I don't really like a 'delay' on a web page.
It's either too short or too long, and we can't put important info
during the delay because there's no guarantee people will see it.



Yes, how about this:

1. redirection=url_to_the_page
2. in=x, where x is number of seconds or a string

- redirect immediately if only `redirection` is supplied or `in=0`
- show Continue to anext page/a link if `in` is present
- show count-down timer if `in` is  1 with You will be redirected in 
`x`s text.
- don't redirect if `in` is negative or NaN (your scenario), e.g.: 
`in=no` up to user

- ignore `in` if `redirection` is not present

Then we will support all described scenarios and the decision will be on 
web-site admin. Feel free to propose better name for `in`.

--
Petr Vobornik

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


Re: [Freeipa-devel] [PATCH] 0007 test group: remove group from protected group

2014-07-29 Thread David Kupka

On 07/28/2014 06:41 PM, Petr Viktorin wrote:

On 07/24/2014 03:11 PM, David Kupka wrote:

Simple test scenario from ticket #4448.

Last test will fail until patch freeipa-dkupka-0006 gets accepted.



Thanks! These look fine, but since the new tests don't require that the
rest of `test_group` is run first, I encourage you to put them in a
separate class.

Put to separate class, as suggested. Looks better now.


This would ensure we don't add new inderdependencies between old and new
tests in the future, making future test refactoring more straightforward.
Also, you can select to run just a single test class from a module, so
testing a targeted fix is faster.
(And you can reuse group1, since the other test cleans it up)

See test_permission_plugin for an example.

The test still fails on current master but works fine with patch 
freeipa-dkupka-0006-2.


--
David Kupka
From 9e3c1bc8a83fc466a6e160842ab37c8e65e66229 Mon Sep 17 00:00:00 2001
From: David Kupka dku...@redhat.com
Date: Tue, 29 Jul 2014 08:40:36 +0200
Subject: [PATCH] test group: remove group from protected group.

Related to https://fedorahosted.org/freeipa/ticket/4448
---
 ipatests/test_xmlrpc/test_group_plugin.py | 67 +++
 1 file changed, 67 insertions(+)

diff --git a/ipatests/test_xmlrpc/test_group_plugin.py b/ipatests/test_xmlrpc/test_group_plugin.py
index 71172893beb97d3373b0b9299f2bfa7bb642dba6..26d71c4fae1a312709f6f27e0397ef3315cba33f 100644
--- a/ipatests/test_xmlrpc/test_group_plugin.py
+++ b/ipatests/test_xmlrpc/test_group_plugin.py
@@ -1009,5 +1009,72 @@ class test_group(Declarative):
 value=[user1],
 ),
 ),
+]
+
+class test_group_remove_group_from_protected_group(Declarative):
+cleanup_commands = [
+('group_del', [group1], {}),
+]
+tests = [
+# Test scenario from ticket #4448
+# https://fedorahosted.org/freeipa/ticket/4448
+dict(
+desc='Add group %s' % group1,
+command=('group_add', [group1], dict(description=u'Test desc 1')),
+expected=dict(
+value=group1,
+summary=u'Added group %s' % group1,
+result=dict(
+cn=[group1],
+description=[u'Test desc 1'],
+objectclass=objectclasses.posixgroup,
+gidnumber=[fuzzy_digits],
+ipauniqueid=[fuzzy_uuid],
+dn=get_group_dn(group1),
+),
+),
+),
+
+dict(
+desc='Add %s group to admins group' % group1,
+command=('group_add_member', [u'admins'], dict(group=group1)),
+expected=dict(
+completed=1,
+failed=dict(
+member=dict(
+group=tuple(),
+user=tuple(),
+),
+),
+result=dict(
+dn=get_group_dn('admins'),
+member_user=[u'admin'],
+member_group=[group1],
+gidnumber=[fuzzy_digits],
+cn=[u'admins'],
+description=[u'Account administrators group'],
+),
+),
+),
 
+dict(
+desc='Remove %s group from admins group' % group1,
+command=('group_remove_member', [u'admins'], dict(group=group1)),
+expected=dict(
+completed=1,
+failed=dict(
+member=dict(
+group=tuple(),
+user=tuple(),
+),
+),
+result=dict(
+dn=get_group_dn(u'admins'),
+cn=[u'admins'],
+gidnumber=[fuzzy_digits],
+member_user=[u'admin'],
+description=[u'Account administrators group'],
+),
+),
+),
 ]
-- 
1.9.3

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

Re: [Freeipa-devel] [PATCH] 0006 Fix group-remove-member crash when group is removed from a protected group

2014-07-29 Thread Martin Kosek
On 07/23/2014 04:32 PM, David Kupka wrote:
 On 07/23/2014 04:15 PM, Martin Kosek wrote:
 On 07/23/2014 04:08 PM, David Kupka wrote:
 https://fedorahosted.org/freeipa/ticket/4448

 Alternatively, we could also update the if condition to avoid running this
 section at all when options['user'] does not exist or is empty. This would 
 save
 us at least from api.Command.group_show call.

 Martin

 You're right as always, Martin :-)

Works fine, ACK.

Pushed to:
master: 6119c21441747a1f2dd49df204effe1f2a3240dc
ipa-4-1: 6119c21441747a1f2dd49df204effe1f2a3240dc
ipa-4-0: 97565cf8ffa8741dac3629e8989d747faa24ddf0

I will not close the ticket until the test that Petr looks on is finished.

Martin

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


Re: [Freeipa-devel] [PATCH] 0005 Verify otptoken timespan is valid

2014-07-29 Thread Jan Cholasta

Dne 24.7.2014 v 10:00 David Kupka napsal(a):



On 07/23/2014 05:07 PM, Jan Cholasta wrote:

Hi,

On 23.7.2014 15:46, David Kupka wrote:

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


1) Use isinstance(X, Y) instead of type(X) is Y.


Thanks for advice, will try to remember.


2) When is type(not_before) is str or type(not_after) is str true?
The values coming from command options or LDAP should always be
datetime, never str.


Actually, it is never true. I don't  know why I thought that there is
such option.


3) There are some misindentations:

+raise ValidationError(name='not_after',
+error='is before not_before!')

+raise ValidationError(name='not_after',
+error='is before not_before!')

+raise ValidationError(name='not_before',
+error='is after not_after!')

4) We don't do exclamation marks in errors messages.


You re right, it's probably better not to shout at customer :)


5) Generally, when you want to validate command options, you should look
into options, not entry_attrs.

6) This is not right:

+result = self.api.Command.otptoken_find(ipatokenuniqueid=
+entry_attrs.get('ipatokenuniqueid', None))['result']

This is:

+result = self.api.Command.otptoken_show(keys[-1])['result']


Both works, but Martin explained me why is otptoken_show better and how
it actually works.


Honza





Few more nitpicks:

1) You can make _check_interval a little bit shorter by removing a 
redundant if statement:


+def _check_interval(not_before, not_after):
+if not_before and not_after:
+return not_before = not_after
+return True

2) Please keep the 2 line space between the last global function and 
first class in the module.


3) Error messages are for users' eyes, please don't use internal 
identifiers in them.


4) You don't need to check if the result of otptoken_show is empty, it 
never is.


--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH] 0005 Verify otptoken timespan is valid

2014-07-29 Thread David Kupka

On 07/29/2014 01:21 PM, Jan Cholasta wrote:

Dne 24.7.2014 v 10:00 David Kupka napsal(a):



On 07/23/2014 05:07 PM, Jan Cholasta wrote:

Hi,

On 23.7.2014 15:46, David Kupka wrote:

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


1) Use isinstance(X, Y) instead of type(X) is Y.


Thanks for advice, will try to remember.


2) When is type(not_before) is str or type(not_after) is str true?
The values coming from command options or LDAP should always be
datetime, never str.


Actually, it is never true. I don't  know why I thought that there is
such option.


3) There are some misindentations:

+raise ValidationError(name='not_after',
+error='is before not_before!')

+raise ValidationError(name='not_after',
+error='is before not_before!')

+raise ValidationError(name='not_before',
+error='is after not_after!')

4) We don't do exclamation marks in errors messages.


You re right, it's probably better not to shout at customer :)


5) Generally, when you want to validate command options, you should look
into options, not entry_attrs.

6) This is not right:

+result = self.api.Command.otptoken_find(ipatokenuniqueid=
+entry_attrs.get('ipatokenuniqueid', None))['result']

This is:

+result = self.api.Command.otptoken_show(keys[-1])['result']


Both works, but Martin explained me why is otptoken_show better and how
it actually works.


Honza





Few more nitpicks:

1) You can make _check_interval a little bit shorter by removing a
redundant if statement:

+def _check_interval(not_before, not_after):
+if not_before and not_after:
+return not_before = not_after
+return True

Nice :)


2) Please keep the 2 line space between the last global function and
first class in the module.

It's good to know that there is one less rule to discover.


3) Error messages are for users' eyes, please don't use internal
identifiers in them.

Done.


4) You don't need to check if the result of otptoken_show is empty, it
never is.


Removed. Although, needless check can't hurt.

--
David Kupka
From 64aff33276a2e929806fac296830b6bdf5b6f621 Mon Sep 17 00:00:00 2001
From: David Kupka dku...@redhat.com
Date: Tue, 29 Jul 2014 13:58:33 +0200
Subject: [PATCH] Verify otptoken timespan is valid

When creating or modifying otptoken check that token validity start is not after
validity end.

https://fedorahosted.org/freeipa/ticket/4244
---
 ipalib/plugins/otptoken.py | 31 ++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/ipalib/plugins/otptoken.py b/ipalib/plugins/otptoken.py
index 2880ee660d5dcdb18c504f50d7b72f5b8fb43d48..aea9891c7e2c3662aa22eb32cc2fadbc78687faa 100644
--- a/ipalib/plugins/otptoken.py
+++ b/ipalib/plugins/otptoken.py
@@ -21,7 +21,7 @@ from ipalib.plugins.baseldap import DN, LDAPObject, LDAPAddMember, LDAPRemoveMem
 from ipalib.plugins.baseldap import LDAPCreate, LDAPDelete, LDAPUpdate, LDAPSearch, LDAPRetrieve
 from ipalib import api, Int, Str, Bool, DateTime, Flag, Bytes, IntEnum, StrEnum, Password, _, ngettext
 from ipalib.plugable import Registry
-from ipalib.errors import PasswordMismatch, ConversionError, LastMemberError, NotFound
+from ipalib.errors import PasswordMismatch, ConversionError, LastMemberError, NotFound, ValidationError
 from ipalib.request import context
 from ipalib.frontend import Local
 
@@ -103,6 +103,11 @@ def _normalize_owner(userobj, entry_attrs):
 if owner is not None:
 entry_attrs['ipatokenowner'] = userobj.get_dn(owner)
 
+def _check_interval(not_before, not_after):
+if not_before and not_after:
+return not_before = not_after
+return True
+
 
 @register()
 class otptoken(LDAPObject):
@@ -254,6 +259,11 @@ class otptoken_add(LDAPCreate):
 entry_attrs['ipatokenuniqueid'] = str(uuid.uuid4())
 dn = DN(ipatokenuniqueid=%s % entry_attrs['ipatokenuniqueid'], dn)
 
+if not _check_interval(options.get('ipatokennotbefore', None),
+   options.get('ipatokennotafter', None)):
+raise ValidationError(name='not_after',
+  error='is before the validity start')
+
 # Set the object class and defaults for specific token types
 entry_attrs['objectclass'] = otptoken.object_class + ['ipatoken' + options['type']]
 for ttype, tattrs in TOKEN_TYPES.items():
@@ -336,6 +346,25 @@ class otptoken_mod(LDAPUpdate):
 msg_summary = _('Modified OTP token %(value)s')
 
 def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys, **options):
+notafter_set = True
+notbefore = options.get('ipatokennotbefore', None)
+notafter = options.get('ipatokennotafter', None)
+# notbefore xor notafter, exactly one of them is not None
+if bool(notbefore) ^ bool(notafter):
+result = 

Re: [Freeipa-devel] [PATCH] 0005 Verify otptoken timespan is valid

2014-07-29 Thread Jan Cholasta

Dne 29.7.2014 v 14:11 David Kupka napsal(a):

On 07/29/2014 01:21 PM, Jan Cholasta wrote:

Dne 24.7.2014 v 10:00 David Kupka napsal(a):



On 07/23/2014 05:07 PM, Jan Cholasta wrote:

Hi,

On 23.7.2014 15:46, David Kupka wrote:

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


1) Use isinstance(X, Y) instead of type(X) is Y.


Thanks for advice, will try to remember.


2) When is type(not_before) is str or type(not_after) is str true?
The values coming from command options or LDAP should always be
datetime, never str.


Actually, it is never true. I don't  know why I thought that there is
such option.


3) There are some misindentations:

+raise ValidationError(name='not_after',
+error='is before not_before!')

+raise ValidationError(name='not_after',
+error='is before not_before!')

+raise ValidationError(name='not_before',
+error='is after not_after!')

4) We don't do exclamation marks in errors messages.


You re right, it's probably better not to shout at customer :)


5) Generally, when you want to validate command options, you should
look
into options, not entry_attrs.

6) This is not right:

+result = self.api.Command.otptoken_find(ipatokenuniqueid=
+entry_attrs.get('ipatokenuniqueid', None))['result']

This is:

+result =
self.api.Command.otptoken_show(keys[-1])['result']


Both works, but Martin explained me why is otptoken_show better and how
it actually works.


Honza





Few more nitpicks:

1) You can make _check_interval a little bit shorter by removing a
redundant if statement:

+def _check_interval(not_before, not_after):
+if not_before and not_after:
+return not_before = not_after
+return True

Nice :)


2) Please keep the 2 line space between the last global function and
first class in the module.

It's good to know that there is one less rule to discover.


3) Error messages are for users' eyes, please don't use internal
identifiers in them.

Done.


4) You don't need to check if the result of otptoken_show is empty, it
never is.


Removed. Although, needless check can't hurt.



One more thing, sorry I didn't notice it earlier: You must check if 
'ipatokennotbefore' and 'ipatokennotafter' keys are in opttoken-show 
result before using them, otherwise you might end up with:


ipa: ERROR: non-public: KeyError: 'ipatokennotbefore'
Traceback (most recent call last):
  File /usr/lib/python2.7/site-packages/ipaserver/rpcserver.py, 
line 348, in wsgi_execute

result = self.Command[name](*args, **options)
  File /usr/lib/python2.7/site-packages/ipalib/frontend.py, line 
439, in __call__

ret = self.run(*args, **options)
  File /usr/lib/python2.7/site-packages/ipalib/frontend.py, line 
754, in run

return self.execute(*args, **options)
  File 
/usr/lib/python2.7/site-packages/ipalib/plugins/baseldap.py, line 
1384, in execute

*keys, **options)
  File 
/usr/lib/python2.7/site-packages/ipalib/plugins/otptoken.py, line 356, 
in pre_callback

notbefore = result['ipatokennotbefore'][0]
KeyError: 'ipatokennotbefore'

--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCH] 0005 Verify otptoken timespan is valid

2014-07-29 Thread David Kupka

On 07/29/2014 03:28 PM, Jan Cholasta wrote:

Dne 29.7.2014 v 14:11 David Kupka napsal(a):

On 07/29/2014 01:21 PM, Jan Cholasta wrote:

Dne 24.7.2014 v 10:00 David Kupka napsal(a):



On 07/23/2014 05:07 PM, Jan Cholasta wrote:

Hi,

On 23.7.2014 15:46, David Kupka wrote:

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


1) Use isinstance(X, Y) instead of type(X) is Y.


Thanks for advice, will try to remember.


2) When is type(not_before) is str or type(not_after) is str true?
The values coming from command options or LDAP should always be
datetime, never str.


Actually, it is never true. I don't  know why I thought that there is
such option.


3) There are some misindentations:

+raise ValidationError(name='not_after',
+error='is before not_before!')

+raise ValidationError(name='not_after',
+error='is before not_before!')

+raise ValidationError(name='not_before',
+error='is after not_after!')

4) We don't do exclamation marks in errors messages.


You re right, it's probably better not to shout at customer :)


5) Generally, when you want to validate command options, you should
look
into options, not entry_attrs.

6) This is not right:

+result = self.api.Command.otptoken_find(ipatokenuniqueid=
+entry_attrs.get('ipatokenuniqueid', None))['result']

This is:

+result =
self.api.Command.otptoken_show(keys[-1])['result']


Both works, but Martin explained me why is otptoken_show better and how
it actually works.


Honza





Few more nitpicks:

1) You can make _check_interval a little bit shorter by removing a
redundant if statement:

+def _check_interval(not_before, not_after):
+if not_before and not_after:
+return not_before = not_after
+return True

Nice :)


2) Please keep the 2 line space between the last global function and
first class in the module.

It's good to know that there is one less rule to discover.


3) Error messages are for users' eyes, please don't use internal
identifiers in them.

Done.


4) You don't need to check if the result of otptoken_show is empty, it
never is.


Removed. Although, needless check can't hurt.



One more thing, sorry I didn't notice it earlier: You must check if
'ipatokennotbefore' and 'ipatokennotafter' keys are in opttoken-show
result before using them, otherwise you might end up with:

 ipa: ERROR: non-public: KeyError: 'ipatokennotbefore'
 Traceback (most recent call last):
   File /usr/lib/python2.7/site-packages/ipaserver/rpcserver.py,
line 348, in wsgi_execute
 result = self.Command[name](*args, **options)
   File /usr/lib/python2.7/site-packages/ipalib/frontend.py, line
439, in __call__
 ret = self.run(*args, **options)
   File /usr/lib/python2.7/site-packages/ipalib/frontend.py, line
754, in run
 return self.execute(*args, **options)
   File
/usr/lib/python2.7/site-packages/ipalib/plugins/baseldap.py, line
1384, in execute
 *keys, **options)
   File
/usr/lib/python2.7/site-packages/ipalib/plugins/otptoken.py, line 356,
in pre_callback
 notbefore = result['ipatokennotbefore'][0]
 KeyError: 'ipatokennotbefore'

Sorry for sending fifth wersion of the patch. I must test my patches 
more carefully.


--
David Kupka
From 0458522dff5be7cb8b7718d379d5cfb3e32c7b33 Mon Sep 17 00:00:00 2001
From: David Kupka dku...@redhat.com
Date: Tue, 29 Jul 2014 15:45:21 +0200
Subject: [PATCH] Verify otptoken timespan is valid

When creating or modifying otptoken check that token validity start is not after
validity end.

https://fedorahosted.org/freeipa/ticket/4244
---
 ipalib/plugins/otptoken.py | 31 ++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/ipalib/plugins/otptoken.py b/ipalib/plugins/otptoken.py
index 2880ee660d5dcdb18c504f50d7b72f5b8fb43d48..dfd010e7f8663b424d9c536907fcc93229181fa3 100644
--- a/ipalib/plugins/otptoken.py
+++ b/ipalib/plugins/otptoken.py
@@ -21,7 +21,7 @@ from ipalib.plugins.baseldap import DN, LDAPObject, LDAPAddMember, LDAPRemoveMem
 from ipalib.plugins.baseldap import LDAPCreate, LDAPDelete, LDAPUpdate, LDAPSearch, LDAPRetrieve
 from ipalib import api, Int, Str, Bool, DateTime, Flag, Bytes, IntEnum, StrEnum, Password, _, ngettext
 from ipalib.plugable import Registry
-from ipalib.errors import PasswordMismatch, ConversionError, LastMemberError, NotFound
+from ipalib.errors import PasswordMismatch, ConversionError, LastMemberError, NotFound, ValidationError
 from ipalib.request import context
 from ipalib.frontend import Local
 
@@ -103,6 +103,11 @@ def _normalize_owner(userobj, entry_attrs):
 if owner is not None:
 entry_attrs['ipatokenowner'] = userobj.get_dn(owner)
 
+def _check_interval(not_before, not_after):
+if not_before and not_after:
+return not_before = not_after
+return True
+
 
 

Re: [Freeipa-devel] [PATCH] 0005 Verify otptoken timespan is valid

2014-07-29 Thread Jan Cholasta

Dne 29.7.2014 v 15:52 David Kupka napsal(a):

On 07/29/2014 03:28 PM, Jan Cholasta wrote:

Dne 29.7.2014 v 14:11 David Kupka napsal(a):

On 07/29/2014 01:21 PM, Jan Cholasta wrote:

Dne 24.7.2014 v 10:00 David Kupka napsal(a):



On 07/23/2014 05:07 PM, Jan Cholasta wrote:

Hi,

On 23.7.2014 15:46, David Kupka wrote:

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


1) Use isinstance(X, Y) instead of type(X) is Y.


Thanks for advice, will try to remember.


2) When is type(not_before) is str or type(not_after) is str
true?
The values coming from command options or LDAP should always be
datetime, never str.


Actually, it is never true. I don't  know why I thought that there is
such option.


3) There are some misindentations:

+raise ValidationError(name='not_after',
+error='is before not_before!')

+raise ValidationError(name='not_after',
+error='is before not_before!')

+raise ValidationError(name='not_before',
+error='is after not_after!')

4) We don't do exclamation marks in errors messages.


You re right, it's probably better not to shout at customer :)


5) Generally, when you want to validate command options, you should
look
into options, not entry_attrs.

6) This is not right:

+result =
self.api.Command.otptoken_find(ipatokenuniqueid=
+entry_attrs.get('ipatokenuniqueid', None))['result']

This is:

+result =
self.api.Command.otptoken_show(keys[-1])['result']


Both works, but Martin explained me why is otptoken_show better and
how
it actually works.


Honza





Few more nitpicks:

1) You can make _check_interval a little bit shorter by removing a
redundant if statement:

+def _check_interval(not_before, not_after):
+if not_before and not_after:
+return not_before = not_after
+return True

Nice :)


2) Please keep the 2 line space between the last global function and
first class in the module.

It's good to know that there is one less rule to discover.


3) Error messages are for users' eyes, please don't use internal
identifiers in them.

Done.


4) You don't need to check if the result of otptoken_show is empty, it
never is.


Removed. Although, needless check can't hurt.



One more thing, sorry I didn't notice it earlier: You must check if
'ipatokennotbefore' and 'ipatokennotafter' keys are in opttoken-show
result before using them, otherwise you might end up with:

 ipa: ERROR: non-public: KeyError: 'ipatokennotbefore'
 Traceback (most recent call last):
   File /usr/lib/python2.7/site-packages/ipaserver/rpcserver.py,
line 348, in wsgi_execute
 result = self.Command[name](*args, **options)
   File /usr/lib/python2.7/site-packages/ipalib/frontend.py, line
439, in __call__
 ret = self.run(*args, **options)
   File /usr/lib/python2.7/site-packages/ipalib/frontend.py, line
754, in run
 return self.execute(*args, **options)
   File
/usr/lib/python2.7/site-packages/ipalib/plugins/baseldap.py, line
1384, in execute
 *keys, **options)
   File
/usr/lib/python2.7/site-packages/ipalib/plugins/otptoken.py, line 356,
in pre_callback
 notbefore = result['ipatokennotbefore'][0]
 KeyError: 'ipatokennotbefore'


Sorry for sending fifth wersion of the patch. I must test my patches
more carefully.



No problem. I must review more carefully. ACK.

--
Jan Cholasta

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


Re: [Freeipa-devel] [PATCHES] 295-299 Allow changing chaining of the IPA CA certificate

2014-07-29 Thread Rob Crittenden
Jan Cholasta wrote:
 Dne 28.7.2014 v 21:39 Rob Crittenden napsal(a):
 This is oh-so close. AFAICT it generally does what it should, I think it
 is ready for a wider audience. Just a few more things:

 306: A while True loop is used for something which AFAICT can only ever
 execute once. I'd think something like this is more readable:

 for ca_nick, ca_flags in db.list_certs():
  if db.has_nickname(ca_cert):
  try:
  db.delete_cert(ca_nick)
  except ipautil.CalledProcessError:
  syslog.syslog(
  syslog.LOG_ERR,
  Failed to remove certificate %s % ca_nick)
 
 Actually the while loop is necessary, because certutil -D (and in turn
 CertDB.delete_cert) deletes just a single cert with the nickname, but
 there may be more versions of it and we need to delete them all.

Aha, excellent point. This would be a great comment in the code!


 +1 on the additional syslogs. It will help figure out what's going on if
 things go sideways.

 Otherwise things seem to be working. I think that fixing the above is
 enough for a +57 with the promise of unit tests to back up some of these
 new functions.
 
 I'm working on that.
 

 rob
 rob

 
 I have made a slight adjustment to patch 246 because of
 https://fedorahosted.org/freeipa/ticket/4039, see
 http://www.redhat.com/archives/freeipa-devel/2014-July/msg00369.html.
 
 Updated rebased patches attached.
 
 (once again, the correct order to apply them is 241-253, 262-294,
 303-305, 295-299, 306-307)
 

ACK on 246.

IMHO this is ready to be pushed if you can add the comment on 306.

A slight rebase on 251 is needed for freeipa.spec.in.

rob

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


Re: [Freeipa-devel] [PATCHES] 295-299 Allow changing chaining of the IPA CA certificate

2014-07-29 Thread Rob Crittenden
Rob Crittenden wrote:
 Jan Cholasta wrote:
 Dne 28.7.2014 v 21:39 Rob Crittenden napsal(a):
 This is oh-so close. AFAICT it generally does what it should, I think it
 is ready for a wider audience. Just a few more things:

 306: A while True loop is used for something which AFAICT can only ever
 execute once. I'd think something like this is more readable:

 for ca_nick, ca_flags in db.list_certs():
  if db.has_nickname(ca_cert):
  try:
  db.delete_cert(ca_nick)
  except ipautil.CalledProcessError:
  syslog.syslog(
  syslog.LOG_ERR,
  Failed to remove certificate %s % ca_nick)

 Actually the while loop is necessary, because certutil -D (and in turn
 CertDB.delete_cert) deletes just a single cert with the nickname, but
 there may be more versions of it and we need to delete them all.
 
 Aha, excellent point. This would be a great comment in the code!
 

 +1 on the additional syslogs. It will help figure out what's going on if
 things go sideways.

 Otherwise things seem to be working. I think that fixing the above is
 enough for a +57 with the promise of unit tests to back up some of these
 new functions.

 I'm working on that.


 rob
 rob


 I have made a slight adjustment to patch 246 because of
 https://fedorahosted.org/freeipa/ticket/4039, see
 http://www.redhat.com/archives/freeipa-devel/2014-July/msg00369.html.

 Updated rebased patches attached.

 (once again, the correct order to apply them is 241-253, 262-294,
 303-305, 295-299, 306-307)

 
 ACK on 246.
 
 IMHO this is ready to be pushed if you can add the comment on 306.
 
 A slight rebase on 251 is needed for freeipa.spec.in.

Oh, or maybe not since you sent the whole shebang. I just did an
interdiff of the current and old 246.

rob

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


[Freeipa-devel] [PATCHES] 0264-0267 backup, restore: Don't overwrite /etc/{passwd, group}

2014-07-29 Thread Petr Viktorin

Hello,

The first patch here consolidates our system user creation code a bit.

The second patch fixes an oversight in the restore script.

The third changes the backup script to not include /etc/{passwd,group}, 
and the restore script to create the PKI user if a CA is being restored. 
Note that the DS user is already created early in the restore process.
(In the future we may want a nice generic framework for restoring users, 
but I'd like to extrapolate from more than one data point when designing 
it.)


The fourth patch adds a log entry I find very useful in testing 
backup/restore.


--
Petr³
From 0f123ec9441b0189d334061865d0937ebe68a0ed Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Tue, 15 Jul 2014 13:31:01 +0200
Subject: [PATCH] ipaserver.install: Consolidate system user creation

Sytem users and their groups are always created together.
Also, users  groups should never be removed once they exist
on the system (see comit a5a55ce).

Use a single function for generic user creation, and specific
funtions in dsinstance and cainstance.
Remove code left over from when we used to delete the DS user.

Preparation for: https://fedorahosted.org/freeipa/ticket/3866
---
 install/tools/ipa-replica-install |  5 ++--
 install/tools/ipa-server-install  |  7 +++---
 ipaserver/install/cainstance.py   | 28 +
 ipaserver/install/dsinstance.py   | 51 ++-
 ipaserver/install/installutils.py | 44 -
 ipaserver/install/ipa_restore.py  |  3 +--
 6 files changed, 68 insertions(+), 70 deletions(-)

diff --git a/install/tools/ipa-replica-install b/install/tools/ipa-replica-install
index 5bfd61ee69d4682823a57f4b99a0d9a054a56d22..a15642e5a3496ed49c7afb424c8d9704db0239ee 100755
--- a/install/tools/ipa-replica-install
+++ b/install/tools/ipa-replica-install
@@ -584,9 +584,8 @@ def main():
 api.bootstrap(in_server=True, context='installer')
 api.finalize()
 
-# Create DS group if it doesn't exist yet
-group_exists = dsinstance.create_ds_group()
-sstore.backup_state(install, group_exists, group_exists)
+# Create DS user/group if it doesn't exist yet
+dsinstance.create_ds_user()
 
 #Automatically disable pkinit w/ dogtag until that is supported
 options.setup_pkinit = False
diff --git a/install/tools/ipa-server-install b/install/tools/ipa-server-install
index da600413271cdf711767fb2d94b23f083bdeb1c3..e2e3c60eb3a80dd13687ae0a829caaeae2a1967e 100755
--- a/install/tools/ipa-server-install
+++ b/install/tools/ipa-server-install
@@ -551,7 +551,8 @@ def uninstall():
 
 ipaclient.ntpconf.restore_forced_ntpd(sstore)
 
-group_exists = sstore.restore_state(install, group_exists)
+# Clean up group_exists (unused since IPA 2.2, not being set since 4.1)
+sstore.restore_state(install, group_exists)
 
 services.knownservices.ipa.disable()
 
@@ -1079,8 +1080,8 @@ def main():
 # configure /etc/sysconfig/network to contain the custom hostname
 tasks.backup_and_replace_hostname(fstore, sstore, host_name)
 
-# Create DS group if it doesn't exist yet
-dsinstance.create_ds_group()
+# Create DS user/group if it doesn't exist yet
+dsinstance.create_ds_user()
 
 # Create a directory server instance
 if external != 2:
diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index 03aec95710d19b0f6cdc8eb6185ab0e832b28031..ce1c360acd7d453575f88f8b1ba1df06969d66dc 100644
--- a/ipaserver/install/cainstance.py
+++ b/ipaserver/install/cainstance.py
@@ -251,6 +251,16 @@ def is_step_one_done():
 return False
 
 
+def create_ca_user():
+Create PKI user/group if it doesn't exist yet.
+installutils.create_system_user(
+name=PKI_USER,
+group=PKI_USER,
+homedir=paths.VAR_LIB,
+shell=paths.NOLOGIN,
+)
+
+
 class CADSInstance(service.Service):
 Certificate Authority DS instance
 
@@ -441,7 +451,7 @@ def configure_instance(self, host_name, domain, dm_password,
 self.cert_chain_file=cert_chain_file
 self.external=2
 
-self.step(creating certificate server user, self.__create_ca_user)
+self.step(creating certificate server user, create_ca_user)
 if self.dogtag_constants.DOGTAG_VERSION = 10:
 self.step(configuring certificate server instance, self.__spawn_instance)
 else:
@@ -658,22 +668,6 @@ def __enable(self):
 # We need to install DS before we can actually ldap_enable a service.
 # so actual enablement is delayed.
 
-def __create_ca_user(self):
-try:
-pwd.getpwnam(PKI_USER)
-root_logger.debug(ca user %s exists % PKI_USER)
-except KeyError:
-root_logger.debug(adding ca user %s % PKI_USER)
-args = [paths.USERADD, -c, CA System User,
- -d, paths.VAR_LIB,
- -s, 

[Freeipa-devel] [PATCH] 480 Do not crash client basedn discovery when SSF not met

2014-07-29 Thread Martin Kosek
ipa-client-install runs anonymous search in non-rootdse space which
may raise UNWILLING_TO_PERFORM error. This case was only covered for
BIND, but not for the actual LDAP queries.

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

-- 
Martin Kosek mko...@redhat.com
Supervisor, Software Engineering - Identity Management Team
Red Hat Inc.
From 08d0a5f2bc7874943021258fedc71e9f6cfb76ba Mon Sep 17 00:00:00 2001
From: Martin Kosek mko...@redhat.com
Date: Thu, 24 Jul 2014 09:57:54 +0200
Subject: [PATCH] Do not crash client basedn discovery when SSF not met

ipa-client-install runs anonymous search in non-rootdse space which
may raise UNWILLING_TO_PERFORM error. This case was only covered for
BIND, but not for the actual LDAP queries.

https://fedorahosted.org/freeipa/ticket/4459
---
 ipa-client/ipaclient/ipadiscovery.py | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/ipa-client/ipaclient/ipadiscovery.py b/ipa-client/ipaclient/ipadiscovery.py
index 1e084dba197380d25f4ed9cad08d34cecb922c0b..0532f618e81d215c4416f62f81af2add48c7dc8e 100644
--- a/ipa-client/ipaclient/ipadiscovery.py
+++ b/ipa-client/ipaclient/ipadiscovery.py
@@ -335,6 +335,10 @@ def ipacheckldap(self, thost, trealm, ca_cert_path=None):
  no_schema=True, decode_attrs=False)
 try:
 lh.do_simple_bind(DN(), '')
+
+# get IPA base DN
+root_logger.debug(Search LDAP server for IPA base DN)
+basedn = get_ipa_basedn(lh)
 except errors.ACIError:
 root_logger.debug(LDAP Error: Anonymous access not allowed)
 return [NO_ACCESS_TO_LDAP]
@@ -350,10 +354,6 @@ def ipacheckldap(self, thost, trealm, ca_cert_path=None):
 else:
 return [UNKNOWN_ERROR]
 
-# get IPA base DN
-root_logger.debug(Search LDAP server for IPA base DN)
-basedn = get_ipa_basedn(lh)
-
 if basedn is None:
 root_logger.debug(The server is not an IPA server)
 return [NOT_IPA_SERVER]
-- 
1.9.3

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

Re: [Freeipa-devel] [PATCH] 0007 test group: remove group from protected group

2014-07-29 Thread Petr Viktorin

On 07/29/2014 12:58 PM, David Kupka wrote:

On 07/28/2014 06:41 PM, Petr Viktorin wrote:

On 07/24/2014 03:11 PM, David Kupka wrote:

Simple test scenario from ticket #4448.

Last test will fail until patch freeipa-dkupka-0006 gets accepted.



Thanks! These look fine, but since the new tests don't require that the
rest of `test_group` is run first, I encourage you to put them in a
separate class.

Put to separate class, as suggested. Looks better now.


This would ensure we don't add new inderdependencies between old and new
tests in the future, making future test refactoring more straightforward.
Also, you can select to run just a single test class from a module, so
testing a targeted fix is faster.
(And you can reuse group1, since the other test cleans it up)

See test_permission_plugin for an example.


The test still fails on current master but works fine with patch
freeipa-dkupka-0006-2.



Thanks! ACK, pushed to:
master: f7e00b9ad626e48a3e78a5ff68512642312a6d3d
ipa-4-1: f7e00b9ad626e48a3e78a5ff68512642312a6d3d
ipa-4-0: 19dd0c67bba41cd76fb1600a3d5f0293ec6f7c75


--
Petr³

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


Re: [Freeipa-devel] [PATCH] 0005 Verify otptoken timespan is valid

2014-07-29 Thread Petr Viktorin

On 07/29/2014 03:58 PM, Jan Cholasta wrote:

Dne 29.7.2014 v 15:52 David Kupka napsal(a):

On 07/29/2014 03:28 PM, Jan Cholasta wrote:

Dne 29.7.2014 v 14:11 David Kupka napsal(a):

On 07/29/2014 01:21 PM, Jan Cholasta wrote:

Dne 24.7.2014 v 10:00 David Kupka napsal(a):



On 07/23/2014 05:07 PM, Jan Cholasta wrote:

Hi,

On 23.7.2014 15:46, David Kupka wrote:

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


1) Use isinstance(X, Y) instead of type(X) is Y.


Thanks for advice, will try to remember.


2) When is type(not_before) is str or type(not_after) is str
true?
The values coming from command options or LDAP should always be
datetime, never str.


Actually, it is never true. I don't  know why I thought that there is
such option.


3) There are some misindentations:

+raise ValidationError(name='not_after',
+error='is before not_before!')

+raise ValidationError(name='not_after',
+error='is before not_before!')

+raise ValidationError(name='not_before',
+error='is after not_after!')

4) We don't do exclamation marks in errors messages.


You re right, it's probably better not to shout at customer :)


5) Generally, when you want to validate command options, you should
look
into options, not entry_attrs.

6) This is not right:

+result =
self.api.Command.otptoken_find(ipatokenuniqueid=
+entry_attrs.get('ipatokenuniqueid',
None))['result']

This is:

+result =
self.api.Command.otptoken_show(keys[-1])['result']


Both works, but Martin explained me why is otptoken_show better and
how
it actually works.


Honza





Few more nitpicks:

1) You can make _check_interval a little bit shorter by removing a
redundant if statement:

+def _check_interval(not_before, not_after):
+if not_before and not_after:
+return not_before = not_after
+return True

Nice :)


2) Please keep the 2 line space between the last global function and
first class in the module.

It's good to know that there is one less rule to discover.


3) Error messages are for users' eyes, please don't use internal
identifiers in them.

Done.


4) You don't need to check if the result of otptoken_show is empty, it
never is.


Removed. Although, needless check can't hurt.



One more thing, sorry I didn't notice it earlier: You must check if
'ipatokennotbefore' and 'ipatokennotafter' keys are in opttoken-show
result before using them, otherwise you might end up with:

 ipa: ERROR: non-public: KeyError: 'ipatokennotbefore'
 Traceback (most recent call last):
   File /usr/lib/python2.7/site-packages/ipaserver/rpcserver.py,
line 348, in wsgi_execute
 result = self.Command[name](*args, **options)
   File /usr/lib/python2.7/site-packages/ipalib/frontend.py, line
439, in __call__
 ret = self.run(*args, **options)
   File /usr/lib/python2.7/site-packages/ipalib/frontend.py, line
754, in run
 return self.execute(*args, **options)
   File
/usr/lib/python2.7/site-packages/ipalib/plugins/baseldap.py, line
1384, in execute
 *keys, **options)
   File
/usr/lib/python2.7/site-packages/ipalib/plugins/otptoken.py, line 356,
in pre_callback
 notbefore = result['ipatokennotbefore'][0]
 KeyError: 'ipatokennotbefore'


Sorry for sending fifth wersion of the patch. I must test my patches
more carefully.



No problem. I must review more carefully. ACK.



Pushed to:
master: 724391a71b018c94aca71b588a24983e228cf2a7
ipa-4-1: 724391a71b018c94aca71b588a24983e228cf2a7
ipa-4-0: 928c87f5c89916e7bb1a6e1e5def45302a2ea8ec

--
Petr³

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


Re: [Freeipa-devel] [PATCH] 480 Do not crash client basedn discovery when SSF not met

2014-07-29 Thread Petr Viktorin

On 07/29/2014 05:03 PM, Martin Kosek wrote:

ipa-client-install runs anonymous search in non-rootdse space which
may raise UNWILLING_TO_PERFORM error. This case was only covered for
BIND, but not for the actual LDAP queries.

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


ACK, pushed to:
master: aa0639284c233d10b1bb4c02317155436685dc38
ipa-4-1: aa0639284c233d10b1bb4c02317155436685dc38
ipa-4-0: b104179e0313358ad3f72b3abde6095dd2a24ac1



--
Petr³

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


Re: [Freeipa-devel] [PATCHES] 0264-0267 backup, restore: Don't overwrite /etc/{passwd, group}

2014-07-29 Thread Petr Viktorin

On 07/29/2014 05:02 PM, Petr Viktorin wrote:

Hello,

The first patch here consolidates our system user creation code a bit.

The second patch fixes an oversight in the restore script.

The third changes the backup script to not include /etc/{passwd,group},
and the restore script to create the PKI user if a CA is being restored.
Note that the DS user is already created early in the restore process.
(In the future we may want a nice generic framework for restoring users,
but I'd like to extrapolate from more than one data point when designing
it.)


Another note: tar uses owner user/group names by default, so no 
additional chowning is required even if the IDs change between backup  
restore.




The fourth patch adds a log entry I find very useful in testing
backup/restore.




--
Petr³

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


Re: [Freeipa-devel] Password Vault Implementation

2014-07-29 Thread Endi Sukma Dewata

On 7/15/2014 9:13 AM, Endi Sukma Dewata wrote:

Hi,

I've been working on the implementation details of password vault:
http://www.freeipa.org/page/V4/Password_Vault_Implementation

There are some issues (i.e. vault password and vault key) that aren't
specifically defined in the design, so we need to make some decisions.

Please let me know if you have any comments or questions. Thanks!


Hi,

I have made a number of changes to the above page including:
* using a vault as a container of secrets
* using a single encryption key per vault instead of per secret
* using one escrow officer per vault
* adding escrow, access control,  configuration
* adding client API  web services
* adding database  transaction
* adding FAQ

The options that I presented in the original version were removed 
because as I worked on the details it appeared that the alternatives 
don't actually make sense.


Please take a look and let me know if you have any comments or 
questions. There are more details to be fleshed out, so I'll keep 
updating the page. Thanks.


--
Endi S. Dewata

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