Add proposal for agent-local revocation notification #54
Conversation
ueno
force-pushed
the
wip/dueno/alt-rev
branch
4 times, most recently
from
e66300a
to
02df08a
Compare
| - `msg` (_string_) - argument passed to the revocation actions | ||
| - `signature` (_string_) - signature calculated over `msg`, using RSA-PSS with SHA-256 as the hash algorithm and the maximum salt length for the RSA key, in base64 format | ||
|
|
||
| In the `cloud_verifier` section of the configuration file, a new |
There was a problem hiding this comment.
Unifying and simplifying the revocation configuration is a good idea.
With this change we should probably generalize the revocation mechanism such that we can easily add new ones.
There was a problem hiding this comment.
After second thought, the new configuration option might not be necessary for agent-local revocation (i.e., the verifier always notifies the agent through the REST API and ignores 404 from unsupported agents).
There was a problem hiding this comment.
I think we should have the option to either handle it via 0mq or REST because how do we handle if the an agent receives a message twice?
There was a problem hiding this comment.
Right now we have no guarantees about revocation message reception, nor guarantees that it will only happen once. I think we should formalize that now and document that custom revocation actions should be idempotent, so it won't matter how many times the same message is received.
There was a problem hiding this comment.
I kind of lean toward less configuration and more Keylime figuring out the right thing. This will allow people to mix/match as a cluster is reconfigured or upgraded. Is there a way to send to the REST API first and then fall back to zeroMQ for that agent if it fails?
|
|
||
| Consider the following in developing an upgrade/downgrade strategy for this enhancement | ||
| --> | ||
| To upgrade/downgrade, the configuration file needs modification if it |
There was a problem hiding this comment.
Maybe we should start to provide a upgrade script for the configuration, so that we can rename and change options without the user manually changing the configuration.
There was a problem hiding this comment.
This is kind of similar to db migrations, but only really works when we have schema versions. How do we do this for configurations that aren't tied to a particular version? I think the best we can do is continue to use the deprecated options and warn about them. If the new option is ever used in conjunction with the old we error and quit, but otherwise providing a warning message about how to use the new options is good enough.
Practically I don't see this as much of an issue since we don't have a formal release with webhook notification as a feature.
There was a problem hiding this comment.
In this case because we haven't had a release including that feature I would just rename/restructure without keeping the old options around because I think we (Lernstick) are the only ones that are using them currently.
For how long do we want to keep the old options around? We should probably write a policy on how new options are added. I agree with you that warning is currently the best option, but I don't really like that this forces all new options to be added as optional parameters.
Maybe we should start to include a version number in the configuration files, so that we at least have the change to do auto migrations.
ueno
force-pushed
the
wip/dueno/alt-rev
branch
from
02df08a
to
bfa8edc
Compare
| - `msg` (_string_) - argument passed to the revocation actions | ||
| - `signature` (_string_) - signature calculated over `msg`, using RSA-PSS with SHA-256 as the hash algorithm and the maximum salt length for the RSA key, in base64 format | ||
|
|
||
| In the `cloud_verifier` section of the configuration file, a new |
There was a problem hiding this comment.
Right now we have no guarantees about revocation message reception, nor guarantees that it will only happen once. I think we should formalize that now and document that custom revocation actions should be idempotent, so it won't matter how many times the same message is received.
| - `msg` (_string_) - argument passed to the revocation actions | ||
| - `signature` (_string_) - signature calculated over `msg`, using RSA-PSS with SHA-256 as the hash algorithm and the maximum salt length for the RSA key, in base64 format | ||
|
|
||
| In the `cloud_verifier` section of the configuration file, a new |
There was a problem hiding this comment.
I kind of lean toward less configuration and more Keylime figuring out the right thing. This will allow people to mix/match as a cluster is reconfigured or upgraded. Is there a way to send to the REST API first and then fall back to zeroMQ for that agent if it fails?
|
|
||
| Consider the following in developing an upgrade/downgrade strategy for this enhancement | ||
| --> | ||
| To upgrade/downgrade, the configuration file needs modification if it |
There was a problem hiding this comment.
This is kind of similar to db migrations, but only really works when we have schema versions. How do we do this for configurations that aren't tied to a particular version? I think the best we can do is continue to use the deprecated options and warn about them. If the new option is ever used in conjunction with the old we error and quit, but otherwise providing a warning message about how to use the new options is good enough.
Practically I don't see this as much of an issue since we don't have a formal release with webhook notification as a feature.
|
Overall this proposal looks great. Just some minor grammar fixes and clarification about handling the deprecation and I think it's good to go. |
|
@ueno Any chance you can make the suggested changes? If so, we should be good to merge. |
Signed-off-by: Daiki Ueno <dueno@redhat.com>
ueno
force-pushed
the
wip/dueno/alt-rev
branch
from
bfa8edc
to
86bf36e
Compare
|
@mpeters thank you for the review; I've updated the PR to incorporate the suggestions, though I leave the config option as is for now. |
Signed-off-by: Daiki Ueno dueno@redhat.com