OTA-3969: Do the full verification of metadata on the IP Secondary #1449

mike-sul · 2019-11-15T13:56:19Z

Signed-off-by: Mykhaylo Sul myk.sul@gmail.com

pattivacek · 2019-11-15T14:42:17Z

Nice, maybe this is easier than I thought! Let me know when you're ready for a deeper review. But at first glance it's looking good.

codecov-io · 2019-11-15T16:00:05Z

Codecov Report

Merging #1449 into master will increase coverage by 0.34%.
The diff coverage is 85.5%.

@@            Coverage Diff             @@
##           master    #1449      +/-   ##
==========================================
+ Coverage   80.25%   80.59%   +0.34%     
==========================================
  Files         182      184       +2     
  Lines       11029    11068      +39     
==========================================
+ Hits         8851     8920      +69     
+ Misses       2178     2148      -30

Impacted Files	Coverage Δ
src/aktualizr_secondary/aktualizr_secondary.h	`100% <ø> (ø)`	⬆️
src/libaktualizr/uptane/imagesrepository.h	`100% <ø> (ø)`	⬆️
src/libaktualizr/uptane/fetcher.cc	`100% <ø> (ø)`	⬆️
.../virtual_secondary/partialverificationsecondary.cc	`70.21% <100%> (ø)`	⬆️
src/libaktualizr/primary/sotauptaneclient.cc	`88.93% <100%> (+0.14%)`	⬆️
...aktualizr_secondary/aktualizr_secondary_metadata.h	`100% <100%> (ø)`
src/libaktualizr/uptane/uptanerepository.h	`100% <100%> (ø)`	⬆️
...c/aktualizr_secondary/aktualizr_secondary_common.h	`100% <100%> (ø)`	⬆️
src/libaktualizr/uptane/tuf.h	`95.86% <100%> (+0.33%)`	⬆️
src/libaktualizr/uptane/directorrepository.cc	`95.94% <100%> (+0.05%)`	⬆️
... and 16 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 2d06138...0fdd4c0. Read the comment docs.

pattivacek

Thanks! This looks really good. I like the metadata "fetching" refactoring, and I think that looks forward-compatible with delegations, once we support them for secondaries. The tests look good, too. We can probably do more, but it's a great start.

pattivacek · 2019-11-25T16:11:15Z

src/aktualizr_secondary/CMakeLists.txt

@@ -61,7 +68,7 @@ if(BUILD_OSTREE)
                       LIBRARIES uptane_generator_lib
                       LIBRARIES aktualizr-posix
                       ARGS ${PROJECT_BINARY_DIR}/ostree_repo PROJECT_WORKING_DIRECTORY)
-    target_link_libraries(t_aktualizr_secondary_uptane virtual_secondary)
+                       target_link_libraries(t_aktualizr_secondary_uptane virtual_secondary)


Not at all critical, but what's up with this indent?

Some kind of typo, will move it back.

pattivacek · 2019-11-26T08:27:17Z

src/aktualizr_secondary/aktualizr_secondary.cc

@@ -113,14 +86,17 @@ bool AktualizrSecondary::putRootResp(const std::string& root, bool director) {
 }

 bool AktualizrSecondary::sendFirmwareResp(const std::shared_ptr<std::string>& firmware) {
-  if (target_ == nullptr) {
-    LOG_ERROR << "No valid installation target found";
+  auto targetsForThisEcu = director_repo_.getTargets(getSerial());


Should we be rechecking the metadata (without fetching) like we do on the Primary before installation? It seems redundant, but it's a protection against unexpected changes in the time since the metadata was received.

Yes, I agree, we should check at least an image file size (binary case) and its hash (for both ostree and binary).

I think it's also worth rechecking the whole metadata chain from the metadata, but yeah, checking the object's hash/size is even more important.

pattivacek · 2019-11-26T08:30:09Z

src/aktualizr_secondary/aktualizr_secondary.cc

+  //
+  // The followin steps of the Director's target metadata verification are missing in DirectorRepository::updateMeta()
+  //  6. If checking Targets metadata from the Director repository, verify that there are no delegations.
+  //  7. If checking Targets metadata from the Director repository, check that no ECU identifier is represented more


Can you make a tech debt ticket for this and put it in the robustness epic?

It looks like you've already implemented the check for the secondary; is your proposal to move that check into the Uptane code directly so it is applied universally as early as is reasonable?

Sure, I'll do it.

I did #7, not #6, and yes my proposal to do apply it universally.

Understood. We implicitly do step 6 in that we ignore the concept of delegations from the Director, but it's not a bad idea to check for them and throw a warning if found. Since we don't use the Snapshot role on the Director, though, all we can do is just check for them in the Targets metadata.

https://saeljira.it.here.com/browse/OTA-4118

pattivacek · 2019-11-26T08:38:17Z

src/libaktualizr/uptane/directorrepository.cc

+    // This procedure doesn't exactly follow the spec:
+    // https://uptane.github.io/uptane-standard/uptane-standard.html#check_root At first, it should try to download N+1
+    // version of the Root metadata file instead of downloading the "latest" one how do we know which version is the
+    // latest and why just root.json is downloaded ?


root.json without a number is guaranteed to be the latest in our scheme. The design is to fetch the latest, and if it matches what we already have stored, we're good. If not, then we have to go fetch the missing ones by name/number until we catch up. Might be worth a renewed discussion on what the best method is; I don't see the harm in our method.

Thanks for explanation.

pattivacek · 2019-11-26T08:39:19Z

src/libaktualizr/uptane/directorrepository.cc

+    // 1. At initial stage (just after provisioning) the root metadata from 1.root.json are not verified
+    // 2. If local_version becomes higher than 1, e.g. 2 than a rollback attack is possible since the business logic
+    // here won't return any error as suggested in #4 of
+    // https://uptane.github.io/uptane-standard/uptane-standard.html#check_root


Good call. Can you make another tech debt for this? (It's the same logic with the Image repo, so it can all be in one ticket.)

https://saeljira.it.here.com/browse/OTA-4119

pattivacek · 2019-11-26T08:42:30Z

src/libaktualizr/uptane/imagesrepository.cc

@@ -235,6 +241,11 @@ bool ImagesRepository::updateMeta(INvStorage& storage, Fetcher& fetcher) {
      local_version = -1;
    }

+    // I am not sure that #6 of https://uptane.github.io/uptane-standard/uptane-standard.html#check_snapshot is
+    // performed


What's your concern? That we don't match the Targets metadata filenames?

Yes, I mean exactly what #6 says

"Check that each Targets metadata filename listed in the previous Snapshot metadata file is also listed in this"

This might be another thing worth making a ticket for and looking into more. I'm not sure how critical this really is, though, and I'm also not sure that the backend doesn't make some optimizations that would prevent that from working. In fact, I'm not sure this is actually sensible; what happens if a delegation gets (legitimately) removed?

FYI: uptane/uptane-standard#149. Let's wait for resolution on that before making changes on our side.

Signed-off-by: Mykhaylo Sul <myk.sul@gmail.com>

mike-sul · 2019-11-26T11:48:22Z

@patrickvacek Updated the PR according to the comments as well as created the suggested "robustness" tickets.
Also, created a follow-up ticket for more detailed testing as we discussed https://saeljira.it.here.com/browse/OTA-4120.

pattivacek

Looks good, thanks!

mike-sul added not-ready do-not-merge-until-ready labels Nov 15, 2019

mike-sul force-pushed the feat/OTA-3969/ip-secondary-full-verification branch from e53af0d to b7867bb Compare November 15, 2019 16:00

mike-sul force-pushed the feat/OTA-3969/ip-secondary-full-verification branch 6 times, most recently from 631945a to 307dcb6 Compare November 22, 2019 09:50

mike-sul force-pushed the feat/OTA-3969/ip-secondary-full-verification branch 4 times, most recently from d33d800 to 09214cb Compare November 25, 2019 14:27

mike-sul removed do-not-merge-until-ready not-ready labels Nov 25, 2019

mike-sul requested review from lbonn and pattivacek November 25, 2019 15:22

pattivacek reviewed Nov 26, 2019

View reviewed changes

mike-sul added 2 commits November 26, 2019 13:11

OTA-3969: Do the full verification of metadata on the IP Secondary

7a2e49a

Signed-off-by: Mykhaylo Sul <myk.sul@gmail.com>

OTA-3969: Uptane verification tests for IP secondary

0fdd4c0

Signed-off-by: Mykhaylo Sul <myk.sul@gmail.com>

mike-sul force-pushed the feat/OTA-3969/ip-secondary-full-verification branch from 09214cb to 0fdd4c0 Compare November 26, 2019 11:46

pattivacek approved these changes Nov 26, 2019

View reviewed changes

pattivacek merged commit 8136440 into master Nov 26, 2019

pattivacek deleted the feat/OTA-3969/ip-secondary-full-verification branch November 26, 2019 12:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OTA-3969: Do the full verification of metadata on the IP Secondary #1449

OTA-3969: Do the full verification of metadata on the IP Secondary #1449

mike-sul commented Nov 15, 2019

pattivacek commented Nov 15, 2019

codecov-io commented Nov 15, 2019 •

edited

Loading

pattivacek left a comment

pattivacek Nov 25, 2019

mike-sul Nov 26, 2019

pattivacek Nov 26, 2019

mike-sul Nov 26, 2019

pattivacek Nov 26, 2019

pattivacek Nov 26, 2019

mike-sul Nov 26, 2019

pattivacek Nov 26, 2019

mike-sul Nov 26, 2019

pattivacek Nov 26, 2019

mike-sul Nov 26, 2019

pattivacek Nov 26, 2019

mike-sul Nov 26, 2019

mike-sul Nov 26, 2019

pattivacek Nov 26, 2019

mike-sul Nov 26, 2019

pattivacek Nov 26, 2019

pattivacek Nov 26, 2019

mike-sul Nov 26, 2019

mike-sul commented Nov 26, 2019

pattivacek left a comment

OTA-3969: Do the full verification of metadata on the IP Secondary #1449

OTA-3969: Do the full verification of metadata on the IP Secondary #1449

Conversation

mike-sul commented Nov 15, 2019

pattivacek commented Nov 15, 2019

codecov-io commented Nov 15, 2019 • edited Loading

Codecov Report

pattivacek left a 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

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

mike-sul commented Nov 26, 2019

pattivacek left a comment

Choose a reason for hiding this comment

codecov-io commented Nov 15, 2019 •

edited

Loading