-
Notifications
You must be signed in to change notification settings - Fork 6
fix(keychain): reject null collection path on linux #538
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -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") | ||
|
|
||
| // getDefaultCollection gets the secret service collection dbus object path. | ||
| // | ||
| // It prefers the loginKeychainObjectPath, since most users on X11 would have | ||
|
|
@@ -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 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, Note: automated verifier was inconclusive; this finding is from static draft analysis. |
||
| } | ||
|
|
||
| return aliasPath, nil | ||
| } | ||
|
|
||
| var errCollectionLocked = errors.New("collection is locked") | ||
|
|
||
| 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) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
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
errNoDefaultCollectionsentinel is package-private. All public store methods (Get,Save,Delete,GetAllMetadata,Filter) propagate it directly to callers without wrapping. Any code outside thekeychainpackage — such as an orchestration layer that wants to fall back gracefully on headless hosts — cannot useerrors.Isto 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.