diff --git a/apps/host_agent/internal/identity/durability_test.go b/apps/host_agent/internal/identity/durability_test.go index e967ed75..56e30d07 100644 --- a/apps/host_agent/internal/identity/durability_test.go +++ b/apps/host_agent/internal/identity/durability_test.go @@ -276,7 +276,18 @@ func TestInitialPossessionDurabilityFailureNeverReportsSuccess(t *testing.T) { for _, stage := range []string{"create", "write", "file_sync", "file_close", "link", "parent_open", "parent_sync", "parent_close"} { t.Run(stage, func(t *testing.T) { dir := filepath.Join(physicalTemp(t), "identity") + real := identityFiles reset := failFiles(t, stage) + if faulty := identityFiles.openDir; strings.HasPrefix(stage, "parent_") { + // Aim the fault at the identity directory's own sync; the parent + // sync after Mkdir has its own tests in hardening_test.go. + identityFiles.openDir = func(p string) (durableFile, error) { + if p != dir { + return real.openDir(p) + } + return faulty(p) + } + } s, e := Prepare(dir, "https://localhost:8443", "") reset() if s != nil || !errors.Is(e, ErrIdentity) { diff --git a/apps/host_agent/internal/identity/durable_file.go b/apps/host_agent/internal/identity/durable_file.go index 3c7b6d80..8adb109b 100644 --- a/apps/host_agent/internal/identity/durable_file.go +++ b/apps/host_agent/internal/identity/durable_file.go @@ -18,6 +18,7 @@ type durableFiles struct { openDir func(string) (durableFile, error) rename func(string, string) error link func(string, string) error + remove func(string) error } var identityFiles = durableFiles{ @@ -25,4 +26,5 @@ var identityFiles = durableFiles{ openDir: func(dir string) (durableFile, error) { return os.Open(dir) }, rename: os.Rename, link: os.Link, + remove: os.Remove, } diff --git a/apps/host_agent/internal/identity/hardening_test.go b/apps/host_agent/internal/identity/hardening_test.go new file mode 100644 index 00000000..25dd7db1 --- /dev/null +++ b/apps/host_agent/internal/identity/hardening_test.go @@ -0,0 +1,173 @@ +// SPDX-License-Identifier: EUPL-1.2 +package identity + +import ( + "crypto/ed25519" + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "math/big" + "os" + "path/filepath" + "syscall" + "testing" + "time" +) + +func traceFiles(t *testing.T, log *[]string, failRemove bool) { + t.Helper() + before := identityFiles + ops := before + ops.openDir = func(d string) (durableFile, error) { + *log = append(*log, "opendir:"+d) + return before.openDir(d) + } + ops.link = func(a, b string) error { *log = append(*log, "link"); return before.link(a, b) } + ops.remove = func(p string) error { + *log = append(*log, "remove") + if failRemove { + return os.ErrPermission + } + return before.remove(p) + } + identityFiles = ops + t.Cleanup(func() { identityFiles = before }) +} + +func TestPrepareSyncsParentAfterMkdirAndRemovesTempBeforeDirectorySync(t *testing.T) { + root := physicalTemp(t) + dir := filepath.Join(root, "identity") + var log []string + traceFiles(t, &log, false) + if _, e := Prepare(dir, "https://localhost:8443", ""); e != nil { + t.Fatal(e) + } + want := []string{"opendir:" + root, "link", "remove", "opendir:" + dir} + if len(log) != len(want) { + t.Fatalf("order %v", log) + } + for i := range want { + if log[i] != want[i] { + t.Fatalf("order %v", log) + } + } + left, _ := filepath.Glob(filepath.Join(dir, ".identity-*")) + if len(left) != 0 { + t.Fatal("temp key copy remains") + } +} + +func TestPrepareRefusesWhenTempRemovalFails(t *testing.T) { + var log []string + traceFiles(t, &log, true) + if _, e := Prepare(filepath.Join(physicalTemp(t), "identity"), "https://localhost:8443", ""); e == nil { + t.Fatal("reported success without durable removal") + } +} + +func writeCA(t *testing.T, mode os.FileMode) string { + t.Helper() + pub, key, _ := ed25519.GenerateKey(rand.Reader) + tpl := &x509.Certificate{SerialNumber: big.NewInt(1), Subject: pkix.Name{CommonName: "ca"}, NotBefore: time.Now().Add(-time.Hour), NotAfter: time.Now().Add(time.Hour), IsCA: true, BasicConstraintsValid: true, KeyUsage: x509.KeyUsageCertSign} + der, e := x509.CreateCertificate(rand.Reader, tpl, tpl, pub, key) + if e != nil { + t.Fatal(e) + } + p := filepath.Join(physicalTemp(t), "ca.pem") + if e = os.WriteFile(p, pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der}), 0600); e != nil { + t.Fatal(e) + } + if e = os.Chmod(p, mode); e != nil { + t.Fatal(e) + } + return p +} + +func TestServerCAPathMustBeAbsoluteAndClean(t *testing.T) { + for _, p := range []string{"ca.pem", "./ca.pem", "/tmp/../tmp/ca.pem", "/tmp//ca.pem"} { + if _, e := Prepare(filepath.Join(physicalTemp(t), "identity"), "https://localhost:8443", p); e == nil { + t.Fatalf("Prepare accepted %q", p) + } + } + dir := filepath.Join(physicalTemp(t), "identity") + s, e := Prepare(dir, "https://localhost:8443", writeCA(t, 0600)) + if e != nil { + t.Fatal(e) + } + s.ServerCAFile = "relative.pem" + if validate(s) == nil || Save(dir, s) == nil { + t.Fatal("validate accepted relative CA path") + } +} + +func TestServerCAFileRefusesWritableAndSymlinkAndAcceptsPrivate(t *testing.T) { + for _, mode := range []os.FileMode{0620, 0602, 0666} { + s := &State{ServerCAFile: writeCA(t, mode)} + if _, e := TLSConfig(s, false); e == nil { + t.Fatalf("accepted CA mode %o", mode) + } + } + for _, mode := range []os.FileMode{0600, 0644} { + if _, e := TLSConfig(&State{ServerCAFile: writeCA(t, mode)}, false); e != nil { + t.Fatalf("refused CA mode %o", mode) + } + } + link := filepath.Join(physicalTemp(t), "link.pem") + if e := os.Symlink(writeCA(t, 0600), link); e != nil { + t.Fatal(e) + } + if _, e := TLSConfig(&State{ServerCAFile: link}, false); e == nil { + t.Fatal("accepted symlink CA") + } +} + +func TestPrepareRefusesWhenParentSyncAfterMkdirFails(t *testing.T) { + for _, stage := range []string{"parent_open", "parent_sync", "parent_close"} { + dir := filepath.Join(physicalTemp(t), "identity") + reset := failFiles(t, stage) + s, e := Prepare(dir, "https://localhost:8443", "") + reset() + if s != nil || e == nil { + t.Fatalf("%s: success without durable directory", stage) + } + if _, err := os.Lstat(filepath.Join(dir, "identity.json")); err == nil { + t.Fatalf("%s: identity written before directory was durable", stage) + } + } +} + +func TestPrepareRetrySyncsParentWhenDirectoryAlreadyExists(t *testing.T) { + dir := filepath.Join(physicalTemp(t), "identity") + reset := failFiles(t, "parent_sync") + _, e := Prepare(dir, "https://localhost:8443", "") + reset() + if e == nil { + t.Fatal("first attempt should fail") + } + var log []string + traceFiles(t, &log, false) + if _, e = Prepare(dir, "https://localhost:8443", ""); e != nil { + t.Fatal(e) + } + if len(log) == 0 || log[0] != "opendir:"+filepath.Dir(dir) { + t.Fatalf("retry skipped parent sync: %v", log) + } +} + +func TestServerCAFifoRefusedWithoutBlocking(t *testing.T) { + p := filepath.Join(physicalTemp(t), "ca.fifo") + if e := syscall.Mkfifo(p, 0600); e != nil { + t.Fatal(e) + } + done := make(chan error, 1) + go func() { _, e := TLSConfig(&State{ServerCAFile: p}, false); done <- e }() + select { + case e := <-done: + if e == nil { + t.Fatal("accepted FIFO CA") + } + case <-time.After(5 * time.Second): + t.Fatal("FIFO CA blocked TLS setup") + } +} diff --git a/apps/host_agent/internal/identity/identity.go b/apps/host_agent/internal/identity/identity.go index 564159de..583e2fdd 100644 --- a/apps/host_agent/internal/identity/identity.go +++ b/apps/host_agent/internal/identity/identity.go @@ -13,6 +13,7 @@ import ( "encoding/base64" "encoding/json" "errors" + "io" "math/big" "net/url" "os" @@ -74,9 +75,17 @@ func Prepare(dir, endpoint, serverCA string) (*State, error) { } else if !os.IsNotExist(e) { return nil, ErrIdentity } + if serverCA != "" && !caPathOK(serverCA) { + return nil, ErrIdentity + } if e := os.Mkdir(dir, 0700); e != nil && !os.IsExist(e) { return nil, ErrIdentity } + // Sync even when the directory already exists: a retry after a failed sync + // must not report success for an entry that may not be durable. + if syncDir(filepath.Dir(dir)) != nil { + return nil, ErrIdentity + } if e := privatePath(dir, true); e != nil { return nil, e } @@ -162,6 +171,16 @@ func save(dir string, state *State, exclusive bool) error { if e != nil { return ErrIdentity } + if exclusive { + // Drop the second name before the directory fsync so one sync makes the + // link and the removal durable; a crash cannot leave a second key copy. + if identityFiles.remove(temporary) != nil { + return ErrIdentity + } + } + return syncDir(dir) +} +func syncDir(dir string) error { parent, e := identityFiles.openDir(dir) if e != nil { return ErrIdentity @@ -173,6 +192,11 @@ func save(dir string, state *State, exclusive bool) error { } return nil } + +// caPathOK requires an absolute, already-clean server CA path. +func caPathOK(path string) bool { + return filepath.IsAbs(path) && filepath.Clean(path) == path +} func privatePath(path string, directory bool) error { if noSymlinkAncestors(path, false) != nil { return ErrIdentity @@ -280,6 +304,9 @@ func validate(s *State) error { if s == nil || s.SchemaVersion != 1 || !endpointOK(s.Endpoint) { return ErrIdentity } + if s.ServerCAFile != "" && !caPathOK(s.ServerCAFile) { + return ErrIdentity + } if s.Active == nil { if s.OwnerID != "" || s.HostID != "" || s.Pending == nil || s.Pending.PredecessorIdentityID != "" || s.Pending.Certificate != "" || s.Pending.IssuerCertificate != "" || s.Pending.IdentityID != "" { return ErrIdentity @@ -435,11 +462,7 @@ func TLSConfig(s *State, pending bool) (*tls.Config, error) { if noSymlinkAncestors(s.ServerCAFile, false) != nil { return nil, ErrIdentity } - info, e := os.Lstat(s.ServerCAFile) - if e != nil || !info.Mode().IsRegular() || info.Mode()&os.ModeSymlink != 0 || info.Size() > 32768 { - return nil, ErrIdentity - } - bytes, e := os.ReadFile(s.ServerCAFile) + bytes, e := readCA(s.ServerCAFile) if e != nil { return nil, ErrIdentity } @@ -548,3 +571,26 @@ func exactKeys(m map[string]any, keys []string) bool { } return true } + +// readCA opens the CA without following symlinks and checks the open file, so +// no stat-then-open race can swap it. Ancestor-directory ownership is not +// checked (known limitation, out of scope for #86). +func readCA(path string) ([]byte, error) { + f, e := os.OpenFile(path, os.O_RDONLY|syscall.O_NOFOLLOW|syscall.O_CLOEXEC|syscall.O_NONBLOCK, 0) + if e != nil { + return nil, ErrIdentity + } + defer f.Close() + info, e := f.Stat() + if e != nil || !info.Mode().IsRegular() || info.Size() > 32768 || info.Mode().Perm()&0022 != 0 { + return nil, ErrIdentity + } + if unix, ok := info.Sys().(*syscall.Stat_t); !ok || (unix.Uid != 0 && unix.Uid != uint32(os.Geteuid())) { + return nil, ErrIdentity + } + data, e := io.ReadAll(io.LimitReader(f, 32769)) + if e != nil || len(data) > 32768 { + return nil, ErrIdentity + } + return data, nil +} diff --git a/docs/development/ci-acceptance.json b/docs/development/ci-acceptance.json index c1997b4a..be16bdf7 100644 --- a/docs/development/ci-acceptance.json +++ b/docs/development/ci-acceptance.json @@ -572,6 +572,13 @@ "github.com/wstein/workharbor-nxt/apps/host_agent/internal/connection/TestAcknowledgementSequenceFailsClosed", "github.com/wstein/workharbor-nxt/apps/host_agent/internal/identity/TestAtomicIdentityPersistenceFailuresAndRestartTransitions", "github.com/wstein/workharbor-nxt/apps/host_agent/internal/identity/TestInitialPossessionDurabilityFailureNeverReportsSuccess", + "github.com/wstein/workharbor-nxt/apps/host_agent/internal/identity/TestPrepareSyncsParentAfterMkdirAndRemovesTempBeforeDirectorySync", + "github.com/wstein/workharbor-nxt/apps/host_agent/internal/identity/TestPrepareRefusesWhenTempRemovalFails", + "github.com/wstein/workharbor-nxt/apps/host_agent/internal/identity/TestServerCAPathMustBeAbsoluteAndClean", + "github.com/wstein/workharbor-nxt/apps/host_agent/internal/identity/TestServerCAFileRefusesWritableAndSymlinkAndAcceptsPrivate", + "github.com/wstein/workharbor-nxt/apps/host_agent/internal/identity/TestServerCAFifoRefusedWithoutBlocking", + "github.com/wstein/workharbor-nxt/apps/host_agent/internal/identity/TestPrepareRefusesWhenParentSyncAfterMkdirFails", + "github.com/wstein/workharbor-nxt/apps/host_agent/internal/identity/TestPrepareRetrySyncsParentWhenDirectoryAlreadyExists", "github.com/wstein/workharbor-nxt/apps/host_agent/cmd/workharbor-agent/TestPrivateDockerProxyModeRejectsMalformedLifetimeAndSocket", "github.com/wstein/workharbor-nxt/apps/host_agent/internal/config/TestDockerfilePlanBindsSelectedStageToExternalBase", "github.com/wstein/workharbor-nxt/apps/host_agent/internal/config/TestDockerfilePlanCanonicalizesMixedCaseStagesAndAliases",