Skip to content

Add legacy support for KWallet maps - #294

Merged
frankosterfeld merged 2 commits into
frankosterfeld:mainfrom
notmart:work/mart/mapType
Mar 26, 2026
Merged

Add legacy support for KWallet maps#294
frankosterfeld merged 2 commits into
frankosterfeld:mainfrom
notmart:work/mart/mapType

Conversation

@notmart

@notmart notmart commented Mar 19, 2026

Copy link
Copy Markdown
Contributor

In the ongoing migration from KWallet to QtKeychain in Plasma, in order to migrate without too strong rewrites and most important have the existing stored data still working, (some secrets actually have the structure of multiple key value maps) make reading from map still work. return it as a json serialized string, which is also how the kwallet compatibility layer stores maps on secretservice.

This is only supported while reading, so when writing again it will be written directly as a serialized json string. this makes an implicit data migration which will make easier to go completely away from kwallet.

In the ongoing migration from KWallet to QtKeychain in Plasma,
in order to migrate without too strong rewrites and most important
have the existing stored data still working, (some secrets actually have
the structure of multiple key value maps) make reading from map
still work. return it as a json serialized string, which is also how
the kwallet compatibility layer stores maps on secretservice.

This is only supported while reading, so when writing again it
will be written directly as a serialized json string. this makes
an implicit data migration which will make easier to go completely
away from kwallet.
@mkurz mkurz mentioned this pull request Mar 19, 2026
@notmart

notmart commented Mar 19, 2026

Copy link
Copy Markdown
Contributor Author

An alternative might be actually supporting maps as a first class citizen (which they would still be stored just as json strings, so should work on any platform) just to have the serializing/unserializing behind a ReadMapJob/WriteMapJob

@michaelk83

michaelk83 commented Mar 19, 2026

Copy link
Copy Markdown

Given that maps are a legacy KWallet concept, and the plan for KWallet is to eventually retire its legacy API, I think the extra complexity of full maps support in QtKeychain is not worth the effort and maintenance cost. Secret Service has no concept of maps, and each client knows which of its stored values are JSON (or any other format). So I think the better approach is to migrate map entries to plain JSON with the generic type="plaintext".

But as I noted elsewhere, we do need the basic read support in this PR to get that migration done.

Comment thread qtkeychain/keychain_unix.cpp
Comment thread qtkeychain/keychain_unix.cpp
@notmart
notmart force-pushed the work/mart/mapType branch from 47788d3 to 39ff1c7 Compare March 24, 2026 10:00
@notmart

notmart commented Mar 24, 2026

Copy link
Copy Markdown
Contributor Author

Given that maps are a legacy KWallet concept, and the plan for KWallet is to eventually retire its legacy API, I think the extra complexity of full maps support in QtKeychain is not worth the effort and maintenance cost. Secret Service has no concept of maps, and each client knows which of its stored values are JSON (or any other format). So I think the better approach is to migrate map entries to plain JSON with the generic type="plaintext".

But as I noted elsewhere, we do need the basic read support in this PR to get that migration done.

As is now in this PR, it will successfully read from maps, but when writing again, it would then write a normal string, thus doing a kind of implicit migrations

@michaelk83

Copy link
Copy Markdown

LGTM

Comment thread qtkeychain/keychain_unix.cpp Outdated
@notmart

notmart commented Mar 25, 2026

Copy link
Copy Markdown
Contributor Author

anything left to do?

@notmart
notmart force-pushed the work/mart/mapType branch from 39ff1c7 to dd034b6 Compare March 26, 2026 09:04
@frankosterfeld
frankosterfeld merged commit 2598fe6 into frankosterfeld:main Mar 26, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants