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 trust-tarballs-from-git-forges setting #10340

Merged
merged 2 commits into from
Apr 2, 2024

Conversation

edolstra
Copy link
Member

Motivation

If enabled, GitHub flakerefs don't require a content hash, a Git revision is enough.

Fixes #10297.

Context

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@github-actions github-actions bot added the fetching Networking with the outside (non-Nix) world, input locking label Mar 27, 2024
If disabled, GitHub flakes are only considered locked if a
`narHash` attribute is specified,
e.g. `github:NixOS/patchelf/7c2f768bf9601268a4e71c2ebe91e2011918a70f?narHash=sha256-PPXqKY2hJng4DBVE0I4xshv/vGLUskL7jl53roB8UdU%3D`.
)"};
Copy link
Member

@Ericson2314 Ericson2314 Mar 27, 2024

Choose a reason for hiding this comment

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

This should be marked experimental (under fetch-tree perhaps?); I don't want to need to have it long term.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it should be experimental. It could be removed (made a no-op) in the future if it's no longer needed.

After all, what's the experimental behaviour? The "true" or "false" case?

Copy link
Member

@Ericson2314 Ericson2314 Mar 27, 2024

Choose a reason for hiding this comment

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

I wasn't there for the rest of the call Monday, but I consider this thing mostly a monkey patch because the other stuff for the fetchers hasn't been done yet.

In the end I think:

  1. When there are no submodules, everything should be trustlesss and ergonomic with a single hash. We record all the Merkle proofs we need.

  2. When there are submodules, the user should explicitly opt into fetching them or pruning them. And git vs github hardly matters because the real issue is not some service lying, but the fetched thing not corresponding to the hash used to fetch it with.

Copy link
Member

Choose a reason for hiding this comment

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

Why wouldn't it be experimental? It only makes sense as part of fetchTree anyways, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's already implicitly behind fetch-tree. But we have a bunch of settings (access-tokens, allow-dirty, warn-dirty, ...) that are also fetcher-specific but not marked as experimental.


If disabled, GitHub trees are only considered locked if a
`narHash` attribute is specified,
e.g. `github:NixOS/patchelf/7c2f768bf9601268a4e71c2ebe91e2011918a70f?narHash=sha256-PPXqKY2hJng4DBVE0I4xshv/vGLUskL7jl53roB8UdU%3D`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
e.g. `github:NixOS/patchelf/7c2f768bf9601268a4e71c2ebe91e2011918a70f?narHash=sha256-PPXqKY2hJng4DBVE0I4xshv/vGLUskL7jl53roB8UdU%3D`.
e.g. `github:NixOS/patchelf/7c2f768bf9601268a4e71c2ebe91e2011918a70f?narHash=sha256-PPXqKY2hJng4DBVE0I4xshv/vGLUskL7jl53roB8UdU%3D`.
This `narHash` is not guaranteed to be required in future versions.

Not a promise, which we should be careful about, especially in reference docs, but some expectation management is in order.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we have to document that requirements could be loosened in the future, since that wouldn't break anything.

@Ma27
Copy link
Member

Ma27 commented Mar 27, 2024

What makes github so special here?
Wouldn't it be better to let such a setting deal with all (or a user-defined selection) of the Git providers libfetchers currently support? I.e. GitLab and Sourcehut as well.

@edolstra
Copy link
Member Author

@Ma27 This setting does apply to all the GitHub-like fetchers, but I couldn't think of a better name for it.

@roberth
Copy link
Member

roberth commented Mar 27, 2024

better name

"Forge" would be the term.
Maybe trust-git-forge-tarball? That also narrows down the purpose of the option a bit.

If enabled, GitHub flakerefs don't require a content hash, a Git
revision is enough.

Fixes NixOS#10297.
@edolstra edolstra changed the title Add trust-github setting Add trust-tarballs-from-git-forges setting Mar 29, 2024
@Ma27
Copy link
Member

Ma27 commented Mar 30, 2024

This setting does apply to all the GitHub-like fetchers, but I couldn't think of a better name for it.

Ehh, should've taken a closer look I guess 😅
Anyways, it's good that it got renamed anyways 👍

I'm wondering though if it'd make sense to restrict the trust to a configurable list of hosts in the future though, e.g. just the org's GitLab. Just a suggestion though, doesn't necessarily need to be part of this PR I think.

@thufschmitt thufschmitt merged commit 29c3e4f into NixOS:master Apr 2, 2024
10 checks passed
@roberth roberth added the backport 2.21-maintenance Automatically creates a PR against the branch label Apr 2, 2024
Copy link

github-actions bot commented Apr 2, 2024

Successfully created backport PR for 2.21-maintenance:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.21-maintenance Automatically creates a PR against the branch fetching Networking with the outside (non-Nix) world, input locking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

builtins.getFlake regression, claims flakeref is unlocked when narHash not supplied
5 participants