Basic first-order delegation support #1074

pattivacek · 2019-01-29T15:34:58Z

Yes, it's really here! However, there are still open questions and more work to be done:

Nested delegations are not yet supported. It's probably not that much more work, but that wasn't my goal at this time.
This has been tested with aktualizr-repo but not with the real ota-tuf on the server.
tuf-test-vectors should really be updated with delegation cases.
I'm open to more unit test suggestions.
The delegation_basic test I wrote is mostly a copy of the full_no_correlation_id. Should these be combined?
There is some inconsistency in using Role objects vs. just their names. This could probably be improved.

Anything else missing or wrong?

codecov-io · 2019-01-29T16:28:16Z

Codecov Report

Merging #1074 into master will decrease coverage by 31.47%.
The diff coverage is 41.11%.

@@             Coverage Diff             @@
##           master    #1074       +/-   ##
===========================================
- Coverage   75.32%   43.84%   -31.48%     
===========================================
  Files         157      158        +1     
  Lines        9137     9258      +121     
===========================================
- Hits         6882     4059     -2823     
- Misses       2255     5199     +2944

Impacted Files	Coverage Δ
src/libaktualizr/storage/invstorage.h	`5.12% <ø> (-92.31%)`	⬇️
src/libaktualizr/primary/aktualizr.h	`5% <ø> (-95%)`	⬇️
src/libaktualizr/storage/sqlstorage.h	`50% <ø> (ø)`	⬆️
...libaktualizr/package_manager/packagemanagerfake.cc	`2.43% <0%> (-90.25%)`	⬇️
src/libaktualizr/uptane/imagesrepository.h	`0% <0%> (-100%)`	⬇️
src/aktualizr_secondary/aktualizr_secondary.cc	`4.25% <0%> (-18.33%)`	⬇️
src/libaktualizr/primary/sotauptaneclient.cc	`0.12% <0%> (-87.15%)`	⬇️
src/libaktualizr/storage/sqlstorage.cc	`2.31% <0%> (-71.59%)`	⬇️
src/libaktualizr/uptane/directorrepository.cc	`8.33% <0%> (-91.67%)`	⬇️
src/libaktualizr/uptane/imagesrepository.cc	`1.16% <0%> (-83.69%)`	⬇️
... and 81 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14ccc71...e51c532. Read the comment docs.

config/sql/migration/migrate.15.sql

src/libaktualizr/storage/invstorage.h

lbonn · 2019-01-30T10:52:35Z

src/libaktualizr/storage/sqlstorage.cc

+    return;
+  }
+
+  auto del_statement = db.prepareStatement<std::string>("DELETE FROM delegations WHERE role_name=?;", role.ToString());


I think it could be replace by a single "INSERT OR REPLACE"

Fair point. I copied the style of the NonRoot functions. Why don't those do the same?

meta has a UNIQUE(repo, meta_type, version) but the delete is DELETE FROM meta WHERE (repo=? AND meta_type=?);.

With the current schema, INSERT OR REPLACE would allow multiple versions to accumulate while the current storeNonRoot only keep the last updated version. To be honest, I'm not sure which would be the preferred semantics, knowing that loadNonRoot only takes the last version (with ORDER BY and LIMIT 1). @OYTIS, any input?

@lbonn Yes, the preferred semantics was to only keep the last version. Again ORDER BY and LIMIT are just for precaution, also known as defensive programming, which is not always considered a good style. We can delete them if you feel like it.

I'm not necessarily against defensive programming. Thing is, in this case the schema doesn't protect against multiple version. So should the unique constraint be only UNIQUE(repo, meta_type)? I find it weird that we keep this constraint in code only.

Ok so after discussion with @OYTIS, it results from the old behavior where different versions could accumulate. The schema migration from UNIQUE(repo, meta_type, version) to UNIQUE(repo, meta_type) was postponed because it requires a bit of care: only the last version should be migrated, otherwise it would trigger db conflicts.

src/libaktualizr/storage/sqlstorage.cc

src/libaktualizr/uptane/imagesrepository.cc

doanac · 2019-01-31T04:47:27Z

src/libaktualizr/uptane/imagesrepository.cc

-  if (it == targets.targets.cend()) {
-    return std::unique_ptr<Uptane::Target>(nullptr);
-  } else {
+  // Search for the target in the top-level targets metadata.


It might be nice (but I can try after this is merged) to turn this logic into an iterator. Then I think the logic of this function would become a std::find call to the new iterator

Sounds feasible. But just so you know, I'm expecting this particular function to get reworked again when we actually get around to supporting nested delegations. It may also get more complex if/when we support prioritization and/or TAP 3 (multi-role delegations).

lbonn · 2019-01-31T11:17:57Z

config/sql/migration/migrate.15.sql

@@ -1,3 +1,4 @@
+-- Don't modify this! Create a new migration instead--see docs/schema-migrations.adoc


Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Copied shamelessly from the full_no_correlation_id test. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Targets can also sign. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

This matches my understanding of the expected behavior for directory structures in target names. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Delegated targets do not have their hashes stored in their parent. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Based on clang-tidy's advice. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

* Simplify SQL logic for delegations. * Reduce if/for nesting for target searching. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

OYTIS · 2019-01-31T16:05:41Z

src/libaktualizr/primary/sotauptaneclient.cc

      last_exception = Uptane::ExpiredMetadata("repo", "targets");
      return false;
    }
  }
+
+  // Update Images delegated Targets Metadata
+  for (const std::string &delegation : images_repo.delegations("targets")) {


IIRC @heartsucker had an idea that the delegation tree can be huge and we only want to download what is relevant on this particular update. Also one of the drivers to add support for delegations now was the idea to save space/bandwidth on potentially huge targets.json for maps by splitting them by region.

So something like an optional (to also support tuf-only scenario) parameter specifying list of targets from director would be useful. Or as an alternative (and probably closer to the spirit of TUF spec) solution, we could only download delegations when intending to download a target or are explicitly asked to.

It may better be left for a next PR though.

I recall the same conversations and had similar concerns. For the moment, we've "avoided" the problem by only processing first-order delegations. On one hand, I'm not sure how likely it really is that we will have more than a couple layers of delegation in the automotive world, but on the other hand I think it makes sense to be flexible with our design goals and thus careful and efficient in our implementation.

I agree that we probably shouldn't download all delegations and instead only get what we need. However, that would require a fairly substantial refactoring and I wasn't willing to do it for this PR. Perhaps that should be a requirement for the nested delegation support, though.

@patrickvacek
I think these issues somewhat correlate, but we can also run into bandwidth problems with simple non-nested delegations. For the aforementioned example of region-based delegation for maps, these don't necessarily need to be nested.

Fair point. Sounds like a higher-priority chunk of work than nesting, then. It shouldn't be too hard, but since I'm unlikely to be able to get it done in the next week or so, would it be possible to merge this as it is and then do that as a follow-up? Others are welcome to contribute; I'm just a bit loathe to let this languish for a week while I'm away.

In other news, it sounds like the backend puts delegated_targets.json into a special delegations/ subdirectory in the images repo.

pattivacek · 2019-01-31T16:51:53Z

These Travis errors are pretty annoying. It's failing to download an Ubuntu package in Docker. Not related to my PR; every Travis run is currently failing in a similar manner.

By the way, I've gotten as far as testing this PR against the server to the point that aktualizr goes looking for the delegated_targets.json, but it gets a 404.

pattivacek requested review from patriotyk, OYTIS, lbonn and eu-siemann January 29, 2019 15:34

pattivacek force-pushed the feat/OTA-678/delegation branch from e583ca1 to 766d9db Compare January 29, 2019 16:28

pattivacek force-pushed the feat/OTA-678/delegation branch 2 times, most recently from 05618c6 to bca0cc5 Compare January 30, 2019 08:33

OYTIS reviewed Jan 30, 2019

View reviewed changes

config/sql/migration/migrate.15.sql Outdated Show resolved Hide resolved

src/libaktualizr/storage/invstorage.h Outdated Show resolved Hide resolved

lbonn reviewed Jan 30, 2019

View reviewed changes

src/libaktualizr/storage/sqlstorage.cc Outdated Show resolved Hide resolved

lbonn reviewed Jan 30, 2019

View reviewed changes

src/libaktualizr/uptane/imagesrepository.cc Outdated Show resolved Hide resolved

pattivacek force-pushed the feat/OTA-678/delegation branch from bca0cc5 to 98ab4c0 Compare January 30, 2019 11:32

doanac mentioned this pull request Jan 30, 2019

Aktualizr "Anonymous" Mode #1056

Closed

doanac reviewed Jan 31, 2019

View reviewed changes

pattivacek force-pushed the feat/OTA-678/delegation branch 2 times, most recently from 58b2cec to 161d95b Compare January 31, 2019 11:05

lbonn reviewed Jan 31, 2019

View reviewed changes

pattivacek force-pushed the feat/OTA-678/delegation branch from 161d95b to 3fe0697 Compare January 31, 2019 11:30

pattivacek added 11 commits January 31, 2019 13:57

WIP draft of basic delegation support.

0a7cd42

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Delegation support in SQL database.

9d2e195

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Const-correctness in storage and clang-tidy recommendations.

fda5515

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Fix checkImagesMetaOffline and delegation storage calls.

91e5388

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Better role error checking with tests.

8f2cd1a

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Refactor signing code from Root for use by Targets too.

d08c763

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Refactoring and removal of a redundant consistency check.

0092f6c

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Proper (non-nested) delegation searching.

b67bc57

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Basic delegation test.

e9290c8

Copied shamelessly from the full_no_correlation_id test. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Specify delegated role when parsing delegations.

f4d7d4e

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Refactor: use more generic "signer" instead of "root".

f76eff1

Targets can also sign. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

pattivacek added 7 commits January 31, 2019 13:57

Refactor to send Role into UnpackSignedObject.

8c6fb54

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Allow / to pass unchanged in encoded URLs.

7307be8

This matches my understanding of the expected behavior for directory structures in target names. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Only check hashes for top-level targets.json.

250435e

Delegated targets do not have their hashes stored in their parent. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Refactor reserved role names and unify IsReserved() function.

72d6b58

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

More TUF role testing. Update actions and changelog.

07e18ba

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Rename Delegated to Delegation.

508e22a

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Pass Roles by reference after all.

e655ae5

Based on clang-tidy's advice. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

pattivacek force-pushed the feat/OTA-678/delegation branch from 3fe0697 to f949868 Compare January 31, 2019 12:58

Fix review comments.

e51c532

* Simplify SQL logic for delegations. * Reduce if/for nesting for target searching. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

pattivacek force-pushed the feat/OTA-678/delegation branch from f949868 to e51c532 Compare January 31, 2019 13:45

OYTIS reviewed Jan 31, 2019

View reviewed changes

lbonn approved these changes Feb 1, 2019

View reviewed changes

lbonn merged commit 49bc10b into master Feb 1, 2019

lbonn deleted the feat/OTA-678/delegation branch February 1, 2019 11:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic first-order delegation support #1074

Basic first-order delegation support #1074

pattivacek commented Jan 29, 2019

codecov-io commented Jan 29, 2019 •

edited

Loading

lbonn Jan 30, 2019

pattivacek Jan 30, 2019

lbonn Jan 30, 2019 •

edited

Loading

OYTIS Jan 31, 2019

lbonn Jan 31, 2019

lbonn Feb 1, 2019

doanac Jan 31, 2019

pattivacek Jan 31, 2019

lbonn Jan 31, 2019

OYTIS Jan 31, 2019

pattivacek Jan 31, 2019

OYTIS Jan 31, 2019

pattivacek Feb 1, 2019

pattivacek commented Jan 31, 2019

		@@ -1,3 +1,4 @@
		-- Don't modify this! Create a new migration instead--see docs/schema-migrations.adoc

Basic first-order delegation support #1074

Basic first-order delegation support #1074

Conversation

pattivacek commented Jan 29, 2019

codecov-io commented Jan 29, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbonn Jan 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pattivacek commented Jan 31, 2019

codecov-io commented Jan 29, 2019 •

edited

Loading

lbonn Jan 30, 2019 •

edited

Loading