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

Add Rollback protection #405

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

flandweber
Copy link
Contributor

This adds rollback protection to the git fetcher.

Motivation

For the moment this merely serves the purpose of making sure no downgrades to running systems happen.
For example users could prohibit accidental Nixpkgs downgrades an thus prevent their server-deployment from receiving an unanticipated software downgrade.

However, when using commit verification such as guix git authenticate or git-verify (of which I'm the author), it is important to verify no rollback-attack is performed. As Nix' functional model can't deal with those itself I implemented them in Niv.

Copy link
Owner

@nmattia nmattia left a comment

Choose a reason for hiding this comment

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

Thanks a lot! I'm trying to understand everything that's happening here and have a couple questions. Haven't looked at that code in a while...

runGit :: [T.Text] -> IO [T.Text]
runGit args = do
-- TODO: only clone shallow repository and fetch needed commits to speed up verification
ensureAncestor :: T.Text -> T.Text -> T.Text -> IO T.Text
Copy link
Owner

Choose a reason for hiding this comment

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

I'm trying to wrap my head around what's happening here. If I understand --rollback-protection correctly, the goal is to prevent rolling back to an ancestor. So here you ensure that the old rev is an ancestor of the new rev.

Wouldn't that prevent a user from changing branches?

         o---o---o---B
        /
---o---o---o---A

If the user was on revision A, and changed the branch to B, wouldn't this implementation throw an error?

Would it make sense to invert the check and do something like this?

ensureNotAncestor ... = 
  exitCode <- readProcessWithExitCode' "git" ["merge-base", "--is-ancestor", newRev, oldRev]
  case ExitCode
    ExitSuccess -> fail "new rev should not be an ancestor"
    _ -> pure ()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it would prevent branch changes iff the new branch head is not an ancestor of the old rev.
I see that this might be surprising for users that 'just' want to switch branches.
Maybe an additional update flag --ignore-rollback-protection would make sense in these cases?

Could you give an example for when the inverted check would be sensible?
`ensureNotAncestor could for example succeed if I switched from a recent state to an old branch-off, but fail if the old rev was before the branching:

         o---o---o---C
        /
---A---o---o---o---B
niv update dep # A -> B
niv modify -b fork
niv update dep # B -> C, succeeds
niv modify -b fork
niv update dep # A -> C, fails

src/Niv/Git/Cmd.hs Show resolved Hide resolved
@flandweber flandweber force-pushed the rollback-protection branch 2 times, most recently from adfbb49 to 1085861 Compare June 25, 2024 15:14
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.

2 participants