[Freeipa-devel] [PATCH] Password vault
Jan Cholasta
jcholast at redhat.com
Tue Jul 7 07:46:09 UTC 2015
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
More information about the Freeipa-devel
mailing list