From 81330b05bfef284de94e40205c97179e170735c6 Mon Sep 17 00:00:00 2001 From: Szczepan Zalega Date: Thu, 10 Aug 2023 18:21:31 +0200 Subject: [PATCH] Update credential support Add tests for credential update with PWS and touch button requirement --- pynitrokey/nk3/secrets_app.py | 47 +++++++++++++++++----- pynitrokey/test_secrets_app.py | 71 ++++++++++++++++++++++++++++++++++ 2 files changed, 109 insertions(+), 9 deletions(-) diff --git a/pynitrokey/nk3/secrets_app.py b/pynitrokey/nk3/secrets_app.py index 2cc05b10..d80eedec 100644 --- a/pynitrokey/nk3/secrets_app.py +++ b/pynitrokey/nk3/secrets_app.py @@ -217,6 +217,7 @@ class Instruction(Enum): SetPIN = 0xB4 GetCredential = 0xB5 RenameCredential = 0xB6 + UpdateCredential = 0xB7 class Tag(Enum): @@ -490,11 +491,32 @@ def get_credential(self, cred_id: bytes) -> PasswordSafeEntry: return p def rename_credential(self, cred_id: bytes, cred_new_id: bytes) -> None: + return self.update_credential(cred_id, cred_new_id) + + def update_credential( + self, + cred_id: bytes, + cred_new_id: Optional[bytes] = None, + login: Optional[bytes] = None, + password: Optional[bytes] = None, + metadata: Optional[bytes] = None, + touch_button: Optional[bool] = None, + ) -> None: structure = [ tlv8.Entry(Tag.CredentialId.value, cred_id), - tlv8.Entry(Tag.CredentialId.value, cred_new_id), + tlv8.Entry(Tag.CredentialId.value, cred_new_id) if cred_new_id else None, + tlv8.Entry(Tag.Properties.value, b"\x01") + 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) + self._send_receive(Instruction.UpdateCredential, structure=structure) def delete(self, cred_id: bytes) -> None: """ @@ -566,13 +588,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 +601,19 @@ 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 + ) -> bytes: + r = RawBytes( + [ + Tag.Properties.value, + (0x02 if touch_button_required else 0x00) + | (0x04 if pin_based_encryption else 0x00), + ] + ) + return r + 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`. diff --git a/pynitrokey/test_secrets_app.py b/pynitrokey/test_secrets_app.py index e972c5bf..89f04c47 100644 --- a/pynitrokey/test_secrets_app.py +++ b/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,69 @@ 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, + cred_new_id=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, and rename again + # It's not possible currently to remove the PWS fields; hence we need to overwrite them + # with a single blank character. + app.verify_pin_raw(PIN) + app.update_credential( + CREDID2, cred_new_id=CREDID, login=b" ", password=b" ", metadata=b" " + ) + app.verify_pin_raw(PIN) + c = app.get_credential(CREDID) + assert c.login == b" " + assert c.password == b" " + assert c.metadata == b" "