config: Implement secure-config design doc. #8222
Conversation
matts1
force-pushed
the
push-utlknmrzwpmm
branch
3 times, most recently
from
eb79eda to
721cf57
Compare
|
I've added Yuja as a reviewer since he's the one I strictly need approval from, but FYI @martinvonz and @PhilipMetzger in case you'd like to review as well. |
|
Some thoughts on the design doc:
|
matts1
force-pushed
the
push-utlknmrzwpmm
branch
from
721cf57 to
02513f6
Compare
This was a remnant of the old proposal. I've renamed it to "config / config signatures"
Not really
Yes, this is explained in the design doc in attack vector 1. Other attack vectors just build on top of this.
This feature is supposed to be transparent to the user. The user does not have to hand-correlate repos with config IDs. It should happen for them automatically. If you did want to manually do it, it's as simple as writing the config id to .jj/repo-config-id, or just running
If someone wanted to implemented it, I wouldn't stop them, but I already mentioned in the design doc that GC was out of scope of the initial version, but that we might consider later. |
PhilipMetzger
left a comment
PhilipMetzger
left a comment
There was a problem hiding this comment.
minor stuff, I think you also have missed implementing the deprecation warning.
| #[cfg(unix)] | ||
| { | ||
| // Make old versions and new versions of jj share the same config file. | ||
| std::fs::remove_file(&legacy_config).context(&legacy_config)?; | ||
| std::os::unix::fs::symlink(dunce::canonicalize(&config_file)?, &legacy_config) | ||
| .context(&legacy_config)?; | ||
| } | ||
| #[cfg(not(unix))] | ||
| { | ||
| // I considered making this readonly, but that would prevent you from | ||
| // updating the config with old versions of jj. | ||
| // In the future, we consider something a little more robust, where as | ||
| // the non-legacy config changes, we propagate that to the legacy config. | ||
| // However, it seems a little overkill, considering it only affects windows | ||
| // users who use multiple versions of jj at once, and only for a year. | ||
| let mut content = CONTENT_PREFIX.as_bytes().to_vec(); | ||
| content.extend_from_slice(&config); | ||
| std::fs::write(&legacy_config, content).context(&legacy_config)?; | ||
| } |
There was a problem hiding this comment.
nit: create a helper for this, this is also missing a deprecation warning with the year long migration timeline.
| content: &[u8], | ||
| metadata: &ConfigMetadata, | ||
| ) -> Result<PathBuf, SecureConfigError> { | ||
| let d = self.config_dir.join(config_id); |
There was a problem hiding this comment.
nit: d could be repo_dir since single char variables make for bad readabilty.
| // We don't use JJRng because that depends on the seed, which comes | ||
| // from config files, so doing so would be circular. | ||
| rng: RefCell<ChaCha20Rng>, | ||
|
|
There was a problem hiding this comment.
nit: delete spurious newline here
| std::fs::create_dir_all(&d).context(&d)?; | ||
| self.update_metadata(&d, metadata)?; | ||
| if !content.is_empty() { | ||
| std::fs::write(&config_path, content).context(&config_path)?; |
There was a problem hiding this comment.
nit: import atleast std so its just fs::write(...) since this code isn't async where it otherwise could lead to a problem.
| Err(e) if e.source.kind() == NotFound => { | ||
| let (path, metadata) = self.generate_initial_config(s)?; |
There was a problem hiding this comment.
I think the deprecation warning belongs here
3 participants
See docs/design/secure-config.md for many more details.
Fixes #3303
Fixes #1595
Checklist
If applicable:
CHANGELOG.mdREADME.md,docs/,demos/)cli/src/config-schema.json)