Re: Review Request 116555: Add support for pam-kwallet in kwalletd
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116555/#review52293 --- I wonder if you're being too much verbose with the debugs. kwalletd/main.cpp https://git.reviewboard.kde.org/r/116555/#comment37041 Make this function static like the other ones? - Albert Astals Cid On March 2, 2014, 11:33 p.m., Àlex Fiestas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116555/ --- (Updated March 2, 2014, 11:33 p.m.) Review request for KDE Runtime, Release Team and Valentin Rusu. Repository: kde-runtime Description --- This patch adds support for pam-kwallet (in my scratch right now, to be released soon). This is how the new pam works, and why this patch is needed: In order to open the wallet in a secure way we have to try hard to not send the hash on an insecure manner. This is how we achieve that: -pam_kwallet creates a pipe. -pam_kwallet opens a local socket listening somewhere (/tmp/foo.socket for example). -pam_kwallet forks+execv kwallet, passing via arguments the sockets (pipe and local socket). -pam_kwallet sends the hash via the pipe. -kwalletd gets the hash and waits for the environment. -startkde uses socat to send the environment to kwalletd. -kwalletd setups the environment before any Qt code is executed. -kwalletd resumes execution. With this way of executing kwallet we get: -pam_kwallet knows to who it is sending the hash (its on child). -hash is never revealed on shared memory (dbus), since pipes are private to the apps. -ptrace is usually disabled so only root can see the hash on the app memory -no Qt code is executed without the proper environment (same as startkde) -if kwalletd is executed normally (not from pam_kwallet) then it is business as usual. The patch also comes with integration tests that simulate how kwalletd is executed in the pam module. For the release team, I would love to add this to 4.13, after all it is innocuous if kwalletd is not executed via pam_module. Diffs - kwalletd/CMakeLists.txt e9aef16 kwalletd/autotests/CMakeLists.txt PRE-CREATION kwalletd/autotests/kdewallet.kwl PRE-CREATION kwalletd/autotests/kwalletexecuter.h PRE-CREATION kwalletd/autotests/kwalletexecuter.cpp PRE-CREATION kwalletd/autotests/qtest_kwallet.h PRE-CREATION kwalletd/autotests/testpamopen.cpp PRE-CREATION kwalletd/autotests/testpamopennofile.cpp PRE-CREATION kwalletd/main.cpp f9af414 Diff: https://git.reviewboard.kde.org/r/116555/diff/ Testing --- Thanks, Àlex Fiestas
Re: Review Request 116604: Allow directories with . as output for meinproc
On March 5, 2014, 3:36 p.m., Burkhard Lück wrote: src/meinproc.cpp, lines 170-179 https://git.reviewboard.kde.org/r/116604/diff/1/?file=252003#file252003line170 How does this affect this code in KHelpcenter: kde-runtime/khelpcenter/glossary.cpp:149:KProcess *meinproc = new KProcess; kde-runtime/khelpcenter/glossary.cpp:153:*meinproc KStandardDirs::locate( exe, QLatin1String( meinproc4 ) ); kde-runtime/khelpcenter/glossary.cpp:154:*meinproc QLatin1String( --output ) m_cacheFile; kde-runtime/khelpcenter/glossary.cpp:155:*meinproc QLatin1String( --stylesheet ) kde-runtime/khelpcenter/glossary.cpp:157:*meinproc m_sourceFile; kde-runtime/khelpcenter/glossary.cpp:176:KProcess *meinproc = static_castKProcess *(sender()); kde-runtime/khelpcenter/toc.cpp:148:KProcess *meinproc = new KProcess; kde-runtime/khelpcenter/toc.cpp:152:*meinproc KStandardDirs::locate(exe, meinproc4); kde-runtime/khelpcenter/toc.cpp:153:*meinproc --stylesheet KStandardDirs::locate( data, khelpcenter/table-of-contents.xslt ); kde-runtime/khelpcenter/toc.cpp:154:*meinproc --output m_cacheFile; kde-runtime/khelpcenter/toc.cpp:155:*meinproc m_sourceFile; kde-runtime/khelpcenter/toc.cpp:172:KProcess *meinproc = static_castKProcess *(sender()); About the issue with dot in Path see also http://lists.kde.org/?l=kde-doc-englishm=127421104303628w=2 Yes, the bug is the one reported in that email. It has been filed as bug https://bugs.kde.org/show_bug.cgi?id=246755 (I set it in this RR). The problem was that the value was passed without quotes as parameter to the the function in libxslt. But on the other side it is not used, because the output of the stylesheet is stored in a string and then written in a file (see transform function in xslt.cpp), so it's pointless to pass it. - Luigi --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116604/#review52091 --- On March 5, 2014, 1:06 a.m., Luigi Toscano wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116604/ --- (Updated March 5, 2014, 1:06 a.m.) Review request for Documentation, KDE Frameworks, kdelibs, and Aleix Pol Gonzalez. Bugs: 246755 https://bugs.kde.org/show_bug.cgi?id=246755 Repository: kdoctools Description --- The outputFile parameter is not used by the stylesheets, so don't pass it. If a directory starts with ., it is interpreted in a wrong way by libxslt with an error like: --- XPath error : Invalid expression /home/kde-devel/.cache5/khelpcenter/help/__home__kde- devel__kde__share__doc__HTML__en__kioslave__file__index.docbook ^ runtime error Evaluating user parameter outputFile failed --- This is an old issue, it was solved on windows by not compiling that code, but I suspect that the issue has been in UNIX systems too for a long time. Another way to solve the bug is quoting the value of the parameter with '...', replacing: params.append(qstrdup(parser.value(QStringLiteral(output)).toLocal8Bit().constData())); with something like QString quotedOutput = ' + parser.value(QStringLiteral(output)) + '; params.append(qstrdup(quotedOutput.toLocal8Bit().constData())); but anyway in this case the name of output file is not used, or I can't find any occurrence in the stylesheets. The stylesheet is applied and the name of the file is used only after to write the generated XML (see tranform() function). A similar patch can be applied to kdelibs/kdoctools too (same codepath). Diffs - src/meinproc.cpp 95adcea Diff: https://git.reviewboard.kde.org/r/116604/diff/ Testing --- Run meinproc5 (and 4) with -o /something/with/a/.dotdir/myfile.txt (the directory must exist), no error anymore and the file is generated. Thanks, Luigi Toscano
Re: Review Request 116555: Add support for pam-kwallet in kwalletd
On March 6, 2014, 7:52 p.m., Albert Astals Cid wrote: kwalletd/main.cpp, line 100 https://git.reviewboard.kde.org/r/116555/diff/2/?file=251596#file251596line100 Make this function static like the other ones? You are right, I will fix it tomorrow morning. - Àlex --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116555/#review52293 --- On March 2, 2014, 11:33 p.m., Àlex Fiestas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116555/ --- (Updated March 2, 2014, 11:33 p.m.) Review request for KDE Runtime, Release Team and Valentin Rusu. Repository: kde-runtime Description --- This patch adds support for pam-kwallet (in my scratch right now, to be released soon). This is how the new pam works, and why this patch is needed: In order to open the wallet in a secure way we have to try hard to not send the hash on an insecure manner. This is how we achieve that: -pam_kwallet creates a pipe. -pam_kwallet opens a local socket listening somewhere (/tmp/foo.socket for example). -pam_kwallet forks+execv kwallet, passing via arguments the sockets (pipe and local socket). -pam_kwallet sends the hash via the pipe. -kwalletd gets the hash and waits for the environment. -startkde uses socat to send the environment to kwalletd. -kwalletd setups the environment before any Qt code is executed. -kwalletd resumes execution. With this way of executing kwallet we get: -pam_kwallet knows to who it is sending the hash (its on child). -hash is never revealed on shared memory (dbus), since pipes are private to the apps. -ptrace is usually disabled so only root can see the hash on the app memory -no Qt code is executed without the proper environment (same as startkde) -if kwalletd is executed normally (not from pam_kwallet) then it is business as usual. The patch also comes with integration tests that simulate how kwalletd is executed in the pam module. For the release team, I would love to add this to 4.13, after all it is innocuous if kwalletd is not executed via pam_module. Diffs - kwalletd/CMakeLists.txt e9aef16 kwalletd/autotests/CMakeLists.txt PRE-CREATION kwalletd/autotests/kdewallet.kwl PRE-CREATION kwalletd/autotests/kwalletexecuter.h PRE-CREATION kwalletd/autotests/kwalletexecuter.cpp PRE-CREATION kwalletd/autotests/qtest_kwallet.h PRE-CREATION kwalletd/autotests/testpamopen.cpp PRE-CREATION kwalletd/autotests/testpamopennofile.cpp PRE-CREATION kwalletd/main.cpp f9af414 Diff: https://git.reviewboard.kde.org/r/116555/diff/ Testing --- Thanks, Àlex Fiestas
Re: Review Request 116122: Do not add trailing slash on http DELETE requests
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116122/#review52294 --- I tested your patch, no trailing slash is added whether the request refers to a collection or resource (as it was before commit 58294ac). - Andrea Iacovitti On March 5, 2014, 2:01 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116122/ --- (Updated March 5, 2014, 2:01 p.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Bugs: 331295 http://bugs.kde.org/show_bug.cgi?id=331295 Repository: kdelibs Description --- This patch fixes a prior commit, http://commits.kde.org/kdelibs/58294ac, which attempted to workaround webdav servers that do not accept delete requests on directories unless there is a trailing slash. It ensures that the fix is only applicable to webdav and the addition of the trailing slash is communicated to the client application. Diffs - kio/kio/deletejob.cpp b0268ef kioslave/http/http.cpp b4d64d4 Diff: https://git.reviewboard.kde.org/r/116122/diff/ Testing --- For HTTP delete: http://greenbytes.de/tech/tc/httpredirects/t301methods.html For WebDAV delete: http://boonfaya.com/sites/webdavapps.com/#targets Thanks, Dawit Alemayehu
Re: Review Request 116555: Add support for pam-kwallet in kwalletd
On March 6, 2014, 7:52 p.m., Albert Astals Cid wrote: I wonder if you're being too much verbose with the debugs. Since it is a beta I want to take the most of it in case any issue appears, I can remove verbosity before release if needed (think that this debug only happens once on session start) - Àlex --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116555/#review52293 --- On March 2, 2014, 11:33 p.m., Àlex Fiestas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116555/ --- (Updated March 2, 2014, 11:33 p.m.) Review request for KDE Runtime, Release Team and Valentin Rusu. Repository: kde-runtime Description --- This patch adds support for pam-kwallet (in my scratch right now, to be released soon). This is how the new pam works, and why this patch is needed: In order to open the wallet in a secure way we have to try hard to not send the hash on an insecure manner. This is how we achieve that: -pam_kwallet creates a pipe. -pam_kwallet opens a local socket listening somewhere (/tmp/foo.socket for example). -pam_kwallet forks+execv kwallet, passing via arguments the sockets (pipe and local socket). -pam_kwallet sends the hash via the pipe. -kwalletd gets the hash and waits for the environment. -startkde uses socat to send the environment to kwalletd. -kwalletd setups the environment before any Qt code is executed. -kwalletd resumes execution. With this way of executing kwallet we get: -pam_kwallet knows to who it is sending the hash (its on child). -hash is never revealed on shared memory (dbus), since pipes are private to the apps. -ptrace is usually disabled so only root can see the hash on the app memory -no Qt code is executed without the proper environment (same as startkde) -if kwalletd is executed normally (not from pam_kwallet) then it is business as usual. The patch also comes with integration tests that simulate how kwalletd is executed in the pam module. For the release team, I would love to add this to 4.13, after all it is innocuous if kwalletd is not executed via pam_module. Diffs - kwalletd/CMakeLists.txt e9aef16 kwalletd/autotests/CMakeLists.txt PRE-CREATION kwalletd/autotests/kdewallet.kwl PRE-CREATION kwalletd/autotests/kwalletexecuter.h PRE-CREATION kwalletd/autotests/kwalletexecuter.cpp PRE-CREATION kwalletd/autotests/qtest_kwallet.h PRE-CREATION kwalletd/autotests/testpamopen.cpp PRE-CREATION kwalletd/autotests/testpamopennofile.cpp PRE-CREATION kwalletd/main.cpp f9af414 Diff: https://git.reviewboard.kde.org/r/116555/diff/ Testing --- Thanks, Àlex Fiestas
Re: Review Request 116555: Add support for pam-kwallet in kwalletd
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116555/#review52298 --- For the release time, since this is really like I would like to give you my PoV on what the worse case scenarios are. -If the kwalletd is not executed using pam, then nothing will happen. This patch will do nothing if --pam-login is not passed as an argument. -If the argument is passed and the environment variable is set (which means kwalletd has been executed by the pam module) then we will attempt to read from the specified sockets the required data to open the pam. In the worse case scenario, lets imagine kwalletd crashes or something similar still the user will not notice a thing, since kwalletd will get executed again by libkwallet, and this time it won't have the --login-pam argument, so it won't be affected by this patch. So, to summary it: -It is impossible this breaks anything even if you use the pam module. - Àlex Fiestas On March 2, 2014, 11:33 p.m., Àlex Fiestas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116555/ --- (Updated March 2, 2014, 11:33 p.m.) Review request for KDE Runtime, Release Team and Valentin Rusu. Repository: kde-runtime Description --- This patch adds support for pam-kwallet (in my scratch right now, to be released soon). This is how the new pam works, and why this patch is needed: In order to open the wallet in a secure way we have to try hard to not send the hash on an insecure manner. This is how we achieve that: -pam_kwallet creates a pipe. -pam_kwallet opens a local socket listening somewhere (/tmp/foo.socket for example). -pam_kwallet forks+execv kwallet, passing via arguments the sockets (pipe and local socket). -pam_kwallet sends the hash via the pipe. -kwalletd gets the hash and waits for the environment. -startkde uses socat to send the environment to kwalletd. -kwalletd setups the environment before any Qt code is executed. -kwalletd resumes execution. With this way of executing kwallet we get: -pam_kwallet knows to who it is sending the hash (its on child). -hash is never revealed on shared memory (dbus), since pipes are private to the apps. -ptrace is usually disabled so only root can see the hash on the app memory -no Qt code is executed without the proper environment (same as startkde) -if kwalletd is executed normally (not from pam_kwallet) then it is business as usual. The patch also comes with integration tests that simulate how kwalletd is executed in the pam module. For the release team, I would love to add this to 4.13, after all it is innocuous if kwalletd is not executed via pam_module. Diffs - kwalletd/CMakeLists.txt e9aef16 kwalletd/autotests/CMakeLists.txt PRE-CREATION kwalletd/autotests/kdewallet.kwl PRE-CREATION kwalletd/autotests/kwalletexecuter.h PRE-CREATION kwalletd/autotests/kwalletexecuter.cpp PRE-CREATION kwalletd/autotests/qtest_kwallet.h PRE-CREATION kwalletd/autotests/testpamopen.cpp PRE-CREATION kwalletd/autotests/testpamopennofile.cpp PRE-CREATION kwalletd/main.cpp f9af414 Diff: https://git.reviewboard.kde.org/r/116555/diff/ Testing --- Thanks, Àlex Fiestas
Re: Review Request 116122: Do not add trailing slash on http DELETE requests
On March 6, 2014, 9:10 p.m., Andrea Iacovitti wrote: I tested your patch, no trailing slash is added whether the request refers to a collection or resource (as it was before commit 58294ac). Dawit Alemayehu wrote: Right, but now the webdav server should redirect to the right address (with trailing slash) and that redirect should succeed. I guess we can always add a trailing slash if there are servers that do not redirect or we want to avoid the redirect. Well, not mine, i get a 204 and the forlder successfully deleted. - Andrea --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116122/#review52294 --- On March 5, 2014, 2:01 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116122/ --- (Updated March 5, 2014, 2:01 p.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Bugs: 331295 http://bugs.kde.org/show_bug.cgi?id=331295 Repository: kdelibs Description --- This patch fixes a prior commit, http://commits.kde.org/kdelibs/58294ac, which attempted to workaround webdav servers that do not accept delete requests on directories unless there is a trailing slash. It ensures that the fix is only applicable to webdav and the addition of the trailing slash is communicated to the client application. Diffs - kio/kio/deletejob.cpp b0268ef kioslave/http/http.cpp b4d64d4 Diff: https://git.reviewboard.kde.org/r/116122/diff/ Testing --- For HTTP delete: http://greenbytes.de/tech/tc/httpredirects/t301methods.html For WebDAV delete: http://boonfaya.com/sites/webdavapps.com/#targets Thanks, Dawit Alemayehu
Re: Review Request 116122: Do not add trailing slash on http DELETE requests
On March 6, 2014, 9:10 p.m., Andrea Iacovitti wrote: I tested your patch, no trailing slash is added whether the request refers to a collection or resource (as it was before commit 58294ac). Dawit Alemayehu wrote: Right, but now the webdav server should redirect to the right address (with trailing slash) and that redirect should succeed. I guess we can always add a trailing slash if there are servers that do not redirect or we want to avoid the redirect. Andrea Iacovitti wrote: Well, not mine, i get a 204 and the forlder successfully deleted. Well that is fine. If you try one of the test servers I listed above, you will see that deleting directory without a trailing slash results in a 302 response that redirects you to the same directory but with the trailing slash appended. The problem before this patch was that redirection would have simply been ignored. That is because the code in DeleteJobPrivate::deleteNextSrc blindly calls KIO::rmdir even for http/webdav resources. And Since KIO::rmdir uses DeleteJob which inherits from SimpleJob, there is no support for redirection. You need TransferJob or its descendents if the protocol requires redirection support. Once we convert to properly call KIO::http_delete for HTTP based protocols, then it works as expected because we will be able to handle the redirection of the delete request. Anyhow, all I was suggesting in my prior response above for us to optimize the directory deletion requests for the servers that require trailing slash. That way we avoid the additional redirection when requesting deletion from such servers without impacting those servers that do not care about the trailing slash. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116122/#review52294 --- On March 5, 2014, 2:01 p.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116122/ --- (Updated March 5, 2014, 2:01 p.m.) Review request for kdelibs, Andrea Iacovitti and David Faure. Bugs: 331295 http://bugs.kde.org/show_bug.cgi?id=331295 Repository: kdelibs Description --- This patch fixes a prior commit, http://commits.kde.org/kdelibs/58294ac, which attempted to workaround webdav servers that do not accept delete requests on directories unless there is a trailing slash. It ensures that the fix is only applicable to webdav and the addition of the trailing slash is communicated to the client application. Diffs - kio/kio/deletejob.cpp b0268ef kioslave/http/http.cpp b4d64d4 Diff: https://git.reviewboard.kde.org/r/116122/diff/ Testing --- For HTTP delete: http://greenbytes.de/tech/tc/httpredirects/t301methods.html For WebDAV delete: http://boonfaya.com/sites/webdavapps.com/#targets Thanks, Dawit Alemayehu
Review Request 116570: Ask user for confirmation before doing POST - POST redirection in KIO
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116570/ --- Review request for kdelibs, Andrea Iacovitti and David Faure. Repository: kdelibs Description --- This patch is a companion to the recent POST-POST redirection implementation in KIO, https://git.reviewboard.kde.org/r/116017/. It prompts the user to approve the redirection as explicitly required in sections 10.3.[2|3] of RFC 2616: If the 301 status code is received in response to a request other than GET or HEAD, the user agent MUST NOT automatically redirect the request unless it can be confirmed by the user, since this might change the conditions under which the request was issued. Please note that this patch only prompts the user for confirmation on POST-POST redirections. It can be expanded to include redirections for other requests such as PUT. There is also an issue of whether this patch should be part of the 4.13 release? Since we are in a freeze and the patch has both message changes as well as a new API, I have simply marked it for inclusion in master branch, i.e. 4.14. Diffs - kio/kio/job.cpp 50b4afb kio/kio/jobuidelegate.h 17fd554 kio/kio/jobuidelegate.cpp 5aff330 Diff: https://git.reviewboard.kde.org/r/116570/diff/ Testing --- http://greenbytes.de/tech/tc/httpredirects/t307methods.html File Attachments POST redirection confirm dialog https://git.reviewboard.kde.org/media/uploaded/files/2014/03/07/e77dd03e-cb37-49bb-8554-cca991c8c546__post_redirection_confirmation.png Thanks, Dawit Alemayehu
Re: Review Request 116524: Make kio_ftp work with ftp server that don't support absolute path with SIZE command
On March 5, 2014, 7:47 a.m., David Faure wrote: kioslave/ftp/ftp.cpp, line 2275 https://git.reviewboard.kde.org/r/116524/diff/2/?file=251597#file251597line2275 This is surely wrong. If you're in /home/dfaure (as the CWD would be, by default, on non-anonymous FTP) and you're downloading a file /home/dfaure/dir1/dir2/file.txt then surely you want to call SIZE dir1/dir2/file.txt and not SIZE home/dfaure/dir1/dir2/file.txt It has to be relative to the CWD, not just skip the first slash, which only works if the CWD is /. This gives two alternatives for the fix: make this code relative to the current CWD whatever it is, or call ftpFolder() with the directory name (e.g. /home/dfaure/dir1/dir2) followed by SIZE with just the filename. The latter sounds like it might work better on android (if it doesn't support absolute paths, maybe it doesn't support ../../foo/bar.txt either?). I have not been able to test whether it supported ../../foo/bar.txt yet. However, making the code relative to m_currentPath seems to work just fine ; so I can avoid having to call ftpFolder and hence sending a cwd request. - Dawit --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/#review52013 --- On March 3, 2014, 12:16 a.m., Dawit Alemayehu wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116524/ --- (Updated March 3, 2014, 12:16 a.m.) Review request for kdelibs and David Faure. Bugs: 168011 and 326292 http://bugs.kde.org/show_bug.cgi?id=168011 http://bugs.kde.org/show_bug.cgi?id=326292 Repository: kdelibs Description --- This patch changes Ftp::ftpSize such that it has support for servers that do not allow absolute paths with the SIZE command. That means when sending the command SIZE /somefile fails, it will try sending SIZE somefile before giving up. See bug report for details. Diffs - kioslave/ftp/ftp.cpp 5bb2e8d Diff: https://git.reviewboard.kde.org/r/116524/diff/ Testing --- Installed Ftp server from bug report on an Android device and run tests. Thanks, Dawit Alemayehu
