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

[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.

[KangLin]

@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: [user1@service.tld](mailto:user1@service.tld):port

    user: user1

    password: password1

    service: [user2@service.tld](mailto:user2@service.tld):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.

[akallabeth]

@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]

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

[KangLin]

* The `service` is a namespace

@akallabeth The service should comply with the URL scheme.

[KangLin]

@akallabeth I think you are complicating this a bit.

[akallabeth]

@KangLin please read:

qtkeychain/qtkeychain/keychain.h

Line 181 in 7668a63

* This job requires a "service" string, which is basically a namespace of keys within the keychain.

[KangLin]

@KangLin please read:

qtkeychain/qtkeychain/keychain.h

Line 181 in 7668a63

* This job requires a "service" string, which is basically a namespace of keys within the keychain.

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.

[akallabeth]

@KangLin please read:

qtkeychain/qtkeychain/keychain.h

Line 181 in 7668a63

* This job requires a "service" string, which is basically a namespace of keys within the keychain.

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 #276 (comment) ?

would gladly remove this thing if you know a way

[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.

[KangLin]

@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.

[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)

[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)

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

[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

[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?

[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

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

[akallabeth]

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

[KangLin]

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]

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.

[akallabeth]

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]

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

[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.
Ｍap username to the key.
only a simple correspondence is made, and the right to interpret is left to the user.

[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. Ｍap 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.

[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. Ｍap 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]

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

[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.

@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.

[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.

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

[KangLin]

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

[akallabeth]

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.

[TheOneRing]

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.

[akallabeth]

@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

[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

Yes, support for migration would even be better ❤️

[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.

So I suggest you add documentation explaining these differences.

[akallabeth]

@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

Yes, support for migration would even be better ❤️

ok, done.
I just instanciate a QSettings that should be in the application namespace (once for user, once for system wide settings)
should I cache this somehow as I have no idea if that could impact performance (although I doubt that it is that critical as credentials are usually not read/written that often)

[akallabeth]

@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

Yes, support for migration would even be better ❤️

migration is sadly out of the question, but now with the last 2 commits we have:

1. compile time option to always use the old behavior (default off)
2. A runtime application specific setting (user and system wide) to use legacy behavior for read/write/delete
3. A runtime application specific setting (user and system wide) to try to read passwords legacy style if EntryNotFound

[KangLin]

LGTM

[frankosterfeld]

Code LGTM, I need to review this in more detail and test on all the platforms, so I won't include it in the next release, to not delay it further, but plan to do one soon after.

[akallabeth]

@frankosterfeld pinging in case this got lost.
rebased to remove merge conflicts.