qtkeychain icon indicating copy to clipboard operation
qtkeychain copied to clipboard

windows: store key@service if a non empty key is given

Open akallabeth opened this issue 2 months ago • 35 comments

NOTE: Breaking change, the service was ignored up until now. so all existing applications that stored secrets with a previous version will not be able to find the stored credentials.

Just like #276 , but this actually resolves the issue. Before a unique key was required, now only the key@service needs to be unique

This is required because the windows API identifies a credential only by TargetName which is neither key nor service but a combination of both.

to keep a way of compatibility the build option USE_COMPAT_NAMING_SCHEME has been introduced.

@frankosterfeld don't know if required (windows applications will most likely bundle this themselves) but I could add a QSettings to check for that as well.

akallabeth avatar Oct 10 '25 01:10 akallabeth

@akallabeth Please refer to the following use cases:

User case 1: Single user
    service: service.tld
    user: user
    password: password

User case 2: Single user, More precise service
    service: service.tld:port
    user: user
    password: password

User case 3: Multi-user

    service: [[email protected]](mailto:[email protected]):port

    user: user1

    password: password1

    service: [[email protected]](mailto:[email protected]):port

    user: user2

    password: password2

I believe key@service should be explicitly determined by the user, rather than implicitly within the program. Otherwise, users may feel confused, leading to errors.

KangLin avatar Oct 10 '25 01:10 KangLin

@KangLin no, that contradicts the way qtkeychain works:

  • The service is a namespace
  • the key identifies a given <key, secret> pair

so, this needs to be done by qtkeychain rather than outside of it. but you've given me a hint, service might also be an empty string, need to take that into account.

akallabeth avatar Oct 10 '25 01:10 akallabeth

@KangLin also, [[email protected]](mailto:[email protected]):port is nothing that should be stored, it is just a way to display something. This is [email protected]:port (so the service part is just like your second example serivce.tld:port)

akallabeth avatar Oct 10 '25 01:10 akallabeth

* The `service` is a namespace

@akallabeth The service should comply with the URL scheme.

KangLin avatar Oct 10 '25 02:10 KangLin

@akallabeth I think you are complicating this a bit.

KangLin avatar Oct 10 '25 02:10 KangLin

@KangLin please read: https://github.com/frankosterfeld/qtkeychain/blob/7668a63a3669400223c5a563928ae042f499dbf4/qtkeychain/keychain.h#L181

akallabeth avatar Oct 10 '25 02:10 akallabeth

@KangLin please read:

https://github.com/frankosterfeld/qtkeychain/blob/7668a63a3669400223c5a563928ae042f499dbf4/qtkeychain/keychain.h#L181

OK. It does not conflict with following the URL scheme. So that It should be explicitly determined by the user, rather than implicitly in the program.

KangLin avatar Oct 10 '25 02:10 KangLin

@KangLin please read: https://github.com/frankosterfeld/qtkeychain/blob/7668a63a3669400223c5a563928ae042f499dbf4/qtkeychain/keychain.h#L181

OK. It does not conflict with following the URL scheme. So that It should be explicitly determined by the user, rather than implicitly in the program.

I don't get your point. can you answer https://github.com/frankosterfeld/qtkeychain/pull/276#issuecomment-3387878573 ?

would gladly remove this thing if you know a way

akallabeth avatar Oct 10 '25 02:10 akallabeth

@KangLin if you want to control the naming all the way you also do have to simply use an empty service or key value, then the whole thing will display just as you like it to have. it is just not mappable to the windows CredReadW and CredWriteW funktions the way you're expecting it.

akallabeth avatar Oct 10 '25 02:10 akallabeth

@akallabeth The TestAppExample is missing the service setting. So it caused your problem. The service can be configured by the user. Users can set the service according to the URL scheme.

KangLin avatar Oct 10 '25 03:10 KangLin

TestAppExample

  • wrong. It uses keychain.example.project.app
  • intended use is to have a unique namespace for your application to read/write/delete the secrets belonging to the application without affecting other applications
  • the mapping to the windows API is not a 1:1 mapping. It only knows TargetName as a unique identifier for an entry and does not have a lot of metadata.
  • there needs to be a mapping done so the application does not override secrets from other applications (namespace collisions)

akallabeth avatar Oct 10 '25 03:10 akallabeth

TestAppExample

* wrong. It uses `keychain.example.project.app`

* intended use is to have a unique `namespace` for your application to read/write/delete the secrets belonging to the application without affecting other applications

* the mapping to the windows `API` is not a 1:1 mapping. It only knows `TargetName` as a unique identifier for an entry and does not have a lot of metadata.

* there needs to be a mapping done so the application does not override secrets from other applications (namespace collisions)

Using a URL scheme for the service can avoid naming conflicts.

KangLin avatar Oct 10 '25 03:10 KangLin

@KangLin also, you can set anything as service and also as key. You need to create a unique identifier from that for your windows credential. you can not really assume anything. Used the @ just because the most common use case will be something like user@domain which looks at least familiar. can also be set to the sha256 hex string of service + key but would be a little hard to identify it from that

akallabeth avatar Oct 10 '25 03:10 akallabeth

Using a URL scheme for the service can avoid naming conflicts.

and your used type of a namespace is relevant here in which way?

akallabeth avatar Oct 10 '25 03:10 akallabeth

@KangLin also, you can set anything as service and also as key. You need to create a unique identifier from that for your windows credential. you can not really assume anything. Used the @ just because the most common use case will be something like user@domain which looks at least familiar. can also be set to the sha256 hex string of service + key but would be a little hard to identify it from that

Yes. So leave the right to set up the service to the users.

KangLin avatar Oct 10 '25 03:10 KangLin

Yes. So leave the right to set up the service to the users.

you can set your namespace however you want. like I already told you, just use an empty key and you're done.

not getting your point at all, the use case this is for is that the way on other platforms this works is:

  1. you set a namespace for your application / service / whatever
  2. you can read/write/delete entries in that namespace for different keys / users

but CredReadW does not allow to query a namespace so you need to create a key for your namespace

akallabeth avatar Oct 10 '25 03:10 akallabeth

you can set your namespace however you want. like I already told you, just use an empty key and you're done.

not getting your point at all, the use case this is for is that the way on other platforms this works is:

1. you set a `namespace` for your application / service / whatever

2. you can read/write/delete entries _in that namespace_ for different `keys / users`

but CredReadW does not allow to query a namespace so you need to create a key for your namespace

I understand what you mean. But it is different from Windows credentials.

KangLin avatar Oct 10 '25 03:10 KangLin

I think it makes more sense to understand this namespace as the namespace of a service, rather than the namespace of an application. Map TargetName to the service.

KangLin avatar Oct 10 '25 03:10 KangLin

I understand what you mean. But it is different from Windows credentials.

no. it is a normal credential without a username. (that is already encoded into the identifier) there are lots of these, your initial assumption just does not match reality.

the windows credential manager is just very limited in displaying stuff, so it is that UI that may be confusing, but that is outside of stuff we can change ;) It also contradicts the documentation of https://learn.microsoft.com/en-us/windows/win32/api/wincred/ns-wincred-credentialw because you can not expect CRED_TYPE_GENERIC to actually use the UserName field. It also does not display the Comment field.

akallabeth avatar Oct 10 '25 03:10 akallabeth

I think it makes more sense to understand this namespace as the namespace of a service, rather than the namespace of an application. Map TargetName to the service.

it is just as wrong as my (now ancient) commit that set it to key. It is both of them.

[edit] as for if it identifies an application or something else, that is up to the use case

akallabeth avatar Oct 10 '25 03:10 akallabeth

I think it makes more sense to understand this namespace as the namespace of a service, rather than the namespace of an application. Map TargetName to the service.

it is just as wrong as my (now ancient) commit that set it to key. It is both of them.

Map TargetName to the service. Map username to the key. only a simple correspondence is made, and the right to interpret is left to the user.

KangLin avatar Oct 10 '25 03:10 KangLin

I think it makes more sense to understand this namespace as the namespace of a service, rather than the namespace of an application. Map TargetName to the service.

it is just as wrong as my (now ancient) commit that set it to key. It is both of them.

Map TargetName to the service. Map username to the key. only a simple correspondence is made, and the right to interpret is left to the user.

then please answer my my question of how you want to read these credentials. this conversations stops now until this essential question has been answered.

akallabeth avatar Oct 10 '25 03:10 akallabeth

I think it makes more sense to understand this namespace as the namespace of a service, rather than the namespace of an application. Map TargetName to the service.

it is just as wrong as my (now ancient) commit that set it to key. It is both of them.

Map TargetName to the service. Map username to the key. only a simple correspondence is made, and the right to interpret is left to the user.

then please answer my my question of how you want to read these credentials. this conversations stops now until this essential question has been answered.

Distinguished by different service. This is also my way of solving this problem. Our difference lies in the fact that I want to leave it to the user to configure, while you want it to be set implicitly by the program.

KangLin avatar Oct 10 '25 03:10 KangLin

@akallabeth Please add key to username , Additionally, add annotations regarding this issue in the code. This makes it easier to understand.

KangLin avatar Oct 10 '25 04:10 KangLin

@KangLin ok, so your agument is: The representation by Windows Credentials is aesthetically displeasing for me, but I'd like to have the other inconsistent behavior?

please tell me I did not get that right.

@akallabeth Please add key to username , Additionally, add annotations regarding this issue in the code. This makes it easier to understand.

no. please do check the other implementations and the documentation of qtkeychain. you had a point in that my initial use of this was wrong, but your first try to resolve is just the other way to do it wrong.

akallabeth avatar Oct 10 '25 06:10 akallabeth

@KangLin ok, so your agument is: The representation by Windows Credentials is aesthetically displeasing for me, but I'd like to have the other inconsistent behavior?

please tell me I did not get that right.

Yes. Try to make it look the same as Windows credentials.

KangLin avatar Oct 10 '25 06:10 KangLin

The current implementation has no problems. It just needs a few comments added.

KangLin avatar Oct 10 '25 06:10 KangLin

The current implementation has no problems. It just needs a few comments added.

wrong. it stores all things in a global namespace and does not respect the way qtkeychain is supposed to work, breaking every multiplatform app that relies on this. you have your use case in mind and are blind for everything said so far, just because you find some display in windows credentials not aesthetically pleasing - which is actually a bug (or limitation) in that application

I'm all ears for syntax (windows often uses \ as a separator instead of @ for example) but your way of doing it in #276 introduces arbitrary limitations just because you do want a username in some optional field that can not be queried.

so, once again: qtkeychain stores a secret for some unique identifier comprised of service and key and does not have a username or something similar stored.

so, think of the TestAppExample: If you have 2 different applications, both with the same key values used they will override each others entries.

setting a unique service, as you suggested, is a way of hacking around that but not the way it is supposed to work.

akallabeth avatar Oct 10 '25 07:10 akallabeth

NOTE: Breaking change, the service was ignored up until now. so all existing applications that stored secrets with a previous version will not be able to find the stored credentials.

Just like #276 , but this actually resolves the issue. Before a unique key was required, now only the key@service needs to be unique

This is required because the windows API identifies a credential only by TargetName which is neither key nor service but a combination of both.

to keep a way of compatibility the build option USE_COMPAT_NAMING_SCHEME has been introduced.

@frankosterfeld don't know if required (windows applications will most likely bundle this themselves) but I could add a QSettings to check for that as well.

Please also add a runtime check, could we somehow "version" the behaviour? It might not be the last breaking change.

TheOneRing avatar Oct 10 '25 07:10 TheOneRing

@TheOneRing thinking about how to best do that. for reading credentials we could just do a fallback to a query for key if the key@service is not found, but I'd like to avoid that for write and delete as that could delete entries of applications other than the one issuing the command

akallabeth avatar Oct 10 '25 07:10 akallabeth