Skip to content
This repository has been archived by the owner on May 21, 2024. It is now read-only.

Fix/ota 4119/ota 4132/root verification #1501

Merged
merged 11 commits into from
Jan 6, 2020

Conversation

pattivacek
Copy link
Collaborator

No description provided.

@pattivacek pattivacek force-pushed the fix/OTA-4119/OTA-4132/root-verification branch from d15af59 to d37167f Compare December 23, 2019 15:45
@@ -47,6 +47,45 @@ bool RepositoryCommon::verifyRoot(const std::string& root_raw) {

void RepositoryCommon::resetRoot() { root = Root(Root::Policy::kAcceptAll); }

bool RepositoryCommon::updateRoot(INvStorage& storage, const IMetadataFetcher& fetcher,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool, that exactly was I thought about when I saw the initial implementation - share as much code as possible between the director and imagerepo implementation.

Perhaps, it makes sense to add a reference(s) to the corresponding paragraphs of the uptane spec.

@pattivacek pattivacek force-pushed the fix/OTA-4119/OTA-4132/root-verification branch 2 times, most recently from 25e169e to f7645b1 Compare December 27, 2019 12:33
@codecov-io
Copy link

codecov-io commented Dec 27, 2019

Codecov Report

Merging #1501 into master will decrease coverage by 0.18%.
The diff coverage is 94.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1501      +/-   ##
==========================================
- Coverage   80.91%   80.72%   -0.19%     
==========================================
  Files         184      184              
  Lines       11151    11141      -10     
==========================================
- Hits         9023     8994      -29     
- Misses       2128     2147      +19
Impacted Files Coverage Δ
...aktualizr_secondary/aktualizr_secondary_metadata.h 100% <ø> (ø) ⬆️
src/libaktualizr/uptane/uptanerepository.h 100% <ø> (ø) ⬆️
src/libaktualizr/uptane/fetcher.cc 100% <ø> (ø) ⬆️
src/libaktualizr/uptane/directorrepository.cc 94.33% <100%> (-1.61%) ⬇️
src/libaktualizr/uptane/tuf.h 96% <100%> (+0.03%) ⬆️
src/libaktualizr/uptane/uptanerepository.cc 100% <100%> (+10.52%) ⬆️
src/libaktualizr/uptane/imagesrepository.cc 87.58% <100%> (-1.51%) ⬇️
...ktualizr_secondary/aktualizr_secondary_metadata.cc 81.81% <87.5%> (+0.86%) ⬆️
src/libaktualizr/storage/sqlstorage.cc 76.33% <0%> (-1.79%) ⬇️
src/libaktualizr/package_manager/ostreemanager.cc 79.02% <0%> (-0.75%) ⬇️
... and 1 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 a6ee6b4...ff053f9. Read the comment docs.

@pattivacek pattivacek force-pushed the fix/OTA-4119/OTA-4132/root-verification branch from 35d4543 to 9365d07 Compare December 27, 2019 14:44
@pattivacek
Copy link
Collaborator Author

Ready for review but depends on advancedtelematic/tuf-test-vectors#65 before merging.

@pattivacek pattivacek force-pushed the fix/OTA-4119/OTA-4132/root-verification branch from 9365d07 to 06f9981 Compare December 30, 2019 09:12
@pattivacek
Copy link
Collaborator Author

Unfortunately, we can't merge this as is because the backend apparently expects that devices fetch the unnumbered root.json to qualify as "online". If that is difficult to change, as a temporary fix, I could reinstate fetching root.json the very first time a device provisions in order to satisfy that requirement, and then we could remove it once the backend no longer requires it.

@mike-sul mike-sul closed this Dec 30, 2019
@pattivacek pattivacek reopened this Dec 30, 2019
@mike-sul
Copy link
Collaborator

If that is difficult to change, as a temporary fix, I could reinstate fetching root.json the very first time a device provisions in order to satisfy that requirement, and then we could remove it once the backend no longer requires it.

I vote for the temporary fix

@pattivacek
Copy link
Collaborator Author

If that is difficult to change, as a temporary fix, I could reinstate fetching root.json the very first time a device provisions in order to satisfy that requirement, and then we could remove it once the backend no longer requires it.

I vote for the temporary fix

Done. I also realized that I forgot to amend uptane-generator to stop creating an unnumbered root.json, but now I can't fix that until we can remove this hack.

@pattivacek pattivacek force-pushed the fix/OTA-4119/OTA-4132/root-verification branch from 24cb71e to f5c0e5a Compare January 2, 2020 15:58
It is essentially identical for the Director and Image repos, so there
is no need to duplicate the code.

Signed-off-by: Patrick Vacek <[email protected]>
Do not check for an unnumbered version of the root metadata. Just check
for the next available version and move on if it is not found.

Signed-off-by: Patrick Vacek <[email protected]>
Previously they just threw exceptions.

Signed-off-by: Patrick Vacek <[email protected]>
It appears to have been dumping an object into the storage field instead
of the actual path.

Also convert the import paths into absolute paths instead of local. This
makes testing and debugging much easier.

Signed-off-by: Patrick Vacek <[email protected]>
It now needs to respect versions.

Signed-off-by: Patrick Vacek <[email protected]>
aktualizr no longer tries an unnumbered version.

Signed-off-by: Patrick Vacek <[email protected]>
New tests should cover the root version checking.

Signed-off-by: Patrick Vacek <[email protected]>
This is a hack, but currently the backend expects that the unnumbered
root.json is fetched once. Otherwise a device is not considered
"online". Once that requirement is lifted, this commit should be
reverted.

Also at that time, uptane-generator should be amended to stop creating
an unnumbered root.json.

Signed-off-by: Patrick Vacek <[email protected]>
@pattivacek pattivacek force-pushed the fix/OTA-4119/OTA-4132/root-verification branch from f5c0e5a to 0419df6 Compare January 6, 2020 10:33
// If not, then we have to go fetch the missing ones by name/number until we catch up.

// 5.4.4.3.2. Update to the latest Root metadata file.
for (int version = rootVersion() + 1;; ++version) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this loop be upper-bounded? It seems a bit dangerous to never stop here if the server keeps supplying valid metadata.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a really good point. Yes, it probably should be, but what's reasonable? 100? We should probably bring this up directly with the Uptane group. Or do you have an opinion?

Copy link
Contributor

Choose a reason for hiding this comment

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

It sounds like another limit implementations should enforce, like the size of downloaded metadata artifacts.

I am thinking if we should have a limit for version jumps during an update or just a global upper bound (which we need anyway since we're using json integers and an int in this code).

The "jump" limit might complicate diagnostic of accidental trigger, since it would only affect a fraction of the devices.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It sounds like another limit implementations should enforce, like the size of downloaded metadata artifacts.

Could be, yes.

I am thinking if we should have a limit for version jumps during an update or just a global upper bound (which we need anyway since we're using json integers and an int in this code).

Global might make more sense, because if you provision a new device, it'll have to catch up the whole way to the latest anyway. But I'm also a bit concerned that it might be unexpected to have such a limit.

I think we should either leave it is or make a quick decision now, and either way make a ticket to follow up on it and in the meantime move forward with getting this merged. Do you have a preference? If we want to just make a quick decision, I'd say 100 should be fine for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I see, then 100 or 1000 would be fine for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done (with 1000 for now), and made https://saeljira.it.here.com/browse/OTA-4273 to re-evaluate again.

The idea is to try to better handle the scenario where the server keeps
unceasingly providing valid metadata, which could be a sort of bizarre
attack.

Suggested-by: Laurent Bonnans <[email protected]>
Signed-off-by: Patrick Vacek <[email protected]>
@pattivacek pattivacek merged commit 77df64c into master Jan 6, 2020
@pattivacek pattivacek deleted the fix/OTA-4119/OTA-4132/root-verification branch January 6, 2020 14:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants