security: create key files with 0o600 permissions atomically

CHANGELOG.md

@@ -4,6 +4,15 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/)
 and this project adheres to [Semantic Versioning](http://semver.org/).
 
+## [Unreleased]
+
+**Security:**
+
+- Key files are now created with `0o600` permissions on POSIX and refuse to overwrite or
+  follow existing paths. `CBORSerializable.save()` now writes atomically with
+  `O_EXCL`/`O_CREAT`, closing a TOCTOU race and preventing world-readable key files. A new
+  `mode` keyword allows callers of non-secret objects to opt into wider permissions.
+
 ## [0.8.1] - 2023-04-06
 
 This patch contains a number of bug fixes to `v0.8.0`.

docs/source/guides/address.rst

@@ -50,6 +50,13 @@ A key could be saved to and loaded from a file::
     >>> payment_signing_key = payment_signing_key.load("payment.skey")
     >>> payment_verification_key = payment_verification_key.load("payment.vkey")
 
+.. note::
+    On POSIX systems, key files are created with owner-only (``0o600``) permissions so that
+    other local users cannot read your secret keys. ``save()`` also refuses to overwrite or
+    follow an existing file. POSIX file modes are advisory on Windows, where this protection
+    is not enforced — Windows users should rely on NTFS ACLs or disk encryption to protect
+    key files.
+
 Signing keys can sign messages (in bytes)::
 
     >>> message = b"Hello world!"

pycardano/serialization.py

@@ -631,27 +631,55 @@ def save(
         path: str,
         key_type: Optional[str] = None,
         description: Optional[str] = None,
+        mode: int = 0o600,
         **kwargs,
     ):
         """
         Save the CBORSerializable object to a file in JSON format.
 
         This method writes the object's JSON representation to the specified file path.
-         It raises an error if the file already exists and is not empty.
+        The file is created atomically and refuses to overwrite or follow an existing
+        path. Because objects such as :class:`~pycardano.key.SigningKey` may contain
+        secret material, the file is created with owner-only (``0o600``) permissions by
+        default on POSIX systems.
+
+        Note:
+            POSIX file modes are advisory on Windows, where ``os.chmod`` only toggles the
+            read-only bit. Owner-only enforcement is therefore POSIX-only; Windows users
+            should rely on NTFS ACLs or disk encryption to protect key files.
 
         Args:
             path (str): The file path to save the object to.
             key_type (str, optional): The type to use in the JSON output. Defaults to the class name.
             description (str, optional): The description to use in the JSON output. Defaults to the class docstring.
+            mode (int): The file permission bits to create the file with. Defaults to
+                ``0o600`` (owner read/write only), which is appropriate for files that may
+                hold secret key material.
             **kwargs: Extra key word arguments to be passed to `json.dumps()`
 
         Raises:
-            IOError: If the file already exists and is not empty.
+            IOError: If the file already exists.
         """
-        if os.path.isfile(path) and os.stat(path).st_size > 0:
+        payload = self.to_json(key_type=key_type, description=description, **kwargs)
+        # O_EXCL makes creation atomic: it fails if the path already exists (closing the
+        # TOCTOU race in the old os.path.isfile check) and refuses to follow a pre-existing
+        # symlink. The mode is subject to the process umask, so chmod afterwards to
+        # guarantee the requested mode regardless of umask.
+        try:
+            fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_EXCL, mode)
+        except FileExistsError:
             raise IOError(f"File {path} already exists!")
-        with open(path, "w") as f:
-            f.write(self.to_json(key_type=key_type, description=description, **kwargs))
+        try:
+            with os.fdopen(fd, "w") as f:
+                os.chmod(path, mode)
+                f.write(payload)
+        except BaseException:
+            # The file was created by us; remove the partial/empty file on failure.
+            try:
+                os.unlink(path)
+            except OSError:
+                pass
+            raise
 
     @classmethod
     def load(cls, path: str):

test/pycardano/test_address.py

@@ -1,6 +1,3 @@
-import os
-import tempfile
-
 import pytest
 
 from pycardano.address import Address, AddressType, PointerAddress
@@ -214,18 +211,11 @@ def test_from_primitive_invalid_type_addr():
         Address.from_primitive(value)
 
 
-def test_save_load_address():
+def test_save_load_address(tmp_path):
     address_string = "addr_test1vr2p8st5t5cxqglyjky7vk98k7jtfhdpvhl4e97cezuhn0cqcexl7"
     address = Address.from_primitive(address_string)
 
-    # On Windows, NamedTemporaryFile keeps the file locked while open, so
-    # save() cannot open it for writing. Use delete=False and close the handle
-    # first, then clean up manually afterward.
-    with tempfile.NamedTemporaryFile(delete=False) as f:
-        tmp_path = f.name
-    try:
-        address.save(tmp_path)
-        loaded_address = Address.load(tmp_path)
-        assert address == loaded_address
-    finally:
-        os.unlink(tmp_path)
+    path = str(tmp_path / "address.txt")
+    address.save(path)
+    loaded_address = Address.load(path)
+    assert address == loaded_address

test/pycardano/test_key.py

@@ -1,7 +1,7 @@
 import json
 import os
 import pathlib
-import tempfile
+import stat
 
 import pytest
 from mnemonic import Mnemonic
@@ -204,43 +204,36 @@ def test_stake_pool_key_load():
     assert vk == StakePoolVerificationKey.from_json(vk.to_json())
 
 
-def test_key_save():
-    # On Windows, NamedTemporaryFile keeps the file locked while open.
-    # Use delete=False and close the handle first, then clean up manually.
-    with tempfile.NamedTemporaryFile(delete=False) as f:
-        tmp_path = f.name
-    try:
-        SK.save(tmp_path)
-        sk = PaymentSigningKey.load(tmp_path)
-        assert SK == sk
-    finally:
-        os.unlink(tmp_path)
-
-
-def test_key_save_invalid_address():
-    with tempfile.NamedTemporaryFile(delete=False) as f:
-        tmp_path = f.name
-    try:
-        SK.save(tmp_path)
-        with pytest.raises(IOError):
-            VK.save(tmp_path)
-    finally:
-        os.unlink(tmp_path)
-
-
-def test_stake_pool_key_save():
-    with tempfile.NamedTemporaryFile(delete=False) as skf:
-        sk_path = skf.name
-    with tempfile.NamedTemporaryFile(delete=False) as vkf:
-        vk_path = vkf.name
-    try:
-        SPSK.save(sk_path)
-        sk = StakePoolSigningKey.load(sk_path)
-        SPVK.save(vk_path)
-        vk = StakePoolSigningKey.load(vk_path)
-    finally:
-        os.unlink(sk_path)
-        os.unlink(vk_path)
+def test_key_save(tmp_path):
+    path = str(tmp_path / "payment.skey")
+    SK.save(path)
+    sk = PaymentSigningKey.load(path)
+    assert SK == sk
+
+
+@pytest.mark.skipif(os.name == "nt", reason="POSIX file modes are advisory on Windows")
+def test_key_save_mode_is_owner_only(tmp_path):
+    path = str(tmp_path / "payment.skey")
+    SK.save(path)
+    # The key file must be readable/writable by its owner only (0o600).
+    assert stat.S_IMODE(os.stat(path).st_mode) == 0o600
+
+
+def test_key_save_invalid_address(tmp_path):
+    path = str(tmp_path / "payment.skey")
+    SK.save(path)
+    # save() refuses to overwrite the existing file.
+    with pytest.raises(IOError):
+        VK.save(path)
+
+
+def test_stake_pool_key_save(tmp_path):
+    sk_path = str(tmp_path / "cold.skey")
+    vk_path = str(tmp_path / "cold.vkey")
+    SPSK.save(sk_path)
+    sk = StakePoolSigningKey.load(sk_path)
+    SPVK.save(vk_path)
+    vk = StakePoolSigningKey.load(vk_path)
     assert SPSK == sk
     assert SPVK == vk
 

test/pycardano/test_serialization.py

@@ -1,6 +1,6 @@
 import json
 import os
-import tempfile
+import stat
 from collections import defaultdict, deque
 from copy import deepcopy
 from dataclasses import dataclass, field
@@ -1016,7 +1016,7 @@ class TestData(MapCBORSerializable):
     assert s_copy[0].value == 100
 
 
-def test_save_load():
+def test_save_load(tmp_path):
     @dataclass
     class Test1(CBORSerializable):
         a: str
@@ -1048,17 +1048,13 @@ def to_shallow_primitive(self) -> Union[Primitive, CBORSerializable]:
     assert test1_json["description"] == "Test Description"
     assert test1_json["cborHex"] == test1.to_cbor_hex()
 
-    with tempfile.NamedTemporaryFile(delete=False) as f:
-        tmp_path = f.name
-    try:
-        test1.save(tmp_path)
-        loaded = Test1.load(tmp_path)
-        assert test1 == loaded
+    path = str(tmp_path / "test1.json")
+    test1.save(path)
+    loaded = Test1.load(path)
+    assert test1 == loaded
 
-        with pytest.raises(IOError):
-            test1.save(tmp_path)
-    finally:
-        os.unlink(tmp_path)
+    with pytest.raises(IOError):
+        test1.save(path)
 
 
 def test_ordered_set_as_key_in_dict():
@@ -1176,3 +1172,69 @@ def test_liqwid_tx():
         cbor_hex = json.load(f).get("cborHex")
     tx = Transaction.load("test/resources/cbors/liqwid.json")
     assert tx.to_cbor().hex() == cbor_hex
+
+
+@dataclass
+class _SaveTest(CBORSerializable):
+    a: str
+
+    @classmethod
+    def from_primitive(
+        cls: Type[CBORSerializable], value: Any, type_args: Optional[tuple] = None
+    ) -> CBORSerializable:
+        return _SaveTest(a=value["a"])
+
+    def to_shallow_primitive(self) -> Union[Primitive, CBORSerializable]:
+        return {"a": self.a}
+
+
+@pytest.mark.skipif(os.name == "nt", reason="POSIX file modes are advisory on Windows")
+def test_save_default_mode_is_owner_only(tmp_path):
+    path = str(tmp_path / "k.skey")
+    _SaveTest(a="secret").save(path)
+    # Secret files default to owner read/write only (0o600).
+    assert stat.S_IMODE(os.stat(path).st_mode) == 0o600
+
+
+@pytest.mark.skipif(os.name == "nt", reason="POSIX file modes are advisory on Windows")
+def test_save_custom_mode(tmp_path):
+    path = str(tmp_path / "tx.json")
+    # Non-secret callers can opt into wider permissions via the mode keyword.
+    _SaveTest(a="public").save(path, mode=0o644)
+    assert stat.S_IMODE(os.stat(path).st_mode) == 0o644
+
+
+def test_save_refuses_existing_file(tmp_path):
+    obj = _SaveTest(a="x")
+    path = str(tmp_path / "k.json")
+
+    # A non-empty existing file is rejected.
+    obj.save(path)
+    with pytest.raises(IOError):
+        obj.save(path)
+    os.unlink(path)
+
+    # An empty pre-existing file is also rejected (O_EXCL tightening).
+    open(path, "w").close()
+    with pytest.raises(IOError):
+        obj.save(path)
+
+
+@pytest.mark.skipif(os.name == "nt", reason="symlink semantics differ on Windows")
+def test_save_does_not_follow_symlink(tmp_path):
+    target = tmp_path / "target"
+    link = tmp_path / "link"
+    target.touch()
+    link.symlink_to(target)
+
+    with pytest.raises(IOError):
+        _SaveTest(a="x").save(str(link))
+    # The pre-existing target must not have been written through the link.
+    assert target.stat().st_size == 0
+
+
+def test_save_round_trips(tmp_path):
+    obj = _SaveTest(a="round-trip")
+    path = str(tmp_path / "k.json")
+    obj.save(path)
+    assert _SaveTest.load(path) == obj

test/pycardano/test_transaction.py

@@ -1,5 +1,3 @@
-import os
-import tempfile
 from dataclasses import dataclass
 from fractions import Fraction
 from test.pycardano.util import check_two_way_cbor
@@ -244,7 +242,7 @@ def test_transaction():
     assert expected_tx_id == signed_tx.id
 
 
-def test_transaction_save_load():
+def test_transaction_save_load(tmp_path):
     tx_cbor = (
         "84a70081825820b35a4ba9ef3ce21adcd6879d08553642224304704d206c74d3ffb3e6eed3ca28000d80018182581d60cc"
         "30497f4ff962f4c1dca54cceefe39f86f1d7179668009f8eb71e598200a1581cec8b7d1dd0b124e8333d3fa8d818f6eac0"
@@ -264,14 +262,10 @@ def test_transaction_save_load():
     )
     tx = Transaction.from_cbor(tx_cbor)
 
-    with tempfile.NamedTemporaryFile(delete=False) as f:
-        tmp_path = f.name
-    try:
-        tx.save(tmp_path)
-        loaded_tx = Transaction.load(tmp_path)
-        assert tx == loaded_tx
-    finally:
-        os.unlink(tmp_path)
+    path = str(tmp_path / "tx.json")
+    tx.save(path)
+    loaded_tx = Transaction.load(path)
+    assert tx == loaded_tx
 
 
 def test_multi_asset():

test/pycardano/test_witness.py

@@ -1,6 +1,4 @@
 import json
-import os
-import tempfile
 
 from pycardano import (
     PaymentSigningKey,
@@ -12,24 +10,20 @@
 )
 
 
-def test_witness_save_load():
+def test_witness_save_load(tmp_path):
     sk = PaymentSigningKey.generate()
     vk = PaymentVerificationKey.from_signing_key(sk)
     witness = VerificationKeyWitness(
         vkey=vk,
         signature=sk.sign(b"test"),
     )
 
-    with tempfile.NamedTemporaryFile(delete=False) as f:
-        tmp_path = f.name
-    try:
-        witness.save(tmp_path)
-        loaded_witness = VerificationKeyWitness.load(tmp_path)
-        assert witness == loaded_witness
+    path = str(tmp_path / "witness.json")
+    witness.save(path)
+    loaded_witness = VerificationKeyWitness.load(path)
+    assert witness == loaded_witness
 
-        assert witness != vk
-    finally:
-        os.unlink(tmp_path)
+    assert witness != vk
 
 
 def test_redeemer_decode():