Merge pull request #425 from Nitrokey/402-update-credential-2

Update credential support

pynitrokey/cli/nk3/secrets.py

@@ -107,6 +107,71 @@ def call(app: SecretsApp) -> None:
         local_print("Done")
 
 
+@secrets.command()
+@click.pass_obj
+@click.argument(
+    "name",
+    type=click.STRING,
+)
+@click.option(
+    "--login",
+    "login",
+    type=click.STRING,
+    help="Password Safe Login",
+    default=None,
+)
+@click.option(
+    "--password",
+    "password",
+    type=click.STRING,
+    help="Password Safe Password",
+    default=None,
+)
+@click.option(
+    "--metadata",
+    "metadata",
+    type=click.STRING,
+    help="Password Safe Metadata - additional field, to which extra information can be encoded",
+    default=None,
+)
+@click.option(
+    "--touch-button",
+    "touch_button",
+    type=click.BOOL,
+    help="Activate/deactivate touch button requirement",
+    default=None,
+)
+def update(
+    ctx: Context,
+    name: str,
+    new_name: Optional[bytes] = None,
+    login: Optional[bytes] = None,
+    password: Optional[bytes] = None,
+    metadata: Optional[bytes] = None,
+    touch_button: Optional[bool] = None,
+) -> None:
+    """
+    Update credential. Change Static Password fields, or touch button requirement attribute.
+    """
+    with ctx.connect_device() as device:
+        app = SecretsApp(device)
+        ask_to_touch_if_needed()
+
+        @repeat_if_pin_needed
+        def call(app: SecretsApp) -> None:
+            app.update_credential(
+                name.encode(),
+                new_name=new_name,
+                login=login,
+                password=password,
+                metadata=metadata,
+                touch_button=touch_button,
+            )
+
+        call(app)
+        local_print("Done")
+
+
 @secrets.command(aliases=["register"])
 @click.pass_obj
 @click.argument(
@@ -242,7 +307,7 @@ def call(app: SecretsApp) -> None:
     "--metadata",
     "metadata",
     type=click.STRING,
-    help="Password Safe Metadata - additional field, to which extra information can be encoded in the future",
+    help="Password Safe Metadata - additional field, to which extra information can be encoded",
     default=None,
 )
 def add_password(

pynitrokey/nk3/secrets_app.py

@@ -217,6 +217,7 @@ class Instruction(Enum):
     SetPIN = 0xB4
     GetCredential = 0xB5
     RenameCredential = 0xB6
+    UpdateCredential = 0xB7
 
 
 class Tag(Enum):
@@ -489,12 +490,51 @@ def get_credential(self, cred_id: bytes) -> PasswordSafeEntry:
         p.properties = p.properties.hex().encode() if p.properties else None
         return p
 
-    def rename_credential(self, cred_id: bytes, cred_new_id: bytes) -> None:
+    def rename_credential(self, cred_id: bytes, new_name: bytes) -> None:
+        """
+        Rename credential.
+        An alias for the update_credential() call.
+        @param cred_id: The credential ID to modify
+        @param new_name: New ID for the credential
+        """
+        return self.update_credential(cred_id, new_name)
+
+    def update_credential(
+        self,
+        cred_id: bytes,
+        new_name: Optional[bytes] = None,
+        login: Optional[bytes] = None,
+        password: Optional[bytes] = None,
+        metadata: Optional[bytes] = None,
+        touch_button: Optional[bool] = None,
+    ) -> None:
+        """
+        Update credential fields - name, attributes, and PWS fields.
+        Unpopulated fields will not be encoded and used during the update process
+        (won't change the current value).
+        @param cred_id: The credential ID to modify
+        @param new_name: New ID for the credential
+        @param login: New login field content
+        @param password: New password field content
+        @param metadata: New metadata field content
+        @param touch_button: Set if the touch button use should be required
+        """
         structure = [
             tlv8.Entry(Tag.CredentialId.value, cred_id),
-            tlv8.Entry(Tag.CredentialId.value, cred_new_id),
+            tlv8.Entry(Tag.CredentialId.value, new_name) if new_name else None,
+            self.encode_properties_to_send(touch_button, False, tlv=True)
+            if touch_button is not None
+            else None,
+            tlv8.Entry(Tag.PwsLogin.value, login) if login is not None else None,
+            tlv8.Entry(Tag.PwsPassword.value, password)
+            if password is not None
+            else None,
+            tlv8.Entry(Tag.PwsMetadata.value, metadata)
+            if metadata is not None
+            else None,
         ]
-        self._send_receive(Instruction.RenameCredential, structure=structure)
+        structure = list(filter(lambda x: x is not None, structure))
+        self._send_receive(Instruction.UpdateCredential, structure=structure)
 
     def delete(self, cred_id: bytes) -> None:
         """
@@ -566,13 +606,7 @@ def register(
             tlv8.Entry(
                 Tag.Key.value, bytes([kind.value | algo.value, digits]) + secret
             ),
-            RawBytes(
-                [
-                    Tag.Properties.value,
-                    (0x02 if touch_button_required else 0x00)
-                    | (0x04 if pin_based_encryption else 0x00),
-                ]
-            ),
+            self.encode_properties_to_send(touch_button_required, pin_based_encryption),
             tlv8.Entry(
                 Tag.InitialCounter.value, initial_counter_value.to_bytes(4, "big")
             )
@@ -585,6 +619,25 @@ def register(
         structure = list(filter(lambda x: x is not None, structure))
         self._send_receive(Instruction.Put, structure)
 
+    @classmethod
+    def encode_properties_to_send(
+        cls, touch_button_required: bool, pin_based_encryption: bool, tlv: bool = False
+    ) -> RawBytes:
+        """
+        Encode properties structure into a single byte
+        @param touch_button_required: whether the touch button use is required
+        @param pin_based_encryption: whether the PIN-encryption is requested (only during registration)
+        @param tlv: set True, if this should be encoded as TLV, as opposed to the default "TV", w/o L
+        """
+        structure = [
+            Tag.Properties.value,
+            1 if tlv else None,
+            (0x02 if touch_button_required else 0x00)
+            | (0x04 if pin_based_encryption else 0x00),
+        ]
+        structure = list(filter(lambda x: x is not None, structure))
+        return RawBytes(structure)
+
     def calculate(self, cred_id: bytes, challenge: Optional[int] = None) -> bytes:
         """
         Calculate the OTP code for the credential named `cred_id`, and with challenge `challenge`.

pynitrokey/test_secrets_app.py

@@ -1703,6 +1703,11 @@ def test_rename_credential(secretsAppResetLogin):
     assert len(l) == 1
     assert l[0].decode() == CREDID2
 
+    # Old name should not be accessible
+    app.verify_pin_raw(PIN)
+    with pytest.raises(SecretsAppException, match="NotFound"):
+        app.get_credential(CREDID)
+
 
 @pytest.mark.parametrize(
     "cred1_encryption", [True, False], ids=lambda x: "cred1" + ("_enc" if x else "")
@@ -1743,3 +1748,84 @@ def test_rename_credential_to_existing(
     app.verify_pin_raw(PIN)
     # There should be still 2 credentials left
     assert set([CREDID.encode(), CREDID2.encode()]) == set(app.list())
+
+
+def test_update_credential(secretsAppResetLogin):
+    """
+    Credential should change its properties. Test both PIN- and HW-encrypted credentials.
+    """
+    app = secretsAppResetLogin
+    app.register(CREDID, SECRET, DIGITS, kind=Kind.Hotp)
+    app.verify_pin_raw(PIN)
+    l = app.list_with_properties()
+    assert len(l) == 1
+    assert l[0].label.decode() == CREDID
+    assert not l[0].properties.touch_required
+    app.verify_pin_raw(PIN)
+    app.update_credential(CREDID, touch_button=True)
+    # There should be only one credential left, with the same name
+    app.verify_pin_raw(PIN)
+    l = app.list_with_properties()
+    assert len(l) == 1
+    assert l[0].label.decode() == CREDID
+    # This credential should be listed now as requiring a touch button for use
+    assert l[0].properties.touch_required
+
+    # Now add some PWS fields, and rename it too
+    app.verify_pin_raw(PIN)
+    c = app.get_credential(CREDID)
+    assert not c.login
+    assert not c.password
+    assert not c.metadata
+    app.verify_pin_raw(PIN)
+    app.update_credential(
+        CREDID,
+        new_name=CREDID2,
+        login=b"login",
+        password=b"password",
+        metadata=b"metadata",
+    )
+
+    # Check if PWS fields are there, and the "touch button required" flag is still present
+    app.verify_pin_raw(PIN)
+    c = app.get_credential(CREDID2)
+    assert c.login == b"login"
+    assert c.password == b"password"
+    assert c.metadata == b"metadata"
+    app.verify_pin_raw(PIN)
+    l = app.list_with_properties()
+    assert len(l) == 1
+    assert l[0].properties.touch_required
+
+    # Old name should not be accessible
+    app.verify_pin_raw(PIN)
+    with pytest.raises(SecretsAppException, match="NotFound"):
+        app.get_credential(CREDID)
+
+    # Try to remove the PWS data with empty strings, and rename again
+    app.verify_pin_raw(PIN)
+    app.update_credential(
+        CREDID2, new_name=CREDID, login=b"", password=b"", metadata=b""
+    )
+    app.verify_pin_raw(PIN)
+    c = app.get_credential(CREDID)
+    assert c.login is None
+    assert c.password is None
+    assert c.metadata is None
+
+    # Disallow to register a PWS credential with any 0-length strings field
+    app.verify_pin_raw(PIN)
+    with pytest.raises(SecretsAppException, match="IncorrectDataParameter"):
+        app.register(
+            CREDID2,
+            SECRET,
+            DIGITS,
+            kind=Kind.Hotp,
+            login=b"",
+            password=b"",
+            metadata=b"",
+        )
+    for i in ["login", "password", "metadata"]:
+        fields = {i: b""}
+        with pytest.raises(SecretsAppException, match="IncorrectDataParameter"):
+            app.register(CREDID2, SECRET, DIGITS, kind=Kind.Hotp, **fields)