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
Use the hash modulo in the derivation outputs #4348
Conversation
A few notes on that:
|
if (!goal.isAllowed(info.id.drvPath)) | ||
throw InvalidPath("cannot register unknown drv output '%s' in recursive Nix", printStorePath(info.id.drvPath)); | ||
// XXX: Should we check for something here? Probably, but I'm not sure | ||
// how |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably registerDrvOutput
shouldn't be allowed in RestrictedStore
at all, since the caller is completely untrusted and supposed to be sandboxed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess once we take the time to properly implement this, we should allow it if the drv output matches an allowed path, but that's complicated to check as we only know its hash modulo and not the drv path. So I'll just forbid everything for now
if (!goal.isAllowed(id.drvPath)) | ||
throw InvalidPath("cannot query the output info for unknown derivation '%s' in recursive Nix", printStorePath(id.drvPath)); | ||
// XXX: Should we check for something here? Probably, but I'm not sure | ||
// how |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
queryRealisation()
should only succeed in RestrictedStore
if DrvOutput
refers to a derivation that the client added (via addToStore
) and where it has built the output (i.e. has done a buildDerivation()
). Otherwise it can leak info to which the sandboxed build isn't supposed to have access.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed… like #4348 (comment) I'll just restrict everything for now as it's slightly tricky to implement and we might change it eventually anyways.
I've opened #4353 to track it
Needs to double-check that this doesn't change the daemon protocol for |
2557e8a
to
3ac9d74
Compare
Rather than storing the derivation outputs as `drvPath!outputName` internally, store them as `drvHashModulo!outputName` (or `outputHash!outputName` for fixed-output derivations). This makes the storage slightly more opaque, but enables an earlier cutoff in cases where a fixed-output dependency changes (but keeps the same output hash) − same as what we already do for input-addressed derivations.
There's currently no way to properly filter them, so disallow them altogether instead.
d93fe07
to
e9b39f6
Compare
This isn't the case, |
🎉 All dependencies have been resolved ! |
PRs #4370 and #4348 had a bad interaction in that the second broke the fist one in a not trivial way. The issue was that since #4348 the logic for detecting whether a derivation output is already built requires some logic that was specific to the `LocalStore`. It happens though that most of this logic could be upstreamed to any `Store`, which is what this commit does.
Rather than storing the derivation outputs as
drvPath!outputName
internally,store them as
drvHashModulo!outputName
(oroutputHash!outputName
forfixed-output derivations).
This makes the storage slightly more opaque, but enables an earlier
cutoff in cases where a fixed-output dependency changes (but keeps the
same output hash) − same as what we already do for input-addressed
derivations.
Depends on: #4330
Fix: #4347