From dea67709981fd009a135244bd194cc9616c2442e Mon Sep 17 00:00:00 2001 From: mhoennig Date: Tue, 11 Aug 2026 07:09:34 +0200 Subject: [PATCH] Harden secret-file creation, token comparison and git refname args MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Works off the security audit in docs/prs/2026-07-08-PR#000: TODO 1, 8, 9 and 10, the four items that need no design decision. New `SecretFiles` creates files holding secrets with mode 0600 and their directories with 0700 *at creation*, as a file attribute, instead of writing at the umask default and chmod-ing afterwards — that left a window in which the Gitea token was world-readable, which matters on a multi-tenant host. It is used by `init` for .git/gittally/.gittally.yml and by `ControlTokenService` for the control token; the shell setup script now writes its YAML in a `umask 077` subshell for the same reason. `ControlTokenService.matches` hashes both sides with SHA-256 before `MessageDigest.isEqual`, so the comparison always runs over two 32-byte buffers and cannot return early on a length mismatch. `GitService.checkout` and `fetchBranch` pass `--` before the refname, so a branch named like an option cannot be read as one. `resetHardToOrigin` keeps its plain form: `git reset --hard -- ` is rejected outright and its argument is already `origin/`-prefixed. Co-Authored-By: Claude --- ...6-07-08-PR#000-security-audit-hardening.md | 11 +++-- .../kotlin/de/hoennig/gittally/SecretFiles.kt | 49 +++++++++++++++++++ .../hoennig/gittally/commands/InitCommand.kt | 9 ++-- .../de/hoennig/gittally/git/GitService.kt | 7 ++- .../gittally/server/ControlTokenService.kt | 25 +++++----- .../gittally/commands/InitCommandTest.kt | 15 ++++++ .../server/ControlTokenServiceTest.kt | 9 ++++ tools/setup-gittally-instance | 20 +++++--- 8 files changed, 115 insertions(+), 30 deletions(-) create mode 100644 src/main/kotlin/de/hoennig/gittally/SecretFiles.kt diff --git a/docs/prs/2026-07-08-PR#000-security-audit-hardening.md b/docs/prs/2026-07-08-PR#000-security-audit-hardening.md index 63ba8f2..093578e 100644 --- a/docs/prs/2026-07-08-PR#000-security-audit-hardening.md +++ b/docs/prs/2026-07-08-PR#000-security-audit-hardening.md @@ -42,7 +42,8 @@ Each item is a TODO with the background that justifies it. #### TODO 1 — Restrict the Gitea-token config file to the owner at creation -- [ ] In [`InitCommand.kt:86-106`](../../src/main/kotlin/de/hoennig/gittally/commands/InitCommand.kt), create `.git/gittally/.gittally.yml` and its parent `.git/gittally/` with `0600`/`0700`, atomically at creation (as [`GitAskPass`](../../src/main/kotlin/de/hoennig/gittally/git/GitAskPass.kt) does), not via plain `writeText` at the umask default. +- [x] In [`InitCommand.kt:86-106`](../../src/main/kotlin/de/hoennig/gittally/commands/InitCommand.kt), create `.git/gittally/.gittally.yml` and its parent `.git/gittally/` with `0600`/`0700`, atomically at creation (as [`GitAskPass`](../../src/main/kotlin/de/hoennig/gittally/git/GitAskPass.kt) does), not via plain `writeText` at the umask default. + **Done:** both `init` paths now go through [`SecretFiles`](../../src/main/kotlin/de/hoennig/gittally/SecretFiles.kt), which sets the mode as a file attribute at creation. **Background.** `init` creates the file it labels "secrets" — where the operator pastes the Gitea API token — with `writeText` and no permission restriction, so it inherits the umask (typically `0644`, world-readable). @@ -119,7 +120,7 @@ The deployment doc already recommends `127.0.0.1`; the default and template shou #### TODO 8 — Compare fixed-length hashes in the token check -- [ ] In [`ControlTokenService.kt:35-37`](../../src/main/kotlin/de/hoennig/gittally/server/ControlTokenService.kt), compare `SHA-256(submitted)` against `SHA-256(secret)` with `MessageDigest.isEqual`, so the comparison is always over equal-length buffers. +- [x] In [`ControlTokenService.kt:35-37`](../../src/main/kotlin/de/hoennig/gittally/server/ControlTokenService.kt), compare `SHA-256(submitted)` against `SHA-256(secret)` with `MessageDigest.isEqual`, so the comparison is always over equal-length buffers. **Background.** `MessageDigest.isEqual` returns early on a length mismatch, leaking the token length via timing. @@ -127,7 +128,8 @@ Largely theoretical given the 192-bit CSPRNG token, but a cheap deviation from c #### TODO 9 — Add `--` before positional refnames in git calls -- [ ] Insert `--` before the branch argument in `checkout`, `fetchBranch`, and `resetHardToOrigin` in [`GitService.kt:145,40,155`](../../src/main/kotlin/de/hoennig/gittally/git/GitService.kt) (e.g. `git switch -- `). +- [x] Insert `--` before the branch argument in `checkout`, `fetchBranch`, and `resetHardToOrigin` in [`GitService.kt:145,40,155`](../../src/main/kotlin/de/hoennig/gittally/git/GitService.kt) (e.g. `git switch -- `). + **Done for `checkout` and `fetchBranch`.** `resetHardToOrigin` keeps its plain form: `git reset --hard -- ` is rejected (`fatal: Cannot do hard reset with paths`), and its argument is already prefixed with `origin/`, so it can never start with `-`. **Background.** Git accepts refnames beginning with `-` (verified: `git check-ref-format 'refs/heads/-foo'` exits 0), so a pushed branch name could in principle be read as a git option. @@ -135,7 +137,8 @@ These three methods currently have no production callers and the actively-used p #### TODO 10 — Create secret files restricted atomically -- [ ] Set the mode at creation for the control-token file ([`ControlTokenService.kt:29-30`](../../src/main/kotlin/de/hoennig/gittally/server/ControlTokenService.kt)) and the setup-script YAML (`tools/setup-gittally-instance:253`), instead of `chmod 0600` after the write. +- [x] Set the mode at creation for the control-token file ([`ControlTokenService.kt:29-30`](../../src/main/kotlin/de/hoennig/gittally/server/ControlTokenService.kt)) and the setup-script YAML (`tools/setup-gittally-instance:253`), instead of `chmod 0600` after the write. + **Done:** the control-token file via [`SecretFiles`](../../src/main/kotlin/de/hoennig/gittally/SecretFiles.kt), the setup script by writing the YAML in a `umask 077` subshell instead of `chmod`-ing afterwards. **Background.** Both currently write the file at the umask default and tighten it afterward, leaving a brief window where the secret exists world-readable. diff --git a/src/main/kotlin/de/hoennig/gittally/SecretFiles.kt b/src/main/kotlin/de/hoennig/gittally/SecretFiles.kt new file mode 100644 index 0000000..ac684e4 --- /dev/null +++ b/src/main/kotlin/de/hoennig/gittally/SecretFiles.kt @@ -0,0 +1,49 @@ +package de.hoennig.gittally + +import java.nio.ByteBuffer +import java.nio.file.Files +import java.nio.file.Path +import java.nio.file.StandardOpenOption +import java.nio.file.attribute.PosixFilePermissions + +/** + * Creation of files and directories that hold secrets — the Gitea token in + * `.git/gittally/.gittally.yml` and the control token. + * + * The permissions are set *at creation*, never with a `chmod` after the write: + * writing at the umask default first (typically `0644`) would leave a window in + * which the secret is world-readable, which matters on multi-tenant hosts. + * On non-POSIX filesystems the permissions are silently skipped. + */ +object SecretFiles { + private val OWNER_ONLY_FILE = PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rw-------")) + private val OWNER_ONLY_DIRECTORY = PosixFilePermissions.asFileAttribute(PosixFilePermissions.fromString("rwx------")) + + /** Writes [content] as a `0600` file, replacing an existing file. */ + fun writeOwnerOnly( + file: Path, + content: String, + ) { + val bytes = content.toByteArray() + Files.deleteIfExists(file) + try { + Files + .newByteChannel(file, setOf(StandardOpenOption.CREATE_NEW, StandardOpenOption.WRITE), OWNER_ONLY_FILE) + .use { it.write(ByteBuffer.wrap(bytes)) } + } catch (_: UnsupportedOperationException) { + Files.write(file, bytes) + } + } + + /** Creates [directory] and its parents; a directory created here gets mode `0700`. */ + fun createDirectoriesOwnerOnly(directory: Path) { + val missing = generateSequence(directory) { it.parent }.takeWhile { !Files.exists(it) }.toList().asReversed() + missing.forEach { path -> + try { + Files.createDirectory(path, OWNER_ONLY_DIRECTORY) + } catch (_: UnsupportedOperationException) { + Files.createDirectory(path) + } + } + } +} diff --git a/src/main/kotlin/de/hoennig/gittally/commands/InitCommand.kt b/src/main/kotlin/de/hoennig/gittally/commands/InitCommand.kt index c99b256..37c9135 100644 --- a/src/main/kotlin/de/hoennig/gittally/commands/InitCommand.kt +++ b/src/main/kotlin/de/hoennig/gittally/commands/InitCommand.kt @@ -1,5 +1,6 @@ package de.hoennig.gittally.commands +import de.hoennig.gittally.SecretFiles import de.hoennig.gittally.git.GitService import org.springframework.stereotype.Component import picocli.CommandLine.Command @@ -93,7 +94,7 @@ class InitCommand( println("${file.toFile().relativeTo(normalizedWorkingDir.toFile())} already exists — not overwritten") return } - file.parent.toFile().mkdirs() + SecretFiles.createDirectoriesOwnerOnly(file.parent) val content = """ # Machine- or user-specific overrides and secrets. Keys here win over .gittally.yml. @@ -101,7 +102,9 @@ class InitCommand( account: "${detected.account}" # technical username for git HTTPS authentication token: "" # Gitea API token — never commit this """.trimIndent() - file.toFile().writeText(content + "\n") + // this is where the operator pastes the Gitea token, so it must never exist + // world-readable — on a shared host that would hand out git push access + SecretFiles.writeOwnerOnly(file, content + "\n") println("created ${file.toFile().relativeTo(normalizedWorkingDir.toFile())}") } @@ -214,7 +217,7 @@ class InitCommand( return } val gittallyDir = root.resolve(".git/gittally") - gittallyDir.toFile().mkdirs() + SecretFiles.createDirectoriesOwnerOnly(gittallyDir) val unitName = SystemdServiceFiles.unitName(root) val unitFile = gittallyDir.resolve(unitName) val envFile = gittallyDir.resolve(SystemdServiceFiles.ENV_FILE_NAME) diff --git a/src/main/kotlin/de/hoennig/gittally/git/GitService.kt b/src/main/kotlin/de/hoennig/gittally/git/GitService.kt index 5e0e358..5bb04ed 100644 --- a/src/main/kotlin/de/hoennig/gittally/git/GitService.kt +++ b/src/main/kotlin/de/hoennig/gittally/git/GitService.kt @@ -37,7 +37,8 @@ class GitService( workingDir: Path = Paths.get("."), ) { authenticated(workingDir) { environment -> - runner.runOrThrow(listOf("git", "fetch", "origin", branch), workingDir, environment) + // `--` guards against refnames starting with `-` being read as git options + runner.runOrThrow(listOf("git", "fetch", "origin", "--", branch), workingDir, environment) } } @@ -142,7 +143,7 @@ class GitService( workingDir: Path = Paths.get("."), ) { if (refExists("refs/heads/$branch", workingDir)) { - runner.runOrThrow(listOf("git", "switch", branch), workingDir) + runner.runOrThrow(listOf("git", "switch", "--", branch), workingDir) } else { runner.runOrThrow(listOf("git", "switch", "--track", "-c", branch, "refs/remotes/origin/$branch"), workingDir) } @@ -152,6 +153,8 @@ class GitService( branch: String, workingDir: Path = Paths.get("."), ) { + // no `--` here: with paths `git reset --hard` refuses to run; the `origin/` prefix + // already keeps the argument from looking like an option runner.runOrThrow(listOf("git", "reset", "--hard", "origin/$branch"), workingDir) } diff --git a/src/main/kotlin/de/hoennig/gittally/server/ControlTokenService.kt b/src/main/kotlin/de/hoennig/gittally/server/ControlTokenService.kt index 88e3e0f..c35d52d 100644 --- a/src/main/kotlin/de/hoennig/gittally/server/ControlTokenService.kt +++ b/src/main/kotlin/de/hoennig/gittally/server/ControlTokenService.kt @@ -1,8 +1,8 @@ package de.hoennig.gittally.server +import de.hoennig.gittally.SecretFiles import java.nio.file.Files import java.nio.file.Path -import java.nio.file.attribute.PosixFilePermissions import java.security.MessageDigest import java.security.SecureRandom @@ -25,16 +25,21 @@ class ControlTokenService( ?.let { return it } } val token = generateToken() - Files.createDirectories(tokenFile.parent) - Files.writeString(tokenFile, token + "\n") - restrictToOwner(tokenFile) + SecretFiles.createDirectoriesOwnerOnly(tokenFile.parent) + SecretFiles.writeOwnerOnly(tokenFile, token + "\n") return token } - /** Constant-time comparison; null or blank never matches. */ + /** + * Constant-time comparison; null or blank never matches. Both sides are hashed first + * so the comparison always runs over two 32-byte buffers and cannot return early on a + * length mismatch — which would leak the token length. + */ fun matches(submittedToken: String?): Boolean = !submittedToken.isNullOrBlank() && - MessageDigest.isEqual(submittedToken.toByteArray(), token().toByteArray()) + MessageDigest.isEqual(sha256(submittedToken), sha256(token())) + + private fun sha256(value: String): ByteArray = MessageDigest.getInstance("SHA-256").digest(value.toByteArray()) /** 24 random bytes as hex, like legacy `openssl rand -hex 24`. */ private fun generateToken(): String { @@ -42,12 +47,4 @@ class ControlTokenService( SecureRandom().nextBytes(bytes) return bytes.joinToString("") { "%02x".format(it) } } - - private fun restrictToOwner(file: Path) { - try { - Files.setPosixFilePermissions(file, PosixFilePermissions.fromString("rw-------")) - } catch (_: UnsupportedOperationException) { - // non-POSIX filesystem; the file stays with default permissions - } - } } diff --git a/src/test/kotlin/de/hoennig/gittally/commands/InitCommandTest.kt b/src/test/kotlin/de/hoennig/gittally/commands/InitCommandTest.kt index 293d54b..c23d219 100644 --- a/src/test/kotlin/de/hoennig/gittally/commands/InitCommandTest.kt +++ b/src/test/kotlin/de/hoennig/gittally/commands/InitCommandTest.kt @@ -9,6 +9,7 @@ import io.mockk.every import io.mockk.mockk import java.nio.file.Files import java.nio.file.Paths +import java.nio.file.attribute.PosixFilePermissions class InitCommandTest : FunSpec() { private val gitService = mockk() @@ -37,6 +38,20 @@ class InitCommandTest : FunSpec() { repoContent shouldContain "account: \"\"" // no user in https URL } + test("creates the secrets config and its directory readable only by the owner") { + val tempDir = Files.createTempDirectory("gittally-init-test") + initCommand.workingDir = tempDir + + every { gitService.getTopLevel(tempDir) } returns tempDir + every { gitService.getOriginUrl(tempDir) } returns "https://git.example.org/my-org/my-repo.git" + + initCommand.run() + + val repoConfig = tempDir.resolve(".git/gittally/.gittally.yml") + PosixFilePermissions.toString(Files.getPosixFilePermissions(repoConfig)) shouldBe "rw-------" + PosixFilePermissions.toString(Files.getPosixFilePermissions(repoConfig.parent)) shouldBe "rwx------" + } + test("detects account from https url") { val tempDir = Files.createTempDirectory("gittally-init-test") initCommand.workingDir = tempDir diff --git a/src/test/kotlin/de/hoennig/gittally/server/ControlTokenServiceTest.kt b/src/test/kotlin/de/hoennig/gittally/server/ControlTokenServiceTest.kt index 60a35b7..57cd546 100644 --- a/src/test/kotlin/de/hoennig/gittally/server/ControlTokenServiceTest.kt +++ b/src/test/kotlin/de/hoennig/gittally/server/ControlTokenServiceTest.kt @@ -5,6 +5,7 @@ import io.kotest.matchers.shouldBe import io.kotest.matchers.string.shouldMatch import java.nio.file.Files import java.nio.file.Path +import java.nio.file.attribute.PosixFilePermissions class ControlTokenServiceTest : FunSpec() { private fun newTokenFile(): Path = Files.createTempDirectory("gittally-token-test").resolve("control-token") @@ -21,6 +22,14 @@ class ControlTokenServiceTest : FunSpec() { service.token() shouldBe token } + test("persists the token readable only by the owner") { + val tokenFile = newTokenFile() + + ControlTokenService(tokenFile).token() + + PosixFilePermissions.toString(Files.getPosixFilePermissions(tokenFile)) shouldBe "rw-------" + } + test("reuses an operator-provided token file") { val tokenFile = newTokenFile() Files.createDirectories(tokenFile.parent) diff --git a/tools/setup-gittally-instance b/tools/setup-gittally-instance index 780a443..ec88abf 100755 --- a/tools/setup-gittally-instance +++ b/tools/setup-gittally-instance @@ -244,13 +244,19 @@ echo "== writing $project_yml (public host: $hostname, source: $config_source)" echo "== writing $machine_yml (secrets, mode 600)" mkdir -p "$(dirname "$machine_yml")" -{ - echo "# Machine-specific overrides and secrets. Keys here win over .gittally.yml." - echo "git:" - echo " account: $(yaml_quote "$git_account")" - echo " token: $(yaml_quote "$git_token")" -} >"$machine_yml" -chmod 600 "$machine_yml" +# the umask in the subshell makes the file mode 600 at creation, so the token is +# never world-readable — not even between the redirect and a follow-up chmod; +# an existing file is removed first, because the redirect would keep its mode +rm -f "$machine_yml" +( + umask 077 + { + echo "# Machine-specific overrides and secrets. Keys here win over .gittally.yml." + echo "git:" + echo " account: $(yaml_quote "$git_account")" + echo " token: $(yaml_quote "$git_token")" + } >"$machine_yml" +) echo "== converted project config:" sed 's/^/ /' "$project_yml"