Uh oh!
There was an error while loading. Please reload this page.
Snapshot RSA Accumulator - #1510
Conversation
This commit uses a custom python implementation of Miller-Rabin that we will want to replace with a well-maintained library. It does not include efficient updates to the RSA Accumulator Signed-off-by: Marina Moore <mnm678@gmail.com>
This is missing test data for the client Signed-off-by: Marina Moore <mnm678@gmail.com>
Signed-off-by: Marina Moore <mnm678@gmail.com>
znewman01
left a comment
There was a problem hiding this comment.
Hey! Sorry for the long lag on this.
I'm a thorough (AKA annoying 😄) code reviewer so feel free to ignore most of what I said. The comments in repository_lib.py are probably the important ones.
If anything's confusing, feel free to ask for clarification or we can set up a time to (video) call about it.
| test_nodes['role1'] = tuf.formats.make_metadata_fileinfo(1, None, None) | ||
| test_nodes['role2'] = tuf.formats.make_metadata_fileinfo(1, None, None) | ||
| root, leaves = repo_lib._build_rsa_acc(test_nodes) |
There was a problem hiding this comment.
Not sure what's going on here -- above, there's a whole build/write/check files/check for specific value cycle, whereas here you just seem to be verifying that it doesn't crash?
| snapshot_info = repository_updater.verify_rsa_acc_proof('role1') | ||
| self.assertEqual(snapshot_info['version'], 1) | ||
| # verify RSA accumulator with invalid role |
There was a problem hiding this comment.
Would like to also see a test for verifying an existing role against an invalid accumulator
| if 'rsa_acc' not in self.metadata['current']['timestamp']: | ||
| # If an RSA Accumulator is defined, do not update snapshot metadata. Instead, | ||
| # we will download the relevant proof files later when downloading | ||
| # a target. |
There was a problem hiding this comment.
The Merkle stuff isn't merged yet, right? You'd have to do the same thing there too.
Which is actually making me think there's room for abstraction here: one interface for "full manifest"/"rsa"/"merkle" so that not every user of the metadata has to interact with something called "rsa"
| def signable_verification(self, metadata_role, file_object, expected_version): |
There was a problem hiding this comment.
Could use a doc comment.
Also, naming nitpick: I tend to like all functions/methods to be verbs, so it's clearer what they do (e.g., "make_signable_verification" over "signable_verification" but even better "attest", "prove", etc. (if those are accurate))
| def signable_verification(self, metadata_role, file_object, expected_version): | ||
| # Verify 'file_object' according to the callable function. |
| def _build_rsa_acc(fileinfodict): |
There was a problem hiding this comment.
You're eventually going to want to separate out:
- initial build
- update
- removal (possibly)
| leaves = [] | ||
| primes = [] | ||
| acc_exp = 1 | ||
| for name, contents in sorted(fileinfodict.items()): |
There was a problem hiding this comment.
I have a preference for splitting all format-specific operations from all number-theoretic operations, which makes testing/understanding a lot easier.
So maybe a method like build(values) which takes a list of things to be hashed?
| json_contents = securesystemslib.formats.encode_canonical(contents) | ||
| prime = hash_to_prime(json_contents) | ||
| primes.append(prime) | ||
| acc_exp = acc_exp * prime |
There was a problem hiding this comment.
This is a hidden O(n^2) algorithm: acc_exp is O(n) in each iteration!
I think there's ways to do this O(n lg n) but need to think about it/dredge them up.
Best trick (O(n)) is to reduce acc_exp mod the order of the group---but that only works if you know the group order (which means knowing the RSA factors---we can think about whether that works in our setting. e.g., if you sign manifests we're kinda implicitly trusting you -- except then we lose a little bit of auditability because you can forge proofs if you know the RSA factors. we should discuss).
| class acc_contents(object): | ||
| contents = None | ||
| name = None | ||
| proof = None |
There was a problem hiding this comment.
Is this so we can hold precomputed proofs for each element?
| cont = acc_contents(name, contents) | ||
| leaves.append(cont) | ||
| json_contents = securesystemslib.formats.encode_canonical(contents) |
There was a problem hiding this comment.
Above, you say acc_contents needs name, but here we're encoding withoutname!
Signed-off-by: Marina Moore <mnm678@gmail.com>
lukpueh
commented
Feb 17, 2022
Ping @mnm678, same questions as in #1113 (comment). |
mnm678
commented
Feb 17, 2022
Closing in favor of writing this on top of the new Metadata api. |
This is an early PoC