Re: [Freeipa-devel] [PATCH 0559] Increase default length of auto-generated passwords
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'] = ipa_generate_password( > > +
Re: [Freeipa-devel] [PATCH 0559] Increase default length of auto-generated passwords
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 81beb652bc81a8e73876f876507a7dabd338667b M
Re: [Freeipa-devel] [PATCH 0004-0012] Automatic CSR generation
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
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 ?
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
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: https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0558] idrange: fix unassigned global variable
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
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
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
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
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] webui: Fix coverity bugs
On Fri, 29 Jul 2016, Pavel Vomacka wrote: 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 ACK 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 ACK 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 ACK 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 ACK 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 b/install/ui/src/freeipa/widgets/APIBrowserWidget.js index 1a3726190d4a5d62
Re: [Freeipa-devel] [PATCH 0197] re-set canonical principal name on migrated users
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
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
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
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
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 --- ipatests/test_webui/ui_driver.py | 11 +
[Freeipa-devel] [PATCH] webui: Fix coverity bugs
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 b/install/ui/src/freeipa/widgets/APIBrowserWidget.js index 1a3726190d4a5d628a8f7c2b564c4c9f6e7cea1f..50c2989fcc126585787df61cd
Re: [Freeipa-devel] [PATCH 0197] re-set canonical principal name on migrated users
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
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
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
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 ?
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
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
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 canon