Re: Banning QNetworkAccessManager
Hi, as Volker said, without any alternative, we can't just ban QNAM. And even with one, we would need time to implement it (for example, for GCompris, this part of the code was written by someone who is less active now, so rewriting it, testing it and be sure it works will take some time). Can't we use lxr to find all applications using it, check if they use it in a good way or not instead? Johnny Le mer. 19 févr. 2020 à 10:05, Ben Cooksley a écrit : > > On Wed, Feb 19, 2020 at 9:30 PM Volker Krause wrote: > > > > On Wednesday, 19 February 2020 08:05:01 CET Ben Cooksley wrote: > > > On Mon, Feb 3, 2020 at 7:42 AM Volker Krause 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
Re: Banning QNetworkAccessManager
On Thu, Feb 20, 2020 at 9:57 PM Volker Krause wrote: > > On Wednesday, 19 February 2020 10:04:11 CET Ben Cooksley wrote: > > On Wed, Feb 19, 2020 at 9:30 PM Volker Krause wrote: > > > On Wednesday, 19 February 2020 08:05:01 CET Ben Cooksley wrote: > > > > On Mon, Feb 3, 2020 at 7:42 AM Volker Krause 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
Re: Banning QNetworkAccessManager
El dijous, 20 de febrer de 2020, a les 14:29:47 CET, Allen Winter va escriure: > On Wednesday, February 19, 2020 6:09:02 PM EST Albert Astals Cid wrote: > > El dimecres, 19 de febrer de 2020, a les 9:28:22 CET, Volker Krause va > > escriure: > > > Additionally, improved documentation, a possible KNAM and/or driving the > > > QNAM > > > changes upstream can still be done alongside this obviously. > > > > Also for Qt5 which will probably never be changed a clazy warning and > > making it easy to run clazy on gitlab CI would be great. > > > > Krazy has a checker for QNetworkAccessManager use in Qt4 code. > I could add a checker for Qt5 code if someone tells me what to look for > (not that many people look at krazy reports. couldn't hurt. might help. There's other ways to do it, but i guess we could start with something that checks for this? https://community.kde.org/Policies/API_to_Avoid#QNetworkAccessManager Cheers, Albert
Re: Banning QNetworkAccessManager
El jue., 20 de feb. de 2020 a la(s) 10:30, Allen Winter (win...@kde.org) escribió: > > On Wednesday, February 19, 2020 6:09:02 PM EST Albert Astals Cid wrote: > > El dimecres, 19 de febrer de 2020, a les 9:28:22 CET, Volker Krause va > > escriure: > > > Additionally, improved documentation, a possible KNAM and/or driving the > > > QNAM > > > changes upstream can still be done alongside this obviously. > > > > Also for Qt5 which will probably never be changed a clazy warning and > > making it easy to run clazy on gitlab CI would be great. > > > > Krazy has a checker for QNetworkAccessManager use in Qt4 code. > I could add a checker for Qt5 code if someone tells me what to look for > (not that many people look at krazy reports. couldn't hurt. might help. I started making a checker for this based on clang-static-analyzer. Looks like clazy uses a different approach altogether by looking at ASTs alone, so I don't think I can integrate into that... -- Nicolás
Re: Banning QNetworkAccessManager
On Wednesday, February 19, 2020 6:09:02 PM EST Albert Astals Cid wrote: > El dimecres, 19 de febrer de 2020, a les 9:28:22 CET, Volker Krause va > escriure: > > Additionally, improved documentation, a possible KNAM and/or driving the > > QNAM > > changes upstream can still be done alongside this obviously. > > Also for Qt5 which will probably never be changed a clazy warning and making > it easy to run clazy on gitlab CI would be great. > Krazy has a checker for QNetworkAccessManager use in Qt4 code. I could add a checker for Qt5 code if someone tells me what to look for (not that many people look at krazy reports. couldn't hurt. might help.
Re: Banning QNetworkAccessManager
On Wednesday, 19 February 2020 10:04:11 CET Ben Cooksley wrote: > On Wed, Feb 19, 2020 at 9:30 PM Volker Krause wrote: > > On Wednesday, 19 February 2020 08:05:01 CET Ben Cooksley wrote: > > > On Mon, Feb 3, 2020 at 7:42 AM Volker Krause 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
Re: Banning QNetworkAccessManager
El dimecres, 19 de febrer de 2020, a les 9:28:22 CET, Volker Krause va escriure: > Additionally, improved documentation, a possible KNAM and/or driving the QNAM > changes upstream can still be done alongside this obviously. Also for Qt5 which will probably never be changed a clazy warning and making it easy to run clazy on gitlab CI would be great. Cheers, Albert > > Regards, > Volker
Re: Banning QNetworkAccessManager
El dimecres, 19 de febrer de 2020, a les 8:05:01 CET, Ben Cooksley va escriure: > The easiest path forward is to simply ban QNetworkAccessManager. Stop saying that, that's not going to happen. Cheers, Albert > > > > > Regards, > > Volker > > Regards, > Ben >
Re: Banning QNetworkAccessManager
On Wed, Feb 19, 2020 at 9:30 PM Volker Krause wrote: > > On Wednesday, 19 February 2020 08:05:01 CET Ben Cooksley wrote: > > On Mon, Feb 3, 2020 at 7:42 AM Volker Krause 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
Re: Banning QNetworkAccessManager
On Wednesday, 19 February 2020 08:05:01 CET Ben Cooksley wrote: > On Mon, Feb 3, 2020 at 7:42 AM Volker Krause 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. > 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? 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. 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
Re: Banning QNetworkAccessManager
On Mon, Feb 3, 2020 at 7:42 AM Volker Krause 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. 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. > > Regards, > Volker Regards, Ben
Re: Banning QNetworkAccessManager
[...] > All of the places where we have actively hit this issue have already > been fixed (Marble and Attica come immediately to mind, not sure if > GCompris fixed their code). > I took a quick look and we use some code to handle redirection: https://github.com/gcompris/GCompris-qt/blob/master/src/core/DownloadManager.cpp#L502 It's not the same code as mentioned by David in https://community.kde.org/Policies/API_to_Avoid#QNetworkAccessManager, I'll update the code tonight. Johnny > The rest continue to be sleeping timebombs which we will only discover > when we change something on the server side and put in place a > redirect. They may never be triggered, or they could be triggered next > week. Even if we fix the code now, due to LTS distributions and people > using software for far longer than they should, it will take years for > the fixes to fully reach user systems. > > To illustrate how long this takes, Marble moved from using > http://files.kde.org/marble/maps/ to https://maps.kde.org/ back in > January 2016. Four years later, we still get 13,000 hits to paths > under the old URL every day. The version numbers sent by Marble as > part of these requests indicate that some of them are from the version > released with KDE 4.14 which was released back in August 2014 > (fortunately this code path was fixed to follow redirects prior to > that release) > > > > > Regards, > > Volker > > Cheers, > Ben
Re: Banning QNetworkAccessManager
On Mon, Feb 3, 2020 at 11:51 PM Johan Ouwerkerk wrote: > > On Mon, Feb 3, 2020 at 11:27 AM laurent Montel wrote: > > > > Le lundi 3 février 2020, 10:49:10 CET David Edmundson a écrit : > > > I updated: > > > > > > https://community.kde.org/Policies/API_to_Avoid > > > > > > Which had no mention of this. > > > > > > David > > > > I think that you made an error > > > > "networkAccessManger->setAttribute(QNetworkRequest::FollowRedirectsAttribute, > > true); " > > Doesn't exist it's a enum from QnetworkRequest::RedirectPolicy > > And FollowRedirectsAttribute is old value > > It seems that we need to use QnetworkRequest::NoLessSafeRedirectPolicy > > directly no ? > > > > Yes, the example code is definitely wrong: in the real world redirects > are an attack vector. A few cases to consider: > > * Loops of redirects (could happen if the site is broken) > * Leaking sensitive information via e.g. the Referrer header I would recommend follwoing the various browsers (Firefox & Chromium in particular) behaviour here. Not only are they what we generally test redirects with when doing server maintenance, but they also have put quite a bit of work into being secure. With regards to loops, stopping after 10 or so redirects should be more than sufficient as a safe guard here. 5 if you'd like to be a bit more restrictive. I can't imagine a need with our systems to need more than 3 (first to https, second from somewhere to the mirror network redirector, third to a mirror) Because mirrors often don't support https (and Mirrorbrain doesn't support handling https mirrors) clients that interact with download.kde.org or files.kde.org *must* support transitioning from https:// to http:// (under a different domain - so https://download.kde.org to http://ftp.gwdg.de for instance). This would primarily affected applications that need to fetch large binary assets. > > Regards, > > - Johan Cheers, Ben
Re: Banning QNetworkAccessManager
On Mon, Feb 3, 2020 at 10:49 PM David Edmundson wrote: > > I updated: > > https://community.kde.org/Policies/API_to_Avoid > > Which had no mention of this. Many thanks for updating the wiki David. > > David Cheers, Ben
Re: Banning QNetworkAccessManager
On Monday, 3 February 2020 10:49:10 CET David Edmundson wrote: > I updated: > > https://community.kde.org/Policies/API_to_Avoid > > Which had no mention of this. Thanks for taking care of this! I'd propose a slightly different approach than the per-request all-or-nothing attribute mentioned in the wiki though, using the redirection policy on QNAM, which prevents redirects to non-TLS connections: nam->setRedirectPolicy(QNetworkRequest::NoLessSafeRedirectPolicy); And while we are at it, let's also enable HSTS: nam->setStrictTransportSecurityEnabled(true); nam->enableStrictTransportSecurityStore(true); Regards, Volker signature.asc Description: This is a digitally signed message part.
Re: Banning QNetworkAccessManager
On Mon, Feb 3, 2020 at 11:27 AM laurent Montel wrote: > > Le lundi 3 février 2020, 10:49:10 CET David Edmundson a écrit : > > I updated: > > > > https://community.kde.org/Policies/API_to_Avoid > > > > Which had no mention of this. > > > > David > > I think that you made an error > > "networkAccessManger->setAttribute(QNetworkRequest::FollowRedirectsAttribute, > true); " > Doesn't exist it's a enum from QnetworkRequest::RedirectPolicy > And FollowRedirectsAttribute is old value > It seems that we need to use QnetworkRequest::NoLessSafeRedirectPolicy > directly no ? > Yes, the example code is definitely wrong: in the real world redirects are an attack vector. A few cases to consider: * Loops of redirects (could happen if the site is broken) * Leaking sensitive information via e.g. the Referrer header Regards, - Johan
Re: Banning QNetworkAccessManager
No, no. Please go the other way round and update the wiki to whatever working code you have. David
Re: Banning QNetworkAccessManager
Le lundi 3 février 2020, 10:49:10 CET David Edmundson a écrit : > I updated: > > https://community.kde.org/Policies/API_to_Avoid > > Which had no mention of this. > > David I think that you made an error "networkAccessManger->setAttribute(QNetworkRequest::FollowRedirectsAttribute, true); " Doesn't exist it's a enum from QnetworkRequest::RedirectPolicy And FollowRedirectsAttribute is old value It seems that we need to use QnetworkRequest::NoLessSafeRedirectPolicy directly no ? -- Laurent Montel | laurent.mon...@kdab.com | KDE/Qt Senior Software Engineer KDAB (France) S.A.S., a KDAB Group company Tel. France +33 (0)4 90 84 08 53, www.kdab.fr KDAB - The Qt, C++ and OpenGL Experts - Platform-independent software solutions
Re: Banning QNetworkAccessManager
I updated: https://community.kde.org/Policies/API_to_Avoid Which had no mention of this. David
Re: Banning QNetworkAccessManager
On Mon, Feb 3, 2020 at 7:42 AM Volker Krause 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. Last I heard they were unwilling to change the default, so it's nice to know they're looking at revisiting their error. Back when this issue originally developed back in the Qt 4.x era (just a couple of releases after QNAM appeared) the Qt Developers at the time not only refused to change the default, but also refused to consider adding any kind of convenience function. They considered that their position was the most correct one and that following redirects simply wasn't required (as it isn't a MUST in the RFCs). It was later on (around Qt 5.7 I think) that a set of functions and enums were added to Qt to make it convenient to change the behaviour. When these were initially introduced the behaviour was changed to follow redirects, however that was then overturned before it was released (on the basis of an absolute need for "behavioural compatibility", something that wasn't an issue when the change went through initially, so my suspicion is a customer of Digia at the time threw their toys) I wasn't involved directly in the conversations in the Qt 5.7 timeframe, so don't have a link to that i'm afraid. The Qt 4.x one was a conversation on IRC. While that merge request is promising, it unfortunately is targeted at the 'dev' branch, which if i'm not wrong is Qt 6.x, so it will be some time before we get to see the benefit of that (more on that below). > > > 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. > It is not ideal I agree. What I would like to ensure however is that we are not impacted in the future by any policy decisions made by the Qt Developers in respect of this set of classes. > 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. > This may work, but we would need to force people to move to using the wrapper, and it still leaves us exposed to anywhere that retrieves a QNAM constructed by code located elsewhere. > 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. > Nicolas did a spot check of some of the places where QNAM is used in KDE, and at first glance it appears that not a single one enabled the following of redirects :( It is likely that very few places handle this correctly (probably only those who have interacted with Sysadmin on the matter) > 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. All of the places where we have
Re: Banning QNetworkAccessManager
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. Regards, Volker signature.asc Description: This is a digitally signed message part.
Banning QNetworkAccessManager
Hi all, For an extremely long time now, it has been a known issue with the QNetworkAccessManager that by default it does not follow redirects as issued by the server it is accessing. This is something the Qt Project has refused to address using the justification of 'behavioural compatibility' This behaviour is contrary to that of just about every other HTTP stack (with the exception of libcurl from my understanding) and is behaviour that nobody expects. As a consequence of this (broken) behaviour, Sysadmin has been effectively forced to implement workarounds and other compatibility measures in place to keep applications functional. These measures harm the long term maintainability of our systems, and sometimes limit our ability to make services more scalable. In some instances, the cost of compatibility measures would be too high, resulting in functionality of applications being broken. I am aware of at least one instance where if a compatibility measure we maintain is removed, the application will crash (segfault) during it's operation (a failure that makes the application unusable) 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: 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. Comments? Regards, Ben Cooksley KDE Sysadmin
