[Freeipa-devel] [PATCH] Password vault

Wed Jun 17 06:32:03 UTC 2015

Dne 16.6.2015 v 01:02 Endi Sukma Dewata napsal(a):
> 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.

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

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

You are comparing apples and oranges:

  a) When the non-split vault_{archive,retrieve} was called from a 
server API with client-only options, it crashed. This is the broken API 
I was talking about.

  b) The non-split vault_{archive,retrieve} had server-only options, 
which were also accepted on client, but setting them had no effect.

  c) The CLI options to read param values from files should be generated 
by the framework without having to specify dummy params. Once this is 
implemented, the dummy params will go away. However, this will still 
leave some client-only options in vault_{archive,retrieve}.

None of the above applies to vault_add - it does not have any 
server-only options and the only client-only options it has are the 
dummy options for file input, which are ignored on the server.

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

How come --password is client-side? When setting password for a user, 
the password is sent to the server. If it's OK for users, why is it not 
OK for vaults?

Does the password need to be set in vault_add? Why not have a separate 
command for setting the password, like what we have for users?

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

It's not. Currently there is a set of commands which operate on the LDAP 
part of vault and another set of commands which operate on the KRA part 
of vault and we don't want the commands in one set to see attributes 
related to the other part of vault. If you insist on keeping both parts 
in a single object, you have to resort to hackery like using PKQuery, 
hence my suggestion to split the data part off to a separate object to 
avoid this.

>Why not use LDAPQuery since vault
> is an LDAPObject?

Because you are retrieving data from KRA, not from LDAP.

> And to be consistent should vault_retrieve_internal
> inherit from the same class?

It could, but it's not necessary.

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

-- 
Jan Cholasta