a63030364948

create the S3 credentials file private from the start and hide the typed secret (3.0.16, codex review pending)

time
2026-09-23 22:27 UTC · 2 weeks ago
machine
koh2
agent
claude-opus-5-5
session
koh 3 review fixes
parent
2c62be3a9131
5 files VERSION+1 −1src/cli/cmd_remote.v+14 −2src/machine/settings.v+41 −3src/machine/settings_test.v+54 −0v.mod+1 −1
VERSION+1 −1
@@ -1 +1 @@1-3.0.151+3.0.16

view file

src/cli/cmd_remote.v+14 −2
@@ -187,8 +187,20 @@187187 	}188188 	print('  access key: ')189189 	access_key_typed := os.get_line().trim_space()190-	print('  secret key: ')191-	secret_key_typed := os.get_line().trim_space()190+	// Hidden input needs a terminal on both ends; with output redirected, the191+	// key is read like any other line.192+	mut secret_key_typed := ''193+	if os.is_atty(1) != 0 {194+		secret_key_typed = os.input_password('  secret key: ') or {195+			// A terminal that cannot hide input still takes a plain line.196+			print('  secret key: ')197+			os.get_line()198+		}199+	} else {200+		print('  secret key: ')201+		secret_key_typed = os.get_line()202+	}203+	secret_key_typed = secret_key_typed.trim_space()192204 	if access_key_typed == '' || secret_key_typed == '' {193205 		return error('both keys are needed — nothing was saved')194206 	}

view file

src/machine/settings.v+41 −3
@@ -73,13 +73,51 @@7373 7474 // save_credentials writes the keys so that only the user can read them.7575 pub fn save_credentials(credentials Credentials) ! {76-	os.mkdir_all(config_dir(), os.MkdirParams{})!76+	os.mkdir_all(config_dir(), mode: 0o700)!77+	// mkdir_all leaves an existing directory's mode alone.78+	os.chmod(config_dir(), 0o700)!7779 	mut text := '# written by koh remote — keep this file private\n'7880 	text += 'access_key=${credentials.access_key}\n'7981 	text += 'secret_key=${credentials.secret_key}\n'8082 	path := config_path(credentials_file)81-	os.write_file(path, text)!82-	os.chmod(path, 0o600)!83+	// Created 0600 and renamed over the old file: the keys are never readable84+	// by others, not even between a write and a chmod, and a looser old file85+	// is replaced rather than rewritten in place.86+	temporary := '${path}.tmp-${os.getpid()}'87+	// A stale file (or link) by that name would keep its own mode, so it must88+	// go; if it cannot, nothing is written.89+	if os.exists(temporary) || os.is_link(temporary) {90+		os.rm(temporary)!91+	}92+	mut file := os.open_file(temporary, 'w', 0o600) or {93+		os.rm(temporary) or {}94+		return err95+	}96+	os.chmod(temporary, 0o600) or {97+		file.close()98+		os.rm(temporary) or {}99+		return err100+	}101+	file.write_string(text) or {102+		file.close()103+		os.rm(temporary) or {}104+		return err105+	}106+	file.close()107+	// close() does not report a failed flush, so check what actually landed.108+	if (os.read_file(temporary) or { '' }) != text {109+		os.rm(temporary) or {}110+		return error('could not write ${path}')111+	}112+	// V's rename, like mv(1), would move the file INTO a directory there.113+	if os.is_dir(path) {114+		os.rm(temporary) or {}115+		return error('${path} is a directory')116+	}117+	os.rename(temporary, path) or {118+		os.rm(temporary) or {}119+		return err120+	}83121 }84122 85123 pub fn load_credentials() !Credentials {

view file

src/machine/settings_test.v+54 −0
@@ -40,3 +40,57 @@4040 	assert config_path(remote_file).ends_with('.config/koh/remote')4141 	assert config_path(credentials_file).ends_with('.config/koh/credentials')4242 }43+44+// The keys file must never be readable by others, not even for the moment45+// between writing it and tightening it, and an old loose file is tightened.46+fn test_credentials_are_private_from_the_start() {47+	home := with_temp_config('credentials')48+	defer {49+		os.rmdir_all(home) or {}50+	}51+	old_home := os.getenv('HOME')52+	os.setenv('HOME', home, true)53+	defer {54+		os.setenv('HOME', old_home, true)55+	}56+	os.mkdir_all(config_dir(), mode: 0o755)!57+	os.chmod(config_dir(), 0o755)!58+	os.write_file(config_path(credentials_file), 'access_key=old\n')!59+	os.chmod(config_path(credentials_file), 0o644)!60+	// A stale temp name from an earlier run, as a link: writing through it61+	// would put the keys wherever it points.62+	stale := '${config_path(credentials_file)}.tmp-${os.getpid()}'63+	os.symlink(os.join_path(home, 'elsewhere'), stale)!64+	save_credentials(Credentials{65+		access_key: 'AK'66+		secret_key: 'SK'67+	})!68+	assert os.stat(config_path(credentials_file))!.get_mode().bitmask() & 0o777 == 0o60069+	assert os.stat(config_dir())!.get_mode().bitmask() & 0o777 == 0o70070+	assert load_credentials()!.secret_key == 'SK'71+	assert os.ls(config_dir())! == [credentials_file]72+	assert !os.exists(os.join_path(home, 'elsewhere'))73+}74+75+// A save that fails part way must not leave the keys lying in a temp file.76+fn test_a_failed_credentials_save_leaves_no_temp_file() {77+	home := with_temp_config('credentials_fail')78+	defer {79+		os.rmdir_all(home) or {}80+	}81+	old_home := os.getenv('HOME')82+	os.setenv('HOME', home, true)83+	defer {84+		os.setenv('HOME', old_home, true)85+	}86+	// A directory where the file should be makes the final rename fail.87+	os.mkdir_all(os.join_path(config_path(credentials_file), 'occupied'))!88+	if _ := save_credentials(Credentials{89+		access_key: 'AK'90+		secret_key: 'SK'91+	})92+	{93+		assert false, 'renaming over a directory should have failed'94+	}95+	assert os.ls(config_dir())! == [credentials_file]96+}

view file

v.mod+1 −1
@@ -1,6 +1,6 @@11 Module {22 	name: 'koh3'3-	version: '3.0.15'3+	version: '3.0.16'44 	description: 'One SQLite file per repo, one line of history.'55 	license: 'MIT'66 }

view file