signalserver integration #242

ElderOrb · 2016-12-07T23:20:51Z

No description provided.

JeromeMartinez · 2016-12-08T11:34:36Z

Source/GUI/preferences.cpp

+    QNetworkRequest request(ui->signalServerUrl_lineEdit->text() + "/fileuploads/upload/test");
+    request.setRawHeader("Authorization", "Basic " + QByteArray(QString("%1:%2")
+							 .arg(ui->signalServerLogin_lineEdit->text())
+							 .arg(ui->signalServerPassword_lineEdit->text()).toLocal8Bit().toBase64()));


As far as I understand, the password is sent in clear text (Authorization: Basic), possibly without secure connection (I see no restriction to HTTPS in signalServerUrl), which is considered as a bad practice nowadays.
The login should be limited to HTTPS or using an algorithm not transmitting the password in clear text and with a nonce (example).

Note: the QCTools server access from anyone not authorized on the same network (between the client and the server) is maybe not a big issue, but there are plenty of infected machines which scan the network and users usually use the same password on other websites, including more sensitive (sometimes personal) content.

Agree, authorization: basic is not the best way, but that's what backend provides for now.

ablwr · 2017-01-05T02:26:33Z

Let's get this conflict resolved!

ablwr · 2017-01-05T02:43:36Z

Thanks for the addition of the first-track/all-tracks for video/audio.

Are we gonna integrate the icons in Updates filter icon; adds signalserver icons #247 later?
Since the red-or-green status in the bottom right will be new to users, lets add a tooltip that says "signalserver status" (or if someone wants to recommend something else).
This may somewhat existing, but if I open the Preferences tab, click away from it, and continue about my business -- I am "not able to" open Preferences again (in reality, it's hidden behind the main window). In current QCTools, other actions are locked up which isn't great either (although I think the Preferences window should always stay at the 'top' until closed).

Should I (or someone else) add some language to the Help pages about how to use signalserver? Can do within this branch or in another branch post-merging.

This is great work, Alexander! (and Yayoi)!

dericed · 2017-01-05T17:25:32Z

Hmmm, both sides of the conflict appear to be @ElderOrb's work, so leaving the resolve to him.

@ablwr, the first/all tracks was added several months ago, mainly because @kieranjol loves processing files with 16 track audio and that really slows everything down substantially, so now there's a first/all choice but it's not part of this PR.

+1 for a tooltip. I think you can add those by adding a

<property name="toolTip">
    <string>click on help!</string>
</property>

under the associated .

I noticed the preference window behavior too. This used to be the case with the player until 3f2a2ef, so perhaps adding BigDisplayArea->hide(); to preferences.cpp would fix it.

metacynicv2 · 2017-01-06T00:52:03Z

In the field with the password, can we change it to asterisks instead of the password being fully visible?

kieranjol · 2017-01-06T09:38:16Z

I agree with @kellyhaydon, obscured passwords would be preferable.

dericed · 2017-01-06T15:24:15Z

Additionally the password should be stored securely as well.

ElderOrb · 2017-01-06T16:17:24Z

What kind of encryption would you prefer?

dericed · 2017-01-06T16:19:20Z

spammimic. jk, do you have a suggestion for something responsible.

retokromer · 2017-01-06T16:36:57Z

ROT13 – sorry, I cannot resist! I use gpg or crypt.

dericed · 2017-01-06T16:40:02Z

For extra security, please apply ROT13 twice.

ElderOrb · 2017-01-06T16:42:47Z

Taking into account qctools is opensource and anyone can review code and learn how to decrypt password, do we really need strong / complicated encryption here? Or I can just qCompress password ? :) I'm thinking about not introducing 3rd-party dependencies without strong need. Just an idea.

JeromeMartinez · 2017-01-06T17:31:44Z

If we speak about security of the password, from my point of view there are 3 separate issues:

1/ do not show the password in the interface: "QLineEdit::setEchoMode(QLineEdit::Password);", so you can share a screen and write your password without showing it.

2/ do not use HTTP for transmission of the password: do it in HTTPS with a strong test of the validity of the certificate, never HTTP and/or HTTPS without test of the certificate. Note that my previous remark about the password in clear text on the network, which is the proposed behavior in the PR, is still valid. So whatever you do on the local machine, I can catch your password with a sniffer on your network until your remove the HTTP stuff...

3/ do not store the password: use an API between the client and the server, and store a token (provided by the server from a login/pass provided once by the user and never saved) provided by this API on the local disk, and only that; also provide an interface on the server for listing tokens and permit to revoke one. With this practice, the password is never stored on the local machine and a machine can be revoked (by revoking the token on the server) if it is no more secure.

obscured passwords would be preferable.
(...)
Additionally the password should be stored securely as well.

Security through obscurity is a placebo, someone wanting to find the password will find it whatever you do, if you decide to store it.

As people use to have the same password everywhere (bad practice but the reality), it is important to not store a password even if the project is not important, it may be a security breach for the end user.

ElderOrb · 2017-01-07T09:13:42Z

1/ do not show the password in the interface: "QLineEdit::setEchoMode(QLineEdit::Password);", so you can share a screen and write your password without showing it.

Done. But do we need something like 'show password' (for the case it was forgotten) ?

2/ do not use HTTP for transmission of the password: do it in HTTPS with a strong test of the validity of the certificate

I don't disagree, but AFAIK at the moment backend provides http-only API. @dericed if you agree that HTTPS is 'must have' - I'll start discussing details with Yayoi.

3/ do not store the password: use an API between the client and the server, and store a token

Are you talking about something like OAuth? Do you have any opensource crossplatform implementations in mind (not requiring C++ 11 and compatible with Qt 4 :) ) ?

…ent periodical connection check, add connection indicator to status bar

JeromeMartinez · 2017-01-07T09:32:03Z

3/ do not store the password: use an API between the client and the server, and store a token
I don't disagree, but AFAIK at the moment backend provides http-only API.

<troll> HTTPS, replacing HTTP, exists for a longer time (1994) than C++11 (2011) and Qt5 (2012), your priorities about "old technologies to kill" are weird (especially here due to the sensitive issue about passwords in clear text on the network), and not mine (my priority is to avoid HTTP for security reasons).
Choosing/developing a backend not supporting a 20+ year old technology is weird, especially when there are so many discussions on the Internet about Internet privacy nowadays.</troll>

Are you talking about something like OAuth? Do you have any opensource crossplatform implementations in mind (not requiring C++ 0x11 and compatible with Qt 4 :) ) ?

OAuth is a possibility, I guess.
@GuillaumeRoques, please provide some technical details about how you implemented in MediaConch (the client) and MediaConchOnline (the server).
(but this may be not usable directly on QCTools as we use QtWebKit/QtWebEngine on the client)

ablwr · 2017-01-10T03:12:19Z

I didn't think about the 'checking' state ... how long do you think that will take? If it's a ping to the server, it shouldn't take very long, right? How about using nothing in the intermediary and I can add something if/when there's a need?

ElderOrb · 2017-01-10T07:25:44Z

I didn't think about the 'checking' state ... how long do you think that will take?

Under normal conditions (server is up & running, internet connection is fine) it will happen nearly immediately, but when something goes wrong, 'checking' phase might take several seconds. To get better idea what I'm talking about just try to specify wrong IP address for backend and then press 'upload'

GuillaumeRoques · 2017-01-10T09:38:40Z

Some more details in response of @JeromeMartinez comment :
The MediaConchOnline API use a simple token based authentication (no OAuth as it's a bit overkill for our needs).
First the client (MediaConch-GUI) call a login API resource with the credentials (provided by the user), the server (MediaConchOnline) send the token (generated by the PHP Symfony framework) and the client store it (only the token, not the credentials).
To access the API the client send the token to the server (we choose to send it in the http header), the server check if the token is valid and authenticate the client.
The API is reachable only through HTTPS.

dericed · 2017-01-10T14:23:29Z

some minor comments:

tab order on signalserver pref pane is wrong (tabbing goes from ip to password to username)
should the upload-signalserver and status-signalserver icons be hidden unless signalserver is enabled?
for url if I enter an ip with no protocol, I get an error "Protocol "" is unknown", can we default to http if there isn't one (since it's all we support).
if i drag 2 files into qctools, it goes to the list view. Processing is happening so if I click on either row, the toolbar icon for upload station is X. Once both files complete processing, the X changes to a check. Then I click on the other row (also uploaded) and status is back to X. If I click on the row I was on earlier, still X.

dericed · 2017-01-10T14:24:20Z

Also perhaps the signalserver status icon is out of context in the list view since it would then be reflecting the status of many files.

dericed · 2017-01-10T14:33:24Z

also password is still stored in plain text in ~/Library/Preferences/net.mediaarea.QCTools.plist

ElderOrb · 2017-01-10T14:39:48Z

Thank you for the feedback, will try to resolve this issues later today.

ElderOrb · 2017-01-10T15:34:35Z

With regards to local password encryption, what do you think about this: https://github.com/roop/qblowfish ? Seems like a good compromise between big advanced library like Crypto++ and ROT13/xor/qCompress :)

retokromer · 2017-01-10T15:57:58Z

In my opinion, Blowfish it’s more than sufficient for QCTools. I didn’t know this Qt implementation: thank you for the information! (The Twofish algorithm would be stronger. If I’m not completely wrong, it’s used in GPG.)

…"http"

ElderOrb · 2017-01-10T22:36:25Z

Also perhaps the signalserver status icon is out of context in the list view since it would then be reflecting the status of many files.

It still have some sense if one file has been selected. Should I hide it if zero / several files has been selected?

- store encrypted password in QSettings

retokromer · 2017-01-11T07:13:15Z

Source/GUI/mainwindow.h

 private:
    void updateScrollBar( bool blockSignals = false );
    bool isPlotZoomable() const;
    void Zoom( bool );
    void changeFilterSelectorsOrder(QList<std::tuple<int, int> > filtersInfo);

    DraggableChildrenBehaviour* draggableBehaviour;
+    SignalServer* signalServer;
+    SignalServerConnectionChecker* connectionChecker;
+    QWidget*                    connectionIndicator;


retokromer · 2017-01-11T07:15:08Z

Caveat: I didn’t test it. Reading the code it LGTM.

JeromeMartinez · 2017-01-11T07:22:34Z

integrate 3rdparty blowfish encryption

The way it is integrated has severe issues:

if the goal is to hide the password, the patch fails to do it (the password can be retrieved with a reverse blowfish), except for kiddies just looking at the plist and not the source code (and/or doing some tests)
the key is the same for everyone (and hardcoded), so the encrypted result is same for 2 persons having the same password.
the user reading the plist may think that the password is securely stored, this is misleading.

I prefer to have the password in clear text so we don't forget that the password is not securely stored rather to sweeping this security issue under the carpet.

JeromeMartinez · 2017-01-11T07:49:06Z

If you can not modify the server API (this is the preferred method), the only solution for securely store a password is to use the Operating System password manager (OS X keychain, Gnome password manager, KWallet, Windows Credential Store...) which will prevent people from accessing the password without the user "master" password.
I found https://github.com/frankosterfeld/qtkeychain/ but never tested it (and the "master" password will be requested sometimes, this is the drawback but there is no choice is you want to keep the password in a safe place ans still having it on the machine).

Note that this does not change the issue about the password sent in clear text on the network (I repeat myself, I know, and I offer no patch, I know, but I prefer that we are 100% clear about this risk in the case the server API is not changed).

ablwr · 2017-01-18T01:11:13Z

Since the issues brought up in this PR now have their own open issues (#140 and #141), can we merge this in? It will assist in people's ability to QA this new version, and help us overall. Daily builds are meant to be bleeding-edge!

dericed · 2017-01-18T01:16:36Z

Can you add some boilerplate note to say that passwords are stored and transmit in plain text. We could then resolve this before the next release so the issue would only exist in a range of daily builds.

JeromeMartinez reviewed Dec 8, 2016

View reviewed changes

ElderOrb force-pushed the signalserver branch 8 times, most recently from 21e8756 to d2eb46a Compare December 17, 2016 11:40

ElderOrb force-pushed the signalserver branch 2 times, most recently from a63bcd0 to 483481b Compare December 20, 2016 00:05

dericed requested a review from ablwr January 4, 2017 18:20

add preference page with signalserver preferences and connection checher

10b0427

ElderOrb force-pushed the signalserver branch from 483481b to f65f247 Compare January 7, 2017 09:22

extract connection checking functionality into separate class, implem…

746f297

…ent periodical connection check, add connection indicator to status bar

ElderOrb force-pushed the signalserver branch from f65f247 to 9505635 Compare January 7, 2017 09:32

ablwr approved these changes Jan 10, 2017

View reviewed changes

ElderOrb added 4 commits January 11, 2017 00:49

fix tab order

cffb44d

hide signalserver-related actions if signalserver disabled

090c1b5

prepend signalserver url string with "http://" if doesn't start with …

789a2f3

…"http"

update upload status on selected file changes if upload was initiated

16409af

- integrate 3rdparty blowfish encryption

676b2cf

- store encrypted password in QSettings

retokromer reviewed Jan 11, 2017

View reviewed changes

fix spaces

03baf22

This was referenced Jan 14, 2017

HTTPS support bavc/signalserver#140

Open

Support of tokens instead of password only bavc/signalserver#141

Open

add warning about insecure password storing & transmit

9d2748d

dericed changed the title ~~[WIP] signalserver integration~~ signalserver integration Jan 18, 2017

dericed merged commit c852f3f into bavc:master Jan 19, 2017

dericed mentioned this pull request Jan 30, 2017

signalserver integration amiaopensource/vrecord#134

Closed

ElderOrb deleted the signalserver branch July 19, 2017 21:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

signalserver integration #242

signalserver integration #242

ElderOrb commented Dec 7, 2016

JeromeMartinez Dec 8, 2016

ElderOrb Dec 8, 2016

ablwr commented Jan 5, 2017

ablwr commented Jan 5, 2017

dericed commented Jan 5, 2017

metacynicv2 commented Jan 6, 2017

kieranjol commented Jan 6, 2017

dericed commented Jan 6, 2017

ElderOrb commented Jan 6, 2017

dericed commented Jan 6, 2017

retokromer commented Jan 6, 2017

dericed commented Jan 6, 2017

ElderOrb commented Jan 6, 2017

JeromeMartinez commented Jan 6, 2017

ElderOrb commented Jan 7, 2017 •

edited

Loading

JeromeMartinez commented Jan 7, 2017 •

edited

Loading

ablwr commented Jan 10, 2017

ElderOrb commented Jan 10, 2017

GuillaumeRoques commented Jan 10, 2017

dericed commented Jan 10, 2017

dericed commented Jan 10, 2017

dericed commented Jan 10, 2017

ElderOrb commented Jan 10, 2017

ElderOrb commented Jan 10, 2017

retokromer commented Jan 10, 2017

ElderOrb commented Jan 10, 2017

retokromer Jan 11, 2017

retokromer commented Jan 11, 2017

JeromeMartinez commented Jan 11, 2017

JeromeMartinez commented Jan 11, 2017 •

edited

Loading

ablwr commented Jan 18, 2017

dericed commented Jan 18, 2017

signalserver integration #242

signalserver integration #242

Conversation

ElderOrb commented Dec 7, 2016

JeromeMartinez Dec 8, 2016

Choose a reason for hiding this comment

ElderOrb Dec 8, 2016

Choose a reason for hiding this comment

ablwr commented Jan 5, 2017

ablwr commented Jan 5, 2017

dericed commented Jan 5, 2017

metacynicv2 commented Jan 6, 2017

kieranjol commented Jan 6, 2017

dericed commented Jan 6, 2017

ElderOrb commented Jan 6, 2017

dericed commented Jan 6, 2017

retokromer commented Jan 6, 2017

dericed commented Jan 6, 2017

ElderOrb commented Jan 6, 2017

JeromeMartinez commented Jan 6, 2017

ElderOrb commented Jan 7, 2017 • edited Loading

JeromeMartinez commented Jan 7, 2017 • edited Loading

ablwr commented Jan 10, 2017

ElderOrb commented Jan 10, 2017

GuillaumeRoques commented Jan 10, 2017

dericed commented Jan 10, 2017

dericed commented Jan 10, 2017

dericed commented Jan 10, 2017

ElderOrb commented Jan 10, 2017

ElderOrb commented Jan 10, 2017

retokromer commented Jan 10, 2017

ElderOrb commented Jan 10, 2017

retokromer Jan 11, 2017

Choose a reason for hiding this comment

retokromer commented Jan 11, 2017

JeromeMartinez commented Jan 11, 2017

JeromeMartinez commented Jan 11, 2017 • edited Loading

ablwr commented Jan 18, 2017

dericed commented Jan 18, 2017

ElderOrb commented Jan 7, 2017 •

edited

Loading

JeromeMartinez commented Jan 7, 2017 •

edited

Loading

JeromeMartinez commented Jan 11, 2017 •

edited

Loading