config: Allow both user-managed and repo-managed configs per repo

[matts1]

Fixes #3684

This adds the following workflow to jj to support repo-level workflows:

1. Repo updates config.toml in the VCS
2. User retrieves changes
3. User runs jj command, gets a warning saying "please run jj config update-repo"
4. jj config update-repo adds a TUI requesting the user to approve the diff
5. The approved changes are copied to a permanent config file.

Checklist

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

[PhilipMetzger]

Please format the commits to adhere to our commit guidelines here: https://github.com/jj-vcs/jj/blob/main/docs/contributing.md#commit-guidelines and add a motivation to each commit. This is also missing tests and a changelog entry since the second commit even introduces a new command.

[matts1]

I know, that's why it's a draft commit. Just want to make sure the concept is approved before I add finishing touches.

[yuja]

cc @arxanas, @ilyagr

The idea sounds generally good to me. We might also want a similar mechanism for the zip file problem. Since the .jj directory may be untrusted, it might be better to store trusted config contents or hashes in e.g. ~/.local directory.

#1595

I have no idea about TUI. I personally would want to just see the diff and accept/reject.

[matts1]

The idea sounds generally good to me. We might also want a similar mechanism for the zip file problem.

I'm not familiar with "the zip file problem". Could you link to the issue?

Since the .jj directory may be untrusted, it might be better to store trusted config contents or hashes in e.g. ~/.local directory.

I'm not opposed to that. I chose this method because it was the same directory that the repo config was stored. I would request, however, that we submit this first and worry about that later, since this is explicitly not making the security any worse than it already is.

I have no idea about TUI. I personally would want to just see the diff and accept/reject.

I do agree, as a user, that that's what I would want, but that's also relatively easy to achieve with the TUI. I think, however, that from a security perspective, a binary accept / reject choice is not great, because it might feel like you have to make a choice between new features and security. With a binary choice, if there's one particular line that you are concerned about and so you reject it, that means you have to reject every future change to the repo config.

[martinvonz]

I'm not familiar with "the zip file problem". Could you link to the issue?

I don't think we have a link, but it's a familiar problem from other VCSs (familiar to VCS developers). The problem is what to do about repo-level configs in .jj/.git/.hg. For example, an attacker can send you a zip file of a repo where the repo-level config has rm -rf / as diff tool. They might ask you to help them troubleshoot some problem with their repo, for example, and you might not expect that simply running jj/git/hg diff in the repo would be harmful.

[matts1]

Makes sense. So I assume the solution would be to store it out of the repo, and require users to run jj config edit --repo instead of something like what I currently do where I type vim .jj/repo/config.toml?

[martinvonz]

I think Yuya's proposal was to record trusted config contents somewhere in ~/.local. When you unpack a zip file and run a command in that repo, you'd get a warning saying that it has untrusted configs. Then you run some command to say which of the configs you approve.

I'm not sure if it's safe to trust configs at the key-value level or only at coarser granularity. Could there be some case where you say that you trust key1="value1" and key2="value2" but somehow the combination is still harmful? I hope not, and it feels like it should be safe.

[matts1]

I'm not sure if it's safe to trust configs at the key-value level or only at coarser granularity. Could there be some case where you say that you trust key1="value1" and key2="value2" but somehow the combination is still harmful? I hope not, and it feels like it should be safe.

AFAICT, the attack surfaces are, in order of danger:

• Mess with the repo
• Send private code
• Remote code execution

This is my analysis of the danger:

• Revsets: Minor nuisances
  • Can mess with the repo
  • Can send code, but generally only to existing remotes
  • Can't escape the repo
• New remotes:
  • Should be obvious to spot, so easy to mitigate
  • RCE possible via pulling code
  • Possible to send private code by accident
• Arbitrary command execution. Only two sources of this are (to the best of my knowledge):
  • jj fix tools
  • Command aliases to jj util exec

I think that it's possible to make one harmful thing and one alias to it, but I don't think it's possible to make two things that are independently safe but together harmful (it's technically possible with two arbitrary commands that execute files in the repo, but if they're doing that then they can already achieve this).

[PhilipMetzger]

Here's a bunch of actual review comments.

[ilyagr]

I think this is probably OK. My worries are:

• Is this too inflexible to be useful? For example, as the user jumps from commit to commit, the repo-managed config may change, and I don't see a way to decide automatically whether they want their config to change accordingly (even if we trusted the config; see my hash idea in the other comment for how we could).

• How easy or hard will it be for the users to miss something important in the diff interface?

Figuring this out might require real-world testing. I'd probably merge some version of this, but disable it by default and call this feature "experimental" for quite a bit after. This is to give us time to polish it, but also to see whether it's worth the complexity/security tradeoff in practice.

Most of my other comments are various ideas about how to polish this further, which only make sense once we have a consensus that we're going forward with this (It's not clear to me how close we are at the moment).

[matts1]

• Is this too inflexible to be useful? For example, as the user jumps from commit to commit, the repo-managed config may change, and I don't see a way to decide automatically whether they want their config to change accordingly (even if we trusted the config; see my hash idea in the other comment for how we could).

My thoughts were, when designing this:

• The simplest approach is to simply not have the config to change at all
• The config will need to be approved, so it should not change accordingly IMO
• The only other option I can see is to have it, instead of reading the config from disk, automatically track trunk().
  • This presents potential problems in the sense that a change could be breaking (eg. jj fix referencing a file format.py that appeared in the same commit as the config change).
  • We could do something akin to dynamic approval where we have a map from hash of config file on disk to approved config. This seems way too complex for the initial solution though.
• I figured that it was unlikely someone would jump from commit to commit across different configs, and even if they did they'd only get a warning
  • Maybe this is wrong. It might just be because my workflow involves every commit being rebased on top of trunk() whenever I sync. Maybe people who use long-term feature branches that eventually get merged into main might not agree that it's unlikely?

• How easy or hard will it be for the users to miss something important in the diff interface?

I think that if the user cares about security, it's pretty hard to miss. It is also relatively easy to just blindly approve, though.

Figuring this out might require real-world testing. I'd probably merge some version of this, but disable it by default and call this feature "experimental" for quite a bit after. This is to give us time to polish it, but also to see whether it's worth the complexity/security tradeoff in practice.

I wouldn't be opposed to putting this behind a flag. Since this is inherently pre config-load, do you know if this would be able to be in our config schema, or would it have to be something like an environment variable flag?

[matts1]

I've added a config setting to toggle this feature, as requested by @ilyagr.

Does anyone have opinions on what the config file should be? My candidates were:

• .jjconfig.toml
• jj/config.toml
• .config/jj.toml
• .config/jj/config.toml

I ended up settling on the last one for a few reasons:

• I saw someone opining somewhere (can't find it now though) that workspace root tends to get cluttered already, so it was probably better to put it under a subdirectory.
  • My two cents is that for ownership purposes, it's nice to be a directory rather than a file.
• .jj is taken by the repo itself, and the file should be hidden, so jj/config.toml is ruled out.
• .config seems like a good place to put it
• .config/jj.toml doesn't give much flexibility if we need to add more jj-related files later (eg. .jjignore, .jjattributes)
• Adding the directory .config/jj allows users to store both the jj config and any commands they reference in the same directory
  • Eg. alias.sync = ["jj", "util", "exec", ".config/jj/sync.py"]
  • Then you can have .config/jj/sync.py in the same directory, which makes things much easier to review from a security standpoint.
  • This also allows .config/jj/OWNERS to exist

That being said, I'd welcome other opinions.

[PhilipMetzger]

@matts1 How do you imagine this working with multiple "correct" upstreams, like Eidolon's which inspired #3670? It isn't clear to me what should happen then, since there are multiple options which available beside erroring out.

[matts1]

@matts1 How do you imagine this working with multiple "correct" upstreams, like Eidolon's which inspired #3670? It isn't clear to me what should happen then, since there are multiple options which available beside erroring out.

I think in general, my answer is:

• They probably have the same config, in which case there's no problem
• If they don't have the same config, you probably spend a lot more time on one branch than the other. Just use the config from that branch.
• Otherwise, they can disable this feature if they like (and potentially manually specify the config file on the command-line)

There is a potential solution to this, but it's definitely not within the scope of the MVP. We can create a mapping from hash of checked-in config -> approved config. Then when you switch between revisions, it would just use the previously approved config instead of warning you that it's out of date.

[joyously]

with people adding a jj util exec alias which blasts everything away.

What if repo configs don't allow aliases?

[matts1]

with people adding a jj util exec alias which blasts everything away.

What if repo configs don't allow aliases?

That wouldn't really solve the problem. You could achieve the same just as easily with a malicious jj fix command, which definitely should be in the repo config, since each repo has their own rules for formatting.

Also, I think that would somewhat defeat the point of the repo config. In chromium, for example:

• The process to sync is:
  1. Run a git fetch
  2. jj rebase onto trunk()
  3. Run gclient sync, which updates the git submodules
• The process to upload (with git) is to run git cl upload, which:
  1. Runs a formatter
  2. Runs pre-upload checks
  3. Uploads to gerrit

While the former might be (somewhat) unique to chromium, I think that an alias jj pre-upload would be pretty reasonable and standard, and I've been running python scripts in lieu of hooks to run the equivalent of jj fix && jj pre-upload && jj git push.

So TLDR, I think they have too much value to disable. I think one approach that could be taken though is some stricter security measures. For example:

• We could execute jj fix in a read-only sandbox so it can't write (or delete) files
• We could, by default, make jj util exec run in a read-only sandbox using overlayfs or similar, with flags such as:
  • --no-sandbox: completely disable the sandbox
  • --allow-repo-writes: allows writes to all files in the repo excluding .jj
  • --allow-global-writes: allows writes to any file in the filesystem
• We could sandbox file reads to, by default, only allow:
  • entries in PATH
  • system directories
  • files in repo
  • $HOME/.*/** (eg. ~/.config/..., ~/.bazelrc

In general, sandboxing reads is a lot more painful, and I'm not sure it's a good idea, but sandboxing writes does sound like a very good idea.

[matts1]

I did a complete rework of it and fixed the zip file problem by storing it in the .local/state directory.

It now does the stuff that I mentioned where we would store hashes. This allows it to work very well in a multi-workspace repo, or when you're frequently changing between commits with different hashes.

[matts1]

I'm currently running into some issues on windows, but I don't have a windows machine to test on. I've just got a windows machine from work, but I've got to go through some red tape in order to install cargo on it.

I'd appreciate it if people would review it ignoring the error on windows, and I'll come back to it once the approval process is complete.

[yuja]

Thanks.

[matts1]

@yuja @martinvonz This should be ready for re-review

[mmhat]

I was hoping we could get this reviewed and submitted soon.

I think there are some open questions, but they're a non-issue for this particular PR IMO:

• How do you manage switching between configs

  • While it's somewhat important, it doesn't affect the MVP

• Can we avoid the zip file problem

  • It's definitely possible, but there's not a single solution. There's many different options we could take. I don't want to block this CL on a discussion on that, since this CL does not degrade security (it uses an insecure method, but it's the same method we already use for existing config files).

Well, here's another idea... (This is not intended as a change request; It's rather a rough idea I had while reading this thread here.)

When approving a repo-managed config:

1. Generate a cryptographic key in $XDG_CONFIG_HOME/jj/key (or $XDG_STATE_HOME/jj/key) unless there exists one.
2. Sign the repo-managed config with that key.
3. Store the signature somewhere in the .jj directory; Either in a log or somehow "attach" it to the revision that contains the approved repo-managed config.

Then, we can check for a certain revision if its config was approved by looking at the most recent signatures found in its ancestors, and if one of those is a valid certificate for the config we are fine.

It's just a rough idea, but that would allow us to keep evidence of the approval close to the repo-managed config without relying on some "global state" in ~/.local/....

[yuja]

Hmm, true. We can use cryptographic hashing/signing with per-user secret key. That makes the zip file problem a totally separate issue from the repo-managed settings. Thoughts?

I'll review this PR later in this week. I didn't have time last week, sorry.

[matts1]

That idea gave me a few other ideas. I didn't really like the idea of storing approvals inside the $XDG_STATE_HOME directory, for a few reasons:

• Approvals are based on the repo path, not the repo itself.
  • Moving the repo directory breaks it
  • Copying the repo directory breaks it
  • If you put a different repo inside the same directory, you'll have preapprovals
• Cleanup is difficult

So what if instead we kept both user and repo-user config inside the .jj directory, but added that cryptographic key you suggested to allow us to sign things. Then instead of ensuring that we don't load config from zip files, we create a signed data structure that cannot be tampered with.

We would then create the following data structure stored in the .jj directory:

struct Security {
  // This is randomly generated
  // This ensures that if you upload your own zip file, someone can't just send you a zip file with the same secure_repo_id
  salt: Vec<u8>,

  // This is sign(repo_path + salt)
  // We include the repo path to ensure that if you upload your own zip file, someone can't just send you a zip file with the same salt and signature.
  // Note that this specifically does not work if you uploaded a repo rooted at /path/to/repo, and then you download the repo to the same directory. However, this also wouldn't be solved by storing the config elsewhere, as then they could just use your preapproved configs.
  // If this does not match the signature we generate, we disable user configs.
  secure_repo_id: Signature,
  
  // Key: sign(secure_repo_id + repo config)
  // Value LHS: sign(secure_repo_id + repo_config + value RHS)
  // Value RHS: approved config for the given config
  // To look up an entry for a given config, we can resign it to recalculate the key.
  // We then need to also sign it including the value to ensure that the value was not tampered with.
  approved_repo_configs: map<Signature, (Vec<u8>, Signature)>
}

Note that the addition of this feature would mark existing user configs as insecure (as would any method that stops the zip file problem). We could work around this issue by automatically adding salts and marking repos as trusted for the first few months, then eventually stopping this practice.

[joyously]

Does that still allow for workspaces and local clones?
(sure seems more simple to just not read repo configs)

Does this impact CI testing of a repo with a repo config? (specifically the jj repo, if it had a repo config)

[matts1]

Does that still allow for workspaces and local clones?

Yes, that's unaffected

Does this impact CI testing of a repo with a repo config? (specifically the jj repo, if it had a repo config)

No. A ci bot would not be able to approve repo Configs, so would ignore them