security: handle transient files in certificate directory loading #157232
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The 'TestDemoLocality' was failing with "no certificates found; does certs dir exist?" errors. This resulted in connection failures when nodes attempted to establish RPC connections. Root cause: The demo cluster stores both TLS certificates and Unix socket files (e.g., .s.PGSQL.26267) in the same directory. When loading certificates, readDir() lists all directory entries and then calls entry.Info() to stat each file. Between these operations, transient socket lock files (e.g., .s.PGSQL.26267.lock.887590299) can be deleted, causing lstat() to fail with ENOENT. This caused the entire certificate loading to fail, even though the actual certificate files existed and were valid. Fix: this change modified the readDir() to skip files that disappear between directory listing and stat operations (a standard pattern for handling concurrent file-system modifications). Fixes cockroachdb#155255 Epic: none Release note: None
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Member
|
This change is |
Contributor
Author
|
For more context: it's a regression from #155196 Maybe we should put this pattern into a utility function and use it for other places where we list files (in above PR). |
rafiss
approved these changes
Nov 12, 2025
Collaborator
rafiss
left a comment
rafiss
left a comment
There was a problem hiding this comment.
great find! thanks for the fix
Collaborator
|
Thanks for chasing down the bug I left you, @shubhamdhama! |
Contributor
Author
|
TFTRs! bors r=rafiss,cthumuluru-crdb |
Contributor
shubhamdhama
added a commit
to shubhamdhama/cockroach
that referenced
this pull request
Nov 13, 2025
Multiple packages had duplicated the pattern of calling os.ReadDir followed by entry.Info() on each entry. In cockroachdb#157232, we fixed this logic for security where files may disappear between listing and stat operations. This fix can be extended to other places. For this reason we are moving this pattern to a shared utility. Fixes: none Epic: none Release note: none
shubhamdhama
added a commit
to shubhamdhama/cockroach
that referenced
this pull request
Nov 13, 2025
Multiple packages had duplicated the pattern of calling os.ReadDir followed by entry.Info() on each entry. In cockroachdb#157232, we fixed this logic for security where files may disappear between listing and stat operations. This fix can be extended to other places. For this reason we are moving this pattern to a shared utility. Fixes: none Epic: none Release note: none
celeste-cockroachdb
bot
added
v26.1.0-prerelease
and removed
target-release-26.1.0
labels
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The 'TestDemoLocality' was failing with "no certificates found; does certs dir exist?" errors. This resulted in connection failures when nodes attempted to establish RPC connections.
Root cause: The demo cluster stores both TLS certificates and Unix socket files (e.g.,
.s.PGSQL.26267) in the same directory. When loading certificates,readDir()lists all directory entries and then callsentry.Info()to stat each file. Between these operations, transient socket lock files (e.g.,.s.PGSQL.26267.lock.887590299) can be deleted, causinglstat()to fail with ENOENT. This caused the entire certificate loading to fail, even though the actual certificate files existed and were valid.Fix: this change modified the
readDir()to skip files that disappear between directory listing and stat operations (a standard pattern for handling concurrent file-system modifications).Fixes #155255
Epic: none
Release note: None