Merge lp:~robertcarr/mir/implement-client-credentials into lp:~mir-team/mir/trunk

[Robert Carr]

Implement a connection authorization mechanism

Implement a connection authorization mechanism for launching applications from the shell. The shell will fork child processes (or later learn the pid through upstart), and wishes to verify that only such processes can connect.

Acceptance test should explain things well.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:776
http://jenkins.qa.ubuntu.com/job/mir-ci/848/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1075/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/960/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/86/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/502

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/848/rebuild

review: Needs Fixing (continuous-integration)

[Robert Ancell (robert-ancell)]

It doesn't make sense that the client PID is in the protocol - the client doesn't volunteer the PID, it is taken from getsockopt ().

430 message ConnectParameters {
431 required string application_name = 1;
432 + optional int32 client_pid = 2;
433 }

Will the whole server go down if one client is not allowed? This shouldn't be an exception right?

358 + if (!session_authorizer->connection_is_allowed(request->client_pid(), request->application_name()))
359 + {
360 + std::string error_message = "Client with pid " + request->client_pid();
361 + error_message += " is not authorized to connect as ";
362 + error_message += request->application_name();
363 +
364 + BOOST_THROW_EXCEPTION(std::runtime_error(error_message));
365 + }

review: Needs Information

[Daniel van Vugt (vanvugt)]

I think Robert Ancell is right. And assuming that's correct, there's no security or logical benefit to passing the PID in the protocol because it can be spoofed. If you can get the PID from the socket that would be more secure, less redundant and simpler.

Perhaps a security expert would have further suggestions...

review: Needs Fixing

[Robert Carr (robertcarr)]

Whoops the acceptance test was missed (bzr add).

Of course the pid is retreived from the socket. The interface of mf::SessionMediator however is generated by protoc and it's difficult to modify without encoding messages in the IPC level. So, we use the same pattern we use for FDs, encode them in the protobuf as optional fields, and have the message processor fill them in using out of band data.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:777
http://jenkins.qa.ubuntu.com/job/mir-ci/858/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1091
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/976/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/96
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/96/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/512

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/858/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:778
http://jenkins.qa.ubuntu.com/job/mir-ci/861/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1095
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/980/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/99
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/99/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/515

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/861/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:779
http://jenkins.qa.ubuntu.com/job/mir-ci/863/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1097
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/982
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/101
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/101/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/517

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/863/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

I have no opinion till further notice...

review: Abstain

[Robert Carr (robertcarr)]

>> Will the whole server go down if one client is not allowed? This shouldn't be an exception right?

The exception is caught to be delivered to the client.

[Robert Ancell (robert-ancell)]

The fd does make sense to be in the .proto because it is set in the message, it just happens to not be encoded with protocol buffers. However the client never sets the pid, it is implicit so it should not be represented in the protocol.

You should pass the PID (and any other required information) in the callback, i.e.:

286 + (display_server.get()->*function)(
287 + 0,
288 + &parameter_message,
289 + &result_message,
290 + callback.get());

This seems like a bad use of an exception but I won't block on it.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

66 + virtual ~SessionAuthorizer() {}

This is a new interface, make destructor nothrow ("= default;")

~~~~

310 + template<class ResultMessage>
311 + void invoke(
312 + void (protobuf::DisplayServer::*function)(
313 + ::google::protobuf::RpcController* controller,
314 + const ::mir::protobuf::ConnectParameters* request,
315 + ResultMessage* response,
316 + ::google::protobuf::Closure* done),
317 + mir::protobuf::wire::Invocation const& invocation);

No need for this to be a template - the exact return type (and even the exact function) is known.

~~~~

Somehow this approach seems inelegant: adding a field to the protocol that isn't passed over the wire and then specializing invoke so that we can get the client PID and fill it in on the server side and then handling it in SessionMediator. SessionMediator should really concentrate on translating requests from and to the wire, not on managing connections.

IMO we should refuse the connection earlier (before reading the connect message) - in ProtobufSocketCommunicator::on_new_connection(). That ought to be simpler and avoid throwing exceptions to tear down a connection that we can drop immediately without even registering it with connected_sessions.

review: Needs Fixing

[Robert Carr (robertcarr)]

The shell wishes to authenticate based on:

(desktop-file, pid), so it's necessary to have read the connect message from the socket containing the session name.

[Gerry Boland (gerboland)]

Just to clarify what shell wants:
- shell launches an application and takes note of the app's PID. Shell considers app to be in "starting" state.
- when application connects to Mir, want shell to be notified that a client with PID x has connected. Shell then checks if that PID matches any it has launched, and if yes considers that app to be in "running" state

So shell just needs to know the PID of each new clients connecting to Mir.

Right now I don't see why shell would want to reject a client's connection. Have we use-cases for that? I don't think that PID and desktop-file is enough information for it to make such a decision anyway.

[Alan Griffiths (alan-griffiths)]

It sounds like we should seek agreement on what the requirement is - this proposal appears to be a facility to reject connections.

[Kevin DuBois (kdub)]

> IMO we should refuse the connection earlier (before reading the connect
> message) - in ProtobufSocketCommunicator::on_new_connection(). That ought to
> be simpler and avoid throwing exceptions to tear down a connection that we can
> drop immediately without even registering it with connected_sessions.

seems like an appropriate place to reject the connection

[Alan Griffiths (alan-griffiths)]

Setting to WIP pending anything changing

[Gerry Boland (gerboland)]

> It sounds like we should seek agreement on what the requirement is - this
> proposal appears to be a facility to reject connections.

I've since been told shell must have total control, so the ability to reject connections is required.

The approach Robert is taking here will suit us.

[Robert Carr (robertcarr)]

I've moved the usage of the authorizer to the Communicator which I hope should satisfy everyones concerns.

The client library wasn't prepared to handle errors when sending connect messages, so I've had to make some changes to the socket RPC channel.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:781
http://jenkins.qa.ubuntu.com/job/mir-ci/947/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1221/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1105/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/185/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/588

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/947/rebuild

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

My larger concerns are addressed. But a few smaller ones:

350 - std::shared_ptr<mfd::SocketSession> const& session,
351 + std::shared_ptr<mfd::SocketSession>& session,
...
393 - void on_new_connection(const std::shared_ptr<detail::SocketSession>& session, const boost::system::error_code& ec);
394 + void on_new_connection(std::shared_ptr<detail::SocketSession>& session, const boost::system::error_code& ec);

we don't need non-const access to session.

~~~~

300 + template<class ResultMessage>
301 + void invoke(
302 + void (protobuf::DisplayServer::*function)(
303 + ::google::protobuf::RpcController* controller,
304 + const ::mir::protobuf::ConnectParameters* request,
305 + ResultMessage* response,
306 + ::google::protobuf::Closure* done),
307 + mir::protobuf::wire::Invocation const& invocation);

No need for this to be a template - the exact return type (and even the exact function) is known.

~~~~

362 + session->read_next_message();
363 connected_sessions->add(session);
364
365 - session->read_next_message();

I don't believe it makes a difference, but do we need this ordering changed?

~~~~

134 +
135 +

Do we need this whitespace?

review: Needs Fixing

[Gerry Boland (gerboland)]

This isn't quite what I expected.

Walking through app launch process: shell launches an app, and knows the app's PID. When app connects to Mir, this authorizer will check the app's PID with shell. Shell knows the info of the application with that PID, so can decide to accept or reject.

However after that, how can shell make a connection between the generated Session and the application list it has internally? PID isn't attached to Session, Session only has a "name" property to identify it uniquely. But what sets "name"?

I had expected that shell would set the name that Mir attaches to the Session.

review: Needs Fixing

[Robert Carr (robertcarr)]

Alan: Issues addressed, mostly leftovers from the first version of the branch (i.e. ::invoke variant is just deleted).

Gerry: I'm having some difficulty figuring out how this session name setting bit should work. So I think in the mean time, we can progress with this, and the limitation that the app only allows one app to be in the 'launching' state at a time (to prevent authentication races).

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:783
http://jenkins.qa.ubuntu.com/job/mir-ci/953/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1232
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1116
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/191/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/593

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/953/rebuild

review: Needs Fixing (continuous-integration)

[Robert Ancell (robert-ancell)]

Still has changes to the .proto, but they're not required:

+ optional int32 client_pid = 2;

[Robert Ancell (robert-ancell)]

In tests/mir_test_framework/socket_detect_server.cpp you've quoted out some code (which was marked as potentially not required) - this code should be deleted if it's not required right?

Otherwise merge looks good to me.

[Alan Griffiths (alan-griffiths)]

272 + if (!session_authorizer->connection_is_allowed(session->client_pid()))
273 + {
274 + start_accept();
275 + return;
276 + }
277 +
278 connected_sessions->add(session);
279 -
280 session->read_next_message();
281 }
282 start_accept();

        if (session_authorizer->connection_is_allowed(session->client_pid()))
        {
            connected_sessions->add(session);
            session->read_next_message();
        }
    }
    start_accept();

~~~~

368 -
369 +
...
377 -
378 +

Introduces trailing whitespace

~~~~

392 + optional int32 client_pid = 2;

Not used

~~~~

712 - struct sockaddr_un remote;
713 +/* struct sockaddr_un remote;
...
721 - close(sockfd);
722 + close(sockfd);*/

Not suitable for trunk

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

I'm seeing a test failure with this branch:

$ bin/unit-tests --gtest_filter=ProtobufCommunicator.double_disconnection_attempt_has_no_effect
Note: Google Test filter = ProtobufCommunicator.double_disconnection_attempt_has_no_effect
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from ProtobufCommunicator
[ RUN ] ProtobufCommunicator.double_disconnection_attempt_has_no_effect
/home/alan/display_server/trunk-mir/tests/unit-tests/frontend/test_protobuf_communicator.cpp:213: Failure
Mock function called more times than expected - taking default action specified at:
/home/alan/display_server/mir5/tests/mir_test_doubles/test_protobuf_client.cpp:55:
    Function call: disconnect_done()
         Expected: to be never called
           Actual: called once - over-saturated and active
[ FAILED ] ProtobufCommunicator.double_disconnection_attempt_has_no_effect (3 ms)
[----------] 1 test from ProtobufCommunicator (3 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (4 ms total)
[ PASSED ] 0 tests.
[ FAILED ] 1 test, listed below:
[ FAILED ] ProtobufCommunicator.double_disconnection_attempt_has_no_effect

 1 FAILED TEST

review: Needs Fixing

[Robert Carr (robertcarr)]

double_disconnect_attempt_has_no_effect updated, the problem is we are now reporting errors on sending messages out of MirSocketRpcChannel, I've changed this to use an exception (previously it was doing nothing, and then in the earlier version of this branch it was creating a synthetic error response), and updated things appropriately.

Suggested cleanups applied

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:787
http://jenkins.qa.ubuntu.com/job/mir-ci/968/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1251
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1135
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/206
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/206/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/968/rebuild

review: Approve (continuous-integration)

[Robert Ancell (robert-ancell)]

review: Approve

[Alan Griffiths (alan-griffiths)]

706 - // TODO the code between here and "return" doesn't change error
707 - // TODO does it do anything useful?
708 - struct sockaddr_un remote;
709 - auto sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
710 - remote.sun_family = AF_UNIX;
711 - strcpy(remote.sun_path, socket_file.c_str());
712 - auto len = strlen(remote.sun_path) + sizeof(remote.sun_family);
713 -
714 - do
715 - {
716 - std::this_thread::sleep_for(std::chrono::milliseconds(0));
717 - }
718 - while ((connect(sockfd, (struct sockaddr *)&remote, len) == -1)
719 - && (std::chrono::system_clock::now() < limit));
720 -
721 - close(sockfd);
722 -

I wish unrelated changes would go in separate MPs - it makes things easier to follow.

review: Approve

[Gerry Boland (gerboland)]

> Gerry: I'm having some difficulty figuring out how this session name setting
> bit should work. So I think in the mean time, we can progress with this, and
> the limitation that the app only allows one app to be in the 'launching' state
> at a time (to prevent authentication races).
Ok, I can work with this for now.

review: Approve

=== modified file 'include/server/mir/default_server_configuration.h'
--- include/server/mir/default_server_configuration.h	2013-06-25 13:19:14 +0000
+++ include/server/mir/default_server_configuration.h	2013-07-10 16:31:29 +0000
@@ -45,6 +45,7 @@
 class ProtobufIpcFactory;
 class SessionMediatorReport;
 class MessageProcessorReport;
+class SessionAuthorizer;
 }
 
 namespace shell
@@ -146,6 +147,7 @@
      *  @{ */
     virtual std::shared_ptr<frontend::SessionMediatorReport>  the_session_mediator_report();
     virtual std::shared_ptr<frontend::MessageProcessorReport> the_message_processor_report();
+    virtual std::shared_ptr<frontend::SessionAuthorizer>      the_session_authorizer();
     virtual std::shared_ptr<frontend::Shell>                  the_frontend_shell();
     /** @} */
 
@@ -231,6 +233,7 @@
     CachedPtr<frontend::ProtobufIpcFactory>  ipc_factory;
     CachedPtr<frontend::SessionMediatorReport> session_mediator_report;
     CachedPtr<frontend::MessageProcessorReport> message_processor_report;
+    CachedPtr<frontend::SessionAuthorizer> session_authorizer;
     CachedPtr<compositor::BufferAllocationStrategy> buffer_allocation_strategy;
     CachedPtr<graphics::Renderer> renderer;
     CachedPtr<compositor::BufferStreamFactory> buffer_stream_factory;

=== added file 'include/server/mir/frontend/session_authorizer.h'
--- include/server/mir/frontend/session_authorizer.h	1970-01-01 00:00:00 +0000
+++ include/server/mir/frontend/session_authorizer.h	2013-07-10 16:31:29 +0000
@@ -0,0 +1,45 @@
+/*
+ * Copyright © 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored By: Robert Carr <[e-mail]>
+ */
+
+#ifndef MIR_FRONTEND_SESSION_AUTHORIZER_H_
+#define MIR_FRONTEND_SESSION_AUTHORIZER_H_
+
+#include <sys/types.h>
+
+namespace mir
+{
+namespace frontend
+{
+
+class SessionAuthorizer
+{
+public:
+    virtual ~SessionAuthorizer() {}
+    
+    virtual bool connection_is_allowed(pid_t pid) = 0;
+protected:
+    SessionAuthorizer() = default;
+    SessionAuthorizer(SessionAuthorizer const&) = delete;
+    SessionAuthorizer& operator=(SessionAuthorizer const&) = delete;
+};
+
+}
+
+} // namespace mir
+
+#endif // MIR_FRONTEND_SESSION_AUTHORIZER_H_

=== added file 'include/test/mir_test_doubles/stub_session_authorizer.h'
--- include/test/mir_test_doubles/stub_session_authorizer.h	1970-01-01 00:00:00 +0000
+++ include/test/mir_test_doubles/stub_session_authorizer.h	2013-07-10 16:31:29 +0000
@@ -0,0 +1,43 @@
+/*
+ * Copyright © 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Robert Carr <[e-mail]>
+ */
+
+#ifndef MIR_TEST_DOUBLES_STUB_SESSION_AUTHORIZER_H_
+#define MIR_TEST_DOUBLES_STUB_SESSION_AUTHORIZER_H_
+
+#include "mir/frontend/session_authorizer.h"
+
+namespace mir
+{
+namespace test
+{
+namespace doubles
+{
+
+class StubSessionAuthorizer : public frontend::SessionAuthorizer
+{
+    bool connection_is_allowed(pid_t /* pid */)
+    {
+        return true;
+    }
+};
+
+}
+}
+} // namespace mir
+
+#endif // MIR_TEST_DOUBLES_STUB_SESSION_AUTHORIZER_H_

=== modified file 'src/client/mir_connection.cpp'
--- src/client/mir_connection.cpp	2013-07-08 22:22:00 +0000
+++ src/client/mir_connection.cpp	2013-07-10 16:31:29 +0000
@@ -195,11 +195,16 @@
 {
     std::lock_guard<std::recursive_mutex> lock(mutex);
 
-    server.disconnect(
-        0,
-        &ignored,
-        &ignored,
-        google::protobuf::NewCallback(this, &MirConnection::done_disconnect));
+    try 
+    {
+        server.disconnect(0, &ignored, &ignored,
+                          google::protobuf::NewCallback(this, &MirConnection::done_disconnect));
+    }
+    catch (std::exception const& x)
+    {
+        set_error_message(std::string("disconnect: ") + x.what());
+        disconnect_wait_handle.result_received();
+    }
 
     return &disconnect_wait_handle;
 }

=== modified file 'src/client/rpc/mir_socket_rpc_channel.cpp'
--- src/client/rpc/mir_socket_rpc_channel.cpp	2013-07-08 04:51:27 +0000
+++ src/client/rpc/mir_socket_rpc_channel.cpp	2013-07-10 16:31:29 +0000
@@ -27,8 +27,11 @@
 #include "mir_protobuf_wire.pb.h"
 
 #include <boost/bind.hpp>
+#include <boost/throw_exception.hpp>
+
 #include <cstring>
 #include <sstream>
+#include <stdexcept>
 
 namespace mclr = mir::client::rpc;
 
@@ -218,7 +221,11 @@
         error);
 
     if (error)
+    {
         rpc_report->invocation_failed(invocation, error);
+        
+        BOOST_THROW_EXCEPTION(std::runtime_error("Failed to send message to server"));
+    }
     else
         rpc_report->invocation_succeeded(invocation);
 }

=== modified file 'src/server/default_server_configuration.cpp'
--- src/server/default_server_configuration.cpp	2013-07-03 03:06:24 +0000
+++ src/server/default_server_configuration.cpp	2013-07-10 16:31:29 +0000
@@ -33,6 +33,7 @@
 #include "mir/frontend/session_mediator_report.h"
 #include "mir/frontend/null_message_processor_report.h"
 #include "mir/frontend/session_mediator.h"
+#include "mir/frontend/session_authorizer.h"
 #include "mir/frontend/resource_cache.h"
 #include "mir/shell/session_manager.h"
 #include "mir/shell/registration_order_focus_sequence.h"
@@ -656,6 +657,23 @@
         });
 }
 
+std::shared_ptr<mf::SessionAuthorizer>
+mir::DefaultServerConfiguration::the_session_authorizer()
+{
+    struct DefaultSessionAuthorizer : public mf::SessionAuthorizer
+    {
+        bool connection_is_allowed(pid_t /* pid */)
+        {
+            return true;
+        }
+    };
+    return session_authorizer(
+        [&]()
+        {
+            return std::make_shared<DefaultSessionAuthorizer>();
+        });
+}
+
 std::shared_ptr<mf::SessionMediatorReport>
 mir::DefaultServerConfiguration::the_session_mediator_report()
 {

=== modified file 'src/server/frontend/make_protobuf_socket_communicator.cpp'
--- src/server/frontend/make_protobuf_socket_communicator.cpp	2013-06-19 16:14:40 +0000
+++ src/server/frontend/make_protobuf_socket_communicator.cpp	2013-07-10 16:31:29 +0000
@@ -40,6 +40,7 @@
             return std::make_shared<mf::ProtobufSocketCommunicator>(
                 the_socket_file(),
                 the_ipc_factory(the_frontend_shell(), the_viewable_area(), the_buffer_allocator()),
+                the_session_authorizer(),
                 threads,
                 [shell_sessions]
                 {

=== modified file 'src/server/frontend/message_processor.h'
--- src/server/frontend/message_processor.h	2013-05-30 11:20:17 +0000
+++ src/server/frontend/message_processor.h	2013-07-10 16:31:29 +0000
@@ -25,6 +25,8 @@
 #include <iosfwd>
 #include <cstdint>
 
+#include <sys/types.h>
+
 namespace mir
 {
 namespace frontend
@@ -35,6 +37,8 @@
 {
     virtual void send(std::string const& body) = 0;
     virtual void send_fds(std::vector<int32_t> const& fd) = 0;
+
+    virtual pid_t client_pid() = 0;
 protected:
     MessageSender() = default;
     virtual ~MessageSender() { /* TODO: make nothrow */ }

=== modified file 'src/server/frontend/protobuf_socket_communicator.cpp'
--- src/server/frontend/protobuf_socket_communicator.cpp	2013-06-19 16:14:40 +0000
+++ src/server/frontend/protobuf_socket_communicator.cpp	2013-07-10 16:31:29 +0000
@@ -22,20 +22,20 @@
 
 #include "mir/frontend/communicator_report.h"
 #include "mir/frontend/protobuf_ipc_factory.h"
+#include "mir/frontend/session_authorizer.h"
 #include "mir/protobuf/google_protobuf_guard.h"
 #include "event_pipe.h"
 
 #include <boost/signals2.hpp>
 
-
 namespace mf = mir::frontend;
 namespace mfd = mir::frontend::detail;
 namespace ba = boost::asio;
 
-
 mf::ProtobufSocketCommunicator::ProtobufSocketCommunicator(
     std::string const& socket_file,
     std::shared_ptr<ProtobufIpcFactory> const& ipc_factory,
+    std::shared_ptr<mf::SessionAuthorizer> const& session_authorizer,
     int threads,
     std::function<void()> const& force_requests_to_complete,
     std::shared_ptr<CommunicatorReport> const& report)
@@ -43,6 +43,7 @@
     acceptor(io_service, socket_file),
     io_service_threads(threads),
     ipc_factory(ipc_factory),
+    session_authorizer(session_authorizer),
     next_session_id(0),
     connected_sessions(std::make_shared<mfd::ConnectedSessions<mfd::SocketSession>>()),
     force_requests_to_complete(force_requests_to_complete),
@@ -147,9 +148,11 @@
 {
     if (!ec)
     {
-        connected_sessions->add(session);
-
-        session->read_next_message();
+        if (session_authorizer->connection_is_allowed(session->client_pid()))
+        {
+            connected_sessions->add(session);
+            session->read_next_message();
+        }
     }
     start_accept();
 }

=== modified file 'src/server/frontend/protobuf_socket_communicator.h'
--- src/server/frontend/protobuf_socket_communicator.h	2013-07-07 23:08:41 +0000
+++ src/server/frontend/protobuf_socket_communicator.h	2013-07-10 16:31:29 +0000
@@ -45,6 +45,7 @@
 {
 class ResourceCache;
 class ProtobufIpcFactory;
+class SessionAuthorizer;
 
 namespace detail
 {
@@ -61,6 +62,7 @@
     explicit ProtobufSocketCommunicator(
         const std::string& socket_file,
         std::shared_ptr<ProtobufIpcFactory> const& ipc_factory,
+        std::shared_ptr<SessionAuthorizer> const& session_authorizer,
         int threads,
         std::function<void()> const& force_requests_to_complete,
         std::shared_ptr<CommunicatorReport> const& report);
@@ -70,7 +72,7 @@
 
 private:
     void start_accept();
-    void on_new_connection(const std::shared_ptr<detail::SocketSession>& session, const boost::system::error_code& ec);
+    void on_new_connection(std::shared_ptr<detail::SocketSession> const& session, const boost::system::error_code& ec);
     int next_id();
 
     const std::string socket_file;
@@ -78,6 +80,7 @@
     boost::asio::local::stream_protocol::acceptor acceptor;
     std::vector<std::thread> io_service_threads;
     std::shared_ptr<ProtobufIpcFactory> const ipc_factory;
+    std::shared_ptr<SessionAuthorizer> const session_authorizer;
     std::atomic<int> next_session_id;
     std::shared_ptr<detail::ConnectedSessions<detail::SocketSession>> const connected_sessions;
     std::function<void()> const force_requests_to_complete;

=== modified file 'src/server/frontend/socket_session.cpp'
--- src/server/frontend/socket_session.cpp	2013-07-09 10:13:52 +0000
+++ src/server/frontend/socket_session.cpp	2013-07-10 16:31:29 +0000
@@ -19,6 +19,10 @@
 #include "socket_session.h"
 
 #include <boost/signals2.hpp>
+#include <boost/throw_exception.hpp>
+
+#include <stdexcept>
+
 #include <sys/types.h>
 #include <sys/socket.h>
 
@@ -153,3 +157,15 @@
         BOOST_THROW_EXCEPTION(std::runtime_error(error.message()));
     }
 }
+
+pid_t mfd::SocketSession::client_pid()
+{
+    struct ucred cr;
+    socklen_t cl = sizeof(cr);
+    
+    auto status = getsockopt(socket.native_handle(), SOL_SOCKET, SO_PEERCRED, &cr, &cl);
+    
+    if (status)
+        BOOST_THROW_EXCEPTION(std::runtime_error("Failed to query client socket credentials"));
+    return cr.pid;
+}

=== modified file 'src/server/frontend/socket_session.h'
--- src/server/frontend/socket_session.h	2013-06-20 09:50:05 +0000
+++ src/server/frontend/socket_session.h	2013-07-10 16:31:29 +0000
@@ -25,6 +25,8 @@
 
 #include <boost/asio.hpp>
 
+#include <sys/types.h>
+
 namespace mir
 {
 namespace frontend
@@ -55,6 +57,8 @@
         return socket;
     }
 
+    pid_t client_pid();
+
 private:
     void send(std::string const& body);
     void send_fds(std::vector<int32_t> const& fd);

=== modified file 'tests/acceptance-tests/CMakeLists.txt'
--- tests/acceptance-tests/CMakeLists.txt	2013-04-29 23:21:58 +0000
+++ tests/acceptance-tests/CMakeLists.txt	2013-07-10 16:31:29 +0000
@@ -10,6 +10,7 @@
   test_test_framework.cpp
   test_focus_selection.cpp
   test_server_shutdown.cpp
+  test_client_authorization.cpp
 )
 
 list(APPEND SOURCES

=== added file 'tests/acceptance-tests/test_client_authorization.cpp'
--- tests/acceptance-tests/test_client_authorization.cpp	1970-01-01 00:00:00 +0000
+++ tests/acceptance-tests/test_client_authorization.cpp	2013-07-10 16:31:29 +0000
@@ -0,0 +1,257 @@
+/*
+ * Copyright © 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Robert Carr <[e-mail]>
+ */
+
+#include "mir/frontend/session_authorizer.h"
+
+#include <mir_toolkit/mir_client_library.h>
+
+#include "mir_test/fake_shared.h"
+#include "mir_test_framework/display_server_test_fixture.h"
+
+#include <gtest/gtest.h>
+#include <gmock/gmock.h>
+
+#include <sys/mman.h>
+#include <sys/types.h>
+#include <semaphore.h>
+#include <time.h>
+#include <unistd.h>
+
+namespace mf = mir::frontend;
+namespace mt = mir::test;
+namespace mtf = mir_test_framework;
+
+namespace
+{
+    char const* const mir_test_socket = mtf::test_socket_file().c_str();
+}
+
+namespace
+{
+
+struct ClientConfigCommon : TestingClientConfiguration
+{
+    ClientConfigCommon() :
+        connection(0),
+        surface(0)
+    {
+    }
+
+    static void connection_callback(MirConnection* connection, void* context)
+    {
+        ClientConfigCommon* config = reinterpret_cast<ClientConfigCommon *>(context);
+        config->connection = connection;
+    }
+
+    static void create_surface_callback(MirSurface* surface, void* context)
+    {
+        ClientConfigCommon* config = reinterpret_cast<ClientConfigCommon *>(context);
+        config->surface_created(surface);
+    }
+
+    static void release_surface_callback(MirSurface* surface, void* context)
+    {
+        ClientConfigCommon* config = reinterpret_cast<ClientConfigCommon *>(context);
+        config->surface_released(surface);
+    }
+
+    virtual void connected(MirConnection* new_connection)
+    {
+        connection = new_connection;
+    }
+
+    virtual void surface_created(MirSurface* new_surface)
+    {
+        surface = new_surface;
+    }
+
+    virtual void surface_released(MirSurface* /*released_surface*/)
+    {
+        surface = NULL;
+    }
+
+    MirConnection* connection;
+    MirSurface* surface;
+};
+
+struct ConnectingClient : ClientConfigCommon
+{
+    void exec()
+    {
+        mir_wait_for(mir_connect(
+            mir_test_socket,
+            __PRETTY_FUNCTION__,
+            connection_callback,
+            this));
+         mir_connection_release(connection);
+    }
+};
+
+struct ClientPidTestFixture : BespokeDisplayServerTestFixture
+{
+    ClientPidTestFixture()
+    {
+        shared_region = static_cast<SharedRegion*>(
+            mmap(NULL, sizeof(SharedRegion), PROT_READ | PROT_WRITE,
+                MAP_ANONYMOUS | MAP_SHARED, 0, 0));
+        sem_init(&shared_region->client_pid_set, 1, 0);
+        shared_region->client_pid = 0;
+    }
+
+    struct SharedRegion 
+    {
+        sem_t client_pid_set;
+        pid_t client_pid;
+
+        pid_t get_client_process_pid()
+        {
+            static time_t const timeout_seconds = 60;
+            struct timespec abs_timeout = { time(NULL) + timeout_seconds, 0 };
+            sem_timedwait(&client_pid_set, &abs_timeout);
+            return client_pid;
+        }
+        void post_client_process_pid(pid_t pid)
+        {
+            client_pid = pid;
+            sem_post(&client_pid_set);
+        }
+    };
+    
+    SharedRegion* shared_region;
+};
+
+struct MockSessionAuthorizer : public mf::SessionAuthorizer
+{
+    MOCK_METHOD1(connection_is_allowed, bool(pid_t));
+};
+
+}
+
+TEST_F(ClientPidTestFixture, session_authorizer_receives_pid_of_connecting_clients)
+{
+    struct ServerConfiguration : TestingServerConfiguration
+    {
+        ServerConfiguration(ClientPidTestFixture::SharedRegion *shared_region)
+            : shared_region(shared_region)
+        {
+        }
+
+        void exec() override
+        {
+            using namespace ::testing;
+            pid_t client_pid = shared_region->get_client_process_pid();
+
+            EXPECT_CALL(mock_authorizer, connection_is_allowed(client_pid)).Times(1)
+                .WillOnce(Return(true));
+        }
+        
+        std::shared_ptr<mf::SessionAuthorizer> the_session_authorizer() override
+        {
+            return mt::fake_shared(mock_authorizer);
+        }
+
+        ClientPidTestFixture::SharedRegion* shared_region;
+        MockSessionAuthorizer mock_authorizer;
+    } server_config(shared_region);
+    launch_server_process(server_config);
+    
+
+    struct ClientConfiguration : ConnectingClient
+    {
+        ClientConfiguration(ClientPidTestFixture::SharedRegion *shared_region)
+            : shared_region(shared_region)
+        {
+        }
+
+        void exec() override
+        {
+            pid_t client_pid = getpid();
+            shared_region->post_client_process_pid(client_pid);
+
+            ConnectingClient::exec();
+        }
+
+        ClientPidTestFixture::SharedRegion* shared_region;
+    } client_config(shared_region);
+    launch_client_process(client_config);    
+}
+
+TEST_F(ClientPidTestFixture, authorizer_may_prevent_connection_of_clients)
+{
+    using namespace ::testing;
+
+    struct ServerConfiguration : TestingServerConfiguration
+    {
+        ServerConfiguration(ClientPidTestFixture::SharedRegion *shared_region)
+            : shared_region(shared_region)
+        {
+        }
+
+        void exec() override
+        {
+            using namespace ::testing;
+            pid_t client_pid = shared_region->get_client_process_pid();
+
+            EXPECT_CALL(mock_authorizer, connection_is_allowed(client_pid)).Times(1)
+                .WillOnce(Return(false));
+        }
+        
+        std::shared_ptr<mf::SessionAuthorizer> the_session_authorizer() override
+        {
+            return mt::fake_shared(mock_authorizer);
+        }
+
+        ClientPidTestFixture::SharedRegion* shared_region;
+        MockSessionAuthorizer mock_authorizer;
+    } server_config(shared_region);
+    launch_server_process(server_config);
+    
+
+    struct ClientConfiguration : ConnectingClient
+    {
+        ClientConfiguration(ClientPidTestFixture::SharedRegion *shared_region)
+            : shared_region(shared_region)
+        {
+        }
+
+        void exec() override
+        {
+            pid_t client_pid = getpid();
+            shared_region->post_client_process_pid(client_pid);
+
+            mir_wait_for(mir_connect(
+                mir_test_socket,
+                __PRETTY_FUNCTION__,
+                connection_callback,
+                this));
+            MirSurfaceParameters const parameters =
+            {
+                __PRETTY_FUNCTION__,
+                1, 1,
+                mir_pixel_format_abgr_8888,
+                mir_buffer_usage_hardware
+            };
+            mir_connection_create_surface_sync(connection, &parameters);
+            EXPECT_GT(strlen(mir_connection_get_error_message(connection)), static_cast<unsigned int>(0));
+            mir_connection_release(connection);
+        }
+
+        ClientPidTestFixture::SharedRegion* shared_region;
+    } client_config(shared_region);
+    launch_client_process(client_config);    
+}

=== modified file 'tests/acceptance-tests/test_client_library.cpp'
--- tests/acceptance-tests/test_client_library.cpp	2013-06-28 10:42:44 +0000
+++ tests/acceptance-tests/test_client_library.cpp	2013-07-10 16:31:29 +0000
@@ -736,7 +736,8 @@
             char const* error = mir_connection_get_error_message(connection);
 
             if (std::strcmp("connect: No such file or directory", error) &&
-                std::strcmp("Can't find MIR server", error))
+                std::strcmp("Can't find MIR server", error) &&
+                std::strcmp("Failed to connect to server socket", error))
             {
                 FAIL() << error;
             }

=== modified file 'tests/mir_test_doubles/test_protobuf_socket_server.cpp'
--- tests/mir_test_doubles/test_protobuf_socket_server.cpp	2013-06-19 16:14:40 +0000
+++ tests/mir_test_doubles/test_protobuf_socket_server.cpp	2013-07-10 16:31:29 +0000
@@ -18,6 +18,7 @@
 
 #include "mir_test/test_protobuf_server.h"
 #include "mir_test_doubles/stub_ipc_factory.h"
+#include "mir_test_doubles/stub_session_authorizer.h"
 #include "mir/frontend/communicator_report.h"
 #include "src/server/frontend/protobuf_socket_communicator.h"
 
@@ -35,6 +36,7 @@
     return std::make_shared<mf::ProtobufSocketCommunicator>(
         socket_name,
         factory,
+        std::make_shared<mtd::StubSessionAuthorizer>(),
         10,
         []{},
         report);

=== modified file 'tests/mir_test_framework/socket_detect_server.cpp'
--- tests/mir_test_framework/socket_detect_server.cpp	2013-01-11 23:47:40 +0000
+++ tests/mir_test_framework/socket_detect_server.cpp	2013-07-10 16:31:29 +0000
@@ -45,23 +45,6 @@
     }
     while (error && std::chrono::system_clock::now() < limit);
 
-    // TODO the code between here and "return" doesn't change error
-    // TODO does it do anything useful?
-    struct sockaddr_un remote;
-    auto sockfd = socket(AF_UNIX, SOCK_STREAM, 0);
-    remote.sun_family = AF_UNIX;
-    strcpy(remote.sun_path, socket_file.c_str());
-    auto len = strlen(remote.sun_path) + sizeof(remote.sun_family);
-
-    do
-    {
-        std::this_thread::sleep_for(std::chrono::milliseconds(0));
-    }
-    while ((connect(sockfd, (struct sockaddr *)&remote, len) == -1)
-            && (std::chrono::system_clock::now() < limit));
-
-    close(sockfd);
-
     return !error;
 }
 

=== modified file 'tests/unit-tests/frontend/test_protobuf_communicator.cpp'
--- tests/unit-tests/frontend/test_protobuf_communicator.cpp	2013-06-20 13:23:08 +0000
+++ tests/unit-tests/frontend/test_protobuf_communicator.cpp	2013-07-10 16:31:29 +0000
@@ -25,6 +25,7 @@
 #include "mir_protobuf.pb.h"
 
 #include "mir_test_doubles/stub_ipc_factory.h"
+#include "mir_test_doubles/stub_session_authorizer.h"
 #include "mir_test_doubles/mock_rpc_report.h"
 #include "mir_test/stub_server_tool.h"
 #include "mir_test/test_protobuf_client.h"
@@ -184,7 +185,7 @@
 }
 
 TEST_F(ProtobufCommunicator,
-       double_disconnection_attempt_has_no_effect)
+       double_disconnection_attempt_throws_exception)
 {
     using namespace testing;
 
@@ -211,10 +212,12 @@
     EXPECT_CALL(*client->rpc_report, invocation_failed(InvocationMethodEq("disconnect"),_));
     EXPECT_CALL(*client, disconnect_done()).Times(0);
 
+    EXPECT_THROW({
     // We don't know if this will be called, so it can't auto destruct
     std::unique_ptr<google::protobuf::Closure> new_callback(google::protobuf::NewPermanentCallback(client.get(), &mt::TestProtobufClient::disconnect_done));
     client->display_server.disconnect(0, &client->ignored, &client->ignored, new_callback.get());
     client->wait_for_disconnect_done();
+    }, std::runtime_error);
 }
 
 TEST_F(ProtobufCommunicator,
@@ -318,7 +321,8 @@
         .Times(2);
 
     auto comms = std::make_shared<mf::ProtobufSocketCommunicator>(
-                    "./test_socket1", ipc_factory, 10,
+                    "./test_socket1", ipc_factory, 
+                    std::make_shared<mtd::StubSessionAuthorizer>(), 10,
                     std::bind(&MockForceRequests::force_requests_to_complete,
                               &mock_force_requests),
                     std::make_shared<mf::NullCommunicatorReport>());