D24245: Add support for passing Unix file descriptors

2019-10-07 Thread Albert Astals Cid 
aacid added a comment.


  ok, fair enough.
  
  Am i correct in understanding this breaks the wire-protocol for the dbus 
messages?
  
  Are we sure that's fine?

REPOSITORY
  R283 KAuth

REVISION DETAIL
  https://phabricator.kde.org/D24245

To: volkov, fvogt, chinmoyr, cfeck, #frameworks, security-team
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24245: Add support for passing Unix file descriptors

2019-10-01 Thread Alexander Volkov 
volkov added a comment.


  It will require adding the following methods:
  
Action::setFileDescriptors()
Action::addFileDescriptor()
Action::fileDescriptors()
ActionReply::setFileDescriptors()
ActionReply::addFileDescriptor()
ActionReply::fileDescriptors()
  
  and
  
ExecuteJob::fileDescriptors()
ExecuteJob::newFileDescriptors()
  
  with the condition that for progress data they can be used only with direct 
signal-slot connections,
  because without `UnixFileDescriptor` we cannot guarantee the open state of 
file descriptors.
  It doesn't seem better than exposing this class.

REPOSITORY
  R283 KAuth

REVISION DETAIL
  https://phabricator.kde.org/D24245

To: volkov, fvogt, chinmoyr, cfeck, #frameworks, security-team
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24245: Add support for passing Unix file descriptors

2019-09-30 Thread Albert Astals Cid 
aacid added a comment.


  In D24245#539508 , @volkov wrote:
  
  > Without UnixFileDescriptor you could try to write
  >
  >   int fd = ...
  >   Action action(...);
  >   action.addArgument(QStringLiteral("fd"), fd);
  >
  >
  > but then KAuth won't be able to detect that it'a file descriptor and will 
pass it to a helper as int.
  
  
  Would it make more sense to add a new function
  
  Action::addFileDescriptor(const QString &, int)?
  
  This way we don't have to expose UnixFileDescriptor to the world

REPOSITORY
  R283 KAuth

REVISION DETAIL
  https://phabricator.kde.org/D24245

To: volkov, fvogt, chinmoyr, cfeck, #frameworks, security-team
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24245: Add support for passing Unix file descriptors

2019-09-29 Thread Alexander Volkov 
volkov added a comment.


  Without UnixFileDescriptor you could try to write
  
int fd = ...
Action action(...);
action.addArgument(QStringLiteral("fd"), fd);
  
  but then KAuth won't be able to detect that it'a file descriptor and will 
pass it to a helper as int.

REPOSITORY
  R283 KAuth

REVISION DETAIL
  https://phabricator.kde.org/D24245

To: volkov, fvogt, chinmoyr, cfeck, #frameworks, security-team
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24245: Add support for passing Unix file descriptors

2019-09-29 Thread Alexander Volkov 
volkov added a comment.


  Yes, UnixFileDescriptor allows to use existing methods Action::setArguments() 
and ActionReply::setData().

REPOSITORY
  R283 KAuth

REVISION DETAIL
  https://phabricator.kde.org/D24245

To: volkov, fvogt, chinmoyr, cfeck, #frameworks, security-team
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24245: Add support for passing Unix file descriptors

2019-09-29 Thread Albert Astals Cid 
aacid added a comment.


  Applications are supposed to do that?

REPOSITORY
  R283 KAuth

REVISION DETAIL
  https://phabricator.kde.org/D24245

To: volkov, fvogt, chinmoyr, cfeck, #frameworks, security-team
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24245: Add support for passing Unix file descriptors

2019-09-28 Thread Alexander Volkov 
volkov added inline comments.

INLINE COMMENTS

> aacid wrote in kauthunixfiledescriptor.h:33
> Does this class really need to be exported and its header installed?
> 
> Seems like this is implementation detail? Who would use this header?

It's needed to mark file descriptors in QVariantMap.
Usage example:

  ActionReply reply;
  QVariantMap data;
  auto variantFd = QVariant::fromValue(UnixFileDescriptor(saveFile.handle()));
  data.insert(QStringLiteral("fd"), variantFd);
  reply.setData(data);
  return reply;

REPOSITORY
  R283 KAuth

REVISION DETAIL
  https://phabricator.kde.org/D24245

To: volkov, fvogt, chinmoyr, cfeck, #frameworks, security-team
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24245: Add support for passing Unix file descriptors

2019-09-27 Thread Albert Astals Cid 
aacid added inline comments.

INLINE COMMENTS

> kauthunixfiledescriptor.h:33
> +
> +class KAUTH_EXPORT UnixFileDescriptor
> +{

Does this class really need to be exported and its header installed?

Seems like this is implementation detail? Who would use this header?

REPOSITORY
  R283 KAuth

REVISION DETAIL
  https://phabricator.kde.org/D24245

To: volkov, fvogt, chinmoyr, cfeck, #frameworks, security-team
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24245: Add support for passing Unix file descriptors

2019-09-27 Thread Alexander Volkov 
volkov edited the summary of this revision.

REPOSITORY
  R283 KAuth

REVISION DETAIL
  https://phabricator.kde.org/D24245

To: volkov, fvogt, chinmoyr, cfeck, #frameworks, security-team
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24245: Add support for passing Unix file descriptors

2019-09-27 Thread Alexander Volkov 
volkov updated this revision to Diff 66968.
volkov added a comment.


  moved dbus stuff to DBusHelperProxy

REPOSITORY
  R283 KAuth

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24245?vs=66946&id=66968

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D24245

AFFECTED FILES
  autotests/CMakeLists.txt
  src/CMakeLists.txt
  src/HelperProxy.h
  src/backends/dbus/DBusHelperProxy.cpp
  src/backends/dbus/DBusHelperProxy.h
  src/backends/dbus/org.kde.kf5auth.xml
  src/backends/fakehelper/FakeHelperProxy.cpp
  src/backends/fakehelper/FakeHelperProxy.h
  src/kauth.h
  src/kauthunixfiledescriptor.cpp
  src/kauthunixfiledescriptor.h

To: volkov, fvogt, chinmoyr, cfeck, #frameworks, security-team
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24245: Add support for passing Unix file descriptors

2019-09-27 Thread Alexander Volkov 
volkov added inline comments.

INLINE COMMENTS

> aacid wrote in HelperProxy.cpp:25
> i don't think we should have dbus stuff in HelperProxy, dbus stuff should be 
> in DBusHelperProxy, no?

yes, thanks

REPOSITORY
  R283 KAuth

REVISION DETAIL
  https://phabricator.kde.org/D24245

To: volkov, fvogt, chinmoyr, cfeck, #frameworks, security-team
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24245: Add support for passing Unix file descriptors

2019-09-27 Thread Alexander Volkov 
volkov added a comment.


  yes, thanks

REPOSITORY
  R283 KAuth

REVISION DETAIL
  https://phabricator.kde.org/D24245

To: volkov, fvogt, chinmoyr, cfeck, #frameworks, security-team
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24245: Add support for passing Unix file descriptors

2019-09-27 Thread Albert Astals Cid 
aacid added inline comments.

INLINE COMMENTS

> HelperProxy.cpp:25
> +
> +#include 
> +#include 

i don't think we should have dbus stuff in HelperProxy, dbus stuff should be in 
DBusHelperProxy, no?

REPOSITORY
  R283 KAuth

REVISION DETAIL
  https://phabricator.kde.org/D24245

To: volkov, fvogt, chinmoyr, cfeck, #frameworks, security-team
Cc: aacid, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24245: Add support for passing Unix file descriptors

2019-09-27 Thread Alexander Volkov 
volkov added a dependent revision: D24259: WIP: Save file directly if possible 
when root privileges are required.

REPOSITORY
  R283 KAuth

REVISION DETAIL
  https://phabricator.kde.org/D24245

To: volkov, fvogt, chinmoyr, cfeck, #frameworks, security-team
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24245: Add support for passing Unix file descriptors

2019-09-27 Thread Alexander Volkov 
volkov updated this revision to Diff 66946.
volkov added a comment.


  fix breaking API and ABI of KAuth::ActionReply

REPOSITORY
  R283 KAuth

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24245?vs=66905&id=66946

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D24245

AFFECTED FILES
  autotests/CMakeLists.txt
  src/CMakeLists.txt
  src/HelperProxy.cpp
  src/HelperProxy.h
  src/backends/dbus/DBusHelperProxy.cpp
  src/backends/dbus/DBusHelperProxy.h
  src/backends/dbus/org.kde.kf5auth.xml
  src/backends/fakehelper/FakeHelperProxy.cpp
  src/backends/fakehelper/FakeHelperProxy.h
  src/kauth.h
  src/kauthunixfiledescriptor.cpp
  src/kauthunixfiledescriptor.h

To: volkov, fvogt, chinmoyr, cfeck, #frameworks, security-team
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24245: Add support for passing Unix file descriptors

2019-09-26 Thread Alexander Volkov 
volkov added a comment.


  DBus is an internal dependency for KAuth. With the wrapper users don't have 
to link with QtDBus.

REPOSITORY
  R283 KAuth

REVISION DETAIL
  https://phabricator.kde.org/D24245

To: volkov, fvogt, chinmoyr, cfeck, #frameworks, security-team
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24245: Add support for passing Unix file descriptors

2019-09-26 Thread Albert Astals Cid 
aacid added a reviewer: security-team.

REPOSITORY
  R283 KAuth

REVISION DETAIL
  https://phabricator.kde.org/D24245

To: volkov, fvogt, chinmoyr, cfeck, #frameworks, security-team
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24245: Add support for passing Unix file descriptors

2019-09-26 Thread Fabian Vogt 
fvogt requested changes to this revision.
fvogt added a comment.
This revision now requires changes to proceed.


  Unfortunately, this breaks public API and ABI by modifying KAuth::ActionReply.
  
  I'm not sure whether there has to be any compatibility for the DBus API, but 
I guess not (logging out and back in should suffice to make sure the backend 
used is the same).
  
  I wonder why you made a wrapper around `QDBusUnixFileDescriptor`, does it not 
work inside a `QVariant` OOTB?

REPOSITORY
  R283 KAuth

REVISION DETAIL
  https://phabricator.kde.org/D24245

To: volkov, fvogt, chinmoyr, cfeck, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24245: Add support for passing Unix file descriptors

2019-09-26 Thread Nathaniel Graham 
ngraham added reviewers: fvogt, chinmoyr, cfeck, Frameworks.

REPOSITORY
  R283 KAuth

REVISION DETAIL
  https://phabricator.kde.org/D24245

To: volkov, fvogt, chinmoyr, cfeck, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24245: Add support for passing Unix file descriptors

2019-09-26 Thread Alexander Volkov 
volkov created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
volkov requested review of this revision.

REVISION SUMMARY
  Introduce UnixFileDescriptor class, which holds a copy of a file
  descriptor. Actually it's a wrapper around QDBusUnixFileDescriptor.
  
  Currently arguments to/from a helper are passed as QVariantMap,
  encoded as QByteArray. To support passing of file descriptors,
  separate them into their own QVariantMap, and pass arguments as
  QVariantList consisting of the QByteArray blob of data arguments
  and the file descriptors map (HelperProxy::{un}packedArguments()).
  
  Remove operator<<(QDataStream &, const ActionReply &) and
  operator>>(QDataStream &, ActionReply &) as they don't make
  sense with file descriptors and used only internally in
  ActionReply::serialized() and ActionReply::deserialize()
  methods. Modify these methods to work with QVariantList
  instead of QByteArray blobs.

REPOSITORY
  R283 KAuth

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D24245

AFFECTED FILES
  autotests/CMakeLists.txt
  src/CMakeLists.txt
  src/HelperProxy.cpp
  src/HelperProxy.h
  src/backends/dbus/DBusHelperProxy.cpp
  src/backends/dbus/DBusHelperProxy.h
  src/backends/dbus/org.kde.kf5auth.xml
  src/backends/fakehelper/FakeHelperProxy.cpp
  src/backends/fakehelper/FakeHelperProxy.h
  src/kauth.h
  src/kauthactionreply.cpp
  src/kauthactionreply.h
  src/kauthunixfiledescriptor.cpp
  src/kauthunixfiledescriptor.h

To: volkov
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns