D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
chinmoyr reopened this revision. This revision is now accepted and ready to land. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6709 To: chinmoyr, thiago, #frameworks, dfaure Cc: dfaure, davidedmundson, elvisangelaccio, shortstheory
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
This revision was automatically updated to reflect the committed changes. Closed by commit R241:0c2a6811dc50: Add support for sharing file descriptor between file ioslave and it's KAuth… (authored by chinmoyr). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D6709?vs=20232&id=20607#toc REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6709?vs=20232&id=20607 REVISION DETAIL https://phabricator.kde.org/D6709 AFFECTED FILES src/ioslaves/file/CMakeLists.txt src/ioslaves/file/fdreceiver.cpp src/ioslaves/file/fdreceiver.h src/ioslaves/file/kauth/fdsender.cpp src/ioslaves/file/kauth/fdsender.h src/ioslaves/file/sharefd_p.h To: chinmoyr, thiago, #frameworks, dfaure Cc: dfaure, davidedmundson, elvisangelaccio, shortstheory
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
chinmoyr added a comment. In https://phabricator.kde.org/D6709#153057, @dfaure wrote: > This is all ready to push now, right? > > Please do, once the 5.39 RC tags are there (should happen today or tomorrow). This patch is. I have yet to add the new flag. BRANCH master REVISION DETAIL https://phabricator.kde.org/D6709 To: chinmoyr, thiago, #frameworks, dfaure Cc: dfaure, davidedmundson, elvisangelaccio, shortstheory
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
dfaure accepted this revision. dfaure added a comment. This is all ready to push now, right? Please do, once the 5.39 RC tags are there (should happen today or tomorrow). BRANCH master REVISION DETAIL https://phabricator.kde.org/D6709 To: chinmoyr, thiago, #frameworks, dfaure Cc: dfaure, davidedmundson, elvisangelaccio, shortstheory
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
chinmoyr updated this revision to Diff 20232. chinmoyr added a comment. Changed isValid() -> isListening() in FdReceiver CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6709?vs=19758&id=20232 BRANCH master REVISION DETAIL https://phabricator.kde.org/D6709 AFFECTED FILES src/ioslaves/file/CMakeLists.txt src/ioslaves/file/fdreceiver.cpp src/ioslaves/file/fdreceiver.h src/ioslaves/file/kauth/fdsender.cpp src/ioslaves/file/kauth/fdsender.h src/ioslaves/file/sharefd_p.h To: chinmoyr, thiago, #frameworks, dfaure Cc: dfaure, davidedmundson, elvisangelaccio, shortstheory
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. Yes `isListening()` would make sense. The kauth subdir for fdsender.* is ok, it matches what you then do in https://phabricator.kde.org/D6197 where you actually use that file (well, you'll have to update that commit to change the filename in CMakeLists.txt) It doesn't look like Thiago will have time to re-review this any time soon, so I'd say, let's get this in meanwhile. BRANCH sharefd REVISION DETAIL https://phabricator.kde.org/D6709 To: chinmoyr, thiago, #frameworks, dfaure Cc: dfaure, davidedmundson, elvisangelaccio, shortstheory
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
chinmoyr updated this revision to Diff 19758. chinmoyr added a comment. Added license header CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6709?vs=19757&id=19758 BRANCH sharefd REVISION DETAIL https://phabricator.kde.org/D6709 AFFECTED FILES src/ioslaves/file/CMakeLists.txt src/ioslaves/file/fdreceiver.cpp src/ioslaves/file/fdreceiver.h src/ioslaves/file/kauth/fdsender.cpp src/ioslaves/file/kauth/fdsender.h src/ioslaves/file/sharefd_p.h To: chinmoyr, thiago, #frameworks, dfaure Cc: dfaure, davidedmundson, elvisangelaccio, shortstheory
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
chinmoyr updated this revision to Diff 19757. chinmoyr marked an inline comment as done. chinmoyr added a comment. - separated FdReceiver and FdSender - KMSgHdr -> FDMessageHeader and KSockaddrUn -> SocketAddress and moved them to private header - removed `stopListening()` method. Qt can handle `m_readNotifier`'s deletion Question : Is it strange that kauth directory appears in this commit? And shall I rename `isValid()` in `FdReceiver` to `isListening()`? CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6709?vs=18502&id=19757 BRANCH sharefd REVISION DETAIL https://phabricator.kde.org/D6709 AFFECTED FILES src/ioslaves/file/CMakeLists.txt src/ioslaves/file/fdreceiver.cpp src/ioslaves/file/fdreceiver.h src/ioslaves/file/kauth/fdsender.cpp src/ioslaves/file/kauth/fdsender.h src/ioslaves/file/sharefd_p.h To: chinmoyr, thiago, #frameworks, dfaure Cc: dfaure, davidedmundson, elvisangelaccio, shortstheory
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> sharefd.cpp:32
> +
> +//borrowed from klocalsocket.cpp
> +class KSockaddrUn
Either move it to a common header (if QString everywhere is OK), or use a
different classname here, to avoid a classname clash at runtime (and document
that it was ported to std::string, so we know why it was forked)
> sharefd.cpp:67
> +
> +class KMsgHdr
> +{
Can you make the classname more descriptive? ShareFDMessageHeader maybe?
>From the looks of it I initially thought this came from kio and not just from
>sharefd.*
> sharefd.cpp:95
> +
> +// File descriptor reciever
> +FdReceiver::FdReceiver(const QString &path, QObject *parent)
typo: receiver
> sharefd.cpp:147
> +{
> +if (m_readNotifier) {
> +delete m_readNotifier;
this if() serves no purpose (delete nullptr is a well-defined no-op)
> sharefd.cpp:159
> +
> +// File descriptor sender
> +FdSender::FdSender(const std::string &path)
If FDSender is only used in the kauthhelper, and FDReceiver is only used in
kio_file, why not split up this .cpp into two, and only compile the bit that's
needed by each target?
(you'll need a private header for the shared classes of course, i.e. the
sockaddr wrapper and the messageheader would go into sharefd_p.h)
It would also make the QString vs std::string API difference look less weird.
And then the files can match the classnames, too (fdsender.cpp/h and
fdreceiver.cpp/h)
> sharefd.cpp:166
> +if (m_socketDes == -1) {
> +std::cerr << "socket error:" << strerror(errno);
> +return;
you need std::endl when using std::cerr (repeats)
REVISION DETAIL
https://phabricator.kde.org/D6709
To: chinmoyr, thiago, #frameworks, dfaure
Cc: dfaure, davidedmundson, elvisangelaccio, shortstheory
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
chinmoyr updated this revision to Diff 18502. chinmoyr added a comment. - Made changes that were suggested here : https://codereview.stackexchange.com/questions/173306/sharing-file-descriptor-between-an-under-privileged-and-a-privileged-process-us CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6709?vs=18298&id=18502 BRANCH master REVISION DETAIL https://phabricator.kde.org/D6709 AFFECTED FILES src/ioslaves/file/CMakeLists.txt src/ioslaves/file/sharefd.cpp src/ioslaves/file/sharefd.h To: chinmoyr, thiago, #frameworks Cc: davidedmundson, elvisangelaccio, shortstheory
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
chinmoyr updated this revision to Diff 18298. chinmoyr added a comment. - add stopListening method to FdReceiver CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6709?vs=17012&id=18298 BRANCH master REVISION DETAIL https://phabricator.kde.org/D6709 AFFECTED FILES src/ioslaves/file/CMakeLists.txt src/ioslaves/file/sharefd.cpp src/ioslaves/file/sharefd.h To: chinmoyr, thiago, #frameworks Cc: davidedmundson, elvisangelaccio, shortstheory
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
chinmoyr added a comment. ping @thiago REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6709 To: chinmoyr, thiago, #frameworks Cc: davidedmundson, elvisangelaccio, shortstheory
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
chinmoyr added a comment. @thiago Used FdSender in the function senndFileDescriptor in https://phabricator.kde.org/D6197 [filehelper.cpp] and FdReceiver in https://phabricator.kde.org/D6830 [file_unix.cpp] REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6709 To: chinmoyr, thiago, #frameworks Cc: davidedmundson, elvisangelaccio, shortstheory
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
chinmoyr added a dependent revision: D6197: Add kauth helper to file ioslave. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6709 To: chinmoyr, thiago, #frameworks Cc: davidedmundson, elvisangelaccio, shortstheory
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
chinmoyr removed a task: T6561: Polkit support in KIO. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6709 To: chinmoyr, thiago, #frameworks Cc: davidedmundson, elvisangelaccio, shortstheory
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
chinmoyr added a task: T6561: Polkit support in KIO. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6709 To: chinmoyr, thiago, #frameworks Cc: davidedmundson, elvisangelaccio, shortstheory
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
chinmoyr added a task: T6561: Polkit support in KIO. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6709 To: chinmoyr, thiago, #frameworks Cc: davidedmundson, elvisangelaccio, shortstheory
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
chinmoyr removed a task: T6561: Polkit support in KIO. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6709 To: chinmoyr, thiago, #frameworks Cc: davidedmundson, elvisangelaccio, shortstheory
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
chinmoyr added a task: T6561: Polkit support in KIO. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6709 To: chinmoyr, thiago, #frameworks Cc: davidedmundson, elvisangelaccio, shortstheory
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
chinmoyr updated this revision to Diff 17012. chinmoyr marked 5 inline comments as done. chinmoyr added a comment. std::string -> QString binded -> bound setNonBlock() - > SOCK_NONBLOCK REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D6709?vs=16732&id=17012 REVISION DETAIL https://phabricator.kde.org/D6709 AFFECTED FILES src/ioslaves/file/CMakeLists.txt src/ioslaves/file/sharefd.cpp src/ioslaves/file/sharefd.h To: chinmoyr, thiago, #frameworks Cc: davidedmundson, elvisangelaccio, shortstheory
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
elvisangelaccio added a comment. In https://phabricator.kde.org/D6709#125694, @thiago wrote: > This class isn't hooked up to anything. It's technically correct as an FD sender and receiver. What I want to see is how you use it, because that's extremely important to get right. I think he's going to upload another patch that uses this code. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6709 To: chinmoyr, thiago, #frameworks Cc: davidedmundson, elvisangelaccio, shortstheory
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
thiago added a comment. This class isn't hooked up to anything. It's technically correct as an FD sender and receiver. What I want to see is how you use it, because that's extremely important to get right. I can confirm to you that anonymous namespace sockets do not work on BSDs (which include macOS). INLINE COMMENTS > sharefd.cpp:112 > + > +m_socketDes = ::socket(AF_LOCAL, SOCK_STREAM, 0); > +if (m_socketDes == -1) If SOCK_NONBLOCK is defined, OR it o SOCK_STREAM and then you don't have call setNonBlocking below. > sharefd.cpp:117 > +KSockaddrUn addr(path); > +bool binded = bind(m_socketDes, addr.address(), addr.length()) != -1; > +bool listening = listen(m_socketDes, 5) != -1; "binded" is not English. You mean "bound". > sharefd.h:33 > + > +bool startListening(const std::string &path); > +int fileDescriptor() const; Use QString, not std::string. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6709 To: chinmoyr, thiago, #frameworks Cc: davidedmundson, elvisangelaccio, shortstheory
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
chinmoyr marked an inline comment as done. chinmoyr added inline comments. INLINE COMMENTS > davidedmundson wrote in sharefd.cpp:107 > If this is on the client side, what stops any other (non authorised) client > listening to here? It doesn't matter. The job of client is to send the file descriptor and any other (unauthorised) client connected cannot see this fd. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6709 To: chinmoyr, thiago, #frameworks Cc: davidedmundson, elvisangelaccio, shortstheory
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
chinmoyr added a comment. And In https://phabricator.kde.org/D6709#125610, @davidedmundson wrote: > > The sequence would be, registering service in ioslave, setting euid of the helper process and sending the file descriptor over user's session bus > > I don't fully know this code, but that doesn't sound right. > > Your helper is running on the system bus, and shouldn't really have access to the user's session bus at all. > Your client side ioslave will have connections to both busses, but only be talking to the helper on the system bus. You can set the effective uid of the root process to the users process to connect to user's session bus and then reset it and gain back the privilege. > With polkit you currently request something over DBus from a helper and you get a reply back. What you're describing you want is exactly that, except the reply happens a file descriptor. (which as you hint is a native DBus type) Why do you need to register a service in the ioslave? QDBusUnixFileDescriptor object is required to send a file descriptor over dbus. The service must have a method accepting QDBusUnixFileDescriptor as an argument, QtDBus won't create it automatically. KAuth doesn't have any such method thats why the need for a separate service. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6709 To: chinmoyr, thiago, #frameworks Cc: davidedmundson, elvisangelaccio, shortstheory
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
davidedmundson added a comment.
> The sequence would be, registering service in ioslave, setting euid of the
helper process and sending the file descriptor over user's session bus
I don't fully know this code, but that doesn't sound right.
Your helper is running on the system bus, and shouldn't really have access to
the user's session bus at all.
Your client side ioslave will have connections to both busses, but only be
talking to the helper on the system bus.
With polkit you currently request something over DBus from a helper and you
get a reply back. What you're describing you want is exactly that, except the
reply happens a file descriptor. (which as you hint is a native DBus type) Why
do you need to register a service in the ioslave?
INLINE COMMENTS
> sharefd.cpp:107
> +
> +bool FdReceiver::startListening(const std::string &path)
> +{
If this is on the client side, what stops any other (non authorised) client
listening to here?
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D6709
To: chinmoyr, thiago, #frameworks
Cc: davidedmundson, elvisangelaccio, shortstheory
D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper
chinmoyr created this revision. Restricted Application added a project: Frameworks. REVISION SUMMARY Some methods in file ioslave, `FileProtocol::copy` and FileProtocol::put to be precise, use file descriptor of source and destination files. So performing any these operations as root user using kauth's helper requires the source or destination file to be opened inside the helper and sending the file descriptor back to ioslave using a suitable IPC mechanism. My patch does the task using unix local domain socket. In principal dbus can also be used. The sequence would be, registering service in ioslave, setting `euid` of the helper process and sending the file descriptor over user's session bus. I tried it but the code turned out messy. In the end it was somewhat a personal preference. There are certain things I would like to know regarding this patch, In this patch I used the abstract namespace. It will work with linux but I don't know about mac os or bsd. So what to use for them? In place of unix sockets, dbus can also be used. So shall i use it ? I am aware there are security issues with both the approaches but using which one of them is less riskier? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D6709 AFFECTED FILES src/ioslaves/file/CMakeLists.txt src/ioslaves/file/sharefd.cpp src/ioslaves/file/sharefd.h To: chinmoyr, thiago, #frameworks Cc: elvisangelaccio, shortstheory
