Skip to content

JobRecord.drainCredentials read-then-clear is non-atomic; credentials added during shutdown escape revocation #47

Description

@nficano

The drainCredentials method in arcp-runtime/src/main/java/dev/arcp/runtime/session/JobRecord.java around line 191 performs List drained = new ArrayList<>(credentials); credentials.clear(); as two separate operations. CopyOnWriteArrayList makes each individual operation thread-safe but does not provide a way to read and clear atomically. The window between the snapshot and the clear is real: SessionLoop calls credentialBinding.revokeAll(record) from multiple paths — the agent worker's onSuccess, onFailure, onCancel handlers, the shutdown handler iterating ownedJobs, and the watchdog's terminateExpiredJob, in addition to user-driven rotateCredential. A rotation that lands between snapshot and clear is added to credentials, then immediately wiped out — never returned to drainCredentials, never revoked. Because revokeAll exists explicitly so the provisioner can release the underlying secret, a missed revocation means a credential continues to be valid on the upstream provider even though the job that owned it has ended. That is a real security regression.\n\nThe related replaceCredential method around line 177 has a parallel race: two concurrent replaceCredential calls for the same id can both find the existing entry, then both call set(i, next), producing a torn replacement where the loser's update is silently lost. revoke is then called for the wrong prior credential.\n\nFix prompt: In arcp-runtime/src/main/java/dev/arcp/runtime/session/JobRecord.java replace the CopyOnWriteArrayList<IssuedCredential> backing field for credentials with a final Object credentialsLock = new Object() plus a plain ArrayList\<IssuedCredential\> credentials, and synchronize every method that touches credentials (credentials(), setCredentials, replaceCredential, drainCredentials) on credentialsLock. drainCredentials should atomically copy and clear inside the synchronized block. replaceCredential should perform its scan, replace-or-append, and return the prior atomically. Because mutation throughput is low (credentials change only on attach, rotate, drain) the synchronized block is fine. Add a JUnit test in arcp-runtime/src/test/java/dev/arcp/runtime/credentials/ that interleaves rotateCredential and revokeAll across 1000 iterations on two threads and asserts the count of revoke calls observed by a recording CredentialProvisioner equals the count of credentials ever attached.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingseverity:mediumMedium severity

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions