[Freeipa-devel] [PATCH] Password vault

[Endi Sukma Dewata]

On 6/15/2015 2:22 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
>
>      ( 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' )

OK. To be consistent the parameters need to be renamed too: 
--vault-public-key and --vault-public-key-file.

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

We know the vault-add will have at least two client-only parameters: 
vault_public_key_file and escrow_public_key_file. Keeping these 
parameters on the server API would be wrong and confusing. If the API is 
called on the server side with vault_public_key_file the operation will 
fail. In the previous discussion you considered this as broken API:

> Server API is used not only by the server itself, but also by installers
> for example. Anyway the point is that there *can't* be a broken API like
> this, you should at least raise an error if the command is called from
> server API, although actually separating it into client and server parts
> would be preferable.

Also, originally the vault was designed like this: when you create a 
symmetric vault you're supposed to specify the password as well, similar 
to adding a public key when creating an asymmetric vault. When you 
archive, you're supposed to enter the same password for verification, 
not a new password. So it would look like this:

$ ipa vault-add test --type symmetric
New password: ********
Verify password: ********

$ ipa vault-archive test --in secret1.txt
Password: ******** (same password)

$ ipa vault-archive test --in secret2.txt
Password: ******** (same password)

In the original design the vault-add would also archive a blank data, 
which later could be used to verify the password during vault-archive by 
decrypting the existing data first. There's also a plan to add a 
mechanism to change the password after the ACL patch.

In the current design the vault-add doesn't archive anything, so during 
vault-archive it cannot verify the password because there is nothing to 
decrypt. In other words you can specify different passwords on each 
archival, regardless of previous archivals:

$ ipa vault-add test --type symmetric

$ ipa vault-archive test --in secret1.txt
New password: ********
Verify password: ********

$ ipa vault-archive test --in secret2.txt
New password: ********
Verify password: ********

So basically here are the options:

1. Specify the crypto parameters once during vault creation, then 
reuse/verify the parameters on each archival & retrieval. You can change 
the parameters only with a special command.

2. Don't specify the crypto parameters during vault creation, but 
specify new parameters on each archival. For retrieval you'd have to 
use/verify the parameters specified in the last archival.

I think the first one makes more sense and is easier to use. That also 
means the vault-add will have additional client-only parameters such as 
--password and --password-file.

>>> 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").

Previously you didn't want to use LDAPQuery because of semantics 
reasons. Is PKQuery fine semantically? Why not use LDAPQuery since vault 
is an LDAPObject? And to be consistent should vault_retrieve_internal 
inherit from the same class?

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

-- 
Endi S. Dewata