date:20150813


On 08/13/2015 02:12 PM, Christian Heimes wrote:

On 2015-08-13 14:05, Petr Vobornik wrote:

On 08/13/2015 12:38 PM, Christian Heimes wrote:

On 2015-08-13 12:10, Petr Vobornik wrote:

On 07/23/2015 08:38 PM, Christian Heimes wrote:

The ipa vault commands now load the public keys in order to verify
them.
The validation also prevents a user from accidentally sending her
private keys to the server. The patch fixes #5142 and #5142.

$ ./ipa vault-add AsymmetricVault --desc "Asymmetric vault" --type
asymmetric --public-key-file mykey.pem
ipa: ERROR: invalid 'ipavaultpublickey': Invalid or unsupported vault
public key: Could not unserialize key data.

https://fedorahosted.org/freeipa/ticket/5142
https://fedorahosted.org/freeipa/ticket/5143



ACK as fix for 5142.

I don't think that it fixes 5143. The traceback is fixed therefore 5143
doesn't occur but if there was other traceback raised by
`self.api.Command.vault_archive(*args, **opts)` then the vault added in
`response = self.api.Command.vault_add_internal(*args, **options)` would
be still created.


Yes, that is correct. There aren't any arguments that can lead to an
exception. The arguments are either already validated by vault_add() or
don't raise an error.

Of course there are plenty of opportunities errors. The connection to
the IPA or LDAP server could fail, NSS DB could be missing and so on.
How should we handle an error in vault_archive? Is there another way
then to delete the new vault all along?

try:
  self.api.Command.vault_archive(*args, **opts)
except Exception:
  log_error()
  self.api.Command.vault_del(*args, **opts)
  report_error()

Christian



Imho this is the way. But it may fail because of the same root cause as
vault_archive.

That said I don't see #5142 as a priority and would defer it.


I'd still like to see my patch for #5142 in RHEL, too. It prevents
accidental exposure of private keys, too. In the test case the test
uploads his private keys to the server. FreeIPA should not leak a user's
private key. My patch prevents that, too.



Sorry, wrong number. I meant defer #5143. The patch for #5142 is OK.

Pushed to:
master: e4dff25838f7a2342779851bd68460080d77683b
ipa-4-2: 06d68b447ff62b64a3a6e4a3c565fcf406a457ec
--
Petr Vobornik

--
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] 371 Added support for changing vault encryption.

2015-08-13 Thread Endi Sukma Dewata


On 8/13/2015 8:06 AM, Martin Basti wrote:

The vault-mod command has been modified to support changing vault
encryption attributes (i.e. type, password, public/private keys)
in addition to normal attributes (i.e. description). Changing the
encryption requires retrieving the stored secret with the old
attributes and rearchieving it with the new attributes.

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


Hello, does this patch require any additional patches?

I have current master branch and I cannot apply it.

git am
freeipa-edewata-0371-Added-support-for-changing-vault-encryption.patch -3
Applying: Added support for changing vault encryption.
error: invalid object 100644 3b62822366a62c90f843a6293589c28383e782ef
for 'ipalib/plugins/vault.py'
fatal: git-write-tree: error building trees
Repository lacks necessary blobs to fall back on 3-way merge.


Martin^2


New patch attached. It requires patch #0369-3.

--
Endi S. Dewata
>From b06e859e51a177369c27c52bff8a70263aed59c0 Mon Sep 17 00:00:00 2001
From: "Endi S. Dewata" 
Date: Fri, 31 Jul 2015 07:53:15 +0200
Subject: [PATCH] Added support for changing vault encryption.

The vault-mod command has been modified to support changing vault
encryption attributes (i.e. type, password, public/private keys)
in addition to normal attributes (i.e. description). Changing the
encryption requires retrieving the stored secret with the old
attributes and rearchiving it with the new attributes.

https://fedorahosted.org/freeipa/ticket/5176
---
 API.txt   |  27 +++-
 VERSION   |   4 +-
 ipalib/plugins/vault.py   | 232 ++--
 ipatests/test_xmlrpc/test_vault_plugin.py | 246 ++
 4 files changed, 495 insertions(+), 14 deletions(-)

diff --git a/API.txt b/API.txt
index 
9dbf86aedf2a1b62dabab21fb30bbceb2f0f237b..26f05cf9e1e27ec4f714bb34174e17972961bda2
 100644
--- a/API.txt
+++ b/API.txt
@@ -5466,11 +5466,12 @@ output: Output('completed', , None)
 output: Output('failed', , None)
 output: Entry('result', , Gettext('A dictionary representing an 
LDAP entry', domain='ipa', localedir=None))
 command: vault_archive
-args: 1,10,3
+args: 1,11,3
 arg: Str('cn', attribute=True, cli_name='name', maxlength=255, 
multivalue=False, pattern='^[a-zA-Z0-9_.-]+$', primary_key=True, query=True, 
required=True)
 option: Flag('all', autofill=True, cli_name='all', default=False, 
exclude='webui')
 option: Bytes('data?')
 option: Str('in?')
+option: Flag('override_password?', autofill=True, default=False)
 option: Str('password?', cli_name='password')
 option: Str('password_file?', cli_name='password_file')
 option: Flag('raw', autofill=True, cli_name='raw', default=False, 
exclude='webui')
@@ -5528,6 +5529,30 @@ output: ListOfEntries('result', (, ), Gettext('A list
 output: Output('summary', (, ), None)
 output: Output('truncated', , None)
 command: vault_mod
+args: 1,18,3
+arg: Str('cn', attribute=True, cli_name='name', maxlength=255, 
multivalue=False, pattern='^[a-zA-Z0-9_.-]+$', primary_key=True, query=True, 
required=True)
+option: Flag('all', autofill=True, cli_name='all', default=False, 
exclude='webui')
+option: Flag('change_password?', autofill=True, default=False)
+option: Str('description?', cli_name='desc')
+option: Bytes('ipavaultpublickey?', cli_name='public_key')
+option: Bytes('ipavaultsalt?', cli_name='salt')
+option: Str('ipavaulttype?', cli_name='type')
+option: Str('new_password?', cli_name='new_password')
+option: Str('new_password_file?', cli_name='new_password_file')
+option: Str('old_password?', cli_name='old_password')
+option: Str('old_password_file?', cli_name='old_password_file')
+option: Bytes('private_key?', cli_name='private_key')
+option: Str('private_key_file?', cli_name='private_key_file')
+option: Str('public_key_file?', cli_name='public_key_file')
+option: Flag('raw', autofill=True, cli_name='raw', default=False, 
exclude='webui')
+option: Str('service?')
+option: Flag('shared?', autofill=True, default=False)
+option: Str('username?', cli_name='user')
+option: Str('version?', exclude='webui')
+output: Entry('result', , Gettext('A dictionary representing an 
LDAP entry', domain='ipa', localedir=None))
+output: Output('summary', (, ), None)
+output: PrimaryKey('value', None, None)
+command: vault_mod_internal
 args: 1,15,3
 arg: Str('cn', attribute=True, cli_name='name', maxlength=255, 
multivalue=False, pattern='^[a-zA-Z0-9_.-]+$', primary_key=True, query=True, 
required=True)
 option: Str('addattr*', cli_name='addattr', exclude='webui')
diff --git a/VERSION b/VERSION
index 
c42bea06522dae55e1a89ff94ae394594086b467..feb9f4db92c7c7b95e9e5d5907b1f97e96b26886
 100644
--- a/VERSION
+++ b/VERSION
@@ -90,5 +90,5 @@ IPA_DATA_VERSION=2010061412
 #  #
 
 IPA_API_VERSION_MAJOR=2
-IPA_API_VERSION_MINOR=149
-# Last change: edewata - Added CLI

Re: [Freeipa-devel] [PATCH] 369 Added CLI param and ACL for vault service operations.

2015-08-13 Thread Endi Sukma Dewata


On 8/13/2015 6:00 AM, Petr Vobornik wrote:

On 08/11/2015 08:42 AM, Jan Cholasta wrote:

On 10.8.2015 21:12, Endi Sukma Dewata wrote:

On 8/4/2015 10:32 AM, Endi Sukma Dewata wrote:

Martin, I do not think going on with business as usual is the right
thing to do here. We know this is going to bite.
I suggest Endy adds a *new* API if making it backwards compatible is
not
possible. The era of bumping whole API version must stop, the sooner
the
better.


My point is that we do not know yet how to do this kind of changes
long term.
So what I did not want to end up are 2 copy&pasted Vault plugins
maintained
forever, differing in just that.

If you know how to do this without copypasting, I will be fine with
that.


We probably can do it like this:
* the old plugin continues to provide Vault 1.0 functionality
* the new plugin will be a proxy to the old plugin except for the parts
that have changed in Vault 1.1.

Or the other way around:
* the new plugin will provide Vault 1.1 functionality
* the old plugin will be a proxy to the new plugin except for the parts
that needs to be maintained for Vault 1.0.

The first option is probably safer.

In any case, IPA 4.2.1 will only provide a single client for Vault 1.1,
but two services for Vault 1.0 and 1.1.


A new patch #369-1 is attached. It has been rebased on top of #372 and
#373 that fix the conflicting parameter while maintaining backward
compatibility.


I have modified the first version of the patch to maintain backward
compatibility and not require your patches #372 and #373. Should be much
easier to review. See attachment.


Jan approach seems better to me for 4.2.1. Endi, do you agree with the
changes? Could we proceed with the review?


Yes, please see the attached patch. I had to update it to remove buggy 
code and revised the docs. I also had to rebase my other patches to make 
sure they work with this patch.


--
Endi S. Dewata
>From c43df23159e5dafd47d5309b3b0f75de4870640b Mon Sep 17 00:00:00 2001
From: "Endi S. Dewata" 
Date: Tue, 11 Aug 2015 08:19:59 +0200
Subject: [PATCH] Added CLI param and ACL for vault service operations.

The CLIs to manage vault owners and members have been modified
to accept services with a new parameter. Due to name conflict,
the existing 'service' parameter has been renamed to
'servicename'.

A new ACL has been added to allow a service to create its own
service container.

https://fedorahosted.org/freeipa/ticket/5172
---
 API.txt|  12 ++-
 VERSION|   4 +-
 install/share/vault.update |   1 +
 ipalib/plugins/vault.py| 179 +
 4 files changed, 95 insertions(+), 101 deletions(-)

diff --git a/API.txt b/API.txt
index 
04f2f894f7667239d94a2a7278d0cc80704afeb5..9dbf86aedf2a1b62dabab21fb30bbceb2f0f237b
 100644
--- a/API.txt
+++ b/API.txt
@@ -5434,13 +5434,14 @@ output: Entry('result', , Gettext('A 
dictionary representing an LDA
 output: Output('summary', (, ), None)
 output: PrimaryKey('value', None, None)
 command: vault_add_member
-args: 1,9,3
+args: 1,10,3
 arg: Str('cn', attribute=True, cli_name='name', maxlength=255, 
multivalue=False, pattern='^[a-zA-Z0-9_.-]+$', primary_key=True, query=True, 
required=True)
 option: Flag('all', autofill=True, cli_name='all', default=False, 
exclude='webui')
 option: Str('group*', alwaysask=True, cli_name='groups', csv=True)
 option: Flag('no_members', autofill=True, default=False, exclude='webui')
 option: Flag('raw', autofill=True, cli_name='raw', default=False, 
exclude='webui')
 option: Str('service?')
+option: Str('services', alwaysask=True, cli_name='services', csv=True, 
multivalue=True, required=False)
 option: Flag('shared?', autofill=True, default=False)
 option: Str('user*', alwaysask=True, cli_name='users', csv=True)
 option: Str('username?', cli_name='user')
@@ -5449,13 +5450,14 @@ output: Output('completed', , None)
 output: Output('failed', , None)
 output: Entry('result', , Gettext('A dictionary representing an 
LDAP entry', domain='ipa', localedir=None))
 command: vault_add_owner
-args: 1,9,3
+args: 1,10,3
 arg: Str('cn', attribute=True, cli_name='name', maxlength=255, 
multivalue=False, pattern='^[a-zA-Z0-9_.-]+$', primary_key=True, query=True, 
required=True)
 option: Flag('all', autofill=True, cli_name='all', default=False, 
exclude='webui')
 option: Str('group*', alwaysask=True, cli_name='groups', csv=True)
 option: Flag('no_members', autofill=True, default=False, exclude='webui')
 option: Flag('raw', autofill=True, cli_name='raw', default=False, 
exclude='webui')
 option: Str('service?')
+option: Str('services', alwaysask=True, cli_name='services', csv=True, 
multivalue=True, required=False)
 option: Flag('shared?', autofill=True, default=False)
 option: Str('user*', alwaysask=True, cli_name='users', csv=True)
 option: Str('username?', cli_name='user')
@@ -5547,13 +5549,14 @@ output: Entry('result', , Gettext('A 
dictionary representing an LDA
 output: Output('summary', (, ), None)
 out

Re: [Freeipa-devel] [PATCH] 374 Fixed vault container ownership.

2015-08-13 Thread Endi Sukma Dewata


On 8/13/2015 9:18 AM, Martin Basti wrote:

The vault-add command has been fixed such that if the user/service
private vault container does not exist yet it will be created and
owned by the user/service instead of the vault creator.

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


I cannot apply this patch, are there any additional required patches?

I have current ipa master branch

git am freeipa-edewata-0374-Fixed-vault-container-ownership.patch -3
Applying: Fixed vault container ownership.
error: invalid object 100644 427b1ea1588af2fb09a99181b8773abdf8099b8d
for 'ipalib/plugins/vault.py'
fatal: git-write-tree: error building trees
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.


Rebased. This patch doesn't have any dependency.

--
Endi S. Dewata
>From da35d235bc8196062b208095aa904e3b7a1905e2 Mon Sep 17 00:00:00 2001
From: "Endi S. Dewata" 
Date: Mon, 10 Aug 2015 20:57:58 +0200
Subject: [PATCH] Fixed vault container ownership.

The vault-add command has been fixed such that if the user/service
private vault container does not exist yet it will be created and
owned by the user/service instead of the vault creator.

https://fedorahosted.org/freeipa/ticket/5194
---
 ipalib/plugins/vault.py | 25 +++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/ipalib/plugins/vault.py b/ipalib/plugins/vault.py
index 
1150d5f3b8cd0001f24756548d7f280494161d19..9a2995ca04cb99b0be46076541cea2638bf3ca56
 100644
--- a/ipalib/plugins/vault.py
+++ b/ipalib/plugins/vault.py
@@ -712,12 +712,33 @@ class vault_add_internal(LDAPCreate):
 else:
 owner_dn = self.api.Object.user.get_dn(name)
 
+parent_dn = DN(*dn[1:])
+
+container_dn = DN(self.api.Object.vault.container_dn,
+  self.api.env.basedn)
+
+services_dn = DN(('cn', 'services'), container_dn)
+users_dn = DN(('cn', 'users'), container_dn)
+
+if dn.endswith(services_dn):
+# service container should be owned by the service
+service = parent_dn[0]['cn']
+parent_owner_dn = self.api.Object.service.get_dn(service)
+
+elif dn.endswith(users_dn):
+# user container should be owned by the user
+user = parent_dn[0]['cn']
+parent_owner_dn = self.api.Object.user.get_dn(user)
+
+else:
+parent_owner_dn = owner_dn
+
 try:
-parent_dn = DN(*dn[1:])
-self.obj.create_container(parent_dn, owner_dn)
+self.obj.create_container(parent_dn, parent_owner_dn)
 except errors.DuplicateEntry as e:
 pass
 
+# vault should be owned by the creator
 entry_attrs['owner'] = owner_dn
 
 return dn
-- 
2.1.0

-- 
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] First part of integration tests for Topology Plugin

2015-08-13 Thread Tomas Babej



On 08/13/2015 05:06 PM, Martin Basti wrote:
> 
> 
> On 08/11/2015 03:36 PM, Oleg Fayans wrote:
>> Hi Martin,
>>
>> On 08/11/2015 02:02 PM, Martin Basti wrote:
>>> NACK, comments inline.
>>>
>>> On 11/08/15 13:25, Oleg Fayans wrote:
 Hi Martin,

 Thanks for the review!

 On 08/10/2015 07:08 PM, Martin Basti wrote:
> Thank you for patch, I have a few nitpicks:
>
> 1)
> On 10/08/15 13:05, Oleg Fayans wrote:
>> +def create_segment(master, leftnode, rightnode):
>> +"""create_segment(master, leftnode, rightnode)
> Why do you add the name of method in docstring?
 My bad, fixed.
>>>
>>> still there
>>>
>>> +tokenize_topologies(command_output)
>>> +takes an output of `ipa topologysegment-find` and returns an
>>> array of
>>>
>> Fixed, sorry.
>>>
>
>
> 2)
>
> +def create_segment(master, leftnode, rightnode):
> +"""create_segment(master, leftnode, rightnode)
> +creates a topology segment. The first argument is a node to
> run the
> command on
> +The first 3 arguments should be objects of class Host
> +Returns a hash object containing segment's name, leftnode,
> rightnode information
> +"""
>
> I would prefer to add assert there instead of just document that a
> Host
> object is needed
> assert(isinstance(master, Host))
> ...

 Fixed. Created a decorator that checks the type of arguments
>>>
>>> This does not scale well.
>>> If we will want to add new argument that is not host object, you need
>>> change it again.
>>
>> Agreed. Modified the decorator so that you can specify a slice of
>> arguments to be checked and a class to compare to. This does scale :)
>>>
>>> This might be used as function with specified variables that have to be
>>> host objects

>
> 3)
> +def destroy_segment(master, segment_name):
> +"""
> +destroy_segment(master, segment_name)
> +Destroys topology segment. First argument should be object of
> class
> Host
>
> Instead of description of params as first, second etc., you may use
> following:
>
> +def destroy_segment(master, segment_name):
> +"""
> +Destroys topology segment.
> +:param master: reference to master object of class Host
> +:param segment: name fo segment
> and eventually this in other methods
> +:returns: Lorem ipsum sit dolor mit amet
> +:raises NotFound: if segment is not found
>
 Fixed
>
> 4)
>
> cls.replicas[:len(cls.replicas) - 1],
>
> I suggest cls.replicas[:-1]
>
> In [2]: a = [1, 2, 3, 4, 5]
>
> In [3]: a[:-1]
> Out[3]: [1, 2, 3, 4]
>
 Fixed
>
> 5)
> Why re.findall() and then you just use the first result?
> 'leftnode': self.leftnode_re.findall(i)[0]
>
> Isn't just re.search() enough?
> leftnode_re.search(value).group(1)

 in fact
 leftnode_re.findall(string)[0]
 and
 leftnode_re.search(string).group(1),
 Are equally bad from the readability point of view. The first one is
 even shorter a bit, so why change? :)
>>>
>>> It depends on point of view,  because when I reviewed it yesterday my
>>> brain raises exception that you are trying to add multiple values to
>>> single value attribute, because you used findall, I expected that you
>>> need multiple values, and then I saw that index [0] at the end, and I
>>> was pretty confused what are you trying to achieve.
>>>
>>> And because findall is not effective in case when you need to find just
>>> one occurrence.
>>
>> I got it. Fixed.
>>
>>>
>>>

>
>
> Python 3 nitpick:
> I'm not sure if time when we should enforce python 2/3 compability
> already comes, but just for record:
> instead of open(file, 'r'), please use io.open(file, 'r') (import io
> before)
>
 Done.
>
>
>

>>>
>>> 1)
>>>
>>> +#
>>>
>>> empty comment here (several times)
>>
>> Removed
>>>
>>
> 
> NACK
> 
> you changed it wrong
> 
> group() returns everything, you need use group(1) to return content in
> braces.
> 

I'd suggest using named groups in this case, it leads to clearer
expressions.

See: https://docs.python.org/2/library/re.html , (?P...) section.

-- 
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] First part of integration tests for Topology Plugin




On 08/11/2015 03:36 PM, Oleg Fayans wrote:

Hi Martin,

On 08/11/2015 02:02 PM, Martin Basti wrote:

NACK, comments inline.

On 11/08/15 13:25, Oleg Fayans wrote:

Hi Martin,

Thanks for the review!

On 08/10/2015 07:08 PM, Martin Basti wrote:

Thank you for patch, I have a few nitpicks:

1)
On 10/08/15 13:05, Oleg Fayans wrote:

+def create_segment(master, leftnode, rightnode):
+"""create_segment(master, leftnode, rightnode)

Why do you add the name of method in docstring?

My bad, fixed.


still there

+tokenize_topologies(command_output)
+takes an output of `ipa topologysegment-find` and returns an
array of


Fixed, sorry.





2)

+def create_segment(master, leftnode, rightnode):
+"""create_segment(master, leftnode, rightnode)
+creates a topology segment. The first argument is a node to 
run the

command on
+The first 3 arguments should be objects of class Host
+Returns a hash object containing segment's name, leftnode,
rightnode information
+"""

I would prefer to add assert there instead of just document that a 
Host

object is needed
assert(isinstance(master, Host))
...


Fixed. Created a decorator that checks the type of arguments


This does not scale well.
If we will want to add new argument that is not host object, you need
change it again.


Agreed. Modified the decorator so that you can specify a slice of 
arguments to be checked and a class to compare to. This does scale :)


This might be used as function with specified variables that have to be
host objects




3)
+def destroy_segment(master, segment_name):
+"""
+destroy_segment(master, segment_name)
+Destroys topology segment. First argument should be object of 
class

Host

Instead of description of params as first, second etc., you may use
following:

+def destroy_segment(master, segment_name):
+"""
+Destroys topology segment.
+:param master: reference to master object of class Host
+:param segment: name fo segment
and eventually this in other methods
+:returns: Lorem ipsum sit dolor mit amet
+:raises NotFound: if segment is not found


Fixed


4)

cls.replicas[:len(cls.replicas) - 1],

I suggest cls.replicas[:-1]

In [2]: a = [1, 2, 3, 4, 5]

In [3]: a[:-1]
Out[3]: [1, 2, 3, 4]


Fixed


5)
Why re.findall() and then you just use the first result?
'leftnode': self.leftnode_re.findall(i)[0]

Isn't just re.search() enough?
leftnode_re.search(value).group(1)


in fact
leftnode_re.findall(string)[0]
and
leftnode_re.search(string).group(1),
Are equally bad from the readability point of view. The first one is
even shorter a bit, so why change? :)


It depends on point of view,  because when I reviewed it yesterday my
brain raises exception that you are trying to add multiple values to
single value attribute, because you used findall, I expected that you
need multiple values, and then I saw that index [0] at the end, and I
was pretty confused what are you trying to achieve.

And because findall is not effective in case when you need to find just
one occurrence.


I got it. Fixed.









Python 3 nitpick:
I'm not sure if time when we should enforce python 2/3 compability
already comes, but just for record:
instead of open(file, 'r'), please use io.open(file, 'r') (import io
before)


Done.








1)

+#

empty comment here (several times)


Removed






NACK

you changed it wrong

group() returns everything, you need use group(1) to return content in 
braces.


--
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 471] ULC: Prevent preserved users from being assigned membership




On 08/12/2015 02:20 PM, Jan Cholasta wrote:

On 12.8.2015 12:22, Jan Cholasta wrote:

Hi,

the attached patch fixes .

Honza


Fixed broken user_show on preserved user. Updated patch attached.




Pushed to:
master: 391ccabb9f0629b3d172d31cdab9067e4bd4e5dd
ipa-4-2: cd81727d6243de2c613afec6dd0bf9a41c724354

-- 
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] 0195 harden trust-fetch-domains oddjobd script


Hi,

see commit message for details.

--
/ Alexander Bokovoy
From 96f4623730f764c73ce4544d0788e8782fecaa99 Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy 
Date: Thu, 13 Aug 2015 17:18:57 +0300
Subject: [PATCH] trusts: harden trust-fetch-domains oddjobd-based script

When ipa-getkeytab is used to fetch trusted domain object credentials,
the fetched entry has always kvno 1. ipa-getkeytab always adds a key to
keytab which means older key versions will be in the SSSD keytab and
will confuse libkrb5 ccache initialization code as all kvno values are
equal to 1. Wrong key is picked up then and kinit fails.

To solve this problem, always remove existing
/var/lib/sss/keytabs/forest.keytab before retrieving a new one.

To make sure script's input cannot be used to define what should be
removed (by passing a relative path), make sure we retrieve trusted
forest name from LDAP. If it is not possible to retrieve, the script
will issue an exception and quit. If abrtd is running, this will be
recorded as a 'crash' and an attempt to use script by malicious user
would be recorded as well in the abrtd journal.

Solves https://bugzilla.redhat.com/show_bug.cgi?id=1250190
---
 install/oddjob/com.redhat.idm.trust-fetch-domains | 29 +++
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/install/oddjob/com.redhat.idm.trust-fetch-domains 
b/install/oddjob/com.redhat.idm.trust-fetch-domains
index e50c81e..6a2171d 100755
--- a/install/oddjob/com.redhat.idm.trust-fetch-domains
+++ b/install/oddjob/com.redhat.idm.trust-fetch-domains
@@ -41,6 +41,9 @@ def retrieve_keytab(api, ccache_name, oneway_keytab_name, 
oneway_principal):
   "-p", oneway_principal,
   "-k", oneway_keytab_name,
   "-r"]
+if os.path.isfile(oneway_keytab_name):
+os.unlink(oneway_keytab_name)
+
 (stdout, stderr, retcode) = ipautil.run(getkeytab_args,
 env={'KRB5CCNAME': ccache_name, 
'LANG': 'C'},
 raiseonerr=False)
@@ -111,7 +114,6 @@ from ipalib.plugins import trust
 # retrieve the keys to oneway_keytab_name.
 
 keytab_name = '/etc/samba/samba.keytab'
-oneway_keytab_name = '/var/lib/sss/keytabs/' + trusted_domain + '.keytab'
 
 principal = str('cifs/' + api.env.host)
 
@@ -137,10 +139,20 @@ else:
 old_ccache = os.environ.get('KRB5CCNAME')
 api.Backend.ldap2.connect(ccache)
 
+# Retrieve own NetBIOS name and trusted forest's name.
+# We use script's input to retrieve the trusted forest's name to sanitize input
+# for file-level access as we might need to wipe out keytab in 
/var/lib/sss/keytabs
 own_trust_dn = DN(('cn', api.env.domain),('cn','ad'), ('cn', 'etc'), 
api.env.basedn)
 own_trust_entry = api.Backend.ldap2.get_entry(own_trust_dn, ['ipantflatname'])
-own_trust_flatname = own_trust_entry['ipantflatname'][0].upper()
+own_trust_flatname = own_trust_entry.single_value.get('ipantflatname').upper()
+trusted_domain_dn = DN(('cn', trusted_domain.lower()), 
api.env.container_adtrusts, api.env.basedn)
+trusted_domain_entry = api.Backend.ldap2.get_entry(trusted_domain_dn, ['cn'])
+trusted_domain = trusted_domain_entry.single_value.get('cn').lower()
 
+# At this point if we didn't find trusted forest name, an exception will be 
raised
+# and script will quit. This is actually intended.
+
+oneway_keytab_name = '/var/lib/sss/keytabs/' + trusted_domain + '.keytab'
 oneway_principal = str('%s$@%s' % (own_trust_flatname, trusted_domain.upper()))
 
 # If keytab does not exist, retrieve it
@@ -152,11 +164,18 @@ try:
 # The keytab may have stale key material (from older trust-add run)
 if not os.path.isfile(oneway_ccache_name):
 oneway_ccache = kinit_keytab(oneway_principal, oneway_keytab_name, 
oneway_ccache_name)
+else:
+oneway_ccache_check = KRB5_CCache(oneway_ccache_name)
+if not oneway_ccache_check.credential_is_valid(oneway_principal):
+# If credentials were invalid, obtain them again
+oneway_ccache = kinit_keytab(oneway_principal, oneway_keytab_name, 
oneway_ccache_name)
+else:
+oneway_ccache = oneway_ccache_check.ccache
 except krbV.Krb5Error as e:
 # If there was failure on using keytab, assume it is stale and retrieve 
again
 retrieve_keytab(api, ccache_name, oneway_keytab_name, oneway_principal)
 
-if oneway_ccache:
+try:
 # There wasn existing ccache, validate its content
 oneway_ccache_check = KRB5_CCache(oneway_ccache_name)
 if not oneway_ccache_check.credential_is_valid(oneway_principal):
@@ -164,7 +183,7 @@ if oneway_ccache:
 oneway_ccache = kinit_keytab(oneway_principal, oneway_keytab_name, 
oneway_ccache_name)
 else:
 oneway_ccache = oneway_ccache_check.ccache
-else:
+except krbV.Krb5Error as e:
 oneway_ccache = kinit_keytab(oneway_principal, oneway_keytab_name, 
oneway_ccache_name)
 
 # We are done: we have ccache with TDO credentials and ca

Re: [Freeipa-devel] [PATCH] 0038 cert-request: remove allowed extensions check

2015-08-13 Thread Ade Lee

Fraser, 

Continuing the discussion started previously, the question is whether
IPA should check for the presence of certain extensions.

There seem to be two kinds of problems which could be encountered here:

1. User could include a CSR which includes an extension that is not
valid for the profile.

2. User could include data for an extension that is invalid.

The original allowed extensions check attempted to address problem (1)
by allowing only extensions that were valid for the small set of
profiles used by IPA.  Now that custom profiles are available, though,
this is no longer sufficient.

I do believe that it would be useful to provide the user with feedback
if a particular extension is not supported by the profile when the CSR
is submitted to IPA.  This should most likely be a non-fatal
notification, because the CA will end up ignoring the extension.

With the Dogtag profile API, it is possible to enumerate the extensions
that are included in a cert for a particular profile.  Couldn't this
data be used as the basis for this check?

For problem (2), although some validation could be done in IPA, this is
most probably something that should be left to Dogtag itself.  I believ
e the error reporting from Dogtag has been sufficiently improved so
that these types of validation errors would be reported back to IPA.

Ade

On Thu, 2015-08-13 at 15:54 +1000, Fraser Tweedale wrote:
> The attached patch fixes
> https://fedorahosted.org/freeipa/ticket/5205
> 
> Thanks,
> Fraser
> -- 
> 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] [PATCH] 374 Fixed vault container ownership.




On 08/10/2015 09:45 PM, Endi Sukma Dewata wrote:

The vault-add command has been fixed such that if the user/service
private vault container does not exist yet it will be created and
owned by the user/service instead of the vault creator.

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





I cannot apply this patch, are there any additional required patches?

I have current ipa master branch

git am freeipa-edewata-0374-Fixed-vault-container-ownership.patch -3
Applying: Fixed vault container ownership.
error: invalid object 100644 427b1ea1588af2fb09a99181b8773abdf8099b8d 
for 'ipalib/plugins/vault.py'

fatal: git-write-tree: error building trees
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.

-- 
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 Add permission for bypassing CA ACL enforcement

2015-08-13 Thread Martin Babinsky


On 08/13/2015 05:46 AM, Fraser Tweedale wrote:

On Tue, Aug 04, 2015 at 03:21:29PM +1000, Fraser Tweedale wrote:

The attached patch fixes
https://fedorahosted.org/freeipa/ticket/5099.

Thanks,
Fraser


Ping; this patch needs review.



ACK

--
Martin^3 Babinsky

--
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] 910 add permission: System: Manage User Certificates

On Thu, Aug 13, 2015 at 12:30:10PM +0300, Alexander Bokovoy wrote:
> On Thu, 13 Aug 2015, Fraser Tweedale wrote:
> >On Thu, Aug 13, 2015 at 11:04:42AM +0200, Petr Vobornik wrote:
> >>On 08/13/2015 05:28 AM, Fraser Tweedale wrote:
> >>>On Wed, Aug 12, 2015 at 02:56:54PM +0200, Petr Vobornik wrote:
> usercertificate attr was moved from "System Modify Users" to this
> new permission.
> 
> https://fedorahosted.org/freeipa/ticket/5177
> 
> Note: hosts have permission "System: Manage Host Certificates", services
> don't have it but usercertificate is in "System: Modify Services". I would
> move it as well if usercertificate was not the only attr in "System: 
> Modify
> Services".
> 
> >>>New permission works as expected.
> >>>
> >>>What are the implications of removing userCertificate attribute from
> >>>"Modify Users" ACI?  Users could be relying on it given that there
> >>>is (until now) no more fine-grained permission.
> >>
> >>I'm not sure what is the expected ACI upgrade behavior but applying this
> >>patch on installed server and running ipa-server-upgrade ends with
> >>userCertificate still in "System: Modify Users" permission - it eliminates
> >>your worry. The rest of users who still run IPA < 4.2 won't even notice.
> >>
> >Ah, I see.
> >
> >This raises a different problem: different ACIs on different
> >deployments, depending on when IPA was installed.  Unless I am
> >misunderstanding something, isn't that an undesirable situation?
> We have precedent of upgrading ACIs via upgrade plugins.
> So this is something we need to design for and execute.
>
If we've done it before and the consensus is that it's a problem for
another day, then I suppose it's an ACK from me.

Cheers,
Fraser

-- 
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] Added try/except for error handling ipautil




On 08/10/2015 01:47 PM, Abhijeet Kasurde wrote:

Hi All,

This patch fixes bug - https://fedorahosted.org/freeipa/ticket/3406

Thanks,
Abhijeet Kasurde




Hello,

thank you for the patch

1)
-except ValueError:
+except EOFError, ValueError:

Please use
except (EOFError, ValueError):
https://docs.python.org/2/tutorial/errors.html#handling-exceptions

2)
I'm not sure if this code will work (I did not test it)

I expect when stdin is closed, this will result into infinite loop, 
because raw_input will always return EOFError.


while True:
try:
ret = raw_input("%s: " % prompt)
if allow_empty or ret.strip():
return ret
except EOFError:
pass

-- 
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]-pytest-multihost-Return File Attributes to sftp.put

2015-08-13 Thread Niranjan

Tomas Babej wrote:
> 
> 
> On 08/13/2015 01:55 PM, Niranjan wrote:
> > Greetings,
> > 
> > This patch is regarding pytest-multihost plugin. 
> > Including a patch to return FileAttributes for sftp.put function
> > used in the function.
> > 
> > Current put_file function in transport.py in ParamikoTransport Class doesn't
> > return any value. So when using this function it's not clear if the 
> > operation
> > was sucessfull or not. 
> > 
> > Returning FileAttributes to put_file function helps in checking if operation
> > was indeed succesful. 
> > 
> > Requesting feedback on the patch attached.
> > 
> > Regards
> > Niranjan 
> > 
> > 
> > 
> 
> Note that ParamikoTransport is not the only kind of Transport class in
> the python-multihost. So using the FileAttributes to distinguish
> success/failure does not seem like the way to go.
> 
> If you want to distinguish between a successful/unsuccessful put_file
> call, we should return a boolean value, or rather raise an exception.
I agree, exception would be better approach.
> 
> Cc-ing Petr to chime in.
> 
> Tomas


pgp5vM3ecdiKc.pgp
Description: PGP signature
-- 
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]-pytest-multihost-Return File Attributes to sftp.put

2015-08-13 Thread Niranjan

Martin Basti wrote:
> 
> 
> On 08/13/2015 01:55 PM, Niranjan wrote:
> >Greetings,
> >
> >This patch is regarding pytest-multihost plugin.
> >Including a patch to return FileAttributes for sftp.put function
> >used in the function.
> >
> >Current put_file function in transport.py in ParamikoTransport Class doesn't
> >return any value. So when using this function it's not clear if the operation
> >was sucessfull or not.
> >
> >Returning FileAttributes to put_file function helps in checking if operation
> >was indeed succesful.
> >
> >Requesting feedback on the patch attached.
> >
> >Regards
> >Niranjan
> >
> >
> You mixed tabs and spaces, please use only spaces.
Sure, will use spaces here after. 


pgpOW4Z7Hp6a4.pgp
Description: PGP signature
-- 
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] [PATCHES 0056-0057] improve backing-up of DNSSEC-related files

2015-08-13 Thread Martin Babinsky


PATCH 0056 just fixes a typo in ipaplatform/paths

PATCH 0057 addresses
https://fedorahosted.org/freeipa/ticket/5159


--
Martin^3 Babinsky
From 9835537bdf46305177bba949f9f87313a6dd337e Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Thu, 13 Aug 2015 15:05:36 +0200
Subject: [PATCH 2/2] ipa-backup: archive DNSSEC zone file and kasp.db

https://fedorahosted.org/freeipa/ticket/5159
---
 ipaserver/install/ipa_backup.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ipaserver/install/ipa_backup.py b/ipaserver/install/ipa_backup.py
index 2af2af902dfd77629b589e5ef6cd2a93edecd6ac..ac8f03beb79ebb323e46aa669bab2e24634a744c 100644
--- a/ipaserver/install/ipa_backup.py
+++ b/ipaserver/install/ipa_backup.py
@@ -171,6 +171,8 @@ class Backup(admintool.AdminTool):
 paths.SVC_LIST_FILE,
 paths.OPENDNSSEC_CONF_FILE,
 paths.OPENDNSSEC_KASP_FILE,
+paths.OPENDNSSEC_ZONELIST_FILE,
+paths.OPENDNSSEC_KASP_DB,
 paths.DNSSEC_SOFTHSM2_CONF,
 paths.DNSSEC_SOFTHSM_PIN_SO,
 paths.IPA_ODS_EXPORTER_KEYTAB,
-- 
2.4.3

From 7c77c1aca6e34acd11ba1efdfeda02266596116c Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Thu, 13 Aug 2015 15:03:05 +0200
Subject: [PATCH 1/2] fix typo in BasePathNamespace member pointing to ods
 exporter config

---
 ipaplatform/base/paths.py| 2 +-
 ipaserver/install/ipa_backup.py  | 2 +-
 ipaserver/install/odsexporterinstance.py | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/ipaplatform/base/paths.py b/ipaplatform/base/paths.py
index 4c93c1f7162b0aeb4f798ef84e1ac8db4573518b..0dd3c7fda3020264a1ace8f2d13557cfddf18c2d 100644
--- a/ipaplatform/base/paths.py
+++ b/ipaplatform/base/paths.py
@@ -119,7 +119,7 @@ class BasePathNamespace(object):
 SYSCONFIG_DIRSRV_PKI_IPA_DIR = "/etc/sysconfig/dirsrv-PKI-IPA"
 SYSCONFIG_DIRSRV_SYSTEMD = "/etc/sysconfig/dirsrv.systemd"
 SYSCONFIG_IPA_DNSKEYSYNCD = "/etc/sysconfig/ipa-dnskeysyncd"
-SYSOCNFIG_IPA_ODS_EXPORTER = "/etc/sysconfig/ipa-ods-exporter"
+SYSCONFIG_IPA_ODS_EXPORTER = "/etc/sysconfig/ipa-ods-exporter"
 SYSCONFIG_HTTPD = "/etc/sysconfig/httpd"
 SYSCONFIG_KRB5KDC_DIR = "/etc/sysconfig/krb5kdc"
 SYSCONFIG_NAMED = "/etc/sysconfig/named"
diff --git a/ipaserver/install/ipa_backup.py b/ipaserver/install/ipa_backup.py
index 685c618d8e7b9dcf26e71b99159769ca2b973e7e..2af2af902dfd77629b589e5ef6cd2a93edecd6ac 100644
--- a/ipaserver/install/ipa_backup.py
+++ b/ipaserver/install/ipa_backup.py
@@ -132,7 +132,7 @@ class Backup(admintool.AdminTool):
 paths.SYSCONFIG_KRB5KDC_DIR,
 paths.SYSCONFIG_PKI_CA_PKI_CA_DIR,
 paths.SYSCONFIG_IPA_DNSKEYSYNCD,
-paths.SYSOCNFIG_IPA_ODS_EXPORTER,
+paths.SYSCONFIG_IPA_ODS_EXPORTER,
 paths.SYSCONFIG_NAMED,
 paths.SYSCONFIG_ODS,
 paths.ETC_SYSCONFIG_AUTHCONFIG,
diff --git a/ipaserver/install/odsexporterinstance.py b/ipaserver/install/odsexporterinstance.py
index 51b0f3efc55c4cd86ca451ccc83f88d6562b451e..dd5a92ebd9488d63220684921a3c897e543f237f 100644
--- a/ipaserver/install/odsexporterinstance.py
+++ b/ipaserver/install/odsexporterinstance.py
@@ -86,7 +86,7 @@ class ODSExporterInstance(service.Service):
 root_logger.error("DNSKeyExporter service already exists")
 
 def __setup_key_exporter(self):
-installutils.set_directive(paths.SYSOCNFIG_IPA_ODS_EXPORTER,
+installutils.set_directive(paths.SYSCONFIG_IPA_ODS_EXPORTER,
'SOFTHSM2_CONF',
paths.DNSSEC_SOFTHSM2_CONF,
quotes=False, separator='=')
-- 
2.4.3

-- 
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] 371 Added support for changing vault encryption.




On 08/04/2015 01:20 AM, Endi Sukma Dewata wrote:

The vault-mod command has been modified to support changing vault
encryption attributes (i.e. type, password, public/private keys)
in addition to normal attributes (i.e. description). Changing the
encryption requires retrieving the stored secret with the old
attributes and rearchieving it with the new attributes.

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




Hello, does this patch require any additional patches?

I have current master branch and I cannot apply it.

git am 
freeipa-edewata-0371-Added-support-for-changing-vault-encryption.patch -3

Applying: Added support for changing vault encryption.
error: invalid object 100644 3b62822366a62c90f843a6293589c28383e782ef 
for 'ipalib/plugins/vault.py'

fatal: git-write-tree: error building trees
Repository lacks necessary blobs to fall back on 3-way merge.


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]-pytest-multihost-Return File Attributes to sftp.put

2015-08-13 Thread Tomas Babej

On 08/13/2015 01:55 PM, Niranjan wrote:
> Greetings,
> 
> This patch is regarding pytest-multihost plugin. 
> Including a patch to return FileAttributes for sftp.put function
> used in the function.
> 
> Current put_file function in transport.py in ParamikoTransport Class doesn't
> return any value. So when using this function it's not clear if the operation
> was sucessfull or not. 
> 
> Returning FileAttributes to put_file function helps in checking if operation
> was indeed succesful. 
> 
> Requesting feedback on the patch attached.
> 
> Regards
> Niranjan 
> 
> 
> 

Note that ParamikoTransport is not the only kind of Transport class in
the python-multihost. So using the FileAttributes to distinguish
success/failure does not seem like the way to go.

If you want to distinguish between a successful/unsuccessful put_file
call, we should return a boolean value, or rather raise an exception.

Cc-ing Petr to chime in.

Tomas

-- 
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]-pytest-multihost-Return File Attributes to sftp.put




On 08/13/2015 01:55 PM, Niranjan wrote:

Greetings,

This patch is regarding pytest-multihost plugin.
Including a patch to return FileAttributes for sftp.put function
used in the function.

Current put_file function in transport.py in ParamikoTransport Class doesn't
return any value. So when using this function it's not clear if the operation
was sucessfull or not.

Returning FileAttributes to put_file function helps in checking if operation
was indeed succesful.

Requesting feedback on the patch attached.

Regards
Niranjan



You mixed tabs and spaces, please use only spaces.
-- 
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 019] Asymmetric vault: validate public key in client

2015-08-13 Thread Christian Heimes

On 2015-08-13 14:05, Petr Vobornik wrote:
> On 08/13/2015 12:38 PM, Christian Heimes wrote:
>> On 2015-08-13 12:10, Petr Vobornik wrote:
>>> On 07/23/2015 08:38 PM, Christian Heimes wrote:
 The ipa vault commands now load the public keys in order to verify
 them.
 The validation also prevents a user from accidentally sending her
 private keys to the server. The patch fixes #5142 and #5142.

 $ ./ipa vault-add AsymmetricVault --desc "Asymmetric vault" --type
 asymmetric --public-key-file mykey.pem
 ipa: ERROR: invalid 'ipavaultpublickey': Invalid or unsupported vault
 public key: Could not unserialize key data.

 https://fedorahosted.org/freeipa/ticket/5142
 https://fedorahosted.org/freeipa/ticket/5143

>>>
>>> ACK as fix for 5142.
>>>
>>> I don't think that it fixes 5143. The traceback is fixed therefore 5143
>>> doesn't occur but if there was other traceback raised by
>>> `self.api.Command.vault_archive(*args, **opts)` then the vault added in
>>> `response = self.api.Command.vault_add_internal(*args, **options)` would
>>> be still created.
>>
>> Yes, that is correct. There aren't any arguments that can lead to an
>> exception. The arguments are either already validated by vault_add() or
>> don't raise an error.
>>
>> Of course there are plenty of opportunities errors. The connection to
>> the IPA or LDAP server could fail, NSS DB could be missing and so on.
>> How should we handle an error in vault_archive? Is there another way
>> then to delete the new vault all along?
>>
>> try:
>>  self.api.Command.vault_archive(*args, **opts)
>> except Exception:
>>  log_error()
>>  self.api.Command.vault_del(*args, **opts)
>>  report_error()
>>
>> Christian
>>
> 
> Imho this is the way. But it may fail because of the same root cause as
> vault_archive.
> 
> That said I don't see #5142 as a priority and would defer it.

I'd still like to see my patch for #5142 in RHEL, too. It prevents
accidental exposure of private keys, too. In the test case the test
uploads his private keys to the server. FreeIPA should not leak a user's
private key. My patch prevents that, too.



signature.asc
Description: OpenPGP digital signature
-- 
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 0002] TEST: Stageuser plugin

On 08/11/2015 10:57 AM, Lenka Doudova wrote:

On 08/11/2015 10:06 AM, thierry bordaz wrote:

On 08/04/2015 01:37 PM, Lenka Doudova wrote:

Dne 30.7.2015 v 16:10 Martin Basti napsal(a):

On 30/07/15 16:09, Martin Basti wrote:

On 29/07/15 16:10, Martin Basti wrote:

On 29/07/15 15:29, Lenka Doudova wrote:

Hi,

thanks a lot for the comments, will work on it tomorrow.

Lenka

Dne 29.7.2015 v 15:27 Martin Basti napsal(a):

On 27/07/15 16:47, Lenka Doudova wrote:

Hi,

I'm attaching a patch with automated tests for stageuser
plugin (https://fedorahosted.org/freeipa/ticket/3813). The
user plugin test is affected as well (one class was added).
The tests seem a bit of a mess even to myself, but what with
the way freeipa behaves I didn't know how else to implement
them, but I'm eager to learn how to do it in a nicer way, if
someone has a better idea.

Lenka

I just applied patches:

1) Please remove whitespace errors
$ git am
freeipa-lryznaro-0002-Automated-test-for-stageuser-plugin.patch

Applying: Automated test for stageuser plugin
/home/mbasti/work/freeipa-devel/.git/rebase-apply/patch:110:
trailing whitespace.

""" Tracker class for staged user LDAP object
/home/mbasti/work/freeipa-devel/.git/rebase-apply/patch:113:
trailing whitespace.

StageUserTracker object stores information about the user.
/home/mbasti/work/freeipa-devel/.git/rebase-apply/patch:121:
trailing whitespace.
u'krbprincipalexpiration', u'usercertificate', u'dn',
u'has_keytab', u'has_password',
/home/mbasti/work/freeipa-devel/.git/rebase-apply/patch:122:
trailing whitespace.
u'street', u'postalcode', u'facsimiletelephonenumber',
u'carlicense',
/home/mbasti/work/freeipa-devel/.git/rebase-apply/patch:125:
trailing whitespace.

u'cn', u'ipauniqueid', u'objectclass', u'description',
warning: squelched 50 whitespace errors
warning: 55 lines add whitespace errors.

2)
Please use new shorter format of license header

3) can you fix some of the most serious PEP8 errors
$ git show -U0 | pep8 --diff | wc -l
198

4)
if options != None:

Please use "options *is not* None"

5)
For consistency it should be u'random'
if key == 'random':
self.attrs[u'randompassword'] = fuzzy_string

Otherwise it looks good
Martin^2
--
Martin Basti

And also fix this please

./make-lint
* Module ipatests.test_xmlrpc.test_stageuser_plugin
ipatests/test_xmlrpc/test_stageuser_plugin.py:337:
[E0102(function-redefined), user2] function already defined line 44)

--
Martin Basti

Ahoj, v patchi mas este uvedene svoje stare meno, mala by si v
gite nastavit redhat email

--
Martin Basti

Sorry for spam, you can safely ignore this. :)

--
Martin Basti

Attaching new patch - (hopefully) fixed the errors from the old one
+ few test cases were added.

Lenka

Hello Lenka,

This is a very impressive work and test framework. I have not
understood all the details of the implementation so I just focus on
the reading the tests body.

The patch is looking great to me and I have really few minors comments.

* About non existing stage user, you may try to activate a non
existing one. Is it what TestStagedUser.test_activate does ?

No, it didn't occur to me to test activation of nonexistent user, I
can add it.

* In test_create_attr, I can see that user6 is activated. How is
checked that the specified values are preserved ? (sorry my
python skill is still very low)

The values are checked - in the test itself, using
'create_from_staged' I first copy attributes of stage user object to a
new active user object (just object, not the ipa user itself) and in
'check_activate' I verify that values of ipa user correspond with
those taken from staged user

* Many testcases

(http://www.freeipa.org/page/V4/User_Life-Cycle_Management/Test_Plan#Test_case:_Try_to_search_for_a_nonexistent_user)
are about creating a stage user with various attributes
(initial,shell, homedir...). I found uid/gid, are the others
implemented ?

Yes, all the options are implemented using parametrized fixture
'stageduser2' + 'options_ok' list, tests are performed by
'test_create_attr' method. These tests also contain activation check
(users can be activated and values are preserved).

* In TestActive.test_delete_preserve, does check_delete check the
active container or the stageuser container ?

check_delete just checks the result of 'user-del' command (it just
says that a certain user has been deleted). But after that, there is a
verification that the user is no longer in _active_ container
performed by running 'user-show' command with 'not found' as expected
result.

* Does test_delete_preserved checks that the deleted entry has been
permanently removed ?

If you mean whether there is a check like running 'user-show' command
and expecting 'not found', no, there is not. I haven't thought about
it. Can add.

* Is test_preserved_membership the test case for

Re: [Freeipa-devel] [PATCH 019] Asymmetric vault: validate public key in client


On 08/13/2015 12:38 PM, Christian Heimes wrote:

On 2015-08-13 12:10, Petr Vobornik wrote:

On 07/23/2015 08:38 PM, Christian Heimes wrote:

The ipa vault commands now load the public keys in order to verify them.
The validation also prevents a user from accidentally sending her
private keys to the server. The patch fixes #5142 and #5142.

$ ./ipa vault-add AsymmetricVault --desc "Asymmetric vault" --type
asymmetric --public-key-file mykey.pem
ipa: ERROR: invalid 'ipavaultpublickey': Invalid or unsupported vault
public key: Could not unserialize key data.

https://fedorahosted.org/freeipa/ticket/5142
https://fedorahosted.org/freeipa/ticket/5143



ACK as fix for 5142.

I don't think that it fixes 5143. The traceback is fixed therefore 5143
doesn't occur but if there was other traceback raised by
`self.api.Command.vault_archive(*args, **opts)` then the vault added in
`response = self.api.Command.vault_add_internal(*args, **options)` would
be still created.


Yes, that is correct. There aren't any arguments that can lead to an
exception. The arguments are either already validated by vault_add() or
don't raise an error.

Of course there are plenty of opportunities errors. The connection to
the IPA or LDAP server could fail, NSS DB could be missing and so on.
How should we handle an error in vault_archive? Is there another way
then to delete the new vault all along?

try:
 self.api.Command.vault_archive(*args, **opts)
except Exception:
 log_error()
 self.api.Command.vault_del(*args, **opts)
 report_error()

Christian



Imho this is the way. But it may fail because of the same root cause as 
vault_archive.


That said I don't see #5142 as a priority and would defer it.
--
Petr Vobornik

--
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]-pytest-multihost-Return File Attributes to sftp.put

2015-08-13 Thread Niranjan

Greetings,

This patch is regarding pytest-multihost plugin. 
Including a patch to return FileAttributes for sftp.put function
used in the function.

Current put_file function in transport.py in ParamikoTransport Class doesn't
return any value. So when using this function it's not clear if the operation
was sucessfull or not. 

Returning FileAttributes to put_file function helps in checking if operation
was indeed succesful. 

Requesting feedback on the patch attached.

Regards
Niranjan 
From 5268b93efec39fc0c04f5bd022497143622e87b4 Mon Sep 17 00:00:00 2001
From: Niranjan MR 
Date: Thu, 13 Aug 2015 17:02:45 +0530
Subject: [PATCH] Return File Attributes to sftp.put

This patch passes argument confirm=True to
sftp.put function to return file attributes
of destination file.

Also put_file function now returns FileAttributes
of type SFTPAttributes

Signed-off-by: Niranjan MR 
---
 pytest_multihost/transport.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/pytest_multihost/transport.py b/pytest_multihost/transport.py
index 
2b1ccbc32e8ed4b28e263d9bbe2df4bc2c8617de..206aeea6616cfb34bf421009f947145162e187cf
 100644
--- a/pytest_multihost/transport.py
+++ b/pytest_multihost/transport.py
@@ -244,7 +244,8 @@ class ParamikoTransport(Transport):
 
 def put_file(self, localpath, remotepath):
 self.log.info('PUT %s', remotepath)
-self.sftp.put(localpath, remotepath)
+fileAttributes = self.sftp.put(localpath, remotepath,confirm=True)
+   return fileAttributes
 
 
 class OpenSSHTransport(Transport):
-- 
1.9.3



pgpHHmGGzMTJr.pgp
Description: PGP signature
-- 
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] 0038 cert-request: remove allowed extensions check


On Thu, 13 Aug 2015, Jan Cholasta wrote:

Hi,

On 13.8.2015 07:54, Fraser Tweedale wrote:

The attached patch fixes
https://fedorahosted.org/freeipa/ticket/5205


Simo wrote this some time ago in a (private) discussion about CSR 
extensions:


On 23.1.2014 18:58, Simo Sorce wrote:

Regardless of which tool we use, I really think we need an API that will
list all the extensions, whether they are understood or not, and then we
need to proceed and check that only 'acceptable' extensions are passed
in. Dogtag will do extra validation for sure, but given IPA does access
control, then IPA needs to be sure of what it is checking.


Simo, does this still hold? Fraser's patch removes the check. Is it OK 
or not?

I don't see a contradiction. Nothing prevents us from actually verifying
the certificate request against the certificate profile in IPA
framework and listing the outcome. This does not require to hardcode
actual extensions.
--
/ 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

Re: [Freeipa-devel] [PATCH] 369 Added CLI param and ACL for vault service operations.


On 08/11/2015 08:42 AM, Jan Cholasta wrote:

On 10.8.2015 21:12, Endi Sukma Dewata wrote:

On 8/4/2015 10:32 AM, Endi Sukma Dewata wrote:

Martin, I do not think going on with business as usual is the right
thing to do here. We know this is going to bite.
I suggest Endy adds a *new* API if making it backwards compatible is
not
possible. The era of bumping whole API version must stop, the sooner
the
better.


My point is that we do not know yet how to do this kind of changes
long term.
So what I did not want to end up are 2 copy&pasted Vault plugins
maintained
forever, differing in just that.

If you know how to do this without copypasting, I will be fine with
that.


We probably can do it like this:
* the old plugin continues to provide Vault 1.0 functionality
* the new plugin will be a proxy to the old plugin except for the parts
that have changed in Vault 1.1.

Or the other way around:
* the new plugin will provide Vault 1.1 functionality
* the old plugin will be a proxy to the new plugin except for the parts
that needs to be maintained for Vault 1.0.

The first option is probably safer.

In any case, IPA 4.2.1 will only provide a single client for Vault 1.1,
but two services for Vault 1.0 and 1.1.


A new patch #369-1 is attached. It has been rebased on top of #372 and
#373 that fix the conflicting parameter while maintaining backward
compatibility.


I have modified the first version of the patch to maintain backward
compatibility and not require your patches #372 and #373. Should be much
easier to review. See attachment.



Jan approach seems better to me for 4.2.1. Endi, do you agree with the 
changes? Could we proceed with the review?


--
Petr Vobornik

--
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] 0038 cert-request: remove allowed extensions check

2015-08-13 Thread Jan Cholasta


Hi,

On 13.8.2015 07:54, Fraser Tweedale wrote:

The attached patch fixes
https://fedorahosted.org/freeipa/ticket/5205


Simo wrote this some time ago in a (private) discussion about CSR 
extensions:


On 23.1.2014 18:58, Simo Sorce wrote:

Regardless of which tool we use, I really think we need an API that will
list all the extensions, whether they are understood or not, and then we
need to proceed and check that only 'acceptable' extensions are passed
in. Dogtag will do extra validation for sure, but given IPA does access
control, then IPA needs to be sure of what it is checking.


Simo, does this still hold? Fraser's patch removes the check. Is it OK 
or not?


Honza

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

Re: [Freeipa-devel] [PATCH 019] Asymmetric vault: validate public key in client

2015-08-13 Thread Christian Heimes

On 2015-08-13 12:10, Petr Vobornik wrote:
> On 07/23/2015 08:38 PM, Christian Heimes wrote:
>> The ipa vault commands now load the public keys in order to verify them.
>> The validation also prevents a user from accidentally sending her
>> private keys to the server. The patch fixes #5142 and #5142.
>>
>> $ ./ipa vault-add AsymmetricVault --desc "Asymmetric vault" --type
>> asymmetric --public-key-file mykey.pem
>> ipa: ERROR: invalid 'ipavaultpublickey': Invalid or unsupported vault
>> public key: Could not unserialize key data.
>>
>> https://fedorahosted.org/freeipa/ticket/5142
>> https://fedorahosted.org/freeipa/ticket/5143
>>
> 
> ACK as fix for 5142.
> 
> I don't think that it fixes 5143. The traceback is fixed therefore 5143
> doesn't occur but if there was other traceback raised by
> `self.api.Command.vault_archive(*args, **opts)` then the vault added in
> `response = self.api.Command.vault_add_internal(*args, **options)` would
> be still created.

Yes, that is correct. There aren't any arguments that can lead to an
exception. The arguments are either already validated by vault_add() or
don't raise an error.

Of course there are plenty of opportunities errors. The connection to
the IPA or LDAP server could fail, NSS DB could be missing and so on.
How should we handle an error in vault_archive? Is there another way
then to delete the new vault all along?

try:
self.api.Command.vault_archive(*args, **opts)
except Exception:
log_error()
self.api.Command.vault_del(*args, **opts)
report_error()

Christian



signature.asc
Description: OpenPGP digital signature
-- 
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] Topology Plugin design questions

2015-08-13 Thread Ludwig Krispenz

On 08/13/2015 10:49 AM, Petr Vobornik wrote:

On 08/13/2015 09:55 AM, Ludwig Krispenz wrote:

On 08/10/2015 10:54 AM, Oleg Fayans wrote:

Hi Ludwig,

although it was generally agreed, that we do not use plugin version at
all.

2. The section
http://www.freeipa.org/page/V4/Manage_replication_topology#Check_after_online_initializatition

this is not true, in segment add youcan specify the direction:

onedirect
-
Added segment "onedirect"
-
Segment name: onedirect
Left node: vm-112.abc.idm.lab.eng.brq.redhat.com
Right node: vm-179.abc.idm.lab.eng.brq.redhat.com
Connectivity: left-right

checking the segment:

This is a bug. Option "direction" was removed from -add and -mod
commands on purpose.
I thought it should only be removed from the mod, as it was not handled
in the plugin, but I think initial creation of a one directional segment
should be ok

But CLI still incorrectly asks for the value and therefore allows to
change the default "both".

--
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 019] Asymmetric vault: validate public key in client


On 07/23/2015 08:38 PM, Christian Heimes wrote:

The ipa vault commands now load the public keys in order to verify them.
The validation also prevents a user from accidentally sending her
private keys to the server. The patch fixes #5142 and #5142.

$ ./ipa vault-add AsymmetricVault --desc "Asymmetric vault" --type
asymmetric --public-key-file mykey.pem
ipa: ERROR: invalid 'ipavaultpublickey': Invalid or unsupported vault
public key: Could not unserialize key data.

https://fedorahosted.org/freeipa/ticket/5142
https://fedorahosted.org/freeipa/ticket/5143



ACK as fix for 5142.

I don't think that it fixes 5143. The traceback is fixed therefore 5143 
doesn't occur but if there was other traceback raised by 
`self.api.Command.vault_archive(*args, **opts)` then the vault added in 
`response = self.api.Command.vault_add_internal(*args, **options)` would 
be still created.

--
Petr Vobornik

--
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] 0039 Prohibit deletion of included profiles

On Thu, Aug 13, 2015 at 12:31:27PM +0300, Alexander Bokovoy wrote:
> On Thu, 13 Aug 2015, Fraser Tweedale wrote:
> >On Thu, Aug 13, 2015 at 12:01:09PM +0300, Alexander Bokovoy wrote:
> >>On Thu, 13 Aug 2015, Fraser Tweedale wrote:
> >>>On Thu, Aug 13, 2015 at 09:53:35AM +0300, Alexander Bokovoy wrote:
> On Thu, 13 Aug 2015, Fraser Tweedale wrote:
> >The attached patch fixes
> >https://fedorahosted.org/freeipa/ticket/5198
> >
> >Thanks,
> >Fraser
> 
> >From 0dd316bf0cbab7b6701bd69f142e82b30bee25b8 Mon Sep 17 00:00:00 2001
> >From: Fraser Tweedale 
> >Date: Thu, 13 Aug 2015 02:32:54 -0400
> >Subject: [PATCH] Prohibit deletion of included profiles
> >
> >Deletion of included profiles, including the default profile, should
> >not be allowed.  Detect this case and raise an error.
> >
> >Also update the included profiles collection to use namedtuple,
> >making it easier to access the various components.
> >
> >Fixes: https://fedorahosted.org/freeipa/ticket/5198
> >---
> >ipalib/plugins/certprofile.py | 13 +++--
> >ipapython/dogtag.py   |  8 +---
> >2 files changed, 16 insertions(+), 5 deletions(-)
> >
> >diff --git a/ipalib/plugins/certprofile.py 
> >b/ipalib/plugins/certprofile.py
> >index 
> >1dd4f403ee4461b83c053eb36019a8896506bb81..03bdd28728dc864adcd7305ddbff34a23405e78f
> > 100644
> >--- a/ipalib/plugins/certprofile.py
> >+++ b/ipalib/plugins/certprofile.py
> >@@ -3,6 +3,7 @@
> >#
> >
> >import re
> >+from operator import attrgetter
> >
> >from ipalib import api, Bool, File, Str
> >from ipalib import output, util
> >@@ -14,6 +15,7 @@ from ipalib.plugins.baseldap import (
> >from ipalib.request import context
> >from ipalib import ngettext
> >from ipalib.text import _
> >+from ipapython.dogtag import INCLUDED_PROFILES
> >from ipapython.version import API_VERSION
> >
> >from ipalib import errors
> >@@ -287,9 +289,16 @@ class certprofile_del(LDAPDelete):
> >__doc__ = _("Delete a Certificate Profile.")
> >msg_summary = _('Deleted profile "%(value)s"')
> >
> >-def execute(self, *args, **kwargs):
> >+def pre_callback(self, ldap, dn, *keys, **options):
> >ca_enabled_check()
> >-return super(certprofile_del, self).execute(*args, **kwargs)
> >+
> >+if keys[0] in map(attrgetter('profile_id'), INCLUDED_PROFILES):
> >+raise errors.ValidationError(name='profile_id',
> >+error=_("Included profile '%(profile_id)s' cannot be 
> >deleted")
> >+% {'profile_id': keys[0]}
> >+)
> >+
> >+return dn
> I think you also want to protect the included profiles from renaming.
> 
> >>>This is already the case.
> >>I'm also wondering about certprofile-mod changing the profile content
> >>and changing profileID there to point to existing profile. Would this
> >>affect CA operation?
> >>
> >Renaming profile / changing profile-id / pointing it to a different
> >profile is not possible.
> >
> >Changing profile content *is* currently possible.  Given that we
> >have custom profiles now, there is an argument to be made that we
> >should prevent profile-mod for updating the Dogtag configuration of
> >predefined profiles.
> >
> >If we did that, we would probably also want to allow admins to
> >change which is the default profile, i.e. changing the default to
> >some custom profile they added.
> >
> >And if we did that, then perhaps we should let them specify a
> >different default profile for users vs hosts/services!
> >
> >How deep does this rabbit hole go? :)
> All the above makes sense and should be done in terms of proper
> hardening and usability fixes. I don't think it is a bottomless hole,
> though, just a normal work we have to do to make certificate profiles
> nice and usable :)
> 
Right; I'll file tickets for these explored regions of the hole, and
leave the unexplored depths for another day.

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

Re: [Freeipa-devel] [PATCH] 0039 Prohibit deletion of included profiles


On Thu, 13 Aug 2015, Fraser Tweedale wrote:

On Thu, Aug 13, 2015 at 12:01:09PM +0300, Alexander Bokovoy wrote:

On Thu, 13 Aug 2015, Fraser Tweedale wrote:
>On Thu, Aug 13, 2015 at 09:53:35AM +0300, Alexander Bokovoy wrote:
>>On Thu, 13 Aug 2015, Fraser Tweedale wrote:
>>>The attached patch fixes
>>>https://fedorahosted.org/freeipa/ticket/5198
>>>
>>>Thanks,
>>>Fraser
>>
>>>From 0dd316bf0cbab7b6701bd69f142e82b30bee25b8 Mon Sep 17 00:00:00 2001
>>>From: Fraser Tweedale 
>>>Date: Thu, 13 Aug 2015 02:32:54 -0400
>>>Subject: [PATCH] Prohibit deletion of included profiles
>>>
>>>Deletion of included profiles, including the default profile, should
>>>not be allowed.  Detect this case and raise an error.
>>>
>>>Also update the included profiles collection to use namedtuple,
>>>making it easier to access the various components.
>>>
>>>Fixes: https://fedorahosted.org/freeipa/ticket/5198
>>>---
>>>ipalib/plugins/certprofile.py | 13 +++--
>>>ipapython/dogtag.py   |  8 +---
>>>2 files changed, 16 insertions(+), 5 deletions(-)
>>>
>>>diff --git a/ipalib/plugins/certprofile.py b/ipalib/plugins/certprofile.py
>>>index 
1dd4f403ee4461b83c053eb36019a8896506bb81..03bdd28728dc864adcd7305ddbff34a23405e78f 100644
>>>--- a/ipalib/plugins/certprofile.py
>>>+++ b/ipalib/plugins/certprofile.py
>>>@@ -3,6 +3,7 @@
>>>#
>>>
>>>import re
>>>+from operator import attrgetter
>>>
>>>from ipalib import api, Bool, File, Str
>>>from ipalib import output, util
>>>@@ -14,6 +15,7 @@ from ipalib.plugins.baseldap import (
>>>from ipalib.request import context
>>>from ipalib import ngettext
>>>from ipalib.text import _
>>>+from ipapython.dogtag import INCLUDED_PROFILES
>>>from ipapython.version import API_VERSION
>>>
>>>from ipalib import errors
>>>@@ -287,9 +289,16 @@ class certprofile_del(LDAPDelete):
>>>__doc__ = _("Delete a Certificate Profile.")
>>>msg_summary = _('Deleted profile "%(value)s"')
>>>
>>>-def execute(self, *args, **kwargs):
>>>+def pre_callback(self, ldap, dn, *keys, **options):
>>>ca_enabled_check()
>>>-return super(certprofile_del, self).execute(*args, **kwargs)
>>>+
>>>+if keys[0] in map(attrgetter('profile_id'), INCLUDED_PROFILES):
>>>+raise errors.ValidationError(name='profile_id',
>>>+error=_("Included profile '%(profile_id)s' cannot be 
deleted")
>>>+% {'profile_id': keys[0]}
>>>+)
>>>+
>>>+return dn
>>I think you also want to protect the included profiles from renaming.
>>
>This is already the case.
I'm also wondering about certprofile-mod changing the profile content
and changing profileID there to point to existing profile. Would this
affect CA operation?


Renaming profile / changing profile-id / pointing it to a different
profile is not possible.

Changing profile content *is* currently possible.  Given that we
have custom profiles now, there is an argument to be made that we
should prevent profile-mod for updating the Dogtag configuration of
predefined profiles.

If we did that, we would probably also want to allow admins to
change which is the default profile, i.e. changing the default to
some custom profile they added.

And if we did that, then perhaps we should let them specify a
different default profile for users vs hosts/services!

How deep does this rabbit hole go? :)

All the above makes sense and should be done in terms of proper
hardening and usability fixes. I don't think it is a bottomless hole,
though, just a normal work we have to do to make certificate profiles
nice and usable :)

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

Re: [Freeipa-devel] [PATCH] 910 add permission: System: Manage User Certificates

On Thu, 13 Aug 2015, Fraser Tweedale wrote:

On Thu, Aug 13, 2015 at 11:04:42AM +0200, Petr Vobornik wrote:

On 08/13/2015 05:28 AM, Fraser Tweedale wrote:
>On Wed, Aug 12, 2015 at 02:56:54PM +0200, Petr Vobornik wrote:
>>usercertificate attr was moved from "System Modify Users" to this
>>new permission.
>>
>>https://fedorahosted.org/freeipa/ticket/5177
>>
>>Note: hosts have permission "System: Manage Host Certificates", services
>>don't have it but usercertificate is in "System: Modify Services". I would
>>move it as well if usercertificate was not the only attr in "System: Modify
>>Services".
>>
>New permission works as expected.
>
>What are the implications of removing userCertificate attribute from
>"Modify Users" ACI?  Users could be relying on it given that there
>is (until now) no more fine-grained permission.

I'm not sure what is the expected ACI upgrade behavior but applying this
patch on installed server and running ipa-server-upgrade ends with
userCertificate still in "System: Modify Users" permission - it eliminates
your worry. The rest of users who still run IPA < 4.2 won't even notice.

Ah, I see.

This raises a different problem: different ACIs on different
deployments, depending on when IPA was installed.  Unless I am
misunderstanding something, isn't that an undesirable situation?

We have precedent of upgrading ACIs via upgrade plugins.
So this is something we need to design for and execute.
--
/ 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

Re: [Freeipa-devel] [PATCH] 0039 Prohibit deletion of included profiles

On Thu, Aug 13, 2015 at 12:01:09PM +0300, Alexander Bokovoy wrote:
> On Thu, 13 Aug 2015, Fraser Tweedale wrote:
> >On Thu, Aug 13, 2015 at 09:53:35AM +0300, Alexander Bokovoy wrote:
> >>On Thu, 13 Aug 2015, Fraser Tweedale wrote:
> >>>The attached patch fixes
> >>>https://fedorahosted.org/freeipa/ticket/5198
> >>>
> >>>Thanks,
> >>>Fraser
> >>
> >>>From 0dd316bf0cbab7b6701bd69f142e82b30bee25b8 Mon Sep 17 00:00:00 2001
> >>>From: Fraser Tweedale 
> >>>Date: Thu, 13 Aug 2015 02:32:54 -0400
> >>>Subject: [PATCH] Prohibit deletion of included profiles
> >>>
> >>>Deletion of included profiles, including the default profile, should
> >>>not be allowed.  Detect this case and raise an error.
> >>>
> >>>Also update the included profiles collection to use namedtuple,
> >>>making it easier to access the various components.
> >>>
> >>>Fixes: https://fedorahosted.org/freeipa/ticket/5198
> >>>---
> >>>ipalib/plugins/certprofile.py | 13 +++--
> >>>ipapython/dogtag.py   |  8 +---
> >>>2 files changed, 16 insertions(+), 5 deletions(-)
> >>>
> >>>diff --git a/ipalib/plugins/certprofile.py b/ipalib/plugins/certprofile.py
> >>>index 
> >>>1dd4f403ee4461b83c053eb36019a8896506bb81..03bdd28728dc864adcd7305ddbff34a23405e78f
> >>> 100644
> >>>--- a/ipalib/plugins/certprofile.py
> >>>+++ b/ipalib/plugins/certprofile.py
> >>>@@ -3,6 +3,7 @@
> >>>#
> >>>
> >>>import re
> >>>+from operator import attrgetter
> >>>
> >>>from ipalib import api, Bool, File, Str
> >>>from ipalib import output, util
> >>>@@ -14,6 +15,7 @@ from ipalib.plugins.baseldap import (
> >>>from ipalib.request import context
> >>>from ipalib import ngettext
> >>>from ipalib.text import _
> >>>+from ipapython.dogtag import INCLUDED_PROFILES
> >>>from ipapython.version import API_VERSION
> >>>
> >>>from ipalib import errors
> >>>@@ -287,9 +289,16 @@ class certprofile_del(LDAPDelete):
> >>>__doc__ = _("Delete a Certificate Profile.")
> >>>msg_summary = _('Deleted profile "%(value)s"')
> >>>
> >>>-def execute(self, *args, **kwargs):
> >>>+def pre_callback(self, ldap, dn, *keys, **options):
> >>>ca_enabled_check()
> >>>-return super(certprofile_del, self).execute(*args, **kwargs)
> >>>+
> >>>+if keys[0] in map(attrgetter('profile_id'), INCLUDED_PROFILES):
> >>>+raise errors.ValidationError(name='profile_id',
> >>>+error=_("Included profile '%(profile_id)s' cannot be 
> >>>deleted")
> >>>+% {'profile_id': keys[0]}
> >>>+)
> >>>+
> >>>+return dn
> >>I think you also want to protect the included profiles from renaming.
> >>
> >This is already the case.
> I'm also wondering about certprofile-mod changing the profile content
> and changing profileID there to point to existing profile. Would this
> affect CA operation?
> 
Renaming profile / changing profile-id / pointing it to a different
profile is not possible.

Changing profile content *is* currently possible.  Given that we
have custom profiles now, there is an argument to be made that we
should prevent profile-mod for updating the Dogtag configuration of
predefined profiles.

If we did that, we would probably also want to allow admins to
change which is the default profile, i.e. changing the default to
some custom profile they added.

And if we did that, then perhaps we should let them specify a
different default profile for users vs hosts/services!

How deep does this rabbit hole go? :)
If anyone else has thoughts please chime in!

Cheers,
Fraser

-- 
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] 910 add permission: System: Manage User Certificates

On Thu, Aug 13, 2015 at 11:04:42AM +0200, Petr Vobornik wrote:
> On 08/13/2015 05:28 AM, Fraser Tweedale wrote:
> >On Wed, Aug 12, 2015 at 02:56:54PM +0200, Petr Vobornik wrote:
> >>usercertificate attr was moved from "System Modify Users" to this
> >>new permission.
> >>
> >>https://fedorahosted.org/freeipa/ticket/5177
> >>
> >>Note: hosts have permission "System: Manage Host Certificates", services
> >>don't have it but usercertificate is in "System: Modify Services". I would
> >>move it as well if usercertificate was not the only attr in "System: Modify
> >>Services".
> >>
> >New permission works as expected.
> >
> >What are the implications of removing userCertificate attribute from
> >"Modify Users" ACI?  Users could be relying on it given that there
> >is (until now) no more fine-grained permission.
> 
> I'm not sure what is the expected ACI upgrade behavior but applying this
> patch on installed server and running ipa-server-upgrade ends with
> userCertificate still in "System: Modify Users" permission - it eliminates
> your worry. The rest of users who still run IPA < 4.2 won't even notice.
> 
Ah, I see.

This raises a different problem: different ACIs on different
deployments, depending on when IPA was installed.  Unless I am
misunderstanding something, isn't that an undesirable situation?

Thanks,
Fraser

> >
> >Perhaps we should
> >
> >a) use update script to add the new permission to any roles that
> >have the Modify Users permission, or
> >b) not remove the userCertificate attribute from the ACI, or
> >c) deem this change acceptable and leave the patch as-is, in which
> >case: ACK
> >
> >Cheers,
> >Fraser
> >
> -- 
> Petr Vobornik

-- 
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] 910 add permission: System: Manage User Certificates


On 08/13/2015 05:28 AM, Fraser Tweedale wrote:

On Wed, Aug 12, 2015 at 02:56:54PM +0200, Petr Vobornik wrote:

usercertificate attr was moved from "System Modify Users" to this
new permission.

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

Note: hosts have permission "System: Manage Host Certificates", services
don't have it but usercertificate is in "System: Modify Services". I would
move it as well if usercertificate was not the only attr in "System: Modify
Services".


New permission works as expected.

What are the implications of removing userCertificate attribute from
"Modify Users" ACI?  Users could be relying on it given that there
is (until now) no more fine-grained permission.


I'm not sure what is the expected ACI upgrade behavior but applying this 
patch on installed server and running ipa-server-upgrade ends with 
userCertificate still in "System: Modify Users" permission - it 
eliminates your worry. The rest of users who still run IPA < 4.2 won't 
even notice.




Perhaps we should

a) use update script to add the new permission to any roles that
have the Modify Users permission, or
b) not remove the userCertificate attribute from the ACI, or
c) deem this change acceptable and leave the patch as-is, in which
case: ACK

Cheers,
Fraser


--
Petr Vobornik

--
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] 0039 Prohibit deletion of included profiles


On Thu, 13 Aug 2015, Fraser Tweedale wrote:

On Thu, Aug 13, 2015 at 09:53:35AM +0300, Alexander Bokovoy wrote:

On Thu, 13 Aug 2015, Fraser Tweedale wrote:
>The attached patch fixes
>https://fedorahosted.org/freeipa/ticket/5198
>
>Thanks,
>Fraser

>From 0dd316bf0cbab7b6701bd69f142e82b30bee25b8 Mon Sep 17 00:00:00 2001
>From: Fraser Tweedale 
>Date: Thu, 13 Aug 2015 02:32:54 -0400
>Subject: [PATCH] Prohibit deletion of included profiles
>
>Deletion of included profiles, including the default profile, should
>not be allowed.  Detect this case and raise an error.
>
>Also update the included profiles collection to use namedtuple,
>making it easier to access the various components.
>
>Fixes: https://fedorahosted.org/freeipa/ticket/5198
>---
>ipalib/plugins/certprofile.py | 13 +++--
>ipapython/dogtag.py   |  8 +---
>2 files changed, 16 insertions(+), 5 deletions(-)
>
>diff --git a/ipalib/plugins/certprofile.py b/ipalib/plugins/certprofile.py
>index 
1dd4f403ee4461b83c053eb36019a8896506bb81..03bdd28728dc864adcd7305ddbff34a23405e78f 
100644
>--- a/ipalib/plugins/certprofile.py
>+++ b/ipalib/plugins/certprofile.py
>@@ -3,6 +3,7 @@
>#
>
>import re
>+from operator import attrgetter
>
>from ipalib import api, Bool, File, Str
>from ipalib import output, util
>@@ -14,6 +15,7 @@ from ipalib.plugins.baseldap import (
>from ipalib.request import context
>from ipalib import ngettext
>from ipalib.text import _
>+from ipapython.dogtag import INCLUDED_PROFILES
>from ipapython.version import API_VERSION
>
>from ipalib import errors
>@@ -287,9 +289,16 @@ class certprofile_del(LDAPDelete):
>__doc__ = _("Delete a Certificate Profile.")
>msg_summary = _('Deleted profile "%(value)s"')
>
>-def execute(self, *args, **kwargs):
>+def pre_callback(self, ldap, dn, *keys, **options):
>ca_enabled_check()
>-return super(certprofile_del, self).execute(*args, **kwargs)
>+
>+if keys[0] in map(attrgetter('profile_id'), INCLUDED_PROFILES):
>+raise errors.ValidationError(name='profile_id',
>+error=_("Included profile '%(profile_id)s' cannot be deleted")
>+% {'profile_id': keys[0]}
>+)
>+
>+return dn
I think you also want to protect the included profiles from renaming.


This is already the case.

I'm also wondering about certprofile-mod changing the profile content
and changing profileID there to point to existing profile. Would this
affect CA operation?

(ACK below for the current code).



And I would change 'Included profile ... cannot be deleted' to
'Predefined profile ... cannot be deleted'.


Fair enough; updated patch attached.

Cheers,
Fraser



From 4dd4e7c273a04e8b386c229959a99d6ec8e55c14 Mon Sep 17 00:00:00 2001
From: Fraser Tweedale 
Date: Thu, 13 Aug 2015 02:32:54 -0400
Subject: [PATCH] Prohibit deletion of predefined profiles

Deletion of predefined profiles, including the default profile,
should not be allowed.  Detect this case and raise an error.

Also update the predefined profiles collection to use namedtuple,
making it easier to access the various components.

Fixes: https://fedorahosted.org/freeipa/ticket/5198
---
ipalib/plugins/certprofile.py | 13 +++--
ipapython/dogtag.py   |  8 +---
2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/ipalib/plugins/certprofile.py b/ipalib/plugins/certprofile.py
index 
1dd4f403ee4461b83c053eb36019a8896506bb81..007cc543406b7e5705fd7474f3685cd6a9ce6aca
 100644
--- a/ipalib/plugins/certprofile.py
+++ b/ipalib/plugins/certprofile.py
@@ -3,6 +3,7 @@
#

import re
+from operator import attrgetter

from ipalib import api, Bool, File, Str
from ipalib import output, util
@@ -14,6 +15,7 @@ from ipalib.plugins.baseldap import (
from ipalib.request import context
from ipalib import ngettext
from ipalib.text import _
+from ipapython.dogtag import INCLUDED_PROFILES
from ipapython.version import API_VERSION

from ipalib import errors
@@ -287,9 +289,16 @@ class certprofile_del(LDAPDelete):
__doc__ = _("Delete a Certificate Profile.")
msg_summary = _('Deleted profile "%(value)s"')

-def execute(self, *args, **kwargs):
+def pre_callback(self, ldap, dn, *keys, **options):
ca_enabled_check()
-return super(certprofile_del, self).execute(*args, **kwargs)
+
+if keys[0] in map(attrgetter('profile_id'), INCLUDED_PROFILES):
+raise errors.ValidationError(name='profile_id',
+error=_("Predefined profile '%(profile_id)s' cannot be 
deleted")
+% {'profile_id': keys[0]}
+)
+
+return dn

def post_callback(self, ldap, dn, *keys, **options):
with self.api.Backend.ra_certprofile as profile_api:
diff --git a/ipapython/dogtag.py b/ipapython/dogtag.py
index 
99bdf066d64d626af05d93953117909c5fbfb693..fc4154719e31eb32e28587ea89fb04ead14d282e
 100644
--- a/ipapython/dogtag.py
+++ b/ipapython/dogtag.py
@@ -17,6 +17,7 @@
# along with this program.  If not, se

Re: [Freeipa-devel] Topology Plugin design questions

On 08/13/2015 09:55 AM, Ludwig Krispenz wrote:

On 08/10/2015 10:54 AM, Oleg Fayans wrote:

Hi Ludwig,

2. The section
http://www.freeipa.org/page/V4/Manage_replication_topology#Check_after_online_initializatition
should be a bit clarified:
Does this mean, that if we prepare a replica from a master that has
domainlevel = 1, then the replica, that already had a domain level = 0
will raise it? Do we support this scenario at all?

this is not true, in segment add youcan specify the direction:

adding the segment:
-
[root@vm-215 ~]# ipa topologysegment-add realm
Left node: vm-112.abc.idm.lab.eng.brq.redhat.com
Right node: vm-179.abc.idm.lab.eng.brq.redhat.com
Connectivity [both]: left-right
Segment name
[vm-112.abc.idm.lab.eng.brq.redhat.com-to-vm-179.abc.idm.lab.eng.brq.redhat.com]:
onedirect
-
Added segment "onedirect"
-
Segment name: onedirect
Left node: vm-112.abc.idm.lab.eng.brq.redhat.com
Right node: vm-179.abc.idm.lab.eng.brq.redhat.com
Connectivity: left-right

checking the segment:

This is a bug. Option "direction" was removed from -add and -mod
commands on purpose. But CLI still incorrectly asks for the value and
therefore allows to change the default "both".

--
Petr Vobornik

--
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] 0039 Prohibit deletion of included profiles