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
↓ koh-a6303036.tar.gzatlas at this save
files changed5 files
VERSION+1 −1src/cli/cmd_remote.v+14 −2src/machine/settings.v+41 −3src/machine/settings_test.v+54 −0v.mod+1 −1src/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 }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 {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+}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 }