Re: [Freeipa-devel] [PATCH 0559] Increase default length of auto-generated passwords

[Alexander Bokovoy]

On Fri, 29 Jul 2016, Martin Basti wrote:



On 29.07.2016 17:09, Alexander Bokovoy wrote:
> On Fri, 29 Jul 2016, Martin Basti wrote:
> > https://fedorahosted.org/freeipa/ticket/6116
> > 
> > 
> > Patch attached
> > 
> 
> > From ca5305e032137b7c9197d0c1050191079a72124e Mon Sep 17 00:00:00 2001

> > From: Martin Basti 
> > Date: Fri, 22 Jul 2016 16:41:29 +0200
> > Subject: [PATCH] Increase default length of auto generated passwords
> > 
> > Installer/IPA generates passwords for warious purpose:

> > * KRA
> > * kerberos master key
> > * NSSDB password
> > * temporary passwords during installation
> > 
> > Length of passwords should be increased to 22, ~128bits of entropy, to

> > be safe nowadays.
> > 
> > https://fedorahosted.org/freeipa/ticket/6116

> ACK with a minor comment.
> 
> > ---

> > ipapython/ipautil.py   | 2 +-
> > ipaserver/plugins/baseuser.py  | 3 ++-
> > ipaserver/plugins/host.py  | 3 ++-
> > ipaserver/plugins/stageuser.py | 3 ++-
> > ipaserver/plugins/user.py  | 3 ++-
> > 5 files changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
> > index 9964fba4f694b57242b3bd3065a418917d977533..ca7e81d666cd6c345bdbbf4660c3451ac1f2c045 
> > 100644

> > --- a/ipapython/ipautil.py
> > +++ b/ipapython/ipautil.py
> > @@ -57,7 +57,7 @@ from ipapython.dn import DN
> > SHARE_DIR = paths.USR_SHARE_IPA_DIR
> > PLUGINS_SHARE_DIR = paths.IPA_PLUGINS
> > 
> > -GEN_PWD_LEN = 12

> > +GEN_PWD_LEN = 22
> It would be good to add a temporary password constant too
> +GEN_TMP_PWD_LEN = 12
> 
> and then use it instead of pwd_len=12 below.
> 
> > # Having this in krb_utils would cause circular import
> > KRB5_KDC_UNREACH = 2529639068 # Cannot contact any KDC for requested 
> > realm
> > diff --git a/ipaserver/plugins/baseuser.py 
> > b/ipaserver/plugins/baseuser.py
> > index e4288a5a131157815ffb2452692a7edb342f6ac3..5e0752c8d3d246fa7c283f05b82ef01de2e5bf34 
> > 100644

> > --- a/ipaserver/plugins/baseuser.py
> > +++ b/ipaserver/plugins/baseuser.py
> > @@ -552,7 +552,8 @@ class baseuser_mod(LDAPUpdate):
> > 
> > def check_userpassword(self, entry_attrs, **options):

> > if 'userpassword' not in entry_attrs and options.get('random'):
> > -entry_attrs['userpassword'] = 
> > ipa_generate_password(baseuser_pwdchars)

> > +entry_attrs['userpassword'] = ipa_generate_password(
> > +baseuser_pwdchars, pwd_len=12)
> > # save the password so it can be displayed in post_callback
> > setattr(context, 'randompassword', 
> > entry_attrs['userpassword'])
> > 
> > diff --git a/ipaserver/plugins/host.py b/ipaserver/plugins/host.py
> > index 413dcf15e0423170d8334902b9dcf8fb5aa14de6..1cefb6224e1a6dad0080369edee35c4524e5bd39 
> > 100644

> > --- a/ipaserver/plugins/host.py
> > +++ b/ipaserver/plugins/host.py
> > @@ -683,7 +683,8 @@ class host_add(LDAPCreate):
> > if 'krbprincipal' in entry_attrs['objectclass']:
> > entry_attrs['objectclass'].remove('krbprincipal')
> > if options.get('random'):
> > -entry_attrs['userpassword'] = 
> > ipa_generate_password(characters=host_pwd_chars)

> > +entry_attrs['userpassword'] = ipa_generate_password(
> > +characters=host_pwd_chars, pwd_len=12)
> > # save the password so it can be displayed in post_callback
> > setattr(context, 'randompassword', 
> > entry_attrs['userpassword'])

> > certs = options.get('usercertificate', [])
> > diff --git a/ipaserver/plugins/stageuser.py 
> > b/ipaserver/plugins/stageuser.py
> > index 3b9388f6020b9a6c40caedd36f3640a05a13da65..6df189c3913171b4990ce115b296b19c7447592d 
> > 100644

> > --- a/ipaserver/plugins/stageuser.py
> > +++ b/ipaserver/plugins/stageuser.py
> > @@ -339,7 +339,8 @@ class stageuser_add(baseuser_add):
> > 
> > # If requested, generate a userpassword

> > if 'userpassword' not in entry_attrs and options.get('random'):
> > -entry_attrs['userpassword'] = 
> > ipa_generate_password(baseuser_pwdchars)

> > +entry_attrs['userpassword'] = ipa_generate_password(
> > +baseuser_pwdchars, pwd_len=12)
> > # save the password so it can be displayed in post_callback
> > setattr(context, 'randompassword', 
> > entry_attrs['userpassword'])
> > 
> > diff --git a/ipaserver/plugins/user.py b/ipaserver/plugins/user.py
> > index b3ae7646fdcfa1dce10d90063dae2a24c091e8ee..62ec529062c7ac39661df2a8c3d2277711268b11 
> > 100644

> > --- a/ipaserver/plugins/user.py
> > +++ b/ipaserver/plugins/user.py
> > @@ -517,7 +517,8 @@ class user_add(baseuser_add):
> > entry_attrs['gidnumber'] = group_attrs['gidnumber']
> > 
> > if 'userpassword' not in entry_attrs and options.get('random'):
> > -entry_attrs['userpassword'] = 
> > ipa_generate_password(baseuser_pwdchars)

> > +entry_attrs['userpassword'] = 

Re: [Freeipa-devel] [PATCH 0559] Increase default length of auto-generated passwords

[Martin Basti]

On 29.07.2016 17:09, Alexander Bokovoy wrote:

On Fri, 29 Jul 2016, Martin Basti wrote:

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


Patch attached




From ca5305e032137b7c9197d0c1050191079a72124e Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Fri, 22 Jul 2016 16:41:29 +0200
Subject: [PATCH] Increase default length of auto generated passwords

Installer/IPA generates passwords for warious purpose:
* KRA
* kerberos master key
* NSSDB password
* temporary passwords during installation

Length of passwords should be increased to 22, ~128bits of entropy, to
be safe nowadays.

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

ACK with a minor comment.


---
ipapython/ipautil.py   | 2 +-
ipaserver/plugins/baseuser.py  | 3 ++-
ipaserver/plugins/host.py  | 3 ++-
ipaserver/plugins/stageuser.py | 3 ++-
ipaserver/plugins/user.py  | 3 ++-
5 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index 
9964fba4f694b57242b3bd3065a418917d977533..ca7e81d666cd6c345bdbbf4660c3451ac1f2c045 
100644

--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -57,7 +57,7 @@ from ipapython.dn import DN
SHARE_DIR = paths.USR_SHARE_IPA_DIR
PLUGINS_SHARE_DIR = paths.IPA_PLUGINS

-GEN_PWD_LEN = 12
+GEN_PWD_LEN = 22

It would be good to add a temporary password constant too
+GEN_TMP_PWD_LEN = 12

and then use it instead of pwd_len=12 below.


# Having this in krb_utils would cause circular import
KRB5_KDC_UNREACH = 2529639068 # Cannot contact any KDC for requested 
realm
diff --git a/ipaserver/plugins/baseuser.py 
b/ipaserver/plugins/baseuser.py
index 
e4288a5a131157815ffb2452692a7edb342f6ac3..5e0752c8d3d246fa7c283f05b82ef01de2e5bf34 
100644

--- a/ipaserver/plugins/baseuser.py
+++ b/ipaserver/plugins/baseuser.py
@@ -552,7 +552,8 @@ class baseuser_mod(LDAPUpdate):

def check_userpassword(self, entry_attrs, **options):
if 'userpassword' not in entry_attrs and options.get('random'):
-entry_attrs['userpassword'] = 
ipa_generate_password(baseuser_pwdchars)

+entry_attrs['userpassword'] = ipa_generate_password(
+baseuser_pwdchars, pwd_len=12)
# save the password so it can be displayed in post_callback
setattr(context, 'randompassword', 
entry_attrs['userpassword'])


diff --git a/ipaserver/plugins/host.py b/ipaserver/plugins/host.py
index 
413dcf15e0423170d8334902b9dcf8fb5aa14de6..1cefb6224e1a6dad0080369edee35c4524e5bd39 
100644

--- a/ipaserver/plugins/host.py
+++ b/ipaserver/plugins/host.py
@@ -683,7 +683,8 @@ class host_add(LDAPCreate):
if 'krbprincipal' in entry_attrs['objectclass']:
entry_attrs['objectclass'].remove('krbprincipal')
if options.get('random'):
-entry_attrs['userpassword'] = 
ipa_generate_password(characters=host_pwd_chars)

+entry_attrs['userpassword'] = ipa_generate_password(
+characters=host_pwd_chars, pwd_len=12)
# save the password so it can be displayed in post_callback
setattr(context, 'randompassword', 
entry_attrs['userpassword'])

certs = options.get('usercertificate', [])
diff --git a/ipaserver/plugins/stageuser.py 
b/ipaserver/plugins/stageuser.py
index 
3b9388f6020b9a6c40caedd36f3640a05a13da65..6df189c3913171b4990ce115b296b19c7447592d 
100644

--- a/ipaserver/plugins/stageuser.py
+++ b/ipaserver/plugins/stageuser.py
@@ -339,7 +339,8 @@ class stageuser_add(baseuser_add):

# If requested, generate a userpassword
if 'userpassword' not in entry_attrs and options.get('random'):
-entry_attrs['userpassword'] = 
ipa_generate_password(baseuser_pwdchars)

+entry_attrs['userpassword'] = ipa_generate_password(
+baseuser_pwdchars, pwd_len=12)
# save the password so it can be displayed in post_callback
setattr(context, 'randompassword', 
entry_attrs['userpassword'])


diff --git a/ipaserver/plugins/user.py b/ipaserver/plugins/user.py
index 
b3ae7646fdcfa1dce10d90063dae2a24c091e8ee..62ec529062c7ac39661df2a8c3d2277711268b11 
100644

--- a/ipaserver/plugins/user.py
+++ b/ipaserver/plugins/user.py
@@ -517,7 +517,8 @@ class user_add(baseuser_add):
entry_attrs['gidnumber'] = group_attrs['gidnumber']

if 'userpassword' not in entry_attrs and options.get('random'):
-entry_attrs['userpassword'] = 
ipa_generate_password(baseuser_pwdchars)

+entry_attrs['userpassword'] = ipa_generate_password(
+baseuser_pwdchars, pwd_len=12)
# save the password so it can be displayed in post_callback
setattr(context, 'randompassword', 
entry_attrs['userpassword'])


--
2.5.5




--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code




Thanks
Updated patch attached

Martin^2
From 

Re: [Freeipa-devel] [PATCH 0004-0012] Automatic CSR generation

[Ben Lipton]

On 07/29/2016 09:39 AM, Petr Spacek wrote:

On 27.7.2016 19:06, Ben Lipton wrote:

Hi all,

I think the automatic CSR generation feature
(https://fedorahosted.org/freeipa/ticket/4899,
http://www.freeipa.org/page/V4/Automatic_Certificate_Request_Generation) is
stable enough to review now. The following are summaries of the attached 
patches:
0004: LDAP schema changes for the new feature
0005: Basic API for new objects and CSR generation
0006: Update install automation to create some default mapping rules
0007: Implement the lookups and text processing that generates the CSR config
0008 and 0009: Implement some actual transformation rules so that the feature
is usable
0010: Add a new cert profile for user certs, with mappings
0011: Implement import/export of cert profiles with mappings
0012: Tests for profile import/export

Generally speaking, later patches depend on earlier ones, but I don't
anticipate any problems from committing earlier patches without later ones.

If you prefer, you can also comment on the pull request version:
https://github.com/LiptonB/freeipa/pull/4. Note that I may force push on this
branch.

Allocation of OIDs for schema change also needs review:
https://code.engineering.redhat.com/gerrit/#/c/80061/

Known issues:
- When the requested principal does not have some of the requested data,
produces funny-looking configs with extra commas, empty sections, etc. They
are still accepted by my copies of openssl and certutil, but they look ugly.
- The new objects don't have any ACIs, so for the moment only admin can run
the new commands.
- Does not yet have support for prompting user for field values, so currently
all data must come from the database.
- All processing happens on the server side. As discussed in a previous
thread, it would be desirable to break this out into a library so it could be
used client-side.

Very excited to hear your thoughts!

Hi Ben,

I wanted to give it a try from user's point of view first, before diving into
implementation details. Here are some observations:


Thanks for giving it a try! This is great feedback.


0. Design pages are using term "helper" and it is used even as option in the
example with smartcards. Please fix either design page or the code so they are
consistent.
Good point. In a previous discussion, Alexander remarked that --helper 
sounded too low-level, but I find that --use sounds very generic and 
--format doesn't make a lot of sense for ipa cert-request, which never 
actually gives you the config that's generated. So if they're all going 
to be the same word, which is probably a good idea, I might be leaning 
towards --helper, but I'm interested to hear opinions on this.


1. The "ipa cert-request" command is missing options --autofill and --use (aka
helper aka format :-) which are mentioned in the design pages.
Yeah, I haven't managed to implement much of the UI niceness suggested 
by the design page. I probably should have mentioned that in the email - 
all that I expect to be working at this point is 'ipa 
cert-get-requestdata' and CRUD for the mapping rules/profiles themselves.


2. "ipa cert_get_requestdata" command passes even without --profile-id and
generates empty config. I think that this is not expected :-)


More expected than you might think :) I'm guessing what's happening is 
that you're passing a user principal and it's defaulting to the 
caIPAserviceCert profile, then failing to fill out any of the fields 
because the data it needs is not there. I agree this isn't great. I was 
planning to address this by having it throw an error if it can't 
generate at least the subject of the request, and maybe suggest using a 
different profile.


I chose to have it default to caIPAserviceCert because that's what ipa 
cert-request does, but maybe that's not as predictable as I thought.


3. "ipa cert_get_requestdata --format=openssl" prints the text to stdout
including label "Configuration file contents:". This is hard to use because
simple redirection like "ipa cert_get_requestdata --format=openssl > config"
will not give you valid OpenSSL config file - it needs hand-editing.

It would be good to add --out parameter you envisioned in the design page.
Please ask jcholast for proper name and semantics :-) With --out option the
command can be used to generate valid config (or script if certutil was 
selected).
Agreed. Another example of the UI not being quite right yet. I've been 
unsure how to handle this, because of certutil taking a command line and 
openssl a config file. It actually gets even more complicated because, 
as you point out in the next item, openssl also needs a command in 
addition to the config file. I'm interested in thinking about how to 
handle this cleanly from a user perspective. Generating a script, or 
providing the command lines as hints, might be ways to get around these 
concerns.

4. "ipa cert_get_requestdata --format=openssl" could print hint what OpenSSL
command should be executed on the generated config 

Re: [Freeipa-devel] [PATCH 0558] idrange: fix unassigned global variable

[Martin Basti]

On 29.07.2016 17:07, Alexander Bokovoy wrote:

On Fri, 29 Jul 2016, Martin Basti wrote:

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

patch attached


Traceback (most recent call last):
 File "/usr/libexec/ipa/oddjob/com.redhat.idm.trust-fetch-domains", 
line 174, in 
   trust.add_new_domains_from_trust(api, None, trust_domain_object, 
domains)
 File "/usr/lib/python2.7/site-packages/ipaserver/plugins/trust.py", 
line 1684, in add_new_domains_from_trust

   trust_name, name, **dom)
 File "/usr/lib/python2.7/site-packages/ipaserver/plugins/trust.py", 
line 435, in add_range

   ipanttrusteddomainsid=dom_sid)
 File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 
447, in __call__

   return self.__do_call(*args, **options)
 File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 
475, in __do_call

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

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

   *keys, **options)
 File 
"/usr/lib/python2.7/site-packages/ipaserver/plugins/idrange.py", line 
465, in pre_callback

   entry_attrs['ipanttrusteddomainsid'])
 File 
"/usr/lib/python2.7/site-packages/ipaserver/plugins/idrange.py", line 
338, in validate_trusted_domain_sid

   domain_validator = self.get_domain_validator()
 File 
"/usr/lib/python2.7/site-packages/ipaserver/plugins/idrange.py", line 
322, in get_domain_validator

   if not _dcerpc_bindings_installed:
NameError: global name '_dcerpc_bindings_installed' is not defined




From 0e0c860f8b555fb5fef7d13a7e3f9d3f361363c4 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Fri, 29 Jul 2016 16:46:09 +0200
Subject: [PATCH] idrange: fix unassigned global variable

Global variable '_dcerpc_bindings_installed' is in some cases used
before assigment. This patch ensures that _dcerpc_bindings_installed is
always initialized.

https://fedorahosted.org/freeipa/ticket/6082
---
ipaserver/plugins/idrange.py | 3 +++
1 file changed, 3 insertions(+)

diff --git a/ipaserver/plugins/idrange.py b/ipaserver/plugins/idrange.py
index 
ccd67995e5b42634387e1064e7c819b711f3ef99..3e9db0b6b734513547423901a8b3212b3cee9147 
100644

--- a/ipaserver/plugins/idrange.py
+++ b/ipaserver/plugins/idrange.py
@@ -35,6 +35,9 @@ if api.env.in_server and api.env.context in 
['lite', 'server']:

_dcerpc_bindings_installed = True
except ImportError:
_dcerpc_bindings_installed = False
+else:
+_dcerpc_bindings_installed = False
+

ID_RANGE_VS_DNA_WARNING = """===
WARNING:
--
2.5.5


ACK. I was intending to look at this but you got there faster.


Pushed to master: c2edfa0adbc1a603a146aa44d73a4024e06063f0

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] certmonger EST RFC7030 support possible ?

[Rob Crittenden]

Marx, Peter wrote:

Hi,

we are using certmonger with SCEP. But SCEP does not support Elliptic
curve keys, only RSA.

The successor protocol EST (Enrollment over Secure Transport) would
support ECC.

Is a EST helper for certmonger/getcert on the roadmap ?


No. I added a ticket to track it, 
https://fedorahosted.org/certmonger/ticket/53



If yes, when ?

How complicated is it to create such a helper around the Cisco
open-sourced libest ?


Hard to say without digging into the library. The library was 
open-sourced less than 3 weeks ago AFAICT.


Practically this also means someone will need to package it for the 
various Linux distributions.


rob

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0559] Increase default length of auto-generated passwords

[Alexander Bokovoy]

On Fri, 29 Jul 2016, Martin Basti wrote:

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


Patch attached




From ca5305e032137b7c9197d0c1050191079a72124e Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Fri, 22 Jul 2016 16:41:29 +0200
Subject: [PATCH] Increase default length of auto generated passwords

Installer/IPA generates passwords for warious purpose:
* KRA
* kerberos master key
* NSSDB password
* temporary passwords during installation

Length of passwords should be increased to 22, ~128bits of entropy, to
be safe nowadays.

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

ACK with a minor comment.


---
ipapython/ipautil.py   | 2 +-
ipaserver/plugins/baseuser.py  | 3 ++-
ipaserver/plugins/host.py  | 3 ++-
ipaserver/plugins/stageuser.py | 3 ++-
ipaserver/plugins/user.py  | 3 ++-
5 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index 
9964fba4f694b57242b3bd3065a418917d977533..ca7e81d666cd6c345bdbbf4660c3451ac1f2c045
 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -57,7 +57,7 @@ from ipapython.dn import DN
SHARE_DIR = paths.USR_SHARE_IPA_DIR
PLUGINS_SHARE_DIR = paths.IPA_PLUGINS

-GEN_PWD_LEN = 12
+GEN_PWD_LEN = 22

It would be good to add a temporary password constant too
+GEN_TMP_PWD_LEN = 12

and then use it instead of pwd_len=12 below.


# Having this in krb_utils would cause circular import
KRB5_KDC_UNREACH = 2529639068 # Cannot contact any KDC for requested realm
diff --git a/ipaserver/plugins/baseuser.py b/ipaserver/plugins/baseuser.py
index 
e4288a5a131157815ffb2452692a7edb342f6ac3..5e0752c8d3d246fa7c283f05b82ef01de2e5bf34
 100644
--- a/ipaserver/plugins/baseuser.py
+++ b/ipaserver/plugins/baseuser.py
@@ -552,7 +552,8 @@ class baseuser_mod(LDAPUpdate):

def check_userpassword(self, entry_attrs, **options):
if 'userpassword' not in entry_attrs and options.get('random'):
-entry_attrs['userpassword'] = 
ipa_generate_password(baseuser_pwdchars)
+entry_attrs['userpassword'] = ipa_generate_password(
+baseuser_pwdchars, pwd_len=12)
# save the password so it can be displayed in post_callback
setattr(context, 'randompassword', entry_attrs['userpassword'])

diff --git a/ipaserver/plugins/host.py b/ipaserver/plugins/host.py
index 
413dcf15e0423170d8334902b9dcf8fb5aa14de6..1cefb6224e1a6dad0080369edee35c4524e5bd39
 100644
--- a/ipaserver/plugins/host.py
+++ b/ipaserver/plugins/host.py
@@ -683,7 +683,8 @@ class host_add(LDAPCreate):
if 'krbprincipal' in entry_attrs['objectclass']:
entry_attrs['objectclass'].remove('krbprincipal')
if options.get('random'):
-entry_attrs['userpassword'] = 
ipa_generate_password(characters=host_pwd_chars)
+entry_attrs['userpassword'] = ipa_generate_password(
+characters=host_pwd_chars, pwd_len=12)
# save the password so it can be displayed in post_callback
setattr(context, 'randompassword', entry_attrs['userpassword'])
certs = options.get('usercertificate', [])
diff --git a/ipaserver/plugins/stageuser.py b/ipaserver/plugins/stageuser.py
index 
3b9388f6020b9a6c40caedd36f3640a05a13da65..6df189c3913171b4990ce115b296b19c7447592d
 100644
--- a/ipaserver/plugins/stageuser.py
+++ b/ipaserver/plugins/stageuser.py
@@ -339,7 +339,8 @@ class stageuser_add(baseuser_add):

# If requested, generate a userpassword
if 'userpassword' not in entry_attrs and options.get('random'):
-entry_attrs['userpassword'] = 
ipa_generate_password(baseuser_pwdchars)
+entry_attrs['userpassword'] = ipa_generate_password(
+baseuser_pwdchars, pwd_len=12)
# save the password so it can be displayed in post_callback
setattr(context, 'randompassword', entry_attrs['userpassword'])

diff --git a/ipaserver/plugins/user.py b/ipaserver/plugins/user.py
index 
b3ae7646fdcfa1dce10d90063dae2a24c091e8ee..62ec529062c7ac39661df2a8c3d2277711268b11
 100644
--- a/ipaserver/plugins/user.py
+++ b/ipaserver/plugins/user.py
@@ -517,7 +517,8 @@ class user_add(baseuser_add):
entry_attrs['gidnumber'] = group_attrs['gidnumber']

if 'userpassword' not in entry_attrs and options.get('random'):
-entry_attrs['userpassword'] = 
ipa_generate_password(baseuser_pwdchars)
+entry_attrs['userpassword'] = ipa_generate_password(
+baseuser_pwdchars, pwd_len=12)
# save the password so it can be displayed in post_callback
setattr(context, 'randompassword', entry_attrs['userpassword'])

--
2.5.5




--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code



--
/ Alexander Bokovoy

--
Manage your subscription for the Freeipa-devel mailing list:

Re: [Freeipa-devel] [PATCH 0558] idrange: fix unassigned global variable

[Alexander Bokovoy]

On Fri, 29 Jul 2016, Martin Basti wrote:

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

patch attached


Traceback (most recent call last):
 File "/usr/libexec/ipa/oddjob/com.redhat.idm.trust-fetch-domains", 
line 174, in 
   trust.add_new_domains_from_trust(api, None, trust_domain_object, 
domains)
 File "/usr/lib/python2.7/site-packages/ipaserver/plugins/trust.py", 
line 1684, in add_new_domains_from_trust

   trust_name, name, **dom)
 File "/usr/lib/python2.7/site-packages/ipaserver/plugins/trust.py", 
line 435, in add_range

   ipanttrusteddomainsid=dom_sid)
 File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 
447, in __call__

   return self.__do_call(*args, **options)
 File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 
475, in __do_call

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

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

   *keys, **options)
 File 
"/usr/lib/python2.7/site-packages/ipaserver/plugins/idrange.py", line 
465, in pre_callback

   entry_attrs['ipanttrusteddomainsid'])
 File 
"/usr/lib/python2.7/site-packages/ipaserver/plugins/idrange.py", line 
338, in validate_trusted_domain_sid

   domain_validator = self.get_domain_validator()
 File 
"/usr/lib/python2.7/site-packages/ipaserver/plugins/idrange.py", line 
322, in get_domain_validator

   if not _dcerpc_bindings_installed:
NameError: global name '_dcerpc_bindings_installed' is not defined




From 0e0c860f8b555fb5fef7d13a7e3f9d3f361363c4 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Fri, 29 Jul 2016 16:46:09 +0200
Subject: [PATCH] idrange: fix unassigned global variable

Global variable '_dcerpc_bindings_installed' is in some cases used
before assigment. This patch ensures that _dcerpc_bindings_installed is
always initialized.

https://fedorahosted.org/freeipa/ticket/6082
---
ipaserver/plugins/idrange.py | 3 +++
1 file changed, 3 insertions(+)

diff --git a/ipaserver/plugins/idrange.py b/ipaserver/plugins/idrange.py
index 
ccd67995e5b42634387e1064e7c819b711f3ef99..3e9db0b6b734513547423901a8b3212b3cee9147
 100644
--- a/ipaserver/plugins/idrange.py
+++ b/ipaserver/plugins/idrange.py
@@ -35,6 +35,9 @@ if api.env.in_server and api.env.context in ['lite', 
'server']:
_dcerpc_bindings_installed = True
except ImportError:
_dcerpc_bindings_installed = False
+else:
+_dcerpc_bindings_installed = False
+

ID_RANGE_VS_DNA_WARNING = """===
WARNING:
--
2.5.5


ACK. I was intending to look at this but you got there faster.

--
/ Alexander Bokovoy

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [PATCH 0560] ipa-client-automount: don not initialize API during uninstall

[Martin Basti]

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

Patch attached.

From 0e1b634bbca33e8be8fc0ea5246d553c85aa3495 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Thu, 28 Jul 2016 09:47:39 +0200
Subject: [PATCH] Do not initialize API in ipa-client-automount uninstall

API is not needed in uninstallation, it may only produce errors.

https://fedorahosted.org/freeipa/ticket/6072
---
 client/ipa-client-automount | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/client/ipa-client-automount b/client/ipa-client-automount
index f06aa7f8d53ba2528bc2c023792771d5fd341e7c..08209c849f155a8394acddc6bb961be8fa68073c 100755
--- a/client/ipa-client-automount
+++ b/client/ipa-client-automount
@@ -378,6 +378,9 @@ def main():
 paths.IPACLIENT_INSTALL_LOG, verbose=False, debug=options.debug,
 filemode='a', console_format='%(message)s')
 
+if options.uninstall:
+return uninstall(fstore, statestore)
+
 cfg = dict(
 context='cli_installer',
 in_server=False,
@@ -392,9 +395,6 @@ def main():
 if os.path.exists(paths.IPA_CA_CRT):
 ca_cert_path = paths.IPA_CA_CRT
 
-if options.uninstall:
-return uninstall(fstore, statestore)
-
 if statestore.has_state('autofs'):
 sys.exit('automount is already configured on this system.\n')
 
-- 
2.5.5

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [PATCH 0559] Increase default length of auto-generated passwords

[Martin Basti]

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


Patch attached

From ca5305e032137b7c9197d0c1050191079a72124e Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Fri, 22 Jul 2016 16:41:29 +0200
Subject: [PATCH] Increase default length of auto generated passwords

Installer/IPA generates passwords for warious purpose:
* KRA
* kerberos master key
* NSSDB password
* temporary passwords during installation

Length of passwords should be increased to 22, ~128bits of entropy, to
be safe nowadays.

https://fedorahosted.org/freeipa/ticket/6116
---
 ipapython/ipautil.py   | 2 +-
 ipaserver/plugins/baseuser.py  | 3 ++-
 ipaserver/plugins/host.py  | 3 ++-
 ipaserver/plugins/stageuser.py | 3 ++-
 ipaserver/plugins/user.py  | 3 ++-
 5 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index 9964fba4f694b57242b3bd3065a418917d977533..ca7e81d666cd6c345bdbbf4660c3451ac1f2c045 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -57,7 +57,7 @@ from ipapython.dn import DN
 SHARE_DIR = paths.USR_SHARE_IPA_DIR
 PLUGINS_SHARE_DIR = paths.IPA_PLUGINS
 
-GEN_PWD_LEN = 12
+GEN_PWD_LEN = 22
 
 # Having this in krb_utils would cause circular import
 KRB5_KDC_UNREACH = 2529639068 # Cannot contact any KDC for requested realm
diff --git a/ipaserver/plugins/baseuser.py b/ipaserver/plugins/baseuser.py
index e4288a5a131157815ffb2452692a7edb342f6ac3..5e0752c8d3d246fa7c283f05b82ef01de2e5bf34 100644
--- a/ipaserver/plugins/baseuser.py
+++ b/ipaserver/plugins/baseuser.py
@@ -552,7 +552,8 @@ class baseuser_mod(LDAPUpdate):
 
 def check_userpassword(self, entry_attrs, **options):
 if 'userpassword' not in entry_attrs and options.get('random'):
-entry_attrs['userpassword'] = ipa_generate_password(baseuser_pwdchars)
+entry_attrs['userpassword'] = ipa_generate_password(
+baseuser_pwdchars, pwd_len=12)
 # save the password so it can be displayed in post_callback
 setattr(context, 'randompassword', entry_attrs['userpassword'])
 
diff --git a/ipaserver/plugins/host.py b/ipaserver/plugins/host.py
index 413dcf15e0423170d8334902b9dcf8fb5aa14de6..1cefb6224e1a6dad0080369edee35c4524e5bd39 100644
--- a/ipaserver/plugins/host.py
+++ b/ipaserver/plugins/host.py
@@ -683,7 +683,8 @@ class host_add(LDAPCreate):
 if 'krbprincipal' in entry_attrs['objectclass']:
 entry_attrs['objectclass'].remove('krbprincipal')
 if options.get('random'):
-entry_attrs['userpassword'] = ipa_generate_password(characters=host_pwd_chars)
+entry_attrs['userpassword'] = ipa_generate_password(
+characters=host_pwd_chars, pwd_len=12)
 # save the password so it can be displayed in post_callback
 setattr(context, 'randompassword', entry_attrs['userpassword'])
 certs = options.get('usercertificate', [])
diff --git a/ipaserver/plugins/stageuser.py b/ipaserver/plugins/stageuser.py
index 3b9388f6020b9a6c40caedd36f3640a05a13da65..6df189c3913171b4990ce115b296b19c7447592d 100644
--- a/ipaserver/plugins/stageuser.py
+++ b/ipaserver/plugins/stageuser.py
@@ -339,7 +339,8 @@ class stageuser_add(baseuser_add):
 
 # If requested, generate a userpassword
 if 'userpassword' not in entry_attrs and options.get('random'):
-entry_attrs['userpassword'] = ipa_generate_password(baseuser_pwdchars)
+entry_attrs['userpassword'] = ipa_generate_password(
+baseuser_pwdchars, pwd_len=12)
 # save the password so it can be displayed in post_callback
 setattr(context, 'randompassword', entry_attrs['userpassword'])
 
diff --git a/ipaserver/plugins/user.py b/ipaserver/plugins/user.py
index b3ae7646fdcfa1dce10d90063dae2a24c091e8ee..62ec529062c7ac39661df2a8c3d2277711268b11 100644
--- a/ipaserver/plugins/user.py
+++ b/ipaserver/plugins/user.py
@@ -517,7 +517,8 @@ class user_add(baseuser_add):
 entry_attrs['gidnumber'] = group_attrs['gidnumber']
 
 if 'userpassword' not in entry_attrs and options.get('random'):
-entry_attrs['userpassword'] = ipa_generate_password(baseuser_pwdchars)
+entry_attrs['userpassword'] = ipa_generate_password(
+baseuser_pwdchars, pwd_len=12)
 # save the password so it can be displayed in post_callback
 setattr(context, 'randompassword', entry_attrs['userpassword'])
 
-- 
2.5.5

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [PATCH 0558] idrange: fix unassigned global variable

[Martin Basti]

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

patch attached


Traceback (most recent call last):
  File "/usr/libexec/ipa/oddjob/com.redhat.idm.trust-fetch-domains", 
line 174, in 
trust.add_new_domains_from_trust(api, None, trust_domain_object, 
domains)
  File "/usr/lib/python2.7/site-packages/ipaserver/plugins/trust.py", 
line 1684, in add_new_domains_from_trust

trust_name, name, **dom)
  File "/usr/lib/python2.7/site-packages/ipaserver/plugins/trust.py", 
line 435, in add_range

ipanttrusteddomainsid=dom_sid)
  File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 447, 
in __call__

return self.__do_call(*args, **options)
  File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 475, 
in __do_call

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

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

*keys, **options)
  File "/usr/lib/python2.7/site-packages/ipaserver/plugins/idrange.py", 
line 465, in pre_callback

entry_attrs['ipanttrusteddomainsid'])
  File "/usr/lib/python2.7/site-packages/ipaserver/plugins/idrange.py", 
line 338, in validate_trusted_domain_sid

domain_validator = self.get_domain_validator()
  File "/usr/lib/python2.7/site-packages/ipaserver/plugins/idrange.py", 
line 322, in get_domain_validator

if not _dcerpc_bindings_installed:
NameError: global name '_dcerpc_bindings_installed' is not defined

From 0e0c860f8b555fb5fef7d13a7e3f9d3f361363c4 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Fri, 29 Jul 2016 16:46:09 +0200
Subject: [PATCH] idrange: fix unassigned global variable

Global variable '_dcerpc_bindings_installed' is in some cases used
before assigment. This patch ensures that _dcerpc_bindings_installed is
always initialized.

https://fedorahosted.org/freeipa/ticket/6082
---
 ipaserver/plugins/idrange.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ipaserver/plugins/idrange.py b/ipaserver/plugins/idrange.py
index ccd67995e5b42634387e1064e7c819b711f3ef99..3e9db0b6b734513547423901a8b3212b3cee9147 100644
--- a/ipaserver/plugins/idrange.py
+++ b/ipaserver/plugins/idrange.py
@@ -35,6 +35,9 @@ if api.env.in_server and api.env.context in ['lite', 'server']:
 _dcerpc_bindings_installed = True
 except ImportError:
 _dcerpc_bindings_installed = False
+else:
+_dcerpc_bindings_installed = False
+
 
 ID_RANGE_VS_DNA_WARNING = """===
 WARNING:
-- 
2.5.5

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0004-0012] Automatic CSR generation

[Petr Spacek]

On 27.7.2016 19:06, Ben Lipton wrote:
> Hi all,
> 
> I think the automatic CSR generation feature
> (https://fedorahosted.org/freeipa/ticket/4899,
> http://www.freeipa.org/page/V4/Automatic_Certificate_Request_Generation) is
> stable enough to review now. The following are summaries of the attached 
> patches:
> 0004: LDAP schema changes for the new feature
> 0005: Basic API for new objects and CSR generation
> 0006: Update install automation to create some default mapping rules
> 0007: Implement the lookups and text processing that generates the CSR config
> 0008 and 0009: Implement some actual transformation rules so that the feature
> is usable
> 0010: Add a new cert profile for user certs, with mappings
> 0011: Implement import/export of cert profiles with mappings
> 0012: Tests for profile import/export
> 
> Generally speaking, later patches depend on earlier ones, but I don't
> anticipate any problems from committing earlier patches without later ones.
> 
> If you prefer, you can also comment on the pull request version:
> https://github.com/LiptonB/freeipa/pull/4. Note that I may force push on this
> branch.
> 
> Allocation of OIDs for schema change also needs review:
> https://code.engineering.redhat.com/gerrit/#/c/80061/
> 
> Known issues:
> - When the requested principal does not have some of the requested data,
> produces funny-looking configs with extra commas, empty sections, etc. They
> are still accepted by my copies of openssl and certutil, but they look ugly.
> - The new objects don't have any ACIs, so for the moment only admin can run
> the new commands.
> - Does not yet have support for prompting user for field values, so currently
> all data must come from the database.
> - All processing happens on the server side. As discussed in a previous
> thread, it would be desirable to break this out into a library so it could be
> used client-side.
> 
> Very excited to hear your thoughts!

Hi Ben,

I wanted to give it a try from user's point of view first, before diving into
implementation details. Here are some observations:

0. Design pages are using term "helper" and it is used even as option in the
example with smartcards. Please fix either design page or the code so they are
consistent.

1. The "ipa cert-request" command is missing options --autofill and --use (aka
helper aka format :-) which are mentioned in the design pages.

2. "ipa cert_get_requestdata" command passes even without --profile-id and
generates empty config. I think that this is not expected :-)

3. "ipa cert_get_requestdata --format=openssl" prints the text to stdout
including label "Configuration file contents:". This is hard to use because
simple redirection like "ipa cert_get_requestdata --format=openssl > config"
will not give you valid OpenSSL config file - it needs hand-editing.

It would be good to add --out parameter you envisioned in the design page.
Please ask jcholast for proper name and semantics :-) With --out option the
command can be used to generate valid config (or script if certutil was 
selected).

4. "ipa cert_get_requestdata --format=openssl" could print hint what OpenSSL
command should be executed on the generated config file. For testing I've used
command "openssl req -new -out csr -text -config config" (stolen and modified
from smart card example). Also, as a second hint, it could print the IPA
command which needs to be used to sign the CSR generated by the helper.

5. My naive attempt to get userCert for admin failed:
$ ipa cert_get_requestdata --format=openssl --principal=admin
--profile-id=userCert > usercert.conf
# remove the trailing label
$ openssl req -new -out csr -text -config usercert.conf
$ ipa cert-request --principal=admin --profile-id=userCert usercert.csr
ipa: ERROR: invalid 'csr': No Common Name was found in subject of request.

I'm attaching files generated by the commands above. I did not modify anything
in the templates or profiles, just tried to use the new profile added by
freeipa-blipton-0010-Add-a-new-cert-profile-for-users.patch .

Hopefully I will get to other things soon (but not this week).


Anyway, all this seems like (expected) initial problems. In general the first
touch with user interface seems reasonable and needs only small improvements.

Good work!

-- 
Petr^2 Spacek
[ req ]
prompt = no
encrypt_key = no

distinguished_name = sec0

[ sec0 ]
O=DOM-058-082.ABC.IDM.LAB.ENG.BRQ.REDHAT.COM
UID=admin



usercert.csr
Description: application/pkcs10
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0197] re-set canonical principal name on migrated users

[Simo Sorce]

On Fri, 2016-07-29 at 15:19 +0200, Martin Basti wrote:
> 
> On 29.07.2016 15:12, Simo Sorce wrote:
> > On Fri, 2016-07-29 at 15:10 +0200, Martin Basti wrote:
> >> On 29.07.2016 14:42, Florence Blanc-Renaud wrote:
> >>> On 07/28/2016 10:56 AM, Martin Babinsky wrote:
>  Fixes https://fedorahosted.org/freeipa/ticket/6101
> 
>  I have also noticed that the principal aliases are not preserved during
>  migration from FreeIPA 4.4.
> 
>  That, however, requires more powerful runes to transform the realm of
>  all values and warrants a separate ticket if we even want to support
>  migration of user aliases.
> 
> 
> 
> >>> Hi Martin,
> >>>
> >>> thanks for your patch. From a technical standpoint, it looks good to
> >>> me as I tested the following scenarios:
> >>>
> >>> 1/ without --user-ignore-attribute
> >>> - call ipa migrate-ds without specifying any attributes to ignore
> >>> The user entries are migrated, and contain a migrated krbprincipalname
> >>> and krbcanonicalname.
> >>> At this point kinit fails but this is expected as the krb attributes
> >>> were not re-generated. Login to the web https://hostname/ipa/ui also
> >>> fails as expected.
> >>> - login to https://hostname/ipa/migration with the user credentials
> >>> - perform kinit => OK
> >>> - login to https://hostname/ipa/ui => OK
> >>>
> >>> 2/ with --user-ignore-attribute={krbPrincipalName,krbextradata,...} as
> >>> explained in the Migration page [1]
> >>> At this point kinit fails as expected, as well as login to the web
> >>> ipa/ui.
> >>> - login to https://hostname/ipa/migration with the user credentials
> >>> - perform kinit => OK
> >>> - login to https://hostname/ipa/ui => OK
> >>>
> >>>
> >>> But the patch produces new pep8 complaints:
> >>> ./ipaserver/plugins/migration.py:39:1: E402 module level import not at
> >>> top of file
> >> This is caused by old code, it should not prevent this patch to be
> >> acked. Imports are heavily mixed in code already, it is not possible to
> >> keep importing right without fixing old ones.
> >> Martin^2
> > Can we make a patch to fix all import order issues across the code base
> > so we can maintain them properly going forward ?
> >
> > Simo.
> >
> 
> Is it worth it?
> 
> a) it will makes git blame harder, you will have to go always deeper to 
> history with any import

I considered this, but I rarely found that imports were such a big
issue, usually it's code in the file that is, so IMO the trade-off is
worth it.

> b) it makes backporting of patches harder. We fixed unused imports in 
> 4.4, and it was PITA to backport patches, always conflicts, several 
> bugs. Changing all import to correct order will be even worse.

Sure backports will be somewhat harder, but I wouldn't say it is a
nightmare, it is not code logic that changes, it is just individual
import lines.

> However plus is, when we once fix it, we can enable pylint check to keep 
> it right forever.

Exactly, this is the appealing point, we get it right once and then the
tool keeps us honest going forward.

> We can open refactoring ticket and we'll see.

Please do.

Simo.

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

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0197] re-set canonical principal name on migrated users

[Martin Basti]

On 29.07.2016 15:12, Simo Sorce wrote:

On Fri, 2016-07-29 at 15:10 +0200, Martin Basti wrote:

On 29.07.2016 14:42, Florence Blanc-Renaud wrote:

On 07/28/2016 10:56 AM, Martin Babinsky wrote:

Fixes https://fedorahosted.org/freeipa/ticket/6101

I have also noticed that the principal aliases are not preserved during
migration from FreeIPA 4.4.

That, however, requires more powerful runes to transform the realm of
all values and warrants a separate ticket if we even want to support
migration of user aliases.




Hi Martin,

thanks for your patch. From a technical standpoint, it looks good to
me as I tested the following scenarios:

1/ without --user-ignore-attribute
- call ipa migrate-ds without specifying any attributes to ignore
The user entries are migrated, and contain a migrated krbprincipalname
and krbcanonicalname.
At this point kinit fails but this is expected as the krb attributes
were not re-generated. Login to the web https://hostname/ipa/ui also
fails as expected.
- login to https://hostname/ipa/migration with the user credentials
- perform kinit => OK
- login to https://hostname/ipa/ui => OK

2/ with --user-ignore-attribute={krbPrincipalName,krbextradata,...} as
explained in the Migration page [1]
At this point kinit fails as expected, as well as login to the web
ipa/ui.
- login to https://hostname/ipa/migration with the user credentials
- perform kinit => OK
- login to https://hostname/ipa/ui => OK


But the patch produces new pep8 complaints:
./ipaserver/plugins/migration.py:39:1: E402 module level import not at
top of file

This is caused by old code, it should not prevent this patch to be
acked. Imports are heavily mixed in code already, it is not possible to
keep importing right without fixing old ones.
Martin^2

Can we make a patch to fix all import order issues across the code base
so we can maintain them properly going forward ?

Simo.



Is it worth it?

a) it will makes git blame harder, you will have to go always deeper to 
history with any import
b) it makes backporting of patches harder. We fixed unused imports in 
4.4, and it was PITA to backport patches, always conflicts, several 
bugs. Changing all import to correct order will be even worse.


However plus is, when we once fix it, we can enable pylint check to keep 
it right forever.


We can open refactoring ticket and we'll see.

Martin^2

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0197] re-set canonical principal name on migrated users

[Simo Sorce]

On Fri, 2016-07-29 at 15:10 +0200, Martin Basti wrote:
> 
> On 29.07.2016 14:42, Florence Blanc-Renaud wrote:
> > On 07/28/2016 10:56 AM, Martin Babinsky wrote:
> >> Fixes https://fedorahosted.org/freeipa/ticket/6101
> >>
> >> I have also noticed that the principal aliases are not preserved during
> >> migration from FreeIPA 4.4.
> >>
> >> That, however, requires more powerful runes to transform the realm of
> >> all values and warrants a separate ticket if we even want to support
> >> migration of user aliases.
> >>
> >>
> >>
> > Hi Martin,
> >
> > thanks for your patch. From a technical standpoint, it looks good to 
> > me as I tested the following scenarios:
> >
> > 1/ without --user-ignore-attribute
> > - call ipa migrate-ds without specifying any attributes to ignore
> > The user entries are migrated, and contain a migrated krbprincipalname 
> > and krbcanonicalname.
> > At this point kinit fails but this is expected as the krb attributes 
> > were not re-generated. Login to the web https://hostname/ipa/ui also 
> > fails as expected.
> > - login to https://hostname/ipa/migration with the user credentials
> > - perform kinit => OK
> > - login to https://hostname/ipa/ui => OK
> >
> > 2/ with --user-ignore-attribute={krbPrincipalName,krbextradata,...} as 
> > explained in the Migration page [1]
> > At this point kinit fails as expected, as well as login to the web 
> > ipa/ui.
> > - login to https://hostname/ipa/migration with the user credentials
> > - perform kinit => OK
> > - login to https://hostname/ipa/ui => OK
> >
> >
> > But the patch produces new pep8 complaints:
> > ./ipaserver/plugins/migration.py:39:1: E402 module level import not at 
> > top of file
> 
> This is caused by old code, it should not prevent this patch to be 
> acked. Imports are heavily mixed in code already, it is not possible to 
> keep importing right without fixing old ones.
> Martin^2

Can we make a patch to fix all import order issues across the code base
so we can maintain them properly going forward ?

Simo.

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

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0197] re-set canonical principal name on migrated users

[Martin Basti]

On 29.07.2016 14:42, Florence Blanc-Renaud wrote:

On 07/28/2016 10:56 AM, Martin Babinsky wrote:

Fixes https://fedorahosted.org/freeipa/ticket/6101

I have also noticed that the principal aliases are not preserved during
migration from FreeIPA 4.4.

That, however, requires more powerful runes to transform the realm of
all values and warrants a separate ticket if we even want to support
migration of user aliases.




Hi Martin,

thanks for your patch. From a technical standpoint, it looks good to 
me as I tested the following scenarios:


1/ without --user-ignore-attribute
- call ipa migrate-ds without specifying any attributes to ignore
The user entries are migrated, and contain a migrated krbprincipalname 
and krbcanonicalname.
At this point kinit fails but this is expected as the krb attributes 
were not re-generated. Login to the web https://hostname/ipa/ui also 
fails as expected.

- login to https://hostname/ipa/migration with the user credentials
- perform kinit => OK
- login to https://hostname/ipa/ui => OK

2/ with --user-ignore-attribute={krbPrincipalName,krbextradata,...} as 
explained in the Migration page [1]
At this point kinit fails as expected, as well as login to the web 
ipa/ui.

- login to https://hostname/ipa/migration with the user credentials
- perform kinit => OK
- login to https://hostname/ipa/ui => OK


But the patch produces new pep8 complaints:
./ipaserver/plugins/migration.py:39:1: E402 module level import not at 
top of file


This is caused by old code, it should not prevent this patch to be 
acked. Imports are heavily mixed in code already, it is not possible to 
keep importing right without fixing old ones.

Martin^2



Flo.


[1] 
https://www.freeipa.org/page/Howto/Migration#Migrating_from_other_FreeIPA_to_FreeIPA




--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH] 0078-82: webui tests: tests for new certificate widget

[Pavel Vomacka]

On 07/28/2016 08:16 AM, Lenka Doudova wrote:




On 07/20/2016 04:51 PM, Pavel Vomacka wrote:
Please review attached patches, which add tests for new certificate 
widget in WebUI.


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




Hi,
thanks for patches.
Functionally ok, but you have lots of PEP8 errors in patches 78, 80, 
81 and 82 -> NACK.
Also in patch 82, method test_arbitrary_certificate, comment says user 
needs to have "arbitrary_cert" configured, but the property in config 
file is correctly "arbitrary_cert_path", so it's a bit misleading.


Patch 79 is OK, ACK.

Lenka


Thank you for review. Attaching patches which have fixed all pep8 erros. 
Bad property of config file was also mentioned in patch 81. These are 
also fixed.


--
Pavel^3 Vomacka

From d0731af7431ecba96b5a7df943bbd97b1a7bb5b2 Mon Sep 17 00:00:00 2001
From: tester 
Date: Wed, 20 Jul 2016 12:59:27 +0200
Subject: [PATCH 1/5] Add possibility to choose parent element by css

Part of: https://fedorahosted.org/freeipa/ticket/6064
---
 ipatests/test_webui/ui_driver.py | 43 +++-
 1 file changed, 34 insertions(+), 9 deletions(-)

diff --git a/ipatests/test_webui/ui_driver.py b/ipatests/test_webui/ui_driver.py
index 7c4ca75efa3e642f4a2c0cdcd72be3cafa3c305a..f7f990a605ab8e1b595d457732be9170fa0f4130 100644
--- a/ipatests/test_webui/ui_driver.py
+++ b/ipatests/test_webui/ui_driver.py
@@ -615,12 +615,17 @@ class UI_driver(object):
 s = "a[name='%s'].action-button" % name
 self._button_click(s, parent, name)
 
-def button_click(self, name, parent=None):
+def button_click(self, name, parent=None,
+ parents_css_sel=None):
 """
 Click on .ui-button
 """
 if not parent:
-parent = self.get_form()
+if parents_css_sel:
+parent = self.find(parents_css_sel, By.CSS_SELECTOR,
+   strict=True)
+else:
+parent = self.get_form()
 
 s = "[name='%s'].btn" % name
 self._button_click(s, parent, name)
@@ -1413,14 +1418,25 @@ class UI_driver(object):
 for key in pkeys:
 self.assert_record(key, parent, table_name, negative=True)
 
-def action_list_action(self, name, confirm=True, confirm_btn="ok"):
+def action_list_action(self, name, confirm=True, confirm_btn="ok",
+   parents_css_sel=None):
 """
 Execute action list action
 """
-cont = self.find(".active-facet .facet-actions", By.CSS_SELECTOR, strict=True)
-expand = self.find(".dropdown-toggle", By.CSS_SELECTOR, cont, strict=True)
+context = None
+
+if not parents_css_sel:
+context = self.find(".active-facet .facet-actions",
+By.CSS_SELECTOR, strict=True)
+else:
+context = self.find(parents_css_sel, By.CSS_SELECTOR,
+strict=True)
+
+expand = self.find(".dropdown-toggle", By.CSS_SELECTOR, context,
+   strict=True)
 expand.click()
-action_link = self.find("li[data-name=%s] a" % name, By.CSS_SELECTOR, cont, strict=True)
+action_link = self.find("li[data-name=%s] a" % name, By.CSS_SELECTOR,
+context, strict=True)
 action_link.click()
 if confirm:
 self.wait(0.5)  # wait for dialog
@@ -1739,17 +1755,26 @@ class UI_driver(object):
 assert is_enabled == enabled, ('Invalid enabled state of action button %s. '
'Expected: %s') % (action, str(visible))
 
-def assert_action_list_action(self, action, visible=True, enabled=True, parent=None):
+def assert_action_list_action(self, action, visible=True, enabled=True,
+  parent=None, parents_css_sel=None,
+  facet_actions=True):
 """
 Assert that action dropdown action is visible/hidden, and enabled/disabled
 
 Enabled is checked only if action is visible.
 """
+
+li_s = " li[data-name='%s']" % action
+
 if not parent:
 parent = self.get_form()
 
-s = ".facet-actions li[data-name='%s']" % action
-li = self.find(s, By.CSS_SELECTOR, parent)
+if facet_actions:
+li_s = ".facet-actions" + li_s
+else:
+li_s = parents_css_sel + li_s
+
+li = self.find(li_s, By.CSS_SELECTOR, parent)
 link = self.find("a", By.CSS_SELECTOR, li)
 
 is_visible = li is not None and link is not None
-- 
2.7.4

From 22c0f8c5b2cd961795770662d22b3bb35573eb07 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka 
Date: Wed, 20 Jul 2016 16:28:38 +0200
Subject: [PATCH 6/9] Add function which check whether the field is empty

Part of: https://fedorahosted.org/freeipa/ticket/6064
---
 

[Freeipa-devel] [PATCH] webui: Fix coverity bugs

[Pavel Vomacka]

Hello,

please review attached patches which fixes errors from Coverity.

--
Pavel^3 Vomacka

From 0391289b3f6844897e2a9f3ae549bd4c33233ffc Mon Sep 17 00:00:00 2001
From: Pavel Vomacka 
Date: Mon, 25 Jul 2016 10:36:47 +0200
Subject: [PATCH 01/13] Coverity - null pointer exception

Variable 'option' can be null and there will be error of reading property of null.
---
 install/ui/src/freeipa/widget.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/install/ui/src/freeipa/widget.js b/install/ui/src/freeipa/widget.js
index 9151ebac9438e9e674f81bfb1ccfe7a63872b1ae..cfdf5d4750951e4549c16a2b9b9c355f61e90c39 100644
--- a/install/ui/src/freeipa/widget.js
+++ b/install/ui/src/freeipa/widget.js
@@ -2249,7 +2249,7 @@ IPA.option_widget_base = function(spec, that) {
 var child_values = [];
 var option = that.get_option(value);
 
-if (option.widget) {
+if (option && option.widget) {
 child_values = option.widget.save();
 values.push.apply(values, child_values);
 }
-- 
2.5.5

From 6df8e608232e25daa9aefe4fccbdeca4dbaf1998 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka 
Date: Mon, 25 Jul 2016 10:43:00 +0200
Subject: [PATCH 02/13] Coverity - null pointer exception

Variable 'row' could be null in some cases. And set css to variable which is pointing to null
causes error. Therefore there is new check.
---
 install/ui/src/freeipa/widget.js | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/install/ui/src/freeipa/widget.js b/install/ui/src/freeipa/widget.js
index cfdf5d4750951e4549c16a2b9b9c355f61e90c39..5844436abf090f12d5a9d65efe7a1aaee14097e2 100644
--- a/install/ui/src/freeipa/widget.js
+++ b/install/ui/src/freeipa/widget.js
@@ -5766,6 +5766,8 @@ exp.fluid_layout = IPA.fluid_layout = function(spec) {
 that.on_visible_change = function(event) {
 
 var row = that._get_row(event);
+if (!row) return;
+
 if (event.visible) {
 row.css('display', '');
 } else {
-- 
2.5.5

From 6f2ddc9e1c5323a640bdf744d2da00bfee7ab766 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka 
Date: Mon, 25 Jul 2016 13:48:16 +0200
Subject: [PATCH 03/13] Coverity - not initialized variable

The variable hasn't been initialized, now it is set to null by default.
---
 install/ui/src/freeipa/widget.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/install/ui/src/freeipa/widget.js b/install/ui/src/freeipa/widget.js
index 5844436abf090f12d5a9d65efe7a1aaee14097e2..43804c5ea524ca741017d02f6e12ccf60d50b5df 100644
--- a/install/ui/src/freeipa/widget.js
+++ b/install/ui/src/freeipa/widget.js
@@ -1047,7 +1047,7 @@ IPA.multivalued_widget = function(spec) {
 
 that.child_spec = spec.child_spec;
 that.size = spec.size || 30;
-that.undo_control;
+that.undo_control = null;
 that.initialized = true;
 that.updating = false;
 
-- 
2.5.5

From b9ddd32ec45aadae5a79e372c3e1b70990071e60 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka 
Date: Mon, 25 Jul 2016 14:42:50 +0200
Subject: [PATCH 04/13] Coverity - identical code for different branches

In both cases when the condition is true or false ut is set the same value.
Changed to assign the value directly.
---
 install/ui/src/freeipa/topology_graph.js | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/install/ui/src/freeipa/topology_graph.js b/install/ui/src/freeipa/topology_graph.js
index ce2ebeaff611987ae27f2655b5da80bdcd1b4f8a..712d38fbe67e87ffa773e0a3a1f8937e9595c9a6 100644
--- a/install/ui/src/freeipa/topology_graph.js
+++ b/install/ui/src/freeipa/topology_graph.js
@@ -325,8 +325,8 @@ topology_graph.TopoGraph = declare([Evented], {
 off = dir ? -1 : 1, // determines shift direction of curve
 ns = 5, // shift on normal vector
 s = target_count > 1 ? 1 : 0, // shift from center?
-spad = d.left ? 18 : 18, // source padding
-tpad = d.right ? 18 : 18, // target padding
+spad = d.left = 18, // source padding
+tpad = d.right = 18, // target padding
 sourceX = d.source.x + (spad * ux) + off * nx * ns * s,
 sourceY = d.source.y + (spad * uy) + off * ny * ns * s,
 targetX = d.target.x - (tpad * ux) + off * nx * ns * s,
-- 
2.5.5

From f1f2b55247d6c7f41f8053f372a47945c93fc8a4 Mon Sep 17 00:00:00 2001
From: Pavel Vomacka 
Date: Mon, 25 Jul 2016 14:52:15 +0200
Subject: [PATCH 05/13] Coverity - Accesing attribute of null

There is a possibility that widget is null and then there could be an error.
Therefore there is new check of widget variable.
---
 install/ui/src/freeipa/widgets/APIBrowserWidget.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/install/ui/src/freeipa/widgets/APIBrowserWidget.js 

Re: [Freeipa-devel] [PATCH 0197] re-set canonical principal name on migrated users

[Florence Blanc-Renaud]

On 07/28/2016 10:56 AM, Martin Babinsky wrote:

Fixes https://fedorahosted.org/freeipa/ticket/6101

I have also noticed that the principal aliases are not preserved during
migration from FreeIPA 4.4.

That, however, requires more powerful runes to transform the realm of
all values and warrants a separate ticket if we even want to support
migration of user aliases.




Hi Martin,

thanks for your patch. From a technical standpoint, it looks good to me 
as I tested the following scenarios:


1/ without --user-ignore-attribute
- call ipa migrate-ds without specifying any attributes to ignore
The user entries are migrated, and contain a migrated krbprincipalname 
and krbcanonicalname.
At this point kinit fails but this is expected as the krb attributes 
were not re-generated. Login to the web https://hostname/ipa/ui also 
fails as expected.

- login to https://hostname/ipa/migration with the user credentials
- perform kinit => OK
- login to https://hostname/ipa/ui => OK

2/ with --user-ignore-attribute={krbPrincipalName,krbextradata,...} as 
explained in the Migration page [1]

At this point kinit fails as expected, as well as login to the web ipa/ui.
- login to https://hostname/ipa/migration with the user credentials
- perform kinit => OK
- login to https://hostname/ipa/ui => OK


But the patch produces new pep8 complaints:
./ipaserver/plugins/migration.py:39:1: E402 module level import not at 
top of file


Flo.


[1] 
https://www.freeipa.org/page/Howto/Migration#Migrating_from_other_FreeIPA_to_FreeIPA


--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0003] Test validity of URIs in certificate

[Lenka Doudova]

On 07/29/2016 11:41 AM, Lenka Doudova wrote:


On 07/28/2016 01:35 PM, Peter Lacko wrote:

Hops, fixed.

Peter


- Original Message -
From: "Lenka Doudova"
To:freeipa-devel@redhat.com
Sent: Thursday, July 28, 2016 1:32:25 PM
Subject: Re: [Freeipa-devel] [PATCH 0003] Test validity of URIs in  
certificate

Hi,

I cannot find any attached patch :)

Lenka


On 07/28/2016 01:30 PM, Peter Lacko wrote:

Attached you can find a patch adding test for URIs in generated certificate
ipatests/test_xmlrpc/test_cert_plugin.py
Since I'm leaving Red Hat in end of July, I won't be able to modify this patch 
anymore.

Regards,

Peter





Hi,

NACK. Code looks fine and works well on master branch, but patch does 
not apply on 4-3 and 4-2 branches.
Peter left the company but claimed he can fix the patch if necessary, 
I'll communicate it with him or fix it myself.


Lenka



Oh, and forgot this one - PEP8 error:
./ipatests/test_xmlrpc/test_cert_plugin.py:191:80: E501 line too long 
(105 > 79 characters)


Lenka
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0003] Test validity of URIs in certificate

[Lenka Doudova]

On 07/28/2016 01:35 PM, Peter Lacko wrote:

Hops, fixed.

Peter


- Original Message -
From: "Lenka Doudova" 
To: freeipa-devel@redhat.com
Sent: Thursday, July 28, 2016 1:32:25 PM
Subject: Re: [Freeipa-devel] [PATCH 0003] Test validity of URIs in  
certificate

Hi,

I cannot find any attached patch :)

Lenka


On 07/28/2016 01:30 PM, Peter Lacko wrote:

Attached you can find a patch adding test for URIs in generated certificate
ipatests/test_xmlrpc/test_cert_plugin.py
Since I'm leaving Red Hat in end of July, I won't be able to modify this patch 
anymore.

Regards,

Peter





Hi,

NACK. Code looks fine and works well on master branch, but patch does 
not apply on 4-3 and 4-2 branches.
Peter left the company but claimed he can fix the patch if necessary, 
I'll communicate it with him or fix it myself.


Lenka
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0027][Tests] Fix failing automember tests in 4.3

[Martin Basti]

On 29.07.2016 09:13, Lenka Doudova wrote:

On 07/28/2016 06:08 PM, Ganna Kaihorodova wrote:

Greetings!

ACK

Best regards,
Ganna Kaihorodova
Associate Software Quality Engineer


- Original Message -
From: "Lenka Doudova" 
To: "freeipa-devel" 
Sent: Wednesday, July 13, 2016 3:21:25 PM
Subject: [Freeipa-devel] [PATCH 0027][Tests] Fix failing automember 
tests in4.3


Hi,

providing patch to fix two failing automember tests in 4.3 branch. The
reason of the failure was the output normalization (specifically manager
attribute for user).

The patch is intended for ipa-4-3 branch only.


Lenka



Hi,

minor fix to the patch - added link to ticket to commit message.

Lenka



Pushed to ipa-4-3: ffe146fbba2e9577a9af5dd1521a110570024455

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] certmonger EST RFC7030 support possible ?

[Marx, Peter]

Hi,



we are using certmonger with SCEP. But SCEP does not support Elliptic curve 
keys, only RSA.
The successor protocol EST (Enrollment over Secure Transport) would support ECC.

Is a EST helper for certmonger/getcert on the roadmap ?
If yes, when ?
How complicated is it to create such a helper around the Cisco open-sourced 
libest ?

Peter

Knorr-Bremse IT-Services GmbH
Sitz: Muenchen
Geschaeftsfuehrer: Helmut Draxler (Vorsitzender), Harald Jessen, Harald 
Schneider
Registergericht Muenchen, HR B 167 268

This transmission is intended solely for the addressee and contains 
confidential information.
If you are not the intended recipient, please immediately inform the sender and 
delete the message and any attachments from your system.
Furthermore, please do not copy the message or disclose the contents to anyone 
unless agreed otherwise. To the extent permitted by law we shall in no way be 
liable for any damages, whatever their nature, arising out of transmission 
failures, viruses, external influence, delays and the like.
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0027][Tests] Fix failing automember tests in 4.3

[Lenka Doudova]

On 07/28/2016 06:08 PM, Ganna Kaihorodova wrote:

Greetings!

ACK

Best regards,
Ganna Kaihorodova
Associate Software Quality Engineer


- Original Message -
From: "Lenka Doudova" 
To: "freeipa-devel" 
Sent: Wednesday, July 13, 2016 3:21:25 PM
Subject: [Freeipa-devel] [PATCH 0027][Tests] Fix failing automember tests in
4.3

Hi,

providing patch to fix two failing automember tests in 4.3 branch. The
reason of the failure was the output normalization (specifically manager
attribute for user).

The patch is intended for ipa-4-3 branch only.


Lenka



Hi,

minor fix to the patch - added link to ticket to commit message.

Lenka
From 94b5b6fa52ac8e5984792f47fec06241a383bb51 Mon Sep 17 00:00:00 2001
From: Lenka Doudova 
Date: Wed, 13 Jul 2016 15:14:11 +0200
Subject: [PATCH] Tests: Fix failing automember tests

Two tests in xmlrpc/automember suite were failing as a result of manager data normalization in user attributes. Tests are fixed to reflect the change.

https://fedorahosted.org/freeipa/ticket/6147
---
 ipatests/test_xmlrpc/test_automember_plugin.py | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/ipatests/test_xmlrpc/test_automember_plugin.py b/ipatests/test_xmlrpc/test_automember_plugin.py
index be0f7390565ed739aa66bc0c5c6d23d25d67df92..dde386286d26ddf4537fbc89647f1afecf51fcad 100644
--- a/ipatests/test_xmlrpc/test_automember_plugin.py
+++ b/ipatests/test_xmlrpc/test_automember_plugin.py
@@ -472,8 +472,7 @@ class test_automember(Declarative):
 summary=u'Added user "tuser1"',
 result=get_user_result(
 user1, u'Test', u'User1', 'add',
-manager=[DN(('uid', 'mscott'), ('cn', 'users'),
-('cn', 'accounts'), api.env.basedn)]
+manager=[manager1]
 )
 ),
 ),
@@ -1394,8 +1393,7 @@ class test_automember(Declarative):
 result=get_user_result(
 user1, u'Test', u'User1', 'add',
 memberof_group=[group1, u'ipausers'],
-manager=[DN(('uid', 'mscott'), ('cn', 'users'),
-('cn', 'accounts'), api.env.basedn)]
+manager=[manager1]
 ),
 ),
 ),
-- 
2.7.4

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 42-44, 48-51][tests] RFE: Allow users to authenticate with alternative names

[Martin Basti]

On 28.07.2016 18:10, Martin Babinsky wrote:

On 07/28/2016 01:44 PM, Milan Kubík wrote:

On 07/28/2016 12:51 PM, Martin Babinsky wrote:

On 07/27/2016 11:54 AM, Milan Kubík wrote:







Hi Milan,

the tests seem to work as expected except
`test_enterprise_principal_UPN_overlap_without_additional_suffix`
which crashes on #6099. I have a few comments, however:

This is a test that hits a known bug. I have added an expected fail
marker for it.


Patch 42:

1.)
+class KerberosAliasMixin:

make sure the class inherits from 'object' so that it behaves like
new-style class in Python 2.

Also, I think that 'check_for_tracker' method is slightly redundant.
If somebody would use the mixin directly, then he will still get
"NotImplementedError" exceptions and see that he probably did
something wrong.

If you really really want to treat to prevent the instantiation of 
the

class, then use ABC module to turn it into proper abstract class.

But in this case it should IMHO be enough that you explained the 
class

usage in the module docstring.

Ok. Fixed the inheritance and removed the check for Tracker. The check
for krbprincipalname attribute has been kept.


2.)
+def _make_add_alias_cmd(self):
+raise RuntimeError("The _make_add_alias_cmd method "
+   "is not implemented.")
+
+def _make_remove_alias_cmd(self):
+raise RuntimeError("The _make_remove_alias_cmd method "
+   "is not implemented."

Abstract methods should raise "NotImplementedError" exception.


Fixed.

3.)
is the 'options=None' kwarg in {add,remove}_principals methods
necessary? I didn't see it anywhere in the later commits so I guess
you can safely remove it. Better yet, you can just replace it with
'**options' since all it does is to pass options to the
`*-add/remove-principal` commands.


Fixed to **options.

4.)
a nitpick but IIRC the mixin class should be listed before the actual
hierarchy base so that MRO works as expected.

So instead

+class UserTracker(Tracker, KerberosAliasMixin):

It should say
+class UserTracker(KerberosAliasMixin, Tracker):

Fixed in all three classes.


PATCH 43: LGTM

PATCH 44:

Please do not create any more utility modules. Either put it to
existing 'ipatests/util.py' or better yet, rename the module to
something like 'mock_trust' since the scope of it is to provide 
mocked

trust entries.
Moved the functions from test_range_plugin.py module to a new 
mock_trust
module. The fix for the range plugin test introduced a new commit 
here.


PATCH 45:

It would be nice if you could add '-E' option in the same way to
indicate enterprise principal name resolution.

Done.


PATCH 46: LGTM
PATCH 47:

1.)
+from ipatests.test_xmlrpc.test_range_plugin import (
+get_trust_dn, get_trusted_dom_dict, encode_mockldap_value)

Since you already introduced a module grouping all trust-mocking
machinery in patch 44, you could extract these functions and put it
there to improve code reuse.

Fixed.


2.)
I am not a big fan of duplicate code in 'trusted_domain' and
'trusted_domain_with_suffix' fixtures. Use some module-level mapping
for common attributes and add more specific attributes per fixture.

Fixed


3.)
I would like to expand the test cases for AD realm namespace overlap
checks. We should reject enterprise principals with suffixes
coinciding with realm names, NETBIOS names, and UPN suffixes and I
would like to test all three cases to get a full coverage.


Extended with explicit checks fot rhe AD realm and NETBIOS name by
constructing the enterprise principal from corresponding LDAP
attributes.

The fixed and rebased version of the commits is in my repo [1].

[1]: 
https://github.com/apophys/freeipa/tree/krb5-principal-aliases-test


Regards



Tests works and code is ok, however you will need to create a separate
ticket to your patches, since it is not allowed to add fixes to
tickets in closed milestones. Sorry that I didn't notice it earlier.

cond-ACK if you create ticket and remove the xfail from
`test_enterprise_principal_overlap_with_AD_realm` test case as the fix
for #6099 was pushed to master.



Hi,

thanks for the review. The xfail marker was removed. The commit messages
now reffer to new ticket [1].
Since the changes during review introduced new commit in the sequence, I
have abandoned the patches 45 to 47 and renumbered them (with the new
one) from 48 onwards.

[1]: https://fedorahosted.org/freeipa/ticket/6142

Regards



Thanks, ACK.



master:
* 5582d1df3213a0727e313d543ee560a09d0cdff8 ipatests: Add tracker class 
for kerberos principal aliases
* dde1240f5d71f3a8c50226a720af6f1000a35be1 ipatests: Extend the MockLDAP 
utility class
* 7c03708734ad7cb8f1a6edd39817212794b5aabd ipatests: Provide a context 
manager for mocking a trust in RPC tests
* ddb7a08084d69a119abdd39a3c82113bb8586db6 ipatests: Move trust mock 
helper functions to a separate module
* 8e83b9715a04fab8d7864b6e02e1210df885119c ipapython: Extend 
kinit_password to support principal