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

--
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

Reply via email to