Create a synthetic authz resource for trust quorum and use it#9946
Create a synthetic authz resource for trust quorum and use it#9946andrewjstone wants to merge 5 commits intomainfrom
Conversation
andrewjstone
force-pushed
the
authz-tq
branch
from
26c6640 to
c4c8aa9
Compare
davepacheco
left a comment
davepacheco
left a comment
There was a problem hiding this comment.
This looks good. Some suggestions below, none of them critical.
| if action == Action::Read { | ||
| return self.not_found(); | ||
| } | ||
|
|
||
| // If the user failed an authz check, and they can't even read this | ||
| // resource, then we should produce a 404 rather than a 401/403. | ||
| match authz.is_allowed(&actor, Action::Read, self) { | ||
| Err(error) => Error::internal_error(&format!( | ||
| "failed to compute read authorization to determine visibility: \ | ||
| {:#}", | ||
| error | ||
| )), | ||
| Ok(false) => self.not_found(), | ||
| Ok(true) => error, | ||
| } |
There was a problem hiding this comment.
I don't think this behavior is wrong but it feels wrong that you should have to duplicate it here. Even though it's short, it's pretty tricky and load-bearing.
I wonder if it would make sense (or even work) to call self.rack().on_unauthorized(authz, error, actor, action), since logically you're trying to match the rack's behavior.
The other obvious option would be to impl ApiResource but I guess that doesn't make sense here because there's no ResourceType for it and we don't want one?
There was a problem hiding this comment.
Yeah, I thought about implementing ApiResource but that seemed to contradict the whole synthetic resource thing.
I'll try calling self.rack().on_unauthorized. I didn't think of that.
There was a problem hiding this comment.
This seems to have worked. Thanks!
| fn not_found(&self) -> Error { | ||
| // The information that we are preventing from leaking is anything | ||
| // having to do with a given rack. | ||
| LookupType::ById(self.0.id()).into_not_found(ResourceType::Rack) |
There was a problem hiding this comment.
What about self.rack().not_found() ?
There was a problem hiding this comment.
The need for this actually goes away when calling self.rack().on_unauthorized() below.
| @@ -1014,17 +990,9 @@ impl DataStore { | |||
|
|
|||
| // Unconditional insert | |||
There was a problem hiding this comment.
Add a comment that the caller must have already authorized this?
There was a problem hiding this comment.
This is true for all _conn functions in this file. I'm going to add a header comment about that up top, which I should have already done.
|
|
||
| if !acked.is_empty() { | ||
| // Write state back to DB | ||
| let authz_tq = authz::TrustQuorumConfig::new(authz::Rack::new( |
There was a problem hiding this comment.
I see about 15-20 uses of this. It seems worth writing a helper authz::TrustQuorumConfig::for_rack_id(RackUuuid).
| rack_id: RackUuid, | ||
| epoch: Epoch, | ||
| ) -> Result<Status, Error> { | ||
| let authz_tq = authz::TrustQuorumConfig::new(authz::Rack::new( |
There was a problem hiding this comment.
It looks like this value ultimately comes from datastore.tq_get_all_active_rack_id_and_latest_epoch(). You could consider having this thing return authz::TrustQuorumConfig objects instead of RackUuids. There are other places where the database lookup returns the authz object (like LookupPath) so that callers don't have to think about wrapping stuff in this way. It's definitely not a big deal either way.
There was a problem hiding this comment.
Makes sense. I'll give it a go.
There was a problem hiding this comment.
Actually, this returns a BTreeMap keyed by the rack_id. I don't think it makes much sense to make authz types sortable and so I think I'll just leave it for now. This will be less painful looking at the three call sites here with the new helper anyway.
| epoch: Epoch, | ||
| coordinator: &BaseboardId, | ||
| ) -> Result<sled_agent_client::Client, Error> { | ||
| let rack_id = RackUuid::from_untyped_uuid(authz_tq.rack().id()); |
There was a problem hiding this comment.
(probably not actionable now)
I went to look at why we still need the rack_id here and I see we're calling datastore.sled_get_commissioned_by_baseboard_and_rack_id(). I think if we applied the same pattern you're applying in this PR to racks themselves, that function would accept an authz::Rack instead of a rack id and then this code (and presumably other code here) would have less stuff like this. But we haven't done this for racks yet and there's no reason to do it right now.
There was a problem hiding this comment.
Makes sense, and happy not to do it in this PR :)
|
@davepacheco All cleaned up in c36066f. |
2 participants
Closes #9749