Harden secret-file creation, token comparison and git refname args
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 -- <commit>` is rejected outright and its argument is already `origin/`-prefixed. Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
#### 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.**
|
**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).
|
`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
|
#### 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.**
|
**Background.**
|
||||||
`MessageDigest.isEqual` returns early on a length mismatch, leaking the token length via timing.
|
`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
|
#### 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 -- <branch>`).
|
- [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 -- <branch>`).
|
||||||
|
**Done for `checkout` and `fetchBranch`.** `resetHardToOrigin` keeps its plain form: `git reset --hard -- <commit>` is rejected (`fatal: Cannot do hard reset with paths`), and its argument is already prefixed with `origin/`, so it can never start with `-`.
|
||||||
|
|
||||||
**Background.**
|
**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.
|
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
|
#### 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.**
|
**Background.**
|
||||||
Both currently write the file at the umask default and tighten it afterward, leaving a brief window where the secret exists world-readable.
|
Both currently write the file at the umask default and tighten it afterward, leaving a brief window where the secret exists world-readable.
|
||||||
|
|||||||
@@ -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)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -1,5 +1,6 @@
|
|||||||
package de.hoennig.gittally.commands
|
package de.hoennig.gittally.commands
|
||||||
|
|
||||||
|
import de.hoennig.gittally.SecretFiles
|
||||||
import de.hoennig.gittally.git.GitService
|
import de.hoennig.gittally.git.GitService
|
||||||
import org.springframework.stereotype.Component
|
import org.springframework.stereotype.Component
|
||||||
import picocli.CommandLine.Command
|
import picocli.CommandLine.Command
|
||||||
@@ -93,7 +94,7 @@ class InitCommand(
|
|||||||
println("${file.toFile().relativeTo(normalizedWorkingDir.toFile())} already exists — not overwritten")
|
println("${file.toFile().relativeTo(normalizedWorkingDir.toFile())} already exists — not overwritten")
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
file.parent.toFile().mkdirs()
|
SecretFiles.createDirectoriesOwnerOnly(file.parent)
|
||||||
val content =
|
val content =
|
||||||
"""
|
"""
|
||||||
# Machine- or user-specific overrides and secrets. Keys here win over .gittally.yml.
|
# 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
|
account: "${detected.account}" # technical username for git HTTPS authentication
|
||||||
token: "" # Gitea API token — never commit this
|
token: "" # Gitea API token — never commit this
|
||||||
""".trimIndent()
|
""".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())}")
|
println("created ${file.toFile().relativeTo(normalizedWorkingDir.toFile())}")
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -214,7 +217,7 @@ class InitCommand(
|
|||||||
return
|
return
|
||||||
}
|
}
|
||||||
val gittallyDir = root.resolve(".git/gittally")
|
val gittallyDir = root.resolve(".git/gittally")
|
||||||
gittallyDir.toFile().mkdirs()
|
SecretFiles.createDirectoriesOwnerOnly(gittallyDir)
|
||||||
val unitName = SystemdServiceFiles.unitName(root)
|
val unitName = SystemdServiceFiles.unitName(root)
|
||||||
val unitFile = gittallyDir.resolve(unitName)
|
val unitFile = gittallyDir.resolve(unitName)
|
||||||
val envFile = gittallyDir.resolve(SystemdServiceFiles.ENV_FILE_NAME)
|
val envFile = gittallyDir.resolve(SystemdServiceFiles.ENV_FILE_NAME)
|
||||||
|
|||||||
@@ -37,7 +37,8 @@ class GitService(
|
|||||||
workingDir: Path = Paths.get("."),
|
workingDir: Path = Paths.get("."),
|
||||||
) {
|
) {
|
||||||
authenticated(workingDir) { environment ->
|
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("."),
|
workingDir: Path = Paths.get("."),
|
||||||
) {
|
) {
|
||||||
if (refExists("refs/heads/$branch", workingDir)) {
|
if (refExists("refs/heads/$branch", workingDir)) {
|
||||||
runner.runOrThrow(listOf("git", "switch", branch), workingDir)
|
runner.runOrThrow(listOf("git", "switch", "--", branch), workingDir)
|
||||||
} else {
|
} else {
|
||||||
runner.runOrThrow(listOf("git", "switch", "--track", "-c", branch, "refs/remotes/origin/$branch"), workingDir)
|
runner.runOrThrow(listOf("git", "switch", "--track", "-c", branch, "refs/remotes/origin/$branch"), workingDir)
|
||||||
}
|
}
|
||||||
@@ -152,6 +153,8 @@ class GitService(
|
|||||||
branch: String,
|
branch: String,
|
||||||
workingDir: Path = Paths.get("."),
|
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)
|
runner.runOrThrow(listOf("git", "reset", "--hard", "origin/$branch"), workingDir)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -1,8 +1,8 @@
|
|||||||
package de.hoennig.gittally.server
|
package de.hoennig.gittally.server
|
||||||
|
|
||||||
|
import de.hoennig.gittally.SecretFiles
|
||||||
import java.nio.file.Files
|
import java.nio.file.Files
|
||||||
import java.nio.file.Path
|
import java.nio.file.Path
|
||||||
import java.nio.file.attribute.PosixFilePermissions
|
|
||||||
import java.security.MessageDigest
|
import java.security.MessageDigest
|
||||||
import java.security.SecureRandom
|
import java.security.SecureRandom
|
||||||
|
|
||||||
@@ -25,16 +25,21 @@ class ControlTokenService(
|
|||||||
?.let { return it }
|
?.let { return it }
|
||||||
}
|
}
|
||||||
val token = generateToken()
|
val token = generateToken()
|
||||||
Files.createDirectories(tokenFile.parent)
|
SecretFiles.createDirectoriesOwnerOnly(tokenFile.parent)
|
||||||
Files.writeString(tokenFile, token + "\n")
|
SecretFiles.writeOwnerOnly(tokenFile, token + "\n")
|
||||||
restrictToOwner(tokenFile)
|
|
||||||
return token
|
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 =
|
fun matches(submittedToken: String?): Boolean =
|
||||||
!submittedToken.isNullOrBlank() &&
|
!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`. */
|
/** 24 random bytes as hex, like legacy `openssl rand -hex 24`. */
|
||||||
private fun generateToken(): String {
|
private fun generateToken(): String {
|
||||||
@@ -42,12 +47,4 @@ class ControlTokenService(
|
|||||||
SecureRandom().nextBytes(bytes)
|
SecureRandom().nextBytes(bytes)
|
||||||
return bytes.joinToString("") { "%02x".format(it) }
|
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
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -9,6 +9,7 @@ import io.mockk.every
|
|||||||
import io.mockk.mockk
|
import io.mockk.mockk
|
||||||
import java.nio.file.Files
|
import java.nio.file.Files
|
||||||
import java.nio.file.Paths
|
import java.nio.file.Paths
|
||||||
|
import java.nio.file.attribute.PosixFilePermissions
|
||||||
|
|
||||||
class InitCommandTest : FunSpec() {
|
class InitCommandTest : FunSpec() {
|
||||||
private val gitService = mockk<GitService>()
|
private val gitService = mockk<GitService>()
|
||||||
@@ -37,6 +38,20 @@ class InitCommandTest : FunSpec() {
|
|||||||
repoContent shouldContain "account: \"\"" // no user in https URL
|
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") {
|
test("detects account from https url") {
|
||||||
val tempDir = Files.createTempDirectory("gittally-init-test")
|
val tempDir = Files.createTempDirectory("gittally-init-test")
|
||||||
initCommand.workingDir = tempDir
|
initCommand.workingDir = tempDir
|
||||||
|
|||||||
@@ -5,6 +5,7 @@ import io.kotest.matchers.shouldBe
|
|||||||
import io.kotest.matchers.string.shouldMatch
|
import io.kotest.matchers.string.shouldMatch
|
||||||
import java.nio.file.Files
|
import java.nio.file.Files
|
||||||
import java.nio.file.Path
|
import java.nio.file.Path
|
||||||
|
import java.nio.file.attribute.PosixFilePermissions
|
||||||
|
|
||||||
class ControlTokenServiceTest : FunSpec() {
|
class ControlTokenServiceTest : FunSpec() {
|
||||||
private fun newTokenFile(): Path = Files.createTempDirectory("gittally-token-test").resolve("control-token")
|
private fun newTokenFile(): Path = Files.createTempDirectory("gittally-token-test").resolve("control-token")
|
||||||
@@ -21,6 +22,14 @@ class ControlTokenServiceTest : FunSpec() {
|
|||||||
service.token() shouldBe token
|
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") {
|
test("reuses an operator-provided token file") {
|
||||||
val tokenFile = newTokenFile()
|
val tokenFile = newTokenFile()
|
||||||
Files.createDirectories(tokenFile.parent)
|
Files.createDirectories(tokenFile.parent)
|
||||||
|
|||||||
@@ -244,13 +244,19 @@ echo "== writing $project_yml (public host: $hostname, source: $config_source)"
|
|||||||
|
|
||||||
echo "== writing $machine_yml (secrets, mode 600)"
|
echo "== writing $machine_yml (secrets, mode 600)"
|
||||||
mkdir -p "$(dirname "$machine_yml")"
|
mkdir -p "$(dirname "$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 "# Machine-specific overrides and secrets. Keys here win over .gittally.yml."
|
||||||
echo "git:"
|
echo "git:"
|
||||||
echo " account: $(yaml_quote "$git_account")"
|
echo " account: $(yaml_quote "$git_account")"
|
||||||
echo " token: $(yaml_quote "$git_token")"
|
echo " token: $(yaml_quote "$git_token")"
|
||||||
} >"$machine_yml"
|
} >"$machine_yml"
|
||||||
chmod 600 "$machine_yml"
|
)
|
||||||
|
|
||||||
echo "== converted project config:"
|
echo "== converted project config:"
|
||||||
sed 's/^/ /' "$project_yml"
|
sed 's/^/ /' "$project_yml"
|
||||||
|
|||||||
Reference in New Issue
Block a user