Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 39 additions & 2 deletions store/keychain/keychain_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,14 @@ const (
// NOTE: do not use this directly, always call [getDefaultCollection]
loginKeychainObjectPath = dbus.ObjectPath("/org/freedesktop/secrets/collection/login")

// the null/root object path returned by the secret service when an alias is
// not assigned to any collection. It is syntactically valid (so
// [dbus.ObjectPath.IsValid] returns true) but does not point at a real
// collection, so it must be rejected explicitly.
//
// https://specifications.freedesktop.org/secret-service-spec/latest/org.freedesktop.Secret.Service.html#org.freedesktop.Secret.Service.ReadAlias
nullObjectPath = dbus.ObjectPath("/")

// used to list all available collections on the secret service API
//
// https://specifications.freedesktop.org/secret-service-spec/latest/org.freedesktop.Secret.Service.html
Expand All @@ -54,6 +62,12 @@ const (
secretServiceIsCollectionLockedProperty = "org.freedesktop.Secret.Collection.Locked"
)

// errNoDefaultCollection is returned when the secret service has no usable
// default collection (no 'login' collection and no collection assigned to the
// 'default' alias). This typically happens on headless hosts where the keyring
// has not been initialized.
var errNoDefaultCollection = errors.New("no default keychain collection available")
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] errNoDefaultCollection is unexported, preventing out-of-package error detection

The new errNoDefaultCollection sentinel is package-private. All public store methods (Get, Save, Delete, GetAllMetadata, Filter) propagate it directly to callers without wrapping. Any code outside the keychain package — such as an orchestration layer that wants to fall back gracefully on headless hosts — cannot use errors.Is to distinguish "no keychain infrastructure available" from other I/O errors. Since the PR's stated motivation is exactly headless Linux deployments where this condition is expected and routine, making this sentinel exported (or wrapping it in an exported type) would allow callers to handle it programmatically rather than resorting to fragile message-string comparisons.

Note: automated verifier was inconclusive; this finding is from static draft analysis.


// getDefaultCollection gets the secret service collection dbus object path.
//
// It prefers the loginKeychainObjectPath, since most users on X11 would have
Expand Down Expand Up @@ -84,11 +98,34 @@ func getDefaultCollection(service *kc.SecretService) (dbus.ObjectPath, error) {
return "", err
}

if !defaultKeychainObjectPath.IsValid() {
return resolveDefaultCollection(collections, defaultKeychainObjectPath)
}

// resolveDefaultCollection selects the collection to use given the available
// collections and the object path returned by the 'default' alias lookup.
//
// It is split out from [getDefaultCollection] so the selection logic can be
// unit tested without a live secret service over dbus.
func resolveDefaultCollection(collections []dbus.ObjectPath, aliasPath dbus.ObjectPath) (dbus.ObjectPath, error) {
// choose the 'login' collection if it exists
if slices.Contains(collections, loginKeychainObjectPath) {
return loginKeychainObjectPath, nil
}

if !aliasPath.IsValid() {
return "", errors.New("the default collection object path is invalid")
}

return defaultKeychainObjectPath, nil
// the secret service returns the null path '/' when no collection is
// assigned to the 'default' alias. This is common on headless hosts where
// neither the 'login' collection nor a 'default' alias has been set up.
// The null path is syntactically valid (so IsValid above returns true) but
// does not point at a real collection, so it must be rejected explicitly.
if aliasPath == nullObjectPath {
return "", errNoDefaultCollection
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] Delete returns errNoDefaultCollection on headless hosts instead of nil, breaking idempotent-delete semantics

On a headless host where no default collection exists, Delete (line 138) propagates errNoDefaultCollection for every call — even when the credential being deleted is guaranteed to be absent (there is no collection to store it in). The existing tests document that deleting an absent credential should return nil. Consider special-casing errNoDefaultCollection inside Delete (return nil) while still propagating it as an error from read and write-new-item operations (Get, Save, GetAllMetadata, Filter).

Note: automated verifier was inconclusive; this finding is from static draft analysis.

}

return aliasPath, nil
}

var errCollectionLocked = errors.New("collection is locked")
Expand Down
85 changes: 85 additions & 0 deletions store/keychain/keychain_linux_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
// Copyright 2026 Docker, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

//go:build linux

package keychain

import (
"testing"

dbus "github.com/godbus/dbus/v5"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestResolveDefaultCollection(t *testing.T) {
const customCollection = dbus.ObjectPath("/org/freedesktop/secrets/collection/custom")

tests := []struct {
name string
collections []dbus.ObjectPath
aliasPath dbus.ObjectPath
want dbus.ObjectPath
wantErr error
}{
{
name: "prefers login collection when present",
collections: []dbus.ObjectPath{customCollection, loginKeychainObjectPath},
// even if the alias points elsewhere, login wins
aliasPath: customCollection,
want: loginKeychainObjectPath,
},
{
name: "falls back to default alias when login is absent",
collections: []dbus.ObjectPath{customCollection},
aliasPath: customCollection,
want: customCollection,
},
{
name: "rejects null path from unassigned default alias",
// headless host: no login collection, no default alias set, so
// ReadAlias returns the null object path "/"
collections: []dbus.ObjectPath{},
aliasPath: nullObjectPath,
wantErr: errNoDefaultCollection,
},
{
name: "rejects syntactically invalid alias path",
collections: []dbus.ObjectPath{},
aliasPath: dbus.ObjectPath(""),
// distinct from the null-path case; see resolveDefaultCollection
want: "",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := resolveDefaultCollection(tt.collections, tt.aliasPath)

switch {
case tt.wantErr != nil:
assert.ErrorIs(t, err, tt.wantErr)
assert.Empty(t, got)
case tt.want == "":
// invalid path case: an error is expected, value is empty
require.Error(t, err)
assert.Empty(t, got)
default:
require.NoError(t, err)
assert.Equal(t, tt.want, got)
}
})
}
}
Loading