feat(network): persist NM secrets to D-Bus Secret Service keyring - #2970
Open
Mikilio wants to merge 3 commits into
Open
feat(network): persist NM secrets to D-Bus Secret Service keyring#2970Mikilio wants to merge 3 commits into
Mikilio wants to merge 3 commits into
Conversation
…ce keyring Store WiFi/802-1x/VPN/WireGuard secrets in the freedesktop Secret Service keyring when the user opts to save, and clean them up when connections are deleted. - Add keyring store/lookup/delete via CreateItem (replace=true), SearchItems, and Item.Delete on the default collection - ensureUnlocked() unlocks the default collection before any keyring op - Extract NM formatting (VPN secrets dict, 802-1x key filtering) into formatKeyringSecrets; secret_service.go stays connection-type agnostic - DeleteSecrets/DeleteSecrets2 now remove keyring items keyed by connection UUID - Support arbitrary NM setting types via hints; warn on unknown settings instead of blindly persisting reply.Secrets - Improve DeleteSecrets/DeleteSecrets2 tests to verify UUID extraction
Extract the password-flags decision chain out of GetSecrets into two helpers and group Phase 2 resolution by setting type instead of by operation: - readPasswordFlags maps setting name to the correct flag key (psk-flags/password-flags) and returns 0xFFFF when absent - handlePasswordFlags owns the whole flag decision chain, returning either resolved fields, an early response, or a *dbus.Error - Phase 2 is now a flat switch over settingName: vpn infers fields directly, wifi/802-1x delegate to the flag helpers Also fixes the broken VPN flow introduced by the earlier merge (VPN never set passwordFlags, so it always hit the 0xFFFF error path) and cleans up the stray indentation.
Add visibility into which NetworkManager connection types the SecretAgent does not handle explicitly, so missed types are easy to spot in logs. - GetSecrets response shaping: Warn when returning secrets as-is for an unhandled setting type - Backend caching, fieldsNeeded, and buildFieldsInfo: Debug when falling back to generic behavior for an unhandled setting type The keyring store path already warns; this extends the same idea to the remaining dispatch points.
Mikilio
marked this pull request as ready for review
July 31, 2026 14:11
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
PR #2402 introduced keyring retrieval, but I never added the logic to actually store secrets in the keyring or remove them when connections are deleted. So the retrieval path had nothing to retrieve. I only noticed after clearing my old keyring and realizing there was nothing left to retrieve. This PR closes that gap.
Changes
secret_service.go: addedstore()(CreateItem withreplace=true),deleteByUuid()(search by connection-uuid, then Item.Delete each match),ensureUnlocked()(reads theLockedproperty and unlocks the default collection if needed), andsearchItems()(which now correctly merges locked and unlocked results instead of discarding one set).trySecretServicereturn type changed fromnmSettingMaptomap[string]string, and the switch statement that restricted keyring lookup to specific setting types was removed.agent_networkmanager.go:formatKeyringSecretshelper to format raw secrets into NM response format (handles VPNsecretsdict, 802-1x key filtering).GetSecretsnow persists secrets to the keyring when the user opts to save (reply.Save). Explicit cases for802-11-wireless-security,802-1x, andvpn/wireguard. Unknown setting types log a Warn with key names (never values) rather than storing blindly.DeleteSecretsandDeleteSecrets2now remove keyring items keyed by connection UUID.DeleteSecrets2'sGetSettingsfailure log is bumped from silent to Warn.fieldsNeededdefault returnshintsinstead of[]string{}so every NM setting type can participate.readPasswordFlagsandhandlePasswordFlagshelpers, and field resolution now groups by setting type (vpn infers fields directly; wifi/802-1x delegate to the flag helpers). This also fixed a bug where VPN requests with empty hints always hit the missing-flags error path.TestDeleteSecretsByConn_EmptyConnUuid,TestDeleteSecrets_ExtractsConnUuid, andTestDeleteSecrets2_ExtractsSettingsto verify UUID extraction and the nil-conn guard.Current state
Working. Secrets are stored when the user saves, retrieved from the keyring on subsequent connections, and cleaned up when connections are deleted. The keyring unlock dialog continues to appear when needed.
Future work
This PR includes some minor refactoring alongside the new functionality. Given permission, I'd like to slowly improve the readability and maintainability of this code over time. The new dispatch-point logging should help surface which connection types future refactors need to account for.
As a side note, may I be tagged on PRs that touch these files just so I can stay informed?