fix/ota-2421/disallow downloading non-OSTree binaries #1282

Zee314159 · 2019-08-07T14:33:26Z

Draft code, just some thoughts to address this issue.

codecov-io · 2019-08-07T15:01:51Z

Codecov Report

Merging #1282 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1282      +/-   ##
==========================================
- Coverage   79.05%   79.02%   -0.04%     
==========================================
  Files         178      178              
  Lines       10475    10482       +7     
==========================================
+ Hits         8281     8283       +2     
- Misses       2194     2199       +5

Impacted Files	Coverage Δ
src/libaktualizr/primary/sotauptaneclient.h	`100% <ø> (ø)`	⬆️
src/libaktualizr/primary/sotauptaneclient.cc	`92.31% <100%> (+0.19%)`	⬆️
src/libaktualizr/uptane/exceptions.h	`91.66% <100%> (+0.55%)`	⬆️
src/libaktualizr/storage/sqlstorage_base.h	`60% <0%> (-40%)`	⬇️
src/libaktualizr/storage/sqlstorage_base.cc	`74.32% <0%> (-4.06%)`	⬇️
src/libaktualizr/utilities/utils.h	`93.33% <0%> (ø)`	⬆️
src/libaktualizr/storage/sqlstorage.cc	`75.86% <0%> (+0.24%)`	⬆️

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 7971998...a2ae26d. Read the comment docs.

mike-sul · 2019-08-08T06:18:03Z

src/libaktualizr/primary/sotauptaneclient.cc

+        if (!target.IsOstree() &&
+            (config.pacman.type == PackageManager::kOstree || config.pacman.type == PackageManager::kOstreeDockerApp)) {
+          LOG_ERROR << "Cannot install a non-OSTree package on an OSTree system";
+          sendEvent<event::InstallTargetComplete>(primary_ecu_serial, false);


IMHO, I'd make this code more generic and extendable, i.e. instead of comparing a specific target type with specific package manager types do something like the following:
if (target.type != config.pacman.type)

IMHO, I'd make this code more generic and extendable

I like the idea but currently we don't really have a good way to support that. The only type that we can recognize in the Target metadata is OSTree, so for now, that's the only one worth checking.

In other cases when is_new = false the event event::InstallTargetComplete is not sent, I am just wondering if it's correct place to do it here.

I agree, instead of sending that event, we should just set last_exception and return false. The log message is fine, though.

mike-sul · 2019-08-08T06:34:46Z

src/libaktualizr/primary/sotauptaneclient.cc

+        if (!target.IsOstree() &&
+            (config.pacman.type == PackageManager::kOstree || config.pacman.type == PackageManager::kOstreeDockerApp)) {
+          LOG_ERROR << "Cannot install a non-OSTree package on an OSTree system";
+          sendEvent<event::InstallTargetComplete>(primary_ecu_serial, false);


In other cases when is_new = false the event event::InstallTargetComplete is not sent, I am just wondering if it's correct place to do it here.

pattivacek · 2019-08-08T13:14:30Z

The logic looks about right, but we need a test for this. It will need to use the OSTree package manager, but it won't need to actually use it since the update will already fail at the check for updates.

Zee314159

It will need to use the OSTree package manager,

https://github.com/advancedtelematic/ostree-basic-pkg
This tool？

Zee314159 · 2019-08-09T04:32:58Z

src/libaktualizr/primary/sotauptaneclient.cc

+        if (!target.IsOstree() &&
+            (config.pacman.type == PackageManager::kOstree || config.pacman.type == PackageManager::kOstreeDockerApp)) {
+          LOG_ERROR << "Cannot install a non-OSTree package on an OSTree system";
+          last_exception = Uptane::InvalidMetadata(target.filename(), "", "Non-OSTree package");


Is ‘InvalidMetadata’ the proper exception type?

That's not a bad choice, but since it's usually used for problems in parsing the metadata, maybe we should find something else. Nothing else looks better, though, so how about making a new exception? InvalidTarget perhaps?

Sure，I was considering making a new one too. Thanks!

pattivacek · 2019-08-12T07:43:15Z

It will need to use the OSTree package manager,

https://github.com/advancedtelematic/ostree-basic-pkg
This tool？

I was referring to https://github.com/advancedtelematic/aktualizr/blob/master/src/libaktualizr/package_manager/ostreemanager.h. You may need to use this script to generate an OSTree sysroot for the test: https://github.com/advancedtelematic/aktualizr/blob/master/scripts/make_ostree_sysroot.sh. For an example of how to use that, see https://github.com/advancedtelematic/aktualizr/blob/master/src/libaktualizr/package_manager/CMakeLists.txt#L36 and https://github.com/advancedtelematic/aktualizr/blob/master/src/libaktualizr/package_manager/ostreemanager_test.cc.

Zee314159 · 2019-08-12T14:40:58Z

Totally raw! Maybe I should use json to make a fake target instead of using aktualizr tool to generate one? And the 'generate ostree sysroot' part also looks not good.

pattivacek · 2019-08-12T15:08:47Z

Maybe I should use json to make a fake target instead of using aktualizr tool to generate one?

Nah, using aktualizr-repo is perfect. That looks fine to me.

And the 'generate ostree sysroot' part also looks not good.

I also think that part is a bit overcomplicated. You don't actually need to mock the sysroot and all that, or at least I don't think so. It looks like you modeled that part off of the test_aktualizr_fullostree, but what you actually want is just to use the simpler method used by ostreemanager_test, which just uses a sysroot generated by the make_ostree_sysroot.sh script I mentioned. Since you don't have to actually install anything, you shouldn't need to do the whole complex mocking bit.

src/libaktualizr/primary/CMakeLists.txt

src/libaktualizr/primary/download_nonostree_test.cc

pattivacek · 2019-08-15T13:09:44Z

If you rebase on latest master, you should be able to get the Travis timeout resolved.

I ran your branch locally and noticed this in the output: image command requires --targetsha256 or --targetsha512, and --targetlength when --filename is not supplied.. That's uptane-generator (former aktualizr-repo) complaining and might be part of what's going wrong.

Zee314159 · 2019-08-15T14:24:28Z

If you rebase on latest master

Trapped in git chaos...I'll figure the way out tomorrow.

image command requires --targetsha256 or --targetsha512, and --targetlength when --filename is not supplied.. That's uptane-generator (former aktualizr-repo) complaining and might be part of what's going wrong.

I might should start with this issue first.

pattivacek

A few minor comments. As far as why the test is failing... perhaps my assumptions about how easy it would be to fake an ostree sysroot for this purpose was wrong. I'm not sure i understand why ostree is complaining, though. Have you been able to trace it with gdb?

pattivacek · 2019-08-16T10:22:50Z

src/libaktualizr/primary/download_nonostree_test.cc

+
+#include <ostree.h>
+
+boost::filesystem::path aktualizr_repo_path;


This variable doesn't need to be global.

pattivacek · 2019-08-16T10:23:20Z

src/libaktualizr/primary/download_nonostree_test.cc

+#include "storage/sqlstorage.h"
+#include "test_utils.h"
+
+#include <ostree.h>


I think this include shouldn't be necessary anymore.

pattivacek · 2019-08-16T10:23:52Z

src/libaktualizr/primary/download_nonostree_test.cc

+#include <future>
+#include <iostream>
+#include <string>
+#include <thread>


Are all of these includes necessary as well?

pattivacek · 2019-08-16T12:40:56Z

src/libaktualizr/primary/download_nonostree_test.cc

+  boost::trim_if(new_rev, boost::is_any_of(" \t\r\n"));
+  LOG_INFO << "DEST: " << new_rev;
+
+  Process akt_repo(aktualizr_repo_path.string());


Not at all critical, but I recommend renaming the "aktualizr_repo" references here to "uptane_generator". (I also changed every "akt_repo" to just "gen".)

Zee314159 · 2019-08-16T12:43:24Z

A few minor comments. As far as why the test is failing... perhaps my assumptions about how easy it would be to fake an ostree sysroot for this purpose was wrong. I'm not sure i understand why ostree is complaining, though. Have you been able to trace it with gdb?

No, I was trying to use the whole mock the sysroot and unptane-generate part from aktuarlizr_fullostree_test. But aktuarlizr_fullostree_test will fail locally too.

src/libaktualizr/primary/download_nonostree_test.cc

lbonn · 2019-08-19T09:14:31Z

The code looks good but can we change the commit messages before merging?

src/libaktualizr/primary/download_nonostree_test.cc

Added unit test Fixed bugs that failed the test Removed unnecessary ostree bits Signed-off-by: Zee314159 <252806294@qq.com>

Zee314159 force-pushed the fix/ota-2421/disallow-downloading-non-ostree-binaries branch from 6e64eb4 to c9e5607 Compare August 7, 2019 14:34

Zee314159 requested a review from lbonn August 7, 2019 14:36

mike-sul reviewed Aug 8, 2019

View reviewed changes

Zee314159 force-pushed the fix/ota-2421/disallow-downloading-non-ostree-binaries branch from 487028c to 4d52a04 Compare August 9, 2019 04:11

Zee314159 commented Aug 12, 2019

View reviewed changes

Zee314159 force-pushed the fix/ota-2421/disallow-downloading-non-ostree-binaries branch 2 times, most recently from ff60411 to 8f1eed4 Compare August 12, 2019 14:25

Zee314159 force-pushed the fix/ota-2421/disallow-downloading-non-ostree-binaries branch 4 times, most recently from fe3430c to 5a24aab Compare August 13, 2019 06:26

pattivacek reviewed Aug 13, 2019

View reviewed changes

src/libaktualizr/primary/CMakeLists.txt Show resolved Hide resolved

src/libaktualizr/primary/download_nonostree_test.cc Show resolved Hide resolved

Zee314159 force-pushed the fix/ota-2421/disallow-downloading-non-ostree-binaries branch 8 times, most recently from 5e7dcca to 4ad2074 Compare August 14, 2019 07:10

Zee314159 force-pushed the fix/ota-2421/disallow-downloading-non-ostree-binaries branch 2 times, most recently from 4c74ecf to 9d1aa78 Compare August 16, 2019 06:20

Zee314159 force-pushed the fix/ota-2421/disallow-downloading-non-ostree-binaries branch 2 times, most recently from 4b3da79 to 3db232a Compare August 16, 2019 09:37

pattivacek reviewed Aug 16, 2019

View reviewed changes

Zee314159 force-pushed the fix/ota-2421/disallow-downloading-non-ostree-binaries branch from 3db232a to 702c9aa Compare August 16, 2019 13:07

pattivacek reviewed Aug 16, 2019

View reviewed changes

src/libaktualizr/primary/download_nonostree_test.cc Show resolved Hide resolved

Zee314159 force-pushed the fix/ota-2421/disallow-downloading-non-ostree-binaries branch 2 times, most recently from 6ae1ed9 to ba061fa Compare August 19, 2019 01:58

pattivacek reviewed Aug 19, 2019

View reviewed changes

src/libaktualizr/primary/download_nonostree_test.cc Outdated Show resolved Hide resolved

Zee314159 force-pushed the fix/ota-2421/disallow-downloading-non-ostree-binaries branch 2 times, most recently from 96ddefb to 784fd81 Compare August 19, 2019 11:34

Added feature disabling binary updates

a2ae26d

Added unit test Fixed bugs that failed the test Removed unnecessary ostree bits Signed-off-by: Zee314159 <252806294@qq.com>

Zee314159 force-pushed the fix/ota-2421/disallow-downloading-non-ostree-binaries branch from 784fd81 to a2ae26d Compare August 20, 2019 12:47

pattivacek approved these changes Aug 21, 2019

View reviewed changes

pattivacek merged commit 2c9fed9 into master Aug 21, 2019

pattivacek deleted the fix/ota-2421/disallow-downloading-non-ostree-binaries branch August 21, 2019 07:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix/ota-2421/disallow downloading non-OSTree binaries #1282

fix/ota-2421/disallow downloading non-OSTree binaries #1282

Zee314159 commented Aug 7, 2019

codecov-io commented Aug 7, 2019 •

edited

Loading

mike-sul Aug 8, 2019

pattivacek Aug 8, 2019

mike-sul Aug 8, 2019

pattivacek commented Aug 8, 2019

Zee314159 left a comment

Zee314159 Aug 9, 2019

pattivacek Aug 12, 2019

Zee314159 Aug 12, 2019 •

edited

Loading

pattivacek commented Aug 12, 2019

Zee314159 commented Aug 12, 2019

pattivacek commented Aug 12, 2019

pattivacek commented Aug 15, 2019

Zee314159 commented Aug 15, 2019

pattivacek left a comment

pattivacek Aug 16, 2019

pattivacek Aug 16, 2019

pattivacek Aug 16, 2019

pattivacek Aug 16, 2019

Zee314159 commented Aug 16, 2019

lbonn commented Aug 19, 2019


		#include <ostree.h>

		boost::filesystem::path aktualizr_repo_path;

fix/ota-2421/disallow downloading non-OSTree binaries #1282

fix/ota-2421/disallow downloading non-OSTree binaries #1282

Conversation

Zee314159 commented Aug 7, 2019

codecov-io commented Aug 7, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pattivacek commented Aug 8, 2019

Zee314159 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zee314159 Aug 12, 2019 • edited Loading

Choose a reason for hiding this comment

pattivacek commented Aug 12, 2019

Zee314159 commented Aug 12, 2019

pattivacek commented Aug 12, 2019

pattivacek commented Aug 15, 2019

Zee314159 commented Aug 15, 2019

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

Zee314159 commented Aug 16, 2019

lbonn commented Aug 19, 2019

codecov-io commented Aug 7, 2019 •

edited

Loading

Zee314159 Aug 12, 2019 •

edited

Loading