configs: Improve security of in-repo configuration. #7761

matts1 · 2025-10-18T03:48:32Z

This completely changes the workflow for in-repo configuration. Instead of being stored in a file on disk, we store the configuration wrapped with a signature in order to determine who wrote the configuration.

In the event that the config is considered insecure, we warn the user and require them to run jj config edit --repo/workspace to re-enable it (test_config_security.rs simulates these workflows and is relatively well documented).

If applicable:

I have updated CHANGELOG.md
I have updated the documentation (README.md, docs/, demos/)
I have updated the config schema (cli/src/config-schema.json)
I have added/updated tests to cover my changes

matts1 · 2025-10-18T04:57:26Z

Notes for reviewers to make your life easier. I recommend reviewing in the following order:

Start with test_config_security.rs to see the UX
Then go to user_config.proto then user_config.rs to see the API
Then look at cli/config.rs to view the implementation

Everything else is just mostly boilerplate changes with little substance.

CHANGELOG.md

cli/src/config.rs

adamse · 2025-10-18T12:06:52Z

cli/src/config.rs

    /// Sets the directory for the workspace and the workspace-specific config
    /// file.
-    pub fn reset_workspace_path(&mut self, path: &Path) {
+    pub fn reset_workspace_path(&mut self, ui: &Ui, path: &Path) -> Result<(), CommandError> {


if this change doesn't land before workspace config is released there should be a migration like for repo config here.

I figured that even if it was released by then it'd be so recently released that my guess is that only you, as the PR author, would probably be the only person who has workspace config.

I can do this if need be, but I'm not convinced it's worth the effort.

I haven't read the code yet, but if workspace-config.toml complicates the stuff, I think it's okay to remove the workspace config layer at all. Maybe we can add --when.workspace_names = ["default"] if --when.workspaces = ["/path/to/workspace"] is tedious to write.

if it's an option, I'd prefer we temporarily disable workspace configs for the next release if this code hasn't landed by then, to avoid introducing more tech debt. I think it's probably ok to wait one extra release for the per-workspace configs.

I don't have issues with per-workspace configs, just don't want to have to go through the same migration process for them that we do for per-repo configs to migrate to this new format.

temporarily disabling or requiring manual migration seems fine to me.

If we agree that workspace config stands on its own[1], then I think we should leave workspace config as is and add migration code here if there is a release in between (it doesn't look too complex to me to duplicate or factor out and share).

[1] I and the other implementors (of workspace config) clearly do. To me it was the first thing I went looking for when starting to use jj (second I tried config conditionals (which also didn't exist for workspaces), but conditionals are not as "immediately obvious" to me).

I don't think anyone is saying that workspace config isn't useful. We can all agree that we want the feature enabled, I just think we should delay the release of the feature until this PR lands.

My main concern with leaving it in for the current release and adding migration code is that it adds another corner case that needs to be handled for a very long time, so unless we consider the feature urgent, I think it's worth delaying.

I personally consider that workspace config can be replaced with repo config + conditionals. I don't mean the workspace config is useless, but I don't think it would be worth the effort/complexity to add two separate config + signing file pairs. (It would be nice if we didn't have repo/workspace config files that could be a security risk, but removing all of them wouldn't be acceptable.)

If we're unsure whether we'll re-add workspace config, temporary removal would be good. If we're sure to support workspace config, I think manual migration (without temporary removal) is good enough.

Another concern is that the priority of repo-managed/repo/workspace configs is a bit weird. Since repo-managed config is actually selected per workspace (or working-copy commit), it could be considered a workspace-level config.

I personally consider that workspace config can be replaced with repo config + conditionals.

I don't think this is true for all the people who want to run their coding agents in separately configured workspaces. Since they want a one-time setup for the agents environment.

I don't mean the workspace config is useless, but I don't think it would be worth the effort/complexity to add two separate config + signing file pairs. (It would be nice if we didn't have repo/workspace config files that could be a security risk, but removing all of them wouldn't be acceptable.)

Agreed, having two signed pairs kinda complicates all of this.

If we're sure to support workspace config, I think manual migration (without temporary removal) is good enough.

I also agree with that. Although having an automated way would improve the user experience.

Another concern is that the priority of repo-managed/repo/workspace configs is a bit weird. Since repo-managed config is actually selected per workspace (or working-copy commit), it could be considered a workspace-level config.

Isn't this technically a bug? Because otherwise this seems to be a really bad oversight.

Another concern is that the priority of repo-managed/repo/workspace configs is a bit weird. Since repo-managed config is actually selected per workspace (or working-copy commit), it could be considered a workspace-level config.

Isn't this technically a bug? Because otherwise this seems to be a really bad oversight.

I mean it's not obvious whether the repo-managed config should be considered a repo-level or not. If it were "workspace-managed", user's .jj/repo/config.toml would be overridden by workspace-managed config. This wouldn't be what the user would want (unless they use workspace-config.toml extensively.)

adamse · 2025-10-19T08:41:57Z

CHANGELOG.md

  Updated tags will be exported to Git as lightweight tags.

 * New commit template keywords `local`/`remote_tags` to show only local/remote
  tags. These keywords may be useful in non-colocated Git repositories where


further up i mentioned the workspace-config.toml, should we update that too?

I'll leave this open, depends on whether we land this before a new release.

cli/src/config.rs

adamse · 2025-10-19T14:57:47Z

cli/src/config.rs

    /// Sets the directory for the workspace and the workspace-specific config
    /// file.
-    pub fn reset_workspace_path(&mut self, path: &Path) {
+    pub fn reset_workspace_path(&mut self, ui: &Ui, path: &Path) -> Result<(), CommandError> {


workspace config has the upside that you don't have to write absolute paths compared to --when.workspaces. to me having the workspace config seems like a useful feature, but i'm not too fussed as my usecases are also covered by when (but i'm not sure about the other people who were pushing for workspace config).

do we think it's likely this pr doesn't land soon? do we think workspace config is not a useful feature?

if workspace config is good and this pr doesn't land soon, we can mention it as experimental in the release notes and require manual migration once this pr lands?

PhilipMetzger

Here's a bunch of stuff and a opinion

PhilipMetzger · 2025-10-19T15:32:00Z

cli/src/config.rs

    /// Sets the directory for the workspace and the workspace-specific config
    /// file.
-    pub fn reset_workspace_path(&mut self, path: &Path) {
+    pub fn reset_workspace_path(&mut self, ui: &Ui, path: &Path) -> Result<(), CommandError> {


do we think workspace config is not a useful feature?

If it was requested by multiple users for multiple use-cases it should stand on its own as a useful feature (which it was). I mean @HybridEidolon's initial use-case was a good example of it uses. I also don't like disabling it when the migration provided here doesn't consider it yet.

do we think it's likely this pr doesn't land soon?

I think this needs some further discussion and I don't expect this go in faster than other stuff still pending.

cli/src/config.rs

cli/tests/common/test_environment.rs

cli/tests/test_config_security.rs

lib/src/user_config.rs

PhilipMetzger · 2025-10-19T15:46:09Z

lib/src/user_config.rs

+    if sign(&canonical) == *secure_config.signature {
+        Ok(UserConfig::Trusted(config))
+    } else if config.config().is_empty() {
+        // We don't trust the providence of the configuration, but it's empty so we'll
+        // just replace it with a similarly empty one.
+        Ok(UserConfig::Trusted(Default::default()))
+    } else if sign(&secure_config.path) == *secure_config.signature {


nit: it's probably simpler to compute the signed keys before doing the comparisons, it's also easier to extend. So move them up and only do the checks here.

I don't think it's simpler? And it's definitely more computationally expensive. Here you only need to sign the file once.

Could you show what you mean, what I'm thinking would just add a few lines of code.

I mean you can do this

//... same as now let canonical = ...; let signed_dir = sign(&canonical); let signed_config_path = sign(&secure_config.path); //... same as now

which only leaves the string comparisons in the current call site.

Fixes jj-vcs#3303 and jj-vcs#1595 This completely changes the workflow for in-repo configuration. Instead of being stored in a file on disk, we store the configuration wrapped with a signature in order to determine who wrote the configuration. In the event that the config is considered insecure, we warn the user and require them to run `jj config edit --repo/workspace` to re-enable it (test_config_security.rs simulates these workflows and is relatively well documented).

This seems like the only feasible option. On nix, apparently we don't have our own place to write to.

PhilipMetzger

Another round, sorry for not replying to your question yesterday.

PhilipMetzger · 2025-10-21T13:24:36Z

cli/src/config.rs

+        // If we did not provide this grace period, repo configs would be
+        // disabled for every repo created by an older version of jj.
+        // TODO: After the grace period is over (~jj 0.47), make this act the
+        // same as InvalidSignature or RepoMovedError .


nit: remove the whitespace before the period

PhilipMetzger · 2025-10-21T13:52:14Z

lib/src/user_config.rs

+    if sign(&canonical) == *secure_config.signature {
+        Ok(UserConfig::Trusted(config))
+    } else if config.config().is_empty() {
+        // We don't trust the providence of the configuration, but it's empty so we'll
+        // just replace it with a similarly empty one.
+        Ok(UserConfig::Trusted(Default::default()))
+    } else if sign(&secure_config.path) == *secure_config.signature {


I mean you can do this

//... same as now let canonical = ...; let signed_dir = sign(&canonical); let signed_config_path = sign(&secure_config.path); //... same as now

which only leaves the string comparisons in the current call site.

PhilipMetzger · 2025-10-21T14:22:14Z

cli/src/config.rs

    /// Sets the directory for the workspace and the workspace-specific config
    /// file.
-    pub fn reset_workspace_path(&mut self, path: &Path) {
+    pub fn reset_workspace_path(&mut self, ui: &Ui, path: &Path) -> Result<(), CommandError> {


I personally consider that workspace config can be replaced with repo config + conditionals.

I don't think this is true for all the people who want to run their coding agents in separately configured workspaces. Since they want a one-time setup for the agents environment.

I don't mean the workspace config is useless, but I don't think it would be worth the effort/complexity to add two separate config + signing file pairs. (It would be nice if we didn't have repo/workspace config files that could be a security risk, but removing all of them wouldn't be acceptable.)

Agreed, having two signed pairs kinda complicates all of this.

If we're sure to support workspace config, I think manual migration (without temporary removal) is good enough.

I also agree with that. Although having an automated way would improve the user experience.

Another concern is that the priority of repo-managed/repo/workspace configs is a bit weird. Since repo-managed config is actually selected per workspace (or working-copy commit), it could be considered a workspace-level config.

Isn't this technically a bug? Because otherwise this seems to be a really bad oversight.

PhilipMetzger · 2025-10-21T14:25:00Z

cli/src/commands/git/mod.rs

nit: squash this and the nix commit into the first one (although you may need a Nix expert to help you with the problems there before doing that).

yuja · 2025-10-22T09:38:51Z

cli/src/config.rs

+        let legacy_config = path.join("config.toml");
+        match std::fs::read_to_string(&legacy_config).context(&legacy_config) {
+            Ok(content) => {
+                self.set_repo_config(&content, true)?;
+                std::fs::remove_file(&legacy_config).context(&legacy_config)?;


Does this mean new config is no longer a plain text file? I thought there would be config.toml which is still readable, and a separate file containing signature.

Deleting config.toml would break forward compatibility, so it would require grace period. Even after the grace period, I prefer plain text format because the file can be inspected/edited by using ordinary tools. For example, we can use less .jj/repo/config.toml to review someone else's repository configuration without trusting it.

yuja · 2025-10-22T09:48:11Z

lib/src/user_config.rs

+pub trait ConfigType: prost::Message + Default + Clone + Debug {
+    /// The name of this type.
+    fn kind() -> &'static str;
+    /// The filename of the configuration file to write.
+    fn filename() -> &'static str;
+    /// The string containing the config.
+    fn config(&self) -> &str;
+}
+
+impl ConfigType for RepoConfig {
+    fn kind() -> &'static str {
+        "repo"
+    }
+
+    fn filename() -> &'static str {
+        "config.binpb"
+    }
+
+    fn config(&self) -> &str {
+        &self.config
+    }
+}


nit: Perhaps, this doesn't have to be abstracted as a trait. Maybe we can use parameter struct something like struct { kind: &'static str, filename: &'static str }?

yuja · 2025-10-22T09:59:33Z

lib/src/user_config.rs

+    let signing_key = key_dir.join("ed25519.der");
+    let verifying_key = key_dir.join("ed25519.der.pub");


I don't think we need public-key cryptography to sign and verify user's private data by his key. What we need is something like hash(private_key + path + config) but is considered secure. I think some MAC algorithm can be used. RustCrypto lists hmac crate in this category.

https://en.wikipedia.org/wiki/Message_authentication_code
https://github.com/RustCrypto

I'm not an expert, but pubkey-based algorithm would be computationally more expensive than symmetric-key algorithms or cryptographic hashing.

yuja · 2025-10-22T10:08:53Z

lib/src/user_config.rs

+    let strategy = match etcetera::choose_base_strategy() {
+        Ok(s) => s,
+        Err(_) => return Err(UserConfigError::HomeDirNotFound),
+    };
+    let key_dir = strategy.data_dir().join("jj/config/keys");


nit: Perhaps, path resolution (and/or key loading) should be done by non-jj-lib code, and provided as a function parameter? It will help write library tests.

matts1 force-pushed the push-slpsoluonwkm branch 5 times, most recently from 1ab950a to 2b1e682 Compare October 18, 2025 04:53

matts1 marked this pull request as ready for review October 18, 2025 04:54

matts1 requested a review from a team as a code owner October 18, 2025 04:54

matts1 force-pushed the push-slpsoluonwkm branch from 2b1e682 to c0bed88 Compare October 18, 2025 05:29

adamse reviewed Oct 18, 2025

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

adamse reviewed Oct 18, 2025

View reviewed changes

matts1 force-pushed the push-slpsoluonwkm branch from c0bed88 to ea2365d Compare October 19, 2025 03:26

matts1 requested a review from adamse October 19, 2025 03:26

matts1 force-pushed the push-slpsoluonwkm branch from ea2365d to dfe3864 Compare October 19, 2025 03:47

matts1 requested a review from yuja October 19, 2025 08:27

adamse reviewed Oct 19, 2025

View reviewed changes

PhilipMetzger reviewed Oct 19, 2025

View reviewed changes

matts1 added 4 commits October 20, 2025 03:09

test-env: Fix etcetera's data_dir method in our test environment.

35ea932

configs: Still use configs if we don't have a signature.

aea2f95

This seems like the only feasible option. On nix, apparently we don't have our own place to write to.

configs: Add a message when migrating from legacy repo configs.

cac1c1a

matts1 force-pushed the push-slpsoluonwkm branch from dfe3864 to cac1c1a Compare October 20, 2025 03:18

PhilipMetzger reviewed Oct 21, 2025

View reviewed changes

yuja reviewed Oct 22, 2025

View reviewed changes

		let signing_key = key_dir.join("ed25519.der");
		let verifying_key = key_dir.join("ed25519.der.pub");

configs: Improve security of in-repo configuration. #7761

Are you sure you want to change the base?

configs: Improve security of in-repo configuration. #7761

Uh oh!

Conversation

matts1 commented Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matts1 commented Oct 18, 2025

Uh oh!

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!

matts1 Oct 19, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PhilipMetzger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

PhilipMetzger left a comment

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!

Choose a reason for hiding this comment

Uh oh!

matts1 commented Oct 18, 2025 •

edited

Loading

matts1 Oct 19, 2025 •

edited

Loading