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

fix(localMeta): Add delegated targets back to localMeta #384

Conversation

BaptisteFoy
Copy link
Contributor

Following ed6788e, delegated targets are not added in localMeta anymore. This PR adds them back in localMeta.

Types of changes:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Description of the changes being introduced by the pull request:
The regression introduced in the aforementioned commit means that the delegated targets have to be read from the remote store all the time, even outside of the update process. In case of a remote failure or a mismatch between local and remote store, the query process breaks down on every call.

In our case, as the remote store contains only the last version of a delegated targets, if it fails to be validated (due to a missing target file for example), we enter this state.

Please verify and check that the pull request fulfills the following requirements:

  • Tests have been added for the bug fix or new feature
  • Docs have been added for the bug fix or new feature

@BaptisteFoy BaptisteFoy force-pushed the fix/add-delegated-targets-to-localmeta branch from 5263f97 to e136a77 Compare September 14, 2022 13:21
@arbll
Copy link
Contributor

arbll commented Sep 14, 2022

Hey Folks 👋🏻 -- Ideally we'd like to have a backport of v0.3.0 + this PR. v0.3.3 would not work for us because v0.3.1 contains new features.

In case that's not possible we can probably use our own fork instead.

client/client.go Outdated Show resolved Hide resolved
client/client.go Outdated Show resolved Hide resolved
@BaptisteFoy BaptisteFoy force-pushed the fix/add-delegated-targets-to-localmeta branch from f05d0a6 to 6f8a65a Compare September 19, 2022 06:45
@trishankatdatadog trishankatdatadog force-pushed the fix/add-delegated-targets-to-localmeta branch from 6f8a65a to 7e326ed Compare September 20, 2022 17:24
Copy link
Member

@trishankatdatadog trishankatdatadog left a comment

Choose a reason for hiding this comment

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

Seems fine to me, but I want to hear from @rdimitrov whether he has any concern about adding this back. Thanks!

@trishankatdatadog
Copy link
Member

@BaptisteFoy was an intern at DD working on this, and he has temporarily left, but will be rejoining Remote Config full-time on Nov 15th. Will wait for him until then.

Baptiste: if you see this, no rush, but we are trying to wrap this up by Nov 30th. I'm sure we'll have enough time. Thanks in advance!

@BaptisteFoy BaptisteFoy force-pushed the fix/add-delegated-targets-to-localmeta branch 9 times, most recently from f569829 to c478700 Compare December 22, 2022 15:37
@trishankatdatadog trishankatdatadog force-pushed the fix/add-delegated-targets-to-localmeta branch from c478700 to 412626a Compare December 22, 2022 17:21
Copy link
Member

@trishankatdatadog trishankatdatadog left a comment

Choose a reason for hiding this comment

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

Great work, thanks very much, @BaptisteFoy! Approving after Zoom discussion. Would be great if you could add the 2-3 clarifying comments we discussed earlier.

client/client.go Show resolved Hide resolved
client/client.go Outdated Show resolved Hide resolved
client/client.go Show resolved Hide resolved
asraa
asraa previously approved these changes Jan 6, 2023
@trishankatdatadog
Copy link
Member

@BaptisteFoy can you pls fix DCO?

@BaptisteFoy BaptisteFoy force-pushed the fix/add-delegated-targets-to-localmeta branch from cfe757e to 367fe61 Compare January 6, 2023 15:28
Signed-off-by: Baptiste Foy <baptiste.foy@datadoghq.com>
Signed-off-by: Baptiste Foy <baptiste.foy@datadoghq.com>
…top metas

Signed-off-by: Baptiste Foy <baptiste.foy@datadoghq.com>
Adds verification of delegated targets when they go from the local
store to the localMeta structure.
The verification is done by selecting one target file per delegated
targets that verify at least one, then build the delegation path from
the top targets to that specific delegated file. Building the path is
already verifying every targets in the path, so the whole path is safe
to add to the localMeta. Doing so for every delegated targets lets us
verify every delegated targets that is used at least once.

Signed-off-by: Baptiste Foy <baptiste.foy@datadoghq.com>
Signed-off-by: Baptiste Foy <baptiste.foy@datadoghq.com>
Signed-off-by: Baptiste Foy <baptiste.foy@datadoghq.com>
@BaptisteFoy BaptisteFoy force-pushed the fix/add-delegated-targets-to-localmeta branch from 367fe61 to 45234ee Compare January 6, 2023 15:32
@asraa asraa merged commit 7e86441 into theupdateframework:master Jan 9, 2023
znewman01 pushed a commit to znewman01/go-tuf that referenced this pull request May 22, 2023
…mework#384)

* upgrade(localMeta): Verify delegated targets

Adds verification of delegated targets when they go from the local
store to the localMeta structure.
The verification is done by selecting one target file per delegated
targets that verify at least one, then build the delegation path from
the top targets to that specific delegated file. Building the path is
already verifying every targets in the path, so the whole path is safe
to add to the localMeta. Doing so for every delegated targets lets us
verify every delegated targets that is used at least once.

Signed-off-by: Baptiste Foy <baptiste.foy@datadoghq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

6 participants