On 6/5/2015 7:13 AM, Jan Cholasta wrote:
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 other thread was talking about removing the pki-base dependency on the client side, but the vault plugin is still loaded on both client and server regardless of KRA installation. Ideally the vault plugin should not even be loaded so you cannot even execute the commands.

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.

I see the vault_archive and vault_retrieve now inherit from PKQuery, and there is a hack to execute the forward() even on the server side. A few things below:

1. Why didn't you use frontend.Local as you initially suggested? If there's a problem with frontend.Local please attach the ticket number in the code.

2. The forward() can be merged into run(). There is no need to keep the code in forward(). It would make more sense to have a run() method that runs both on client and server, rather than a forward() that is supposed to run on the client only but now forced to run on server too, semantically speaking.

Attached is a patch including the requested changes.

I have also changed vault_config to vaultconfig_show, for consistency
with {,dns}config_show (it also makes the transport certificate
retrieval code in vault_{archive,retrieve} simpler).

3. The parameter description for nonce should be just 'Nonce' instead of 'Nonce encrypted'.

4. There's a PEP8 error.

5. The VERSION needs to be updated.

Assuming the above issues are addressed, ACK.

I have noticed that triple-length DES is used for the session key.
Wouldn't AES be better?

         # generate session key
         mechanism = nss.CKM_DES3_CBC_PAD

That's the default used by the KRA's client library, and that's what the KRA has been tested with. We probably can change it to AES later. It shouldn't be blocking this patch.

BTW, ipa-kra-install is broken with pki-core-10.2.4-1, but it works with
pki-core-10.2.1-3.

There's a bug in IPA: https://bugzilla.redhat.com/show_bug.cgi?id=1228671

--
Endi S. Dewata

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