Dne 25.6.2015 v 19:01 Endi Sukma Dewata napsal(a):
On 6/25/2015 12:35 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
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.)
It's unchanged for now. In a previous discussion it was advised to reuse
the existing attribute type whenever possible.
Well, in this discussion, it is not. Escrow public key should also reuse
ipaPublicKey, but it can't if you use it for vault public key. By using
ipaPublicKey subtypes you can distinguish between the two uses and still
use ipaPublicKey to refer to either of them.
So what's changed? This is what you said when I posted the same patch
six months ago:
In this patch I'm adding ipaVaultSalt and ipaVaultPublicKey attribute
types to store salt and public key for vault. Are there existing
attribute types that I can use instead? I see there's an ipaPublicKey,
should I use that and maybe add ipaSalt/ipaEncSalt? Thanks.
yes, please re-use existing attributes where possible.
Honza
What changed is that I now know there is also escrow public key, which I
didn't know six months ago.
Could you show me how to use ipaPublicKey to refer to ipaVaultPublicKey
and ipaEscrowPublicKey? Under what situation would that be useful?
For example for ipaPublicKey searches - if ipaVaultPublicKey and
ipaEscrowPublicKey both inherit from ipaPublicKey, then an ipaPublicKey
search will look in both ipaVaultPublicKey and ipaEscrowPublicKey. This
is not something we actually need right now, but once the schema is
done, it can't be fixed and I don't think we should prevent this,
especially since we can get it for free. BTW even the core LDAP schema
does this, see for example how the cn attribute inherits from the more
general name attribute: <https://tools.ietf.org/html/rfc4519#section-2.3>.
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.
This is because in the current framework any API called on the server
side will be a server API, so you are not supposed to call it with
client options in the first place. Because of that limitation, the only
way to use client options is to use a separate API on the client side to
call the original API on the server side. The point is, client options
belong to client API, and server options belong to server API. In
vault_add the public key file name belongs to client API because it's
used to load a file on the client side. You should not add public key
file name option to the server API just because it can safely be
ignored.
I don't disagree, but file name options do not belong to the general
client API either, as they are strictly CLI-specific.
To my understanding the current framework doesn't have a separate CLI
class, so you don't have a choice but to put CLI-specific options in the
client API class too. However, you do have a choice not to combine
client API class and server API class because otherwise that will put
CLI-specific options in the server API class too.
Right.
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.
This because the framework was based on simplistic assumptions which
create unnecessary restrictions, for example:
* client API is just a proxy to server API (i.e. client and server
cannot do different things)
They can do different things the same way vault_archive/vault_retrieve
does that, the commands just can't be called the same (which is not
necessarily a bad thing).
Of course different APIs can do different things, like vault_add calling
vault_archive, or vault_archive calling vault_archive_internal. The
point is right now the client portion of an API (i.e. the forward()
method) cannot do anything other than forwarding the request to the
server, so the API has to be split into different APIs:
* vault_archive
* vault_archive_internal
It would be nice to have formal separation between client and server
APIs so it's clear they are different but still related without
resorting to ugly names:
* client.vault_archive
* server.vault_archive
Yes.
* CLI options will be identical to client and server API options (i.e.
no CLI-only, client-only, or server-only options)
Actually, you can create CLI-only options (add include='cli' to the
param's kwargs).
I need to look at this more closely. If I understand correctly in
user_del there are two 'preserve' options, the Bool preserve is for
client and server API, and the Flag preserve is for CLI. Wouldn't it be
better if they are stored in separate lists (or maybe separate classes)?
And it looks like you still need to delete the CLI options explicitly
anyway.
Well, it would be better if there was no Flag class at all and flags
were handled by CLI exclusively, because parameter classes should
reflect the data type (bool) and not the presentation (flag).
Does the API.txt actually show the CLI options, the client API options,
or the server API options? I only see the Flag preserve, not the Bool
preserve.
It shows CLI options, see how the API object is initialized in makeapi.
* a plugin will only access one type of data (i.e. LDAP plugin can only
access LDAP data)
This is not assumed anywhere in the framework, you can access whatever
you want, but you can't expect baseldap to do everything for you.
Nobody is expecting baseldap to do KRA operations.
As the
name implies, it is LDAP specific, if you want something else, you have
to implement it yourself.
In the previous patch vault_retrieve inherits from LDAPRetrieve so it
can rely on baseldap to retrieve the vault entry, then on top of that it
implements an additional KRA operations (without baseldap obviously). If
that is not allowed, aren't you basically saying LDAP plugin can only
access LDAP data?
Yes, basically, but I'm also saying that you are not limited to doing
LDAP plugins only.
You can abuse the callbacks to do anything, including data retrieval
from other sources, but it doesn't make it right, as it only leads to
code duplication, inconsistencies and weird bugs. I have seen too much
of this, hence my reluctance to do it again.
* a command name will match the object name (i.e. must use vaultdata_mod
instead of a more intuitive vault_archive)
I don't see how consistency is a bad thing, or how this could limit
anyone doing things cleanly. I do agree that vaultdata_mod is ugly, but
it's not the only way to achieve the same goal.
Look at it from user's perspective. If you create a vault using
vault-add <vault name>, then archive data using vaultdata-mod <vault
name>, how is this consistent?
Because it's object-verb and not object-verbofsomeotherobject. (Also I
already acknowledged the vaultdata idea is ugly.)
We know that some use cases do not fit these assumptions. Rather than
compromising the use case, or looking at workarounds as hacks, I'd
suggest finding ideas to improve the framework itself to be more
accommodating.
I would personally love to improve the framework (it's just retarded
sometimes as you may have noticed), but it does not have high priority
right now (not my decision).
We don't have to modify the current framework right now, but we can
align new codes that don't fit the current framework to match the future
framework. Although the future framework is not defined yet, some things
are already clear, for example there should be separate client and
server APIs. So if a command like vault_add has differing client and
server options, regardless how insignificant it is, there's no reason to
force it to be combined. The current framework doesn't prevent
separation anyway.
Aligning new code is exactly what I'm aiming to do and why I want people
to look at their APIs from an object oriented perspective rather than
just dumb RPC, because that's the direction the framework is heading.
Keep in mind that workarounds which screw with the object model will
always be considered hacks, even after the framework is made more
accomodating.
Don't get this wrong. The framework will only be considered
accommodating if it allows people to implement features without
'hacking'. 'Hacking' itself is never a goal, it's the last resort to
work around the framework's current limitations, just like how you ended
up using PKQuery for vault_archive_internal.
Well, there's so many hacks in every corner of IPA that many people
don't see them as hacks. PKQuery is not that bad, though it doesn't
implement as many bits as would be useful in this case.
--
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
