Re: [Freeipa-devel] [PATCH] Password vault
Dne 7.7.2015 v 16:42 Endi Sukma Dewata napsal(a):
- Original Message -
On 07/07/2015 10:51 AM, Jan Cholasta wrote:
Dne 3.7.2015 v 15:44 Endi Sukma Dewata napsal(a):
Here is the rebased patch for vault access control.
LGTM, except:
@@ -356,6 +386,13 @@ class vault(LDAPObject):
{
'objectclass': ['nsContainer'],
'cn': rdn['cn'],
+'aci':
+'(targetfilter=(objectClass=ipaVault))' +
+'(version 3.0; ' +
+'acl User can manage private vaults; ' +
+'allow(read, search, compare, add, delete) ' +
+'userdn=ldap:///%s;;)'
+% owner_dn
})
# if entry can be added, return
I don't think dynamically creating ACIs with hardcoded userdn is something
we
want to do. This should be handled by a single ACI in cn=vaults.
+1. Single ACI like
+default: aci: (targetfilter=(objectClass=ipaVault))(version 3.0; acl
Vault
owners can manage the vault; allow(read, search, compare, write)
userattr=owner#USERDN;)
you already have there is more preferred.
New patch attached. For this to work the container itself needs an 'owner'
attribute, so I changed the nsContainer into ipaVaultContainer.
I don't think that's really necessary on the top-level containers.
Anyway, the patch works, so ACK.
Pushed to master: bf6df3df9b388753a52a0040d9c15b1eabce41ca
--
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] Password vault
Dne 3.7.2015 v 15:44 Endi Sukma Dewata napsal(a):
Here is the rebased patch for vault access control.
LGTM, except:
@@ -356,6 +386,13 @@ class vault(LDAPObject):
{
'objectclass': ['nsContainer'],
'cn': rdn['cn'],
+'aci':
+'(targetfilter=(objectClass=ipaVault))' +
+'(version 3.0; ' +
+'acl User can manage private vaults; ' +
+'allow(read, search, compare, add, delete) ' +
+'userdn=ldap:///%s;;)'
+% owner_dn
})
# if entry can be added, return
I don't think dynamically creating ACIs with hardcoded userdn is
something we want to do. This should be handled by a single ACI in
cn=vaults.
--
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] Password vault
Dne 3.7.2015 v 14:23 Endi Sukma Dewata napsal(a): On 7/1/2015 1:53 AM, Jan Cholasta wrote: I think it would be better to use a new attribute type which inherits from ipaPublicKey (ipaVaultPublicKey?) rather than ipaPublicKey directly for assymetric vault public keys, so that assymetric public key and escrow public key are on the same level and you can still use ipaPublicKey to refer to either one: ipaPublicKey ipaVaultPublicKey ipaEscrowPublicKey OK. To be consistent the parameters need to be renamed too: --vault-public-key and --vault-public-key-file. It doesn't need to, there is no requirement for CLI names to always match attribute names. (Also I don't insist on the name ipaVaultPublicKey, feel free to change it if you want.) It's unchanged for now. In a previous discussion it was advised to reuse the existing attribute type whenever possible. Well, in this discussion, it is not. Escrow public key should also reuse ipaPublicKey, but it can't if you use it for vault public key. By using ipaPublicKey subtypes you can distinguish between the two uses and still use ipaPublicKey to refer to either of them. So what's changed? This is what you said when I posted the same patch six months ago: In this patch I'm adding ipaVaultSalt and ipaVaultPublicKey attribute types to store salt and public key for vault. Are there existing attribute types that I can use instead? I see there's an ipaPublicKey, should I use that and maybe add ipaSalt/ipaEncSalt? Thanks. yes, please re-use existing attributes where possible. Honza What changed is that I now know there is also escrow public key, which I didn't know six months ago. Here's patch #368 to be applied on top of patch #357-5, but see comments below. Thanks for the patch. Could you show me how to use ipaPublicKey to refer to ipaVaultPublicKey and ipaEscrowPublicKey? Under what situation would that be useful? For example for ipaPublicKey searches - if ipaVaultPublicKey and ipaEscrowPublicKey both inherit from ipaPublicKey, then an ipaPublicKey search will look in both ipaVaultPublicKey and ipaEscrowPublicKey. This is not something we actually need right now, but once the schema is done, it can't be fixed and I don't think we should prevent this, especially since we can get it for free. BTW even the core LDAP schema does this, see for example how the cn attribute inherits from the more general name attribute: https://tools.ietf.org/html/rfc4519#section-2.3. I don't think that's how LDAP works. It is, see https://tools.ietf.org/html/rfc4512#section-2.5.3. The RFC doesn't say that either. The cn does inherit from name, but if you search for name it won't match/return cn. See queries below: $ ldapsearch -LLL -x -b dc=example,dc=com (cn=Accounting Managers) dn: cn=Accounting Managers,ou=Groups,dc=example,dc=com objectClass: top objectClass: groupOfUniqueNames cn: Accounting Managers ou: groups description: People who can manage accounting entries uniqueMember: cn=Directory Manager $ ldapsearch -LLL -x -b dc=example,dc=com (cn=Accounting Managers) \ name dn: cn=Accounting Managers,ou=Groups,dc=example,dc=com (no cn attribute) $ ldapsearch -LLL -x -b dc=example,dc=com (name=Accounting Managers) (no result) This seems like a bug in 389 DS, it works correctly with OpenLDAP: $ ldapsearch -H ldap://localhost -D 'cn=Manager,dc=example,dc=com' -w password -b 'dc=example,dc=com' '(name=Manager)' dn: cn=Manager,dc=example,dc=com objectClass: organizationalRole cn: Manager Assuming this is what you meant, which doesn't seem to be working, is there still a valid reason to add a new ipaVaultPublicKey instead of using the existing ipaPublicKey? I think everything mentioned in the RFC section I linked above is a good enough reason. * CLI options will be identical to client and server API options (i.e. no CLI-only, client-only, or server-only options) Actually, you can create CLI-only options (add include='cli' to the param's kwargs). I need to look at this more closely. If I understand correctly in user_del there are two 'preserve' options, the Bool preserve is for client and server API, and the Flag preserve is for CLI. Wouldn't it be better if they are stored in separate lists (or maybe separate classes)? And it looks like you still need to delete the CLI options explicitly anyway. Well, it would be better if there was no Flag class at all and flags were handled by CLI exclusively, because parameter classes should reflect the data type (bool) and not the presentation (flag). That indicates there should be a separation between client API and the CLI too because, as you see in user_del, they can be different. Not really, what there should be is separation between data type and presentation. This is what the web UI already does and so should the CLI. Does the API.txt actually show the CLI options, the client API options, or the server API options? I only see the Flag preserve, not the Bool
Re: [Freeipa-devel] [PATCH] Password vault
On 07/07/2015 10:51 AM, Jan Cholasta wrote:
Dne 3.7.2015 v 15:44 Endi Sukma Dewata napsal(a):
Here is the rebased patch for vault access control.
LGTM, except:
@@ -356,6 +386,13 @@ class vault(LDAPObject):
{
'objectclass': ['nsContainer'],
'cn': rdn['cn'],
+'aci':
+'(targetfilter=(objectClass=ipaVault))' +
+'(version 3.0; ' +
+'acl User can manage private vaults; ' +
+'allow(read, search, compare, add, delete) ' +
+'userdn=ldap:///%s;;)'
+% owner_dn
})
# if entry can be added, return
I don't think dynamically creating ACIs with hardcoded userdn is something we
want to do. This should be handled by a single ACI in cn=vaults.
+1. Single ACI like
+default: aci: (targetfilter=(objectClass=ipaVault))(version 3.0; acl Vault
owners can manage the vault; allow(read, search, compare, write)
userattr=owner#USERDN;)
you already have there is more preferred.
--
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] Password vault
- Original Message -
On 07/07/2015 10:51 AM, Jan Cholasta wrote:
Dne 3.7.2015 v 15:44 Endi Sukma Dewata napsal(a):
Here is the rebased patch for vault access control.
LGTM, except:
@@ -356,6 +386,13 @@ class vault(LDAPObject):
{
'objectclass': ['nsContainer'],
'cn': rdn['cn'],
+'aci':
+'(targetfilter=(objectClass=ipaVault))' +
+'(version 3.0; ' +
+'acl User can manage private vaults; ' +
+'allow(read, search, compare, add, delete) ' +
+'userdn=ldap:///%s;;)'
+% owner_dn
})
# if entry can be added, return
I don't think dynamically creating ACIs with hardcoded userdn is something
we
want to do. This should be handled by a single ACI in cn=vaults.
+1. Single ACI like
+default: aci: (targetfilter=(objectClass=ipaVault))(version 3.0; acl
Vault
owners can manage the vault; allow(read, search, compare, write)
userattr=owner#USERDN;)
you already have there is more preferred.
New patch attached. For this to work the container itself needs an 'owner'
attribute, so I changed the nsContainer into ipaVaultContainer.
--
Endi S. Dewata
From 087f36b888e068ee732af6e0a2c24b1d50849ccd Mon Sep 17 00:00:00 2001
From: Endi S. Dewata edew...@redhat.com
Date: Fri, 17 Oct 2014 12:05:34 -0400
Subject: [PATCH] Added vault access control.
New LDAP ACIs have been added to allow vault owners to manage the
vaults and to allow members to access the vaults. New CLIs have
been added to manage the owner and member list. The LDAP schema
has been updated as well.
https://fedorahosted.org/freeipa/ticket/3872
---
API.txt | 92 +++
VERSION | 4 +-
install/share/60basev3.ldif | 3 +-
install/share/vault.update| 15 +++-
ipalib/plugins/vault.py | 118 --
ipatests/test_xmlrpc/test_vault_plugin.py | 27 +--
6 files changed, 226 insertions(+), 33 deletions(-)
diff --git a/API.txt b/API.txt
index 99fa528733200fc3d797a9847b1d6df2188b92d5..98eaee8ce8b2804a6d34e42c3eff26ddb3851963 100644
--- a/API.txt
+++ b/API.txt
@@ -5422,27 +5422,58 @@ option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui
option: Str('service?')
option: Str('setattr*', cli_name='setattr', exclude='webui')
option: Flag('shared?', autofill=True, default=False)
-option: Str('user?')
+option: Str('username?', cli_name='user')
option: Str('version?', exclude='webui')
output: Entry('result', type 'dict', Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
output: Output('summary', (type 'unicode', type 'NoneType'), None)
output: PrimaryKey('value', None, None)
command: vault_add_internal
-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, required=True)
option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=False)
option: Bytes('ipavaultpublickey', attribute=True, cli_name='public_key', multivalue=False, required=False)
option: Bytes('ipavaultsalt', attribute=True, cli_name='salt', multivalue=False, required=False)
option: Str('ipavaulttype', attribute=True, autofill=True, cli_name='type', default=u'standard', multivalue=False, required=False)
+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: Flag('shared?', autofill=True, default=False)
-option: Str('user?')
+option: Str('username?', cli_name='user')
option: Str('version?', exclude='webui')
output: Entry('result', type 'dict', Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
output: Output('summary', (type 'unicode', type 'NoneType'), None)
output: PrimaryKey('value', None, None)
+command: vault_add_member
+args: 1,9,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: Flag('shared?', autofill=True, default=False)
+option: Str('user*', alwaysask=True, cli_name='users', csv=True)
+option: Str('username?', cli_name='user')
+option:
Re: [Freeipa-devel] [PATCH] Password vault
On 7/1/2015 1:53 AM, Jan Cholasta wrote: I think it would be better to use a new attribute type which inherits from ipaPublicKey (ipaVaultPublicKey?) rather than ipaPublicKey directly for assymetric vault public keys, so that assymetric public key and escrow public key are on the same level and you can still use ipaPublicKey to refer to either one: ipaPublicKey ipaVaultPublicKey ipaEscrowPublicKey OK. To be consistent the parameters need to be renamed too: --vault-public-key and --vault-public-key-file. It doesn't need to, there is no requirement for CLI names to always match attribute names. (Also I don't insist on the name ipaVaultPublicKey, feel free to change it if you want.) It's unchanged for now. In a previous discussion it was advised to reuse the existing attribute type whenever possible. Well, in this discussion, it is not. Escrow public key should also reuse ipaPublicKey, but it can't if you use it for vault public key. By using ipaPublicKey subtypes you can distinguish between the two uses and still use ipaPublicKey to refer to either of them. So what's changed? This is what you said when I posted the same patch six months ago: In this patch I'm adding ipaVaultSalt and ipaVaultPublicKey attribute types to store salt and public key for vault. Are there existing attribute types that I can use instead? I see there's an ipaPublicKey, should I use that and maybe add ipaSalt/ipaEncSalt? Thanks. yes, please re-use existing attributes where possible. Honza What changed is that I now know there is also escrow public key, which I didn't know six months ago. Here's patch #368 to be applied on top of patch #357-5, but see comments below. Could you show me how to use ipaPublicKey to refer to ipaVaultPublicKey and ipaEscrowPublicKey? Under what situation would that be useful? For example for ipaPublicKey searches - if ipaVaultPublicKey and ipaEscrowPublicKey both inherit from ipaPublicKey, then an ipaPublicKey search will look in both ipaVaultPublicKey and ipaEscrowPublicKey. This is not something we actually need right now, but once the schema is done, it can't be fixed and I don't think we should prevent this, especially since we can get it for free. BTW even the core LDAP schema does this, see for example how the cn attribute inherits from the more general name attribute: https://tools.ietf.org/html/rfc4519#section-2.3. I don't think that's how LDAP works. The RFC doesn't say that either. The cn does inherit from name, but if you search for name it won't match/return cn. See queries below: $ ldapsearch -LLL -x -b dc=example,dc=com (cn=Accounting Managers) dn: cn=Accounting Managers,ou=Groups,dc=example,dc=com objectClass: top objectClass: groupOfUniqueNames cn: Accounting Managers ou: groups description: People who can manage accounting entries uniqueMember: cn=Directory Manager $ ldapsearch -LLL -x -b dc=example,dc=com (cn=Accounting Managers) \ name dn: cn=Accounting Managers,ou=Groups,dc=example,dc=com (no cn attribute) $ ldapsearch -LLL -x -b dc=example,dc=com (name=Accounting Managers) (no result) Assuming this is what you meant, which doesn't seem to be working, is there still a valid reason to add a new ipaVaultPublicKey instead of using the existing ipaPublicKey? * CLI options will be identical to client and server API options (i.e. no CLI-only, client-only, or server-only options) Actually, you can create CLI-only options (add include='cli' to the param's kwargs). I need to look at this more closely. If I understand correctly in user_del there are two 'preserve' options, the Bool preserve is for client and server API, and the Flag preserve is for CLI. Wouldn't it be better if they are stored in separate lists (or maybe separate classes)? And it looks like you still need to delete the CLI options explicitly anyway. Well, it would be better if there was no Flag class at all and flags were handled by CLI exclusively, because parameter classes should reflect the data type (bool) and not the presentation (flag). That indicates there should be a separation between client API and the CLI too because, as you see in user_del, they can be different. Does the API.txt actually show the CLI options, the client API options, or the server API options? I only see the Flag preserve, not the Bool preserve. It shows CLI options, see how the API object is initialized in makeapi. Does that mean we're only doing the versioning on the CLI, and not the client API or server API? Suppose there are changes in client or server API that do not appear in API.txt but will affect the XML RPC, it might cause a compatibility problem. I think it just shows how convoluted the CLI, client API, and server API are in this framework. * a plugin will only access one type of data (i.e. LDAP plugin can only access LDAP data) This is not assumed anywhere in the framework, you can access whatever you want, but you can't expect baseldap to do
Re: [Freeipa-devel] [PATCH] Password vault
Here is the rebased patch for vault access control.
--
Endi S. Dewata
From 6bec99d51552a6415c45d655f95627e341fae44b Mon Sep 17 00:00:00 2001
From: Endi S. Dewata edew...@redhat.com
Date: Fri, 17 Oct 2014 12:05:34 -0400
Subject: [PATCH] Added vault access control.
New LDAP ACIs have been added to allow vault owners to manage the
vaults and to allow members to access the vaults. New CLIs have
been added to manage the owner and member list. The LDAP schema
has been updated as well.
https://fedorahosted.org/freeipa/ticket/3872
---
API.txt | 92 ++
VERSION | 4 +-
install/share/60basev3.ldif | 2 +-
install/share/vault.update| 5 ++
ipalib/plugins/vault.py | 122 --
ipatests/test_xmlrpc/test_vault_plugin.py | 27 +--
6 files changed, 224 insertions(+), 28 deletions(-)
diff --git a/API.txt b/API.txt
index
d0ae1b72c2ae445a4e2cc168da5fd53f9a4de56d..c182098fe1017d46f9f7980c7b6891a1031f1068
100644
--- a/API.txt
+++ b/API.txt
@@ -5341,27 +5341,58 @@ option: Flag('raw', autofill=True, cli_name='raw',
default=False, exclude='webui
option: Str('service?')
option: Str('setattr*', cli_name='setattr', exclude='webui')
option: Flag('shared?', autofill=True, default=False)
-option: Str('user?')
+option: Str('username?', cli_name='user')
option: Str('version?', exclude='webui')
output: Entry('result', type 'dict', Gettext('A dictionary representing an
LDAP entry', domain='ipa', localedir=None))
output: Output('summary', (type 'unicode', type 'NoneType'), None)
output: PrimaryKey('value', None, None)
command: vault_add_internal
-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, required=True)
option: Flag('all', autofill=True, cli_name='all', default=False,
exclude='webui')
option: Str('description', attribute=True, cli_name='desc', multivalue=False,
required=False)
option: Bytes('ipavaultpublickey', attribute=True, cli_name='public_key',
multivalue=False, required=False)
option: Bytes('ipavaultsalt', attribute=True, cli_name='salt',
multivalue=False, required=False)
option: Str('ipavaulttype', attribute=True, autofill=True, cli_name='type',
default=u'standard', multivalue=False, required=False)
+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: Flag('shared?', autofill=True, default=False)
-option: Str('user?')
+option: Str('username?', cli_name='user')
option: Str('version?', exclude='webui')
output: Entry('result', type 'dict', Gettext('A dictionary representing an
LDAP entry', domain='ipa', localedir=None))
output: Output('summary', (type 'unicode', type 'NoneType'), None)
output: PrimaryKey('value', None, None)
+command: vault_add_member
+args: 1,9,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: Flag('shared?', autofill=True, default=False)
+option: Str('user*', alwaysask=True, cli_name='users', csv=True)
+option: Str('username?', cli_name='user')
+option: Str('version?', exclude='webui')
+output: Output('completed', type 'int', None)
+output: Output('failed', type 'dict', None)
+output: Entry('result', type 'dict', Gettext('A dictionary representing an
LDAP entry', domain='ipa', localedir=None))
+command: vault_add_owner
+args: 1,9,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: Flag('shared?', autofill=True, default=False)
+option: Str('user*', alwaysask=True, cli_name='users', csv=True)
+option: Str('username?', cli_name='user')
+option: Str('version?', exclude='webui')
+output: Output('completed', type 'int', None)
+output: Output('failed', type 'dict', None)
+output: Entry('result', type 'dict', Gettext('A dictionary representing an
LDAP entry', domain='ipa', localedir=None))
command: vault_archive
args: 1,10,3
arg: Str('cn', attribute=True, cli_name='name',
Re: [Freeipa-devel] [PATCH] Password vault
Dne 25.6.2015 v 19:01 Endi Sukma Dewata napsal(a):
On 6/25/2015 12:35 AM, Jan Cholasta wrote:
I think it would be better to use a new attribute type which inherits
from ipaPublicKey (ipaVaultPublicKey?) rather than ipaPublicKey
directly
for assymetric vault public keys, so that assymetric public key and
escrow public key are on the same level and you can still use
ipaPublicKey to refer to either one:
ipaPublicKey
ipaVaultPublicKey
ipaEscrowPublicKey
OK. To be consistent the parameters need to be renamed too:
--vault-public-key and --vault-public-key-file.
It doesn't need to, there is no requirement for CLI names to always
match attribute names. (Also I don't insist on the name
ipaVaultPublicKey, feel free to change it if you want.)
It's unchanged for now. In a previous discussion it was advised to reuse
the existing attribute type whenever possible.
Well, in this discussion, it is not. Escrow public key should also reuse
ipaPublicKey, but it can't if you use it for vault public key. By using
ipaPublicKey subtypes you can distinguish between the two uses and still
use ipaPublicKey to refer to either of them.
So what's changed? This is what you said when I posted the same patch
six months ago:
In this patch I'm adding ipaVaultSalt and ipaVaultPublicKey attribute
types to store salt and public key for vault. Are there existing
attribute types that I can use instead? I see there's an ipaPublicKey,
should I use that and maybe add ipaSalt/ipaEncSalt? Thanks.
yes, please re-use existing attributes where possible.
Honza
What changed is that I now know there is also escrow public key, which I
didn't know six months ago.
Could you show me how to use ipaPublicKey to refer to ipaVaultPublicKey
and ipaEscrowPublicKey? Under what situation would that be useful?
For example for ipaPublicKey searches - if ipaVaultPublicKey and
ipaEscrowPublicKey both inherit from ipaPublicKey, then an ipaPublicKey
search will look in both ipaVaultPublicKey and ipaEscrowPublicKey. This
is not something we actually need right now, but once the schema is
done, it can't be fixed and I don't think we should prevent this,
especially since we can get it for free. BTW even the core LDAP schema
does this, see for example how the cn attribute inherits from the more
general name attribute: https://tools.ietf.org/html/rfc4519#section-2.3.
a) When the non-split vault_{archive,retrieve} was called from a
server API with client-only options, it crashed. This is the broken API
I was talking about.
This is because in the current framework any API called on the server
side will be a server API, so you are not supposed to call it with
client options in the first place. Because of that limitation, the only
way to use client options is to use a separate API on the client side to
call the original API on the server side. The point is, client options
belong to client API, and server options belong to server API. In
vault_add the public key file name belongs to client API because it's
used to load a file on the client side. You should not add public key
file name option to the server API just because it can safely be
ignored.
I don't disagree, but file name options do not belong to the general
client API either, as they are strictly CLI-specific.
To my understanding the current framework doesn't have a separate CLI
class, so you don't have a choice but to put CLI-specific options in the
client API class too. However, you do have a choice not to combine
client API class and server API class because otherwise that will put
CLI-specific options in the server API class too.
Right.
2. Since the vault_archive_internal inherits from Update, it
accepts
all non
primary-key attributes automatically. This is incorrect since we
don't want to
update these parameters during archival. Can this behavior be
overridden?
Inherit from PKQuery instead (don't forget to add has_output =
output.standard_entry).
Previously you didn't want to use LDAPQuery because of semantics
reasons. Is PKQuery fine semantically?
It's not. Currently there is a set of commands which operate on the
LDAP
part of vault and another set of commands which operate on the KRA part
of vault and we don't want the commands in one set to see attributes
related to the other part of vault. If you insist on keeping both parts
in a single object, you have to resort to hackery like using PKQuery,
hence my suggestion to split the data part off to a separate object to
avoid this.
This because the framework was based on simplistic assumptions which
create unnecessary restrictions, for example:
* client API is just a proxy to server API (i.e. client and server
cannot do different things)
They can do different things the same way vault_archive/vault_retrieve
does that, the commands just can't be called the same (which is not
necessarily a bad thing).
Of course different APIs can do different things, like vault_add calling
vault_archive,
Re: [Freeipa-devel] [PATCH] Password vault
On 6/25/2015 12:35 AM, Jan Cholasta wrote:
I think it would be better to use a new attribute type which inherits
from ipaPublicKey (ipaVaultPublicKey?) rather than ipaPublicKey
directly
for assymetric vault public keys, so that assymetric public key and
escrow public key are on the same level and you can still use
ipaPublicKey to refer to either one:
ipaPublicKey
ipaVaultPublicKey
ipaEscrowPublicKey
OK. To be consistent the parameters need to be renamed too:
--vault-public-key and --vault-public-key-file.
It doesn't need to, there is no requirement for CLI names to always
match attribute names. (Also I don't insist on the name
ipaVaultPublicKey, feel free to change it if you want.)
It's unchanged for now. In a previous discussion it was advised to reuse
the existing attribute type whenever possible.
Well, in this discussion, it is not. Escrow public key should also reuse
ipaPublicKey, but it can't if you use it for vault public key. By using
ipaPublicKey subtypes you can distinguish between the two uses and still
use ipaPublicKey to refer to either of them.
So what's changed? This is what you said when I posted the same patch
six months ago:
In this patch I'm adding ipaVaultSalt and ipaVaultPublicKey attribute
types to store salt and public key for vault. Are there existing
attribute types that I can use instead? I see there's an ipaPublicKey,
should I use that and maybe add ipaSalt/ipaEncSalt? Thanks.
yes, please re-use existing attributes where possible.
Honza
Could you show me how to use ipaPublicKey to refer to ipaVaultPublicKey
and ipaEscrowPublicKey? Under what situation would that be useful?
a) When the non-split vault_{archive,retrieve} was called from a
server API with client-only options, it crashed. This is the broken API
I was talking about.
This is because in the current framework any API called on the server
side will be a server API, so you are not supposed to call it with
client options in the first place. Because of that limitation, the only
way to use client options is to use a separate API on the client side to
call the original API on the server side. The point is, client options
belong to client API, and server options belong to server API. In
vault_add the public key file name belongs to client API because it's
used to load a file on the client side. You should not add public key
file name option to the server API just because it can safely be ignored.
I don't disagree, but file name options do not belong to the general
client API either, as they are strictly CLI-specific.
To my understanding the current framework doesn't have a separate CLI
class, so you don't have a choice but to put CLI-specific options in the
client API class too. However, you do have a choice not to combine
client API class and server API class because otherwise that will put
CLI-specific options in the server API class too.
2. Since the vault_archive_internal inherits from Update, it accepts
all non
primary-key attributes automatically. This is incorrect since we
don't want to
update these parameters during archival. Can this behavior be
overridden?
Inherit from PKQuery instead (don't forget to add has_output =
output.standard_entry).
Previously you didn't want to use LDAPQuery because of semantics
reasons. Is PKQuery fine semantically?
It's not. Currently there is a set of commands which operate on the LDAP
part of vault and another set of commands which operate on the KRA part
of vault and we don't want the commands in one set to see attributes
related to the other part of vault. If you insist on keeping both parts
in a single object, you have to resort to hackery like using PKQuery,
hence my suggestion to split the data part off to a separate object to
avoid this.
This because the framework was based on simplistic assumptions which
create unnecessary restrictions, for example:
* client API is just a proxy to server API (i.e. client and server
cannot do different things)
They can do different things the same way vault_archive/vault_retrieve
does that, the commands just can't be called the same (which is not
necessarily a bad thing).
Of course different APIs can do different things, like vault_add calling
vault_archive, or vault_archive calling vault_archive_internal. The
point is right now the client portion of an API (i.e. the forward()
method) cannot do anything other than forwarding the request to the
server, so the API has to be split into different APIs:
* vault_archive
* vault_archive_internal
It would be nice to have formal separation between client and server
APIs so it's clear they are different but still related without
resorting to ugly names:
* client.vault_archive
* server.vault_archive
* CLI options will be identical to client and server API options (i.e.
no CLI-only, client-only, or server-only options)
Actually, you can create CLI-only options (add include='cli' to the
param's kwargs).
I need to
Re: [Freeipa-devel] [PATCH] Password vault
Dne 23.6.2015 v 05:27 Endi Sukma Dewata napsal(a):
Please take a look at the new patch.
On 6/17/2015 1:32 AM, Jan Cholasta wrote:
I think it would be better to use a new attribute type which inherits
from ipaPublicKey (ipaVaultPublicKey?) rather than ipaPublicKey
directly
for assymetric vault public keys, so that assymetric public key and
escrow public key are on the same level and you can still use
ipaPublicKey to refer to either one:
ipaPublicKey
ipaVaultPublicKey
ipaEscrowPublicKey
OK. To be consistent the parameters need to be renamed too:
--vault-public-key and --vault-public-key-file.
It doesn't need to, there is no requirement for CLI names to always
match attribute names. (Also I don't insist on the name
ipaVaultPublicKey, feel free to change it if you want.)
It's unchanged for now. In a previous discussion it was advised to reuse
the existing attribute type whenever possible.
Well, in this discussion, it is not. Escrow public key should also reuse
ipaPublicKey, but it can't if you use it for vault public key. By using
ipaPublicKey subtypes you can distinguish between the two uses and still
use ipaPublicKey to refer to either of them.
1. The vault_add was split into a client-side vault_add and
server-side
vault_add_internal since the parameters are different (i.e. public
key file and
future escrow-related params). Since vault_add inherits from Local
all
non-primary-key attributes have to be added explicitly.
The split is not really necessary, since the only difference is the
public_key_file option, which exists only because of the lack of proper
file support in the framework. This is a different situation from
vault_{archive,retrieve}, which has two different sets of options on
client and server side. Escrow adds only ipaescrowpublickey and
escrow_public_key_file, right? If yes, we can safely keep the
command in
a single piece.
We know the vault-add will have at least two client-only parameters:
vault_public_key_file and escrow_public_key_file. Keeping these
parameters on the server API would be wrong and confusing. If the API is
called on the server side with vault_public_key_file the operation will
fail. In the previous discussion you considered this as broken API:
Server API is used not only by the server itself, but also by
installers
for example. Anyway the point is that there *can't* be a broken API
like
this, you should at least raise an error if the command is called from
server API, although actually separating it into client and server
parts
would be preferable.
You are comparing apples and oranges:
Non-identical items are different by definition. Even between 2 apples
there are differences, but it doesn't mean the distinction is important.
The latest patch shows that the vault_add needs to be split, not just
because of the options, but because of what they do differently on the
client and server.
a) When the non-split vault_{archive,retrieve} was called from a
server API with client-only options, it crashed. This is the broken API
I was talking about.
This is because in the current framework any API called on the server
side will be a server API, so you are not supposed to call it with
client options in the first place. Because of that limitation, the only
way to use client options is to use a separate API on the client side to
call the original API on the server side. The point is, client options
belong to client API, and server options belong to server API. In
vault_add the public key file name belongs to client API because it's
used to load a file on the client side. You should not add public key
file name option to the server API just because it can safely be ignored.
I don't disagree, but file name options do not belong to the general
client API either, as they are strictly CLI-specific.
b) The non-split vault_{archive,retrieve} had server-only options,
which were also accepted on client, but setting them had no effect.
Similarly, in a combined vault_add the public key file name option will
be accepted by the server, but it will be ignored. If something calls
vault_add on the server side and provides a file name, the operation
will crash too because the command expects the public key data to be
provided via another option. Splitting the vault_add into client and
server components avoids the potential problems.
c) The CLI options to read param values from files should be generated
by the framework without having to specify dummy params. Once this is
implemented, the dummy params will go away. However, this will still
leave some client-only options in vault_{archive,retrieve}.
I'm not sure how the options will look like when that's implemented, but
regardless, the vault_add will still have client-only password option.
None of the above applies to vault_add - it does not have any
server-only options and the only client-only options it has are the
dummy options for file input, which are ignored on the
Re: [Freeipa-devel] [PATCH] Password vault
Please take a look at the new patch.
On 6/17/2015 1:32 AM, Jan Cholasta wrote:
I think it would be better to use a new attribute type which inherits
from ipaPublicKey (ipaVaultPublicKey?) rather than ipaPublicKey directly
for assymetric vault public keys, so that assymetric public key and
escrow public key are on the same level and you can still use
ipaPublicKey to refer to either one:
ipaPublicKey
ipaVaultPublicKey
ipaEscrowPublicKey
OK. To be consistent the parameters need to be renamed too:
--vault-public-key and --vault-public-key-file.
It doesn't need to, there is no requirement for CLI names to always
match attribute names. (Also I don't insist on the name
ipaVaultPublicKey, feel free to change it if you want.)
It's unchanged for now. In a previous discussion it was advised to reuse
the existing attribute type whenever possible.
1. The vault_add was split into a client-side vault_add and
server-side
vault_add_internal since the parameters are different (i.e. public
key file and
future escrow-related params). Since vault_add inherits from Local all
non-primary-key attributes have to be added explicitly.
The split is not really necessary, since the only difference is the
public_key_file option, which exists only because of the lack of proper
file support in the framework. This is a different situation from
vault_{archive,retrieve}, which has two different sets of options on
client and server side. Escrow adds only ipaescrowpublickey and
escrow_public_key_file, right? If yes, we can safely keep the command in
a single piece.
We know the vault-add will have at least two client-only parameters:
vault_public_key_file and escrow_public_key_file. Keeping these
parameters on the server API would be wrong and confusing. If the API is
called on the server side with vault_public_key_file the operation will
fail. In the previous discussion you considered this as broken API:
Server API is used not only by the server itself, but also by installers
for example. Anyway the point is that there *can't* be a broken API like
this, you should at least raise an error if the command is called from
server API, although actually separating it into client and server parts
would be preferable.
You are comparing apples and oranges:
Non-identical items are different by definition. Even between 2 apples
there are differences, but it doesn't mean the distinction is important.
The latest patch shows that the vault_add needs to be split, not just
because of the options, but because of what they do differently on the
client and server.
a) When the non-split vault_{archive,retrieve} was called from a
server API with client-only options, it crashed. This is the broken API
I was talking about.
This is because in the current framework any API called on the server
side will be a server API, so you are not supposed to call it with
client options in the first place. Because of that limitation, the only
way to use client options is to use a separate API on the client side to
call the original API on the server side. The point is, client options
belong to client API, and server options belong to server API. In
vault_add the public key file name belongs to client API because it's
used to load a file on the client side. You should not add public key
file name option to the server API just because it can safely be ignored.
b) The non-split vault_{archive,retrieve} had server-only options,
which were also accepted on client, but setting them had no effect.
Similarly, in a combined vault_add the public key file name option will
be accepted by the server, but it will be ignored. If something calls
vault_add on the server side and provides a file name, the operation
will crash too because the command expects the public key data to be
provided via another option. Splitting the vault_add into client and
server components avoids the potential problems.
c) The CLI options to read param values from files should be generated
by the framework without having to specify dummy params. Once this is
implemented, the dummy params will go away. However, this will still
leave some client-only options in vault_{archive,retrieve}.
I'm not sure how the options will look like when that's implemented, but
regardless, the vault_add will still have client-only password option.
None of the above applies to vault_add - it does not have any
server-only options and the only client-only options it has are the
dummy options for file input, which are ignored on the server.
Let's not get fixated with just the options. The vault_add will now
archive a blank initial data as it was originally designed. The data can
be used later to verify the vault password in subsequent archival
operations. The vault_archive must be called by vault_add's client
component since it takes a password and the password cannot be sent to
the server.
Also, originally the vault was designed like this: when
Re: [Freeipa-devel] [PATCH] Password vault
Dne 16.6.2015 v 01:02 Endi Sukma Dewata napsal(a):
On 6/15/2015 2:22 AM, Jan Cholasta wrote:
I think it would be better to use a new attribute type which inherits
from ipaPublicKey (ipaVaultPublicKey?) rather than ipaPublicKey directly
for assymetric vault public keys, so that assymetric public key and
escrow public key are on the same level and you can still use
ipaPublicKey to refer to either one:
ipaPublicKey
ipaVaultPublicKey
ipaEscrowPublicKey
( 2.16.840.1.113730.3.8.18.2.? NAME 'ipaVaultPublicKey' DESC
'Assymetric vault public key as DER-encoded SubjectPublicKeyInfo (RFC
5280)' SUP ipaPublicKey EQUALITY octetStringMatch SYNTAX
1.3.6.1.4.1.1466.115.121.1.40 X-ORIGIN 'IPA v4.2' )
( 2.16.840.1.113730.3.8.18.2.3 NAME 'ipaEscrowPublicKey' DESC 'IPA
escrow public key as DER-encoded SubjectPublicKeyInfo (RFC 5280)' SUP
ipaPublicKey EQUALITY octetStringMatch SYNTAX
1.3.6.1.4.1.1466.115.121.1.40 X-ORIGIN 'IPA v4.2' )
OK. To be consistent the parameters need to be renamed too:
--vault-public-key and --vault-public-key-file.
It doesn't need to, there is no requirement for CLI names to always
match attribute names. (Also I don't insist on the name
ipaVaultPublicKey, feel free to change it if you want.)
1. The vault_add was split into a client-side vault_add and server-side
vault_add_internal since the parameters are different (i.e. public
key file and
future escrow-related params). Since vault_add inherits from Local all
non-primary-key attributes have to be added explicitly.
The split is not really necessary, since the only difference is the
public_key_file option, which exists only because of the lack of proper
file support in the framework. This is a different situation from
vault_{archive,retrieve}, which has two different sets of options on
client and server side. Escrow adds only ipaescrowpublickey and
escrow_public_key_file, right? If yes, we can safely keep the command in
a single piece.
We know the vault-add will have at least two client-only parameters:
vault_public_key_file and escrow_public_key_file. Keeping these
parameters on the server API would be wrong and confusing. If the API is
called on the server side with vault_public_key_file the operation will
fail. In the previous discussion you considered this as broken API:
Server API is used not only by the server itself, but also by installers
for example. Anyway the point is that there *can't* be a broken API like
this, you should at least raise an error if the command is called from
server API, although actually separating it into client and server parts
would be preferable.
You are comparing apples and oranges:
a) When the non-split vault_{archive,retrieve} was called from a
server API with client-only options, it crashed. This is the broken API
I was talking about.
b) The non-split vault_{archive,retrieve} had server-only options,
which were also accepted on client, but setting them had no effect.
c) The CLI options to read param values from files should be generated
by the framework without having to specify dummy params. Once this is
implemented, the dummy params will go away. However, this will still
leave some client-only options in vault_{archive,retrieve}.
None of the above applies to vault_add - it does not have any
server-only options and the only client-only options it has are the
dummy options for file input, which are ignored on the server.
Also, originally the vault was designed like this: when you create a
symmetric vault you're supposed to specify the password as well, similar
to adding a public key when creating an asymmetric vault. When you
archive, you're supposed to enter the same password for verification,
not a new password. So it would look like this:
$ ipa vault-add test --type symmetric
New password:
Verify password:
$ ipa vault-archive test --in secret1.txt
Password: (same password)
$ ipa vault-archive test --in secret2.txt
Password: (same password)
In the original design the vault-add would also archive a blank data,
which later could be used to verify the password during vault-archive by
decrypting the existing data first. There's also a plan to add a
mechanism to change the password after the ACL patch.
In the current design the vault-add doesn't archive anything, so during
vault-archive it cannot verify the password because there is nothing to
decrypt. In other words you can specify different passwords on each
archival, regardless of previous archivals:
$ ipa vault-add test --type symmetric
$ ipa vault-archive test --in secret1.txt
New password:
Verify password:
$ ipa vault-archive test --in secret2.txt
New password:
Verify password:
So basically here are the options:
1. Specify the crypto parameters once during vault creation, then
reuse/verify the parameters on each archival retrieval. You can change
the parameters only with a special command.
2. Don't
Re: [Freeipa-devel] [PATCH] Password vault
On 6/15/2015 2:22 AM, Jan Cholasta wrote:
I think it would be better to use a new attribute type which inherits
from ipaPublicKey (ipaVaultPublicKey?) rather than ipaPublicKey directly
for assymetric vault public keys, so that assymetric public key and
escrow public key are on the same level and you can still use
ipaPublicKey to refer to either one:
ipaPublicKey
ipaVaultPublicKey
ipaEscrowPublicKey
( 2.16.840.1.113730.3.8.18.2.? NAME 'ipaVaultPublicKey' DESC
'Assymetric vault public key as DER-encoded SubjectPublicKeyInfo (RFC
5280)' SUP ipaPublicKey EQUALITY octetStringMatch SYNTAX
1.3.6.1.4.1.1466.115.121.1.40 X-ORIGIN 'IPA v4.2' )
( 2.16.840.1.113730.3.8.18.2.3 NAME 'ipaEscrowPublicKey' DESC 'IPA
escrow public key as DER-encoded SubjectPublicKeyInfo (RFC 5280)' SUP
ipaPublicKey EQUALITY octetStringMatch SYNTAX
1.3.6.1.4.1.1466.115.121.1.40 X-ORIGIN 'IPA v4.2' )
OK. To be consistent the parameters need to be renamed too:
--vault-public-key and --vault-public-key-file.
1. The vault_add was split into a client-side vault_add and server-side
vault_add_internal since the parameters are different (i.e. public
key file and
future escrow-related params). Since vault_add inherits from Local all
non-primary-key attributes have to be added explicitly.
The split is not really necessary, since the only difference is the
public_key_file option, which exists only because of the lack of proper
file support in the framework. This is a different situation from
vault_{archive,retrieve}, which has two different sets of options on
client and server side. Escrow adds only ipaescrowpublickey and
escrow_public_key_file, right? If yes, we can safely keep the command in
a single piece.
We know the vault-add will have at least two client-only parameters:
vault_public_key_file and escrow_public_key_file. Keeping these
parameters on the server API would be wrong and confusing. If the API is
called on the server side with vault_public_key_file the operation will
fail. In the previous discussion you considered this as broken API:
Server API is used not only by the server itself, but also by installers
for example. Anyway the point is that there *can't* be a broken API like
this, you should at least raise an error if the command is called from
server API, although actually separating it into client and server parts
would be preferable.
Also, originally the vault was designed like this: when you create a
symmetric vault you're supposed to specify the password as well, similar
to adding a public key when creating an asymmetric vault. When you
archive, you're supposed to enter the same password for verification,
not a new password. So it would look like this:
$ ipa vault-add test --type symmetric
New password:
Verify password:
$ ipa vault-archive test --in secret1.txt
Password: (same password)
$ ipa vault-archive test --in secret2.txt
Password: (same password)
In the original design the vault-add would also archive a blank data,
which later could be used to verify the password during vault-archive by
decrypting the existing data first. There's also a plan to add a
mechanism to change the password after the ACL patch.
In the current design the vault-add doesn't archive anything, so during
vault-archive it cannot verify the password because there is nothing to
decrypt. In other words you can specify different passwords on each
archival, regardless of previous archivals:
$ ipa vault-add test --type symmetric
$ ipa vault-archive test --in secret1.txt
New password:
Verify password:
$ ipa vault-archive test --in secret2.txt
New password:
Verify password:
So basically here are the options:
1. Specify the crypto parameters once during vault creation, then
reuse/verify the parameters on each archival retrieval. You can change
the parameters only with a special command.
2. Don't specify the crypto parameters during vault creation, but
specify new parameters on each archival. For retrieval you'd have to
use/verify the parameters specified in the last archival.
I think the first one makes more sense and is easier to use. That also
means the vault-add will have additional client-only parameters such as
--password and --password-file.
2. Since the vault_archive_internal inherits from Update, it accepts
all non
primary-key attributes automatically. This is incorrect since we
don't want to
update these parameters during archival. Can this behavior be
overridden?
Inherit from PKQuery instead (don't forget to add has_output =
output.standard_entry).
Previously you didn't want to use LDAPQuery because of semantics
reasons. Is PKQuery fine semantically? Why not use LDAPQuery since vault
is an LDAPObject? And to be consistent should vault_retrieve_internal
inherit from the same class?
BTW the correct solution would be to have a separate object and commands
for vault data (e.g.
Re: [Freeipa-devel] [PATCH] Password vault
Dne 10.6.2015 v 08:13 Martin Kosek napsal(a):
On 06/09/2015 11:13 PM, Endi Sukma Dewata wrote:
Please take a look at the attached patch to add symmetric asymmetric vaults.
Some comments about the patch:
I think it would be better to use a new attribute type which inherits
from ipaPublicKey (ipaVaultPublicKey?) rather than ipaPublicKey directly
for assymetric vault public keys, so that assymetric public key and
escrow public key are on the same level and you can still use
ipaPublicKey to refer to either one:
ipaPublicKey
ipaVaultPublicKey
ipaEscrowPublicKey
( 2.16.840.1.113730.3.8.18.2.? NAME 'ipaVaultPublicKey' DESC
'Assymetric vault public key as DER-encoded SubjectPublicKeyInfo (RFC
5280)' SUP ipaPublicKey EQUALITY octetStringMatch SYNTAX
1.3.6.1.4.1.1466.115.121.1.40 X-ORIGIN 'IPA v4.2' )
( 2.16.840.1.113730.3.8.18.2.3 NAME 'ipaEscrowPublicKey' DESC 'IPA
escrow public key as DER-encoded SubjectPublicKeyInfo (RFC 5280)' SUP
ipaPublicKey EQUALITY octetStringMatch SYNTAX
1.3.6.1.4.1.1466.115.121.1.40 X-ORIGIN 'IPA v4.2' )
1. The vault_add was split into a client-side vault_add and server-side
vault_add_internal since the parameters are different (i.e. public key file and
future escrow-related params). Since vault_add inherits from Local all
non-primary-key attributes have to be added explicitly.
The split is not really necessary, since the only difference is the
public_key_file option, which exists only because of the lack of proper
file support in the framework. This is a different situation from
vault_{archive,retrieve}, which has two different sets of options on
client and server side. Escrow adds only ipaescrowpublickey and
escrow_public_key_file, right? If yes, we can safely keep the command in
a single piece.
2. Since the vault_archive_internal inherits from Update, it accepts all non
primary-key attributes automatically. This is incorrect since we don't want to
update these parameters during archival. Can this behavior be overridden?
Inherit from PKQuery instead (don't forget to add has_output =
output.standard_entry).
BTW the correct solution would be to have a separate object and commands
for vault data (e.g. vaultdata object, vault_archive - vaultdata_mod,
vault_retrieve - vauldata_show), then we wouldn't have to deal with
mixing vault attributes with vault data and could use proper crud base
classes.
Just for the record, this changes API, right? It would be better to have this
in Alpha planned for this week. Not a blocker for Alpha though, we can give
warning that the internal API may change before GA.
--
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] Password vault
Dne 15.6.2015 v 09:22 Jan Cholasta napsal(a):
Dne 10.6.2015 v 08:13 Martin Kosek napsal(a):
On 06/09/2015 11:13 PM, Endi Sukma Dewata wrote:
Please take a look at the attached patch to add symmetric
asymmetric vaults.
Some comments about the patch:
I think it would be better to use a new attribute type which inherits
from ipaPublicKey (ipaVaultPublicKey?) rather than ipaPublicKey directly
for assymetric vault public keys, so that assymetric public key and
escrow public key are on the same level and you can still use
ipaPublicKey to refer to either one:
ipaPublicKey
ipaVaultPublicKey
ipaEscrowPublicKey
( 2.16.840.1.113730.3.8.18.2.? NAME 'ipaVaultPublicKey' DESC
'Assymetric vault public key as DER-encoded SubjectPublicKeyInfo (RFC
5280)' SUP ipaPublicKey EQUALITY octetStringMatch SYNTAX
1.3.6.1.4.1.1466.115.121.1.40 X-ORIGIN 'IPA v4.2' )
( 2.16.840.1.113730.3.8.18.2.3 NAME 'ipaEscrowPublicKey' DESC 'IPA
escrow public key as DER-encoded SubjectPublicKeyInfo (RFC 5280)' SUP
ipaPublicKey EQUALITY octetStringMatch SYNTAX
1.3.6.1.4.1.1466.115.121.1.40 X-ORIGIN 'IPA v4.2' )
BTW what is the reason for that vault type is idendified by ipaVaultType
attribute rather than object class?
1. The vault_add was split into a client-side vault_add and server-side
vault_add_internal since the parameters are different (i.e. public
key file and
future escrow-related params). Since vault_add inherits from Local all
non-primary-key attributes have to be added explicitly.
The split is not really necessary, since the only difference is the
public_key_file option, which exists only because of the lack of proper
file support in the framework. This is a different situation from
vault_{archive,retrieve}, which has two different sets of options on
client and server side. Escrow adds only ipaescrowpublickey and
escrow_public_key_file, right? If yes, we can safely keep the command in
a single piece.
2. Since the vault_archive_internal inherits from Update, it accepts
all non
primary-key attributes automatically. This is incorrect since we
don't want to
update these parameters during archival. Can this behavior be
overridden?
Inherit from PKQuery instead (don't forget to add has_output =
output.standard_entry).
BTW the correct solution would be to have a separate object and commands
for vault data (e.g. vaultdata object, vault_archive - vaultdata_mod,
vault_retrieve - vauldata_show), then we wouldn't have to deal with
mixing vault attributes with vault data and could use proper crud base
classes.
Just for the record, this changes API, right? It would be better to
have this
in Alpha planned for this week. Not a blocker for Alpha though, we can
give
warning that the internal API may change before GA.
--
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] Password vault
Dne 8.6.2015 v 12:04 Jan Cholasta napsal(a): Dne 5.6.2015 v 21:50 Endi Sukma Dewata napsal(a): On 6/5/2015 7:13 AM, Jan Cholasta wrote: BTW, ipa-kra-install is broken with pki-core-10.2.4-1, but it works with pki-core-10.2.1-3. There's a bug in IPA: https://bugzilla.redhat.com/show_bug.cgi?id=1228671 Cloned the bug to https://fedorahosted.org/freeipa/ticket/5058. The patch needs a rebase and version bumb (VERSION line at the top of ipa-pki-proxy.conf). I have bumped VERSION and rebased the patch, see attachment. Pushed to master: 62ef11efad4ebbb8fa6f13a15c5ed8e833e90d43 -- Jan Cholasta From 548636e6b1c7c0f921f882ec7510dc1365d4042a Mon Sep 17 00:00:00 2001 From: Endi S. Dewata edew...@redhat.com Date: Mon, 8 Jun 2015 05:30:47 + Subject: [PATCH] Fixed KRA installation problem. The ipa-pki-proxy.conf has been modified to optionally require client certificate authentication for PKI REST services as it's done in standalone PKI to allow the proper KRA installation. https://fedorahosted.org/freeipa/ticket/5058 --- install/conf/ipa-pki-proxy.conf | 15 --- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/install/conf/ipa-pki-proxy.conf b/install/conf/ipa-pki-proxy.conf index 354b340..4b5b6f7 100644 --- a/install/conf/ipa-pki-proxy.conf +++ b/install/conf/ipa-pki-proxy.conf @@ -1,4 +1,4 @@ -# VERSION 7 - DO NOT REMOVE THIS LINE +# VERSION 8 - DO NOT REMOVE THIS LINE ProxyRequests Off @@ -11,7 +11,7 @@ ProxyRequests Off /LocationMatch # matches for admin port and installer -LocationMatch ^/ca/admin/ca/getCertChain|^/ca/admin/ca/getConfigEntries|^/ca/admin/ca/getCookie|^/ca/admin/ca/getStatus|^/ca/admin/ca/securityDomainLogin|^/ca/admin/ca/getDomainXML|^/ca/rest/installer/installToken|^/ca/admin/ca/updateNumberRange|^/ca/rest/securityDomain/domainInfo|^/ca/admin/ca/tokenAuthenticate|^/ca/admin/ca/updateNumberRange|^/ca/admin/ca/updateDomainXML|^/ca/rest/securityDomain/installToken|^/ca/admin/ca/updateConnector|^/ca/admin/ca/getSubsystemCert|^/kra/admin/kra/updateNumberRange|^/kra/admin/kra/getConfigEntries|^/kra/rest/config/cert/transport +LocationMatch ^/ca/admin/ca/getCertChain|^/ca/admin/ca/getConfigEntries|^/ca/admin/ca/getCookie|^/ca/admin/ca/getStatus|^/ca/admin/ca/securityDomainLogin|^/ca/admin/ca/getDomainXML|^/ca/admin/ca/updateNumberRange|^/ca/admin/ca/tokenAuthenticate|^/ca/admin/ca/updateNumberRange|^/ca/admin/ca/updateDomainXML|^/ca/admin/ca/updateConnector|^/ca/admin/ca/getSubsystemCert|^/kra/admin/kra/updateNumberRange|^/kra/admin/kra/getConfigEntries NSSOptions +StdEnvVars +ExportCertData +StrictRequire +OptRenegotiate NSSVerifyClient none ProxyPassMatch ajp://localhost:$DOGTAG_PORT @@ -19,24 +19,25 @@ ProxyRequests Off /LocationMatch # matches for agent port and eeca port -LocationMatch ^/ca/agent/ca/displayBySerial|^/ca/agent/ca/doRevoke|^/ca/agent/ca/doUnrevoke|^/ca/agent/ca/updateDomainXML|^/ca/eeca/ca/profileSubmitSSLClient|^/kra/agent/kra/connector|^/kra/rest/account|^/kra/rest/agent/keyrequests|^/kra/rest/agent/keys|^/ca/rest/admin/kraconnector/remove +LocationMatch ^/ca/agent/ca/displayBySerial|^/ca/agent/ca/doRevoke|^/ca/agent/ca/doUnrevoke|^/ca/agent/ca/updateDomainXML|^/ca/eeca/ca/profileSubmitSSLClient|^/kra/agent/kra/connector NSSOptions +StdEnvVars +ExportCertData +StrictRequire +OptRenegotiate NSSVerifyClient require ProxyPassMatch ajp://localhost:$DOGTAG_PORT ProxyPassReverse ajp://localhost:$DOGTAG_PORT /LocationMatch -# matches for REST API -LocationMatch ^/ca/rest/account/login|^/ca/rest/account/logout +# matches for CA REST API +LocationMatch ^/ca/rest/account/login|^/ca/rest/account/logout|^/ca/rest/installer/installToken|^/ca/rest/securityDomain/domainInfo|^/ca/rest/securityDomain/installToken|^/ca/rest/profiles|^/ca/rest/admin/kraconnector/remove NSSOptions +StdEnvVars +ExportCertData +StrictRequire +OptRenegotiate NSSVerifyClient optional ProxyPassMatch ajp://localhost:$DOGTAG_PORT ProxyPassReverse ajp://localhost:$DOGTAG_PORT /LocationMatch -LocationMatch ^/ca/rest/profiles +# matches for KRA REST API +LocationMatch ^/kra/rest/config/cert/transport|^/kra/rest/account|^/kra/rest/agent/keyrequests|^/kra/rest/agent/keys NSSOptions +StdEnvVars +ExportCertData +StrictRequire +OptRenegotiate -NSSVerifyClient none +NSSVerifyClient optional ProxyPassMatch ajp://localhost:$DOGTAG_PORT ProxyPassReverse ajp://localhost:$DOGTAG_PORT /LocationMatch -- 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] Password vault
Dne 5.6.2015 v 21:50 Endi Sukma Dewata napsal(a):
On 6/5/2015 7:13 AM, Jan Cholasta wrote:
If KRA is not installed, vault-archive and vault-retrieve fail with
internal error.
Added a code to check KRA installation in all vault commands. If you
know a way not to load the vault plugin if the KRA is not installed
please let me know, that's probably even better. Not sure how that will
work on the client side though.
I see this has been already resolved in the other thread.
The other thread was talking about removing the pki-base dependency on
the client side, but the vault plugin is still loaded on both client and
server regardless of KRA installation. Ideally the vault plugin should
not even be loaded so you cannot even execute the commands.
I don't agree - ideally the vault plugin should do the check at runtime,
because it should work without httpd restart when KRA is installed on
other replica. The KRA installer needs to be fixed in order to support
this. I will provide a patch.
The commands still behave differently based on whether they were
called
from API which was initialized with in_server set to True or False.
That is unfortunately a restriction imposed by the framework. In order
to guarantee the security, the vault is designed to have separate
client
and server code. The client code encrypts the secret, the server code
forwards the encrypted secret to KRA. To archive a secret into a vault
properly, you are supposed to call the client code. If you're calling
the server code directly, you are responsible to do your own encryption
(i.e. generating session key, nonce, and vault data).
I understand why the code has to be separated, what I don't understand
is why it is in fact *not* separated and crammed into a single command,
making weird and undefined behavior possible.
If another plugin wants to use vault, it should implement a client code
which calls the vault client code to perform the archival from the
client side.
What is the use case for calling the vault API from the server side
anyway? Wouldn't that defeat the purpose of having a vault? If a secret
exists on the server side in an unencrypted form doesn't it mean the
secret may already have been compromised?
Server API is used not only by the server itself, but also by installers
for example. Anyway the point is that there *can't* be a broken API like
this, you should at least raise an error if the command is called from
server API, although actually separating it into client and server parts
would be preferable.
There is no point in exposing the session_key, nonce and vault_data
options in CLI when their value is always overwritten in forward().
I agree there is no need to expose them in CLI, but in this framework
the API also defines the CLI. If there's a way to keep them in the
server API but not expose them in the CLI please let me know. Or, if
there's a way to define completely separate server API (without a
matching client CLI) and client CLI (without a matching server API)
that
will work too.
As I suggested above, you can split the commands into separate client
and server commands. The client command should inherit from
frontend.Local so that it is always executed locally and the server
command should have a NO_CLI = True attribute so that it is not
available in the CLI.
I see the vault_archive and vault_retrieve now inherit from PKQuery, and
there is a hack to execute the forward() even on the server side. A few
things below:
1. Why didn't you use frontend.Local as you initially suggested? If
there's a problem with frontend.Local please attach the ticket number in
the code.
2. The forward() can be merged into run(). There is no need to keep the
code in forward(). It would make more sense to have a run() method that
runs both on client and server, rather than a forward() that is supposed
to run on the client only but now forced to run on server too,
semantically speaking.
I have fixed the commands to inherit from Local.
Attached is a patch including the requested changes.
I have also changed vault_config to vaultconfig_show, for consistency
with {,dns}config_show (it also makes the transport certificate
retrieval code in vault_{archive,retrieve} simpler).
3. The parameter description for nonce should be just 'Nonce' instead of
'Nonce encrypted'.
Fixed.
4. There's a PEP8 error.
Fixed.
5. The VERSION needs to be updated.
Fixed.
Assuming the above issues are addressed, ACK.
OK, pushed to master: df1bd39a43f30138cf55e0e7720fa3dec1d912e0
I have noticed that triple-length DES is used for the session key.
Wouldn't AES be better?
# generate session key
mechanism = nss.CKM_DES3_CBC_PAD
That's the default used by the KRA's client library, and that's what the
KRA has been tested with. We probably can change it to AES later. It
shouldn't be blocking this patch.
OK, no problem.
BTW, ipa-kra-install is broken with pki-core-10.2.4-1, but it works with
Re: [Freeipa-devel] [PATCH] Password vault
Dne 3.6.2015 v 14:17 Jan Cholasta napsal(a): Dne 2.6.2015 v 02:00 Endi Sukma Dewata napsal(a): Please take a look at the updated patch. On 5/27/2015 12:39 AM, Jan Cholasta wrote: 21) vault_archive is not a retrieve operation, it should be based on LDAPUpdate instead of LDAPRetrieve. Or Command actually, since it does not do anything with LDAP. The same applies to vault_retrieve. The vault_archive does not actually modify the LDAP entry because it stores the data in KRA. It is actually an LDAPRetrieve operation because it needs to get the vault info before it can perform the archival operation. Same thing with vault_retrieve. It is not a LDAPRetrieve operation, because it has different semantics. Please use Command as base class and either use ldap2 for direct LDAP or call vault_show instead of hacking around LDAPRetrieve. It's been changed to inherit from LDAPQuery instead. NACK, it's not a LDAPQuery operation, because it has different semantics. There is more to a command than executing code, so you should use a correct base class. Changed to inherit from Command as requested. Now these commands no longer have a direct access to the vault object (self.obj) although they are accessing vault objects like other vault commands. Also now the vault name argument has to be added explicitly on each command. You can inherit from crud.Retrieve and crud.Update to get self.obj and the argument back. I tried this: class vault_retrieve(Command, crud.Retrieve): and it gave me an error: TypeError: Error when calling the metaclass bases Cannot create a consistent method resolution order (MRO) for bases Retrieve, Command I'm sticking with the original code since it works fine although not ideal. I'm not a Python expert, so if you know how to fix this properly please feel free to post a patch on top of this. The class hierarchy is as follows: frontend.Command frontend.Method crud.PKQuery crud.Retrieve cdur.Update So removing Command from the list of base classes should fix it. If KRA is not installed, vault-archive and vault-retrieve fail with internal error. Added a code to check KRA installation in all vault commands. If you know a way not to load the vault plugin if the KRA is not installed please let me know, that's probably even better. Not sure how that will work on the client side though. I see this has been already resolved in the other thread. The commands still behave differently based on whether they were called from API which was initialized with in_server set to True or False. That is unfortunately a restriction imposed by the framework. In order to guarantee the security, the vault is designed to have separate client and server code. The client code encrypts the secret, the server code forwards the encrypted secret to KRA. To archive a secret into a vault properly, you are supposed to call the client code. If you're calling the server code directly, you are responsible to do your own encryption (i.e. generating session key, nonce, and vault data). I understand why the code has to be separated, what I don't understand is why it is in fact *not* separated and crammed into a single command, making weird and undefined behavior possible. If another plugin wants to use vault, it should implement a client code which calls the vault client code to perform the archival from the client side. What is the use case for calling the vault API from the server side anyway? Wouldn't that defeat the purpose of having a vault? If a secret exists on the server side in an unencrypted form doesn't it mean the secret may already have been compromised? Server API is used not only by the server itself, but also by installers for example. Anyway the point is that there *can't* be a broken API like this, you should at least raise an error if the command is called from server API, although actually separating it into client and server parts would be preferable. There is no point in exposing the session_key, nonce and vault_data options in CLI when their value is always overwritten in forward(). I agree there is no need to expose them in CLI, but in this framework the API also defines the CLI. If there's a way to keep them in the server API but not expose them in the CLI please let me know. Or, if there's a way to define completely separate server API (without a matching client CLI) and client CLI (without a matching server API) that will work too. As I suggested above, you can split the commands into separate client and server commands. The client command should inherit from frontend.Local so that it is always executed locally and the server command should have a NO_CLI = True attribute so that it is not available in the CLI. Will this always succeed? +# deactivate vault record in KRA +response = kra_client.keys.list_keys( +client_key_id, pki.key.KeyClient.KEY_STATUS_ACTIVE)
Re: [Freeipa-devel] [PATCH] Password vault
On 6/5/2015 7:13 AM, Jan Cholasta wrote:
If KRA is not installed, vault-archive and vault-retrieve fail with
internal error.
Added a code to check KRA installation in all vault commands. If you
know a way not to load the vault plugin if the KRA is not installed
please let me know, that's probably even better. Not sure how that will
work on the client side though.
I see this has been already resolved in the other thread.
The other thread was talking about removing the pki-base dependency on
the client side, but the vault plugin is still loaded on both client and
server regardless of KRA installation. Ideally the vault plugin should
not even be loaded so you cannot even execute the commands.
The commands still behave differently based on whether they were called
from API which was initialized with in_server set to True or False.
That is unfortunately a restriction imposed by the framework. In order
to guarantee the security, the vault is designed to have separate client
and server code. The client code encrypts the secret, the server code
forwards the encrypted secret to KRA. To archive a secret into a vault
properly, you are supposed to call the client code. If you're calling
the server code directly, you are responsible to do your own encryption
(i.e. generating session key, nonce, and vault data).
I understand why the code has to be separated, what I don't understand
is why it is in fact *not* separated and crammed into a single command,
making weird and undefined behavior possible.
If another plugin wants to use vault, it should implement a client code
which calls the vault client code to perform the archival from the
client side.
What is the use case for calling the vault API from the server side
anyway? Wouldn't that defeat the purpose of having a vault? If a secret
exists on the server side in an unencrypted form doesn't it mean the
secret may already have been compromised?
Server API is used not only by the server itself, but also by installers
for example. Anyway the point is that there *can't* be a broken API like
this, you should at least raise an error if the command is called from
server API, although actually separating it into client and server parts
would be preferable.
There is no point in exposing the session_key, nonce and vault_data
options in CLI when their value is always overwritten in forward().
I agree there is no need to expose them in CLI, but in this framework
the API also defines the CLI. If there's a way to keep them in the
server API but not expose them in the CLI please let me know. Or, if
there's a way to define completely separate server API (without a
matching client CLI) and client CLI (without a matching server API) that
will work too.
As I suggested above, you can split the commands into separate client
and server commands. The client command should inherit from
frontend.Local so that it is always executed locally and the server
command should have a NO_CLI = True attribute so that it is not
available in the CLI.
I see the vault_archive and vault_retrieve now inherit from PKQuery, and
there is a hack to execute the forward() even on the server side. A few
things below:
1. Why didn't you use frontend.Local as you initially suggested? If
there's a problem with frontend.Local please attach the ticket number in
the code.
2. The forward() can be merged into run(). There is no need to keep the
code in forward(). It would make more sense to have a run() method that
runs both on client and server, rather than a forward() that is supposed
to run on the client only but now forced to run on server too,
semantically speaking.
Attached is a patch including the requested changes.
I have also changed vault_config to vaultconfig_show, for consistency
with {,dns}config_show (it also makes the transport certificate
retrieval code in vault_{archive,retrieve} simpler).
3. The parameter description for nonce should be just 'Nonce' instead of
'Nonce encrypted'.
4. There's a PEP8 error.
5. The VERSION needs to be updated.
Assuming the above issues are addressed, ACK.
I have noticed that triple-length DES is used for the session key.
Wouldn't AES be better?
# generate session key
mechanism = nss.CKM_DES3_CBC_PAD
That's the default used by the KRA's client library, and that's what the
KRA has been tested with. We probably can change it to AES later. It
shouldn't be blocking this patch.
BTW, ipa-kra-install is broken with pki-core-10.2.4-1, but it works with
pki-core-10.2.1-3.
There's a bug in IPA: https://bugzilla.redhat.com/show_bug.cgi?id=1228671
--
Endi S. Dewata
--
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] Password vault
Dne 2.6.2015 v 20:40 Simo Sorce napsal(a): On Tue, 2015-06-02 at 07:07 -0500, Endi Sukma Dewata wrote: On 6/2/2015 1:10 AM, Martin Kosek wrote: Hi Endi, Quickly skimming through your patches raised couple questions on my side: 1) Will it be possible to also store plain text password via Vault? It talks about taking in the binary data or the text file, but will it also work with plain user secrets (passwords)? I am talking about use like this: # ipa vault-archive name --user mkosek --data Secret123 For security the plain text password should be stored in a file first: # vi password.txt # ipa vault-archive name --user mkosek --in password.txt It's also possible to specify the password as base-64 encoded data: # echo -n Secret123 | base64 # ipa vault-archive name --user mkosek --data U2VjcmV0MTIz But it's not recommended since the data will be stored in the command history and someone could see and decode it. I think passing a plain text password as command line argument would be even worse. The --data parameter is mainly used for unit testing. Later we might be able to add an option to read from standard input: # cat password.txt | ipa vault-archive name --user mkosek --std-in Yes please, a way to pass in via stdin is extremely useful, as leaving files on the filesystem is also a big risk. This will not work well, it should use the normal prompting mechanism: $ ipa vault-archive name --user user Data: type data here -- 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] Password vault
On 06/02/2015 11:22 PM, Alexander Bokovoy wrote: On Tue, 02 Jun 2015, Endi Sukma Dewata wrote: Please take a look at the new patch. On 6/2/2015 10:05 AM, Martin Kosek wrote: 4) In the vault-archive forward method, you use pki module. However, this module will be only available on FreeIPA PKI-powered servers and not on FreeIPA clients - so this will not work unless freeipa-client gets a dependency on pki-base - which is definitely not something we want... In my opinion it should be fine to require pki-base on the client because it contains just the client library, unless you have other concerns? Any objections to having pki-nss and pki-cryptography dependencies on the client? Even if we can change the client code not to depend on pki module, since in this framework the client and server code are written in the same plugin, the import pki still cannot be removed since it's still needed by the server code, and I don't think conditional import is a good programming practice. I have major concerns here. Look at the different between installing freeipa-client and freeipa-client + pki-base on my F21: ~~ $ sudo yum install freeipa-client ... Install 1 Package (+4 Dependent packages) Total download size: 2.6 M Installed size: 14 M ~~ $ sudo yum install freeipa-client pki-base ... Install 2 Packages (+288 Dependent packages) Total download size: 160 M Installed size: 235 M ~~ This is obviously a no-go for client. The conditional import is smaller concern that big dependency growth on the client. We do them in trust plugin for example and it works fine (though I agree it is not ideal programming practice). IMO, we should limit new freeipa-client dependencies only to python-cryptography (or also python-nss in the worst case, in case python-cryptography is not enough) - there should be no pki dependencies at all, these should be only on the server side. OK. I opened a ticket to split the pki-base into separate Python and Java packages: https://fedorahosted.org/pki/ticket/1399 For now in this patch I added conditional imports for pki.account and pki.key which are needed to access KRA on the server side. I removed dependency on pki.crypto on the client side and replaced it with direct python-nss code. On 6/2/2015 1:40 PM, Simo Sorce wrote: I have coded in python (jwcrypto) support for some key wrapping not yet available in python-cryptography and can lend you the code as needed. Thanks. I'll get back to you when it's time to add support for python-cryptography in KRA: https://fedorahosted.org/pki/ticket/1400 On 6/2/2015 10:16 AM, Alexander Bokovoy wrote: Yes, please use conditional import here, it is perfectly valid use case for the client side. On 6/2/2015 1:40 PM, Simo Sorce wrote: conditional import is just fine The conditional imports that I've seen usually are used for importing different versions of the same module, which I think is acceptable because the dependency always exists. In the vault case we're selectively importing a module depending on where the code runs. I think that is bad because it adds complexity and it's easy to make mistakes. Any code that depends on that module would have to be (a) guarded: if pki_is_loaded: ... call pki ... or (b) used in a method that's only called if the module is loaded: def do_something(self): # runs only on server ... call pki ... The (a) is similar to #ifdef's which should be avoidable using OOD, and in (b) we may inadvertently call a wrong method indirectly. I think ideally the client and server code should be in separate files (so they can be deployed separately too), but the framework doesn't seem to allow that. This exactly the case we have to use here and we are using that in trusts case as well -- some code has to run on server only and shouldn't cause to install Samba related packages on the client. This is because IPA client is actually using the same IPA plugins that server uses, to have access to the API calls metadata and client-side callbacks are defined in the same place where server-side callbacks are. It is IPA framework design, so we have to use what we have. This is planned to be changed BTW, when we start with the Thin Client concept and have different code/plugins for FreeIPA server side and client side. In other words, it is not necessarily an evil under conditions we are dealing with. -- 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] Password vault
On 06/02/2015 08:34 PM, Simo Sorce wrote: On Tue, 2015-06-02 at 12:04 +0200, Jan Cholasta wrote: Dne 2.6.2015 v 02:02 Endi Sukma Dewata napsal(a): On 5/28/2015 12:46 AM, Jan Cholasta wrote: On a related note, since KRA is optional, can we move the vaults container to cn=kra,cn=vaults? This is the convetion used by the other optional components (DNS and recently CA). I mean cn=vaults,cn=kra of course. If you are talking about the o=kra,PKI suffix, I'm not sure whether the IPA framework will work with it. If you are talking about adding a new cn=kra,IPA suffix entry on top of cn=vaults, what is the purpose of this entry? Is the entry going to be created/deleted automatically when the KRA is installed/removed? Is it going to be used for something else other than vaults? I'm talking about cn=kra,IPA suffix. It should be created only when KRA is installed, although I think this can be done later after the release, moving vaults to cn=kra should be good enough for now. It's going to be used for everything KRA-specific. There are a lot of questions that need to be answered before we can make this change. This is about sticking to a convention, which everyone should do, and everyone except KRA already does. I'm sorry I didn't realize this earlier, but the change must be done now. We probably should revisit this issue after the core vault functionality is added. We can't revisit it later because after release we are stuck with whatever is there forever. See attachment for a patch which implements the change. Shouldn't we s/kra/vault/ ? After all the feature is called Vault, not KRA. I thought we are naming it by the name of the optional subsystem, not the feature itself. If for example, another feature from KRA is used, it would still live in cn=kra, no? -- 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] Password vault
On Wed, 03 Jun 2015, Endi Sukma Dewata wrote: On 6/3/2015 1:41 AM, Martin Kosek wrote: On 06/02/2015 11:22 PM, Alexander Bokovoy wrote: On Tue, 02 Jun 2015, Endi Sukma Dewata wrote: I think ideally the client and server code should be in separate files (so they can be deployed separately too), but the framework doesn't seem to allow that. This exactly the case we have to use here and we are using that in trusts case as well -- some code has to run on server only and shouldn't cause to install Samba related packages on the client. This is because IPA client is actually using the same IPA plugins that server uses, to have access to the API calls metadata and client-side callbacks are defined in the same place where server-side callbacks are. It is IPA framework design, so we have to use what we have. This is planned to be changed BTW, when we start with the Thin Client concept and have different code/plugins for FreeIPA server side and client side. Is there a ticket for this? In other words, it is not necessarily an evil under conditions we are dealing with. Having to use the same plugins for client and server is a framework limitation/poor design. Having to use conditional imports to work around the limitation is a bad programming practice. The fact that trust plugin has to implement a similar workaround is not a justification, it just shows that the problem is not vault-specific. There is another thing. Even when splitting client/server sides, we'll need to check on the server side that certain functionality is available. In trust case we have ID Views (a separate plugin) which does use information about trusts to resolve users from AD to their normalized references (SIDs) and few other places would be depending on functionality only provided when Samba packages are installed. To continue your approach, we would need to split also server-side parts of plugins into separate callable units that would only be provided and called when appropriate rpm subpackages are installed. This is unneeded complication in place where we can simply handle dependencies in run time and make sure the packaging deps are managed separately. -- / 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] Password vault
On 6/3/2015 8:52 AM, Alexander Bokovoy wrote: Having to use the same plugins for client and server is a framework limitation/poor design. Having to use conditional imports to work around the limitation is a bad programming practice. The fact that trust plugin has to implement a similar workaround is not a justification, it just shows that the problem is not vault-specific. There is another thing. Even when splitting client/server sides, we'll need to check on the server side that certain functionality is available. In trust case we have ID Views (a separate plugin) which does use information about trusts to resolve users from AD to their normalized references (SIDs) and few other places would be depending on functionality only provided when Samba packages are installed. To continue your approach, we would need to split also server-side parts of plugins into separate callable units that would only be provided and called when appropriate rpm subpackages are installed. This is unneeded complication in place where we can simply handle dependencies in run time and make sure the packaging deps are managed separately. So there are two issues: 1. Conditional imports due to combined client and server plugin. 2. Conditional imports due to feature availability. Issue #1 is generic and I think we pretty much agree that this is supposed to be fixed somehow. Issue #2 is plugin-specific. I think there are different ways to solve this, some might be better than others. The solution that you pick will only affect that particular plugin and should not be generalized to other plugins or to justify #1. In my opinion a code should have a fixed dependency, but some features can be enabled/disabled based on the configuration. Enabling a feature shouldn't be based on package installation because people might install a package for different reasons. So the code may look something like this: import module if feature is enabled: do something with the module It shouldn't be like this: if package is installed: import module if package is installed: do something with the module Of course this assumes that the package is lightweight enough to be installed regardless whether it will be used. I don't know if it's applicable to your case, but again, there are different ways to address issue #2. -- Endi S. Dewata -- 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] Password vault
On 6/3/2015 1:41 AM, Martin Kosek wrote: On 06/02/2015 11:22 PM, Alexander Bokovoy wrote: On Tue, 02 Jun 2015, Endi Sukma Dewata wrote: I think ideally the client and server code should be in separate files (so they can be deployed separately too), but the framework doesn't seem to allow that. This exactly the case we have to use here and we are using that in trusts case as well -- some code has to run on server only and shouldn't cause to install Samba related packages on the client. This is because IPA client is actually using the same IPA plugins that server uses, to have access to the API calls metadata and client-side callbacks are defined in the same place where server-side callbacks are. It is IPA framework design, so we have to use what we have. This is planned to be changed BTW, when we start with the Thin Client concept and have different code/plugins for FreeIPA server side and client side. Is there a ticket for this? In other words, it is not necessarily an evil under conditions we are dealing with. Having to use the same plugins for client and server is a framework limitation/poor design. Having to use conditional imports to work around the limitation is a bad programming practice. The fact that trust plugin has to implement a similar workaround is not a justification, it just shows that the problem is not vault-specific. -- Endi S. Dewata -- 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] Password vault
On 6/2/2015 1:34 PM, Simo Sorce wrote: On Tue, 2015-06-02 at 12:04 +0200, Jan Cholasta wrote: Dne 2.6.2015 v 02:02 Endi Sukma Dewata napsal(a): On 5/28/2015 12:46 AM, Jan Cholasta wrote: On a related note, since KRA is optional, can we move the vaults container to cn=kra,cn=vaults? This is the convetion used by the other optional components (DNS and recently CA). I mean cn=vaults,cn=kra of course. If you are talking about the o=kra,PKI suffix, I'm not sure whether the IPA framework will work with it. If you are talking about adding a new cn=kra,IPA suffix entry on top of cn=vaults, what is the purpose of this entry? Is the entry going to be created/deleted automatically when the KRA is installed/removed? Is it going to be used for something else other than vaults? I'm talking about cn=kra,IPA suffix. It should be created only when KRA is installed, although I think this can be done later after the release, moving vaults to cn=kra should be good enough for now. It's going to be used for everything KRA-specific. There are a lot of questions that need to be answered before we can make this change. This is about sticking to a convention, which everyone should do, and everyone except KRA already does. I'm sorry I didn't realize this earlier, but the change must be done now. We probably should revisit this issue after the core vault functionality is added. We can't revisit it later because after release we are stuck with whatever is there forever. See attachment for a patch which implements the change. Shouldn't we s/kra/vault/ ? After all the feature is called Vault, not KRA. Simo. Here are the options: 1. the original code uses cn=vaults,IPA suffix. 2. Honza proposed cn=vaults,cn=kra,IPA suffix, ACKed by Martin. Are you proposing a third option cn=vaults,cn=vault,IPA suffix or did you mean the first option? I think the first option would make more sense since we're not introducing KRA to the end user, but I'll let the IPA team decide. My next patch will be based on whatever pushed at the time. -- Endi S. Dewata -- 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] Password vault
Dne 3.6.2015 v 14:58 Endi Sukma Dewata napsal(a): On 6/2/2015 1:34 PM, Simo Sorce wrote: On Tue, 2015-06-02 at 12:04 +0200, Jan Cholasta wrote: Dne 2.6.2015 v 02:02 Endi Sukma Dewata napsal(a): On 5/28/2015 12:46 AM, Jan Cholasta wrote: On a related note, since KRA is optional, can we move the vaults container to cn=kra,cn=vaults? This is the convetion used by the other optional components (DNS and recently CA). I mean cn=vaults,cn=kra of course. If you are talking about the o=kra,PKI suffix, I'm not sure whether the IPA framework will work with it. If you are talking about adding a new cn=kra,IPA suffix entry on top of cn=vaults, what is the purpose of this entry? Is the entry going to be created/deleted automatically when the KRA is installed/removed? Is it going to be used for something else other than vaults? I'm talking about cn=kra,IPA suffix. It should be created only when KRA is installed, although I think this can be done later after the release, moving vaults to cn=kra should be good enough for now. It's going to be used for everything KRA-specific. There are a lot of questions that need to be answered before we can make this change. This is about sticking to a convention, which everyone should do, and everyone except KRA already does. I'm sorry I didn't realize this earlier, but the change must be done now. We probably should revisit this issue after the core vault functionality is added. We can't revisit it later because after release we are stuck with whatever is there forever. See attachment for a patch which implements the change. Shouldn't we s/kra/vault/ ? After all the feature is called Vault, not KRA. Simo. Here are the options: 1. the original code uses cn=vaults,IPA suffix. 2. Honza proposed cn=vaults,cn=kra,IPA suffix, ACKed by Martin. Are you proposing a third option cn=vaults,cn=vault,IPA suffix or did you mean the first option? I think the first option would make more sense since we're not introducing KRA to the end user, but I'll let the IPA team decide. My next patch will be based on whatever pushed at the time. The DNs are not exposed to the end user, they are only relevant for our internal organization of entries. -- 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] Password vault
Dne 2.6.2015 v 02:00 Endi Sukma Dewata napsal(a): Please take a look at the updated patch. On 5/27/2015 12:39 AM, Jan Cholasta wrote: 21) vault_archive is not a retrieve operation, it should be based on LDAPUpdate instead of LDAPRetrieve. Or Command actually, since it does not do anything with LDAP. The same applies to vault_retrieve. The vault_archive does not actually modify the LDAP entry because it stores the data in KRA. It is actually an LDAPRetrieve operation because it needs to get the vault info before it can perform the archival operation. Same thing with vault_retrieve. It is not a LDAPRetrieve operation, because it has different semantics. Please use Command as base class and either use ldap2 for direct LDAP or call vault_show instead of hacking around LDAPRetrieve. It's been changed to inherit from LDAPQuery instead. NACK, it's not a LDAPQuery operation, because it has different semantics. There is more to a command than executing code, so you should use a correct base class. Changed to inherit from Command as requested. Now these commands no longer have a direct access to the vault object (self.obj) although they are accessing vault objects like other vault commands. Also now the vault name argument has to be added explicitly on each command. You can inherit from crud.Retrieve and crud.Update to get self.obj and the argument back. I tried this: class vault_retrieve(Command, crud.Retrieve): and it gave me an error: TypeError: Error when calling the metaclass bases Cannot create a consistent method resolution order (MRO) for bases Retrieve, Command I'm sticking with the original code since it works fine although not ideal. I'm not a Python expert, so if you know how to fix this properly please feel free to post a patch on top of this. The class hierarchy is as follows: frontend.Command frontend.Method crud.PKQuery crud.Retrieve cdur.Update So removing Command from the list of base classes should fix it. If KRA is not installed, vault-archive and vault-retrieve fail with internal error. Added a code to check KRA installation in all vault commands. If you know a way not to load the vault plugin if the KRA is not installed please let me know, that's probably even better. Not sure how that will work on the client side though. I see this has been already resolved in the other thread. The commands still behave differently based on whether they were called from API which was initialized with in_server set to True or False. That is unfortunately a restriction imposed by the framework. In order to guarantee the security, the vault is designed to have separate client and server code. The client code encrypts the secret, the server code forwards the encrypted secret to KRA. To archive a secret into a vault properly, you are supposed to call the client code. If you're calling the server code directly, you are responsible to do your own encryption (i.e. generating session key, nonce, and vault data). I understand why the code has to be separated, what I don't understand is why it is in fact *not* separated and crammed into a single command, making weird and undefined behavior possible. If another plugin wants to use vault, it should implement a client code which calls the vault client code to perform the archival from the client side. What is the use case for calling the vault API from the server side anyway? Wouldn't that defeat the purpose of having a vault? If a secret exists on the server side in an unencrypted form doesn't it mean the secret may already have been compromised? Server API is used not only by the server itself, but also by installers for example. Anyway the point is that there *can't* be a broken API like this, you should at least raise an error if the command is called from server API, although actually separating it into client and server parts would be preferable. There is no point in exposing the session_key, nonce and vault_data options in CLI when their value is always overwritten in forward(). I agree there is no need to expose them in CLI, but in this framework the API also defines the CLI. If there's a way to keep them in the server API but not expose them in the CLI please let me know. Or, if there's a way to define completely separate server API (without a matching client CLI) and client CLI (without a matching server API) that will work too. As I suggested above, you can split the commands into separate client and server commands. The client command should inherit from frontend.Local so that it is always executed locally and the server command should have a NO_CLI = True attribute so that it is not available in the CLI. Will this always succeed? +# deactivate vault record in KRA +response = kra_client.keys.list_keys( +client_key_id, pki.key.KeyClient.KEY_STATUS_ACTIVE) Yes. If there's no active keys it will
Re: [Freeipa-devel] [PATCH] Password vault
On Wed, 2015-06-03 at 09:27 +0200, Martin Kosek wrote: On 06/02/2015 08:34 PM, Simo Sorce wrote: On Tue, 2015-06-02 at 12:04 +0200, Jan Cholasta wrote: Dne 2.6.2015 v 02:02 Endi Sukma Dewata napsal(a): On 5/28/2015 12:46 AM, Jan Cholasta wrote: On a related note, since KRA is optional, can we move the vaults container to cn=kra,cn=vaults? This is the convetion used by the other optional components (DNS and recently CA). I mean cn=vaults,cn=kra of course. If you are talking about the o=kra,PKI suffix, I'm not sure whether the IPA framework will work with it. If you are talking about adding a new cn=kra,IPA suffix entry on top of cn=vaults, what is the purpose of this entry? Is the entry going to be created/deleted automatically when the KRA is installed/removed? Is it going to be used for something else other than vaults? I'm talking about cn=kra,IPA suffix. It should be created only when KRA is installed, although I think this can be done later after the release, moving vaults to cn=kra should be good enough for now. It's going to be used for everything KRA-specific. There are a lot of questions that need to be answered before we can make this change. This is about sticking to a convention, which everyone should do, and everyone except KRA already does. I'm sorry I didn't realize this earlier, but the change must be done now. We probably should revisit this issue after the core vault functionality is added. We can't revisit it later because after release we are stuck with whatever is there forever. See attachment for a patch which implements the change. Shouldn't we s/kra/vault/ ? After all the feature is called Vault, not KRA. I thought we are naming it by the name of the optional subsystem, not the feature itself. If for example, another feature from KRA is used, it would still live in cn=kra, no? For services so far we have CA, not dogtag, and LDAP, not 389ds, also KDC not krb5kdc and kpasswd not kadmind, etc... we normally named everything after the function. Now kra is probably a somewhat generic term, but I have not been able to find what it means exactly in 5 minutes, and it is quite obscure as a name. OTOH cn=Vault would make it really clear what's in it. I do not have a very strong opinion but a generic and clear name is important for the DIT. Simo. -- Simo Sorce * Red Hat, Inc * New York -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] Password vault
Dne 3.6.2015 v 15:20 Simo Sorce napsal(a): On Wed, 2015-06-03 at 09:27 +0200, Martin Kosek wrote: On 06/02/2015 08:34 PM, Simo Sorce wrote: On Tue, 2015-06-02 at 12:04 +0200, Jan Cholasta wrote: Dne 2.6.2015 v 02:02 Endi Sukma Dewata napsal(a): On 5/28/2015 12:46 AM, Jan Cholasta wrote: On a related note, since KRA is optional, can we move the vaults container to cn=kra,cn=vaults? This is the convetion used by the other optional components (DNS and recently CA). I mean cn=vaults,cn=kra of course. If you are talking about the o=kra,PKI suffix, I'm not sure whether the IPA framework will work with it. If you are talking about adding a new cn=kra,IPA suffix entry on top of cn=vaults, what is the purpose of this entry? Is the entry going to be created/deleted automatically when the KRA is installed/removed? Is it going to be used for something else other than vaults? I'm talking about cn=kra,IPA suffix. It should be created only when KRA is installed, although I think this can be done later after the release, moving vaults to cn=kra should be good enough for now. It's going to be used for everything KRA-specific. There are a lot of questions that need to be answered before we can make this change. This is about sticking to a convention, which everyone should do, and everyone except KRA already does. I'm sorry I didn't realize this earlier, but the change must be done now. We probably should revisit this issue after the core vault functionality is added. We can't revisit it later because after release we are stuck with whatever is there forever. See attachment for a patch which implements the change. Shouldn't we s/kra/vault/ ? After all the feature is called Vault, not KRA. I thought we are naming it by the name of the optional subsystem, not the feature itself. If for example, another feature from KRA is used, it would still live in cn=kra, no? For services so far we have CA, not dogtag, and LDAP, not 389ds, also KDC not krb5kdc and kpasswd not kadmind, etc... we normally named everything after the function. Now kra is probably a somewhat generic term, but I have not been able to find what it means exactly in 5 minutes, and it is quite obscure as a name. OTOH cn=Vault would make it really clear what's in it. I do not have a very strong opinion but a generic and clear name is important for the DIT. There is also ipa-kra-install and I guess cn=KRA,cn=fqdn,cn=masters,cn=ipa,cn=etc. If we rename it, it should be renamed everywhere, and I'm not sure if that's worth it. Also vault is too generic, it should be password vault, but that's too long, so IMO KRA is better, as it's short and descriptive. Are vaults the only feature KRA provides? If there are more possible features provided by KRA, it's another reason to keep it KRA. -- 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] Password vault
On Tue, 2015-06-02 at 07:07 -0500, Endi Sukma Dewata wrote: On 6/2/2015 1:10 AM, Martin Kosek wrote: Hi Endi, Quickly skimming through your patches raised couple questions on my side: 1) Will it be possible to also store plain text password via Vault? It talks about taking in the binary data or the text file, but will it also work with plain user secrets (passwords)? I am talking about use like this: # ipa vault-archive name --user mkosek --data Secret123 For security the plain text password should be stored in a file first: # vi password.txt # ipa vault-archive name --user mkosek --in password.txt It's also possible to specify the password as base-64 encoded data: # echo -n Secret123 | base64 # ipa vault-archive name --user mkosek --data U2VjcmV0MTIz But it's not recommended since the data will be stored in the command history and someone could see and decode it. I think passing a plain text password as command line argument would be even worse. The --data parameter is mainly used for unit testing. Later we might be able to add an option to read from standard input: # cat password.txt | ipa vault-archive name --user mkosek --std-in Yes please, a way to pass in via stdin is extremely useful, as leaving files on the filesystem is also a big risk. 2) Didn't we discuss a dependency of IPA/Vault on python-cryptography in the past? I rather see use of python-nss for cryptography... Yes. I might have mentioned that it would be in the 2nd (current) vault patch. Actually it will be in the 3rd patch when we add the symmetric and asymmetric vaults. The symmetric and asymmetric encryption will be implemented using python-cryptography. You can also see this in an old patch (#358) but it's obsolete now. The standard vault in the current patch uses python-nss for transport encryption because when the KRA interface was written python-cryptography wasn't available on Fedora, it didn't support certificates, and I'm not sure if it supports key wrapping. It depends on the key wrapping, I have coded in python (jwcrypto) support for some key wrapping not yet available in python-cryptography and can lend you the code as needed. The symmetric and asymmetric vaults add an additional layer of encryption on top of the standard transport encryption, so it will depend on both python-nss and python-cryptography. In the future if the KRA can support python-cryptography without python-nss we may be able to drop the python-nss dependency from vaults. 3) You do a lot of actions in the forward() method (as planned in https://www.freeipa.org/page/V4/Password_Vault#Archival). But how do you envision that this is consumed by the Web UI? It does not have access to the forward() method. Would it need to also include some crypto library? If Web UI wants to access vault (not sure if everybody agrees with that), it would have to perform an encryption on the browser side. In that case we will need to use either WebCrypto or a browser-specific extension to implement something similar to vault_archive.forward(), assuming the required cryptographic functionalities are available. In the future PKI might be able to provide a JavaScript interface for KRA. I so much want to NACK crypto in web browsers ... but we may have to do it, it stinks soo much though ... Perhaps a plugin ? 4) In the vault-archive forward method, you use pki module. However, this module will be only available on FreeIPA PKI-powered servers and not on FreeIPA clients - so this will not work unless freeipa-client gets a dependency on pki-base - which is definitely not something we want... In my opinion it should be fine to require pki-base on the client NACK look at the dependency chain for that packages. because it contains just the client library, unless you have other concerns? Any objections to having pki-nss and pki-cryptography dependencies on the client? you mean python-nss/python-cryptography ? I see no problem having them as dependencies on the client. Even if we can change the client code not to depend on pki module, since in this framework the client and server code are written in the same plugin, the import pki still cannot be removed since it's still needed by the server code, and I don't think conditional import is a good programming practice. conditional import is just fine Simo. -- Simo Sorce * Red Hat, Inc * New York -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] Password vault
On Tue, 2015-06-02 at 12:04 +0200, Jan Cholasta wrote: Dne 2.6.2015 v 02:02 Endi Sukma Dewata napsal(a): On 5/28/2015 12:46 AM, Jan Cholasta wrote: On a related note, since KRA is optional, can we move the vaults container to cn=kra,cn=vaults? This is the convetion used by the other optional components (DNS and recently CA). I mean cn=vaults,cn=kra of course. If you are talking about the o=kra,PKI suffix, I'm not sure whether the IPA framework will work with it. If you are talking about adding a new cn=kra,IPA suffix entry on top of cn=vaults, what is the purpose of this entry? Is the entry going to be created/deleted automatically when the KRA is installed/removed? Is it going to be used for something else other than vaults? I'm talking about cn=kra,IPA suffix. It should be created only when KRA is installed, although I think this can be done later after the release, moving vaults to cn=kra should be good enough for now. It's going to be used for everything KRA-specific. There are a lot of questions that need to be answered before we can make this change. This is about sticking to a convention, which everyone should do, and everyone except KRA already does. I'm sorry I didn't realize this earlier, but the change must be done now. We probably should revisit this issue after the core vault functionality is added. We can't revisit it later because after release we are stuck with whatever is there forever. See attachment for a patch which implements the change. Shouldn't we s/kra/vault/ ? After all the feature is called Vault, not KRA. Simo. -- Simo Sorce * Red Hat, Inc * New York -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
Re: [Freeipa-devel] [PATCH] Password vault
Please take a look at the new patch.
On 6/2/2015 10:05 AM, Martin Kosek wrote:
4) In the vault-archive forward method, you use pki module. However,
this module will be only available on FreeIPA PKI-powered servers and
not on FreeIPA clients - so this will not work unless freeipa-client
gets a dependency on pki-base - which is definitely not something we
want...
In my opinion it should be fine to require pki-base on the client because it
contains just the client library, unless you have other concerns? Any
objections to having pki-nss and pki-cryptography dependencies on the client?
Even if we can change the client code not to depend on pki module, since in
this framework the client and server code are written in the same plugin, the
import pki still cannot be removed since it's still needed by the server
code, and I don't think conditional import is a good programming practice.
I have major concerns here. Look at the different between installing
freeipa-client and freeipa-client + pki-base on my F21:
~~
$ sudo yum install freeipa-client
...
Install 1 Package (+4 Dependent packages)
Total download size: 2.6 M
Installed size: 14 M
~~
$ sudo yum install freeipa-client pki-base
...
Install 2 Packages (+288 Dependent packages)
Total download size: 160 M
Installed size: 235 M
~~
This is obviously a no-go for client. The conditional import is smaller concern
that big dependency growth on the client. We do them in trust plugin for
example and it works fine (though I agree it is not ideal programming practice).
IMO, we should limit new freeipa-client dependencies only to
python-cryptography (or also python-nss in the worst case, in case
python-cryptography is not enough) - there should be no pki dependencies at
all, these should be only on the server side.
OK. I opened a ticket to split the pki-base into separate Python and
Java packages:
https://fedorahosted.org/pki/ticket/1399
For now in this patch I added conditional imports for pki.account and
pki.key which are needed to access KRA on the server side. I removed
dependency on pki.crypto on the client side and replaced it with direct
python-nss code.
On 6/2/2015 1:40 PM, Simo Sorce wrote:
I have coded in python (jwcrypto)
support for some key wrapping not yet available in python-cryptography
and can lend you the code as needed.
Thanks. I'll get back to you when it's time to add support for
python-cryptography in KRA:
https://fedorahosted.org/pki/ticket/1400
On 6/2/2015 10:16 AM, Alexander Bokovoy wrote:
Yes, please use conditional import here, it is perfectly valid use case
for the client side.
On 6/2/2015 1:40 PM, Simo Sorce wrote:
conditional import is just fine
The conditional imports that I've seen usually are used for importing
different versions of the same module, which I think is acceptable
because the dependency always exists. In the vault case we're
selectively importing a module depending on where the code runs. I think
that is bad because it adds complexity and it's easy to make mistakes.
Any code that depends on that module would have to be (a) guarded:
if pki_is_loaded:
... call pki ...
or (b) used in a method that's only called if the module is loaded:
def do_something(self): # runs only on server
... call pki ...
The (a) is similar to #ifdef's which should be avoidable using OOD, and
in (b) we may inadvertently call a wrong method indirectly. I think
ideally the client and server code should be in separate files (so they
can be deployed separately too), but the framework doesn't seem to allow
that.
Regardless, the conditional imports are in.
--
Endi S. Dewata
From 0e9d3868423c21dc47d125f4b3c23e8261c4655f Mon Sep 17 00:00:00 2001
From: Endi S. Dewata edew...@redhat.com
Date: Tue, 21 Oct 2014 10:57:08 -0400
Subject: [PATCH] Added vault-archive and vault-retrieve commands.
New commands have been added to archive and retrieve
data into and from a vault, also to retrieve the
transport certificate.
https://fedorahosted.org/freeipa/ticket/3872
---
API.txt | 28 ++
VERSION | 4 +-
ipalib/plugins/vault.py | 517 +-
ipatests/test_xmlrpc/test_vault_plugin.py | 71 +++-
4 files changed, 616 insertions(+), 4 deletions(-)
diff --git a/API.txt b/API.txt
index
da69f32de5c12c0d85a7d61d9027385aa3c0ee05..3741e6f16689e43838c2d31a44872d1ea47589c7
100644
--- a/API.txt
+++ b/API.txt
@@ -4768,6 +4768,24 @@ option: Str('version?', exclude='webui')
output: Entry('result', type 'dict', Gettext('A dictionary representing an
LDAP entry', domain='ipa', localedir=None))
output: Output('summary', (type 'unicode', type 'NoneType'), None)
output: PrimaryKey('value', None, None)
+command: vault_archive
+args: 1,9,1
+arg: Str('cn', cli_name='name',
Re: [Freeipa-devel] [PATCH] Password vault
On Tue, 02 Jun 2015, Endi Sukma Dewata wrote: Please take a look at the new patch. On 6/2/2015 10:05 AM, Martin Kosek wrote: 4) In the vault-archive forward method, you use pki module. However, this module will be only available on FreeIPA PKI-powered servers and not on FreeIPA clients - so this will not work unless freeipa-client gets a dependency on pki-base - which is definitely not something we want... In my opinion it should be fine to require pki-base on the client because it contains just the client library, unless you have other concerns? Any objections to having pki-nss and pki-cryptography dependencies on the client? Even if we can change the client code not to depend on pki module, since in this framework the client and server code are written in the same plugin, the import pki still cannot be removed since it's still needed by the server code, and I don't think conditional import is a good programming practice. I have major concerns here. Look at the different between installing freeipa-client and freeipa-client + pki-base on my F21: ~~ $ sudo yum install freeipa-client ... Install 1 Package (+4 Dependent packages) Total download size: 2.6 M Installed size: 14 M ~~ $ sudo yum install freeipa-client pki-base ... Install 2 Packages (+288 Dependent packages) Total download size: 160 M Installed size: 235 M ~~ This is obviously a no-go for client. The conditional import is smaller concern that big dependency growth on the client. We do them in trust plugin for example and it works fine (though I agree it is not ideal programming practice). IMO, we should limit new freeipa-client dependencies only to python-cryptography (or also python-nss in the worst case, in case python-cryptography is not enough) - there should be no pki dependencies at all, these should be only on the server side. OK. I opened a ticket to split the pki-base into separate Python and Java packages: https://fedorahosted.org/pki/ticket/1399 For now in this patch I added conditional imports for pki.account and pki.key which are needed to access KRA on the server side. I removed dependency on pki.crypto on the client side and replaced it with direct python-nss code. On 6/2/2015 1:40 PM, Simo Sorce wrote: I have coded in python (jwcrypto) support for some key wrapping not yet available in python-cryptography and can lend you the code as needed. Thanks. I'll get back to you when it's time to add support for python-cryptography in KRA: https://fedorahosted.org/pki/ticket/1400 On 6/2/2015 10:16 AM, Alexander Bokovoy wrote: Yes, please use conditional import here, it is perfectly valid use case for the client side. On 6/2/2015 1:40 PM, Simo Sorce wrote: conditional import is just fine The conditional imports that I've seen usually are used for importing different versions of the same module, which I think is acceptable because the dependency always exists. In the vault case we're selectively importing a module depending on where the code runs. I think that is bad because it adds complexity and it's easy to make mistakes. Any code that depends on that module would have to be (a) guarded: if pki_is_loaded: ... call pki ... or (b) used in a method that's only called if the module is loaded: def do_something(self): # runs only on server ... call pki ... The (a) is similar to #ifdef's which should be avoidable using OOD, and in (b) we may inadvertently call a wrong method indirectly. I think ideally the client and server code should be in separate files (so they can be deployed separately too), but the framework doesn't seem to allow that. This exactly the case we have to use here and we are using that in trusts case as well -- some code has to run on server only and shouldn't cause to install Samba related packages on the client. This is because IPA client is actually using the same IPA plugins that server uses, to have access to the API calls metadata and client-side callbacks are defined in the same place where server-side callbacks are. It is IPA framework design, so we have to use what we have. In other words, it is not necessarily an evil under conditions we are dealing with. -- / 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] Password vault
On 06/02/2015 12:04 PM, Jan Cholasta wrote: Dne 2.6.2015 v 02:02 Endi Sukma Dewata napsal(a): On 5/28/2015 12:46 AM, Jan Cholasta wrote: On a related note, since KRA is optional, can we move the vaults container to cn=kra,cn=vaults? This is the convetion used by the other optional components (DNS and recently CA). I mean cn=vaults,cn=kra of course. If you are talking about the o=kra,PKI suffix, I'm not sure whether the IPA framework will work with it. If you are talking about adding a new cn=kra,IPA suffix entry on top of cn=vaults, what is the purpose of this entry? Is the entry going to be created/deleted automatically when the KRA is installed/removed? Is it going to be used for something else other than vaults? I'm talking about cn=kra,IPA suffix. It should be created only when KRA is installed, although I think this can be done later after the release, moving vaults to cn=kra should be good enough for now. It's going to be used for everything KRA-specific. There are a lot of questions that need to be answered before we can make this change. This is about sticking to a convention, which everyone should do, and everyone except KRA already does. I'm sorry I didn't realize this earlier, but the change must be done now. +1 for this change. I do not even think it will that big deal, it is just about the default space in the IPA tree - to have proper structure in it (DNS has cn=dns, KRA has cn=kra, etc.). We probably should revisit this issue after the core vault functionality is added. We can't revisit it later because after release we are stuck with whatever is there forever. Right. -- 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] Password vault
On 6/2/2015 1:10 AM, Martin Kosek wrote: Hi Endi, Quickly skimming through your patches raised couple questions on my side: 1) Will it be possible to also store plain text password via Vault? It talks about taking in the binary data or the text file, but will it also work with plain user secrets (passwords)? I am talking about use like this: # ipa vault-archive name --user mkosek --data Secret123 For security the plain text password should be stored in a file first: # vi password.txt # ipa vault-archive name --user mkosek --in password.txt It's also possible to specify the password as base-64 encoded data: # echo -n Secret123 | base64 # ipa vault-archive name --user mkosek --data U2VjcmV0MTIz But it's not recommended since the data will be stored in the command history and someone could see and decode it. I think passing a plain text password as command line argument would be even worse. The --data parameter is mainly used for unit testing. Later we might be able to add an option to read from standard input: # cat password.txt | ipa vault-archive name --user mkosek --std-in 2) Didn't we discuss a dependency of IPA/Vault on python-cryptography in the past? I rather see use of python-nss for cryptography... Yes. I might have mentioned that it would be in the 2nd (current) vault patch. Actually it will be in the 3rd patch when we add the symmetric and asymmetric vaults. The symmetric and asymmetric encryption will be implemented using python-cryptography. You can also see this in an old patch (#358) but it's obsolete now. The standard vault in the current patch uses python-nss for transport encryption because when the KRA interface was written python-cryptography wasn't available on Fedora, it didn't support certificates, and I'm not sure if it supports key wrapping. The symmetric and asymmetric vaults add an additional layer of encryption on top of the standard transport encryption, so it will depend on both python-nss and python-cryptography. In the future if the KRA can support python-cryptography without python-nss we may be able to drop the python-nss dependency from vaults. 3) You do a lot of actions in the forward() method (as planned in https://www.freeipa.org/page/V4/Password_Vault#Archival). But how do you envision that this is consumed by the Web UI? It does not have access to the forward() method. Would it need to also include some crypto library? If Web UI wants to access vault (not sure if everybody agrees with that), it would have to perform an encryption on the browser side. In that case we will need to use either WebCrypto or a browser-specific extension to implement something similar to vault_archive.forward(), assuming the required cryptographic functionalities are available. In the future PKI might be able to provide a JavaScript interface for KRA. 4) In the vault-archive forward method, you use pki module. However, this module will be only available on FreeIPA PKI-powered servers and not on FreeIPA clients - so this will not work unless freeipa-client gets a dependency on pki-base - which is definitely not something we want... In my opinion it should be fine to require pki-base on the client because it contains just the client library, unless you have other concerns? Any objections to having pki-nss and pki-cryptography dependencies on the client? Even if we can change the client code not to depend on pki module, since in this framework the client and server code are written in the same plugin, the import pki still cannot be removed since it's still needed by the server code, and I don't think conditional import is a good programming practice. Thanks, Martin -- Endi S. Dewata -- 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] Password vault
Dne 2.6.2015 v 02:02 Endi Sukma Dewata napsal(a):
On 5/28/2015 12:46 AM, Jan Cholasta wrote:
On a related note, since KRA is optional, can we move the vaults
container to cn=kra,cn=vaults? This is the convetion used by the other
optional components (DNS and recently CA).
I mean cn=vaults,cn=kra of course.
If you are talking about the o=kra,PKI suffix, I'm not sure whether
the IPA framework will work with it.
If you are talking about adding a new cn=kra,IPA suffix entry on top
of cn=vaults, what is the purpose of this entry? Is the entry going to
be created/deleted automatically when the KRA is installed/removed? Is
it going to be used for something else other than vaults?
I'm talking about cn=kra,IPA suffix. It should be created only when
KRA is installed, although I think this can be done later after the
release, moving vaults to cn=kra should be good enough for now. It's
going to be used for everything KRA-specific.
There are a lot of questions that need to be answered before we can make
this change.
This is about sticking to a convention, which everyone should do, and
everyone except KRA already does.
I'm sorry I didn't realize this earlier, but the change must be done now.
We probably should revisit this issue after the core vault
functionality is added.
We can't revisit it later because after release we are stuck with
whatever is there forever.
See attachment for a patch which implements the change.
--
Jan Cholasta
From 47295de5fb63195f7c07fa2528c6d6450e25d659 Mon Sep 17 00:00:00 2001
From: Jan Cholasta jchol...@redhat.com
Date: Tue, 2 Jun 2015 09:40:50 +
Subject: [PATCH] vault: Move vaults to cn=vaults,cn=kra
https://fedorahosted.org/freeipa/ticket/3872
---
install/updates/40-vault.update | 13 +
ipa-client/man/default.conf.5 | 2 +-
ipalib/constants.py | 2 +-
ipatests/test_xmlrpc/test_vault_plugin.py | 24
4 files changed, 23 insertions(+), 18 deletions(-)
diff --git a/install/updates/40-vault.update b/install/updates/40-vault.update
index 5a6b8c6..dcd1e2a 100644
--- a/install/updates/40-vault.update
+++ b/install/updates/40-vault.update
@@ -1,19 +1,24 @@
-dn: cn=vaults,$SUFFIX
+dn: cn=kra,$SUFFIX
+default: objectClass: top
+default: objectClass: nsContainer
+default: cn: kra
+
+dn: cn=vaults,cn=kra,$SUFFIX
default: objectClass: top
default: objectClass: nsContainer
default: cn: vaults
-dn: cn=services,cn=vaults,$SUFFIX
+dn: cn=services,cn=vaults,cn=kra,$SUFFIX
default: objectClass: top
default: objectClass: nsContainer
default: cn: services
-dn: cn=shared,cn=vaults,$SUFFIX
+dn: cn=shared,cn=vaults,cn=kra,$SUFFIX
default: objectClass: top
default: objectClass: nsContainer
default: cn: shared
-dn: cn=users,cn=vaults,$SUFFIX
+dn: cn=users,cn=vaults,cn=kra,$SUFFIX
default: objectClass: top
default: objectClass: nsContainer
default: cn: users
diff --git a/ipa-client/man/default.conf.5 b/ipa-client/man/default.conf.5
index 0973f1a..e345e93 100644
--- a/ipa-client/man/default.conf.5
+++ b/ipa-client/man/default.conf.5
@@ -221,7 +221,7 @@ The following define the containers for the IPA server. Containers define where
container_sudocmdgroup: cn=sudocmdgroups,cn=sudo
container_sudorule: cn=sudorules,cn=sudo
container_user: cn=users,cn=accounts
- container_vault: cn=vaults
+ container_vault: cn=vaults,cn=kra
container_virtual: cn=virtual operations,cn=etc
.SH FILES
diff --git a/ipalib/constants.py b/ipalib/constants.py
index 95dec54..7815e14 100644
--- a/ipalib/constants.py
+++ b/ipalib/constants.py
@@ -99,7 +99,7 @@ DEFAULT_CONFIG = (
('container_hbacservice', DN(('cn', 'hbacservices'), ('cn', 'hbac'))),
('container_hbacservicegroup', DN(('cn', 'hbacservicegroups'), ('cn', 'hbac'))),
('container_dns', DN(('cn', 'dns'))),
-('container_vault', DN(('cn', 'vaults'))),
+('container_vault', DN(('cn', 'vaults'), ('cn', 'kra'))),
('container_virtual', DN(('cn', 'virtual operations'), ('cn', 'etc'))),
('container_sudorule', DN(('cn', 'sudorules'), ('cn', 'sudo'))),
('container_sudocmd', DN(('cn', 'sudocmds'), ('cn', 'sudo'))),
diff --git a/ipatests/test_xmlrpc/test_vault_plugin.py b/ipatests/test_xmlrpc/test_vault_plugin.py
index 44d397c..012baec 100644
--- a/ipatests/test_xmlrpc/test_vault_plugin.py
+++ b/ipatests/test_xmlrpc/test_vault_plugin.py
@@ -54,7 +54,7 @@ class test_vault_plugin(Declarative):
'value': vault_name,
'summary': 'Added vault %s' % vault_name,
'result': {
-'dn': u'cn=%s,cn=admin,cn=users,cn=vaults,%s'
+'dn': u'cn=%s,cn=admin,cn=users,cn=vaults,cn=kra,%s'
% (vault_name, api.env.basedn),
'objectclass': [u'top', u'ipaVault'],
'cn': [vault_name],
@@ -75,7 +75,7 @@ class test_vault_plugin(Declarative):
'summary': u'1 vault matched',
Re: [Freeipa-devel] [PATCH] Password vault
On 06/02/2015 02:07 PM, Endi Sukma Dewata wrote: On 6/2/2015 1:10 AM, Martin Kosek wrote: Hi Endi, Quickly skimming through your patches raised couple questions on my side: 1) Will it be possible to also store plain text password via Vault? It talks about taking in the binary data or the text file, but will it also work with plain user secrets (passwords)? I am talking about use like this: # ipa vault-archive name --user mkosek --data Secret123 For security the plain text password should be stored in a file first: # vi password.txt # ipa vault-archive name --user mkosek --in password.txt It's also possible to specify the password as base-64 encoded data: # echo -n Secret123 | base64 # ipa vault-archive name --user mkosek --data U2VjcmV0MTIz But it's not recommended since the data will be stored in the command history and someone could see and decode it. I think passing a plain text password as command line argument would be even worse. The --data parameter is mainly used for unit testing. Later we might be able to add an option to read from standard input: # cat password.txt | ipa vault-archive name --user mkosek --std-in Ok. Well, base64 + file input look as good enough for now. I was mostly concerned about usability of the solution for normal users as for a manual secret, it is not convenient to always create an interim file. We will see based on user experience, maybe Web UI or further CLI-only additions will be the answer. 2) Didn't we discuss a dependency of IPA/Vault on python-cryptography in the past? I rather see use of python-nss for cryptography... Yes. I might have mentioned that it would be in the 2nd (current) vault patch. Actually it will be in the 3rd patch when we add the symmetric and asymmetric vaults. The symmetric and asymmetric encryption will be implemented using python-cryptography. You can also see this in an old patch (#358) but it's obsolete now. Ok. The standard vault in the current patch uses python-nss for transport encryption because when the KRA interface was written python-cryptography wasn't available on Fedora, it didn't support certificates, and I'm not sure if it supports key wrapping. The symmetric and asymmetric vaults add an additional layer of encryption on top of the standard transport encryption, so it will depend on both python-nss and python-cryptography. In the future if the KRA can support python-cryptography without python-nss we may be able to drop the python-nss dependency from vaults. Ok. 3) You do a lot of actions in the forward() method (as planned in https://www.freeipa.org/page/V4/Password_Vault#Archival). But how do you envision that this is consumed by the Web UI? It does not have access to the forward() method. Would it need to also include some crypto library? If Web UI wants to access vault (not sure if everybody agrees with that), it would have to perform an encryption on the browser side. In that case we will need to use either WebCrypto or a browser-specific extension to implement something similar to vault_archive.forward(), assuming the required cryptographic functionalities are available. In the future PKI might be able to provide a JavaScript interface for KRA. Ok, makes sense. I think we will want Web UI at some point, but the summary for FreeIPA 4.2 seems - no Web UI for Vault (yet). 4) In the vault-archive forward method, you use pki module. However, this module will be only available on FreeIPA PKI-powered servers and not on FreeIPA clients - so this will not work unless freeipa-client gets a dependency on pki-base - which is definitely not something we want... In my opinion it should be fine to require pki-base on the client because it contains just the client library, unless you have other concerns? Any objections to having pki-nss and pki-cryptography dependencies on the client? Even if we can change the client code not to depend on pki module, since in this framework the client and server code are written in the same plugin, the import pki still cannot be removed since it's still needed by the server code, and I don't think conditional import is a good programming practice. I have major concerns here. Look at the different between installing freeipa-client and freeipa-client + pki-base on my F21: ~~ $ sudo yum install freeipa-client ... Install 1 Package (+4 Dependent packages) Total download size: 2.6 M Installed size: 14 M ~~ $ sudo yum install freeipa-client pki-base ... Install 2 Packages (+288 Dependent packages) Total download size: 160 M Installed size: 235 M ~~ This is obviously a no-go for client. The conditional import is smaller concern that big dependency growth on the client. We do them in trust plugin for example and it works fine (though I agree it is not ideal programming
Re: [Freeipa-devel] [PATCH] Password vault
On Tue, 02 Jun 2015, Martin Kosek wrote: But it's not recommended since the data will be stored in the command history and someone could see and decode it. I think passing a plain text password as command line argument would be even worse. The --data parameter is mainly used for unit testing. Later we might be able to add an option to read from standard input: # cat password.txt | ipa vault-archive name --user mkosek --std-in Ok. Well, base64 + file input look as good enough for now. I was mostly concerned about usability of the solution for normal users as for a manual secret, it is not convenient to always create an interim file. We will see based on user experience, maybe Web UI or further CLI-only additions will be the answer. Correct, this is a part that can and should be driven by actual use experience. Reading from the stdin is easy to implement (we have it done for password already) so maybe we can simply reuse password option here for such case, we even have a flag for omitting the confirmation prompt. This is fairly small addition. 4) In the vault-archive forward method, you use pki module. However, this module will be only available on FreeIPA PKI-powered servers and not on FreeIPA clients - so this will not work unless freeipa-client gets a dependency on pki-base - which is definitely not something we want... In my opinion it should be fine to require pki-base on the client because it contains just the client library, unless you have other concerns? Any objections to having pki-nss and pki-cryptography dependencies on the client? Even if we can change the client code not to depend on pki module, since in this framework the client and server code are written in the same plugin, the import pki still cannot be removed since it's still needed by the server code, and I don't think conditional import is a good programming practice. I have major concerns here. Look at the different between installing freeipa-client and freeipa-client + pki-base on my F21: ~~ $ sudo yum install freeipa-client ... Install 1 Package (+4 Dependent packages) Total download size: 2.6 M Installed size: 14 M ~~ $ sudo yum install freeipa-client pki-base ... Install 2 Packages (+288 Dependent packages) Total download size: 160 M Installed size: 235 M ~~ This is obviously a no-go for client. The conditional import is smaller concern that big dependency growth on the client. We do them in trust plugin for example and it works fine (though I agree it is not ideal programming practice). IMO, we should limit new freeipa-client dependencies only to python-cryptography (or also python-nss in the worst case, in case python-cryptography is not enough) - there should be no pki dependencies at all, these should be only on the server side. Yes, please use conditional import here, it is perfectly valid use case for the client side. -- / 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] Password vault
On 06/02/2015 02:00 AM, Endi Sukma Dewata wrote: Please take a look at the updated patch. On 5/27/2015 12:39 AM, Jan Cholasta wrote: 21) vault_archive is not a retrieve operation, it should be based on LDAPUpdate instead of LDAPRetrieve. Or Command actually, since it does not do anything with LDAP. The same applies to vault_retrieve. The vault_archive does not actually modify the LDAP entry because it stores the data in KRA. It is actually an LDAPRetrieve operation because it needs to get the vault info before it can perform the archival operation. Same thing with vault_retrieve. It is not a LDAPRetrieve operation, because it has different semantics. Please use Command as base class and either use ldap2 for direct LDAP or call vault_show instead of hacking around LDAPRetrieve. It's been changed to inherit from LDAPQuery instead. NACK, it's not a LDAPQuery operation, because it has different semantics. There is more to a command than executing code, so you should use a correct base class. Changed to inherit from Command as requested. Now these commands no longer have a direct access to the vault object (self.obj) although they are accessing vault objects like other vault commands. Also now the vault name argument has to be added explicitly on each command. You can inherit from crud.Retrieve and crud.Update to get self.obj and the argument back. I tried this: class vault_retrieve(Command, crud.Retrieve): and it gave me an error: TypeError: Error when calling the metaclass bases Cannot create a consistent method resolution order (MRO) for bases Retrieve, Command I'm sticking with the original code since it works fine although not ideal. I'm not a Python expert, so if you know how to fix this properly please feel free to post a patch on top of this. If KRA is not installed, vault-archive and vault-retrieve fail with internal error. Added a code to check KRA installation in all vault commands. If you know a way not to load the vault plugin if the KRA is not installed please let me know, that's probably even better. Not sure how that will work on the client side though. The commands still behave differently based on whether they were called from API which was initialized with in_server set to True or False. That is unfortunately a restriction imposed by the framework. In order to guarantee the security, the vault is designed to have separate client and server code. The client code encrypts the secret, the server code forwards the encrypted secret to KRA. To archive a secret into a vault properly, you are supposed to call the client code. If you're calling the server code directly, you are responsible to do your own encryption (i.e. generating session key, nonce, and vault data). If another plugin wants to use vault, it should implement a client code which calls the vault client code to perform the archival from the client side. What is the use case for calling the vault API from the server side anyway? Wouldn't that defeat the purpose of having a vault? If a secret exists on the server side in an unencrypted form doesn't it mean the secret may already have been compromised? There is no point in exposing the session_key, nonce and vault_data options in CLI when their value is always overwritten in forward(). I agree there is no need to expose them in CLI, but in this framework the API also defines the CLI. If there's a way to keep them in the server API but not expose them in the CLI please let me know. Or, if there's a way to define completely separate server API (without a matching client CLI) and client CLI (without a matching server API) that will work too. Will this always succeed? +# deactivate vault record in KRA +response = kra_client.keys.list_keys( +client_key_id, pki.key.KeyClient.KEY_STATUS_ACTIVE) Yes. If there's no active keys it will return an empty collection. +for key_info in response.key_infos: +kra_client.keys.modify_key_status( +key_info.get_key_id(), +pki.key.KeyClient.KEY_STATUS_INACTIVE) This loop will do nothing given an empty collection. If not, we might get into an inconsistent state, where the vault is deleted in LDAP but still active in KRA. (I'm not sure if this is actually a problem or not.) That can only happen if the server crashes after deleting the vault but before deactivating the key. Regardless, it will not be a problem because the key is identified by vault ID/path so it will not conflict with other vaults, and it will get overwritten if the same vault is recreated again. Hi Endi, Quickly skimming through your patches raised couple questions on my side: 1) Will it be possible to also store plain text password via Vault? It talks about taking in the binary data or the text file, but will it also work with plain user secrets (passwords)? I am talking about use like this: # ipa vault-archive name --user mkosek --data Secret123
Re: [Freeipa-devel] [PATCH] Password vault
Please take a look at the updated patch. On 5/27/2015 12:39 AM, Jan Cholasta wrote: 21) vault_archive is not a retrieve operation, it should be based on LDAPUpdate instead of LDAPRetrieve. Or Command actually, since it does not do anything with LDAP. The same applies to vault_retrieve. The vault_archive does not actually modify the LDAP entry because it stores the data in KRA. It is actually an LDAPRetrieve operation because it needs to get the vault info before it can perform the archival operation. Same thing with vault_retrieve. It is not a LDAPRetrieve operation, because it has different semantics. Please use Command as base class and either use ldap2 for direct LDAP or call vault_show instead of hacking around LDAPRetrieve. It's been changed to inherit from LDAPQuery instead. NACK, it's not a LDAPQuery operation, because it has different semantics. There is more to a command than executing code, so you should use a correct base class. Changed to inherit from Command as requested. Now these commands no longer have a direct access to the vault object (self.obj) although they are accessing vault objects like other vault commands. Also now the vault name argument has to be added explicitly on each command. You can inherit from crud.Retrieve and crud.Update to get self.obj and the argument back. I tried this: class vault_retrieve(Command, crud.Retrieve): and it gave me an error: TypeError: Error when calling the metaclass bases Cannot create a consistent method resolution order (MRO) for bases Retrieve, Command I'm sticking with the original code since it works fine although not ideal. I'm not a Python expert, so if you know how to fix this properly please feel free to post a patch on top of this. If KRA is not installed, vault-archive and vault-retrieve fail with internal error. Added a code to check KRA installation in all vault commands. If you know a way not to load the vault plugin if the KRA is not installed please let me know, that's probably even better. Not sure how that will work on the client side though. The commands still behave differently based on whether they were called from API which was initialized with in_server set to True or False. That is unfortunately a restriction imposed by the framework. In order to guarantee the security, the vault is designed to have separate client and server code. The client code encrypts the secret, the server code forwards the encrypted secret to KRA. To archive a secret into a vault properly, you are supposed to call the client code. If you're calling the server code directly, you are responsible to do your own encryption (i.e. generating session key, nonce, and vault data). If another plugin wants to use vault, it should implement a client code which calls the vault client code to perform the archival from the client side. What is the use case for calling the vault API from the server side anyway? Wouldn't that defeat the purpose of having a vault? If a secret exists on the server side in an unencrypted form doesn't it mean the secret may already have been compromised? There is no point in exposing the session_key, nonce and vault_data options in CLI when their value is always overwritten in forward(). I agree there is no need to expose them in CLI, but in this framework the API also defines the CLI. If there's a way to keep them in the server API but not expose them in the CLI please let me know. Or, if there's a way to define completely separate server API (without a matching client CLI) and client CLI (without a matching server API) that will work too. Will this always succeed? +# deactivate vault record in KRA +response = kra_client.keys.list_keys( +client_key_id, pki.key.KeyClient.KEY_STATUS_ACTIVE) Yes. If there's no active keys it will return an empty collection. +for key_info in response.key_infos: +kra_client.keys.modify_key_status( +key_info.get_key_id(), +pki.key.KeyClient.KEY_STATUS_INACTIVE) This loop will do nothing given an empty collection. If not, we might get into an inconsistent state, where the vault is deleted in LDAP but still active in KRA. (I'm not sure if this is actually a problem or not.) That can only happen if the server crashes after deleting the vault but before deactivating the key. Regardless, it will not be a problem because the key is identified by vault ID/path so it will not conflict with other vaults, and it will get overwritten if the same vault is recreated again. -- Endi S. Dewata From d1123f07745fea856ced305a814d933cd793dbf2 Mon Sep 17 00:00:00 2001 From: Endi S. Dewata edew...@redhat.com Date: Tue, 21 Oct 2014 10:57:08 -0400 Subject: [PATCH] Added vault-archive and vault-retrieve commands. New commands have been added to archive and retrieve data into and from a vault, also to retrieve the transport certificate.
Re: [Freeipa-devel] [PATCH] Password vault
On 5/28/2015 12:46 AM, Jan Cholasta wrote: On a related note, since KRA is optional, can we move the vaults container to cn=kra,cn=vaults? This is the convetion used by the other optional components (DNS and recently CA). I mean cn=vaults,cn=kra of course. If you are talking about the o=kra,PKI suffix, I'm not sure whether the IPA framework will work with it. If you are talking about adding a new cn=kra,IPA suffix entry on top of cn=vaults, what is the purpose of this entry? Is the entry going to be created/deleted automatically when the KRA is installed/removed? Is it going to be used for something else other than vaults? There are a lot of questions that need to be answered before we can make this change. We probably should revisit this issue after the core vault functionality is added. -- Endi S. Dewata -- 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] Password vault
Dne 27.5.2015 v 07:39 Jan Cholasta napsal(a):
Dne 27.5.2015 v 02:38 Endi Sukma Dewata napsal(a):
Please take a look at the attached patch to add vault-archive/retrieve
commands.
On 4/20/2015 1:12 AM, Jan Cholasta wrote:
16) You do way too much stuff in vault_add.forward(). Only code that
must be done on the client needs to be there, i.e. handling of the
data, text and in options.
The vault_archive call must be in vault_add.execute(), otherwise
a) we
will be making 2 RPC calls from the client and b) it won't be
called at
all when api.env.in_server is True.
This is done by design. The vault_add.forward() generates the salt
and
the keys. The vault_archive.forward() will encrypt the data. These
operations have to be done on the client side to secure the
transport of
the data from the client through the server and finally to KRA. This
mechanism prevents the server from looking at the unencrypted data.
OK, but that does not justify that it's broken in server-side API. It
can and should be done so that it works the same way on both client
and
server.
I think the best solution would be to split the command into two
commands, server-side vault_archive_raw to archive already encrypted
data, and client-side vault_archive to encrypt data and archive them
with vault_archive_raw in its .execute(). Same thing for
vault_retrieve.
Actually I think it's better to just merge the add and archive,
reducing
the number of RPC calls. The vault_archive now will support two
types of
operations:
(a) Archive data into a new vault (it will create the vault just before
archiving the data):
$ ipa vault-archive testvault --create --in data ...
(b) Archive data into an existing vault:
$ ipa vault-archive testvault --in data ...
The vault_add is now just a wrapper for the vault_archive(a).
If that's just an implementation detail, OK.
If it's possible to modify existing vault objects using vault-add or
create new objects using vault-archive, then NACK.
The vault-archive is now completely separate from vault-add. You can no
longer archive data while creating a vault:
$ ipa vault-add test
$ ipa vault-archive test --in secret.bin
OK.
BTW, I also think it would be better if there were 2 separate sets of
commands for binary and textual data
(vault_{archive,retrieve}_{data,text}) rather than trying to handle
everything in vault_{archive,retrieve}.
I don't think we want to provide a separate command of every possible
data type operation combination. Users would get confused. The
archive
retrieve commands should be able to handle all current future data
types with options.
A command with two sets of mutually exclusive options is really two
commands in disguise, which is a good sign it should be divided into two
actual commands.
Who are you to say users would get confused? I say they would be more
confused by a command with a plethora of mutually exclusive options.
What other possible data types are there?
The patch has been simplified to support only binary data. The data can
be specified using either of these options:
$ ipa vault-archive test --data base-64 encoded data
$ ipa vault-archive test --in input file
I don't think we want to provide two separate commands for these options
although they are mutually exclusive, do we?
No, these are not really 2 different options, but rather 2 forms of the
same option, which for the lack of better support for files in the
framework have to be done as 2 options.
The add archive combination was added for convenience, not for
optimization. This way you would be able to archive data into a new
vault using a single command. Without this, you'd have to execute two
separate commands: add archive, which will result in 2 RPC calls
anyway.
I think I would prefer if it was separate, as that would be consistent
with other plugins (e.g. for objects with members, we don't allow
adding
members directly in -add, you have to use -add-member after -add).
The vault data is similar to group description, not group members. When
creating a group we can supply the description. If not specified it
will
be blank. Archiving vault data is similar to updating the group
description.
It's similar to group members because there are separate commands to
manipulate them.
Just because there are separate commands doesn't mean vault data
(single-valued) is similar to group members (multi-valued). It uses
separate commands because of the encryption steps involved while
archiving/retrieving data.
That was not the point, but whatever.
You have to choose one of:
a) vault data is settable using vault-add and vault-mod and gettable
using vault-mod, vault-show and vault-find
b) vault data is settable using vault-archive and gettable using
vault-retrieve
Anything in between is not permitted.
As mentioned above, the add and archive commands are now separate.
21) vault_archive is not a retrieve operation, it should be based on
LDAPUpdate instead of
Re: [Freeipa-devel] [PATCH] Password vault
Dne 28.5.2015 v 07:43 Jan Cholasta napsal(a):
Dne 27.5.2015 v 07:39 Jan Cholasta napsal(a):
Dne 27.5.2015 v 02:38 Endi Sukma Dewata napsal(a):
Please take a look at the attached patch to add vault-archive/retrieve
commands.
On 4/20/2015 1:12 AM, Jan Cholasta wrote:
16) You do way too much stuff in vault_add.forward(). Only code
that
must be done on the client needs to be there, i.e. handling of the
data, text and in options.
The vault_archive call must be in vault_add.execute(), otherwise
a) we
will be making 2 RPC calls from the client and b) it won't be
called at
all when api.env.in_server is True.
This is done by design. The vault_add.forward() generates the salt
and
the keys. The vault_archive.forward() will encrypt the data. These
operations have to be done on the client side to secure the
transport of
the data from the client through the server and finally to KRA. This
mechanism prevents the server from looking at the unencrypted data.
OK, but that does not justify that it's broken in server-side API. It
can and should be done so that it works the same way on both client
and
server.
I think the best solution would be to split the command into two
commands, server-side vault_archive_raw to archive already encrypted
data, and client-side vault_archive to encrypt data and archive them
with vault_archive_raw in its .execute(). Same thing for
vault_retrieve.
Actually I think it's better to just merge the add and archive,
reducing
the number of RPC calls. The vault_archive now will support two
types of
operations:
(a) Archive data into a new vault (it will create the vault just
before
archiving the data):
$ ipa vault-archive testvault --create --in data ...
(b) Archive data into an existing vault:
$ ipa vault-archive testvault --in data ...
The vault_add is now just a wrapper for the vault_archive(a).
If that's just an implementation detail, OK.
If it's possible to modify existing vault objects using vault-add or
create new objects using vault-archive, then NACK.
The vault-archive is now completely separate from vault-add. You can no
longer archive data while creating a vault:
$ ipa vault-add test
$ ipa vault-archive test --in secret.bin
OK.
BTW, I also think it would be better if there were 2 separate sets of
commands for binary and textual data
(vault_{archive,retrieve}_{data,text}) rather than trying to handle
everything in vault_{archive,retrieve}.
I don't think we want to provide a separate command of every possible
data type operation combination. Users would get confused. The
archive
retrieve commands should be able to handle all current future data
types with options.
A command with two sets of mutually exclusive options is really two
commands in disguise, which is a good sign it should be divided into
two
actual commands.
Who are you to say users would get confused? I say they would be more
confused by a command with a plethora of mutually exclusive options.
What other possible data types are there?
The patch has been simplified to support only binary data. The data can
be specified using either of these options:
$ ipa vault-archive test --data base-64 encoded data
$ ipa vault-archive test --in input file
I don't think we want to provide two separate commands for these options
although they are mutually exclusive, do we?
No, these are not really 2 different options, but rather 2 forms of the
same option, which for the lack of better support for files in the
framework have to be done as 2 options.
The add archive combination was added for convenience, not for
optimization. This way you would be able to archive data into a new
vault using a single command. Without this, you'd have to execute
two
separate commands: add archive, which will result in 2 RPC calls
anyway.
I think I would prefer if it was separate, as that would be
consistent
with other plugins (e.g. for objects with members, we don't allow
adding
members directly in -add, you have to use -add-member after -add).
The vault data is similar to group description, not group members.
When
creating a group we can supply the description. If not specified it
will
be blank. Archiving vault data is similar to updating the group
description.
It's similar to group members because there are separate commands to
manipulate them.
Just because there are separate commands doesn't mean vault data
(single-valued) is similar to group members (multi-valued). It uses
separate commands because of the encryption steps involved while
archiving/retrieving data.
That was not the point, but whatever.
You have to choose one of:
a) vault data is settable using vault-add and vault-mod and gettable
using vault-mod, vault-show and vault-find
b) vault data is settable using vault-archive and gettable using
vault-retrieve
Anything in between is not permitted.
As mentioned above, the add and archive commands are now separate.
21) vault_archive is not a retrieve operation, it
Re: [Freeipa-devel] [PATCH] Password vault
Please take a look at the attached patch to add vault-archive/retrieve
commands.
On 4/20/2015 1:12 AM, Jan Cholasta wrote:
16) You do way too much stuff in vault_add.forward(). Only code that
must be done on the client needs to be there, i.e. handling of the
data, text and in options.
The vault_archive call must be in vault_add.execute(), otherwise a) we
will be making 2 RPC calls from the client and b) it won't be
called at
all when api.env.in_server is True.
This is done by design. The vault_add.forward() generates the salt and
the keys. The vault_archive.forward() will encrypt the data. These
operations have to be done on the client side to secure the
transport of
the data from the client through the server and finally to KRA. This
mechanism prevents the server from looking at the unencrypted data.
OK, but that does not justify that it's broken in server-side API. It
can and should be done so that it works the same way on both client and
server.
I think the best solution would be to split the command into two
commands, server-side vault_archive_raw to archive already encrypted
data, and client-side vault_archive to encrypt data and archive them
with vault_archive_raw in its .execute(). Same thing for vault_retrieve.
Actually I think it's better to just merge the add and archive, reducing
the number of RPC calls. The vault_archive now will support two types of
operations:
(a) Archive data into a new vault (it will create the vault just before
archiving the data):
$ ipa vault-archive testvault --create --in data ...
(b) Archive data into an existing vault:
$ ipa vault-archive testvault --in data ...
The vault_add is now just a wrapper for the vault_archive(a).
If that's just an implementation detail, OK.
If it's possible to modify existing vault objects using vault-add or
create new objects using vault-archive, then NACK.
The vault-archive is now completely separate from vault-add. You can no
longer archive data while creating a vault:
$ ipa vault-add test
$ ipa vault-archive test --in secret.bin
BTW, I also think it would be better if there were 2 separate sets of
commands for binary and textual data
(vault_{archive,retrieve}_{data,text}) rather than trying to handle
everything in vault_{archive,retrieve}.
I don't think we want to provide a separate command of every possible
data type operation combination. Users would get confused. The archive
retrieve commands should be able to handle all current future data
types with options.
A command with two sets of mutually exclusive options is really two
commands in disguise, which is a good sign it should be divided into two
actual commands.
Who are you to say users would get confused? I say they would be more
confused by a command with a plethora of mutually exclusive options.
What other possible data types are there?
The patch has been simplified to support only binary data. The data can
be specified using either of these options:
$ ipa vault-archive test --data base-64 encoded data
$ ipa vault-archive test --in input file
I don't think we want to provide two separate commands for these options
although they are mutually exclusive, do we?
The add archive combination was added for convenience, not for
optimization. This way you would be able to archive data into a new
vault using a single command. Without this, you'd have to execute two
separate commands: add archive, which will result in 2 RPC calls
anyway.
I think I would prefer if it was separate, as that would be consistent
with other plugins (e.g. for objects with members, we don't allow adding
members directly in -add, you have to use -add-member after -add).
The vault data is similar to group description, not group members. When
creating a group we can supply the description. If not specified it will
be blank. Archiving vault data is similar to updating the group
description.
It's similar to group members because there are separate commands to
manipulate them.
Just because there are separate commands doesn't mean vault data
(single-valued) is similar to group members (multi-valued). It uses
separate commands because of the encryption steps involved while
archiving/retrieving data.
You have to choose one of:
a) vault data is settable using vault-add and vault-mod and gettable
using vault-mod, vault-show and vault-find
b) vault data is settable using vault-archive and gettable using
vault-retrieve
Anything in between is not permitted.
As mentioned above, the add and archive commands are now separate.
21) vault_archive is not a retrieve operation, it should be based on
LDAPUpdate instead of LDAPRetrieve. Or Command actually, since it does
not do anything with LDAP. The same applies to vault_retrieve.
The vault_archive does not actually modify the LDAP entry because it
stores the data in KRA. It is actually an LDAPRetrieve operation
because
it needs to get the vault info before it can perform the archival
operation.
Re: [Freeipa-devel] [PATCH] Password vault
Dne 27.5.2015 v 02:38 Endi Sukma Dewata napsal(a):
Please take a look at the attached patch to add vault-archive/retrieve
commands.
On 4/20/2015 1:12 AM, Jan Cholasta wrote:
16) You do way too much stuff in vault_add.forward(). Only code that
must be done on the client needs to be there, i.e. handling of the
data, text and in options.
The vault_archive call must be in vault_add.execute(), otherwise
a) we
will be making 2 RPC calls from the client and b) it won't be
called at
all when api.env.in_server is True.
This is done by design. The vault_add.forward() generates the salt and
the keys. The vault_archive.forward() will encrypt the data. These
operations have to be done on the client side to secure the
transport of
the data from the client through the server and finally to KRA. This
mechanism prevents the server from looking at the unencrypted data.
OK, but that does not justify that it's broken in server-side API. It
can and should be done so that it works the same way on both client and
server.
I think the best solution would be to split the command into two
commands, server-side vault_archive_raw to archive already encrypted
data, and client-side vault_archive to encrypt data and archive them
with vault_archive_raw in its .execute(). Same thing for
vault_retrieve.
Actually I think it's better to just merge the add and archive, reducing
the number of RPC calls. The vault_archive now will support two types of
operations:
(a) Archive data into a new vault (it will create the vault just before
archiving the data):
$ ipa vault-archive testvault --create --in data ...
(b) Archive data into an existing vault:
$ ipa vault-archive testvault --in data ...
The vault_add is now just a wrapper for the vault_archive(a).
If that's just an implementation detail, OK.
If it's possible to modify existing vault objects using vault-add or
create new objects using vault-archive, then NACK.
The vault-archive is now completely separate from vault-add. You can no
longer archive data while creating a vault:
$ ipa vault-add test
$ ipa vault-archive test --in secret.bin
OK.
BTW, I also think it would be better if there were 2 separate sets of
commands for binary and textual data
(vault_{archive,retrieve}_{data,text}) rather than trying to handle
everything in vault_{archive,retrieve}.
I don't think we want to provide a separate command of every possible
data type operation combination. Users would get confused. The archive
retrieve commands should be able to handle all current future data
types with options.
A command with two sets of mutually exclusive options is really two
commands in disguise, which is a good sign it should be divided into two
actual commands.
Who are you to say users would get confused? I say they would be more
confused by a command with a plethora of mutually exclusive options.
What other possible data types are there?
The patch has been simplified to support only binary data. The data can
be specified using either of these options:
$ ipa vault-archive test --data base-64 encoded data
$ ipa vault-archive test --in input file
I don't think we want to provide two separate commands for these options
although they are mutually exclusive, do we?
No, these are not really 2 different options, but rather 2 forms of the
same option, which for the lack of better support for files in the
framework have to be done as 2 options.
The add archive combination was added for convenience, not for
optimization. This way you would be able to archive data into a new
vault using a single command. Without this, you'd have to execute two
separate commands: add archive, which will result in 2 RPC calls
anyway.
I think I would prefer if it was separate, as that would be consistent
with other plugins (e.g. for objects with members, we don't allow
adding
members directly in -add, you have to use -add-member after -add).
The vault data is similar to group description, not group members. When
creating a group we can supply the description. If not specified it will
be blank. Archiving vault data is similar to updating the group
description.
It's similar to group members because there are separate commands to
manipulate them.
Just because there are separate commands doesn't mean vault data
(single-valued) is similar to group members (multi-valued). It uses
separate commands because of the encryption steps involved while
archiving/retrieving data.
That was not the point, but whatever.
You have to choose one of:
a) vault data is settable using vault-add and vault-mod and gettable
using vault-mod, vault-show and vault-find
b) vault data is settable using vault-archive and gettable using
vault-retrieve
Anything in between is not permitted.
As mentioned above, the add and archive commands are now separate.
21) vault_archive is not a retrieve operation, it should be based on
LDAPUpdate instead of LDAPRetrieve. Or Command actually, since it
does
not
Re: [Freeipa-devel] [PATCH] Password vault
Dne 21.5.2015 v 17:45 Endi Sukma Dewata napsal(a):
Please take a look at the new patch.
On 5/20/2015 1:53 AM, Jan Cholasta wrote:
I suppose you meant you're OK with not adding host vaults now?
Yes.
The only way to know if the design will be future proof is if we have at
least some idea how it will be used. Without that there is no guarantee.
Host principals have this form: host/hostname@realm, so with the
current code they will be considered a service and will have a service
container.
Do you want to add a new cn=hosts container just for hosts? Unless we
have a specific reason (i.e. use case) I don't see a need to add
specific code for hosts now, or at least until we get the core vault
functionality working.
The reason is consistency. Private vaults should be available for all
identities, because anything else would be an arbitrary limitation
(which is not elegant). If private vaults were available for all
identities, we would need a container for host vaults. I'm not saying
the container has to be added now, but there should at least be a check
to reject requests when the authenticated identity is a host (i.e.
context.principal.startswith('host/')).
In the previous patch, since a host was considered a service it could
have private vaults too. But anyway, I added the code to reject host
principals as you requested.
When is the self.api actually initialized? Can we initialize the
container_dn (or base_dn as in the original code) attribute
immediately
after that?
Not yet, but this will be fixed in the future. (Also, container_dn is
part of the LDAPObject API, unlike base_dn used in the original code.)
Is there a ticket for this?
I don't think there is a ticket for this particular issue.
Added TODOs in the code.
This change is not included. The code will now obtain the values from
apilib.api.env at init time and store it in class attributes so it can
be reused.
container_dn = api.env.container_vault
base_dn = DN(container_dn, api.env.basedn)
Sorry, but no. Please just follow the best practice instead of
trying to
invent something new. This is not the right time and place to discuss
this. We should be discussing the vault, not framework idiosyncracies.
OK.
Thanks for understanding.
Changed the code as requested.
There is an obvious inefficiency here: all containers in the path
(including the built-in ones) will be re-added on every vault-add
operation.
I don't see anything wrong with recursions, especially if it works more
efficiently since only the immediate parent will be re-added.
I tend to stick to the don't use recursion in production code rule,
epsecially in simple cases like this one.
I think the rule is kind of misguided. Recursion might be considered bad
if it goes very deep thus consuming so much stack space, which is not
the case here given the short and flat tree topology. Or, if it's
unnecessary such as a tail-recursion, which is not the case here either.
So for example, with the loop every time you add a private vault you're
guaranteed to have three failed LDAP Add operations whereas with the
recursion there's only one failed operation.
This is not about loop vs. recursion, this is about the used approach.
Your code can be easily transformed into a loop while keeping the same
approach:
entries = []
while dn:
assert dn.endswith(self.base_dn)
rdn = dn[0]
entry = self.backend.make_entry(
dn,
{
'objectclass': ['nsContainer'],
'cn': rdn['cn'],
})
# if entry can be added, return
try:
self.backend.add_entry(entry)
break
except errors.NotFound:
pass
# otherwise, create parent entry first
dn = DN(*dn[1:])
entries.insert(0, entry)
for entry in entries:
# then create the entry itself again
self.backend.add_entry(entry)
Do you still want to use the loop?
Yes please.
This algorithm is recursive by nature (start at the bottom, but add the
parent first). The above code basically emulates recursion with two
loops and an explicit stack of entries, but the memory requirement is
pretty much the same because it's still using a stack. With real
recursion there is no loops and the stack is implicit, so the code is at
least cleaner.
I thought you cared about efficiency in the first place, given our
discussion about container_dn. The looped variant is faster, even when
it has 2 for loops, and consumes less memory, because of the function
call overhead in the recursive variant.
If you really want to avoid recursion you probably should use an RDN
iterator instead of a stack, but the code would be either even uglier or
less efficient.
Anyway, it's not a big deal, I included this change.
Thanks, LGTM.
Pushed to master: fde21adcbd62b9a300740d9ba237ca9e89a905e4
--
Jan Cholasta
--
Manage your subscription for the
Re: [Freeipa-devel] [PATCH] Password vault
Dne 19.5.2015 v 16:40 Endi Sukma Dewata napsal(a):
Before I send another patch I have some questions below.
On 5/19/2015 3:27 AM, Jan Cholasta wrote:
I changed the 'host vaults' to become 'service vaults'. The interface
will look like this:
$ ipa vault-find --service HTTP/server.example.com
$ ipa vault-add test --service HTTP/server.example.com
I also added user vaults:
$ ipa vault-find --user testuser
$ ipa vault-add test --user testuser
Private vaults is a special case of user vaults where username=you.
Host vaults can be added later once we define the use case.
OK.
I suppose you meant you're OK with not adding host vaults now?
Yes.
4. The following code in get_dn() is incorrect:
principal = getattr(context, 'principal')
(name, realm) = split_principal(principal)
name = name.split('/')
if len(name) == 1:
container_dn = DN(('cn', 'users'), container_dn)
else:
container_dn = DN(('cn', 'services'), container_dn)
container_dn = DN(('cn', name[-1]), container_dn)
A service does not have a private container like users
(cn=username,
cn=users, cn=vaults). The entry cn=name, cn=host, cn=services,
cn=vaults is a service vault, not a container. The service vault is
used
by the admin to provide a secret for a service.
I'm not sure what the behavior should be if a service is executing a
vault command that uses a private container such as:
$ ipa vault-add test
Maybe it should just generate an error.
Users, hosts and services are all user-like objects, is there a reason
not to support private vaults for all of them?
As mentioned above, it's not required in the design doc, but we can add
it if there's a clear use case. I agree that at least for now we can
change the service vault into a service container to store multiple
service's private vaults.
I don't really care about having a clear use case, I would prefer if the
design was elegant enough to handle *all* the cases without any extra
effort.
The only way to know if the design will be future proof is if we have at
least some idea how it will be used. Without that there is no guarantee.
Host principals have this form: host/hostname@realm, so with the
current code they will be considered a service and will have a service
container.
Do you want to add a new cn=hosts container just for hosts? Unless we
have a specific reason (i.e. use case) I don't see a need to add
specific code for hosts now, or at least until we get the core vault
functionality working.
The reason is consistency. Private vaults should be available for all
identities, because anything else would be an arbitrary limitation
(which is not elegant). If private vaults were available for all
identities, we would need a container for host vaults. I'm not saying
the container has to be added now, but there should at least be a check
to reject requests when the authenticated identity is a host (i.e.
context.principal.startswith('host/')).
5. In create_container() why do you need to reconstruct the
container_dn
on each invocation even though the value is fixed?
container_dn = DN(self.container_dn, self.api.env.basedn)
Because self.api may not necessarily be the same as ipalib.api.
Under what scenario would that be a problem?
When someone uses the plugin with a different API object than ipalib.api.
The original code seems to
be working fine with ipalib.api.
The current best practice is to use self.api and *all* new plugin code
should do that.
If it is a problem, why do we still use ipalib.api to initialize
container_dn vault class attribute?
container_dn = api.env.container_vault
Then in get_dn() we basically construct the container_dn variable with
values from both self.api and ipalib.api:
container_dn = DN(self.container_dn, self.api.env.basedn)
When is the self.api actually initialized? Can we initialize the
container_dn (or base_dn as in the original code) attribute immediately
after that?
Not yet, but this will be fixed in the future. (Also, container_dn is
part of the LDAPObject API, unlike base_dn used in the original code.)
Is there a ticket for this?
I don't think there is a ticket for this particular issue.
This change is not included. The code will now obtain the values from
apilib.api.env at init time and store it in class attributes so it can
be reused.
container_dn = api.env.container_vault
base_dn = DN(container_dn, api.env.basedn)
Sorry, but no. Please just follow the best practice instead of trying to
invent something new. This is not the right time and place to discuss
this. We should be discussing the vault, not framework idiosyncracies.
OK.
Thanks for understanding.
6. The loop in create_container() is incorrect. Suppose we're
creating a
container cn=A, cn=B, suffix and the parent container cn=B,
suffix
doesn't exist yet. The first add_entry() invocation will fail as
expected, but instead of adding the parent entry the whole method
will
fail.
Right,
Re: [Freeipa-devel] [PATCH] Password vault
Dne 18.5.2015 v 21:17 Endi Sukma Dewata napsal(a):
Please take a look at the attached new patch which includes some of your
changes you proposed.
On 5/14/2015 7:17 PM, Endi Sukma Dewata wrote:
On 5/14/2015 1:42 PM, Jan Cholasta wrote:
Question: Services in IPA are identified by Kerberos principal. Why
are
service vaults identified by hostname alone?
The service vaults are actually identified by the hostname and service
name assuming the principal is in this format: name/host@realm.
The vault is stored in cn=name, cn=host, cn=services, cn=vaults,
suffix. When accessing a vault service you are supposed to specify
the
name and the host (except for vault-find which will return all services
in the same host):
$ ipa vault-find --host host
$ ipa vault-add name --host host
Are there other service principal formats that we need to support?
Do we
need to support services of different realms?
Well, users are identified by username (uid attribute), hosts by
hostname (fqdn attribute) and services by principal name
(krbprincipalname attribute). Each of them has separate container and is
also uniquely identified by principal name. I guess it would make sense
to follow that for vaults as well, in which case service vaults should
be called host vaults (because they are created in a host-specific
container, not a service-specific one) and maybe real service vaults
should be added.
There's no plan for host vaults in the design doc, but it's not
impossible to add it in the future. What is the use case?
I suppose we can change the service vault into a container, and possibly
add generic user vaults too (in addition to private user vaults), the
interface will look like this:
$ ipa vault-add PrivateVault
$ ipa vault-add ServiceVault --service name/hostname
$ ipa vault-add SharedVault --shared
$ ipa vault-add UsersVault --user username
$ ipa vault-add HostVault --host hostname
Basically we'll need a new parameter for each new container. For the
initial implementation we only need the first 2 or 3.
I changed the 'host vaults' to become 'service vaults'. The interface
will look like this:
$ ipa vault-find --service HTTP/server.example.com
$ ipa vault-add test --service HTTP/server.example.com
I also added user vaults:
$ ipa vault-find --user testuser
$ ipa vault-add test --user testuser
Private vaults is a special case of user vaults where username=you.
Host vaults can be added later once we define the use case.
OK.
1. The following code in get_dn() is causing problems for vault-find.
dn = super(vault, self).get_dn(*keys, **options)
rdn = dn[0]
container_dn = DN(*dn[1:])
The super.get_dn() will return cn=vaults, suffix, so the rdn will
become cn=vaults and container_dn will become suffix. When combined
with the subcontainer (private/shared/service) it will produce an
invalid DN.
Right, fixed.
I should have tested the patch before posting it, my bad, sorry.
OK.
This change has been included in this patch.
OK.
2. Not sure if it'related to #1, the vault-find is failing.
$ ipa vault-find
ipa: ERROR: an internal error has occurred
The error_log shows TypeError: 'NoneType' object is not iterable
Fixed. It was caused by missing return value in vault_find.exc_callback.
We can actually ignore all NotFound errors since now all containers are
either created automatically or already created by default.
if call_func.__name__ == 'find_entries':
if isinstance(exc, errors.NotFound):
shared = options.get('shared')
# if private or service container has not been created, ignore
if not shared:
raise errors.EmptyResult(reason=str(exc))
The original code was only ignoring NotFound errors on user vaults
because previously only the user containers was supposed to be created
automatically, and there wasn't a plan to provide service container.
This change has been included and it will ignore all NotFound errors.
OK.
3. The --shared option in vault-find is now requiring an argument even
though it's a Flag.
$ ipa vault-find --shared
Usage: ipa [global-options] vault-find [CRITERIA] [options]
ipa: error: --shared option requires an argument
Fixed.
OK.
Not sure which code you changed, but the new patch doesn't exhibit this
problem.
I changed Flag('shared') to Flag('shared?').
4. The following code in get_dn() is incorrect:
principal = getattr(context, 'principal')
(name, realm) = split_principal(principal)
name = name.split('/')
if len(name) == 1:
container_dn = DN(('cn', 'users'), container_dn)
else:
container_dn = DN(('cn', 'services'), container_dn)
container_dn = DN(('cn', name[-1]), container_dn)
A service does not have a private container like users (cn=username,
cn=users, cn=vaults). The entry cn=name, cn=host, cn=services,
cn=vaults is a service vault, not a container. The service vault is
used
by the admin to provide a secret for a service.
I'm not sure what the behavior should
Re: [Freeipa-devel] [PATCH] Password vault
Before I send another patch I have some questions below.
On 5/19/2015 3:27 AM, Jan Cholasta wrote:
I changed the 'host vaults' to become 'service vaults'. The interface
will look like this:
$ ipa vault-find --service HTTP/server.example.com
$ ipa vault-add test --service HTTP/server.example.com
I also added user vaults:
$ ipa vault-find --user testuser
$ ipa vault-add test --user testuser
Private vaults is a special case of user vaults where username=you.
Host vaults can be added later once we define the use case.
OK.
I suppose you meant you're OK with not adding host vaults now?
4. The following code in get_dn() is incorrect:
principal = getattr(context, 'principal')
(name, realm) = split_principal(principal)
name = name.split('/')
if len(name) == 1:
container_dn = DN(('cn', 'users'), container_dn)
else:
container_dn = DN(('cn', 'services'), container_dn)
container_dn = DN(('cn', name[-1]), container_dn)
A service does not have a private container like users (cn=username,
cn=users, cn=vaults). The entry cn=name, cn=host, cn=services,
cn=vaults is a service vault, not a container. The service vault is
used
by the admin to provide a secret for a service.
I'm not sure what the behavior should be if a service is executing a
vault command that uses a private container such as:
$ ipa vault-add test
Maybe it should just generate an error.
Users, hosts and services are all user-like objects, is there a reason
not to support private vaults for all of them?
As mentioned above, it's not required in the design doc, but we can add
it if there's a clear use case. I agree that at least for now we can
change the service vault into a service container to store multiple
service's private vaults.
I don't really care about having a clear use case, I would prefer if the
design was elegant enough to handle *all* the cases without any extra
effort.
The only way to know if the design will be future proof is if we have at
least some idea how it will be used. Without that there is no guarantee.
Host principals have this form: host/hostname@realm, so with the
current code they will be considered a service and will have a service
container.
Do you want to add a new cn=hosts container just for hosts? Unless we
have a specific reason (i.e. use case) I don't see a need to add
specific code for hosts now, or at least until we get the core vault
functionality working.
5. In create_container() why do you need to reconstruct the
container_dn
on each invocation even though the value is fixed?
container_dn = DN(self.container_dn, self.api.env.basedn)
Because self.api may not necessarily be the same as ipalib.api.
Under what scenario would that be a problem?
When someone uses the plugin with a different API object than ipalib.api.
The original code seems to
be working fine with ipalib.api.
The current best practice is to use self.api and *all* new plugin code
should do that.
If it is a problem, why do we still use ipalib.api to initialize
container_dn vault class attribute?
container_dn = api.env.container_vault
Then in get_dn() we basically construct the container_dn variable with
values from both self.api and ipalib.api:
container_dn = DN(self.container_dn, self.api.env.basedn)
When is the self.api actually initialized? Can we initialize the
container_dn (or base_dn as in the original code) attribute immediately
after that?
Not yet, but this will be fixed in the future. (Also, container_dn is
part of the LDAPObject API, unlike base_dn used in the original code.)
Is there a ticket for this?
This change is not included. The code will now obtain the values from
apilib.api.env at init time and store it in class attributes so it can
be reused.
container_dn = api.env.container_vault
base_dn = DN(container_dn, api.env.basedn)
Sorry, but no. Please just follow the best practice instead of trying to
invent something new. This is not the right time and place to discuss
this. We should be discussing the vault, not framework idiosyncracies.
OK.
6. The loop in create_container() is incorrect. Suppose we're
creating a
container cn=A, cn=B, suffix and the parent container cn=B, suffix
doesn't exist yet. The first add_entry() invocation will fail as
expected, but instead of adding the parent entry the whole method will
fail.
Right, fixed.
It's still not working. The new code is trying to add cn=vaults first,
and it stops because it already exists, but the intermediary entries are
still not added. Also the DN displayed in the message misleading:
$ ipa vault-add test
ipa: ERROR: container entry (cn=vaults) not found
$ ipa vault-add test --host server.example.com
ipa: ERROR: container entry (cn=vaults) not found
The unit tests are failing because of this.
I forgot to remove the break statement in the for loop.
This change is not included. The original code and the tests work just
fine.
I would prefer if it was done
Re: [Freeipa-devel] [PATCH] Password vault
Please take a look at the attached new patch which includes some of your
changes you proposed.
On 5/14/2015 7:17 PM, Endi Sukma Dewata wrote:
On 5/14/2015 1:42 PM, Jan Cholasta wrote:
Question: Services in IPA are identified by Kerberos principal. Why are
service vaults identified by hostname alone?
The service vaults are actually identified by the hostname and service
name assuming the principal is in this format: name/host@realm.
The vault is stored in cn=name, cn=host, cn=services, cn=vaults,
suffix. When accessing a vault service you are supposed to specify the
name and the host (except for vault-find which will return all services
in the same host):
$ ipa vault-find --host host
$ ipa vault-add name --host host
Are there other service principal formats that we need to support? Do we
need to support services of different realms?
Well, users are identified by username (uid attribute), hosts by
hostname (fqdn attribute) and services by principal name
(krbprincipalname attribute). Each of them has separate container and is
also uniquely identified by principal name. I guess it would make sense
to follow that for vaults as well, in which case service vaults should
be called host vaults (because they are created in a host-specific
container, not a service-specific one) and maybe real service vaults
should be added.
There's no plan for host vaults in the design doc, but it's not
impossible to add it in the future. What is the use case?
I suppose we can change the service vault into a container, and possibly
add generic user vaults too (in addition to private user vaults), the
interface will look like this:
$ ipa vault-add PrivateVault
$ ipa vault-add ServiceVault --service name/hostname
$ ipa vault-add SharedVault --shared
$ ipa vault-add UsersVault --user username
$ ipa vault-add HostVault --host hostname
Basically we'll need a new parameter for each new container. For the
initial implementation we only need the first 2 or 3.
I changed the 'host vaults' to become 'service vaults'. The interface
will look like this:
$ ipa vault-find --service HTTP/server.example.com
$ ipa vault-add test --service HTTP/server.example.com
I also added user vaults:
$ ipa vault-find --user testuser
$ ipa vault-add test --user testuser
Private vaults is a special case of user vaults where username=you.
Host vaults can be added later once we define the use case.
1. The following code in get_dn() is causing problems for vault-find.
dn = super(vault, self).get_dn(*keys, **options)
rdn = dn[0]
container_dn = DN(*dn[1:])
The super.get_dn() will return cn=vaults, suffix, so the rdn will
become cn=vaults and container_dn will become suffix. When combined
with the subcontainer (private/shared/service) it will produce an
invalid DN.
Right, fixed.
I should have tested the patch before posting it, my bad, sorry.
OK.
This change has been included in this patch.
2. Not sure if it'related to #1, the vault-find is failing.
$ ipa vault-find
ipa: ERROR: an internal error has occurred
The error_log shows TypeError: 'NoneType' object is not iterable
Fixed. It was caused by missing return value in vault_find.exc_callback.
We can actually ignore all NotFound errors since now all containers are
either created automatically or already created by default.
if call_func.__name__ == 'find_entries':
if isinstance(exc, errors.NotFound):
shared = options.get('shared')
# if private or service container has not been created, ignore
if not shared:
raise errors.EmptyResult(reason=str(exc))
The original code was only ignoring NotFound errors on user vaults
because previously only the user containers was supposed to be created
automatically, and there wasn't a plan to provide service container.
This change has been included and it will ignore all NotFound errors.
3. The --shared option in vault-find is now requiring an argument even
though it's a Flag.
$ ipa vault-find --shared
Usage: ipa [global-options] vault-find [CRITERIA] [options]
ipa: error: --shared option requires an argument
Fixed.
OK.
Not sure which code you changed, but the new patch doesn't exhibit this
problem.
4. The following code in get_dn() is incorrect:
principal = getattr(context, 'principal')
(name, realm) = split_principal(principal)
name = name.split('/')
if len(name) == 1:
container_dn = DN(('cn', 'users'), container_dn)
else:
container_dn = DN(('cn', 'services'), container_dn)
container_dn = DN(('cn', name[-1]), container_dn)
A service does not have a private container like users (cn=username,
cn=users, cn=vaults). The entry cn=name, cn=host, cn=services,
cn=vaults is a service vault, not a container. The service vault is used
by the admin to provide a secret for a service.
I'm not sure what the behavior should be if a service is executing a
vault command that uses a private container such as:
$ ipa vault-add test
Maybe it
Re: [Freeipa-devel] [PATCH] Password vault
Dne 14.5.2015 v 05:01 Endi Sukma Dewata napsal(a):
On 5/13/2015 4:09 AM, Jan Cholasta wrote:
Dne 12.5.2015 v 12:52 Endi Sukma Dewata napsal(a):
Please take a look at the attached patch (#353-9). It obsoletes all
previous patches. See comments below.
On 4/20/2015 1:12 AM, Jan Cholasta wrote:
I'm planning to merge the vault and vault container object and use the
vault type attribute to distinguish between the two. See more
discussion
about that below.
OK.
The vault container plugin has been removed instead of merged (see
explanation below). Internally the vaults are still stored in built-in
containers in the DS, but there won't be an interface to manage them.
The following containers are available for use: private, shared, and
services, but they are flat, not hierarchical.
To speed up the review, I have amended your patch with code and coding
style fixes (attached), please review my changes.
Question: Services in IPA are identified by Kerberos principal. Why are
service vaults identified by hostname alone?
The service vaults are actually identified by the hostname and service
name assuming the principal is in this format: name/host@realm.
The vault is stored in cn=name, cn=host, cn=services, cn=vaults,
suffix. When accessing a vault service you are supposed to specify the
name and the host (except for vault-find which will return all services
in the same host):
$ ipa vault-find --host host
$ ipa vault-add name --host host
Are there other service principal formats that we need to support? Do we
need to support services of different realms?
Well, users are identified by username (uid attribute), hosts by
hostname (fqdn attribute) and services by principal name
(krbprincipalname attribute). Each of them has separate container and is
also uniquely identified by principal name. I guess it would make sense
to follow that for vaults as well, in which case service vaults should
be called host vaults (because they are created in a host-specific
container, not a service-specific one) and maybe real service vaults
should be added.
Do we
need to support services of different realms?
That depends. Do we want to support vaults for users and services from
AD trusts?
Some comments about your changes:
1. The following code in get_dn() is causing problems for vault-find.
dn = super(vault, self).get_dn(*keys, **options)
rdn = dn[0]
container_dn = DN(*dn[1:])
The super.get_dn() will return cn=vaults, suffix, so the rdn will
become cn=vaults and container_dn will become suffix. When combined
with the subcontainer (private/shared/service) it will produce an
invalid DN.
Right, fixed.
I should have tested the patch before posting it, my bad, sorry.
2. Not sure if it'related to #1, the vault-find is failing.
$ ipa vault-find
ipa: ERROR: an internal error has occurred
The error_log shows TypeError: 'NoneType' object is not iterable
Fixed. It was caused by missing return value in vault_find.exc_callback.
3. The --shared option in vault-find is now requiring an argument even
though it's a Flag.
$ ipa vault-find --shared
Usage: ipa [global-options] vault-find [CRITERIA] [options]
ipa: error: --shared option requires an argument
Fixed.
4. The following code in get_dn() is incorrect:
principal = getattr(context, 'principal')
(name, realm) = split_principal(principal)
name = name.split('/')
if len(name) == 1:
container_dn = DN(('cn', 'users'), container_dn)
else:
container_dn = DN(('cn', 'services'), container_dn)
container_dn = DN(('cn', name[-1]), container_dn)
A service does not have a private container like users (cn=username,
cn=users, cn=vaults). The entry cn=name, cn=host, cn=services,
cn=vaults is a service vault, not a container. The service vault is used
by the admin to provide a secret for a service.
I'm not sure what the behavior should be if a service is executing a
vault command that uses a private container such as:
$ ipa vault-add test
Maybe it should just generate an error.
Users, hosts and services are all user-like objects, is there a reason
not to support private vaults for all of them?
5. In create_container() why do you need to reconstruct the container_dn
on each invocation even though the value is fixed?
container_dn = DN(self.container_dn, self.api.env.basedn)
Because self.api may not necessarily be the same as ipalib.api.
6. The loop in create_container() is incorrect. Suppose we're creating a
container cn=A, cn=B, suffix and the parent container cn=B, suffix
doesn't exist yet. The first add_entry() invocation will fail as
expected, but instead of adding the parent entry the whole method will
fail.
while dn.endswith(container_dn):
entry = self.backend.make_entry(
dn,
objectclass=['nsContainer'],
cn=[dn[0]['cn']],
)
try:
self.backend.add_entry(entry)
except errors.DuplicateEntry:
break
dn =
Re: [Freeipa-devel] [PATCH] Password vault
On 5/14/2015 1:42 PM, Jan Cholasta wrote:
Question: Services in IPA are identified by Kerberos principal. Why are
service vaults identified by hostname alone?
The service vaults are actually identified by the hostname and service
name assuming the principal is in this format: name/host@realm.
The vault is stored in cn=name, cn=host, cn=services, cn=vaults,
suffix. When accessing a vault service you are supposed to specify the
name and the host (except for vault-find which will return all services
in the same host):
$ ipa vault-find --host host
$ ipa vault-add name --host host
Are there other service principal formats that we need to support? Do we
need to support services of different realms?
Well, users are identified by username (uid attribute), hosts by
hostname (fqdn attribute) and services by principal name
(krbprincipalname attribute). Each of them has separate container and is
also uniquely identified by principal name. I guess it would make sense
to follow that for vaults as well, in which case service vaults should
be called host vaults (because they are created in a host-specific
container, not a service-specific one) and maybe real service vaults
should be added.
There's no plan for host vaults in the design doc, but it's not
impossible to add it in the future. What is the use case?
I suppose we can change the service vault into a container, and possibly
add generic user vaults too (in addition to private user vaults), the
interface will look like this:
$ ipa vault-add PrivateVault
$ ipa vault-add ServiceVault --service name/hostname
$ ipa vault-add SharedVault --shared
$ ipa vault-add UsersVault --user username
$ ipa vault-add HostVault --host hostname
Basically we'll need a new parameter for each new container. For the
initial implementation we only need the first 2 or 3.
Do we
need to support services of different realms?
That depends. Do we want to support vaults for users and services from
AD trusts?
It wasn't mentioned in the design doc either, but probably in the future
we can support external vaults too:
$ ipa vault-add ExternalVault --principal principal
cn=vaults
+ cn=external
+ cn=name/hostname@realm
+ cn=vault 1
+ cn=vault 2
+ cn=user@realm
+ cn=vault 3
+ cn=vault 4
1. The following code in get_dn() is causing problems for vault-find.
dn = super(vault, self).get_dn(*keys, **options)
rdn = dn[0]
container_dn = DN(*dn[1:])
The super.get_dn() will return cn=vaults, suffix, so the rdn will
become cn=vaults and container_dn will become suffix. When combined
with the subcontainer (private/shared/service) it will produce an
invalid DN.
Right, fixed.
I should have tested the patch before posting it, my bad, sorry.
OK.
2. Not sure if it'related to #1, the vault-find is failing.
$ ipa vault-find
ipa: ERROR: an internal error has occurred
The error_log shows TypeError: 'NoneType' object is not iterable
Fixed. It was caused by missing return value in vault_find.exc_callback.
We can actually ignore all NotFound errors since now all containers are
either created automatically or already created by default.
if call_func.__name__ == 'find_entries':
if isinstance(exc, errors.NotFound):
shared = options.get('shared')
# if private or service container has not been created, ignore
if not shared:
raise errors.EmptyResult(reason=str(exc))
The original code was only ignoring NotFound errors on user vaults
because previously only the user containers was supposed to be created
automatically, and there wasn't a plan to provide service container.
3. The --shared option in vault-find is now requiring an argument even
though it's a Flag.
$ ipa vault-find --shared
Usage: ipa [global-options] vault-find [CRITERIA] [options]
ipa: error: --shared option requires an argument
Fixed.
OK.
4. The following code in get_dn() is incorrect:
principal = getattr(context, 'principal')
(name, realm) = split_principal(principal)
name = name.split('/')
if len(name) == 1:
container_dn = DN(('cn', 'users'), container_dn)
else:
container_dn = DN(('cn', 'services'), container_dn)
container_dn = DN(('cn', name[-1]), container_dn)
A service does not have a private container like users (cn=username,
cn=users, cn=vaults). The entry cn=name, cn=host, cn=services,
cn=vaults is a service vault, not a container. The service vault is used
by the admin to provide a secret for a service.
I'm not sure what the behavior should be if a service is executing a
vault command that uses a private container such as:
$ ipa vault-add test
Maybe it should just generate an error.
Users, hosts and services are all user-like objects, is there a reason
not to support private vaults for all of them?
As mentioned above, it's not required in the design doc, but we can add
it if there's a clear use case. I agree that at least for now we can
change the service
Re: [Freeipa-devel] [PATCH] Password vault
Dne 12.5.2015 v 12:52 Endi Sukma Dewata napsal(a):
Please take a look at the attached patch (#353-9). It obsoletes all
previous patches. See comments below.
On 4/20/2015 1:12 AM, Jan Cholasta wrote:
I'm planning to merge the vault and vault container object and use the
vault type attribute to distinguish between the two. See more discussion
about that below.
OK.
The vault container plugin has been removed instead of merged (see
explanation below). Internally the vaults are still stored in built-in
containers in the DS, but there won't be an interface to manage them.
The following containers are available for use: private, shared, and
services, but they are flat, not hierarchical.
To speed up the review, I have amended your patch with code and coding
style fixes (attached), please review my changes.
Question: Services in IPA are identified by Kerberos principal. Why are
service vaults identified by hostname alone?
--
Jan Cholasta
From 3996a1519b8408e751e0f1424eb4e2e69fda9ff6 Mon Sep 17 00:00:00 2001
From: Endi S. Dewata edew...@redhat.com
Date: Tue, 21 Oct 2014 10:57:08 -0400
Subject: [PATCH] Added vault plugin.
A new plugin has been added to manage vaults. Test scripts have
also been added to verify the functionality.
https://fedorahosted.org/freeipa/ticket/3872
---
API.txt | 69 ++
VERSION | 4 +-
install/share/60basev3.ldif | 1 +
install/updates/40-vault.update | 19 ++
install/updates/Makefile.am | 1 +
ipa-client/man/default.conf.5 | 1 +
ipalib/constants.py | 1 +
ipalib/plugins/vault.py | 260 +++
ipatests/test_xmlrpc/test_vault_plugin.py | 338 ++
9 files changed, 692 insertions(+), 2 deletions(-)
create mode 100644 install/updates/40-vault.update
create mode 100644 ipalib/plugins/vault.py
create mode 100644 ipatests/test_xmlrpc/test_vault_plugin.py
diff --git a/API.txt b/API.txt
index 346e35f..1ad73e4 100644
--- a/API.txt
+++ b/API.txt
@@ -4563,6 +4563,75 @@ option: Str('version?', exclude='webui')
output: Output('result', type 'bool', None)
output: Output('summary', (type 'unicode', type 'NoneType'), None)
output: PrimaryKey('value', None, None)
+command: vault_add
+args: 1,8,3
+arg: Str('cn', attribute=True, cli_name='vault_name', maxlength=255, multivalue=False, pattern='^[a-zA-Z0-9_.-]+$', primary_key=True, required=True)
+option: Str('addattr*', cli_name='addattr', exclude='webui')
+option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
+option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=False)
+option: Str('host?')
+option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
+option: Str('setattr*', cli_name='setattr', exclude='webui')
+option: Flag('shared?', autofill=True, default=False)
+option: Str('version?', exclude='webui')
+output: Entry('result', type 'dict', Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
+output: Output('summary', (type 'unicode', type 'NoneType'), None)
+output: PrimaryKey('value', None, None)
+command: vault_del
+args: 1,4,3
+arg: Str('cn', attribute=True, cli_name='vault_name', maxlength=255, multivalue=True, pattern='^[a-zA-Z0-9_.-]+$', primary_key=True, query=True, required=True)
+option: Flag('continue', autofill=True, cli_name='continue', default=False)
+option: Str('host?')
+option: Flag('shared?', autofill=True, default=False)
+option: Str('version?', exclude='webui')
+output: Output('result', type 'dict', None)
+output: Output('summary', (type 'unicode', type 'NoneType'), None)
+output: ListOfPrimaryKeys('value', None, None)
+command: vault_find
+args: 1,10,4
+arg: Str('criteria?', noextrawhitespace=False)
+option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
+option: Str('cn', attribute=True, autofill=False, cli_name='vault_name', maxlength=255, multivalue=False, pattern='^[a-zA-Z0-9_.-]+$', primary_key=True, query=True, required=False)
+option: Str('description', attribute=True, autofill=False, cli_name='desc', multivalue=False, query=True, required=False)
+option: Str('host?')
+option: Flag('pkey_only?', autofill=True, default=False)
+option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
+option: Flag('shared?', autofill=True, default=False)
+option: Int('sizelimit?', autofill=False, minvalue=0)
+option: Int('timelimit?', autofill=False, minvalue=0)
+option: Str('version?', exclude='webui')
+output: Output('count', type 'int', None)
+output: ListOfEntries('result', (type 'list', type 'tuple'), Gettext('A list of LDAP entries', domain='ipa', localedir=None))
+output: Output('summary', (type 'unicode', type 'NoneType'), None)
+output: Output('truncated', type 'bool', None)
+command: vault_mod
+args: 1,10,3
+arg: Str('cn',
Re: [Freeipa-devel] [PATCH] Password vault
On 5/13/2015 4:09 AM, Jan Cholasta wrote:
Dne 12.5.2015 v 12:52 Endi Sukma Dewata napsal(a):
Please take a look at the attached patch (#353-9). It obsoletes all
previous patches. See comments below.
On 4/20/2015 1:12 AM, Jan Cholasta wrote:
I'm planning to merge the vault and vault container object and use the
vault type attribute to distinguish between the two. See more
discussion
about that below.
OK.
The vault container plugin has been removed instead of merged (see
explanation below). Internally the vaults are still stored in built-in
containers in the DS, but there won't be an interface to manage them.
The following containers are available for use: private, shared, and
services, but they are flat, not hierarchical.
To speed up the review, I have amended your patch with code and coding
style fixes (attached), please review my changes.
Question: Services in IPA are identified by Kerberos principal. Why are
service vaults identified by hostname alone?
The service vaults are actually identified by the hostname and service
name assuming the principal is in this format: name/host@realm.
The vault is stored in cn=name, cn=host, cn=services, cn=vaults,
suffix. When accessing a vault service you are supposed to specify the
name and the host (except for vault-find which will return all services
in the same host):
$ ipa vault-find --host host
$ ipa vault-add name --host host
Are there other service principal formats that we need to support? Do we
need to support services of different realms?
Some comments about your changes:
1. The following code in get_dn() is causing problems for vault-find.
dn = super(vault, self).get_dn(*keys, **options)
rdn = dn[0]
container_dn = DN(*dn[1:])
The super.get_dn() will return cn=vaults, suffix, so the rdn will
become cn=vaults and container_dn will become suffix. When combined
with the subcontainer (private/shared/service) it will produce an
invalid DN.
2. Not sure if it'related to #1, the vault-find is failing.
$ ipa vault-find
ipa: ERROR: an internal error has occurred
The error_log shows TypeError: 'NoneType' object is not iterable
3. The --shared option in vault-find is now requiring an argument even
though it's a Flag.
$ ipa vault-find --shared
Usage: ipa [global-options] vault-find [CRITERIA] [options]
ipa: error: --shared option requires an argument
4. The following code in get_dn() is incorrect:
principal = getattr(context, 'principal')
(name, realm) = split_principal(principal)
name = name.split('/')
if len(name) == 1:
container_dn = DN(('cn', 'users'), container_dn)
else:
container_dn = DN(('cn', 'services'), container_dn)
container_dn = DN(('cn', name[-1]), container_dn)
A service does not have a private container like users (cn=username,
cn=users, cn=vaults). The entry cn=name, cn=host, cn=services,
cn=vaults is a service vault, not a container. The service vault is used
by the admin to provide a secret for a service.
I'm not sure what the behavior should be if a service is executing a
vault command that uses a private container such as:
$ ipa vault-add test
Maybe it should just generate an error.
5. In create_container() why do you need to reconstruct the container_dn
on each invocation even though the value is fixed?
container_dn = DN(self.container_dn, self.api.env.basedn)
6. The loop in create_container() is incorrect. Suppose we're creating a
container cn=A, cn=B, suffix and the parent container cn=B, suffix
doesn't exist yet. The first add_entry() invocation will fail as
expected, but instead of adding the parent entry the whole method will fail.
while dn.endswith(container_dn):
entry = self.backend.make_entry(
dn,
objectclass=['nsContainer'],
cn=[dn[0]['cn']],
)
try:
self.backend.add_entry(entry)
except errors.DuplicateEntry:
break
dn = DN(*dn[1:])
7. The host and shared parameters are no longer available in vault-show
and vault-del commands. The unit tests are failing because of that. Why
do these commands not automatically inherit all parameters from the class?
--
Endi S. Dewata
--
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] Password vault
Please take a look at the attached patch (#353-9). It obsoletes all previous patches. See comments below. On 4/20/2015 1:12 AM, Jan Cholasta wrote: I'm planning to merge the vault and vault container object and use the vault type attribute to distinguish between the two. See more discussion about that below. OK. The vault container plugin has been removed instead of merged (see explanation below). Internally the vaults are still stored in built-in containers in the DS, but there won't be an interface to manage them. The following containers are available for use: private, shared, and services, but they are flat, not hierarchical. 3) The container_vault config option should be renamed to container_vaultcontainer, as it is used in the vaultcontainer plugin, not the vault plugin. It was named container_vault because it defines the DN for of the subtree that contains all vault-related entries. I moved the base_dn variable from vaultcontainer object to the vault object for clarity. That does not make much sense to me. Vault objects are contained in their respective vaultcontainer objects, not directly in cn=vaults. The cn=vaults itself is actually a vault container (i.e. ipaVaultContainer). Theoretically you could store a vault in any container including cn=vaults, but we just don't want people to use it that way. I think this is consistent with other plugins. For example, the container_user points to cn=users, which is an nsContainer. There is no concept of 'user container' other than the cn=users itself. But even if there is one, the container_user will still be stored in the user class. In fact it is not consistent with other plugins. All entries managed by the user plugin are stored *directly* under cn=users. Entries managed by the vault plugin are not stored directly under cn=vaults, but rather anywhere in the cn=vaults subtree and their DN is derived from the DN of the parent vault container. For such objects, we don't set plugin.container_dn and don't have container_plugin constant, but rather define them as child objects of their container objects. When the vault vaultcontainer is merged, this will no longer be an issue. OK. The vaults are still stored in the cn=vaults subtree, but now the containers will use nsContainer instead of ipaVaultContainer. The container_vault variable still defines the root container DN, i.e. cn=vaults. 4) The vault object should be child of the vaultcontainer object. Not only is this correct from the object model perspective, but it would also make all the container_id hacks go away. It's a bit difficult because it will affect how the container vault ID's are represented on the CLI. Yes, but the API should be done right (without hacks) first. You can tune the CLI after that if you want. I think the current framework is rather limiting. It's kind of hard to build an interface that looks exactly what you want then add the implementation later to match the interface because many things are interrelated. In this particular case the object hierarchy on the server side would affect how the vault ID will be represented on the client side. It indeed is limiting and that's a good thing. We don't want people to be able to create any crazy interfaces they can imagine, inconsistent with everything else in IPA. So the vault container plugin was removed because the current framework cannot support the hierarchical structure described in the vault design without overriding the default parameter handling (which was referred to as 'hacks', although it was actually suggested by the previous reviewer). Adding the missing functionality will require modifications to the base framework classes. Such changes should only be done after thoroughly evaluating the impact on the existing plugins, probably in a future release. In the design the container ID would be a single value like this: $ ipa vault-add /services/server.example.com/HTTP And if the vault ID is relative (without initial slash), it will be appended to the user's private container (i.e. /users/username/): $ ipa vault-add PrivateVault The implementation is not complete yet. Currently it accepts this format: $ ipa vault-add vault name [--container container ID] and I'm still planning to add this: $ ipa vault-add vault ID This is actually now done in the latest patch. Internally the ID is still split into name parent ID. If the vault must be a child of vaultcontainer, and the vaultcontainer must be a child of a vaultcontainer, does it mean the vault ID would have to be split into separate arguments like this? $ ipa vaultcontainer-add services server.example.com HTTP If that's the case we'd lose the ability to specify a relative vault ID. Yes, that's the case. But I don't think relative IDs should be a problem, we can do this: $ ipa vaultcontainer-add a b c # absolute $ ipa vaultcontainer-add . c# relative I think a . will be
Re: [Freeipa-devel] [PATCH] Password vault
Dne 3.4.2015 v 05:37 Endi Sukma Dewata napsal(a): Hi, Attached are new patches replacing all old ones. Please take a look at them. They should applied in this order: 365, 353-8, 355-6, 357-3, 359-2, 360-1, 364-1, 361-1. Thanks for squashing patches 362-364 into the original patches, it's much more digestible this way. I'm planning to merge the vault and vault container object and use the vault type attribute to distinguish between the two. See more discussion about that below. OK. 3) The container_vault config option should be renamed to container_vaultcontainer, as it is used in the vaultcontainer plugin, not the vault plugin. It was named container_vault because it defines the DN for of the subtree that contains all vault-related entries. I moved the base_dn variable from vaultcontainer object to the vault object for clarity. That does not make much sense to me. Vault objects are contained in their respective vaultcontainer objects, not directly in cn=vaults. The cn=vaults itself is actually a vault container (i.e. ipaVaultContainer). Theoretically you could store a vault in any container including cn=vaults, but we just don't want people to use it that way. I think this is consistent with other plugins. For example, the container_user points to cn=users, which is an nsContainer. There is no concept of 'user container' other than the cn=users itself. But even if there is one, the container_user will still be stored in the user class. In fact it is not consistent with other plugins. All entries managed by the user plugin are stored *directly* under cn=users. Entries managed by the vault plugin are not stored directly under cn=vaults, but rather anywhere in the cn=vaults subtree and their DN is derived from the DN of the parent vault container. For such objects, we don't set plugin.container_dn and don't have container_plugin constant, but rather define them as child objects of their container objects. When the vault vaultcontainer is merged, this will no longer be an issue. OK. 4) The vault object should be child of the vaultcontainer object. Not only is this correct from the object model perspective, but it would also make all the container_id hacks go away. It's a bit difficult because it will affect how the container vault ID's are represented on the CLI. Yes, but the API should be done right (without hacks) first. You can tune the CLI after that if you want. I think the current framework is rather limiting. It's kind of hard to build an interface that looks exactly what you want then add the implementation later to match the interface because many things are interrelated. In this particular case the object hierarchy on the server side would affect how the vault ID will be represented on the client side. It indeed is limiting and that's a good thing. We don't want people to be able to create any crazy interfaces they can imagine, inconsistent with everything else in IPA. In the design the container ID would be a single value like this: $ ipa vault-add /services/server.example.com/HTTP And if the vault ID is relative (without initial slash), it will be appended to the user's private container (i.e. /users/username/): $ ipa vault-add PrivateVault The implementation is not complete yet. Currently it accepts this format: $ ipa vault-add vault name [--container container ID] and I'm still planning to add this: $ ipa vault-add vault ID This is actually now done in the latest patch. Internally the ID is still split into name parent ID. If the vault must be a child of vaultcontainer, and the vaultcontainer must be a child of a vaultcontainer, does it mean the vault ID would have to be split into separate arguments like this? $ ipa vaultcontainer-add services server.example.com HTTP If that's the case we'd lose the ability to specify a relative vault ID. Yes, that's the case. But I don't think relative IDs should be a problem, we can do this: $ ipa vaultcontainer-add a b c # absolute $ ipa vaultcontainer-add . c# relative I think a . will be confusing because there's no concept of current vaultcontainer like current directory. or this: $ ipa vaultcontainer-add '' a b c # absolute $ ipa vaultcontainer-add c # relative An empty string is also confusing and can be problematic to distinguish with missing argument. I didn't mean empty string specifically, it could have been any special value. or this: $ ipa vaultcontainer-add a b c # absolute $ ipa vaultcontainer-add c --relative # relative or this: $ ipa vaultcontainer-add a b c --absolute # absolute $ ipa vaultcontainer-add c # relative Per discussion in the IPA-CS meeting, we'd rather keep the / for vault ID delimiters because the spaces will be confusing to users, but we'll not use absolute ID anymore. I'm sorry if I gave you the impression that this is up for discussion,
Re: [Freeipa-devel] [PATCH] Password vault
Dne 11.3.2015 v 15:12 Endi Sukma Dewata napsal(a):
Thanks for the review. New patch attached to be applied on top of all
previous patches. Please see comments below.
Thanks. I have replied to some of your comments below.
On 3/6/2015 3:53 PM, Jan Cholasta wrote:
Patch 353:
1) Please follow PEP8 in new code.
The pep8 tool reports these errors in existing files:
./ipalib/constants.py:98:80: E501 line too long (84 79 characters)
./ipalib/plugins/baseldap.py:1527:80: E501 line too long (81 79
characters)
./ipalib/plugins/user.py:915:80: E501 line too long (80 79 characters)
as well as many errors in the files this patch adds.
For some reason pylint keeps crashing during build so I cannot run it
for all files. I'm fixing the errors that I can see. If you see other
errors please let me know while I'm still trying to figure out the problem.
Well, I did not use pylint, but pep8:
http://pep8.readthedocs.org/en/latest/
Is there an existing ticket for fixing PEP8 errors? Let's use that for
fixing the errors in the existing code.
There is no ticket, but we still follow PEP8 in new code, so please do
that. It shouldn't be too hard.
...
3) The container_vault config option should be renamed to
container_vaultcontainer, as it is used in the vaultcontainer plugin,
not the vault plugin.
It was named container_vault because it defines the DN for of the
subtree that contains all vault-related entries. I moved the base_dn
variable from vaultcontainer object to the vault object for clarity.
That does not make much sense to me. Vault objects are contained in
their respective vaultcontainer objects, not directly in cn=vaults.
4) The vault object should be child of the vaultcontainer object.
Not only is this correct from the object model perspective, but it would
also make all the container_id hacks go away.
It's a bit difficult because it will affect how the container vault
ID's are represented on the CLI.
Yes, but the API should be done right (without hacks) first. You can
tune the CLI after that if you want.
In the design the container ID would be a single value like this:
$ ipa vault-add /services/server.example.com/HTTP
And if the vault ID is relative (without initial slash), it will be
appended to the user's private container (i.e. /users/username/):
$ ipa vault-add PrivateVault
The implementation is not complete yet. Currently it accepts this format:
$ ipa vault-add vault name [--container container ID]
and I'm still planning to add this:
$ ipa vault-add vault ID
If the vault must be a child of vaultcontainer, and the vaultcontainer
must be a child of a vaultcontainer, does it mean the vault ID would
have to be split into separate arguments like this?
$ ipa vaultcontainer-add services server.example.com HTTP
If that's the case we'd lose the ability to specify a relative vault ID.
Yes, that's the case.
But I don't think relative IDs should be a problem, we can do this:
$ ipa vaultcontainer-add a b c # absolute
$ ipa vaultcontainer-add . c# relative
or this:
$ ipa vaultcontainer-add '' a b c # absolute
$ ipa vaultcontainer-add c # relative
or this:
$ ipa vaultcontainer-add a b c # absolute
$ ipa vaultcontainer-add c --relative # relative
or this:
$ ipa vaultcontainer-add a b c --absolute # absolute
$ ipa vaultcontainer-add c # relative
...
11) No clever optimizations like this please:
+# vault DN cannot be the container base DN
+if len(dn) == len(api.Object.vaultcontainer.base_dn):
+raise ValueError('Invalid vault DN: %s' % dn)
Compare the DNs by value instead.
Actually the DN values have already been compared in the code right
above it:
# make sure the DN is a vault DN
if not dn.endswith(self.api.Object.vaultcontainer.base_dn):
raise ValueError('Invalid vault DN: %s' % dn)
This code confirms that the incoming vault DN is within the vault
subtree. After that, the DN length comparison above is just to make sure
the incoming vault DN is not the root of the vault subtree itself. It
doesn't need to compare the values again.
I see. You can combine both of the checks into one:
if not dn.endswith(self.api.Object.vaultcontainer.base_dn, 1):
raise ValueError(...)
...
14) Use File instead of Str for input files:
+Str('in?',
+cli_name='in',
+doc=_('File containing data to archive'),
+),
The File type doesn't work with binary files because it tries to decode
the content.
OK. I know File is broken and plan to fix it in the future. Just add a
comment saying that it should be a File, but it's broken, OK?
...
16) You do way too much stuff in vault_add.forward(). Only code that
must be done on the client needs to be there, i.e. handling of the
data, text and in options.
The vault_archive call must be in vault_add.execute(), otherwise a) we
will be making 2
Re: [Freeipa-devel] [PATCH] Password vault
On 3/13/2015 2:27 AM, Endi Sukma Dewata wrote:
On 3/11/2015 9:12 PM, Endi Sukma Dewata wrote:
Thanks for the review. New patch attached to be applied on top of all
previous patches. Please see comments below.
New patch #362-1 attached replacing #362. It fixed some issues in
handle_not_found().
New patch #363 attached. It adds supports for vault vaultcontainer ID
parameter.
--
Endi S. Dewata
From e1d2a3a62e6d16c1c9b19f4cb19b900427ea5e1f Mon Sep 17 00:00:00 2001
From: Endi S. Dewata edew...@redhat.com
Date: Thu, 12 Mar 2015 09:21:02 -0400
Subject: [PATCH] Vault ID improvements.
The vault plugin has been modified to accept a single vault ID
in addition to separate name and parent ID attributes. The vault
container has also been modified in the same way. New test cases
have been added to verify this functionality.
https://fedorahosted.org/freeipa/ticket/3872
---
ipalib/plugins/vault.py| 143 +--
ipalib/plugins/vaultcontainer.py | 137 +--
ipalib/plugins/vaultsecret.py | 10 +-
ipatests/test_xmlrpc/test_vault_plugin.py | 151 +
ipatests/test_xmlrpc/test_vaultcontainer_plugin.py | 90 ++--
ipatests/test_xmlrpc/test_vaultsecret_plugin.py| 115
6 files changed, 565 insertions(+), 81 deletions(-)
diff --git a/ipalib/plugins/vault.py b/ipalib/plugins/vault.py
index
d47067758186601365e5924f5d13c7ab51ba66e5..38693d0710e000695cae21fb4db5dfb4c85b5c74
100644
--- a/ipalib/plugins/vault.py
+++ b/ipalib/plugins/vault.py
@@ -61,7 +61,7 @@ EXAMPLES:
ipa vault-find
) + _(
List shared vaults:
- ipa vault-find --container-id /shared
+ ipa vault-find /shared
) + _(
Add a standard vault:
ipa vault-add MyVault
@@ -171,8 +171,8 @@ class vault(LDAPObject):
cli_name='vault_name',
label=_('Vault name'),
primary_key=True,
-pattern='^[a-zA-Z0-9_.-]+$',
-pattern_errmsg='may only include letters, numbers, _, ., and -',
+pattern='^[a-zA-Z0-9_.-/]+$',
+pattern_errmsg='may only include letters, numbers, _, ., -, and /',
maxlength=255,
),
Str('vault_id?',
@@ -217,7 +217,7 @@ class vault(LDAPObject):
# get vault ID from parameters
name = keys[-1]
-container_id =
self.api.Object.vaultcontainer.normalize_id(options.get('container_id'))
+container_id =
self.api.Object.vaultcontainer.absolute_id(options.get('container_id'))
vault_id = container_id + name
dn = self.base_dn
@@ -250,6 +250,81 @@ class vault(LDAPObject):
return id
+def split_id(self, id):
+
+Splits a vault ID into (vault name, container ID) tuple.
+
+
+if not id:
+return (None, None)
+
+# split ID into container ID and vault name
+parts = id.rsplit(u'/', 1)
+
+if len(parts) == 2:
+vault_name = parts[1]
+container_id = u'%s/' % parts[0]
+
+else:
+vault_name = parts[0]
+container_id = None
+
+if not vault_name:
+vault_name = None
+
+return (vault_name, container_id)
+
+def merge_id(self, vault_name, container_id):
+
+Merges a vault name and a container ID into a vault ID.
+
+
+if not vault_name:
+id = container_id
+
+elif vault_name.startswith('/') or not container_id:
+id = vault_name
+
+else:
+id = container_id + vault_name
+
+return id
+
+def normalize_params(self, *args, **options):
+
+Normalizes the vault ID in the parameters.
+
+
+vault_id = self.parse_params(*args, **options)
+(vault_name, container_id) = self.split_id(vault_id)
+return self.update_params(vault_name, container_id, *args, **options)
+
+def parse_params(self, *args, **options):
+
+Extracts the vault name and container ID in the parameters.
+
+
+# get vault name and container ID from parameters
+vault_name = args[0]
+if type(vault_name) is tuple:
+vault_name = vault_name[0]
+container_id =
self.api.Object.vaultcontainer.normalize_id(options.get('container_id'))
+
+return self.merge_id(vault_name, container_id)
+
+def update_params(self, new_vault_name, new_container_id, *args,
**options):
+
+Stores vault name and container ID back into the parameters.
+
+
+args_list = list(args)
+args_list[0] = new_vault_name
+args = tuple(args_list)
+
+options['container_id'] = new_container_id
+
+return (args, options)
+
def get_kra_id(self, id):
Generates a client key ID to store/retrieve data in KRA.
@@ -363,10 +438,14 @@ class vault_add(LDAPCreate):
msg_summary = _('Added
Re: [Freeipa-devel] [PATCH] Password vault
Thanks for the review. New patch attached to be applied on top of all
previous patches. Please see comments below.
On 3/6/2015 3:53 PM, Jan Cholasta wrote:
Patch 353:
1) Please follow PEP8 in new code.
The pep8 tool reports these errors in existing files:
./ipalib/constants.py:98:80: E501 line too long (84 79 characters)
./ipalib/plugins/baseldap.py:1527:80: E501 line too long (81 79
characters)
./ipalib/plugins/user.py:915:80: E501 line too long (80 79 characters)
as well as many errors in the files this patch adds.
For some reason pylint keeps crashing during build so I cannot run it
for all files. I'm fixing the errors that I can see. If you see other
errors please let me know while I'm still trying to figure out the problem.
Is there an existing ticket for fixing PEP8 errors? Let's use that for
fixing the errors in the existing code.
2) Pylint reports the following error:
ipatests/test_xmlrpc/test_vault_plugin.py:153:
[E0102(function-redefined), test_vault] class already defined line 27)
Fixed.
3) The container_vault config option should be renamed to
container_vaultcontainer, as it is used in the vaultcontainer plugin,
not the vault plugin.
It was named container_vault because it defines the DN for of the
subtree that contains all vault-related entries. I moved the base_dn
variable from vaultcontainer object to the vault object for clarity.
4) The vault object should be child of the vaultcontainer object.
Not only is this correct from the object model perspective, but it would
also make all the container_id hacks go away.
It's a bit difficult because it will affect how the container vault
ID's are represented on the CLI.
In the design the container ID would be a single value like this:
$ ipa vault-add /services/server.example.com/HTTP
And if the vault ID is relative (without initial slash), it will be
appended to the user's private container (i.e. /users/username/):
$ ipa vault-add PrivateVault
The implementation is not complete yet. Currently it accepts this format:
$ ipa vault-add vault name [--container container ID]
and I'm still planning to add this:
$ ipa vault-add vault ID
If the vault must be a child of vaultcontainer, and the vaultcontainer
must be a child of a vaultcontainer, does it mean the vault ID would
have to be split into separate arguments like this?
$ ipa vaultcontainer-add services server.example.com HTTP
If that's the case we'd lose the ability to specify a relative vault ID.
5) When specifying param flags, use set literals.
This is especially wrong, because it's not a tuple, but a string in
parentheses:
+flags=('virtual_attribute'),
Fixed.
6) The `container` param of vault should actually be an option in
vault_* commands.
Also it should be renamed to `container_id`, for consistency with
vaultcontainer.
Fixed. It was actually made to be consistent with the 'parent' attribute
in the vaultcontainer class. Now the 'parent' has been renamed to
'parent_id' as well.
7) The `vault_id` param of vault should have no_option in flags, since
it is output-only.
Fixed.
8) Don't translate docstrings where not necessary:
+def get_dn(self, *keys, **options):
+__doc__ = _(
+Generates vault DN from vault ID.
+)
Only plugin modules and classes should have translated docstrings.
Fixed.
9) This looks wrong in vault.get_dn() and vaultcontainer.get_dn():
+name = None
+if keys:
+name = keys[0]
Primary key of the object should always be set, so the if statement
should not be there.
Fixed.
Also, primary key of any given object is always last in keys, so use
keys[-1] instead of keys[0].
Fixed.
10) Use self.api instead of api to access the API in plugins.
Fixed.
11) No clever optimizations like this please:
+# vault DN cannot be the container base DN
+if len(dn) == len(api.Object.vaultcontainer.base_dn):
+raise ValueError('Invalid vault DN: %s' % dn)
Compare the DNs by value instead.
Actually the DN values have already been compared in the code right
above it:
# make sure the DN is a vault DN
if not dn.endswith(self.api.Object.vaultcontainer.base_dn):
raise ValueError('Invalid vault DN: %s' % dn)
This code confirms that the incoming vault DN is within the vault
subtree. After that, the DN length comparison above is just to make sure
the incoming vault DN is not the root of the vault subtree itself. It
doesn't need to compare the values again.
12) vault.split_id() is not used anywhere.
Removed.
13) Bytes is not base64-encoded data:
+Bytes('data?',
+cli_name='data',
+doc=_('Base-64 encoded binary data to archive'),
+),
It is base64-encoded in the CLI, but on the API level it is not. The doc
should say just Binary data to archive.
Fixed.
14) Use File instead of Str for input files:
+Str('in?',
+cli_name='in',
+
Re: [Freeipa-devel] [PATCH] Password vault
On 3/11/2015 9:12 PM, Endi Sukma Dewata wrote:
Thanks for the review. New patch attached to be applied on top of all
previous patches. Please see comments below.
New patch #362-1 attached replacing #362. It fixed some issues in
handle_not_found().
--
Endi S. Dewata
From 6741138b647427cd3448ff72369dfc6fa21354aa Mon Sep 17 00:00:00 2001
From: Endi S. Dewata edew...@redhat.com
Date: Mon, 9 Mar 2015 12:09:20 -0400
Subject: [PATCH] Vault improvements.
The vault plugins have been modified to clean up the code, to fix
some issues, and to improve error handling. The LDAPCreate and
LDAPSearch classes have been refactored to allow subclasses to
provide custom error handling. The test scripts have been updated
accordingly.
https://fedorahosted.org/freeipa/ticket/3872
---
API.txt| 50 ++--
ipalib/plugins/baseldap.py | 35 +--
ipalib/plugins/user.py | 6 +-
ipalib/plugins/vault.py| 273 ++---
ipalib/plugins/vaultcontainer.py | 226 +
ipalib/plugins/vaultsecret.py | 108
ipatests/test_xmlrpc/test_vault_plugin.py | 2 +-
ipatests/test_xmlrpc/test_vaultcontainer_plugin.py | 24 +-
ipatests/test_xmlrpc/test_vaultsecret_plugin.py| 2 +-
9 files changed, 371 insertions(+), 355 deletions(-)
diff --git a/API.txt b/API.txt
index
ffbffa78cde372d5c7027b758be58bf07caebbc6..3a741755ab3e15e0175599a16a090b04d46d6be8
100644
--- a/API.txt
+++ b/API.txt
@@ -4518,7 +4518,7 @@ args: 1,20,3
arg: Str('cn', attribute=True, cli_name='vault_name', maxlength=255,
multivalue=False, pattern='^[a-zA-Z0-9_.-]+$', primary_key=True, required=True)
option: Str('addattr*', cli_name='addattr', exclude='webui')
option: Flag('all', autofill=True, cli_name='all', default=False,
exclude='webui')
-option: Str('container', attribute=False, cli_name='container',
multivalue=False, pattern='^[a-zA-Z0-9_.-/]+$', required=False)
+option: Str('container_id?', cli_name='container_id')
option: Bytes('data?', cli_name='data')
option: Str('description', attribute=True, cli_name='desc', multivalue=False,
required=False)
option: Str('escrow_public_key_file?', cli_name='escrow_public_key_file')
@@ -4543,7 +4543,7 @@ command: vault_add_member
args: 1,7,3
arg: Str('cn', attribute=True, cli_name='vault_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('container?', cli_name='container')
+option: Str('container_id?', cli_name='container_id')
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')
@@ -4556,7 +4556,7 @@ command: vault_add_owner
args: 1,7,3
arg: Str('cn', attribute=True, cli_name='vault_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('container?', cli_name='container')
+option: Str('container_id?', cli_name='container_id')
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')
@@ -4569,7 +4569,7 @@ command: vault_archive
args: 1,15,3
arg: Str('cn', attribute=True, cli_name='vault_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('container?', cli_name='container')
+option: Str('container_id?', cli_name='container_id')
option: Bytes('data?', cli_name='data')
option: Bytes('encryption_key?', cli_name='encryption_key')
option: Str('in?', cli_name='in')
@@ -4589,7 +4589,7 @@ output: PrimaryKey('value', None, None)
command: vault_del
args: 1,3,3
arg: Str('cn', attribute=True, cli_name='vault_name', maxlength=255,
multivalue=True, pattern='^[a-zA-Z0-9_.-]+$', primary_key=True, query=True,
required=True)
-option: Str('container?', cli_name='container')
+option: Str('container_id?', cli_name='container_id')
option: Flag('continue', autofill=True, cli_name='continue', default=False)
option: Str('version?', exclude='webui')
output: Output('result', type 'dict', None)
@@ -4600,7 +4600,7 @@ args: 1,15,4
arg: Str('criteria?', noextrawhitespace=False)
option: Flag('all', autofill=True, cli_name='all', default=False,
exclude='webui')
option: Str('cn', attribute=True, autofill=False, cli_name='vault_name',
maxlength=255, multivalue=False, pattern='^[a-zA-Z0-9_.-]+$', primary_key=True,
Re: [Freeipa-devel] [PATCH] Password vault
Hi Endi,
Dne 24.2.2015 v 04:09 Endi Sukma Dewata napsal(a):
On 2/16/2015 2:50 AM, Endi Sukma Dewata wrote:
Hi,
Attached are the updated patches for the password vault, and some new
ones (please disregard previous patch submissions). Please give them a
try. Thanks.
New patches attached replacing all previous vault patches. They include
the new escrow functionality, changes to the asymmetric vault, and some
cleanups. Thanks.
Patch 353:
1) Please follow PEP8 in new code.
The pep8 tool reports these errors in existing files:
./ipalib/constants.py:98:80: E501 line too long (84 79 characters)
./ipalib/plugins/baseldap.py:1527:80: E501 line too long (81 79
characters)
./ipalib/plugins/user.py:915:80: E501 line too long (80 79 characters)
as well as many errors in the files this patch adds.
2) Pylint reports the following error:
ipatests/test_xmlrpc/test_vault_plugin.py:153:
[E0102(function-redefined), test_vault] class already defined line 27)
3) The container_vault config option should be renamed to
container_vaultcontainer, as it is used in the vaultcontainer plugin,
not the vault plugin.
4) The vault object should be child of the vaultcontainer object.
Not only is this correct from the object model perspective, but it would
also make all the container_id hacks go away.
5) When specifying param flags, use set literals.
This is especially wrong, because it's not a tuple, but a string in
parentheses:
+flags=('virtual_attribute'),
6) The `container` param of vault should actually be an option in
vault_* commands.
Also it should be renamed to `container_id`, for consistency with
vaultcontainer.
7) The `vault_id` param of vault should have no_option in flags, since
it is output-only.
8) Don't translate docstrings where not necessary:
+def get_dn(self, *keys, **options):
+__doc__ = _(
+Generates vault DN from vault ID.
+)
Only plugin modules and classes should have translated docstrings.
9) This looks wrong in vault.get_dn() and vaultcontainer.get_dn():
+name = None
+if keys:
+name = keys[0]
Primary key of the object should always be set, so the if statement
should not be there.
Also, primary key of any given object is always last in keys, so use
keys[-1] instead of keys[0].
10) Use self.api instead of api to access the API in plugins.
11) No clever optimizations like this please:
+# vault DN cannot be the container base DN
+if len(dn) == len(api.Object.vaultcontainer.base_dn):
+raise ValueError('Invalid vault DN: %s' % dn)
Compare the DNs by value instead.
12) vault.split_id() is not used anywhere.
13) Bytes is not base64-encoded data:
+Bytes('data?',
+cli_name='data',
+doc=_('Base-64 encoded binary data to archive'),
+),
It is base64-encoded in the CLI, but on the API level it is not. The doc
should say just Binary data to archive.
14) Use File instead of Str for input files:
+Str('in?',
+cli_name='in',
+doc=_('File containing data to archive'),
+),
15) Use MutuallyExclusiveError instead of ValidationError when there are
mutually exclusive options specified.
16) You do way too much stuff in vault_add.forward(). Only code that
must be done on the client needs to be there, i.e. handling of the
data, text and in options.
The vault_archive call must be in vault_add.execute(), otherwise a) we
will be making 2 RPC calls from the client and b) it won't be called at
all when api.env.in_server is True.
17) Why are vaultcontainer objects automatically created in vault_add?
If you have to automatically create them, you also have to automatically
delete them when the command fails. But that's a hassle, so I would just
not create them automatically.
18) Why are vaultcontainer objects automatically created in vault_find?
This is just plain wrong and has to be removed, now.
19) What is the reason behind all the json stuff in vault_transport_cert?
vault_transport_cert.__json__() is exactly the same as
Command.__json__() and hence redundant.
20) Are vault_transport_cert, vault_archive and vault_retrieve supposed
to be runnable by users? If not, add NO_CLI = True to the class
definition.
21) vault_archive is not a retrieve operation, it should be based on
LDAPUpdate instead of LDAPRetrieve. Or Command actually, since it does
not do anything with LDAP. The same applies to vault_retrieve.
22) vault_archive will break with binary data that is not UTF-8 encoded
text.
This is where it occurs:
+vault_data[u'data'] = unicode(data)
Generally, don't use unicode() on str values and str() on unicode values
directly, always use .decode() and .encode().
23) Since vault containers are nested, the vaultcontainer object should
be child of itself.
There is no support for nested objects in the framework, but it
shouldn't be too hard to do
