On 2021-03-18 at 22:03:35, Albert Cui via GitGitGadget wrote: > +User Goals / Critical User Journeys > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > + > +* As a repository owner / project maintainer, > + > + ** I want to enforce code style so I require developers to use a > + standardized linter. This sounds like you're proposing pre-commit hooks for this purpose. Let me quote you this passage from the FAQ that I wrote that explains my position perfectly: [S]ome advanced users find `pre-commit` hooks to be an impediment to workflows that use temporary commits to stage work in progress or that create fixup commits, so it's better to push these kinds of checks to the server anyway. It also outlines that pre-commit hooks are not an effective control and for effective implementation, this must live on the server. I can never imagine myself wanting to use a pre-commit hook for these reasons. It would interrupt my typical workflow which uses many, many fixup and squash commits and near constant rebases. Similarly, a pre-push hook prevents me from pushing up in-progress code to a remote so my colleagues and other interested parties can follow my progress, contribute design feedback, and, if I need to be out of the office for a while, pick up my work and continue it. > + ** I want to prevent leaks / share code widely so I check that developers > + are uploading code to the right place. This is also not an effective control. Any user can bypass this trivially, so there's little point in shipping a hook that does this. > + ** I want this to just work for all the developers in my repository, without > + needing to support them through configuration. Unfortunately, in most cases, what hooks are to be used and whether they should be used is a developer choice, and developers should be able to use or not use hooks much the same way as they choose an editor. Thus, a proposal that a project maintainer should specify a set of hooks to be used on a developer machine is much like that maintainer proposing that a developer use a specific editor. While there are situations where that's done, in general, it's not a best practice to do so, so I'm generally negative on the idea of automatically deploying hooks to developer machines. I think doing so is a fundamental mistake in the developer process in most circumstances. Certainly there are exceptions, but I think they are so few that the burden of running a script is very minor in comparison. > +Design Principles > +~~~~~~~~~~~~~~~~~ > + > +* *Make it easy:* Developers just want to get their work done. Introducing > +friction is bad, especially when it prevents development from even starting e.g. > +workspace set up. I think this assumes that developers need these hooks to be productive, and I'm not in agreement with that assumption. For the reasons I've outlined above, I think that's a misunderstanding about best practices and how codebases should be structured. > +* *Treat hooks as software, not configuration:* We take seriously the > +responsibility that comes with causing arbitrary code to run on a user's > +machine. Such code needs to come from a reputable source, be automatically > +updated, and run with user consent. I don't think we can securely automatically update hooks from a remote source. That sounds like a terrible idea from a security perspective. If users really want this, they can add a post-checkout hook to do so. > +Feature Requirements > +* Users must explicitly approve hook execution at least once (yes/no) > + > + ** This could happen during setup or at execution time > + > + ** When a hook command changes, a user should re-approve execution (note: > + implementation should not interfere with requirement listed in “Fast > + Follows") > + > + * Users do not need to run setup scripts to install hooks --- the setup flow > + happens automatically at clone time This sounds like you're proposing automatically installing hooks, which would of course be a security vulnerability. If that's not what you're proposing, then perhaps you'll want to rephrase this point. > +* Automation is able to continue to use clone and other commands > +non-interactively > + > +* Works across Windows/Linux/macOS Git supports other platforms as well. > +Implementation Exploration: Check "magic" branch for configs at fetch time > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + > +Example workflow > +^^^^^^^^^^^^^^^^ > + > +* Perform fetch as normal > + > +* After fetch is complete, Git checks for a "magic" config branch (e.g. > ++origin/refs/recommended-config+) which contains information about config lines > +an end-user may want (including hooks). As I explain further below, shipping config is a security problem. > +* As part of the fetch subcommand, Git prompts users to install the configs > +contained there. I don't want to be prompted to install hooks, because I never want to use hooks from a repository like this. Moreover, how could a user possibly make an educated decision? They can't actually see the hooks at this point because no checkout has occurred, so the user must make a leap of faith based on the purported reputation of the repository owner. In similar situations, such as piping curl to bash, the recommendation is always to download the script manually, inspect it for any problems or vulnerabilities, and only then execute it. This proposal doesn't allow that. > + ** User responses to that prompt could be "sticky" - e.g. a user could reply > + "no (this time)", "no (never)", "yes (this time)", or "yes (always)". > + Always/never indicate that the user trusts the remote this config is coming > + from, and should not apply to configs fetched from other remotes. "Yes (always)" is almost certainly a security problem. > +Future Work > +~~~~~~~~~~~ > + > +* Extending this to allow repository owners to specify specific configurations > +in general e.g. this repository should use partial-clone with these parameters. I don't think any sort of automatic configuration is going to be secure. Git has had numerous vulnerabilities with the .gitmodules file and it is highly restricted in its contents. So I'm pretty much completely opposed to this since I think it's likely to result in Git becoming a CVE magnet. Even if the entire rest of the project disagreed with me and adopted this, the config would need to be restricted to an allowlist, not a denylist, because we have good and sufficient evidence that the latter is not secure. Even things as simple as changing diff colors can cause a security problem (e.g., hiding malicious code in a code review), so I'm having trouble imagining how we could come up with more than a tiny set of options that could be legitimately shipped securely, at which point the extra maintenance and development burden becomes excessive. > +* A way to perform general set up of a code base e.g. installing dependencies, > +any first-build related tasks There are already a variety of proposals for this, such as https://github.com/github/scripts-to-rule-them-all. I don't believe Git needs to be in the business of doing this, and all of my concerns about security are appropriate here as well. In general, I have grave concerns about this proposal. It makes assumptions that users should or will want to use the repository owner's hooks, which I disagree with. I think prompting users to install hooks will encourage them to be much more likely to do so, especially for less skilled users, which is likely to lead to social engineering attacks. And overall, this proposal sounds like it does not give enough consideration to the giant security risks it introduces. I don't think this proposal as it stands should be accepted, and I am having a hard time thinking of any similar proposal I could support. I appreciate that this proposal does make things slightly easier for a subset of users and environments, but I think picking a standard set of scripts for those users and environments and letting users execute them if and when they want is only slightly more difficult and much more secure. I've used that approach at work and it works great, so I feel confident it can work in most other environments as well. -- brian m. carlson (he/him or they/them) Houston, Texas, US
Attachment:
signature.asc
Description: PGP signature
