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
>From bf83dfd7d1f140eb26cc3f7a9c265a0d3743fbbb Mon Sep 17 00:00:00 2001
From: "Endi S. Dewata" <edew...@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
--
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
