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

[Fraser Tweedale]

On Fri, Jul 29, 2016 at 11:13:16AM -0400, Ben Lipton wrote:
> 
> 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.
>
In general use I think that 'caIPAserviceCert' is unlikely to be
used a majory of the time, and it is a new command so there are no
compatibility issues; therefore why not make the profile option
mandatory?

> > 
> > 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 

Re: [Freeipa-devel] [Test][Patch-0049, 0050] Certs in ID overrides test

[Oleg Fayans]

The test was redesigned so that it actually tests against an AD user. 
cleanly applies, passes lint and passes


https://paste.fedoraproject.org/399504/00843641/


On 06/28/2016 01:40 PM, Oleg Fayans wrote:

Patch-0050 rebased against latest upstream branch

On 06/28/2016 10:45 AM, Oleg Fayans wrote:

Passing test output:

https://paste.fedoraproject.org/385774/71035231/









--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.

--
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] 0001 six.u function instead of the decode

[Petr Spacek]

On 1.8.2016 18:31, Martin Basti wrote:
> 
> 
> On 28.07.2016 18:29, Ariel Barria wrote:
>> 2016-07-28 7:10 GMT-05:00 Petr Spacek :
>>> On 27.7.2016 18:26, Ariel Barria wrote:
 2016-07-26 9:39 GMT-05:00 Petr Spacek :
> On 26.7.2016 16:28, Jan Cholasta wrote:
>> Hi,
>>
>> On 26.7.2016 16:09, Martin Basti wrote:
>>>
>>> On 22.07.2016 00:14, Ariel Barria wrote:
 Hello everyone.

 I send patch for review.
>> NACK, six.u() is supposed to be used on string literals *only* [1].
>>
>> A proper fix would be something like:
>>
>>  value = self.to_text()
>>  if not isinstance(value, unicode):
>>  value = value.decode('ascii')
>>  return value
 thanks for the guidance and comments

> Most importantly, we should fix/provide this method in python-dns and
> inherit
> this method from there.
 Well, I made a pr, but apparently travis ci test failed with other
 versions of python, so it is possible that they do not approve, I will
 be performing other tests to see what the downside.

 https://github.com/rthalley/dnspython/pull/195
>>> Looking at the PR, there are functions
>>> dns.name.to_text()
>>> dns.name.to_unicode()
>>>
>>> What is missing in them?
>>>
>>> Petr^2 Spacek
>>>
>>>
>>> I will look on this, for some reason we received your e-mail just today
>>> (2016-07-26)
 welcome.
 it was my mistake, I sent the patch to the list before being signed to the
 list

>> Honza
>>
>> [1] 
>>> -- 
>>> Petr^2 Spacek
>> Hi.
>> I send the requested changes
>> thanks for review.
>>
>>
> 
> According the 
> https://github.com/rthalley/dnspython/blob/master/dns/name.py#L375
> 
> New dnspython always return binary strings, so we should always decode it,
> because IPA internals need punycoded domain in unicode format
> 
> IMO this is broken in current dnspython released in Fedora. There are plenty
> of py3 bugs, we have to provide newer dnspython package to fedora, so this
> should not be fixed on IPA side.

I'm fine with rebasing python-dns in Fedora when we find a version which works
better. Just tell me what version it is :-)

-- 
Petr^2 Spacek

-- 
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] 0001 six.u function instead of the decode

[Martin Basti]

On 28.07.2016 18:29, Ariel Barria wrote:

2016-07-28 7:10 GMT-05:00 Petr Spacek :

On 27.7.2016 18:26, Ariel Barria wrote:

2016-07-26 9:39 GMT-05:00 Petr Spacek :

On 26.7.2016 16:28, Jan Cholasta wrote:

Hi,

On 26.7.2016 16:09, Martin Basti wrote:


On 22.07.2016 00:14, Ariel Barria wrote:

Hello everyone.

I send patch for review.

NACK, six.u() is supposed to be used on string literals *only* [1].

A proper fix would be something like:

 value = self.to_text()
 if not isinstance(value, unicode):
 value = value.decode('ascii')
 return value

thanks for the guidance and comments


Most importantly, we should fix/provide this method in python-dns and inherit
this method from there.

Well, I made a pr, but apparently travis ci test failed with other
versions of python, so it is possible that they do not approve, I will
be performing other tests to see what the downside.

https://github.com/rthalley/dnspython/pull/195

Looking at the PR, there are functions
dns.name.to_text()
dns.name.to_unicode()

What is missing in them?

Petr^2 Spacek



I will look on this, for some reason we received your e-mail just today
(2016-07-26)

welcome.
it was my mistake, I sent the patch to the list before being signed to the list


Honza

[1] 

--
Petr^2 Spacek

Hi.
I send the requested changes
thanks for review.




According the 
https://github.com/rthalley/dnspython/blob/master/dns/name.py#L375


New dnspython always return binary strings, so we should always decode 
it, because IPA internals need punycoded domain in unicode format


IMO this is broken in current dnspython released in Fedora. There are 
plenty of py3 bugs, we have to provide newer dnspython package to 
fedora, so this should not be fixed on IPA side.


regards
Martin
-- 
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

[Petr Vobornik]

On 07/29/2016 03:25 PM, Alexander Bokovoy wrote:
> 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

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


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

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

NACK

following lines are not 

[Freeipa-devel] [PATCH 0153] Fix ipa-replica-prepare's error message about missing local CA instanc

[Petr Spacek]

Hello,

Fix ipa-replica-prepare's error message about missing local CA instance

ipa-replica-prepare must be run on a replica with CA or all the certs
needs to be provided (for CA-less case).

The old messages were utterly confusing because they mixed errors about
missing certs and missing local CA instance into one text.

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

-- 
Petr^2 Spacek
From c47c6966107f7d913137667cb9164f5f43c5daaa Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Mon, 1 Aug 2016 17:32:04 +0200
Subject: [PATCH] Fix ipa-replica-prepare's error message about missing local
 CA instance

ipa-replica-prepare must be run on a replica with CA or all the certs
needs to be provided (for CA-less case).

The old messages were utterly confusing because they mixed errors about
missing certs and missing local CA instance into one text.

https://fedorahosted.org/freeipa/ticket/6134
---
 ipaserver/install/ipa_replica_prepare.py | 11 ---
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/ipaserver/install/ipa_replica_prepare.py b/ipaserver/install/ipa_replica_prepare.py
index fdd32f0c8437a0d8c3947d57089662ea09bb2304..49c4552a80540e4b7c1242bdaf9dc583259d7149 100644
--- a/ipaserver/install/ipa_replica_prepare.py
+++ b/ipaserver/install/ipa_replica_prepare.py
@@ -236,6 +236,10 @@ class ReplicaPrepare(admintool.AdminTool):
 except errors.DatabaseError as e:
 raise admintool.ScriptError(e.desc)
 
+if ca_enabled and not ipautil.file_exists(paths.CA_CS_CFG_PATH):
+raise admintool.ScriptError(
+"CA is not installed on this server. "
+"ipa-replica-prepare must be ran on an IPA server with CA.")
 if not ca_enabled and not options.http_cert_files:
 raise admintool.ScriptError(
 "Cannot issue certificates: a CA is not installed. Use the "
@@ -347,13 +351,6 @@ class ReplicaPrepare(admintool.AdminTool):
 "Apache Server SSL certificate and Directory Server SSL "
  "certificate are not signed by the same CA certificate")
 
-if (not ipautil.file_exists(paths.CA_CS_CFG_PATH) and
-options.dirsrv_pin is None):
-self.log.info("If you installed IPA with your own certificates "
-"using PKCS#12 files you must provide PKCS#12 files for any "
-"replicas you create as well.")
-raise admintool.ScriptError("The replica must be created on the "
-"primary IPA server.")
 
 def run(self):
 options = self.options
-- 
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 0151-0152] install: Call hostnamectl set-hostname only if --hostname option is use server-install: Fix --hostname option to always override api.env value

[Petr Spacek]

On 1.8.2016 08:27, Jan Cholasta wrote:
> On 28.7.2016 16:55, Petr Spacek wrote:
>> On 28.7.2016 16:44, Jan Cholasta wrote:
>>> On 28.7.2016 16:37, Petr Spacek wrote:
 On 28.7.2016 16:35, Jan Cholasta wrote:
> On 28.7.2016 16:20, Petr Spacek wrote:
>> Hello,
>>
>> install: Call hostnamectl set-hostname only if --hostname option is used
>>
>> This commit also splits hostname backup and configuration into two 
>> separate
>> functions. This allows us to backup hostname without setting it at the
>> same time.
>>
>> https://fedorahosted.org/freeipa/ticket/6071
>
> Note that you can set ca_host in cfg unconditionally, the value is only
> valid
> during install and is not written anywhere.

 I prefer not to set it so the code blows up when we attempt to touch
 variables
 we should reference in particular setups. I.e. Take this as a first step
 towards api.env without invalid values :-)
>>>
>>> OK. Use the proper condition then ("if setup_ca:").
>>
>> Oh, I'm probably blind. Here is revised version.
> 
> But you used "if not setup_ca:" rather than "if setup_ca:" :-)

This patch set is cursed!

Petr^2 Spacek

> 
>>
>> Petr^2 Spacek
>>
>>>

 (In my stash I have a patch which removes nonsense defaults from
 ipalib.constants. To be pushed when we a new git branch for 4.4...)

 Petr^2 Spacek

>> server-install: Fix --hostname option to always override api.env values
>>
>> Attempts to compare local hostname with user-provided values are error
>> prone as we found out in #5794. This patch removes comparison and makes
>> the env values deterministic.
>>
>> https://fedorahosted.org/freeipa/ticket/6071
>>
>>
>> Jan, this patch set should fix problems you have seen in containers.
> 
> 


-- 
Petr^2 Spacek
From 16c62d99391ec2efff93e0652bf00ecb72f37a42 Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Tue, 12 Jul 2016 17:42:40 +0200
Subject: [PATCH] server-install: Fix --hostname option to always override
 api.env values

Attempts to compare local hostname with user-provided values are error
prone as we found out in #5794. This patch removes comparison and makes
the env values deterministic.

https://fedorahosted.org/freeipa/ticket/6071
---
 ipaserver/install/server/install.py | 13 +
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/ipaserver/install/server/install.py b/ipaserver/install/server/install.py
index 65f9318201e648b30a3c13626e807ac6f3a9416d..1e925595b93ff95a98b3e6c3d0c357b1766dc1dc 100644
--- a/ipaserver/install/server/install.py
+++ b/ipaserver/install/server/install.py
@@ -560,7 +560,12 @@ def install_check(installer):
 cfg = dict(
 context='installer',
 in_server=True,
+# make sure host name specified by user is used instead of default
+host=host_name,
 )
+if setup_ca:
+# we have an IPA-integrated CA
+cfg['ca_host'] = host_name
 
 # Create the management framework config file and finalize api
 target_fname = paths.IPA_DEFAULT_CONF
@@ -586,14 +591,6 @@ def install_check(installer):
 # Must be readable for everyone
 os.chmod(target_fname, 0o644)
 
-system_hostname = get_fqdn()
-if host_name != system_hostname:
-root_logger.debug("Chosen hostname (%s) differs from system hostname "
-  "(%s) - change it" % (host_name, system_hostname))
-# update `api.env.ca_host` to correct hostname
-# https://fedorahosted.org/freeipa/ticket/4936
-api.env.ca_host = host_name
-
 api.bootstrap(**cfg)
 api.finalize()
 
-- 
2.7.4

From 719db115f1e02e1f1e635f3f8c16b234458d0703 Mon Sep 17 00:00:00 2001
From: Petr Spacek 
Date: Thu, 28 Jul 2016 16:13:55 +0200
Subject: [PATCH] install: Call hostnamectl set-hostname only if --hostname
 option is used

This commit also splits hostname backup and configuration into two separate
functions. This allows us to backup hostname without setting it at the
same time.

https://fedorahosted.org/freeipa/ticket/6071
---
 client/ipa-client-install   |  3 ++-
 doc/guide/guide.org | 10 +-
 ipaplatform/base/tasks.py   |  7 ++-
 ipaplatform/redhat/tasks.py | 13 ++---
 ipaserver/install/server/install.py | 10 +-
 5 files changed, 16 insertions(+), 27 deletions(-)

diff --git a/client/ipa-client-install b/client/ipa-client-install
index 05b6b6e0da07353750d0dca4e6df9d1f58d69c35..4a263b3d0e25960fc57d198e3efb24447cbed98e 100755
--- a/client/ipa-client-install
+++ b/client/ipa-client-install
@@ -2525,7 +2525,8 @@ def install(options, env, fstore, statestore):
 if options.hostname and not options.on_master:
 # skip this step when run by ipa-server-install as it always configures
 # hostname
-tasks.backup_and_replace_hostname(fstore, statestore, options.hostname)
+   

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

[Martin Basti]

On 01.08.2016 14:28, Florence Blanc-Renaud wrote:

On 07/29/2016 04:56 PM, Martin Basti wrote:

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

Patch attached.




Hi Martin,

thanks for your patch. I could not reproduce the issue anymore with 
ipa-client-automount --uninstall, thus looks good to me.


Ack,
Flo.


Pushed to master: 2d4d1a9dc0ef2bbe86751768d6e6b009a52c0dc9

--
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 0030][Tests] Fix authentication indicators tests failing due to removal of has_keytab key from list of expected attributes of update command

[Martin Basti]

On 01.08.2016 12:01, Ganna Kaihorodova wrote:

Greetings!

ACK

Best regards,
Ganna Kaihorodova
Associate Software Quality Engineer


- Original Message -
From: "Lenka Doudova" 
To: "freeipa-devel" 
Sent: Monday, August 1, 2016 8:20:05 AM
Subject: [Freeipa-devel] [PATCH 0030][Tests] Fix authentication indicators 
tests failing due to removal of has_keytab key from list of expected attributes 
of update command

Hi,

solution for https://fedorahosted.org/freeipa/ticket/5281 has removed
has_keytab attribute from list of expected keys of service-mod command.
Attached patch provides a fix for tests that consequently started to fail.

Lenka



Pushed to master: 63a91ca49a40590784dbc3e91001f9864f3a185e

--
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:10, 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



Flo.


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







Pushed to master: 1a04edd36bd95d0e6e8c43e742113b3e3cadfb6b

--
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] 0001 Added new authentication method

[Tibor Dudlak]

Hi,

I have added few lines to code to make optional login with personal
certificate (or with smartcard) possible. Some ui changes has to be made.
It is not cosher but it definitely work.

Thank you, Tibor
From 91201035c02a186ad7311880730554925d7bf58e Mon Sep 17 00:00:00 2001
From: Tiboris 
Date: Mon, 1 Aug 2016 11:28:05 +0200
Subject: [PATCH] Added new authentication method.

This edit only added few changes to be able to login with personal certificate into IPA server.
---
 ipaserver/plugins/xmlserver.py | 3 ++-
 ipaserver/rpcserver.py | 5 +
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/ipaserver/plugins/xmlserver.py b/ipaserver/plugins/xmlserver.py
index d8fe24e0cb407603e9898e934229c9373f3c8b62..1843c0568543951f2c817616d9e988deaab47056 100644
--- a/ipaserver/plugins/xmlserver.py
+++ b/ipaserver/plugins/xmlserver.py
@@ -28,12 +28,13 @@ register = Registry()
 
 
 if api.env.context in ('server', 'lite'):
-from ipaserver.rpcserver import wsgi_dispatch, xmlserver, jsonserver_kerb, jsonserver_session, login_kerberos, login_password, change_password, sync_token, xmlserver_session
+from ipaserver.rpcserver import wsgi_dispatch, xmlserver, jsonserver_kerb, jsonserver_session, login_kerberos, login_x509, login_password, change_password, sync_token, xmlserver_session
 register()(wsgi_dispatch)
 register()(xmlserver)
 register()(jsonserver_kerb)
 register()(jsonserver_session)
 register()(login_kerberos)
+register()(login_x509)
 register()(login_password)
 register()(change_password)
 register()(sync_token)
diff --git a/ipaserver/rpcserver.py b/ipaserver/rpcserver.py
index d036f3c27521f17709672b830d5aa58167c76b34..a181ecfcb1d01b1c2dd5ee6cb9721d69be8c1863 100644
--- a/ipaserver/rpcserver.py
+++ b/ipaserver/rpcserver.py
@@ -876,6 +876,11 @@ class login_kerberos(Backend, KerberosSession, HTTP_Status):
 
 return self.finalize_kerberos_acquisition('login_kerberos', user_ccache_name, environ, start_response)
 
+
+class login_x509(login_kerberos, KerberosSession, HTTP_Status):
+key = '/session/login_x509'
+
+
 class login_password(Backend, KerberosSession, HTTP_Status):
 
 content_type = 'text/plain'
-- 
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] 0078-82: webui tests: tests for new certificate widget

[Lenka Doudova]

On 07/29/2016 03:00 PM, Pavel Vomacka wrote:




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

Hi,

all is fine now, ACK for all patches.

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 0560] ipa-client-automount: don not initialize API during uninstall

[Florence Blanc-Renaud]

On 07/29/2016 04:56 PM, Martin Basti wrote:

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

Patch attached.




Hi Martin,

thanks for your patch. I could not reproduce the issue anymore with 
ipa-client-automount --uninstall, thus looks good to me.


Ack,
Flo.

--
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 0030][Tests] Fix authentication indicators tests failing due to removal of has_keytab key from list of expected attributes of update command

[Ganna Kaihorodova]

Greetings!

ACK

Best regards,
Ganna Kaihorodova
Associate Software Quality Engineer


- Original Message -
From: "Lenka Doudova" 
To: "freeipa-devel" 
Sent: Monday, August 1, 2016 8:20:05 AM
Subject: [Freeipa-devel] [PATCH 0030][Tests] Fix authentication indicators 
tests failing due to removal of has_keytab key from list of expected attributes 
of update command

Hi,

solution for https://fedorahosted.org/freeipa/ticket/5281 has removed 
has_keytab attribute from list of expected keys of service-mod command. 
Attached patch provides a fix for tests that consequently started to fail.

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

-- 
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] [PATCHES 681-682] cert: speed up cert-find, do not crash on invalid data in cert-find

[Jan Cholasta]

On 1.8.2016 10:19, Jan Cholasta wrote:

Hi,

the attached patches fix 
and .


Self-NACK, proper patches attached.

Honza

--
Jan Cholasta
From 70ef1075bc361d836c1e836114a5367c3c5879d6 Mon Sep 17 00:00:00 2001
From: Jan Cholasta 
Date: Mon, 1 Aug 2016 09:53:39 +0200
Subject: [PATCH] cert: speed up cert-find

Use issuer+serial rather than raw DER blob to identify certificates in
cert-find's intermediate result.

Restructure the code to make it (hopefully) easier to follow.

https://fedorahosted.org/freeipa/ticket/6098
---
 ipaserver/plugins/cert.py | 394 +-
 1 file changed, 215 insertions(+), 179 deletions(-)

diff --git a/ipaserver/plugins/cert.py b/ipaserver/plugins/cert.py
index 06041d3..1367448 100644
--- a/ipaserver/plugins/cert.py
+++ b/ipaserver/plugins/cert.py
@@ -21,6 +21,7 @@
 
 import base64
 import binascii
+import collections
 import datetime
 import os
 
@@ -295,18 +296,24 @@ class BaseCertObject(Object):
 ),
 )
 
-def _parse(self, obj):
-cert = x509.load_certificate(obj['certificate'])
-obj['subject'] = DN(unicode(cert.subject))
-obj['issuer'] = DN(unicode(cert.issuer))
-obj['valid_not_before'] = unicode(cert.valid_not_before_str)
-obj['valid_not_after'] = unicode(cert.valid_not_after_str)
-obj['md5_fingerprint'] = unicode(
-nss.data_to_hex(nss.md5_digest(cert.der_data), 64)[0])
-obj['sha1_fingerprint'] = unicode(
-nss.data_to_hex(nss.sha1_digest(cert.der_data), 64)[0])
-obj['serial_number'] = cert.serial_number
-obj['serial_number_hex'] = u'0x%X' % cert.serial_number
+def _parse(self, obj, full=True):
+cert = obj.get('certificate')
+if cert is not None:
+cert = x509.load_certificate(cert)
+obj['subject'] = DN(unicode(cert.subject))
+obj['issuer'] = DN(unicode(cert.issuer))
+obj['serial_number'] = cert.serial_number
+if full:
+obj['valid_not_before'] = unicode(cert.valid_not_before_str)
+obj['valid_not_after'] = unicode(cert.valid_not_after_str)
+obj['md5_fingerprint'] = unicode(
+nss.data_to_hex(nss.md5_digest(cert.der_data), 64)[0])
+obj['sha1_fingerprint'] = unicode(
+nss.data_to_hex(nss.sha1_digest(cert.der_data), 64)[0])
+
+serial_number = obj.get('serial_number')
+if serial_number is not None:
+obj['serial_number_hex'] = u'0x%X' % serial_number
 
 
 class BaseCertMethod(Method):
@@ -691,10 +698,14 @@ class cert(BaseCertObject):
 yield self.api.Object[name]
 
 def _fill_owners(self, obj):
+dns = obj.pop('owner', None)
+if dns is None:
+return
+
 for owner in self._owners():
 container_dn = DN(owner.container_dn, self.api.env.basedn)
 name = 'owner_' + owner.name
-for dn in obj['owner']:
+for dn in dns:
 if dn.endswith(container_dn, 1):
 value = owner.get_primary_key_from_dn(dn)
 obj.setdefault(name, []).append(value)
@@ -984,36 +995,171 @@ class cert_find(Search, CertMethod):
 label=owner.object_name,
 )
 
-def execute(self, criteria=None, all=False, raw=False, pkey_only=False,
-no_members=True, timelimit=None, sizelimit=None, **options):
-ca_options = {'cacn',
-  'revocation_reason',
-  'issuer',
-  'subject',
-  'min_serial_number', 'max_serial_number',
-  'exactly',
-  'validnotafter_from', 'validnotafter_to',
-  'validnotbefore_from', 'validnotbefore_to',
-  'issuedon_from', 'issuedon_to',
-  'revokedon_from', 'revokedon_to'}
-ldap_options = {prefix + owner.name
-for owner in self.obj._owners()
-for prefix in ('', 'no_')}
-has_ca_options = (
-any(name in options for name in ca_options - {'exactly'}) or
-options['exactly'])
-has_ldap_options = any(name in options for name in ldap_options)
-has_cert_option = 'certificate' in options
+def _get_cert_key(self, cert):
+nss_cert = x509.load_certificate(cert, x509.DER)
+
+return (DN(unicode(nss_cert.issuer)), nss_cert.serial_number)
+
+def _get_cert_obj(self, cert, all, raw, pkey_only):
+obj = {'certificate': unicode(base64.b64encode(cert))}
+
+full = not pkey_only and all
+if not raw:
+self.obj._parse(obj, full)
+if not full:
+del obj['certificate']
+
+return obj
+
+def _cert_search(self, 

Re: [Freeipa-devel] [PATCH 0024][Tests] Fix integration tests not to produce incorrect /etc/hosts file

[Martin Basti]

On 01.08.2016 09:21, Oleg Fayans wrote:

ACK

On 07/19/2016 01:19 PM, Lenka Doudova wrote:



On 06/29/2016 06:49 PM, Petr Spacek wrote:

On 29.6.2016 18:39, Oleg Fayans wrote:

In fact, I believe /etc/hosts file should not be touched at all.
Hostname resolution is usually governed by the DNS system of the 
lab in

which tests are running. We do not modify it when perform tests
manually, so I'd rather remove this method at all.

+1, it should not be need. Let me know if it is needed for some reason
and I
will have a look.

Petr^2 Spacek

Hi,

providing new (and renamed) patch as was suggested in the discussion
above - removing manipulation with /etc/hosts file from the tests.
The "fix_etc_hosts" function was completely removed from the tasks file.
Verification that nothing is broken by this change was done by running
some random integration test (trust tests), and also on Milan's
suggestion by running a test requiring two replicas (replica promotion
tests).

Lenka


On 06/29/2016 06:27 PM, Lenka Doudova wrote:

Hi all,

a function 'fix_etc_hosts' in ipatests/test_integration/tasks.py
produces incorrect /etc/hosts file (solitary IPv6 address), and
currently parser is not able to resolve the issue, causing
ipa-server-install to fail with 'list index out of range' error.

Hence I'm attaching patch to fix this issue before parser is fixed
(related ticket to it #6014). The fix is just change of regexs
responsible for creating incorrect file so that all the lines
containing
defined hostname are removed, not just specific IP/hostname/shortname
strings.


Lenka







Pushed to master: a20c04033a437c5531b0ae4781d76738d1f02029

--
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 0024][Tests] Fix integration tests not to produce incorrect /etc/hosts file

[Oleg Fayans]

ACK

On 07/19/2016 01:19 PM, Lenka Doudova wrote:



On 06/29/2016 06:49 PM, Petr Spacek wrote:

On 29.6.2016 18:39, Oleg Fayans wrote:

In fact, I believe /etc/hosts file should not be touched at all.
Hostname resolution is usually governed by the DNS system of the lab in
which tests are running. We do not modify it when perform tests
manually, so I'd rather remove this method at all.

+1, it should not be need. Let me know if it is needed for some reason
and I
will have a look.

Petr^2 Spacek

Hi,

providing new (and renamed) patch as was suggested in the discussion
above - removing manipulation with /etc/hosts file from the tests.
The "fix_etc_hosts" function was completely removed from the tasks file.
Verification that nothing is broken by this change was done by running
some random integration test (trust tests), and also on Milan's
suggestion by running a test requiring two replicas (replica promotion
tests).

Lenka


On 06/29/2016 06:27 PM, Lenka Doudova wrote:

Hi all,

a function 'fix_etc_hosts' in ipatests/test_integration/tasks.py
produces incorrect /etc/hosts file (solitary IPv6 address), and
currently parser is not able to resolve the issue, causing
ipa-server-install to fail with 'list index out of range' error.

Hence I'm attaching patch to fix this issue before parser is fixed
(related ticket to it #6014). The fix is just change of regexs
responsible for creating incorrect file so that all the lines
containing
defined hostname are removed, not just specific IP/hostname/shortname
strings.


Lenka






--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.

--
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 0151-0152] install: Call hostnamectl set-hostname only if --hostname option is use server-install: Fix --hostname option to always override api.env value

[Jan Cholasta]

On 28.7.2016 16:55, Petr Spacek wrote:

On 28.7.2016 16:44, Jan Cholasta wrote:

On 28.7.2016 16:37, Petr Spacek wrote:

On 28.7.2016 16:35, Jan Cholasta wrote:

On 28.7.2016 16:20, Petr Spacek wrote:

Hello,

install: Call hostnamectl set-hostname only if --hostname option is used

This commit also splits hostname backup and configuration into two separate
functions. This allows us to backup hostname without setting it at the
same time.

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


Note that you can set ca_host in cfg unconditionally, the value is only valid
during install and is not written anywhere.


I prefer not to set it so the code blows up when we attempt to touch variables
we should reference in particular setups. I.e. Take this as a first step
towards api.env without invalid values :-)


OK. Use the proper condition then ("if setup_ca:").


Oh, I'm probably blind. Here is revised version.


But you used "if not setup_ca:" rather than "if setup_ca:" :-)



Petr^2 Spacek





(In my stash I have a patch which removes nonsense defaults from
ipalib.constants. To be pushed when we a new git branch for 4.4...)

Petr^2 Spacek


server-install: Fix --hostname option to always override api.env values

Attempts to compare local hostname with user-provided values are error
prone as we found out in #5794. This patch removes comparison and makes
the env values deterministic.

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


Jan, this patch set should fix problems you have seen in containers.



--
Jan Cholasta

--
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 0030][Tests] Fix authentication indicators tests failing due to removal of has_keytab key from list of expected attributes of update command

[Lenka Doudova]

Hi,

solution for https://fedorahosted.org/freeipa/ticket/5281 has removed 
has_keytab attribute from list of expected keys of service-mod command. 
Attached patch provides a fix for tests that consequently started to fail.


Lenka

From eb6b692b484816084a48e64f969b28adae9fb967 Mon Sep 17 00:00:00 2001
From: Lenka Doudova 
Date: Mon, 1 Aug 2016 08:00:16 +0200
Subject: [PATCH] Tests: Remove has_keytab from list of expected keys of update
 command

As part of https://fedorahosted.org/freeipa/ticket/5281, the has_keytab
attribute was removed from results of service-mod command. Removing this
attribute from list of expected keys to prevent failing tests.

Ticket: https://fedorahosted.org/freeipa/ticket/6149
---
 ipatests/test_xmlrpc/tracker/service_plugin.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipatests/test_xmlrpc/tracker/service_plugin.py b/ipatests/test_xmlrpc/tracker/service_plugin.py
index e51232f8056908ef298c62db158c128e1d7436b4..3b970b98985f6d0528aba369064f889256853b79 100644
--- a/ipatests/test_xmlrpc/tracker/service_plugin.py
+++ b/ipatests/test_xmlrpc/tracker/service_plugin.py
@@ -48,7 +48,7 @@ class ServiceTracker(KerberosAliasMixin, Tracker):
 
 create_keys = (retrieve_keys | {u'objectclass', u'ipauniqueid'}) - {
 u'usercertificate', u'has_keytab'}
-update_keys = retrieve_keys - {u'dn'}
+update_keys = retrieve_keys - {u'dn', u'has_keytab'}
 
 def __init__(self, name, host_fqdn, options=None):
 super(ServiceTracker, self).__init__(default_version=None)
-- 
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