Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: update smids and rmids on rotate #857

Merged

Conversation

kentbull
Copy link
Contributor

@kentbull kentbull commented Sep 5, 2024

This updates the database HabitatRecord smids and rmids properties when a group rotation happens.

Fixes #856

@kentbull kentbull changed the title feat: update smids and rmids on rotate fix: update smids and rmids on rotate Sep 5, 2024
@kentbull kentbull force-pushed the 856-fix-stale-smids-in-group-habs branch 3 times, most recently from 87ccea9 to 9b580d7 Compare September 6, 2024 19:47
@kentbull
Copy link
Contributor Author

It would seem as though doing a name check on BaseHab.save will prevent any updates because the name argument will already be populated in the self.db.names database and thus the self.db.names.get(keys=(ns, self.name)) will always return a value and raise the following value error:

class BaseHab:
    ...
    def save(self, habord):
        self.db.habs.pin(keys=self.pre,
                         val=habord)
        ns = "" if self.ns is None else self.ns
        if self.db.names.get(keys=(ns, self.name)) is not None:
            raise ValueError("AID already exists with that name")

        self.db.names.pin(keys=(ns, self.name),
                          val=self.pre)

It seems like the solution is to have some sort of BaseHab.update that allows updating an existing HabitatRecord only if the record already exists. I will submit that.

Copy link
Member

@pfeairheller pfeairheller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can reorganize the rotate method without introducing the hidden method and calling save. See my suggestion in Discord.

src/keri/app/habbing.py Outdated Show resolved Hide resolved
@kentbull kentbull force-pushed the 856-fix-stale-smids-in-group-habs branch 2 times, most recently from 0e5ceb7 to 44220b1 Compare September 10, 2024 21:00
This updates the database HabitatRecord smids and rmids properties when a group rotation happens.

Signed-off-by: Kent Bull <[email protected]>
@kentbull kentbull force-pushed the 856-fix-stale-smids-in-group-habs branch from 44220b1 to a754223 Compare September 10, 2024 21:02
@SmithSamuelM
Copy link
Collaborator

I am confused by what is happening here:

the habbing.GroupHab.rotate clearly updates the rmids and smids when it is called.


        if serder is None:
            return super(GroupHab, self).rotate(**kwargs)

        if (habord := self.db.habs.get(keys=(self.pre,))) is None:
            raise kering.ValidationError(f"Missing HabitatRecord for pre={self.pre}")

        # sign handles group hab with .mhab case
        sigers = self.sign(ser=serder.raw, verfers=serder.verfers, rotated=True)

        # update own key event verifier state
        msg = eventing.messagize(serder, sigers=sigers)

        try:
            self.kvy.processEvent(serder=serder, sigers=sigers)
        except MissingSignatureError:
            pass
        except Exception as ex:
            raise kering.ValidationError("Improper Habitat rotation for "
                                         "pre={self.pre}.") from ex

        self.smids = smids
        self.rmids = rmids
        habord.smids = smids
        habord.rmids = rmids
        self.db.habs.pin(keys=(self.pre,), val=habord)

        return msg

This called by JoinDoer.rotate which is called by joinDo when the rotation completes sucessfully. So unless there is bug in how the logic for JoinDoer.joinDo and what leads up to that. Then there should be no fix required. I am fearful that this is brute forcing a rotate but without completing the dependencies of verification that have to happen in order for it to be successful.

Since I did not write this code, I am not familiar enough to have a definitive opinion, but I am concerned, joinDo is waiting for all the group multisig events to occur, this seems to be the correct approach. Otherwise one would be brute forcing a rotate that actually was not approved by the group, and therefore one would have to have a way to rollback.

This is antithetical to the architecture of KERI in general. KERI is safe because it doesn't accept or change state until and unless the dependencies are met. It doesn't provisionally change state and then after the fact rollback. I understand that the latter is a common architecture for database based applications, but these are in general insecure, because of that. The former is the proper way to implement state changes that are secure. What this means is that a provisional state of waiting for depedencies to resolve and then possibly retrying interaction to refresh them when they time out or are unavailable is the correct approach. But the rmid, smid record is the state record not the provisional state record.

If the application logic requires provisional state then that is a different database that does not exist yet.

@SmithSamuelM
Copy link
Collaborator

SmithSamuelM commented Sep 10, 2024

A provisional source of truth is not the same as the primary source of truth. Escrows are an example of a provisional source of truth. But escrows are ephemeral so that are meant to buffer the asynchronous nature of faulty unreliable network transports. A non-ephemeral but provisional source of truth usually resides in its own sandbox in the application. This allows some transaction to live persistently attempting to enact a change in the primary source of truth, but that change to the primary source of truth requires the cooperation of other actors in the distributed application space. Thus all the dependencies must be met prior to updating the primary source of truth to reflect a desired provisional source of truth.

Unfortunately many applications builders do not like this extra complication but then they must implement roll backs. The problem is that in distributed applications, its virtually impossible to rollback state that has propagated downstream. Event sourcing is one approach to a universal rollback mechanism, where you delete the database and recreate it by replaying all the events, albeit with the bad events elided. While the KERI design has elements that feel like event sourcing, these do not exist at the application layer, and are not meant to protect from applications that brute force changes in key state or even application state such as the multi-sig group aid state (rmids and smids) Habord record.

Indeed in KERI a memoryless replay of Key Events is called a deletion or partial deletion attack (dead attack). If I have no memory of first seen events, then any compromises of stale key state can be used to create an alternate history that puts the identifiers under the control of the compromiser. So a true rollback is memoryless in that you can roll back to the genisis (inception event) and then replay with an alternate revised set of events. Hence we never want memoryless rollback, which means we don't want rollback, because by definition rollback must be memoyrless or its not rolling back. Memoried rollback is not a thing. Idempotent replay is a thing and that is what KERI supports. Idempotent replay allows distributed verification without trusted third parties. It allows for high availablity and provides a way to detect partial deletion attacks on some of your infrastructure.
The important property is that at least one node has to have memory so you can detect that the other nodes were compromised. If all nodes lose memory, then you can have a true rollback, and refresh to a different state with different events. This is a bad idea for security as you have just built your source of truth on a foundation of sand.

What KERI does instead of rollback, it to allow limited recovery, where nothing is forgotten but events can be disputed or superseded and the differences are reconciliable to any validator. But recovery is relatively limited becasue reconciliation would become unmanageable. So KERI goes to great lengths to ensure it was right the first time instead of just brute forcing and then rolling back to fix mistakes.

So when things don't "work" its usually because a required dependency was not met, and the solution is not to ignore the required dependency but to figure out why the dependency was not met and then figure out how to make that dependency more reliably satisfied, not to ignore the dependency.

@kentbull
Copy link
Contributor Author

kentbull commented Oct 2, 2024

Can this be merged now given our offline chat discussing that the smids and rmids are desired state, not final state?

@pfeairheller pfeairheller merged commit c2eef91 into WebOfTrust:v1.1.18 Oct 4, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants