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
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.
I agree.
* 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.
I don't agree with the "user's perspective first, developer's
perspective later" approach and I think it has already been proven wrong
by the existing plugin code in IPA. The plugins work (almost) fine from
the user's perspective, but they are (almost) completely unmaintainable
spaghetti code balls for developers. I would prefer if the code was both
user friendly and maintainable.
Anyway, the patches work for me, so ACK.
There was one thing that didn't work, but this can be fixed in an
additional patch:
$ ipa vault-add asymtest --type=asymmetric
--public-key=MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDR+xRAv5lCgNzJVcY7TW3xwx31+nGbrXSmOZWaH/EywKKH8O2IdSzoEgbFn7L4x+QcXWa4pcq8R5BfabJZs+uDUfPOS09LcB5gQlq+jgUtEKCgGy/u2yNrXiLs712p8cnHfIc4C6VCO3xxY0rZXimJTLjK0KTHzCzaQeDRq04JNQIDAQAB
ipa: ERROR: non-public: UnicodeDecodeError: 'utf8' codec can't decode
byte 0x81 in position 1: invalid start byte
Traceback (most recent call last):
File "/usr/lib/python2.7/site-packages/ipalib/backend.py", line 129,
in execute
result = self.Command[_name](*args, **options)
File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 440,
in __call__
ret = self.run(*args, **options)
File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line
1106, in run
return self.forward(*args, **options)
File "/usr/lib/python2.7/site-packages/ipalib/plugins/vault.py", line
612, in forward
self.api.Command.vault_archive(*args, **opts)
File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line 440,
in __call__
ret = self.run(*args, **options)
File "/usr/lib/python2.7/site-packages/ipalib/frontend.py", line
1106, in run
return self.forward(*args, **options)
File "/usr/lib/python2.7/site-packages/ipalib/plugins/vault.py", line
923, in forward
public_key = vault['ipavaultpublickey'][0].encode('utf-8')
UnicodeDecodeError: 'utf8' codec can't decode byte 0x81 in position 1:
invalid start byte
ipa: ERROR: an internal error has occurred
I was able to work around this by removing the faulty ".encode('utf-8')"
and replacing load_pem_{public,private}_key by
load_der_{public,private}_key.
For the record, I still think that the ipaVaultType attribute is a bad
design and object classes should have been used to distinguish between
the different vault types.
Rebased and pushed to master: 475ade4becd4cdb59a9bcf0da7de1d2739e293c8
--
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
