TLS certificates should be Silo-scoped #3095
Conversation
| /// Synthetic resource describing the list of Certificates associated with a | ||
| /// Silo | ||
| #[derive(Clone, Debug, Eq, PartialEq)] | ||
| pub struct SiloCertificateList(Silo); |
There was a problem hiding this comment.
I know we've talked about this a little bit, but I just want to clarify... is the reason this needs to have a synthetic resource because it's permissions differ in some meaningful way from other silo resources or is it because you think all silo resources (or every resource) should have a synthetic resource?
There was a problem hiding this comment.
The obvious alternative would be directly checking "list children" on the Silo. Compared to that, a synthetic resource gives us two things: first what you said that the policy is different for certificates than for, say, projects (both of which are Silo children); and second: the policy ("a person can list the certificates in the Silo if they are an administrator of the Silo's parent fleet") can be separated from the code that checks it (which just asks "can this person list the certificates in the Silo"). This makes the code easier to write and verify (since you're not making any judgments about the policy there) and the policy is easier to write and verify too (since you're not worrying about implementation details there).
| "read" if "admin" on "parent_silo"; | ||
| "modify" if "admin" on "parent_silo"; | ||
| "create_child" if "admin" on "parent_silo"; | ||
| "list_children" if "admin" on "parent_silo"; |
There was a problem hiding this comment.
If the synthetic resource represents the list container, why does the base resource need create_child and list_children?
There was a problem hiding this comment.
It's probably not necessary. Removed in af6d6d5.
| "list_children" if "admin" on "parent_silo"; | ||
| "list_children" if "admin" on "parent_silo"; |
There was a problem hiding this comment.
Yup. Fixed in af6d6d5.
| diesel::insert_into(dsl::certificate) | ||
| .values(certificates) | ||
| .on_conflict(dsl::id) | ||
| .do_nothing() |
There was a problem hiding this comment.
Does this mean there would be no error?
There was a problem hiding this comment.
Yeah. The uuid was just generated, though, so it would be rather surprising if this happened. But I'll go ahead and remove that.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
Thanks for the review. It looks like I was fixing the /v1 thing as you were reviewing -- that should be fixed now. I'll address the rest in a bit. |
Fixes #2367 (see that issue for details).