Raise an error on loading/writing unsigned metadata

[sechkova]

Fixes issue #: #1050

Description of the changes being introduced by the pull request:

• Modify check_signable_object_format() to raise an error if signable has an empty 'signatures' list.
• Remove usage of empty signing keys lists in developer_tool
• Update test cases which use unsigned metadata.

Note Marked as WIP since test_root_rotation_missing_keys correctly fails due to a known verification issue on the Updater (#1101)

Please verify and check that the pull request fulfills the following
requirements:

• The code follows the Code Style Guidelines
• Tests have been added for the bug fix or new feature
• Docs have been added for the bug fix or new feature

[sechkova]

No longer WIP since the merging of #1101 and ready for review.

[joshuagl]

Thanks for the PR @sechkova. Obviously this needs rebasing on recent changes in develop and I've requested a change in test_developer_tool to ensure we're testing behaviour we expect to support.

I've also added a question to my more experienced colleagues on the use of partially signed metadata.

[sechkova]

Thanks all for sharing your opinion!
Taking the discussion into account I decided to keep the error raised in check_signable_object_format() if signable has an empty 'signatures' list but to except it and log a warning in repository_lib (and developer_tool).
This way we both keep the current functionality allowing for signing offline and we require developers to justify their actions when using check_signable_object_format() with unsigned metadata. And the error is still raised on the client side when loading unsigned metadata file.

I removed the assert cases expecting an error from the tests but still kept the rest which in my opinion improves their logic in general.

Let me know what do you think

[joshuagl]

I like that a client loading unsigned metadata causes an error, yet we still support generating metadata on the repository for offline signing.

What do you think @trishankatdatadog @mnm678 ?

[mnm678]

This looks good to me! What should the behavior be is there are less than a threshold of signatures on the metadata? Should this also cause the error?

[trishankatdatadog]

Client definitely has to unwaveringly and uncompromisingly strict: no threshold of sigs, no go.

As for repository/developer tools, I'm okay with explicit warnings.

[sechkova]

This looks good to me! What should the behavior be is there are less than a threshold of signatures on the metadata? Should this also cause the error?

The threshold of signatures is validated by sig.verify() and the changes in this PR don't affect this.

In the client logic sig.verify() is always called (often hidden down in the call stack) after check_signable_object_format().
The only exception is Updater.__init__ where metadata which is assumed trusted is loaded. In this case check_signable_object_format() will now raise an error if the metadata is unsigned.

Anyway, if you have some doubts and you think it is worth the effort I can add a couple of tests for updater that will clarify this behaviour.

[mnm678]

Sorry, my question wasn't clear. I meant what happens if the repository tries to write metadata that has less than a threshold of signatures? The error will be raised if there are no signatures, but I don't think it will if there are less than a threshold of signatures.

[sechkova]

Let me try to explain my vision of it and hopefully all are on the same page.

On the repository side:

1. Current behaviour (before this set of changes):

• sig.verify ensures that threshold of signatures is met during writing of metadata
• if allow_partially_signed is enabled, there is no check of a threshold of signatures or if metadata is signed at all

A more detailed discussion is in #1050.

2. After the changes included in this PR:

• sig.verify ensures that threshold of signatures is met during writing of metadata
• if allow_partially_signed is enabled, after taking into account the discussion here, only a warning will be logged when the metadata is unsigned. Nothing will happen if the metadata is signed but a threshold of signatures is not met (I guess this is expected since allow_partially_signed is chosen). Maybe another warning in this case?

[mnm678]

Let me try to explain my vision of it and hopefully all are on the same page.

Thanks, I think this helps.

Nothing will happen if the metadata is signed but a threshold of signatures is not met (I guess this is expected since `allow_partially_signed` is chosen). Maybe another warning in this case?

This is what I wanted to make sure we clarify. If allow_partially_signed is already specified, I'm not sure another warning is needed, but I just want to make sure the behavior makes sense to users.

[joshuagl]

Thanks for the explanations @sechkova and for the discussion @mnm678 – seems like we agree this change is good to merge?

[joshuagl]

Excellent, thanks!