On 06/02/2015 02:00 AM, Endi Sukma Dewata wrote:
Please take a look at the updated patch.

On 5/27/2015 12:39 AM, Jan Cholasta wrote:
21) vault_archive is not a retrieve operation, it should be based on
LDAPUpdate instead of LDAPRetrieve. Or Command actually, since it
does
not do anything with LDAP. The same applies to vault_retrieve.

The vault_archive does not actually modify the LDAP entry because it
stores the data in KRA. It is actually an LDAPRetrieve operation
because
it needs to get the vault info before it can perform the archival
operation. Same thing with vault_retrieve.

It is not a LDAPRetrieve operation, because it has different
semantics.
Please use Command as base class and either use ldap2 for direct
LDAP or
call vault_show instead of hacking around LDAPRetrieve.

It's been changed to inherit from LDAPQuery instead.

NACK, it's not a LDAPQuery operation, because it has different
semantics. There is more to a command than executing code, so you should
use a correct base class.

Changed to inherit from Command as requested. Now these commands no
longer have a direct access to the vault object (self.obj) although they
are accessing vault objects like other vault commands. Also now the
vault name argument has to be added explicitly on each command.

You can inherit from crud.Retrieve and crud.Update to get self.obj and
the argument back.

I tried this:

   class vault_retrieve(Command, crud.Retrieve):

and it gave me an error:

   TypeError: Error when calling the metaclass bases
       Cannot create a consistent method resolution
   order (MRO) for bases Retrieve, Command

I'm sticking with the original code since it works fine although not ideal. I'm
not a Python expert, so if you know how to fix this properly please feel free
to post a patch on top of this.

If KRA is not installed, vault-archive and vault-retrieve fail with
internal error.

Added a code to check KRA installation in all vault commands. If you know a way
not to load the vault plugin if the KRA is not installed please let me know,
that's probably even better. Not sure how that will work on the client side
though.

The commands still behave differently based on whether they were called
from API which was initialized with in_server set to True or False.

That is unfortunately a restriction imposed by the framework. In order to
guarantee the security, the vault is designed to have separate client and
server code. The client code encrypts the secret, the server code forwards the
encrypted secret to KRA. To archive a secret into a vault properly, you are
supposed to call the client code. If you're calling the server code directly,
you are responsible to do your own encryption (i.e. generating session key,
nonce, and vault data).

If another plugin wants to use vault, it should implement a client code which
calls the vault client code to perform the archival from the client side.

What is the use case for calling the vault API from the server side anyway?
Wouldn't that defeat the purpose of having a vault? If a secret exists on the
server side in an unencrypted form doesn't it mean the secret may already have
been compromised?

There is no point in exposing the session_key, nonce and vault_data
options in CLI when their value is always overwritten in forward().

I agree there is no need to expose them in CLI, but in this framework the API
also defines the CLI. If there's a way to keep them in the server API but not
expose them in the CLI please let me know. Or, if there's a way to define
completely separate server API (without a matching client CLI) and client CLI
(without a matching server API) that will work too.

Will this always succeed?

+        # deactivate vault record in KRA
+        response = kra_client.keys.list_keys(
+            client_key_id, pki.key.KeyClient.KEY_STATUS_ACTIVE)

Yes. If there's no active keys it will return an empty collection.

+        for key_info in response.key_infos:
+            kra_client.keys.modify_key_status(
+                key_info.get_key_id(),
+                pki.key.KeyClient.KEY_STATUS_INACTIVE)

This loop will do nothing given an empty collection.

If not, we might get into an inconsistent state, where the vault is
deleted in LDAP but still active in KRA. (I'm not sure if this is
actually a problem or not.)

That can only happen if the server crashes after deleting the vault but before
deactivating the key. Regardless, it will not be a problem because the key is
identified by vault ID/path so it will not conflict with other vaults, and it
will get overwritten if the same vault is recreated again.


Hi Endi,

Quickly skimming through your patches raised couple questions on my side:

1) Will it be possible to also store plain text password via Vault? It talks about taking in the binary data or the text file, but will it also work with plain user secrets (passwords)? I am talking about use like this:

# ipa vault-archive <name> --user mkosek --data Secret123


2) Didn't we discuss a dependency of IPA/Vault on python-cryptography in the past? I rather see use of python-nss for cryptography...

3) You do a lot of actions in the forward() method (as planned in
https://www.freeipa.org/page/V4/Password_Vault#Archival). But how do you envision that this is consumed by the Web UI? It does not have access to the forward() method. Would it need to also include some crypto library?

4) In the vault-archive forward method, you use "pki" module. However, this module will be only available on FreeIPA PKI-powered servers and not on FreeIPA clients - so this will not work unless freeipa-client gets a dependency on pki-base - which is definitely not something we want...

Thanks,
Martin

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