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

Remove encrypted password storage #1023

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from

Conversation

Vekhir
Copy link
Contributor

@Vekhir Vekhir commented Jun 20, 2024

OpenBoard saves the credentials for proxy connections and for uploading to YouTube. Since those need to be forwarded, they cannot be hashed, instead they are encrypted. This encryption is out of date and insecure.
The code has been imported from Open-Sankoré, i.e. it's over 13 years old. In the meantime, password managers have become widespread and easy-to-use, while being much more secure and better audited than the solution within OpenBoard. As such, they are better suited for storing the passwords, while OpenBoard will only ask and forward them without persistently storing them.

The proxy credentials need to be accessible from the UBNetworkAccessManager, so they are saved in a setting which is cleared at shutdown, i.e. no persistent storage.
The YouTube credentials are immediately forwarded, while the email can optionally be stored. The password is never saved.

In combination with #1019, this allows to get rid of the explicit OpenSSL dependency - it is of course still required implicitly via Qt.

Tested on Arch Linux with the community build and Qt6. While I have made sure that qmake should work too, I unfortunately cannot test that. I don't expect any differences with Qt5.

Questions and suggestions are welcome.

Closes #1021

Vekhir added 6 commits June 21, 2024 01:00
The password is saved in an insecure way and is only used for
later auto-fill-in. Password managers are a better fit for that.
The proxy credentials are currently stored in an insecure way, and
should not be stored persistently if it can be avoided.

For architectural reasons, the credentials have to be saved for the
runtime of the program to be accessed by the network manager.

Clear the credentials at the end of the program.
These methods are no longer used due to insecure practices, thus
remove them.
They are no longer used anywhere.
No UBCryptoUtils object will ever created, so destroy() is a noop.
Remove unused public functions
They provide no public functions and as such can be safely removed.
Also remove them from the build system (qmake and cmake).
@letsfindaway
Copy link
Collaborator

I have not tested, but I'm quite sure that the YouTube upload does not work in the current version of OpenBoard:

QUrl url("https://www.google.com/youtube/accounts/ClientLogin");

This URL which is used for login produces a 404 error. In the end this means that the complete YouTube upload cannot work.

Later the video is uploaded to the following URL:

QUrl url("http://uploads.gdata.youtube.com/feeds/api/users/default/uploads");

Here not even the server exists.

So the question is finally whether we should remove YouTube upload support completely - or fix it. I would vote for removal, as apparently nobody missed it.

@Vekhir
Copy link
Contributor Author

Vekhir commented Jun 21, 2024

I thought as much, thanks for confirming. Web stuff has incredibly fast bitrot, wouldn't surprise me if uploading to YT didn't work already when it was imported...
Equally, the proxy stuff probably holds up better over time, but suffers the same standards changes.

In any case, additionally removing YT support is better in a different PR. The removal of YT upload support entirely is probably also something that @kaamui could comment on.

@Vekhir
Copy link
Contributor Author

Vekhir commented Aug 13, 2024

I made a local branch over at Vekhir/OpenBoard#branch=remove-yt-upload-support. It is based on this PR and removes upload support to YouTube entirely, seeing as it's both unmaintained and unused/unusable.

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.

2 participants