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 initial / PoC gittuf remote handling protocol #411

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

adityasaky
Copy link
Member

@adityasaky adityasaky commented May 21, 2024

Edit 2024-05-24: I think the transport is at least at a proof of concept level now. Current status:

  • HTTP(s) and SSH remotes
  • Clone/fetch also updates refs/gittuf/ with what's on the remote
  • Push creates signed RSL entries for each ref being pushed
  • Clone has been tested across different repositories, including some "large" repositories such as the kernel

There are a bunch of TODOs and FIXMEs in the code. There's also a bunch of repeated code between the curl backend and the ssh backend that we can maybe clean up. Things that we also want to do:

  • On push, first check local RSL is updated and then create RSL entries
  • On push, support high traffic repositories where RSL entries may be pushed after they are fetched and before locally created entry is submitted
  • gittuf must support a mismatch in remote and local refs #413
  • On fetch, verify new RSL entries (there's some commented out code for this, we need some other changes)
  • Support local file backend (when the "remote" is available locally)

Play with it:

  • Fetch this branch
  • Run make install (it should also place git-remote-gittuf in your GOBIN, so this assumes that's in your PATH)
  • Clone using gittuf protocol; Eg: git clone gittuf::git@github.com:gittuf/gittuf for ssh, git clone gittuf::https://github.com/gittuf/gittuf for https
  • Proceed with your standard git push/pull/fetch operations
  • Ensure you have commit signing configured in Git config to sign RSL entries for pushes

h/t @SantiagoTorres for insights on how this works!

@adityasaky
Copy link
Member Author

I think this is ready for a first round of feedback!

@wlynch @JustinCappos @lukpueh @patzielinski @neilnaveen

@adityasaky
Copy link
Member Author

Note: as of 13e4a00, the transport doesn't work. I'm working on making it more resilient to different local conditions.

@adityasaky
Copy link
Member Author

Things work again (and are more robust than before). I also tested out some trivial force pushes / fetches and they work as well, though we will need #369 in some form to mark rsl entries for rebased comments as skipped.

@neilnaveen
Copy link
Collaborator

A feature that would work well with the transport, since it controls all pushes and pulls, is caching the latest verified RSL entry in a specific reference, allowing only the user's signing key to commit to it. This approach would significantly speed up verification. After the initial pull from the main repository, gittuf's impact on the user experience would be minimal, even with large repositories, as long as users pull from their remote regularly.

@adityasaky
Copy link
Member Author

Yeah, we definitely need that (#245). This and a couple other things make me wonder if we want to carve out refs/gittuf-local/ so that it doesn't get synced accidentally.

@adityasaky
Copy link
Member Author

adityasaky commented May 30, 2024

the code is still a mess and i want to work on cleaning things up, but the functionality seems fairly stable. This hasn't switched on gittuf verify-ref yet, I'm waiting on some other changes to land. In the meantime, seeking feedback and opinions on the best way to structure the curl and ssh helpers!

@lukpueh
Copy link
Contributor

lukpueh commented Jun 10, 2024

Impressive work, Aditya! Your code walkthrough the other day was very helpful, but I must admit, I still find it hard to navigate by myself.

Maybe you could try to better separate (1) main flow, (2) transport, and (3) gittuf logic.

IIUC (1) is controlled by the messages received from and sent to the git parent process, where stdin/stdout are the read and write message queues.

By (2) I mean the messages sent to and received from the http transport (curl) or ssh client (ssh) child processes spawned by us, with similar queues.

And finally, (3) would be the interposed gittuf logic this feature is actually about, i.e. fetching, verifying, creating and pushing additional refs.

It would be extremely helpful to see at one glance the expected messages in both directions and on both ends, and maybe even their typical sequence. If this is not possible in code, it could be done in documentation.

What could be done in code, would be to reduce the amount of nesting. In that regard, I have one specific question: Is the internal currentState variable necessary, or would the flow of messages be enough state, e.g. could the main loop have a single dispatch level based on the message content?

@adityasaky adityasaky force-pushed the gittuf-transport branch 2 times, most recently from 598c78f to fd99ef3 Compare June 28, 2024 16:38
@adityasaky adityasaky changed the title (WIP) Add gittuf remote handling protocol Add initial / PoC gittuf remote handling protocol Jun 28, 2024
@adityasaky adityasaky marked this pull request as ready for review June 28, 2024 16:40
Signed-off-by: Aditya Sirish <aditya@saky.in>
@adityasaky
Copy link
Member Author

adityasaky commented Jun 28, 2024

@lukpueh and I have discussed how to improve this package and make it more maintainable (see his comment above for the general improvements). I propose we do this iteratively, separately, and for now go ahead and merge this as an alpha implementation of the transport.

@wlynch @patzielinski @neilnaveen

Copy link
Collaborator

@patzielinski patzielinski left a comment

Choose a reason for hiding this comment

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

Since this doesn't affect people who don't explicitly opt-in to using the transport, I think we can indeed go ahead and merge this in. I can open an issue about this being in alpha and needing improvements.

Side note: we should also probably add something to the docs discussing/describing/detailing this.

Copy link
Collaborator

@neilnaveen neilnaveen left a comment

Choose a reason for hiding this comment

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

This looks good. I haven't run it; I have just one suggestion. Thank you!

+1 on adding docs detailing that this is an alpha feature, and also explaining the current functionalities it adds on top of git, maybe somthing like a list of commands that have additional calls to gittuf, and what those calls are/do.

Comment on lines +140 to +145
// var rslTip string
// cmd := exec.Command("git", "--git-dir", gitDir, "rev-parse", rsl.Ref) //nolint:gosec
// rslTipB, err := cmd.Output()
// if err == nil {
// rslTip = string(bytes.TrimSpace(rslTipB))
// }
Copy link
Collaborator

@neilnaveen neilnaveen Jun 28, 2024

Choose a reason for hiding this comment

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

Is this supposed to still be here, or is this something that should be uncommented later on?

Copy link
Member Author

Choose a reason for hiding this comment

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

the latter

@adityasaky
Copy link
Member Author

Suggestions on the best place to put the docs? We don't get nice auto-gen in docs/cli like with the main cmd package.

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.

None yet

4 participants