[Freeipa-devel] [PATCH] Password vault
Endi Sukma Dewata
edewata at redhat.com
Fri Jul 3 12:23:42 UTC 2015
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 everything for you.
>>
>> Nobody is expecting baseldap to do KRA operations.
>>
>>> As the
>>> name implies, it is LDAP specific, if you want something else, you have
>>> to implement it yourself.
>>
>> In the previous patch vault_retrieve inherits from LDAPRetrieve so it
>> can rely on baseldap to retrieve the vault entry, then on top of that it
>> implements an additional KRA operations (without baseldap obviously). If
>> that is not allowed, aren't you basically saying LDAP plugin can only
>> access LDAP data?
>
> Yes, basically, but I'm also saying that you are not limited to doing
> LDAP plugins only.
I think this logic is flawed. Suppose later we add a code to remove
user's vaults when the user is deleted, does it mean the user_del can no
longer inherit from LDAPDelete?
> You can abuse the callbacks to do anything, including data retrieval
> from other sources, but it doesn't make it right, as it only leads to
> code duplication, inconsistencies and weird bugs. I have seen too much
> of this, hence my reluctance to do it again.
I don't think extending the base class to perform additional
functionalities can be generalized as 'abuse' or 'hack' or called
'semantically wrong'. Sometimes it is the right solution. Sometimes if
the framework is so limiting that the only solution is to extend
uncommon methods, it's called a 'workaround'. If there is code
duplication we should find a way to refactor it. What's considered
inconsistencies are very subjective. Weird bugs are case specific, it
cannot be generalized.
>>>> * a command name will match the object name (i.e. must use
>>>> vaultdata_mod
>>>> instead of a more intuitive vault_archive)
>>>
>>> I don't see how consistency is a bad thing, or how this could limit
>>> anyone doing things cleanly. I do agree that vaultdata_mod is ugly, but
>>> it's not the only way to achieve the same goal.
>>
>> Look at it from user's perspective. If you create a vault using
>> vault-add <vault name>, then archive data using vaultdata-mod <vault
>> name>, how is this consistent?
>
> Because it's object-verb and not object-verbofsomeotherobject. (Also I
> already acknowledged the vaultdata idea is ugly.)
In that case, strictly speaking, vault-mod will violate that rule too
because you're modifying an attribute, not the object itself like
vault-add or vault-del. From user's perspective the secret 'data' is
just another attribute in the vault. So similarly, vault-archive is
modifying the 'data' attribute in the vault.
The fact that the 'data' is stored in KRA rather than in IPA is just
implementation details. If we have to expose this distinction to the
user, that's a problem with the framework.
Also, if you're willing to use vault-archive rather than vaultdata-mod,
that means the rule is irrelevant. Consistency should be viewed from
user's perspective first, then developer's perspective later (if
possible at all).
>>>> We know that some use cases do not fit these assumptions. Rather than
>>>> compromising the use case, or looking at workarounds as hacks, I'd
>>>> suggest finding ideas to improve the framework itself to be more
>>>> accommodating.
>>>
>>> I would personally love to improve the framework (it's just retarded
>>> sometimes as you may have noticed), but it does not have high priority
>>> right now (not my decision).
>>
>> We don't have to modify the current framework right now, but we can
>> align new codes that don't fit the current framework to match the future
>> framework. Although the future framework is not defined yet, some things
>> are already clear, for example there should be separate client and
>> server APIs. So if a command like vault_add has differing client and
>> server options, regardless how insignificant it is, there's no reason to
>> force it to be combined. The current framework doesn't prevent
>> separation anyway.
>
> Aligning new code is exactly what I'm aiming to do and why I want people
> to look at their APIs from an object oriented perspective rather than
> just dumb RPC, because that's the direction the framework is heading.
Again, user's perspective first, developer's perspective later, and with
the right CLI, client API, and server API separation.
--
Endi S. Dewata
-------------- next part --------------
>From bf83dfd7d1f140eb26cc3f7a9c265a0d3743fbbb Mon Sep 17 00:00:00 2001
From: "Endi S. Dewata" <edewata at redhat.com>
Date: Thu, 2 Jul 2015 15:27:16 -0400
Subject: [PATCH] Added ipaVaultPublicKey attribute.
A new attribute ipaVaultPublicKey has been added to replace the
existing ipaPublicKey used to store the vault public key.
https://fedorahosted.org/freeipa/ticket/3872
---
API.txt | 6 +++---
VERSION | 4 ++--
install/share/60basev3.ldif | 3 ++-
ipalib/plugins/vault.py | 16 ++++++++--------
ipatests/test_xmlrpc/test_vault_plugin.py | 4 ++--
5 files changed, 17 insertions(+), 16 deletions(-)
diff --git a/API.txt b/API.txt
index a90e60ad97fa56a304c54fd61a4b02ad7559882f..d0ae1b72c2ae445a4e2cc168da5fd53f9a4de56d 100644
--- a/API.txt
+++ b/API.txt
@@ -5332,7 +5332,7 @@ arg: Str('cn', attribute=True, cli_name='name', maxlength=255, multivalue=False,
option: Str('addattr*', cli_name='addattr', exclude='webui')
option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
option: Str('description?', cli_name='desc')
-option: Bytes('ipapublickey?', cli_name='public_key')
+option: Bytes('ipavaultpublickey?', cli_name='public_key')
option: Str('ipavaulttype?', cli_name='type')
option: Str('password?', cli_name='password')
option: Str('password_file?', cli_name='password_file')
@@ -5351,7 +5351,7 @@ args: 1,10,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('ipapublickey', attribute=True, cli_name='public_key', 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('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
@@ -5430,7 +5430,7 @@ option: Str('addattr*', cli_name='addattr', exclude='webui')
option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
option: Str('delattr*', cli_name='delattr', exclude='webui')
option: Str('description', attribute=True, autofill=False, cli_name='desc', multivalue=False, required=False)
-option: Bytes('ipapublickey', attribute=True, autofill=False, cli_name='public_key', multivalue=False, required=False)
+option: Bytes('ipavaultpublickey', attribute=True, autofill=False, cli_name='public_key', multivalue=False, required=False)
option: Bytes('ipavaultsalt', attribute=True, autofill=False, cli_name='salt', multivalue=False, required=False)
option: Str('ipavaulttype', attribute=True, autofill=False, cli_name='type', default=u'standard', multivalue=False, required=False)
option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
diff --git a/VERSION b/VERSION
index f96638721fb10c5925e9289da4ba41c86e39adeb..f69a5bb880c1141b620159fa3e6ea6f0eb6a30fd 100644
--- a/VERSION
+++ b/VERSION
@@ -90,5 +90,5 @@ IPA_DATA_VERSION=20100614120000
# #
########################################################
IPA_API_VERSION_MAJOR=2
-IPA_API_VERSION_MINOR=136
-# Last change: edewata - added symmetric and asymmetric vaults
+IPA_API_VERSION_MINOR=137
+# Last change: edewata - added ipaVaultPublicKey attribute
diff --git a/install/share/60basev3.ldif b/install/share/60basev3.ldif
index cb159db05a5371c71e421160f60140d85ba5496f..5491f99f5e78f122f94e9215bf5751d487f19d2e 100644
--- a/install/share/60basev3.ldif
+++ b/install/share/60basev3.ldif
@@ -58,6 +58,7 @@ attributeTypes: (2.16.840.1.113730.3.8.11.70 NAME 'ipaPermTargetTo' DESC 'Destin
attributeTypes: (2.16.840.1.113730.3.8.11.71 NAME 'ipaPermTargetFrom' DESC 'Source location from where moving an entry IPA permission ACI' EQUALITY distinguishedNameMatch SYNTAX 1.3.6.1.4.1.1466.115.121.1.12 SINGLE-VALUE X-ORIGIN 'IPA v4.0' )
attributeTypes: (2.16.840.1.113730.3.8.18.2.1 NAME 'ipaVaultType' DESC 'IPA vault type' EQUALITY caseExactMatch SYNTAX 1.3.6.1.4.1.1466.115.121.1.15 X-ORIGIN 'IPA v4.2')
attributeTypes: (2.16.840.1.113730.3.8.18.2.2 NAME 'ipaVaultSalt' DESC 'IPA vault salt' EQUALITY octetStringMatch SYNTAX 1.3.6.1.4.1.1466.115.121.1.40 X-ORIGIN 'IPA v4.2' )
+attributeTypes: (2.16.840.1.113730.3.8.18.2.3 NAME 'ipaVaultPublicKey' DESC 'IPA vault public key' SUP ipaPublicKey X-ORIGIN 'IPA v4.2' )
objectClasses: (2.16.840.1.113730.3.8.12.1 NAME 'ipaExternalGroup' SUP top STRUCTURAL MUST ( cn ) MAY ( ipaExternalMember $ memberOf $ description $ owner) X-ORIGIN 'IPA v3' )
objectClasses: (2.16.840.1.113730.3.8.12.2 NAME 'ipaNTUserAttrs' SUP top AUXILIARY MUST ( ipaNTSecurityIdentifier ) MAY ( ipaNTHash $ ipaNTLogonScript $ ipaNTProfilePath $ ipaNTHomeDirectory $ ipaNTHomeDirectoryDrive ) X-ORIGIN 'IPA v3' )
objectClasses: (2.16.840.1.113730.3.8.12.3 NAME 'ipaNTGroupAttrs' SUP top AUXILIARY MUST ( ipaNTSecurityIdentifier ) X-ORIGIN 'IPA v3' )
@@ -81,4 +82,4 @@ objectClasses: (2.16.840.1.113730.3.8.12.24 NAME 'ipaPublicKeyObject' DESC 'Wrap
objectClasses: (2.16.840.1.113730.3.8.12.25 NAME 'ipaPrivateKeyObject' DESC 'Wrapped private keys' SUP top AUXILIARY MUST ( ipaPrivateKey $ ipaWrappingKey $ ipaWrappingMech ) X-ORIGIN 'IPA v4.1' )
objectClasses: (2.16.840.1.113730.3.8.12.26 NAME 'ipaSecretKeyObject' DESC 'Wrapped secret keys' SUP top AUXILIARY MUST ( ipaSecretKey $ ipaWrappingKey $ ipaWrappingMech ) X-ORIGIN 'IPA v4.1' )
objectClasses: (2.16.840.1.113730.3.8.12.34 NAME 'ipaSecretKeyRefObject' DESC 'Indirect storage for encoded key material' SUP top AUXILIARY MUST ( ipaSecretKeyRef ) X-ORIGIN 'IPA v4.1' )
-objectClasses: (2.16.840.1.113730.3.8.18.1.1 NAME 'ipaVault' DESC 'IPA vault' SUP top STRUCTURAL MUST ( cn ) MAY ( description $ ipaVaultType $ ipaVaultSalt $ ipaPublicKey ) X-ORIGIN 'IPA v4.2' )
+objectClasses: (2.16.840.1.113730.3.8.18.1.1 NAME 'ipaVault' DESC 'IPA vault' SUP top STRUCTURAL MUST ( cn ) MAY ( description $ ipaVaultType $ ipaVaultSalt $ ipaVaultPublicKey ) X-ORIGIN 'IPA v4.2' )
diff --git a/ipalib/plugins/vault.py b/ipalib/plugins/vault.py
index 193fa5cbb6eb06d22a30d8cfba62e10e9557c1d6..9fcd619d19de9ae036a73bb3af9dc050c6be6c76 100644
--- a/ipalib/plugins/vault.py
+++ b/ipalib/plugins/vault.py
@@ -233,7 +233,7 @@ class vault(LDAPObject):
'description',
'ipavaulttype',
'ipavaultsalt',
- 'ipapublickey',
+ 'ipavaultpublickey',
]
search_display_attributes = [
'cn',
@@ -276,7 +276,7 @@ class vault(LDAPObject):
flags=['no_search'],
),
Bytes(
- 'ipapublickey?',
+ 'ipavaultpublickey?',
cli_name='public_key',
label=_('Public key'),
doc=_('Vault public key'),
@@ -509,7 +509,7 @@ class vault_add(PKQuery, Local):
doc=_('File containing the vault password'),
),
Bytes(
- 'ipapublickey?',
+ 'ipavaultpublickey?',
cli_name='public_key',
doc=_('Vault public key'),
),
@@ -527,7 +527,7 @@ class vault_add(PKQuery, Local):
vault_type = options.get('ipavaulttype', u'standard')
password = options.get('password')
password_file = options.get('password_file')
- public_key = options.get('ipapublickey')
+ public_key = options.get('ipavaultpublickey')
public_key_file = options.get('public_key_file')
# don't send these parameters to server
@@ -584,11 +584,11 @@ class vault_add(PKQuery, Local):
public_key = f.read()
# store vault public key
- options['ipapublickey'] = public_key
+ options['ipavaultpublickey'] = public_key
else:
raise errors.ValidationError(
- name='ipapublickey',
+ name='ipavaultpublickey',
error=_('Missing vault public key'))
# create vault
@@ -606,7 +606,7 @@ class vault_add(PKQuery, Local):
del opts['ipavaultsalt']
elif vault_type == u'asymmetric':
- del opts['ipapublickey']
+ del opts['ipavaultpublickey']
# archive blank data
self.api.Command.vault_archive(*args, **opts)
@@ -920,7 +920,7 @@ class vault_archive(PKQuery, Local):
elif vault_type == u'asymmetric':
- public_key = vault['ipapublickey'][0].encode('utf-8')
+ public_key = vault['ipavaultpublickey'][0].encode('utf-8')
# generate encryption key
encryption_key = base64.b64encode(os.urandom(32))
diff --git a/ipatests/test_xmlrpc/test_vault_plugin.py b/ipatests/test_xmlrpc/test_vault_plugin.py
index f8b57855a152c4c86d3a7681e6cc187a85b2c468..3db93b207fac405ba654b84a2a07668d9a69edb6 100644
--- a/ipatests/test_xmlrpc/test_vault_plugin.py
+++ b/ipatests/test_xmlrpc/test_vault_plugin.py
@@ -634,7 +634,7 @@ class test_vault_plugin(Declarative):
[asymmetric_vault_name],
{
'ipavaulttype': u'asymmetric',
- 'ipapublickey': public_key,
+ 'ipavaultpublickey': public_key,
},
),
'expected': {
@@ -646,7 +646,7 @@ class test_vault_plugin(Declarative):
'objectclass': [u'top', u'ipaVault'],
'cn': [asymmetric_vault_name],
'ipavaulttype': [u'asymmetric'],
- 'ipapublickey': [public_key],
+ 'ipavaultpublickey': [public_key],
},
},
},
--
1.9.3
More information about the Freeipa-devel
mailing list