Skip to content

Conversation

@m-005
Copy link
Member

@m-005 m-005 commented Jan 21, 2026

Part 4 of the Foundation stage for objects-node daemon.

Changes

Implements state persistence with secure file I/O:

  • StateError enum with proper error handling (IO, Parse, Permission errors)
  • load_or_create() - Load from file or generate new keypair if missing
  • load() and save() - JSON file I/O with secure permissions
  • generate_new() - Cryptographically secure keypair generation using rand::rng()
  • ensure_permissions() - Parent directory creation and validation
  • Unix file permissions enforced (600) on state files

Security

  • State files automatically created with 600 permissions (owner read/write only)
  • Parent directories created recursively as needed
  • Validation prevents writing to invalid paths (e.g., when parent is a file)
  • Uses cryptographically secure random number generation

Testing

Comprehensive test coverage:

  • File creation and loading
  • Round-trip serialization with and without identity
  • Unix file permission verification (600)
  • Parent directory creation
  • Error cases (missing files, invalid paths)

Dependencies

  • Added rand to main dependencies for secure key generation

Part of the Foundation stage (4/4). Builds on: #51.

Foundation stage complete! Config and state management fully implemented with 22 passing tests.

m-005 added a commit that referenced this pull request Jan 21, 2026
Part 3 of the Foundation stage for objects-node daemon.

## Changes

Adds state type definitions for persistent node state:

- NodeState - Persistent state with node keypair and optional identity
  - Uses SecretKey from objects-transport (Iroh Ed25519 key)
  - Optional IdentityInfo for registered OBJECTS identities
- IdentityInfo - RFC-001 identity linking:
  - Identity ID (obj_ + base58 hash)
  - Registered handle
  - 8-byte nonce for ID derivation
  - Signer type (Passkey or Wallet)

## Security

Comprehensive documentation on security requirements:
- State files must use 600 permissions (owner read/write only)
- Node key must be kept secure and never committed to version control
- Supports anonymous mode (no identity) for nodes that don't publish assets

## Testing

- Serialization round-trip tests
- Identity persistence tests
- Both Passkey and Wallet signer types validated

Part of the Foundation stage (3/4). Builds on: #35. Next: State Persistence (#37).

Pull Request: #40
@m-005 m-005 force-pushed the spr/m-005/state-persistence-file-io-and-keypair-generation branch from 500bcd3 to 0891b8f Compare January 21, 2026 03:42
@m-005 m-005 changed the base branch from spr/m-005/main.state-persistence-file-io-and-keypair-generation to spr/m-005/state-types-nodestate-struct-definitions January 21, 2026 03:45
@OBJECTSHQ OBJECTSHQ deleted a comment from claude bot Jan 21, 2026
@OBJECTSHQ OBJECTSHQ deleted a comment from claude bot Jan 21, 2026
@OBJECTSHQ OBJECTSHQ deleted a comment from claude bot Jan 21, 2026
@OBJECTSHQ OBJECTSHQ deleted a comment from claude bot Jan 21, 2026
@OBJECTSHQ OBJECTSHQ deleted a comment from claude bot Jan 21, 2026
@m-005 m-005 force-pushed the spr/m-005/state-persistence-file-io-and-keypair-generation branch from 0891b8f to dd27ab9 Compare January 23, 2026 23:22
@m-005 m-005 force-pushed the spr/m-005/state-types-nodestate-struct-definitions branch from 2fcb887 to 4daee81 Compare January 23, 2026 23:22
@claude
Copy link

claude bot commented Jan 23, 2026

Code Review

Security Issue: TOCTOU Race Condition in File Permissions

Location: bins/objects-node/src/state.rs:100-114

Issue: The file containing the private key is written using std::fs::write() with default permissions (typically 0o644 based on umask), and only afterward are the permissions changed to 0o600. During this window between file creation and permission change, other users on the system could potentially read the private key.

According to Rust's documentation for OpenOptionsExt::mode(): "If no mode is set, the default of 0o666 will be used. The operating system masks out bits with the system's umask, to produce the final permissions." With a typical umask of 0o022, files are created with 0o644 permissions (world-readable).

Suggested fix:

use std::os::unix::fs::OpenOptionsExt;
use std::io::Write;

let contents = serde_json::to_string_pretty(self)?;
let mut file = std::fs::OpenOptions::new()
    .write(true)
    .create(true)
    .truncate(true)
    .mode(0o600)  // Set permissions atomically at creation
    .open(path)?;
file.write_all(contents.as_bytes())?;

This eliminates the race window entirely by setting permissions atomically when the file is created.

@m-005 m-005 force-pushed the spr/m-005/state-persistence-file-io-and-keypair-generation branch from dd27ab9 to ff4a6dc Compare January 24, 2026 00:19
@m-005 m-005 force-pushed the spr/m-005/state-types-nodestate-struct-definitions branch 2 times, most recently from 399db79 to 6650f30 Compare January 24, 2026 11:31
@m-005 m-005 force-pushed the spr/m-005/state-persistence-file-io-and-keypair-generation branch 2 times, most recently from cd3fc68 to 5f315cd Compare January 24, 2026 11:34
@m-005 m-005 force-pushed the spr/m-005/state-types-nodestate-struct-definitions branch from 6650f30 to 531e040 Compare January 24, 2026 11:34
@claude
Copy link

claude bot commented Jan 24, 2026

Code Review

Found 1 security issue that needs to be addressed.

See inline comment on bins/objects-node/src/state.rs for details about a race condition that exposes the private key during file creation.

@claude
Copy link

claude bot commented Jan 24, 2026

Test comment - checking inline comment capability

@claude
Copy link

claude bot commented Jan 24, 2026

Inline Comment for bins/objects-node/src/state.rs:104

Security Issue: Race condition exposes private key

The save() method creates the state file with world-readable permissions before restricting them to 0o600. This creates a race condition window where the node's private key can be read by other processes.

Problem:

See:

let contents = serde_json::to_string_pretty(self)?;
std::fs::write(path, contents)?;
// Set file permissions to 600 (owner read/write only)
#[cfg(unix)]
{
use std::os::unix::fs::PermissionsExt;
let metadata = std::fs::metadata(path)?;
let mut permissions = metadata.permissions();
permissions.set_mode(0o600);
std::fs::set_permissions(path, permissions)?;
}

std::fs::write() creates files using the process's umask, typically resulting in 0644 permissions (world-readable). Only afterwards are the permissions restricted to 0o600.

Fix:

Use std::fs::OpenOptions to atomically create the file with correct permissions. This is a larger structural change (6+ lines), so describing rather than providing a committable suggestion:

  1. Import std::fs::OpenOptions and std::os::unix::fs::OpenOptionsExt
  2. Replace std::fs::write() with OpenOptions::new().write(true).create(true).truncate(true).mode(0o600).open(path)?.write_all(contents.as_bytes())? on Unix
  3. Keep std::fs::write() for non-Unix platforms

This ensures the file is created with 0o600 permissions from the start, eliminating the race condition.

@claude
Copy link

claude bot commented Jan 24, 2026

Code Review Summary

Issues Found: 1

I reviewed this PR for bugs and CLAUDE.md compliance and found 1 high-severity security issue:

Security Issue - Race Condition in File Permissions

Location: bins/objects-node/src/state.rs:104

The save() method has a race condition that exposes the node's private key with world-readable permissions between file creation and permission restriction. See detailed comment above with fix recommendations.


CLAUDE.md Compliance: ✓ No violations found
Bug Scan: ⚠️ 1 security issue found (see above)

@m-005 m-005 force-pushed the spr/m-005/state-types-nodestate-struct-definitions branch from 531e040 to 2376222 Compare January 24, 2026 12:01
@m-005 m-005 force-pushed the spr/m-005/state-persistence-file-io-and-keypair-generation branch from 5f315cd to c2da686 Compare January 24, 2026 21:20
@m-005 m-005 force-pushed the spr/m-005/state-types-nodestate-struct-definitions branch 2 times, most recently from b5896a6 to 9857c26 Compare January 28, 2026 02:17
@m-005 m-005 changed the base branch from spr/m-005/state-types-nodestate-struct-definitions to main January 28, 2026 02:24
Part 4 of the Foundation stage for objects-node daemon.

## Changes

Implements state persistence with secure file I/O:

- StateError enum with proper error handling (IO, Parse, Permission errors)
- load_or_create() - Load from file or generate new keypair if missing
- load() and save() - JSON file I/O with secure permissions
- generate_new() - Cryptographically secure keypair generation using rand::rng()
- ensure_permissions() - Parent directory creation and validation
- set_secure_permissions() - Unix file permission enforcement (600) with verification

## Security

- State files created with 600 permissions (owner read/write only)
- Permissions verified after setting (prevents silent failures on NFS/FUSE)
- File deleted if permission verification fails (prevents key exposure)
- Non-Unix platforms: warning logged (users notified keys not protected)
- Parent directories created recursively as needed
- Validation prevents writing to invalid paths
- Uses cryptographically secure random number generation

## Encapsulation

- NodeState fields are private with accessor methods
- Prevents external code from bypassing validation
- IdentityInfo fields remain private with getters

## Observability

- Tracing instrumentation on load(), save(), load_or_create()
- Logging at key decision points (file creation, permission setting)

## Race Condition Fixes

- Use create_new() for atomic file creation in load_or_create()
- Prevents TOCTOU where multiple processes create state simultaneously

## Testing

Comprehensive test coverage (19 tests):
- File creation and loading
- Round-trip serialization with and without identity
- Unix file permission verification (600)
- Permission restoration (fixes wrong permissions on save)
- Parent directory creation
- Error cases: missing files, corrupted JSON, empty files, invalid paths
- Validates uses IdentityId and Handle types (not raw strings)

## Dependencies

- Added rand to main dependencies for secure key generation
- Removed duplicate rand from dev-dependencies
@m-005 m-005 force-pushed the spr/m-005/state-persistence-file-io-and-keypair-generation branch from c2da686 to 387f081 Compare January 28, 2026 03:23
- Implement atomic write pattern (temp-file-then-rename) in save()
- Fix silent file deletion failure with proper error logging
- Add race condition permission verification in load_or_create()
- Upgrade Windows security warning from WARN to ERROR level
- Add missing error context logging for load failures
- Add tests: idempotency, permissions on create, identity persistence
- Update documentation for permission timing and behavior
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants