Fix b5 module digest calculation with transitive dependencies #3631
Conversation
This fixes the B5 digests calculation for remote Modules when a transitive dependency is no longer a declared dependency in the target ModuleSet. The digest calculation on Module.Digest would filter declared dependencies of the remote Module to those of the parents. On creation of bufmodule.NewModuleData the given getDeclaredDepModuleKeys must return all direct and transistive dependencies. All declared dependencies must be used when calculating the digest to ensure the digests match for the Module and ModuleData.
Remove the term declared from the dependencies in a ModuleData that refer to the full transitive list of dependencies, those in the buf.lock file. Declared dependencies are the direct dependencies, those in the buf.yaml file.
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
| } | ||
| moduleDepFullNames[fullName.String()] = struct{}{} | ||
| } | ||
| prunedDepModuleKeys := make([]ModuleKey, 0, len(declaredDepModuleKeys)) |
There was a problem hiding this comment.
This is the fix and the cause of the tamper-proof error. Dependencies should not be filtered to the context of the parent Module. This is required to avoid the Digest for the remote changing when included in a Module. If the parent Module A doesn't include dependency X but the remote module B depends on X, its dependency should still be used in the calculation of the Digest for B.
| // Dependencies are not required to be resolved to a Module to calculate the digest. | ||
| // Each dependent ModuleKey is a reference to a specific commit of a module | ||
| // that includes the dependencies expected Digest. |
There was a problem hiding this comment.
This comment was a little hard for me to parse. Maybe something like this?
The B5 digest is calculated based on the dependencies. We do not need to construct a Module to calculate a digest as we can trust that
DepModuleKeysis the complete set of deps for the Module and is exactly the deps that were present in the ModuleSet when this Module was pushed.
There was a problem hiding this comment.
I'm not sure trust is correct, as it is always validated if used. What about:
// The B5 digest is calculated based on the dependencies.
// The dependencies are not required to be resolved to a Module to calculate this Digest.
// Each ModuleKey includes the expected Digest which is validated when loading that dependencies ModuleData, if needed.
This fixes the b5 digest calculation for remote Modules when a transitive dependency is no longer a dependency in the parents ModuleSet. The digest calculation on Module.Digest would filter dependencies causing a ModuleData to fail tamper-proofing, as it would not filter it's dependencies. The documentation has been reworded to highlight the error case.
The ModuleData's method
DeclaredDepModuleKeyshas been renamed toDepModuleKeys. This clarifies the difference between declared dependencies, those in thebuf.yaml, and the full dependency list, those in thebuf.lock. The digest calculation must use the full list of dependencies, not the declared.This is similar to the fix proposed in #3190 but does not cache the digest calculation in ModuleData for reuse in the Modules digest calculation. A Modules digest will always be calculated locally.