[__opendkim_genkey] key's selector is the de-facto object id

evilham commented

2022-03-15 08:12:19 +00:00

Collaborator

This is an issue because it makes common scenarios like using a different default key (with the default selector) for different domains impossible.

The selector becomes the de-facto object id because the key file path is defined as:
${DIRECTORY}${SELECTOR}.private
(it's a tad worse because calls to the same object_id with different params are an error in cdist, while this situation wouldn't catch those, and instead each call will treat that file as if it were theirs)

A more sensible location would be:
${DIRECTORY}${__object_id}.private

Fixing this requires some type-backwards compatibility in order not to break existing deployments.

An idea is fixing #19 by adding a key-state explorer that returns the location and state of the key, and checks ${DIRECTORY}${SELECTOR}.private when ${DIRECTORY}${__object_id}.private does not exist.

As a result, old deployments should keep working while new deployments should never be affected by this.

Cc: @sparrowhawk

This is an issue because it makes common scenarios like using a different default key (with the `default` selector) for different domains impossible. The selector becomes the de-facto object id because the key file path is defined as: `${DIRECTORY}${SELECTOR}.private` (it's a tad worse because calls to the same object_id with different params are an error in cdist, while this situation wouldn't catch those, and instead each call will treat that file as if it were theirs) A more sensible location would be: `${DIRECTORY}${__object_id}.private` Fixing this requires some type-backwards compatibility in order not to break existing deployments. An idea is fixing #19 by adding a `key-state` explorer that returns the location and state of the key, and checks `${DIRECTORY}${SELECTOR}.private` when `${DIRECTORY}${__object_id}.private` does not exist. As a result, old deployments should keep working while new deployments should never be affected by this. Cc: @sparrowhawk

sparrowhawk commented

2022-03-15 14:14:12 +00:00

Collaborator

@evilham that seems like a nice fix, I completely agree with your idea. I don't really have the time to fix this rn (unless the fix is needed for $work soon, which I don't think it will), but feel free to go ahead if you do :)

Also, pintsize \o/

@evilham that seems like a nice fix, I completely agree with your idea. I don't really have the time to fix this rn (unless the fix is needed for $work soon, which I don't think it will), but feel free to go ahead if you do :) Also, pintsize \o/

evilham commented

2022-03-15 21:41:14 +00:00

Author

Collaborator

@sparrowhawk have a fix for this and #19 mostly ready; got a bit of a doubt: do you think it's a terrible-idea-tm (and why? :-D) to default --selector to __object_id?

It feels to me like the reason we have this inconsistency is because... Well, they usually match :-p.

It'd be a good time to simplify the type by making that parameter optional; what do you think?

@sparrowhawk have a fix for this and #19 mostly ready; got a bit of a doubt: do you think it's a terrible-idea-tm (and why? :-D) to default `--selector` to `__object_id`? It feels to me like the reason we have this inconsistency is because... Well, they usually match :-p. It'd be a good time to simplify the type by making that parameter optional; what do you think?

sparrowhawk commented

2022-03-22 14:34:46 +00:00

Collaborator

Hmmm... Thing is, the unique identity of a key is the combination of domain/selector. So in effect, we can:

leave the object_id, the --selector and --domain independant
set the object_id to be the default selector, and make --selector optional
set the object_id to be the default domain, and make --domain optional

Looking at our cdist infrastructure, I have a case of each of the latter x) I can't find an argument for one or the other, so I'd just say look at how you use it, and if in your usage you almost always default selector to object_id, then go for making it optional with a default.

TL;DR: I don't think it's a Terrible Idea (TM) ;)

Hmmm... Thing is, the unique identity of a key is the combination of domain/selector. So in effect, we can: * leave the object_id, the `--selector` and `--domain` independant * set the object_id to be the default selector, and make `--selector` optional * set the object_id to be the default domain, and make `--domain` optional Looking at our cdist infrastructure, I have a case of each of the latter x) I can't find an argument for one or the other, so I'd just say look at how you use it, and if in your usage you almost always default `selector` to `object_id`, then go for making it optional with a default. TL;DR: I don't think it's a Terrible Idea (TM) ;)

evilham commented

2022-03-22 16:28:27 +00:00

Author

Collaborator

I think you are totally right and both things make sense, so this seems like the most sensible thing to me:

Both --selector and --domain are "optional" parameters for the type
If and only if --selector and --domain are absent and __object_id is in form DOMAIN/SELECTOR, we use those values and Normalized__object_id.private (*) for the key file name
If both --selector and --domain are absent and __object_id is not in form DOMAIN/SELECTOR, that's an informative error
If only one of --selector or --domain are absent, that's an informative error
If both --selector and --domain are used, that's what we use and Normalized__object_id (*) for the key file name

(*) Where Normalized__object_id means something like s!/!_!g on __object_id

So, I guess most of the time we could use __opendkim_genkey DOMAIN/SELECTOR and still consistently support other use-cases.

Thoughts? :-)

I think you are totally right and both things make sense, so this seems like the most sensible thing to me: - Both `--selector` and `--domain` are "optional" parameters for the type - If and only if `--selector` and `--domain` are absent and `__object_id` is in form `DOMAIN/SELECTOR`, we use those values and `Normalized__object_id.private` (*) for the key file name - If both `--selector` and `--domain` are absent and `__object_id` is not in form `DOMAIN/SELECTOR`, that's an informative error - If only one of `--selector` or `--domain` are absent, that's an informative error - If both `--selector` and `--domain` are used, that's what we use and `Normalized__object_id` (*) for the key file name (*) Where `Normalized__object_id` means something like `s!/!_!g` on `__object_id` So, I guess most of the time we could use `__opendkim_genkey DOMAIN/SELECTOR` and still consistently support other use-cases. Thoughts? :-)

sparrowhawk commented

2022-03-22 16:45:58 +00:00

Collaborator

Yep, awesome. If you have the time to implement this, I'll happily review and test it :)