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

Default settings: Store broadcast credentials as plain text #2158

Merged
merged 1 commit into from
Jun 15, 2019
Merged

Default settings: Store broadcast credentials as plain text #2158

merged 1 commit into from
Jun 15, 2019

Conversation

uklotzde
Copy link
Contributor

I get the following confusing error message when building Mixxx with QtKeychain and starting with a fresh profile:

qtkeychain_error

This fix prevents the error and instead logs a warning. I have no idea what the correct behavior is supposed to do! Please accept this PR or fix it yourself, whoever knows the solution!

@uklotzde uklotzde added this to the 2.3.0 milestone Jun 11, 2019
@daschuer
Copy link
Member

I cannot confirm the issue. I can store passwords with an empty login.
What does it complain on "show details"?

@daschuer
Copy link
Member

Which libgt5keychain do you uses? My is 0.5.0.

@uklotzde
Copy link
Contributor Author

v0.7.0

First start:
first_start

Restart:
next_start

KWalletManager is not installed, only GNOME Keyring. I'm not able to get rid of this error message. My existing profile works, no error message pops up.

@uklotzde
Copy link
Contributor Author

I'm not able to delete 'Connection 1'. Creating a new connection doesn't help either.

@daschuer
Copy link
Member

How is this set:
echo $XDG_CURRENT_DESKTOP
echo $DESKTOP_SESSION

@uklotzde
Copy link
Contributor Author

uklotzde commented Jun 12, 2019

Default Fedora installation with GNOME on Wayland:

$ echo $XDG_CURRENT_DESKTOP
GNOME
$ echo $DESKTOP_SESSION
gnome

@daschuer
Copy link
Member

OK, so you finally are here:
https://github.com/frankosterfeld/qtkeychain/blob/60e0d9dcd2a6a72514dea9e124c8b42f00a29625/keychain_unix.cpp#L138
It looks like it struggles to get in contact with Gnome Keyring.

I guess this PR is not a fix. I think we need to single step to the source to identify the issue.
The error messages fro qt5Keychain can be also improved, To get less confusing infos in a case like that.

@uklotzde
Copy link
Contributor Author

uklotzde commented Jun 13, 2019

My existing profile works without any issues. Any newly created profile doesn't work and I'm not able to fix this situation in Mixxx. This is a bad user experience and obviously an error, independent of whether Mixxx is able to acces GNOME Keyring or not.

@daschuer
Copy link
Member

I guess that the old profiles store the password in a file and the new one tried to use the chain. There is a radio button to control this.

@uklotzde
Copy link
Contributor Author

The Mixxx profile does not store any passwords and I have never set it.

@daschuer
Copy link
Member

Ok, so we need to special case the empty passwort instead of the empty login.

However, this is still a band aid, because Mixxx should be able to store an empty password to the file and to the key chain.

I think the final solution it to check what really happens on your system and give some better error messages and offer to fall back to insecure password storage.

@uklotzde
Copy link
Contributor Author

Mixxx should not generate any error messages regarding broadcasting after a fresh install if I never intend to use it. Even if Mixxx is built with QtKeychain support it should not use it unless actually needed.

@daschuer
Copy link
Member

Yes, of cause. The error message happens if you create a new profile with an empty password, right?

We have to figure out why qtkeychain is not able to store a password. Detect this and fall back to insecure password storage.

We may special case that passwort empty case, but I am in doubt if it it worth the work, because in almost all real live cases the password is not empty.

Do you have a clue why you get such confusing error messages? We need an idea how to work around it.

@uklotzde
Copy link
Contributor Author

Once again: Mixxx should not try to store any bogus login credentials anywhere in the system before the user enters one!

@daschuer
Copy link
Member

This means we need to store a new flag like "password empty" in the profile file. Than we need to migrate all empty passwords to this flag. A lot of work, that does not fix the issue you suffer here. I think we should find a fix first. I don't mind to store an empty password.

@uklotzde
Copy link
Contributor Author

The issue I suffer just reveals that Mixxx is not behaving as expected. A default installation of Mixxx should not make any modifications to the system unless the user gives permission. The only allowed exception is the application's settings folder.

I don't need neither a broadcast profile nor do I want to store any login credentials. This implementation seems to be really broken and we should not release 2.3 until it has been fixed! Seriously, the non-deletable default connection named 'Connection 1' already looks like a hack. I just never noticed until those issues appeared.

As a workaround we can simply switch the default from Secure store to Plain text. That would work for me.

@uklotzde uklotzde changed the title Skip QtKeychain access with empty login key Default settings: Store broadcast credentials as plain text Jun 14, 2019
@uklotzde
Copy link
Contributor Author

Done. This versions works out of the box without any issues.

@daschuer
Copy link
Member

The default connection 'Connection 1' is a legacy thing. Original we support only one connection and this was also not deletable. I have no strong issue with this.

The proposed change works for me. I guess if you try to store a secure password it will still fail right?
Such a setup showing the issue is of a high value.
I think we should handle your error with better error messages like:
"Failed to store secure password. Fall back to plain text password"
"Original error message: ..... "

Would you also mind to single step though the qt5keychain code and check why it fails in your case.
Maybe we can provide an upstream fix.

@daschuer
Copy link
Member

LGTM, waiting for CI.

@daschuer
Copy link
Member

OK Thank you. Merging now. If you find time it would be nice to do a test run to check what's the real issue in your case. We will later suffer this issue on other users machines using broadcasting i guess.

@daschuer daschuer merged commit 1710d69 into mixxxdj:master Jun 15, 2019
@uklotzde uklotzde deleted the qtkeychain_warning branch June 21, 2019 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants