Banning QNetworkAccessManager

[Volker Krause]

On Wednesday, 19 February 2020 10:04:11 CET Ben Cooksley wrote:
> On Wed, Feb 19, 2020 at 9:30 PM Volker Krause <vkrause at kde.org> wrote:
> > On Wednesday, 19 February 2020 08:05:01 CET Ben Cooksley wrote:
> > > On Mon, Feb 3, 2020 at 7:42 AM Volker Krause <vkrause at kde.org> wrote:
> > > > I agree on the problem of QNAM's default, see also
> > > > https://conf.kde.org/en/
> > > > akademy2019/public/events/135 on that subject.
> > > > 
> > > > On Saturday, 1 February 2020 23:24:14 CET Ben Cooksley wrote:
> > > > [...]
> > > > 
> > > > > Prior to now, i've taken the approach of advertising that
> > > > > QNetworkAccessManager is broken and needs a flag set within
> > > > > applications when used, however unfortunately this seems to have
> > > > > been
> > > > > an ineffective approach and cases of broken behaviour continue to
> > > > > appear (despite this brokeness being known about and advertised
> > > > > since
> > > > > back in the Qt 4.x series when this class was introduced)
> > > > > 
> > > > > Therefore, given the Qt Project is unlikely to change their position
> > > > 
> > > > > on this, I would like to propose the following:
> > > > The Qt Project is actually in favor of changing at least the
> > > > redirection
> > > > default to exactly what we want it to be.
> > > > https://codereview.qt-project.org/c/qt/qtbase/+/273175 has the change,
> > > > and
> > > > got approval both by Lars and one of the maintainers. It is merely
> > > > stuck
> > > > in dealing with the unit test fallout in Qt's CI that somebody has to
> > > > take care of. Doesn't immediately help us of course, but claiming Qt
> > > > is
> > > > unwilling to change this is simply wrong.
> > > > 
> > > > > 1) That effective immediately, QNetworkAccessManager and it's
> > > > > children
> > > > > classes be banned and prohibited within KDE software, to be enforced
> > > > > by server side hooks.
> > > > > 2) That we fork QNetworkAccessManager and the associated classes
> > > > > within the appropriate Framework (to be determined), where the
> > > > > defective behaviour can then be corrected.
> > > > 
> > > > While this might solve the redirection problem, it brings us yet
> > > > another
> > > > then unmaintained HTTP implementation next to the one in KIO, which is
> > > > a
> > > > far bigger problem long term. We need less of those, not more.
> > > > 
> > > > To address the problem of people not using the correct defaults, I did
> > > > propose a much less extreme approach in the above mentioned talk at
> > > > Akademy, namely having a KNetworkAccessManager as a sub-class of QNAM
> > > > in
> > > > a low-tier frameworks that essentially just enables redirects and
> > > > HSTS.
> > > > QNAM's implementation isn't the problem after all, the defaults are.
> > > > 
> > > > Commit hooks warning about QNAM usage might still be a good idea, but
> > > > as a
> > > > warning only. Hard blocking QNAM-using code would block at least three
> > > > repos I spend a lot of time on currently, none of which even talks to
> > > > KDE
> > > > servers.
> > > > 
> > > > If you need to take more drastic measures against code not doing this
> > > > correctly regardless, you can do that my dropping the server-side
> > > > workarounds. Yes, that would break the affected application possibly,
> > > > but
> > > > at least it would not cause massive collateral damage for the people
> > > > using this correctly.
> > > > 
> > > > It would also help to know where specifically we have that problem, so
> > > > we
> > > > can actually solve it, and so we can figure out why we failed to fix
> > > > this
> > > > there earlier.
> > > 
> > > Just bringing this up again - it seems we've not had much movement on
> > > this aside from the Wiki page.
> > > 
> > > Examining that Qt merge request, it not only is targeted at just Qt
> > > 6.x, it also hasn't had any movement since the start of October 2019 -
> > > 4 months ago.
> > > Given that we are going to be on Qt 5.x for some time, that merge
> > > request can't be considered to be the 'fix' for this issue.
> > 
> > The targeting of Qt6 is due to this aiming at dev when that was still Qt
> > 5.x, but you are right of course, somebody needs to do the work there to
> > drive this to completion, no matter in which version it will actually
> > land.
> 
> Indeed.
> 
> > > We need a solution that will ensure software released today doesn't
> > > cause us pain in several years time, because as I illustrated in an
> > > earlier email to this thread, software has a habit of remaining in use
> > > for an extremely long amount of time (August 2014 being the release
> > > date of the Marble versions in question hitting that old URL).
> > > 
> > > The easiest path forward is to simply ban QNetworkAccessManager.
> > 
> > That might be the easiest path for you, but certainly not for me, given
> > two of the modules I work on most atm are using QNAM (not even to talk to
> > KDE infrastructure), and would suddenly no longer be allowed to be hosted
> > here?
> Ideally they would move to whatever replacement we have for QNAM
> should it be banned :)
> 
> > Also, I have yet to see a suggestion for a viable alternative to QNAM. The
> > initially proposed fork would mean trading the possibility of broken
> > redirect handling for an unmaintained HTTP stack, I don't think that's a
> > good deal for the security of our users.
> 
> It isn't perfect yes.
> 
> Sadly however the negative user experience created by countless broken
> applications due to the Qt Project being uncooperative in this matter
> for many years doesn't really give us much in the way of other
> options.
> 
> It doesn't help when Developers come to us asking for temporary
> measures to be put in place to keep their applications alive as
> they're getting bug reports about broken functionality. Saying no
> saves us the effort of maintaining the fixes, but leaves applications
> broken - which is not a fun position to be in as the one making the
> decision.
> 
> > Instead, how about the way we approached this for KUserFeedback? Before
> > getting things deployed on KDE infrastructure for use by library or
> > application code, sysadmins and developers review if the implementation is
> > allowing easy and secure operation of the infrastructure, redirect
> > handling
> > being one part of this. In case of KUserFeedback you suggested protocol
> > changes, they were implemented, and automatic tests for a number of
> > possible redirect scenarios were added. Worked out fine IMHO, despite
> > QNAM (the needed changes weren't due to missing redirect handling, but
> > due to somewhat non- standard redirects on HTTP POST).
> 
> Having a Sysadmin review an implementation that will be talking to a
> server is in general always a good idea, even outside of this
> particular usecase.
> It certainly helps protect against this class of issues that is for sure.
> 
> It isn't a perfect defense though, as if the review isn't requested
> issues could slip past :(

Right. However, as sysadmins are in control of what gets deployed on our 
infrastructure, you are in the perfect position to make sure such a review 
happens. I don't think anyone would have a problem with that being a hard 
requirement for getting server-side components or data deployed.

Regards,
Volker

> > There is a risk that the code breaks this again over time, independent of
> > test coverage, but I'd say that's not sysadmin's problem then, it's due
> > to application code not following the rules.
> > 
> > This of course only helps with new infrastructure deployments, but so
> > would a QNAM ban only help with new releases.
> > 
> > Additionally, improved documentation, a possible KNAM and/or driving the
> > QNAM changes upstream can still be done alongside this obviously.
> 
> Something like KNAM would allow for a more refined ban yes (allowing
> QNAM in the repository that hosts KNAM, but blocking it elsewhere)
> 
> > Regards,
> > Volker
> 
> Cheers,
> Ben

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20200220/41f0bb35/attachment.sig>