identities: updated delegation causes replication failure #711

FintanH · 2021-06-21T16:30:23Z

We have the following steps:

Peer 1 creates a Person document
Peer 2 replicates this Person
Peer 1 updates Person document
Peer 1 creates Project delegating to new Person document
Peer 2 attempts to replicate the Project, but fails with error below

Error:

Revision 7bb4bbce268d89a62e1962f5dded7a9f68c9e665 of 4b84e8407d602317b01f22207908dbea1d9d4e17 not in ancestry path of eada5419806f643d402c92b790c9b471098f1e81

The error originates from updated_person, which seems to not consider the fact that the Person in the delegate is a fast-forward of the Person that Peer 2 knows about at the top-level. Instead, it only checks that the Person delegate was a previous version of the Person at the top-level.

Should this check be done for fast-forwards as well and do the following if it is?

self.as_person().verify(known.revision)

kim · 2021-06-21T18:14:14Z

I think this is related to #709, which, however, doesn't actually fix the root cause. At some point, it did work that a fetch would update the top-level person-typed namespace, such that the symref can never be ahead or behind. I'm not sure why or when this broke, but it is possible that we shouldn't rely on it anyways: there is still a TODO which punts on the problem that technically more top-level namespaces need to be updated, because the tracking graph may have changed.

A possible fix could thus be that #709 resets the top-level rad/id to the local remotes/../rad/self iff the latter can be fast-forwarded onto the former. I also think that the fetch turns any symrefs into direct refs if they get updated as a result of the fetch, so the symrefs need to be forced always.

FintanH · 2021-06-23T08:27:06Z

At some point, it did work that a fetch would update the top-level person-typed namespace, such that the symref can never be ahead or behind.

I think in this case because the rad/id of the Person is replicated before the Project it doesn't get a chance to create the symref, because it will fail the verification with the above error before it gets to the symref creation point.

Hmmm, actually it seems we've had a similar conversation before here: #420.

A possible fix could thus be that #709 resets the top-level rad/id to the local remotes/../rad/self iff the latter can be fast-forwarded onto the former. I also think that the fetch turns any symrefs into direct refs if they get updated as a result of the fetch, so the symrefs need to be forced always.

Ok, I can look into this on top of #709 :)

kim · 2021-06-23T10:17:36Z

I think in this case because the rad/id of the Person is replicated before the Project it doesn't get a chance to create the symref, because it will fail the verification with the above error before it gets to the symref creation point.

Ah yes... peer 2 already has the top-level namespace, that's why. We would need to do another roundtrip to first fetch everything in `rad/ids` in order to make sure that the top-level is up-to-date. That's annoying. I think what I proposed could still be viable -- after the `Peek`, the top-level can be reset to the corresponding `rad/ids/XXX` if and only if that'd be a fast-forward. This may render the identity invalid, however, so we would need to verify it before replicating it again (and not replicate if it became invalid). In this case, the project would also fail to verify -- which has dubious semantics. So I'm not sure. Maybe the correct thing to do is to only verify the project against the state of `rad/ids`. If both `rad/namespaces/XXX/refs/rad/id` and `refs/rad/ids/XXX` verify and are reachable from each other, the former can be reset to the more recent one, and the latter turned into a symref. If the latter verifies, but the former doesn't, then the project is in some kind of limbo state.

FintanH · 2021-06-24T13:09:00Z

librad/src/identities/git.rs

        // Nb. technically we could coerce `known` into a `VerifiedPerson` if its
        // `content_id` equals `latest_head`. Let's not introduce an unsafe
        // coercion, but rely on caching to be implemented efficiently.
        if self.is_in_ancestry_path(latest_head, known.revision.into())? {
            self.as_person().verify(latest_head)
+        // If the latest is a fast-forward then we can update the top-level
+        } else if is_fast_forward(&known, latest_head)? {


I'm not sure about the placement of this, but I was struggling to find where the logic could go. What do you think @kim?

Into replication.

I said here that it is probably a mistake to fail verification if top level delegates fail verification. The reason is this:

Imagine you create that project with valid delegates, which are then "checkpointed" as rad/ids/*. Now, one of them creates an invalid revision after this checkpoint. If the project fails verification by auto-resolving to the top-level, it can never recover from that (eg. by removing the faulty delegate), because we never replicate it again.

To solve this, we'd need to allow passing the find_latest_head function to git::identities::project::verify, and make it resolve to rad/ids/<urn.id> on the first pass. After replication is done doing its thing, we run verification again, this time resolving to refs/namespaces/<urn.id>/rad/id, and return a different value from replicate if that fails.

xla

Respect your elders.

🔒 🛫 🏃 🏙

test/src/test/integration/librad/scenario/updated_delegate.rs

kim · 2021-06-30T11:29:26Z

librad/src/git/identities/person.rs

@@ -189,6 +189,21 @@ where
    Ok(verified(storage).newer(a, b)?)
 }

+pub fn fast_forward<S>(


I get how you're guided by the types, but you end up with a lot of IO:

see if known exists

load and verify known

do ancestry check between known and latest

update ref

There is also a correctness problem: the reason for introducing this is to reset refs/namespaces/XXX/rad/id to refs/namespaces/YYY/rad/ids/XXX iff the latter can be fast-forwarded onto the former. However, this must be a fast-forward in the git sense, not the tree history sense, otherwise YYY can rewrite the history of XXX!

(No, we cannot actually prevent this entirely without a blockchain, but it is surely safer to disallow history rewrites on identity branches wherever possible)

So I think this can be simplified to a function which takes a VerifiedPerson and:

Looks up the oid of the rad/id(ie. refname_to_id(RefLike::try_from(pers.urn().with_path(None))?))

Calls graph_descendant_of(pers.content_id, canonical_oid)

If true, reset the ref from 1. to pers.content_id

Nb.: VerifiedPerson is already verified (qed), so if 1. and 2. pass we have also proven validity of the top-level person.

Thanks for the help, that makes more sense. Hopefully, the simplified version is right 🤞

kim · 2021-06-30T11:31:15Z

librad/src/git/replication.rs

-                    let proj = identities::project::verify(storage, &rad_id)?
-                        .ok_or(Error::MissingIdentity)?;
+                    let proj = identities::project::verify_with(storage, &rad_id, |delegate| {
+                        let refname =


It is a bit hard to see from the code how the lookup functions are different for the various invocations. Do you think this could be extracted out somehow (maybe curried function "constructors" or something)?

FintanH · 2021-08-09T09:51:02Z

Is there anything blocking this @kim? Or is it a matter of you working on replication v3?

We have the following steps: * Peer 1 creates a Person document * Peer 2 replicates this Person * Peer 1 updates Person document * Peer 1 creates Project delegating to new Person document * Peer 2 attempts to replicate the Project, but fails with error below Error: ``` Revision 7bb4bbce268d89a62e1962f5dded7a9f68c9e665 of 4b84e8407d602317b01f22207908dbea1d9d4e17 not in ancestry path of eada5419806f643d402c92b790c9b471098f1e81 ``` Signed-off-by: Fintan Halpenny <[email protected]>

We introduce verify_with for projects, which allows the caller to specify where the tips of the delegates are found. We use this in replication to verify the projects via their rad/ids/<urn>. Signed-off-by: Fintan Halpenny <[email protected]>

When a delegate's rad/id already exists we check that the rad/ids/<urn.id> for the project being replicated is a fast-forward for rad/id. If that's the case we can safely update the rad/id to the latest tip. The `fast_forward` function works as follows: 1. Get tip of rad/id 2. Check the verified person's tip is a descendant of 1. 3. If so, update rad/id Signed-off-by: Fintan Halpenny <[email protected]>

FintanH force-pushed the finto/updated-delegation branch from 9b6d685 to 46b943d Compare June 24, 2021 13:06

FintanH changed the base branch from master to finto/rad-id-symref June 24, 2021 13:06

FintanH force-pushed the finto/updated-delegation branch from 46b943d to 4ecd47c Compare June 24, 2021 13:07

FintanH commented Jun 24, 2021

View reviewed changes

FintanH force-pushed the finto/updated-delegation branch from 4ecd47c to d5f1af2 Compare June 25, 2021 14:08

FintanH force-pushed the finto/rad-id-symref branch from 723a08b to 39cd7fb Compare June 25, 2021 14:11

FintanH force-pushed the finto/updated-delegation branch from d5f1af2 to 6a1fb60 Compare June 25, 2021 14:12

FintanH marked this pull request as ready for review June 25, 2021 14:12

FintanH requested a review from a team as a code owner June 25, 2021 14:12

FintanH force-pushed the finto/rad-id-symref branch from 39cd7fb to a98218d Compare June 25, 2021 20:07

Base automatically changed from finto/rad-id-symref to master June 25, 2021 20:07

FintanH force-pushed the finto/updated-delegation branch from 6a1fb60 to 509c498 Compare June 25, 2021 20:08

xla previously approved these changes Jun 29, 2021

View reviewed changes

test/src/test/integration/librad/scenario/updated_delegate.rs Outdated Show resolved Hide resolved

FintanH dismissed xla’s stale review via 55e95f5 June 30, 2021 08:15

kim reviewed Jun 30, 2021

View reviewed changes

FintanH force-pushed the finto/updated-delegation branch 2 times, most recently from 72e04fc to fba6595 Compare July 1, 2021 14:37

FintanH force-pushed the finto/updated-delegation branch from fba6595 to fc011df Compare July 12, 2021 09:53

FintanH force-pushed the finto/updated-delegation branch from fc011df to db80379 Compare July 27, 2021 12:19

kim previously approved these changes Aug 9, 2021

View reviewed changes

FintanH dismissed kim’s stale review via b192331 August 9, 2021 14:33

FintanH force-pushed the finto/updated-delegation branch from db80379 to b192331 Compare August 9, 2021 14:33

FintanH added 3 commits August 9, 2021 16:03

identities: introduce verify_with

8c5650b

We introduce verify_with for projects, which allows the caller to specify where the tips of the delegates are found. We use this in replication to verify the projects via their rad/ids/<urn>. Signed-off-by: Fintan Halpenny <[email protected]>

FintanH force-pushed the finto/updated-delegation branch from b192331 to c01da9f Compare August 9, 2021 15:03

FintanH merged commit c01da9f into master Aug 9, 2021

FintanH deleted the finto/updated-delegation branch August 9, 2021 15:16

github-pages bot temporarily deployed to github-pages August 9, 2021 15:16 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

identities: updated delegation causes replication failure #711

identities: updated delegation causes replication failure #711

FintanH commented Jun 21, 2021

kim commented Jun 21, 2021

FintanH commented Jun 23, 2021

kim commented Jun 23, 2021 via email

FintanH Jun 24, 2021

kim Jun 24, 2021

xla left a comment

kim Jun 30, 2021

FintanH Jun 30, 2021

kim Jun 30, 2021

FintanH commented Aug 9, 2021

identities: updated delegation causes replication failure #711

identities: updated delegation causes replication failure #711

Conversation

FintanH commented Jun 21, 2021

kim commented Jun 21, 2021

FintanH commented Jun 23, 2021

kim commented Jun 23, 2021 via email

FintanH Jun 24, 2021

Choose a reason for hiding this comment

kim Jun 24, 2021

Choose a reason for hiding this comment

xla left a comment

Choose a reason for hiding this comment

kim Jun 30, 2021

Choose a reason for hiding this comment

FintanH Jun 30, 2021

Choose a reason for hiding this comment

kim Jun 30, 2021

Choose a reason for hiding this comment

FintanH commented Aug 9, 2021