Remove uses of keyid_hash_algorithms #1014

mnm678 · 2020-04-02T22:02:52Z

This is a first step toward removing keyid_hash_algorithms from the reference implementation as discussed in #848. This pr removes all uses of this field during the client verification by using the keyids provided in the metadata instead of recalculating keyids using the keyid_hash_algorithms.

This pr requires changes to securesystemslib that are is a pull request at secure-systems-lab/securesystemslib#225.

tests/test_keydb.py

joshuagl

Thanks for the submission Marina! Am I correct in thinking the behavioural changes introduced by this PR are that:

a client no longer recalculates keyids, and instead trusts the values in the metadata
when loading an existing repository with repository_tool, keyids are not calculated and instead the keyids from the metadata are used

I'd like to see the additional test for a duplicate keyid enabled, or removed from this PR. I also pointed had some minor comments inline about not passing the default value for an argument and what appears to be unnecessarily assigning a value to a dictionary field.

NOTE: the required securesystemslib change has been merged and is available in release 0.15.0, which we already depend on in py-TUF.

joshuagl · 2020-07-23T10:41:09Z

tests/test_keydb.py

+    # New key with a duplicate keyid
+    #rsakey4 = KEYS[1]
+    #keyid4 = KEYS[3]['keyid']
+    #keydict[keyid4] = rsakey4


Should this test work? If not, what's preventing it from working? Should we be addressing it in this PR?

As keyids are no longer calculated before use, the client will want a different way to verify that all keys applied to a threshold are unique. I think this may be better left to a separate pr (which I've documented in #1084) for 2 reasons:

This is a larger change that will require creating a standardized representation for keys

It is only an issue if the delegating metadata makes a mistake or in the case of multi-role delegations (which are not currently in the reference implementation). Also, this issue already exists with the current implementation if multiple keyid_hash_algorithms are used

That being said, I have started looking into this issue and could include the fix here if that would be more clear. Otherwise I'll remove this test for now.

joshuagl · 2020-07-23T10:41:56Z

tests/test_keydb.py

    self.assertRaises(tuf.exceptions.UnknownKeyError, tuf.keydb.get_key, keyid3)
+    #self.assertRaises(tuf.exceptions.UnknownKeyError, tuf.keydb.get_key, keyid4)


Can we enable this part of the test?

tuf/client/updater.py

joshuagl · 2020-07-23T10:49:00Z

tuf/keydb.py

-          # keyids, otherwise add_key() will have no reference to it.
-          key_dict['keyid'] = keyid
-          add_key(key_dict, keyid=None, repository_name=repository_name)
+        key_dict['keyid'] = keyid


I think key_dict['keyid'] should already be set to keyid here, because we pass it in as the default_keyid in format_metadata_to_key?

tuf/keydb.py

joshuagl · 2020-07-23T10:49:46Z

tuf/repository_lib.py

-          key_object['keyid'] = keyid
-          tuf.keydb.add_key(key_object, keyid=None,
-              repository_name=repository_name)
+        key_object['keyid'] = keyid


Again, I think this is already set correctly by format_metadata_to_key()?

joshuagl · 2020-07-23T11:05:29Z

Note the DCO check is failing because none of the commits have a Signed-off-by: line, the check details suggest the following to address that:

git rebase HEAD~8 --signoff
git push --force-with-lease origin remove-keyid_hash_algorithms

… as the keyid is provided in signed metadata. Removing this check allows the client to avoid use of the keyid_hash_algorithm field during verification. Note that this change requires a small change to the securesystemslib api. Signed-off-by: marinamoore <mmoore32@calpoly.edu>

…eyid. Check that this keyid matches the keyid listed in the key for consistent behavior. Signed-off-by: marinamoore <mmoore32@calpoly.edu>

Signed-off-by: marinamoore <mmoore32@calpoly.edu>

…e keyid provided in the delegation. Note that this requires a change to securesystems lib. Signed-off-by: marinamoore <mmoore32@calpoly.edu>

Signed-off-by: marinamoore <mmoore32@calpoly.edu>

Co-authored-by: Joshua Lock <jlock@vmware.com> Signed-off-by: marinamoore <mmoore32@calpoly.edu>

Signed-off-by: marinamoore <mmoore32@calpoly.edu>

mnm678 · 2020-07-23T16:56:34Z

Thanks for the submission Marina! Am I correct in thinking the behavioural changes introduced by this PR are that:
* a client no longer recalculates keyids, and instead trusts the values in the metadata

* when loading an existing repository with repository_tool, keyids are not calculated and instead the keyids from the metadata are used

Thanks for the review! Yes, those are the main behavioural changes. These changes will help both with the TAP 12 implementation and with the removal of keyid_hash_algorithms from TUF metadata per #848.

mnm678 · 2020-07-23T17:55:27Z

The failing test looks like a timeout issue that should be unrelated to this pr.

joshuagl

Some minor comments in-line, but otherwise LGTM.

tests/test_keydb.py

Co-authored-by: Joshua Lock <jlock@vmware.com> Signed-off-by: marinamoore <mmoore32@calpoly.edu>

trishankatdatadog

💯

Should we have an accompany securesystemslib PR to remove securesystemslib.settings.HASH_ALGORITHMS?

mnm678 · 2020-07-29T15:50:45Z

Should we have an accompany securesystemslib PR to remove securesystemslib.settings.HASH_ALGORITHMS?

Is this setting used by other securesystemslib users, especially intoto? @SantiagoTorres @lukpueh

joshuagl · 2020-08-18T13:00:17Z

Should we have an accompany securesystemslib PR to remove securesystemslib.settings.HASH_ALGORITHMS?

Is this setting used by other securesystemslib users, especially intoto? @SantiagoTorres @lukpueh

Based on a quick git grep of in-toto, it doesn't appear to be used there.

PR theupdateframework#1014 removed uses of keyid_hash_algorithms in favour of using the calculated keyid values from the metadata. A few instances of this removal were unintentionally reintroduced in PR theupdateframework#1016, when changing to explicitly passing a list of hash algorithms rather than changing securesystemslib settings values. This change removes uneccessary uses of keyid_hash_algorithms. Signed-off-by: Joshua Lock <jlock@vmware.com>

lukpueh · 2021-02-01T14:30:14Z

Hi @lorenzo-blasa! What exactly are you trying to achieve? Maybe taking a look at the corresponding PR (#1014) helps to better understand the commit (3c78d67)?

lorenzo-blasa · 2021-02-01T14:50:01Z

@lukpueh many thanks! That PR definitely helps. I'm currently working on having a TUF reference implementation written in JavaScript. The effort started last year and only last week I resumed.

So I was running the local JavaScript tests and comparing that with the version I had in Python and I found it was also failing but maybe because the unit tests were actually referring the TUF package installed instead of the local copy.

Running pip install -r requirements-dev.txt made it all work but it wasn't very clear what had changed as I didn't do a very good job at tracking the revision I was using as base.

It's all good now :)

trishankatdatadog · 2021-02-01T14:52:34Z

@lorenzo-blasa A tuf.js has been a dream of mine for a while now... you might look at my toy reimplementation tuf-on-a-plane to get a few more clarifications or ideas. Thanks for your feedback!

lorenzo-blasa · 2021-02-01T14:58:21Z

@trishankatdatadog thanks for sharing that!

The intention I have with the JS implementation is to open source it as soon as possible, being complete and going through necessary approvals. So it's great to know there's more people interested in it.

lukpueh · 2021-02-01T15:03:22Z

@lorenzo-blasa, tuf.js sounds exciting, please keep us posted! And of course, feel free to ping us if you have any questions regarding the TUF spec or this reference implementation.

jku reviewed Jul 2, 2020

View reviewed changes

tests/test_keydb.py Show resolved Hide resolved

mnm678 mentioned this pull request Jul 17, 2020

Implement TAP 12 #1084

Open

3 tasks

mnm678 requested review from lukpueh and joshuagl July 21, 2020 17:49

joshuagl added the securesystemslib Requires corresponding implementation in securesystemslib label Jul 23, 2020

joshuagl requested changes Jul 23, 2020

View reviewed changes

mnm678 and others added 11 commits July 23, 2020 09:53

Replace use of keyid_hash_algorithms in keydb by using the provided k…

0bdc78d

…eyid. Check that this keyid matches the keyid listed in the key for consistent behavior. Signed-off-by: marinamoore <mmoore32@calpoly.edu>

fix

c84ffaf

Signed-off-by: marinamoore <mmoore32@calpoly.edu>

update tests so that keyids do not have to be verified

3c78d67

Signed-off-by: marinamoore <mmoore32@calpoly.edu>

Remove the use of keyid_hash_algorithms in repository_lib by using th…

68bb55c

…e keyid provided in the delegation. Note that this requires a change to securesystems lib. Signed-off-by: marinamoore <mmoore32@calpoly.edu>

improve comments in keydb tests and add future test for duplicate key

ed2c597

Signed-off-by: marinamoore <mmoore32@calpoly.edu>

update test to not use keyid_hash_algorithms

86f4436

Signed-off-by: marinamoore <mmoore32@calpoly.edu>

remove unused variable

47120e4

Signed-off-by: marinamoore <mmoore32@calpoly.edu>

Apply suggestions from code review

902beb5

Co-authored-by: Joshua Lock <jlock@vmware.com> Signed-off-by: marinamoore <mmoore32@calpoly.edu>

remove redundant setting of keyid after calls to format_metadata_to_key

e283fa1

Signed-off-by: marinamoore <mmoore32@calpoly.edu>

remove unused test

376590d

Signed-off-by: marinamoore <mmoore32@calpoly.edu>

mnm678 force-pushed the remove-keyid_hash_algorithms branch from 8fe9ef7 to 376590d Compare July 23, 2020 16:53

joshuagl approved these changes Jul 28, 2020

View reviewed changes

tests/test_keydb.py Outdated Show resolved Hide resolved

tests/test_keydb.py Outdated Show resolved Hide resolved

Apply suggestions from code review

f96cf50

Co-authored-by: Joshua Lock <jlock@vmware.com> Signed-off-by: marinamoore <mmoore32@calpoly.edu>

mnm678 force-pushed the remove-keyid_hash_algorithms branch from 0c1ee76 to f96cf50 Compare July 28, 2020 13:54

trishankatdatadog reviewed Jul 28, 2020

View reviewed changes

joshuagl merged commit 7b4ffe3 into theupdateframework:develop Aug 18, 2020

joshuagl mentioned this pull request Sep 9, 2020

Remove accidentally introduced uses of keyid_hash_algorithms #1121

Merged

3 tasks

mnm678 deleted the remove-keyid_hash_algorithms branch April 16, 2021 21:01

joshuagl mentioned this pull request Jun 7, 2021

Metadata API: implement sig verification in Key, store id in key #1423

Merged

3 tasks

znewman01 mentioned this pull request Sep 1, 2022

Remove keyid_hash_algorithms theupdateframework/go-tuf#368

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove uses of keyid_hash_algorithms #1014

Remove uses of keyid_hash_algorithms #1014

mnm678 commented Apr 2, 2020

joshuagl left a comment

joshuagl Jul 23, 2020

mnm678 Jul 23, 2020

joshuagl Jul 23, 2020

joshuagl Jul 23, 2020

joshuagl Jul 23, 2020

joshuagl commented Jul 23, 2020

mnm678 commented Jul 23, 2020

mnm678 commented Jul 23, 2020

joshuagl left a comment

trishankatdatadog left a comment

mnm678 commented Jul 29, 2020

joshuagl commented Aug 18, 2020

lukpueh commented Feb 1, 2021

lorenzo-blasa commented Feb 1, 2021

trishankatdatadog commented Feb 1, 2021

lorenzo-blasa commented Feb 1, 2021

lukpueh commented Feb 1, 2021

		self.assertRaises(tuf.exceptions.UnknownKeyError, tuf.keydb.get_key, keyid3)
		#self.assertRaises(tuf.exceptions.UnknownKeyError, tuf.keydb.get_key, keyid4)

Remove uses of keyid_hash_algorithms #1014

Remove uses of keyid_hash_algorithms #1014

Conversation

mnm678 commented Apr 2, 2020

joshuagl left a comment

Choose a reason for hiding this comment

joshuagl Jul 23, 2020

Choose a reason for hiding this comment

mnm678 Jul 23, 2020

Choose a reason for hiding this comment

joshuagl Jul 23, 2020

Choose a reason for hiding this comment

joshuagl Jul 23, 2020

Choose a reason for hiding this comment

joshuagl Jul 23, 2020

Choose a reason for hiding this comment

joshuagl commented Jul 23, 2020

mnm678 commented Jul 23, 2020

mnm678 commented Jul 23, 2020

joshuagl left a comment

Choose a reason for hiding this comment

trishankatdatadog left a comment

Choose a reason for hiding this comment

mnm678 commented Jul 29, 2020

joshuagl commented Aug 18, 2020

lukpueh commented Feb 1, 2021

lorenzo-blasa commented Feb 1, 2021

trishankatdatadog commented Feb 1, 2021

lorenzo-blasa commented Feb 1, 2021

lukpueh commented Feb 1, 2021