Keyset locking by Philip-NLnetLabs · Pull Request #139 · NLnetLabs/dnst

Philip-NLnetLabs · 2025-11-19T10:53:01Z

Fixes #150.

Co-authored-by: Terts Diepraam <terts.diepraam@gmail.com>

- Adds KMIP server based key generation, signing and destruction, equivalent to the existing Ring/OpenSSL functionality. - Adds new kmip subcommands for managing KMIP server configurations. - Adds support for referring to KMIP keys by a new KMIP URL scheme. - Add a feature for the KMIP crypto backend just like the Ring and OpenSSL crypto backends. - Adds support for storing sensitive credentials in files separate to the KMIP server configuration.

…port to the proposed channel at packages.nlnetlabs.nl.

* Restructure roll commands. * Import public keys. * Import a public/private key pair from files. * Add a default TTL to config. Use that for DNSKEY/CDS/CDNSKEY/DS RRsets. * Cargo.lock. * Support for importing KMIP keys. * Import public/private keys in decoupled state * Add --private-key option to importing a public/private key pair from files. * Add remove-key command.

#138) * Introduce a 'WorkSpace' object to keep the current working state of keyset. * Fallout from merging. * Switch to the crypto-and-keyset-fixes branch in domain for the time being. Disable kmip because the branch does not support that. * Update lock file. * Bump Rust version to 1.85 because domain is at 1.85.0. * Switch to domain-kmip. (#142) * Switch to domain-kmip. * Use https URL for domain-kmip. * Update for domain-kmip change. * Switch to main branch of domain-kmip. * Clippy. * Bump Rust version to 1.88 for kmip-protocol.

src/commands/keyset/cmd.rs

ximon18 · 2026-03-02T09:08:38Z

Cargo.toml

 tracing-subscriber = "0.3.19"
 url = "2.5.4"
 futures = "0.3.31"
+fs2 = "0.4.3"


I'm wondering if this is a good choice of dependency. What were your reasons for selecting it?
I ask because:

Krill uses https://lib.rs/crates/fd-lock.

fs2 has a single maintainer.

That maintainer hasn't responded to issues in the GH repo since 2017 as far as I can see.

fs2 hasn't had a release in 8 years (though if it "just works" it may not need any).

https://stackoverflow.com/questions/27469928/is-there-file-locking-in-rust#comment132066314_45040455 says:

(a) Rust has https://doc.rust-lang.org/stable/std/fs/struct.File.html#method.lock and,

(b) that there is more maintained fork of fs2 at https://github.com/al8n/fs4-rs.

Conversely, I see that fd2 seems to be the most widely used file locking crate of some I locked at (fs2, fd-lock and file-lock).

I'll leave it at fs2 for now. If fs2 gets less popular or picks up security issues then we can switch. The interface of fd-lock looks scary.

ximon18 · 2026-03-02T09:19:11Z

src/commands/keyset/cmd.rs

+/// locking the file, the function has to check if the locked file is this
+/// the current fine under that name.


This comment has some typos and grammatical issues.

Updated to:

/// Assume changes are saved by creating a new file and renaming. After /// locking the file, the function has to check if the locked file is the /// same as the current file under that name.

ximon18 · 2026-03-02T09:19:49Z

src/commands/keyset/cmd.rs

+    // The config file is updated by writing to a new file and the renaming.
+    // Might have locked the old file. Check. Try a number of times and
+    // then give up. Lock contention is expected to be low.
+    for _try in 0..MAX_FILE_LOCK_TRIES {


Is it okay to retry lock acquisition with no delay between attempts?

This will lead to the thundering herd problem. But concurrent access to keyset for a single domain is expected to be very low. We need to do something if we ever fail to get a lock within MAX_FILE_LOCK_TRIES.

src/commands/keyset/cmd.rs

ximon18 · 2026-03-02T09:25:11Z

src/commands/keyset/cmd.rs

+        let file = File::open(filename)
+            .map_err(|e| format!("unable to open file {}: {e}", filename.display()))?;
+
+        file.lock_exclusive()


According to the documentation this call will block if the file is currently locked, is that desirable or would it be better to call try_lock_exclusive() instead?

For this PR the goal is to block. Maybe the UI needs to be improved in the future.

ximon18 · 2026-03-02T09:28:59Z

src/commands/keyset/cmd.rs

+                filename.display()
+            )
+        })?;
+        let current_file_handle = Handle::from_path(filename)


You open a file by path, lock it and get its handle, then get another handle for the same path and compare the two handles. Why? What is this handle comparison achieving/intended to do?

This is explained by the comment at the top of the function. Maybe it needs more text. The idea is that because the config is saved by creating a new file and then renaming that file, it is possible to endup with a lock on the old config file that no longer exists. So check that we have locked to current config file.

You only call write_locked_file() once, so any rename that happens within your code when the config file is saved won't be detected later by this logic, at least currently. So is this for the some future potential second call to this function after saving/renaming of the config file?

Also, when this test fails you call continue so you go round the loop again, dropping the file you just opened and the lock you just acquired, and try re-opening the file by the same filename as you just tried, even though that succeeded. I don't understand what this achieves.

If this function were called while a second concurrent invocation of dnst keyset was busy saving the config file, one would be able to open the file then block until the lock can be acquired, is this the issue, that you would then be possibly be opening the old FD? (though what happens to that FD during the rename/once the rename completes, is it still valid?)

After a rename, any processes that run keyset and that were trying to lock the config file now try to lock the wrong file. So they need to start again. The logic of keyset is that it gets a lock on the config file, and then saves the state file if it changed and saves the config file if it changed. There will not be a second call to lock the config file in a single run of keyset.

ximon18 · 2026-03-02T09:32:47Z

src/commands/keyset/cmd.rs

+    ///
+    /// First write to a new filename and then rename to make sure that
+    /// changes are atomic.
+    fn write_config(&self, keyset_conf: &PathBuf) -> Result<(), Error> {


This and fn write_state() look almost identical, I would suggest factoring their core logic out into a single helper function, especially as they both contain the same comment and work around for lacking -fn add_extension() support.

ximon18 · 2026-03-02T09:34:46Z

src/commands/keyset/cmd.rs

+
+        let mut file = File::create(&conf_file_new)
+            .map_err(|e| format!("unable to create file {}: {e}", conf_file_new.display()))?;
+        write!(file, "{json}")


If some reason creation, writing or renaming fail, the created .new file will be left behind, right? Is that a problem? Subsequent attempts would fail I assume as the created file already exists. Should you attempt to remove the file in such scenarios?

I don't expect create to fail if a file with the same name already exists.

ximon18 · 2026-03-02T09:36:41Z

src/commands/keyset/cmd.rs

+/// Assume changes are saved by creating a new file and renaming. After
+/// locking the file, the function has to check if the locked file is this
+/// the current fine under that name.
+fn write_locked_file(filename: &PathBuf) -> Result<File, Error> {


I find the name of this function confusing, as "write" reads like a verb yet this function does neither opens the file for writing nor writes to it.

Changed to file_with_write_lock

Co-authored-by: Ximon Eighteen <3304436+ximon18@users.noreply.github.com>

Philip-NLnetLabs and others added 30 commits April 17, 2025 13:37

Initial version of keyset.

f8c1453

Now with keyset.rs.

3621409

Fix Cargo.toml

88c45bd

The CDS/CDNSKEY RRsets also need to be signed. Cron command.

2ee5528

Display *-generate-params as *-algorithm in show.

54fd380

Avoid starting a KSK keyroll before init.

df5ef0e

Make sure key tags are unique.

97e218b

Support for algorithm rolls.

3583c32

Merge branch 'main' into keyset

19c2ff1

Fix up merge.

9328445

Remove unneeded format!.

5d946be

Merge branch 'main' into keyset

174f31e

Store key references are file URLs.

2129fc2

More detailed error messages.

688aada

More URL handling.

7977bf7

Add support for cron_next.

75ce06f

Use clap to parse keyset subcommands (#100)

559f00e

Co-authored-by: Terts Diepraam <terts.diepraam@gmail.com>

Merge branch 'main' into keyset

ba81d62

Add support for automatic key rolls (#108)

6fad119

Merge branch 'main' into keyset

df48644

Restore white space.

1f8f259

Getting ready to publish an 0.1.0-rc2 version of dnst with keyset sup…

5bf2f3f

…port to the proposed channel at packages.nlnetlabs.nl.

Add missed Cargo.lock change due to rc2 version bump.

64b9ce9

Mark keyset as experimental.

1d6b2f2

Extend O/S's supported for packaging.

6a1e4cb

Test missing/incorrect O/S variants.

f50b657

Permit KMIP servers to be added in an inactive state. (#122)

4f11b90

Delete errant character that broke the YML syntax.

e8dd8e2

Philip-NLnetLabs and others added 19 commits November 26, 2025 12:22

Merge branch 'keyset-improvements' into keyset-locking

535f572

Cargo fmt.

c84e21c

Switch to same-file.

c21a9db

Minor typo correction in man page sources.

0a13d74

Minor typo correction in man page sources.

48d1eb2

Fix RST syntax error in man page source.

4044fe5

Switch to main branch of domain-kmip.

ac75810

Clippy.

4f6f762

Bump Rust version to 1.88 for kmip-protocol.

7accc03

Merge branch 'main' into keyset

ceca925

Merge branch 'keyset' into keyset-improvements

7d37971

Undo Cascade-related changes.

ce572cd

Update manual pages.

d20b476

Merge branch 'main' into keyset

8bfef3b

Update man pages.

7337a3a

Merge branch 'main' into keyset

953cd96

Merge branch 'keyset' into keyset-improvements

212a8ff

Merge branch 'keyset-improvements' into keyset-locking

84cca6a

Philip-NLnetLabs changed the base branch from keyset-improvements to main January 23, 2026 08:54

ximon18 modified the milestone: next Feb 17, 2026

Merge branch 'main' into keyset-locking

9d072a5

ximon18 approved these changes Mar 2, 2026

View reviewed changes

Philip-NLnetLabs and others added 4 commits March 2, 2026 10:56

Update src/commands/keyset/cmd.rs

570220a

Co-authored-by: Ximon Eighteen <3304436+ximon18@users.noreply.github.com>

Review feedback.

9301b12

Extract common code in write_config and write_state.

10d2fa6

write_locked_file -> file_with_write_lock

5976442

Philip-NLnetLabs merged commit fdaa43a into main Mar 2, 2026
20 checks passed

Philip-NLnetLabs deleted the keyset-locking branch March 2, 2026 11:01

		/// locking the file, the function has to check if the locked file is this
		/// the current fine under that name.

Uh oh!

Conversation

Philip-NLnetLabs commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ximon18 Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Philip-NLnetLabs commented Nov 19, 2025 •

edited

Loading

ximon18 Mar 2, 2026 •

edited

Loading