Merge pull request #87 from jku/target-mgmt-v3

signer: Compare target changes to known good metadata

playground/README.md

@@ -101,16 +101,6 @@ Notes on remotes configured in `.playground-sign.ini`:
    ```
 1. Respond to the prompts
 
-
-### Modify target files
-
-1. Add, remove or modify files under targets/ directory
-1. Run signer tool
-   ```
-   playground-sign <event-name>
-   ```
-1. Respond to the prompts
-
 ### Add a delegation or modify an existing one
 
 1. Run delegate tool when you want to modify a roles delegation
@@ -119,6 +109,30 @@ Notes on remotes configured in `.playground-sign.ini`:
    ```
 1. Respond to the prompts
 
+### Modify target files
+
+1. Make target file changes in the signing event git branch
+   * Choose a signing event name, create a branch
+     ```
+     git fetch origin
+     git switch -C sign/my-target-changes origin/main
+     ```
+   * Make changes with tools of your choosing:
+     ```
+     echo "test content" > targets/file.txt
+     git add targets/file.txt
+     git commit -m "Add a target file"
+     ```
+   * Submit changes to a signing event branch on the repository (by pushing to repository
+     or by using a PR to a signing event branch): This starts a signing event
+     ```
+     git push origin sign/my-target-changes
+     ```
+1. Update targets metadata
+   ```
+   playground-sign sign/my-target-changes
+   ```
+
 ### Sign changes made by others
 
 Signing should be done when the signing event (GitHub issue) asks for it:

playground/repo/pyproject.toml

@@ -12,7 +12,8 @@ version = "0.0.1"
 description = "CI tools for Repository Plaground"
 readme = "README.md"
 dependencies = [
-  "securesystemslib[gcpkms, sigstore, pynacl] @ git+https://github.com/secure-systems-lab/securesystemslib", 
+  "sigstore @ git+https://github.com/sigstore/sigstore-python",
+  "securesystemslib[gcpkms, sigstore, pynacl] @ git+https://github.com/secure-systems-lab/securesystemslib",
   "tuf @ git+https://github.com/theupdateframework/python-tuf",
   "click",
 ]

playground/signer/playground_sign/_signer_repository.py

@@ -26,7 +26,9 @@
 KEY_FOR_TYPE_AND_SCHEME[("sigstore-oidc", "Fulcio")] = SigstoreKey
 SIGNER_FOR_URI_SCHEME[SigstoreSigner.SCHEME] = SigstoreSigner
 
-
+# NOTE This signer state should likely be just separate attributes
+# of the SignerRepository: It should be possible to have multiple states
+# "on" at the same time (e.g. INVITED, TARGETS_CHANGED & SIGNATURE_NEEDED)
 @unique
 class SignerState(Enum):
     NO_ACTION = 0,
@@ -145,13 +147,9 @@ def __init__(self, dir: str, prev_dir: str, user_name: str, secret_func: Callabl
                 config = json.load(f)
             self._invites = config["invites"]
 
-        # Find local target file changes
-        # NOTE comparison is between target-files-on-disk vs current metadata-on-disk
-        # So this state is for _local_ changes initiated by this user
-        # * possibly the comparison should be against upstream branch metadata:
-        #   to cover the case of running the tool multiple times
-        # * possibly similar functionality is required to present upstream change
-        #   to signer to make an informed decision about signing
+        # Find changes between known good metadata and the target files in signing event.
+        # NOTE: Currently target file location is hard coded to a directory in the git-tree
+        # There is a plan to expose an external targets location in the UI as well.
         target_dir = os.path.join(self._dir, "..", "targets")
         self.target_changes = self._get_target_states(target_dir)
 
@@ -166,7 +164,7 @@ def __init__(self, dir: str, prev_dir: str, user_name: str, secret_func: Callabl
             self.state = SignerState.UNINITIALIZED
         elif self.invites:
             self.state = SignerState.INVITED
-        elif self.target_changes:
+        elif self._unapplied_target_changes():
             self.state = SignerState.TARGETS_CHANGED
         elif self.unsigned:
             self.state = SignerState.SIGNATURE_NEEDED
@@ -184,25 +182,43 @@ def invites(self) -> list[str]:
     def _get_target_states(self, target_dir: str) -> dict[str, dict[str, TargetState]]:
         """Returns current state of target files vs target metadata.
 
+        Current state of target files comes from given targets directory.
+        Target metadata on the other hand is from the "known good metadata state".
+
         Raises ValueError if target files have been added for a role that does not exist.
-        First dict key in return value is rolename, second is targetpath
+        First dict key in return value is rolename, second key is targetpath
         """
 
-        # Check what targets we have on disk, mark the as ADDED for now
+        # Check what targets we have in the signing event, mark them as ADDED for now
         target_states = TargetStates(target_dir)
 
-        # Update target states based on all current targets metadata
-        targets = self.targets()
+        # Update target states based on targets metadata in known good state
+        targets = self._known_good_targets("targets")
         target_states.update_target_states("targets", targets)
         if targets.delegations and targets.delegations.roles:
             for rolename in targets.delegations.roles:
-                target_states.update_target_states(rolename, self.targets(rolename))
+                target_states.update_target_states(rolename, self._known_good_targets(rolename))
 
         if target_states.unknown_rolenames:
             raise ValueError(f"Targets have been added for unknown roles {target_states.unknown_rolenames}")
 
         return target_states
 
+    def _unapplied_target_changes(self) -> bool:
+        """Returns True if there are target changes in the signing event branch that are
+        not yet included in the signing event metadata"""
+        for rolename, target_states in self.target_changes.items():
+            targets = self.targets(rolename)
+            for path, target_state in target_states.items():
+                if target_state.state == State.REMOVED:
+                    if path in targets.targets:
+                        return True
+                else:
+                    if path not in targets.targets or targets.targets[target_state.target.path] != target_state.target:
+                        return True
+
+        return False
+
     def _user_signature_needed(self, rolename: str) -> bool:
         """Return true if current role metadata is unsigned by user"""
         md = self.open(rolename)
@@ -222,7 +238,7 @@ def _get_filename(self, role: str) -> str:
     def _get_versioned_root_filename(self, version: int) -> str:
         return os.path.join(self._dir, "root_history", f"{version}.root.json")
 
-    def _prev_version(self, rolename: str) -> int:
+    def _known_good_version(self, rolename: str) -> int:
         prev_path = os.path.join(self._prev_dir, f"{rolename}.json")
         if os.path.exists(prev_path):
             with open(prev_path, "rb") as f:
@@ -303,7 +319,7 @@ def open(self, role:str) -> Metadata:
     def close(self, role: str, md: Metadata) -> None:
         """Write metadata to a file in the repository directory"""
         # Make sure version is bumped only once per signing event
-        md.signed.version = self._prev_version(role) + 1
+        md.signed.version = self._known_good_version(role) + 1
 
         # Set expiry based on custom metadata
         days = md.signed.unrecognized_fields["x-playground-expiry-period"]
@@ -333,6 +349,18 @@ def close(self, role: str, md: Metadata) -> None:
 
         self._write(role, md)
 
+    def _known_good_targets(self, rolename: str) -> Targets:
+        prev_path = os.path.join(self._prev_dir, f"{rolename}.json")
+        if os.path.exists(prev_path):
+            with open(prev_path, "rb") as f:
+                md = Metadata.from_bytes(f.read())
+            assert isinstance(md.signed, Targets)
+            return md.signed
+        else:
+            # this role did not exist: return an empty one for comparison purposes
+            return Targets()
+
+
     @staticmethod
     def _get_delegated_rolenames(md: Metadata) -> list[str]:
         if isinstance(md.signed, Root):
@@ -524,14 +552,21 @@ def status(self, rolename: str) -> str:
         return "TODO: Describe the changes in the signing event for this role"
 
     def update_targets(self):
-        """Modify targets metadata to match targets on disk and sign"""
+        """Modify targets metadata to match the target file changes and sign
+
+        Start with 'known good' TargetFiles: the metadata in the signing
+        event could have been changed in unpredictable ways: target_changes
+        documents changes from known good state"""
         for rolename, target_states in self.target_changes.items():
+            known_good_targets = self._known_good_targets(rolename).targets
+            for target_state in target_states.values():
+                if target_state.state == State.REMOVED:
+                    del known_good_targets[target_state.target.path]
+                else:
+                    known_good_targets[target_state.target.path] = target_state.target
+
             with self.edit_targets(rolename) as targets:
-                for target_state in target_states.values():
-                    if target_state.state == State.REMOVED:
-                        del targets.targets[target_state.target.path]
-                    else:
-                        targets.targets[target_state.target.path] = target_state.target
+                targets.targets = known_good_targets
 
     def sign(self, rolename: str):
         """Sign without payload changes"""

playground/signer/playground_sign/sign.py

@@ -54,30 +54,20 @@ def sign(verbose: int, push: bool, event_name: str):
                     click.echo(repo.status(rolename))
                     repo.sign(rolename)
             changed = True
-        elif repo.state == SignerState.SIGNATURE_NEEDED:
-            click.echo(f"Your signature is requested for role(s) {repo.unsigned}.")
-            for rolename in repo.unsigned:
-                click.echo(repo.status(rolename))
-                repo.sign(rolename)
-            changed = True
         elif repo.state == SignerState.TARGETS_CHANGED:
-            click.echo(f"Following local target files changes have been found:")
+            click.echo(f"Target file changes have been found in this signing event:")
             for rolename, states in repo.target_changes.items():
                 for target_state in states.values():
                     click.echo(f"  {target_state.target.path} ({target_state.state.name})")
             click.prompt(bold("Press enter to approve these changes"), default=True, show_default=False)
 
             repo.update_targets()
-
-            for rolename, states in repo.target_changes.items():
-                for target_state in states.values():
-                    parent, _, name = target_state.target.path.rpartition("/")
-                    path = os.path.join("targets", parent, name)
-                    if target_state.state == State.REMOVED:
-                        git_expect(["rm", "--", path])
-                    else:
-                        git_expect(["add", "--", path])
-
+            changed = True
+        elif repo.state == SignerState.SIGNATURE_NEEDED:
+            click.echo(f"Your signature is requested for role(s) {repo.unsigned}.")
+            for rolename in repo.unsigned:
+                click.echo(repo.status(rolename))
+                repo.sign(rolename)
             changed = True
         elif repo.state == SignerState.NO_ACTION:
             changed = False

playground/signer/pyproject.toml

@@ -12,6 +12,7 @@ version = "0.0.1"
 description = "TUF signing tool for Repository Plaground"
 readme = "README.md"
 dependencies = [
+  "sigstore @ git+https://github.com/sigstore/sigstore-python",
   "securesystemslib[gcpkms,hsm,sigstore] @ git+https://github.com/secure-systems-lab/securesystemslib",
   "tuf @ git+https://github.com/theupdateframework/python-tuf",
   "click",