Ensure query only allows access to entries in holding list #1893
Conversation
| let entry_wh = test_entry_with_header(); | ||
| let entry = entry_wh.entry.clone(); | ||
|
|
||
| let store = reduce_hold_entry(&store.dht(), &ActionWrapper::new(Action::Hold(entry_wh))) |
There was a problem hiding this comment.
Had to rewrite this test to use proper mutations of the DHT store. This is a better way of doing things anyway.
| let _ = (*context.state().unwrap().dht()).clone().add(&entry); | ||
| let result = super::get_entry_from_dht(&context, &entry.address()); | ||
| assert_eq!(Ok(Some(entry.clone())), result); | ||
| // write this test when its easier to get a mutable dht state |
There was a problem hiding this comment.
This test is no longer an easy thing to write as it is impossible to get a mutable reference to the DhtStore and then put it back in the state. Rewriting this would require setting up the context to process actions and it doesn't seem worth it to test a one-line function that is tested higher up anyway.
willemolding
requested review from
lucksus,
zippy and
StaticallyTypedAnxiety
willemolding
changed the title
| @@ -317,15 +317,19 @@ impl DhtStore { | |||
|
|
|||
| impl GetContent for DhtStore { | |||
| fn get_raw(&self, address: &Address) -> HcResult<Option<Content>> { | |||
| Ok((*self.content_storage.read().unwrap()).fetch(address)?) | |||
| if self.holding_list.contains(address) { | |||
There was a problem hiding this comment.
I said yesterday in voice chat that I would be surprised (in a good way) if we can actually have this check here on this general level, as I was suspecting the query and fetch code to be assuming direct CAS access.
I still haven't checked this but I see both app-spec and stress-test fail and have this error message in the logs: Error getting entry aspects of authoring list: EntryIsPrivate
There was a problem hiding this comment.
I said yesterday in voice chat that I would be surprised (in a good way) if we can actually have this check here on this general level, as I was suspecting the query and fetch code to be assuming direct CAS access.
I still haven't checked this but I see both app-spec and stress-test fail and have this error message in the logs:
Error getting entry aspects of authoring list: EntryIsPrivate
Ah interesting I think this might need a small revisit. I saw the app spec pass locally and assumed it was just flakyness on the CI side. Its really close though!
# tests 179
# pass 177
# fail 2
There was a problem hiding this comment.
and passes on a rerun... Still no luck with the stress test though
There was a problem hiding this comment.
And this was repeatable locally as well. I didn't think to run the stress tests (I assumed if app spec passed so would they. Any idea what could be causing stress tests to fail but not app spec?
| .add(content) | ||
| .map_err(|e| e.into()) | ||
| (*self.content_storage.write().unwrap()).add(content)?; | ||
| self.mark_entry_as_held(content); |
There was a problem hiding this comment.
Ok further investigations show the error is caused by this line. Removing it and adding the call to mark_entry_as_held back to reduce_hold_entry makes the panic error stop. The assumption is that everything added to the DHT belongs on the hold list but there must be some exceptions to this (likely a Dna | CapTokenGrant | CapTokenClaim )
I don't think this code is at fault but rather it has revealed a fault in another part of the code base where something was added to the CAS via the DHT store that was not meant to be shared
|
Hey @willemolding, the PR is going to need to be harmonized with the work we've done over here in: #1904 |
PR summary
Follow on from #1876
Using the traits introduced above there is a single place we can add management of the DHT holding list. It is now self managing and the
mark_entry_as_heldcan be made private.Entries added to the DHT are now added to the holding list automatically and reads will return
Noneif an entry is not in the holding list rather than checking the CAS. This means less CAS reads and less chance of leaking private entries!changelog
documentation