[Freeipa-devel] [PATCH] Password vault

[Jan Cholasta]

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