On 5/14/2015 1:42 PM, Jan Cholasta wrote:
Question: Services in IPA are identified by Kerberos principal. Why are
service vaults identified by hostname alone?


The service vaults are actually identified by the hostname and service
name assuming the principal is in this format: <name>/<host>@<realm>.
The vault is stored in cn=<name>, cn=<host>, cn=services, cn=vaults,
<suffix>. When accessing a vault service you are supposed to specify the
name and the host (except for vault-find which will return all services
in the same host):

$ ipa vault-find --host <host>
$ ipa vault-add <name> --host <host>

Are there other service principal formats that we need to support? Do we
need to support services of different realms?

Well, users are identified by username (uid attribute), hosts by
hostname (fqdn attribute) and services by principal name
(krbprincipalname attribute). Each of them has separate container and is
also uniquely identified by principal name. I guess it would make sense
to follow that for vaults as well, in which case service vaults should
be called host vaults (because they are created in a host-specific
container, not a service-specific one) and maybe "real" service vaults
should be added.

There's no plan for host vaults in the design doc, but it's not impossible to add it in the future. What is the use case?

I suppose we can change the service vault into a container, and possibly add generic user vaults too (in addition to private user vaults), the interface will look like this:

$ ipa vault-add PrivateVault
$ ipa vault-add ServiceVault --service <name>/<hostname>
$ ipa vault-add SharedVault --shared
$ ipa vault-add UsersVault --user <username>
$ ipa vault-add HostVault --host <hostname>

Basically we'll need a new parameter for each new container. For the initial implementation we only need the first 2 or 3.

Do we
need to support services of different realms?

That depends. Do we want to support vaults for users and services from
AD trusts?

It wasn't mentioned in the design doc either, but probably in the future we can support external vaults too:

$ ipa vault-add ExternalVault --principal <principal>

  cn=vaults
  + cn=external
    + cn=<name>/<hostname>@<realm>
      + cn=<vault 1>
      + cn=<vault 2>
    + cn=<user>@<realm>
      + cn=<vault 3>
      + cn=<vault 4>

1. The following code in get_dn() is causing problems for vault-find.

   dn = super(vault, self).get_dn(*keys, **options)
   rdn = dn[0]
   container_dn = DN(*dn[1:])

The super.get_dn() will return cn=vaults, <suffix>, so the rdn will
become cn=vaults and container_dn will become <suffix>. When combined
with the subcontainer (private/shared/service) it will produce an
invalid DN.

Right, fixed.

I should have tested the patch before posting it, my bad, sorry.

OK.

2. Not sure if it'related to #1, the vault-find is failing.

$ ipa vault-find
ipa: ERROR: an internal error has occurred

The error_log shows TypeError: 'NoneType' object is not iterable

Fixed. It was caused by missing return value in vault_find.exc_callback.

We can actually ignore all NotFound errors since now all containers are either created automatically or already created by default.

  if call_func.__name__ == 'find_entries':
      if isinstance(exc, errors.NotFound):
          shared = options.get('shared')

          # if private or service container has not been created, ignore
          if not shared:
              raise errors.EmptyResult(reason=str(exc))

The original code was only ignoring NotFound errors on user vaults because previously only the user containers was supposed to be created automatically, and there wasn't a plan to provide service container.

3. The --shared option in vault-find is now requiring an argument even
though it's a Flag.

$ ipa vault-find --shared
Usage: ipa [global-options] vault-find [CRITERIA] [options]

ipa: error: --shared option requires an argument

Fixed.

OK.

4. The following code in get_dn() is incorrect:

   principal = getattr(context, 'principal')
   (name, realm) = split_principal(principal)
   name = name.split('/')
   if len(name) == 1:
       container_dn = DN(('cn', 'users'), container_dn)
   else:
       container_dn = DN(('cn', 'services'), container_dn)
   container_dn = DN(('cn', name[-1]), container_dn)

A service does not have a private container like users (cn=<username>,
cn=users, cn=vaults). The entry cn=<name>, cn=<host>, cn=services,
cn=vaults is a service vault, not a container. The service vault is used
by the admin to provide a secret for a service.

I'm not sure what the behavior should be if a service is executing a
vault command that uses a private container such as:

   $ ipa vault-add test

Maybe it should just generate an error.

Users, hosts and services are all user-like objects, is there a reason
not to support private vaults for all of them?

As mentioned above, it's not required in the design doc, but we can add it if there's a clear use case. I agree that at least for now we can change the service vault into a service container to store multiple service's private vaults.

5. In create_container() why do you need to reconstruct the container_dn
on each invocation even though the value is fixed?

   container_dn = DN(self.container_dn, self.api.env.basedn)

Because self.api may not necessarily be the same as ipalib.api.

Under what scenario would that be a problem? The original code seems to be working fine with ipalib.api.

If it is a problem, why do we still use ipalib.api to initialize container_dn vault class attribute?

  container_dn = api.env.container_vault

Then in get_dn() we basically construct the container_dn variable with values from both self.api and ipalib.api:

  container_dn = DN(self.container_dn, self.api.env.basedn)

When is the self.api actually initialized? Can we initialize the container_dn (or base_dn as in the original code) attribute immediately after that?

6. The loop in create_container() is incorrect. Suppose we're creating a
container cn=A, cn=B, <suffix> and the parent container cn=B, <suffix>
doesn't exist yet. The first add_entry() invocation will fail as
expected, but instead of adding the parent entry the whole method will
fail.

Right, fixed.

It's still not working. The new code is trying to add cn=vaults first, and it stops because it already exists, but the intermediary entries are still not added. Also the DN displayed in the message misleading:

$ ipa vault-add test
ipa: ERROR: container entry (cn=vaults) not found

$ ipa vault-add test --host server.example.com
ipa: ERROR: container entry (cn=vaults) not found

The unit tests are failing because of this.

7. The host and shared parameters are no longer available in vault-show
and vault-del commands. The unit tests are failing because of that. Why
do these commands not automatically inherit all parameters from the
class?

The parameters of Object plugins represent attributes of the object, so
they are all provided in commands which allow setting values of the
attributes (-add and -mod). Other commands need to only know which
object to look up, so they provide only the primary key, which is
supposed to uniquely identify the object.

The issue with vaults is that they are in fact identified by
(vault_name, host, shared) tuple, but the primary_key is just
vault_name. Fixed by reverting to your original approach.

OK. As discussed above, the vault may need to be identified by even more parameters, e.g. service, user, principal.

Updated patch attached.

8. There's a new PEP8 error:

.../ipalib/plugins/vault.py:98:1: E302 expected 2 blank lines, found 1

9. The API.txt needs to be regenerated.

Also attached is a modified patch, which implements hierarchical vault
container objects. It is very hacky, but functional and self-contained.
You can use it if you want.

Let's circle back to this after we get the essential vault functionality checked in. The latest plan was to merge vaultcontainer into vault.

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