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

The class hierarchy is as follows:

   frontend.Command
       frontend.Method
           crud.PKQuery
               crud.Retrieve
               cdur.Update

So removing Command from the list of base classes should fix it.


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.

I see this has been already resolved in the other thread.


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

I understand why the code has to be separated, what I don't understand is why it is in fact *not* separated and crammed into a single command, making weird and undefined behavior possible.


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?

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.


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.

As I suggested above, you can split the commands into separate client and server commands. The client command should inherit from frontend.Local so that it is always executed locally and the server command should have a "NO_CLI = True" attribute so that it is not available in the CLI.


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.

OK.

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