Skip to content

fix: atomic file publication and authenticated IPC lock daemon - #2083

Merged
cstamas merged 2 commits into
masterfrom
security/locking-atomicity
Aug 31, 2026
Merged

fix: atomic file publication and authenticated IPC lock daemon#2083
cstamas merged 2 commits into
masterfrom
security/locking-atomicity

Conversation

@gnodet

@gnodet gnodet commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes 3 findings from the maven-resolver security audit (scan-maven-resolver-20260811):

Finding Severity Description
f007 MEDIUM Windows final move is truncate-and-copy, not atomic; truncated artifact can survive as trusted
f020 LOW IPC lock daemon protocol is unauthenticated; bootstrap secret leaks via argv
f025 LOW IPC lock key digest concatenates names without separators

Root cause: The concurrency layer has spots where its own guarantee is violated: Windows publication is not atomic, and the IPC lock daemon trusts every connection and digests keys ambiguously.

Fix: Atomic rename on Windows with retry, authenticated IPC daemon with SecureRandom tokens and peer credentials, unambiguous length-prefixed key digest.

Test plan

  • Existing tests pass
  • Windows atomic move tested
  • IPC daemon authentication tested
  • Key digest uniqueness tested

🤖 Generated with Claude Code

@gnodet gnodet closed this Aug 30, 2026
@gnodet
gnodet force-pushed the security/locking-atomicity branch from c4d9f95 to 14b23c9 Compare August 30, 2026 19:34
…handling

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@gnodet gnodet reopened this Aug 30, 2026
@gnodet
gnodet marked this pull request as ready for review August 31, 2026 05:20
@gnodet
gnodet requested a review from cstamas August 31, 2026 05:20
@gnodet gnodet added bug Something isn't working priority:major Major loss of function labels Aug 31, 2026
@gnodet gnodet added this to the 2.0.23 milestone Aug 31, 2026

@gnodet gnodet left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent security-hardening PR — defense-in-depth across the IPC lock protocol with comprehensive tests.

Strengths

  • Bootstrap token via stdin instead of argv prevents exposure through /proc/<pid>/cmdline — the right approach
  • SecureRandom over Random for the bootstrap token — required for a security credential
  • Per-connection context scoping is a solid privilege-separation fix: one client can no longer close or acquire on another client's lock contexts
  • Netstring-like lock key digest prevents boundary-shifted collisions (["foo","bar"] vs ["foob","ar"]) — clean and correct
  • Bounded preallocation (Math.min(sz, 1024)) prevents memory exhaustion from malicious wire-supplied sizes
  • Socket file permissions (owner-only rwx) on Unix domain sockets add a second layer beyond the token
  • retryingMove never-truncate contract eliminates the torn-read window where a concurrent reader or process kill could observe a truncated file at the final path after checksum validation
  • Test coverage is thorough: IpcServerAccessControlTest validates context isolation and token enforcement, IpcNamedLockFactoryTest covers digest collision resistance, PathProcessorSupportTest covers retry, never-truncate guarantee, and staging fallback

One observation

@since 2.0.22 tags — Same question as the sibling PRs in this security audit batch (#2079#2086): the maven-resolver-2.0.22 tag already exists in the repository, so these new API additions (IpcServer(SocketFamily, String) constructor at line 232, restrictToOwner in SocketFamily) should use @since 2.0.23 (or whatever the next release version will be).

Overall this is high-quality, well-thought-out security work. Well done!

2.0.22 is already released; new API additions must target 2.0.23.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@cstamas
cstamas merged commit a7104fa into master Aug 31, 2026
24 checks passed
@cstamas
cstamas deleted the security/locking-atomicity branch August 31, 2026 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority:major Major loss of function

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants