Skip to content
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

Credential store keys are not namespaced #6125

Closed
ckamm opened this issue Oct 26, 2017 · 1 comment
Closed

Credential store keys are not namespaced #6125

ckamm opened this issue Oct 26, 2017 · 1 comment
Assignees
Labels
ReadyToTest QA, please validate the fix/enhancement Windows
Milestone

Comments

@ckamm
Copy link
Contributor

ckamm commented Oct 26, 2017

@guruz pointed out frankosterfeld/qtkeychain#105 and I've verified it on Windows 10:

When we store credentials in the credential store on Windows through QtKeychain we choose the key similarly to other platforms. However, on Windows the library doesn't automatically add the application name or organization name.

Effects

  • Keys like "user:http://localhost:0" show up in "Generic Credenticals" section of the Windows Credential Manager
  • Different branded clients may share credentials if the same user/server is used. (should be exceedingly rare)

MSDN says the following about the key:

If the Type is CRED_TYPE_GENERIC, this member should identify the service that uses the credential in addition to the actual target. Microsoft suggests the name be prefixed by the name of the company implementing the service. Microsoft will use the prefix "Microsoft". Services written by Microsoft should append their service name, for example Microsoft_RAS_TargetName.

(https://msdn.microsoft.com/en-us/library/windows/desktop/aa374788(v=vs.85).aspx)

Suggestion: Change the key used on Windows to be "$AppName_$oldkey". This is a good opportunity since we're migrating http credential keys anyway.

@ckamm ckamm added the Windows label Oct 26, 2017
@ckamm ckamm added this to the 2.4.0 milestone Oct 26, 2017
@ckamm ckamm self-assigned this Oct 26, 2017
ckamm added a commit that referenced this issue Oct 26, 2017
The application name is prepended to the key. QtKeychain doesn't
do that automatically on the platform.
ckamm added a commit that referenced this issue Nov 1, 2017
The application name is prepended to the key. QtKeychain doesn't
do that automatically on the platform.
@ckamm ckamm added the ReadyToTest QA, please validate the fix/enhancement label Nov 1, 2017
@SamuAlfageme
Copy link
Contributor

SamuAlfageme commented Nov 2, 2017

It seems to be working just fine now (tested in W7/8.1). Nice catch.

2.3.3 2.4.0
screenshot 2017-11-02 13 34 51 screenshot 2017-11-02 13 42 30

Some notes:

  • After Different accounts may use same keychain entry #5830 we have to be extra-careful and check for the trailing slash to be present when creating the keychain/cred.store entry in all cases - otherwise, the account id might be taken by mistake as the port no.
  • Also, in some upgrade scenario, the standard SSL port was included in the instance's URL everywhere - (might be unrelated - I'm trying to find out the conditions/steps again, this is merely aesthetic though)

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ReadyToTest QA, please validate the fix/enhancement Windows
Projects
None yet
Development

No branches or pull requests

2 participants