Look for target in delegations

[lebauce]

When using delegations, a specific target file should be search among all the delegations targets

[trishankatdatadog]

Cc @asraa @ethan-lowman-dd @hosseinsia

[coveralls]

Pull Request Test Coverage Report for Build 1242331363

• 5 of 6 (83.33%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.03%) to 69.538%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
client/client.go	5	6	83.33%

Totals
Change from base Build 1221339773:	0.03%
Covered Lines:	1808
Relevant Lines:	2600

---

💛 - Coveralls

[asraa]

LGTM -- I think I convinced myself that even though getTargetFileMeta starts from the top-level targets, that c.Targets() is needed for verification consistency and loading the local metadata.

Could you please add a quick test for this?

[trishankatdatadog]

Seems redundant. Doesn't getTargetFileMeta already do the work of resolving form the top-level targets role first before delving into delegations? If so, why do we need to manually resolve in the top-level targets first using the old code (basically lines 719-727)?

[trishankatdatadog]

Seems redundant. Doesn't getTargetFileMeta already do the work of resolving form the top-level targets role first before delving into delegations? If so, why do we need to manually resolve in the top-level targets first using the old code (basically lines 719-727)?

Cc @raphaelgavache to confirm

[asraa]

There's some root/top-level verification that Targets() was doing that isn't done by the delegation iterator.

go-tuf/client/client.go

Line 266 in aee6270

func (c *Client) getLocalMeta() error {

[trishankatdatadog]

There's some root/top-level verification that Targets() was doing that isn't done by the delegation iterator.

go-tuf/client/client.go

Line 266 in aee6270

func (c *Client) getLocalMeta() error {

Boy, thanks, Asra, that was totally not obvious by just looking at the function. The current API has a lot of side fx (violates principle of least surprise) we should remove later, but for now, let's make sure we keep security while removing redundancy.

[trishankatdatadog]

@lebauce any updates?

[raphaelgavache]

nice catch, lgtm

[trishankatdatadog]

Was my comment resolved? Slyvain, please don't force-push next time, makes it hard to track what changed or didn't

[raphaelgavache]

@trishankatdatadog tests were added since your last review and the comment can be considered as resolved.
This Targets function allows in particular to get the custom of a Target from the matching targets.json file. I missed this very helpful function on the delegations PR

[trishankatdatadog]

@trishankatdatadog tests were added since your last review and the comment can be considered as resolved.
This Targets function allows in particular to get the custom of a Target from the matching targets.json file. I missed this very helpful function on the delegations PR

Yes, I understand and agree that getting the custom along with the targets metadata is useful.

What I disagree with is that my question has been resolved. Does everyone understand what I mean? getTargetFileMeta() is doing redundant work of checking the top-level targets role all over again. So why not just remove the existing code inside the Targets function for checking the top-level targets role?

There's some root/top-level verification that Targets() was doing that isn't done by the delegation iterator.

Hold on: are you telling me the current code for parsing delegations --- which starts from the top-level targets role anyway, according to my current understanding --- does not check for signatures from the top-level targets role?

Also, why do we need root/top-level (I'm assuming you mean root->timestamp->snapshot->targets role) verification here? Is that how the API works? That users can just call Targets() directly without explicitly calling some sort of verification procedure?

[raphaelgavache]

Also, why do we need root/top-level (I'm assuming you mean root->timestamp->snapshot->targets role) verification here? Is that how the API works?
This function fetches the validated top targets.json from the local store first, so if the searched TargetMeta is in it, it's a quick return. With the client delegations PR we left a similar first quick check in the Download func.
@lebauce actually uses it for partial ECU verifications when we use Uptane 2nd client verification.
There's definitely cleaner ways to do the ECU check but in the mean time this PR does not break the current behavior and allows to verify Delegations with Targets func too

That users can just call Targets() directly without explicitly calling some sort of verification procedure?

Yes similarly to the Download() function, Targets() can be called after an update to verify all top roles to extract a validated TargetMeta. When called the resolution is based by the locally stored snapshot

client.Update() // updates and verifies all top roles
verifiedTargetMeta <- client.Targets("file3.txt")
verifiedCustom <- verifiedTargetMeta.Custom

[asraa]

LGTM. I wonder if the comment for

// Targets returns the complete list of available targets.
func (c *Client) Targets() (data.TargetFiles, error) {

should clarify that this is top-level targets and

// Target returns the target metadata for a specific target if it
// exists. If it does not, ErrNotFound will be returned.
func (c *Client) Target(name string) (data.TargetFileMeta, error) {

searches target metadata through all delegations.
That would probably iron out some confusion?

Consider this non-blocking though.

[trishankatdatadog]

Why all the coverage failures for ubuntu?

[raphaelgavache]

Coveralls is having an outage: https://status.coveralls.io/ and it's failing on an http call to coveralls

  env:
    GOROOT: /opt/hostedtoolcache/go/1.13.15/x64
    pythonLocation: /opt/hostedtoolcache/Python/3.6.15/x64
    LD_LIBRARY_PATH: /opt/hostedtoolcache/Python/3.6.15/x64/lib
    COVERALLS_TOKEN: ***
bad response status from coveralls: 405
<html>
<head><title>405 Not Allowed</title></head>
<body>

I think it can be ignored given that the tests succeeded

[trishankatdatadog]

I think it can be ignored given that the tests succeeded

I'm sure it works, but let's wait until Coveralls is up again.