Refactor delegations to hopefuly be more secure #1089
Conversation
OYTIS
force-pushed
the
feat/secure-delegations
branch
from
252be62 to
5bde49f
Compare
Codecov Report
@@ Coverage Diff @@
## master #1089 +/- ##
==========================================
- Coverage 75.6% 75.58% -0.03%
==========================================
Files 158 158
Lines 9346 9431 +85
==========================================
+ Hits 7066 7128 +62
- Misses 2280 2303 +23
Continue to review full report at Codecov.
|
OYTIS
force-pushed
the
feat/secure-delegations
branch
from
5bde49f to
10fa87a
Compare
There was a problem hiding this comment.
Looks pretty good. Nesting logic looks good except for the version issue, and I like not downloading delegations unless/until we need them.
I don't think we have support anywhere for actually generating nested delegations, though. I don't think it'd be too hard to add to aktualizr-repo, and we need some way to test that this logic works. What do you think?
| static std::shared_ptr<Uptane::Targets> verifyDelegation(const std::string& delegation_raw, const Uptane::Role& role, | ||
| const Targets& parent_target); | ||
| std::shared_ptr<Uptane::Targets> getTrustedTargets() { return targets; } | ||
|
|
||
| private: | ||
| // Map from role name -> metadata ("targets" for top-level): |
There was a problem hiding this comment.
This comment is no longer relevant.
| @@ -29,9 +29,13 @@ class ImagesRepository : public RepositoryCommon { | |||
|
|
|||
| Exception getLastException() const { return last_exception; } | |||
|
|
|||
| static std::shared_ptr<Uptane::Targets> verifyDelegation(const std::string& delegation_raw, const Uptane::Role& role, | |||
| const Targets& parent_target); | |||
| std::shared_ptr<Uptane::Targets> getTrustedTargets() { return targets; } | |||
There was a problem hiding this comment.
"Trusted targets" is an interesting name. Based on the line auto toplevel_targets = images_repo.getTrustedTargets(); it appears to be just the toplevel targets; is that accurate? Maybe a comment in the header to explain what is meant by "trusted" here would be helpful.
| delegation = Uptane::ImagesRepository::verifyDelegation(delegation_meta, delegate_role, cur_targets); | ||
| if (delegation == nullptr) { | ||
| storage->deleteDelegation(delegate_role); | ||
| delegation_meta.clear(); |
There was a problem hiding this comment.
Just to make sure I have this straight: we only delete a delegation if verification fails? So if a delegation is removed and later re-added, it wouldn't get deleted after removal, but hopefully verification will fail and then it will get deleted. Right?
| if (!uptane_fetcher->fetchLatestRole(&delegation_meta, Uptane::kMaxImagesTargetsSize, | ||
| Uptane::RepositoryType::Image(), delegate_role)) { | ||
| throw std::runtime_error(std::string("Couldn't fetch ") + | ||
| delegate_role.ToString()); // TODO: connectivity exception |
There was a problem hiding this comment.
Don't we still need to fetch even if we have a stored version in case the version has incremented? Maybe I'm confusing something with versions, but this seems like it won't ever fetch new versions.
There was a problem hiding this comment.
Oh wait, you're right. So storing delegations in a persistent storage doesn't make any sense at all, we don't know what's there until we go to the server. That's annoying.
There was a problem hiding this comment.
I can propose one of the two procedures then.
There was a problem hiding this comment.
Either:
- At the beginning of update cycle clear all delegations.
- Cache delegations during the update cycle.
- Use cached version when it's available, download otherwise.
This way we can avoid downloading the same delegation over for different targets in the same update cycle, but still can't share them between the update cycles.
There was a problem hiding this comment.
Or:
- Store delegations permanently.
- Use the same procedure that is implemented now on the first pass.
- If some a target has not been found, update delegations for it on a second pass.
This way we are more bandwith-effective, but there is a potential security problem in not being able to revoke trust to targets in delegated roles in a timely fashion.
There was a problem hiding this comment.
Second option doesn't seem safe enough to me, but the first option is interesting. Does it make since to do any sort of comparison between old and new versions? If we have both available anyway, I wonder if there's anything intelligent we can do with them or if we really just have to ignore the old one and throw it away.
There was a problem hiding this comment.
That's what's in docker notary documentation by the way:
The snapshot key signs the snapshot metadata file, which enumerates the filenames, sizes, and hashes of the root, targets, and delegation metadata files for the collection.
Makes a lot of sense to me and resolves some of the points of confusion.
There was a problem hiding this comment.
This way we can cache the delegated meta and only update it if the version in snapshot.json has been incremented. And versioning for delegated targets suddenly makes sense again.
There was a problem hiding this comment.
That's fine, but is that what the backend is doing? I also want to make sure TUF/Uptane are friendly to that notion, but I think they should be. To make sure I understand correctly: this seems like the second option you mentioned, but more robust, because we don't store delegations "permanently", just as long as the version specified in snapshot.json hasn't changed.
There was a problem hiding this comment.
TUF spec says
The snapshot.json file is signed by the snapshot role. It lists the version numbers of all metadata on the repository, excluding timestamp.json and mirrors.json. For the root role, the hash(es), size, and version number are listed.
which doesn't contradict to what's in Notary docs, just does not name delegations explicitly. I believe that if backend doesn't add delegated roles to snapshot.json yet, it should do so. Otherwise we have weaker security guarantees for delegated targets than for top-level target, and I can't see a reason why it should be the case.
OYTIS
force-pushed
the
feat/secure-delegations
branch
from
039657d to
cfee6f9
Compare
Also, this enables nested delegations Signed-off-by: Anton Gerasimov <[email protected]>
OYTIS
force-pushed
the
feat/secure-delegations
branch
2 times, most recently
from
ffc9569 to
e430f36
Compare
Also list delegated roles in snapshot.json Signed-off-by: Anton Gerasimov <[email protected]>
Signed-off-by: Anton Gerasimov <[email protected]>
OYTIS
force-pushed
the
feat/secure-delegations
branch
2 times, most recently
from
8fd046f to
d25c02a
Compare
Signed-off-by: Anton Gerasimov <[email protected]>
| hash_exists = true; | ||
| break; | ||
| default: | ||
| break; |
There was a problem hiding this comment.
Looks like my comment may have disappeared. Is it possible for the other verify functions to reuse verifyRoleHashes as well? If it's too complicated, don't worry about it.
There was a problem hiding this comment.
verifyTargets is already using it. Other roles are not listed in snapshot.json (well, root.json is, but it makes no sense to verify it).
There was a problem hiding this comment.
I meant specifically the hashing logic in verifySnapshot. But after looking more closely, it doesn't isn't worth trying to combine those.
There was a problem hiding this comment.
@patrickvacek Yes, maybe the logic of iterating over a list of hashes could be abstracted out a bit, but right now it's not too bad I think, it's just two places.
OYTIS
force-pushed
the
feat/secure-delegations
branch
from
d25c02a to
155b39b
Compare
|
@patrickvacek Yes, that breaks compatibility with the backend as implemented now. But we have already agreed, it should be fixed there, and no-one uses it yet I believe. |
Also, this enables nested delegations
Signed-off-by: Anton Gerasimov [email protected]