Merge into development-branch : attestable-timestamps-server : Code : Mir

Status:	Merged
Approved by:	Alan Griffiths on 2015-09-17
Approved revision:	no longer in the source branch.
Merged at revision:	2943
Proposed branch:	lp:~mir-team/mir/attestable-timestamps-server
Merge into:	lp:mir
Diff against target:	1375 lines (+951/-5) 32 files modified CMakeLists.txt (+1/-0) cmake/ABICheck.cmake (+2/-1) debian/control (+28/-0) debian/libmircookie-dev.install (+3/-0) debian/libmircookie1.install (+1/-0) debian/mir-test-tools.install (+1/-0) include/cookie/mir/cookie_factory.h (+104/-0) include/cookie/mir_toolkit/cookie.h (+44/-0) include/server/mir/server.h (+12/-0) include/test/mir_test_framework/executable_path.h (+1/-0) src/CMakeLists.txt (+3/-0) src/cookie/CMakeLists.txt (+48/-0) src/cookie/cookie_factory.cpp (+161/-0) src/cookie/mircookie.pc.in (+11/-0) src/cookie/symbols.map (+9/-0) src/include/server/mir/default_server_configuration.h (+7/-1) src/include/server/mir/server_configuration.h (+5/-0) src/server/CMakeLists.txt (+2/-1) src/server/default_server_configuration.cpp (+21/-1) src/server/server.cpp (+17/-0) src/server/symbols.map (+2/-0) tests/acceptance-tests/CMakeLists.txt (+3/-0) tests/include/mir_test_framework/udev_environment.h (+26/-0) tests/integration-tests/CMakeLists.txt (+1/-0) tests/mir_test_framework/CMakeLists.txt (+4/-0) tests/mir_test_framework/executable_path.cpp (+13/-0) tests/mir_test_framework/udev_environment.cpp (+38/-1) tests/mir_test_framework/udev_recordings/CMakeLists.txt (+3/-0) tests/mir_test_framework/udev_recordings/laptop-keyboard-hello.evemu (+272/-0) tests/unit-tests/CMakeLists.txt (+2/-0) tests/unit-tests/test_mir_cookie.cpp (+105/-0) tools/update_package_abis.sh (+1/-0)
To merge this branch:	bzr merge lp:~mir-team/mir/attestable-timestamps-server
Related bugs:	Link a bug report

Reviewer	Review Type	Status
Tyler Hicks (community)		Needs Fixing on 2015-10-30
Brandon Schaefer (community)		Approve on 2015-09-17
Andreas Pokorny (community)		Approve on 2015-09-17
Alexandros Frantzis (community)		Approve on 2015-09-17
Kevin DuBois (community)		Approve on 2015-09-17
PS Jenkins bot (community)	continuous-integration	Approve on 2015-09-17
Alan Griffiths		Approve on 2015-09-17
Chris Halse Rogers		Approve on 2015-09-10
Review via email: mp+270457@code.launchpad.net

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-08:

#

FAILED: Continuous integration, rev:2909
http://jenkins.qa.ubuntu.com/job/mir-ci/4825/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3868/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2775/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3814/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/974/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3814/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-09:

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2015-09-09:

#

From the other review:

AFAICS libmircookie is only used by libmirserver.

What is the advantage to having a shared library over linking the code directly into libmirserver?

review: Needs Information

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-09:

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-09:

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-09-09:

#

+std::string mir_test_framework::udev_recordings_path()
+{
+ std::string bin_path = MIR_BUILD_PREFIX"/bin/udev_recordings";
+ std::string share_path = MIR_INSTALL_PREFIX"/share/udev_recordings";
+
+ if (boost::filesystem::exists(bin_path))
+ return bin_path;
+ else if (boost::filesystem::exists(share_path))
+ return share_path;
+
+ BOOST_THROW_EXCEPTION(std::runtime_error("Failed to find udev_recordings in standard search locations"));
+}

Can this really be the right behaviour? An installed test binary looks in the location where it was built? Even though a developer may be making changes there to compare results with.

~~~~

+ char const* RANDOM_DEVICE_PATH = "/dev/random";
+ int const WAIT_SECONDS = 30;
...
+ int const MAX_WAIT = 4;

http://unity.ubuntu.com/mir/cppguide/index.html?showone=Constant_Names#Constant_Names

~~~~

AFAICS libmircookie is only used by libmirserver.

What is the advantage to having a shared library over linking the code directly into libmirserver?

~~~~

/mir/tests/acceptance-tests/test_mir_cookie.cpp:39:13: error: unused variable 'MAX_WAIT' [-Werror,-Wunused-const-variable]
int const MAX_WAIT = 4;
^
1 error generated.

review: Needs Fixing

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2015-09-09:

#

>
> +std::string mir_test_framework::udev_recordings_path()
> +{
> + std::string bin_path = MIR_BUILD_PREFIX"/bin/udev_recordings";
> + std::string share_path = MIR_INSTALL_PREFIX"/share/udev_recordings";
> +
> + if (boost::filesystem::exists(bin_path))
> + return bin_path;
> + else if (boost::filesystem::exists(share_path))
> + return share_path;
> +
> + BOOST_THROW_EXCEPTION(std::runtime_error("Failed to find udev_recordings
> in standard search locations"));
> +}
>
> Can this really be the right behaviour? An installed test binary looks in the
> location where it was built? Even though a developer may be making changes
> there to compare results with.

I should check the share folder first. Other then that... I think its correct behaviour. We still want to be able to test it in a build directory. Unless I add an env variable that we set for the test it self. Let me know which one you would like!

>
> ~~~~
>
> + char const* RANDOM_DEVICE_PATH = "/dev/random";
> + int const WAIT_SECONDS = 30;
> ...
> + int const MAX_WAIT = 4;
>
> http://unity.ubuntu.com/mir/cppguide/index.html?showone=Constant_Names#Constan
> t_Names
>

I need to read through the coding standard for mir :). Use to compiz/unity7/nux.

> ~~~~
>
> AFAICS libmircookie is only used by libmirserver.
>
> What is the advantage to having a shared library over linking the code
> directly into libmirserver?
>

This is how it currently stands but we are planning on using this for the content hub as well. So best to keep it shared now, as it will be used by something else.

> ~~~~
>
> /mir/tests/acceptance-tests/test_mir_cookie.cpp:39:13: error: unused variable
> 'MAX_WAIT' [-Werror,-Wunused-const-variable]
> int const MAX_WAIT = 4;
> ^
> 1 error generated.

Thanks! Missed removing that.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-09:

#

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-09-10:

#

Sorry about the long list here, most of it is about fitting in with the design styles that have grown in Mir and you would not yet be familiar with. I've voted "Needs Fixing" but many of the points might be addressed by discussion.

~~~~

src/server/default_server_configuration.cpp

There's a lot of code added here (to generate a seed) that probably doesn't belong in a translation unit that is about constructing the right objects to configure the system. I'd prefer to see it put in a separate file. (Is there any reason it can't be part of the new library?)

~~~~

+void fill_vector_with_random_data(std::vector<uint8_t>& buffer)

Could we not use a return value instead of an out parameter?

~~~~

+std::string mir_test_framework::udev_recordings_path()
+{
+ std::string share_path = MIR_INSTALL_PREFIX"/share/udev_recordings";
+ std::string bin_path = MIR_BUILD_PREFIX"/bin/udev_recordings";
+
+ if (boost::filesystem::exists(share_path))
+ return share_path;
+ else if (boost::filesystem::exists(bin_path))
+ return bin_path;
+
+ BOOST_THROW_EXCEPTION(std::runtime_error("Failed to find udev_recordings in standard search locations"));
+}

Can this be right? A development test binary looks in the location where it would be installed? Even though a developer may be making changes to the version in the build path.

The older logic (based on the executable path) seems less surprising.

~~~~

+class CookieFactory
+{
+public:
+ CookieFactory(std::vector<uint8_t> const& secret);
+ ~CookieFactory() noexcept;
+
+ MirCookie timestamp_to_cookie(uint64_t const& timestamp);
+
+ bool attest_timestamp(MirCookie const& cookie);
+
+private:
+ class CookieImpl;
+ std::unique_ptr<CookieImpl> impl;
+};

What is the reason for preferring the Cheshire Cat idiom over Interface Class? This makes it far harder to substitute alternative implementations (e.g. for testing that cookies actually are validated when necessary).

I can also imagine a desire to provide a different verification model in a (hypothetical) downstream server.

~~~~~

+void mir::Server::set_cookie_secret(std::vector<uint8_t> const& secret)
+{
+ verify_setting_allowed(self->server_config);
+ self->cookie_factory = std::make_shared<CookieFactory>(secret);
+}

It is a little surprising that this sets the cookie factory, not the secret. I can't immediately think of anything this breaks but...

Between this and the previous, I think this would fit better into Mir's design patterns if:

1. CookieFactory were an interface and CookieImpl a derived class.
2. the configuration point were override_the_cookie_factory

~~~~~

=== added file 'tests/acceptance-tests/test_mir_cookie.cpp'

This reads more like a unit test

~~~~~

+ mir_discover_tests_with_fd_leak_detection(mir_acceptance_tests LD_PRELOAD=libumockdev-preload.so.0 G_SLICE=always-malloc G_DEBUG=gc-friendly)

The acceptance tests (before and after this MP) don't need libumockdev-preload.so.0 (which is convenient when running them by hand) why change that?

~~~~~

AFAICS the_cookie_factory() isn't used. I've not looked in the parent branch - I assume use (and real acceptance tests) is coming soon?

Sorry about the long list here, most of it is about fitting in with the design styles that have grown in Mir and you would not yet be familiar with. I've voted "Needs Fixing" but many of the points might be addressed by discussion.

~~~~

src/server/default_server_configuration.cpp

There's a lot of code added here (to generate a seed) that probably doesn't belong in a translation unit that is about constructing the right objects to configure the system. I'd prefer to see it put in a separate file. (Is there any reason it can't be part of the new library?)

~~~~

+void fill_vector_with_random_data(std::vector<uint8_t>& buffer)

Could we not use a return value instead of an out parameter?

~~~~

+std::string mir_test_framework::udev_recordings_path()
+{
+    std::string share_path = MIR_INSTALL_PREFIX"/share/udev_recordings";
+    std::string bin_path   = MIR_BUILD_PREFIX"/bin/udev_recordings";
+
+    if (boost::filesystem::exists(share_path))
+        return share_path;
+    else if (boost::filesystem::exists(bin_path))
+        return bin_path;
+
+    BOOST_THROW_EXCEPTION(std::runtime_error("Failed to find udev_recordings in standard search locations"));
+}

Can this be right? A development test binary looks in the location where it would be installed? Even though a developer may be making changes to the version in the build path.

The older logic (based on the executable path) seems less surprising.

~~~~

+class CookieFactory
+{
+public:
+    CookieFactory(std::vector<uint8_t> const& secret);
+    ~CookieFactory() noexcept;
+
+    MirCookie timestamp_to_cookie(uint64_t const& timestamp);
+
+    bool attest_timestamp(MirCookie const& cookie);
+
+private:
+    class CookieImpl;
+    std::unique_ptr<CookieImpl> impl;
+};

What is the reason for preferring the Cheshire Cat idiom over Interface Class? This makes it far harder to substitute alternative implementations (e.g. for testing that cookies actually are validated when necessary).

I can also imagine a desire to provide a different verification model in a (hypothetical) downstream server.

~~~~~

+void mir::Server::set_cookie_secret(std::vector<uint8_t> const& secret)
+{
+    verify_setting_allowed(self->server_config);
+    self->cookie_factory = std::make_shared<CookieFactory>(secret);
+}

It is a little surprising that this sets the cookie factory, not the secret. I can't immediately think of anything this breaks but...

Between this and the previous, I think this would fit better into Mir's design patterns if:

1. CookieFactory were an interface and CookieImpl a derived class.
2. the configuration point were override_the_cookie_factory

~~~~~

=== added file 'tests/acceptance-tests/test_mir_cookie.cpp'

This reads more like a unit test

~~~~~

+ mir_discover_tests_with_fd_leak_detection(mir_acceptance_tests LD_PRELOAD=libumockdev-preload.so.0 G_SLICE=always-malloc G_DEBUG=gc-friendly)

The acceptance tests (before and after this MP) don't need libumockdev-preload.so.0 (which is convenient when running them by hand) why change that?

~~~~~

AFAICS the_cookie_factory() isn't used. I've not looked in the parent branch - I assume use (and real acceptance tests) is coming soon?

review: Needs Fixing

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2015-09-10:

#

Download full text (4.6 KiB)

> Sorry about the long list here, most of it is about fitting in with the design
> styles that have grown in Mir and you would not yet be familiar with. I've
> voted "Needs Fixing" but many of the points might be addressed by discussion.
>
> ~~~~
>

No problem at all :), Thanks for going through in depth!

> src/server/default_server_configuration.cpp
>
> There's a lot of code added here (to generate a seed) that probably doesn't
> belong in a translation unit that is about constructing the right objects to
> configure the system. I'd prefer to see it put in a separate file. (Is there
> any reason it can't be part of the new library?)
>
> ~~~~

This is true, Im not 100% sure why it cant be in the library it self. Ill have to talk to Chris about that.

>
> +void fill_vector_with_random_data(std::vector<uint8_t>& buffer)
>
> Could we not use a return value instead of an out parameter?
>
> ~~~~

Changed!

>
> +std::string mir_test_framework::udev_recordings_path()
> +{
> + std::string share_path = MIR_INSTALL_PREFIX"/share/udev_recordings";
> + std::string bin_path = MIR_BUILD_PREFIX"/bin/udev_recordings";
> +
> + if (boost::filesystem::exists(share_path))
> + return share_path;
> + else if (boost::filesystem::exists(bin_path))
> + return bin_path;
> +
> + BOOST_THROW_EXCEPTION(std::runtime_error("Failed to find udev_recordings
> in standard search locations"));
> +}
>
> Can this be right? A development test binary looks in the location where it
> would be installed? Even though a developer may be making changes to the
> version in the build path.
>
> The older logic (based on the executable path) seems less surprising.
>
> ~~~~

Yes... Hmm so my overall thought here is do we need to always install the file to run the tests? Or should we ever be able to run the tests on a build directory? If we just want the test to run off the install location im 100% fine with that. All else I can think about is setting an env variable so we can run it to the build directory (and the test can set it).

Either way im happy to change it.

>
> +class CookieFactory
> +{
> +public:
> + CookieFactory(std::vector<uint8_t> const& secret);
> + ~CookieFactory() noexcept;
> +
> + MirCookie timestamp_to_cookie(uint64_t const& timestamp);
> +
> + bool attest_timestamp(MirCookie const& cookie);
> +
> +private:
> + class CookieImpl;
> + std::unique_ptr<CookieImpl> impl;
> +};
>
> What is the reason for preferring the Cheshire Cat idiom over Interface Class?
> This makes it far harder to substitute alternative implementations (e.g. for
> testing that cookies actually are validated when necessary).
>
> I can also imagine a desire to provide a different verification model in a
> (hypothetical) downstream server.
>
> ~~~~~

Not sure the reasoning here, Ill have to talk to Chris about it. Wouldnt be to hard to switch over to just an interface.
>
> +void mir::Server::set_cookie_secret(std::vector<uint8_t> const& secret)
> +{
> + verify_setting_allowed(self->server_config);
> + self->cookie_factory = std::make_shared<CookieFactory>(secret);
> +}
>
> It is a little surprising that this sets the cookie factory, n...

> Sorry about the long list here, most of it is about fitting in with the design
> styles that have grown in Mir and you would not yet be familiar with. I've
> voted "Needs Fixing" but many of the points might be addressed by discussion.
> 
> ~~~~
>

No problem at all :), Thanks for going through in depth!

> src/server/default_server_configuration.cpp
> 
> There's a lot of code added here (to generate a seed) that probably doesn't
> belong in a translation unit that is about constructing the right objects to
> configure the system. I'd prefer to see it put in a separate file. (Is there
> any reason it can't be part of the new library?)
> 
> ~~~~

This is true, Im not 100% sure why it cant be in the library it self. Ill have to talk to Chris about that.

> 
> +void fill_vector_with_random_data(std::vector<uint8_t>& buffer)
> 
> Could we not use a return value instead of an out parameter?
> 
> ~~~~

Changed!

> 
> +std::string mir_test_framework::udev_recordings_path()
> +{
> +    std::string share_path = MIR_INSTALL_PREFIX"/share/udev_recordings";
> +    std::string bin_path   = MIR_BUILD_PREFIX"/bin/udev_recordings";
> +
> +    if (boost::filesystem::exists(share_path))
> +        return share_path;
> +    else if (boost::filesystem::exists(bin_path))
> +        return bin_path;
> +
> +    BOOST_THROW_EXCEPTION(std::runtime_error("Failed to find udev_recordings
> in standard search locations"));
> +}
> 
> Can this be right? A development test binary looks in the location where it
> would be installed? Even though a developer may be making changes to the
> version in the build path.
> 
> The older logic (based on the executable path) seems less surprising.
> 
> ~~~~

Yes... Hmm so my overall thought here is do we need to always install the file to run the tests? Or should we ever be able to run the tests on a build directory? If we just want the test to run off the install location im 100% fine with that. All else I can think about is setting an env variable so we can run it to the build directory (and the test can set it).

Either way im happy to change it.

> 
> +class CookieFactory
> +{
> +public:
> +    CookieFactory(std::vector<uint8_t> const& secret);
> +    ~CookieFactory() noexcept;
> +
> +    MirCookie timestamp_to_cookie(uint64_t const& timestamp);
> +
> +    bool attest_timestamp(MirCookie const& cookie);
> +
> +private:
> +    class CookieImpl;
> +    std::unique_ptr<CookieImpl> impl;
> +};
> 
> What is the reason for preferring the Cheshire Cat idiom over Interface Class?
> This makes it far harder to substitute alternative implementations (e.g. for
> testing that cookies actually are validated when necessary).
> 
> I can also imagine a desire to provide a different verification model in a
> (hypothetical) downstream server.
> 
> ~~~~~

Not sure the reasoning here, Ill have to talk to Chris about it. Wouldnt be to hard to switch over to just an interface.
> 
> +void mir::Server::set_cookie_secret(std::vector<uint8_t> const& secret)
> +{
> +    verify_setting_allowed(self->server_config);
> +    self->cookie_factory = std::make_shared<CookieFactory>(secret);
> +}
> 
> It is a little surprising that this sets the cookie factory, not the secret. I
> can't immediately think of anything this breaks but...
> 
> Between this and the previous, I think this would fit better into Mir's design
> patterns if:
> 
> 1. CookieFactory were an interface and CookieImpl a derived class.
> 2. the configuration point were override_the_cookie_factory
> 
> ~~~~~

Right, well if we switch to an interface this should be simpler to make clearer. Have to talk to Chris about it was well :).
> 
> === added file 'tests/acceptance-tests/test_mir_cookie.cpp'
> 
> This reads more like a unit test
> 
> ~~~~~
test_cookie_factory sound any better? (Im bad at names)
> 
> + mir_discover_tests_with_fd_leak_detection(mir_acceptance_tests LD_PRELOAD
> =libumockdev-preload.so.0 G_SLICE=always-malloc G_DEBUG=gc-friendly)
> 
> The acceptance tests (before and after this MP) don't need libumockdev-
> preload.so.0 (which is convenient when running them by hand) why change that?
> 
> ~~~~~

This was done because the client code has a test that needs this. Seems I didnt split this as well as I could have. Ill remove it for now!

> 
> AFAICS the_cookie_factory() isn't used. I've not looked in the parent branch -
> I assume use (and real acceptance tests) is coming soon?

This is used in the client side of the code, this branch shows what the changes will come (but is broken and I was waiting for the server code to merge to get a client side branch up):

https://code.launchpad.net/~mir-team/mir/attestable-timestamps-client/+merge/270470

The main start of the test is 1142 in the diff.

Thank you!

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-10:

#

FAILED: Continuous integration, rev:2914
http://jenkins.qa.ubuntu.com/job/mir-ci/4858/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3910/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2815/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3854/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1007/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3854/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-10:

#

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

review: Approve (continuous-integration)

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-09-10:

#

> What is the reason for preferring the Cheshire Cat idiom over Interface Class?
> This makes it far harder to substitute alternative implementations (e.g. for
> testing that cookies actually are validated when necessary).
>
> I can also imagine a desire to provide a different verification model in a
> (hypothetical) downstream server.

libmircookie is going to be used by Content Hub; Mir will share the secret with CH, and then CH will be able to verify cookies sent to it (for copy/paste and drag/drop operations). This is why it's a split-out library.

As such, it's actively harmful for downstream servers to override the implementation, as multiple processes need to agree on the method to verify cookies.

As an added bonus feature, it also means that libmircookie can have an actual ABI that's stable even under mild code churn :).

> Can this be right? A development test binary looks in the location where it
> would be installed? Even though a developer may be making changes to the
> version in the build path.
>
> The older logic (based on the executable path) seems less surprising.

Yeah, I'd have the test look first in the bin dir and *then* in the system-install location.

> src/server/default_server_configuration.cpp
>
> There's a lot of code added here (to generate a seed) that probably doesn't
> belong in a translation unit that is about constructing the right objects to
> configure the system. I'd prefer to see it put in a separate file. (Is there
> any reason it can't be part of the new library?)

I'd have no objections to it being in the new library.

Other comments:

+ * This is not in any way cryptographically secure. This DOES NOT provide security.

We should drop this bit of the comment.

Once the above are dealt with: approve.

We should probably add a debian/libmircookie1.symbols file, but that can easily be done later.

review: Approve

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-09-10:

#

Oh, whitespace before '}':
362 + MirCookie cookie { timestamp, 0};

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-11:

#

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

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-11:

#

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

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-11:

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-09-11:

#

> > I can also imagine a desire to provide a different verification model in a
> > (hypothetical) downstream server.
>
> libmircookie is going to be used by Content Hub; Mir will share the secret
> with CH, and then CH will be able to verify cookies sent to it (for copy/paste
> and drag/drop operations). This is why it's a split-out library.
>
> As such, it's actively harmful for downstream servers to override the
> implementation, as multiple processes need to agree on the method to verify
> cookies.

Oh, I agree that within a UI stack it ought to be fixed. Just imagining that someone /could/ want a different implementation in a different stack.

> As an added bonus feature, it also means that libmircookie can have an actual
> ABI that's stable even under mild code churn :).

OK, that's an argument for a separate library. In fact, is it really something that will need to be released with every Mir release? Or should it be a separate project?

BTW this last argument doesn't address the "Cheshire Cat vs Interface" point. The ABI could easily be:

std::unique_ptr<CookieFactory> CookieFactory::create_with_secret(std::vector<uint8_t> const& secret);
std::unique_ptr<CookieFactory> CookieFactory::create_without_secret();

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-09-11:

#

> > === added file 'tests/acceptance-tests/test_mir_cookie.cpp'
> >
> > This reads more like a unit test
> >
> > ~~~~~
> test_cookie_factory sound any better? (Im bad at names)

You misunderstand, I mean the test body reads like a unit test, not a test of the system.

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2015-09-11:

#

O yeah sorry, I was planning on moving all those tests to unit tests in the client code ... but i should do that now and save on diff +/- :)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-11:

#

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

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-11:

#

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-09-14:

#

+namespace mir
+{
...
+class CookieFactory
...
+std::vector<uint8_t> get_random_data(unsigned size);

We reserve the general mir namespace for system setup. These could be in a "cookie" namespace.

~~~~

+class CookieFactory
+{
+public:
+ CookieFactory(std::vector<uint8_t> const& secret);
+ ~CookieFactory() noexcept;
+
+ MirCookie timestamp_to_cookie(uint64_t const& timestamp);
+
+ bool attest_timestamp(MirCookie const& cookie);
+
+private:
+ class CookieImpl;
+ std::unique_ptr<CookieImpl> impl;
+};

I'm still not convinced we should prefer the Cheshire Cat idiom (over Interface Class) here, but if we are taking this approach then impl should be const.

~~~~

+std::string mir_test_framework::udev_recordings_path()
+{
+ std::string bin_path = MIR_BUILD_PREFIX"/bin/udev_recordings";
+ std::string share_path = MIR_INSTALL_PREFIX"/share/udev_recordings";
+
+ if (boost::filesystem::exists(bin_path))
+ return bin_path;
+ else if (boost::filesystem::exists(share_path))
+ return share_path;
+
+ BOOST_THROW_EXCEPTION(std::runtime_error("Failed to find udev_recordings in standard search locations"));
+}

Can this really be the right behaviour? An installed test binary looks in the location where it was built? Even though a developer may be making changes there?

The older logic (based on the executable path) seems less surprising.

~~~~

+ CookieFactory(std::vector<uint8_t> const& secret);

This will throw if the secret is too small, but the behaviour and minimum size is not documented anywhere.

I suggest providing a static class member (minimum_secret_size) and documenting the requirement.

review: Needs Fixing

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-09-14:

#

Is libmircookie really something that will need to be released in sync with every Mir release? Or should it be a separate project?

~~~~

+std::vector<uint8_t> get_random_data(unsigned size);

Could be:

CookieFactory::CookieFactory(unsigned size);

~~~~

+void mir::Server::set_cookie_secret(std::vector<uint8_t> const& secret)
+{
+ verify_setting_allowed(self->server_config);
+ self->cookie_factory = std::make_shared<CookieFactory>(secret);
+}

It is a little surprising that this sets the cookie factory, not the secret.

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2015-09-14:

#

> Is libmircookie really something that will need to be released in sync with
> every Mir release? Or should it be a separate project?

I dont have a clear answer for this one. I just imagine since the project is pretty small it makes it easier to keep it in lp:mir atm? Will have to poke raof about that one.
>
> ~~~~
>
> +std::vector<uint8_t> get_random_data(unsigned size);
>
> Could be:
>
> CookieFactory::CookieFactory(unsigned size);
>

I had this at first, but switched up because one of the primary use-cases for CookieFactory is when you've shared the secret with another process, which you can't do if you've called CookieFactory(unsigned size)

> ~~~~
>
> +void mir::Server::set_cookie_secret(std::vector<uint8_t> const& secret)
> +{
> + verify_setting_allowed(self->server_config);
> + self->cookie_factory = std::make_shared<CookieFactory>(secret);
> +}
>
> It is a little surprising that this sets the cookie factory, not the secret.

I think this function should just be renamed to initialize_cookie_factory(..)

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2015-09-14:

#

> +namespace mir
> +{
> ...
> +class CookieFactory
> ...
> +std::vector<uint8_t> get_random_data(unsigned size);
>
> We reserve the general mir namespace for system setup. These could be in a
> "cookie" namespace.
>
> ~~~~

Done.

>
> +class CookieFactory
> +{
> +public:
> + CookieFactory(std::vector<uint8_t> const& secret);
> + ~CookieFactory() noexcept;
> +
> + MirCookie timestamp_to_cookie(uint64_t const& timestamp);
> +
> + bool attest_timestamp(MirCookie const& cookie);
> +
> +private:
> + class CookieImpl;
> + std::unique_ptr<CookieImpl> impl;
> +};
>
> I'm still not convinced we should prefer the Cheshire Cat idiom (over
> Interface Class) here, but if we are taking this approach then impl should be
> const.
>
> ~~~~

Move to an NVI idiom:
http://www.gotw.ca/publications/mill18.htm
>
> +std::string mir_test_framework::udev_recordings_path()
> +{
> + std::string bin_path = MIR_BUILD_PREFIX"/bin/udev_recordings";
> + std::string share_path = MIR_INSTALL_PREFIX"/share/udev_recordings";
> +
> + if (boost::filesystem::exists(bin_path))
> + return bin_path;
> + else if (boost::filesystem::exists(share_path))
> + return share_path;
> +
> + BOOST_THROW_EXCEPTION(std::runtime_error("Failed to find udev_recordings
> in standard search locations"));
> +}
>
> Can this really be the right behaviour? An installed test binary looks in the
> location where it was built? Even though a developer may be making changes
> there?
>
> The older logic (based on the executable path) seems less surprising.
>
> ~~~~
Not sure what the correct answer is here. What would be the best solution?

>
> + CookieFactory(std::vector<uint8_t> const& secret);
>
> This will throw if the secret is too small, but the behaviour and minimum size
> is not documented anywhere.
>
> I suggest providing a static class member (minimum_secret_size) and
> documenting the requirement.

Done.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-15:

#

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-15:

#

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-09-15:

#

> > +std::vector<uint8_t> get_random_data(unsigned size);
> >
> > Could be:
> >
> > CookieFactory::CookieFactory(unsigned size);
> >
>
> I had this at first, but switched up because one of the primary use-cases for
> CookieFactory is when you've shared the secret with another process, which you
> can't do if you've called CookieFactory(unsigned size)

I was thinking two constructors would be:

explicit CookieFactory(std::vector<uint8_t> const& secret);
explicit CookieFactory(unsigned size);

But I take your point - you need access to the "random data" to share the secret.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-09-15:

#

> > Is libmircookie really something that will need to be released in sync with
> > every Mir release? Or should it be a separate project?
>
> I dont have a clear answer for this one. I just imagine since the project is
> pretty small it makes it easier to keep it in lp:mir atm? Will have to poke
> raof about that one.

A further thought, the header is dependent on MirCookie (introduced here into include/common/mir_toolkit/common.h). That makes client code (compile time) dependent on libmircommon-dev.

As we're introducing this dependency on mircommon, could we just put the code into libmircommon and not a separate library?

Or should MirCookie be defined in libmircookie-dev and not libmircommon-dev?

review: Needs Information

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-09-15:

#

+#include <nettle/hmac.h>
...
+ struct hmac_sha1_ctx ctx;

Surely we don't want to introduce this dependency to user code? (See [1].)

There is no point in this class heirarchy if you are putting the derived class definition in the same header.

An option to consider here would the Named Constructor Idiom.

~~~~

+// Using the NVI Idiom

Nobody takes NVPI seriously - it is a solution in search of a problem.

The principle options to consider are Interface and Cheshire Cat (a.k.a. Pimpl). For a comparison of the different options see[2].

~~~~~

+ CookieFactoryNettle(std::vector<uint8_t> const& secret);

This will throw if the secret is too small, but the behaviour and minimum size is not documented in a doc comment on the function. (Having a non-doc comment in the private section of the class doesn't help the user.)

(But as I don't think this class should be made public anyway the documentation should go on the corresponding factory function.)

~~~~

> > The older logic (based on the executable path) seems less surprising.
> >
> > ~~~~
>
> Not sure what the correct answer is here. What would be the best solution?

I think to first check relative to the executable path (as it did before this MP), then to check the install location.

E.g. if running from a build directory it should pick up the one from e.g. <build>/bin/udev_recordings/ if not it picks up the installed version.

~~~~

[1] http://wiki.hsr.ch/Prog3/files/overload72-FINAL_DesigningHeaderFiles.pdf

[2] http://www.twonine.co.uk/articles/SeparatingInterfaceAndImplementation.pdf

review: Needs Fixing

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2015-09-15:

#

> +#include <nettle/hmac.h>
> ...
> + struct hmac_sha1_ctx ctx;
>
> Surely we don't want to introduce this dependency to user code? (See [1].)
>
> There is no point in this class heirarchy if you are putting the derived class
> definition in the same header.
>
> An option to consider here would the Named Constructor Idiom.

Done.

>
> ~~~~
>
> +// Using the NVI Idiom
>
> Nobody takes NVPI seriously - it is a solution in search of a problem.
>
> The principle options to consider are Interface and Cheshire Cat (a.k.a.
> Pimpl). For a comparison of the different options see[2].
>
> ~~~~~

Done.

>
> + CookieFactoryNettle(std::vector<uint8_t> const& secret);
>
> This will throw if the secret is too small, but the behaviour and minimum size
> is not documented in a doc comment on the function. (Having a non-doc comment
> in the private section of the class doesn't help the user.)
>
> (But as I don't think this class should be made public anyway the
> documentation should go on the corresponding factory function.)
>
> ~~~~

Done.

>
> > > The older logic (based on the executable path) seems less surprising.
> > >
> > > ~~~~
> >
> > Not sure what the correct answer is here. What would be the best solution?
>
> I think to first check relative to the executable path (as it did before this
> MP), then to check the install location.
>
> E.g. if running from a build directory it should pick up the one from e.g.
> <build>/bin/udev_recordings/ if not it picks up the installed version.
>
> ~~~~

Done.

>
> [1] http://wiki.hsr.ch/Prog3/files/overload72-FINAL_DesigningHeaderFiles.pdf
>
> [2] http://www.twonine.co.uk/articles/SeparatingInterfaceAndImplementation.pdf

Thanks! Those were good reads. If you've more articles, I would be more then happy to read :).

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2015-09-15:

#

> > > Is libmircookie really something that will need to be released in sync
> with
> > > every Mir release? Or should it be a separate project?
> >
> > I dont have a clear answer for this one. I just imagine since the project is
> > pretty small it makes it easier to keep it in lp:mir atm? Will have to poke
> > raof about that one.
>
> A further thought, the header is dependent on MirCookie (introduced here into
> include/common/mir_toolkit/common.h). That makes client code (compile time)
> dependent on libmircommon-dev.
>
> As we're introducing this dependency on mircommon, could we just put the code
> into libmircommon and not a separate library?
>
> Or should MirCookie be defined in libmircookie-dev and not libmircommon-dev?

Well I dont see why it couldnt be defined in libmircookie-dev. I think *if* we keep libmircookie-dev we should move the MirCookie into it (unless Im missing something for why this isnt possible). If we move to libmircommon-dev then we'll keep it :).

Need to talk with RAOF about it as well

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-15:

#

FAILED: Continuous integration, rev:2927
http://jenkins.qa.ubuntu.com/job/mir-ci/4913/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3990
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2897/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3932/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1063/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3933/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-15:

#

FAILED: Continuous integration, rev:2928
http://jenkins.qa.ubuntu.com/job/mir-ci/4915/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3992
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2899/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3934/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1065/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3935/console

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

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-16:

#

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

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-16:

#

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

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-16:

#

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-09-16:

#

+class CookieFactory
...

Should delete copy constructor and assignment.

~~~~

+ static unsigned const minimum_secret_size;

1. Ought to be public, so that it can be accessed by client code.

2. Not providing the value in the header allows it to be changed without breaking ABI, but it would probably be more convenient for users to have the value visible.

~~~~

+ * Contruction function used to create a CookieFactory. The secret size must be
+ * greater then minimum_secret_size otherwise an expection will be thrown
...
+ * Contruction function used to create a CookieFactory as well as a secret.
+ * The secret size must be greater then minimum_secret_size otherwise an expection will be thrown

s/greater then/no less than/

~~~~

+/**
+* Checks entropy exists on the system, then fills in a vector with random numbers
+* up to size.
+*
+* \param [in] size The number of random numbers to generate.
+* \return A filled in vector with random numbers up to size
+*/
+std::vector<uint8_t> get_random_data(unsigned size);

Not needed in the API

~~~~

+unsigned const min_secret_size{8};

This isn't needed, minimum_secret_size should be initialized directly.

~~~~~

+namespace
+{
+ unsigned const secret_size{64};
+}

There ought to be a guarantee (e.g. a static assert) that this is greater than CookieFactory::minimum_secret_size. (And that is why I think CookieFactory::minimum_secret_size should be public and evaluate at compile time.)

~~~~

+TEST(MirCookieFactory, throw_when_secret_size_to_small)
+{
+ std::vector<uint8_t> bob{ 0x01 };
+ EXPECT_THROW({
+ auto factory = mir::cookie::CookieFactory::create(bob);
+ }, std::logic_error);
+}

OK, but it is better to test the "off by one" case. I.e. CookieFactory::minimum_secret_size (as the documentation says) or CookieFactory::minimum_secret_size-1 (as the code says).

~~~~

There are missing tests:

1. create(unsigned secret_size, std::vector<uint8_t>& save_secret);

This should be tested to check that a secret of the right size is saved, and that it throws if too small a size is requested.

2. get_random_data(unsigned size);

*If* this is in the API there should be corresponding tests.

~~~~

+MIR_COOKIE_1 {
+ global:
+ extern "C++" {
+ mir::cookie::CookieFactory::CookieFactory*;
+ mir::cookie::CookieFactory::?CookieFactory*;
+ mir::cookie::CookieFactory::create*;
+ mir::cookie::CookieFactory::timestamp_to_cookie*;
+ mir::cookie::CookieFactory::attest_timestamp*;
+ mir::cookie::get_random_data*;
+ };
+ local: *;
+};

The only functions that need to be exported are mir::cookie::CookieFactory::create*; (and, if it is in the API mir::cookie::get_random_data*;)

+class CookieFactory
...

Should delete copy constructor and assignment.

~~~~

+ static unsigned const minimum_secret_size;

1. Ought to be public, so that it can be accessed by client code.

2. Not providing the value in the header allows it to be changed without breaking ABI, but it would probably be more convenient for users to have the value visible.

~~~~

+    *   Contruction function used to create a CookieFactory. The secret size must be
+    *   greater then minimum_secret_size otherwise an expection will be thrown
...
+    *   Contruction function used to create a CookieFactory as well as a secret.
+ * The secret size must be greater then minimum_secret_size otherwise an expection will be thrown

s/greater then/no less than/

~~~~

+/**
+*   Checks entropy exists on the system, then fills in a vector with random numbers
+*   up to size.
+*
+*   \param [in] size  The number of random numbers to generate.
+*   \return           A filled in vector with random numbers up to size
+*/
+std::vector<uint8_t> get_random_data(unsigned size);

Not needed in the API

~~~~

+unsigned const min_secret_size{8};

This isn't needed, minimum_secret_size should be initialized directly.

~~~~~

+namespace
+{
+    unsigned const secret_size{64};
+}

There ought to be a guarantee (e.g. a static assert) that this is greater than CookieFactory::minimum_secret_size. (And that is why I think CookieFactory::minimum_secret_size should be public and evaluate at compile time.)

~~~~

+TEST(MirCookieFactory, throw_when_secret_size_to_small)
+{
+    std::vector<uint8_t> bob{ 0x01 };
+    EXPECT_THROW({
+        auto factory = mir::cookie::CookieFactory::create(bob);
+    }, std::logic_error);
+}

OK, but it is better to test the "off by one" case. I.e. CookieFactory::minimum_secret_size (as the documentation says) or CookieFactory::minimum_secret_size-1 (as the code says).

~~~~

There are missing tests:

1. create(unsigned secret_size, std::vector<uint8_t>& save_secret);

This should be tested to check that a secret of the right size is saved, and that it throws if too small a size is requested.

2. get_random_data(unsigned size);

*If* this is in the API there should be corresponding tests.

~~~~

+MIR_COOKIE_1 {
+ global:
+  extern "C++" {
+    mir::cookie::CookieFactory::CookieFactory*;
+    mir::cookie::CookieFactory::?CookieFactory*;
+    mir::cookie::CookieFactory::create*;
+    mir::cookie::CookieFactory::timestamp_to_cookie*;
+    mir::cookie::CookieFactory::attest_timestamp*;
+    mir::cookie::get_random_data*;
+  };
+ local: *;
+};

The only functions that need to be exported are mir::cookie::CookieFactory::create*; (and, if it is in the API mir::cookie::get_random_data*;)

review: Needs Fixing

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-09-16:

#

Just a bit of musing, not anything that requires action:

The create functions could be:

std::unique_ptr<CookieFactory> create_from_secret(std::vector<uint8_t> const& save_secret);
std::unique_ptr<CookieFactory> create_saving_secret(std::vector<uint8_t>& save_secret, unsigned secret_size = 2*minimum_secret_size);

That might be more convenient for code that doesn't care about the secret size. There could also be other variants:

std::unique_ptr<CookieFactory> create_keeping_secret(unsigned secret_size = 2*minimum_secret_size);

Which would support user code that just does:

auto const cf = CookieFactory::create_keeping_secret();

Maybe not an important use case?

~~~~

It could also be convenient to include a definition of Secret:

using Secret = std::vector<uint8_t>;

and use that in both the interface and user code.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-09-16:

#

+TEST(MirCookieFactory, timestamp_trusted_with_saved_secret_does_attest)
+{
+ uint64_t timestamp = 23;
+ unsigned secret_size = 64;
+ std::vector<uint8_t> secret;
+
+ auto factory = mir::cookie::CookieFactory::create_saving_secret(secret, secret_size);
+ auto cookie = factory->timestamp_to_cookie(timestamp);
+
+ EXPECT_TRUE(factory->attest_timestamp(cookie));
+}

Not unreasonable, but I was actually thinking of...

    uint64_t timestamp = 23;
    unsigned secret_size = 64;
    std::vector<uint8_t> secret;

    auto const source_factory = mir::cookie::CookieFactory::create_saving_secret(secret, secret_size);
    auto const sink_factory = mir::cookie::CookieFactory::create_from_secret(secret);
    auto cookie = source_factory->timestamp_to_cookie(timestamp);

EXPECT_TRUE(sink_factory->attest_timestamp(cookie));

Which as one of the use cases we intend to support is something we should be testing.

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-09-16:

#

For me, this is the remaining blocker:

> > > > Is libmircookie really something that will need to be released in sync
> > with
> > > > every Mir release? Or should it be a separate project?
> > >
> > > I dont have a clear answer for this one. I just imagine since the project
> is
> > > pretty small it makes it easier to keep it in lp:mir atm? Will have to
> poke
> > > raof about that one.
> >
> > A further thought, the header is dependent on MirCookie (introduced here
> into
> > include/common/mir_toolkit/common.h). That makes client code (compile time)
> > dependent on libmircommon-dev.
> >
> > As we're introducing this dependency on mircommon, could we just put the
> code
> > into libmircommon and not a separate library?
> >
> > Or should MirCookie be defined in libmircookie-dev and not libmircommon-dev?
>
> Well I dont see why it couldnt be defined in libmircookie-dev. I think *if* we
> keep libmircookie-dev we should move the MirCookie into it (unless Im missing
> something for why this isnt possible). If we move to libmircommon-dev then
> we'll keep it :).
>
> Need to talk with RAOF about it as well

review: Needs Information

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-09-16:

#

It would be good for cookie clients to have a narrow, stable API.

To which end we should keep libmircookie as a separate project and make it independent of libmircommon-dev.

review: Needs Fixing

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2015-09-16:

#

> It would be good for cookie clients to have a narrow, stable API.
>
> To which end we should keep libmircookie as a separate project and make it
> independent of libmircommon-dev.

Done. Is it fine to keep the entire mir cookie project inside lp:mir or should a new project be created?

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-16:

#

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-09-16:

#

On Wednesday, 16 September 2015 20:47:24 BST, Brandon Schaefer wrote:
>> It would be good for cookie clients to have a narrow, stable API.
>>
>> To which end we should keep libmircookie as a separate project
>> and make it
>> independent of libmircommon-dev.
>
> Done. Is it fine to keep the entire mir cookie project inside
> lp:mir or should a new project be created?

Fine in mir tree. (For now anyway)

--
Alan Griffiths. +44 (0)798 9938 758
Octopull Limited. http://www.octopull.co.uk/

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2015-09-16:

#

Hmm the only issue im not 100% sure about now. Is how we are going to expose the MirCookie to the mir_toolkit C api. I suppose Ill need to create a C api for the CookieFactory just to hold the MirCookie?

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-16:

#

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

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-16:

#

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

review: Approve (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-17:

#

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

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-09-17:

#

Pushed trivial fixes:

1. src/CMakeLists.txt to build cookie independently of any other Mir code
2. include/cookie/mir_toolkit/cookie.h to compile standalone and add to mir_toolkit docs
3. include/cookie/mir/cookie_factory.h include include/cookie/mir_toolkit/cookie.h first (to ensure it compiles standalone) and move doc comment to correct type.

review: Approve

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2015-09-17:

#

NB I think the server API needs extending to retrieve the generated secret, but that can come in the next MP.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-09-17:

#

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

review: Approve (continuous-integration)

Revision history for this message

Kevin DuBois (kdub) wrote on 2015-09-17:

#

+ static std::unique_ptr<CookieFactory> create_saving_secret(Secret& save_secret,
+ unsigned secret_size = 2 * minimum_secret_size);
+ static std::unique_ptr<CookieFactory> create_keeping_secret(unsigned secret_size = 2 * minimum_secret_size);

http://unity.ubuntu.com/mir/cppguide/index.html?showone=Default_Arguments#Default_Arguments

not too flummoxed by that, the rest lgtm

review: Approve

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2015-09-17:

#

Looks good.

Not blocking, but I find the following API simpler to understand:

create_with_new_secret(unsigned int secret_size);
create_with_existing_secret(Secret const& secret);
Secret secret();

Of course, with this API the secret is no longer really "secret" (i.e. internal to the class), but I don't think it matters. If one has access to the class, then it doesn't really matter if one also has the secret (for our use cases at least).

review: Approve

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2015-09-17:

#

Overall looks good for me.

This has changed quite a bit since the last time. When do we need override_the_cookie_factory(secret)?

nit :+ 470 braces of lambda functor..

review: Approve

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2015-09-17:

#

> Looks good.
>
> Not blocking, but I find the following API simpler to understand:
>
> create_with_new_secret(unsigned int secret_size);
> create_with_existing_secret(Secret const& secret);
> Secret secret();
>
> Of course, with this API the secret is no longer really "secret" (i.e.
> internal to the class), but I don't think it matters. If one has access to the
> class, then it doesn't really matter if one also has the secret (for our use
> cases at least).

A main use of the API is to share the secret around once its been created. So we need to be able to expose the secret on creation so we can share it with another factory.

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2015-09-17:

#

LGTM

review: Approve

Revision history for this message

Brandon Schaefer (brandontschaefer) wrote on 2015-09-17:

#

> Overall looks good for me.
>
> This has changed quite a bit since the last time. When do we need
> override_the_cookie_factory(secret)?
>

This will be used in the client side of things. Not a perfect split from server/client but it will be used :).

> nit :+ 470 braces of lambda functor..

Revision history for this message

Tyler Hicks (tyhicks) wrote on 2015-10-22:

#

Hello - A review from the security team was requested and I've started to look at the code. However, I feel like I'm missing a lot of context from the greater design. It seems like this is only the very low-level building blocks needed by the greater design.

1) Does any documentation exist on the design? Thomas spoke with us about potential high-level designs months ago and I roughly understand which direction you all chose but this HMAC design was not one that we discussed at that time.

2) How will content-hub register itself with Mir? What's the flow of cookies between Mir, the foreground app, and content-hub during copy and paste events?

3) What prevents malicious apps from attempting to brute force cookies? I don't see any type of penalty implementation for repeated, bad guesses. Is content-hub expected to implement that?

review: Needs Information

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-10-22:

#

1) The copy/paste doc is at https://docs.google.com/document/d/16qCQ8vYJmS7da1yfzBuKWnlUCrw_IXXsTIhWruPN0nk

2) The design is for content-hub to share a secret with Mir, and each construct their own instance of CookieFactory from that secret. Content-hub does not need to register itself with Mir. The copy/paste doc has some cookie flow examples.

3) The Mir calls which accept a MirCookie¹ will disconnect any client that submits an invalid cookie. Content-hub should probably do the same. Would it be better to (a) document that any attestation failure should be fatal to the client, and (b) rename “bool attest_timestamp(MirCookie const& cookie)” to “assert_timestamp()” and have it throw an exception on failure?

¹: Here's an example: https://code.launchpad.net/~mir-team/mir/cookie-raise-surface/+merge/274728

Revision history for this message

Tyler Hicks (tyhicks) wrote on 2015-10-23:

#

1) Thanks! The copy/paste doc helps a lot but it lacks implementation details. For example, "secret" isn't mentioned anywhere in the document. It does get me closer to understanding the design.

2) I'm confused by the "Content-hub does not need to register itself with Mir" bit since content-hub must share a secret with Mir and then Mir's CookieFactory is constructed from that secret. How does Mir know that the CookieFactory that it constructed was done so with a secret from Content-hub and not some other process?

3) If Mir disconnects a client that submits an invalid cookie, what prevents the client from reconnecting and submitting another invalid cookie?
- Documenting that any attestation failure should be fatal to the client would be a good thing as long as a reconnecting is expensive.
- IIUC, assert_timestamp() is in the address space of the client, correct? If so, throwing an exception on failure would do no good because a malicious client could link against a modified library that doesn't throw an exception.

review: Needs Information

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-10-27:

#

1, 2) Yeah, that bit of the design is unspecified. I was assuming that we'd either go for:
a) A parent process of unity8 and content-hub generates a secret and fd-passes it to both, or
b) Unity8 and content-hub share a file readable only by them, or
c) content-hub is started by unity8 and gets the secret fd-passed to it.

I'm not sufficiently familiar with the global design to know which one is most appropriate.

3) Unity8 only allows one connection per client. Actually, that's a lie; Unity8 effectively allows any client to connect to it, but in *theory* Unity8 only allows one connection per client. In theory, unity8 only allows applications started by upstart-app-launch and only allows one connection per launch, but in practice it allows anything that has --desktop-file-hint=valid.desktop on its commandline, or any binary whose name starts with maliit-server or which contains qt5/libexec/QtWebProcess.

So reconnection *should* be expensive, but currently isn't.

assert_timestamp() is in the server's address space; the secret required to assert a timestamp isn't available in the client address space.

Revision history for this message

Tyler Hicks (tyhicks) wrote on 2015-10-29:

#

1, 2) Ok, I'll follow up with tvoss on that aspect of the design.

3) We could probably do something as simple as adding a penalty delay after each failed attest attempt, before disconnecting the client.

Note that I also included a couple inline questions.

review: Needs Information

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-10-29:

#

3) It'd be reasonably complex to add a penalty delay after a failed attest attempt, but possible. It's much simpler for us to just disconnect the client.

Replies inline.

Revision history for this message

Tyler Hicks (tyhicks) wrote on 2015-10-30:

#

Left some inline replies/comments.

review: Needs Fixing

Revision history for this message

Tyler Hicks (tyhicks) wrote on 2015-10-30:

#

One more inline comment...

Revision history for this message

Chris Halse Rogers (raof) wrote on 2015-10-30:

#

Replies also inline.

Mir

Merge lp:~mir-team/mir/attestable-timestamps-server into lp:mir

Commit message

Description of the change

Preview Diff

Subscribers

 === modified file 'CMakeLists.txt'
 --- CMakeLists.txt	2015-08-12 09:52:57 +0000
 +++ CMakeLists.txt	2015-09-17 09:09:28 +0000
@@ -175,6 +175,7 @@
  find_package(LTTngUST REQUIRED)
  pkg_check_modules(UDEV REQUIRED libudev)
  pkg_check_modules(GLIB REQUIRED glib-2.0)
++pkg_check_modules(NETTLE REQUIRED nettle)
  pkg_check_modules(UUID REQUIRED uuid)
  include_directories (${GLESv2_INCLUDE_DIRS})
 === modified file 'cmake/ABICheck.cmake'
 --- cmake/ABICheck.cmake	2015-07-23 02:39:20 +0000
 +++ cmake/ABICheck.cmake	2015-09-17 09:09:28 +0000
@@ -79,6 +79,7 @@
  make_lib_descriptor(client)
  make_lib_descriptor(server)
  make_lib_descriptor(common INCLUDE_PRIVATE EXCLUDE_HEADERS ${mircommon-exclude-headers})
++make_lib_descriptor(cookie)
  make_lib_descriptor(platform INCLUDE_PRIVATE EXCLUDE_HEADERS ${mirplatform-exclude-headers})
  if(MIR_BUILD_PLATFORM_MESA_KMS)
  make_lib_descriptor(clientplatformmesa LIBRARY_HEADER ${CMAKE_SOURCE_DIR}/src/include/client/mir/client_platform_factory.h)
@@ -124,7 +125,7 @@
+   )
  endmacro(_define_abi_check_for)
--set(the_libs mirserver mirclient mircommon mirplatform)
++set(the_libs mirserver mirclient mircommon mirplatform mircookie)
  if(MIR_BUILD_PLATFORM_MESA_KMS)
    set(the_libs ${the_libs} mirclientplatformmesa mirplatformgraphicsmesakms)
  endif()
 === modified file 'debian/control'
 --- debian/control	2015-09-15 12:37:25 +0000
 +++ debian/control	2015-09-17 09:09:28 +0000
@@ -43,6 +43,7 @@
                 uuid-dev,
                 python3:any,
                 dh-python,
++               nettle-dev,
  Standards-Version: 3.9.4
  Homepage: https://launchpad.net/mir
  # If you aren't a member of ~mir-team but need to upload packaging changes,
@@ -402,6 +403,33 @@
   This package depends on a full set of graphics drivers for running Mir on top
   of an existing Android driver stack.
++Package: libmircookie1
++Section: libs
++Architecture: any
++Multi-Arch: same
++Pre-Depends: ${misc:Pre-Depends}
++Depends: ${misc:Depends},
++Description: Produce and verify spoof-resistant timestamps - runtime library
++ libmircookie provides a simple mechanism for a group of cooperating processes
++ to hand out and verify difficult-to-forge timestamps to untrusted 3rd parties.
++ .
++ This package contains the runtime library for generating and verifying the
++ attestable timestamps.
++
++Package: libmircookie-dev
++Section: libdevel
++Architecture: any
++Multi-Arch: same
++Pre-Depends: ${misc:Pre-Depends}
++Depends: libmircookie1 (= ${binary:Version}),
++         ${misc:Depends},
++Description: Produce and verify spoof-resistant timestamps - development headers
++ libmircookie provides a simple mechanism for a group of cooperating processes
++ to hand out and verify difficult-to-forge timestamps to untrusted 3rd parties.
++ .
++ This package contains the development headers for building programs that
++ generate or verify the attestable timestamps.
++
  Package: python3-mir-perf-framework
  Section: python
  Architecture: all
 === added file 'debian/libmircookie-dev.install'
 --- debian/libmircookie-dev.install	1970-01-01 00:00:00 +0000
 +++ debian/libmircookie-dev.install	2015-09-17 09:09:28 +0000
@@ -0,0 +1,3 @@
++/usr/include/mircookie/
++/usr/lib/*/libmircookie.so
++/usr/lib/*/pkgconfig/mircookie.pc
 === added file 'debian/libmircookie1.install'
 --- debian/libmircookie1.install	1970-01-01 00:00:00 +0000
 +++ debian/libmircookie1.install	2015-09-17 09:09:28 +0000
@@ -0,0 +1,1 @@
++/usr/lib/*/libmircookie.so.1
 === modified file 'debian/mir-test-tools.install'
 --- debian/mir-test-tools.install	2015-06-23 08:00:53 +0000
 +++ debian/mir-test-tools.install	2015-09-17 09:09:28 +0000
@@ -10,3 +10,4 @@
  usr/lib/*/mir/server-platform/graphics-dummy.so
  usr/lib/*/mir/server-platform/input-stub.so
  usr/lib/*/mir/client-platform/dummy.so
++usr/share/udev_recordings
 === added directory 'include/cookie'
 === added directory 'include/cookie/mir'
 === added file 'include/cookie/mir/cookie_factory.h'
 --- include/cookie/mir/cookie_factory.h	1970-01-01 00:00:00 +0000
 +++ include/cookie/mir/cookie_factory.h	2015-09-17 09:09:28 +0000
@@ -0,0 +1,104 @@
++/*
++ * Copyright © 2015 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: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com>
++ */
++
++#ifndef MIR_COOKIE_COOKIE_FACTORY_H_
++#define MIR_COOKIE_COOKIE_FACTORY_H_
++
++#include "mir_toolkit/cookie.h"
++
++#include <vector>
++#include <memory>
++
++namespace mir
++{
++namespace cookie
++{
++using Secret = std::vector<uint8_t>;
++
++/**
++ * \brief A source of moderately-difficult-to-spoof cookies.
++ *
++ * The primary motivation for this is to provide event timestamps that clients find it difficult to spoof.
++ * This is useful for focus grant and similar operations where shell behaviour should be dependent on
++ * the timestamp of the client event that caused the request.
++ *
++ * Some spoofing protection is desirable; experience with X clients shows that they will go to some effort
++ * to attempt to bypass focus stealing prevention.
++ *
++ */
++class CookieFactory
++{
++public:
++    /**
++    *   Contruction function used to create a CookieFactory. The secret size must be
++    *   no less then minimum_secret_size otherwise an expection will be thrown
++    *
++    *   \param [in] secret  A filled in secret used to set the key for the hash function
++    *   \return             A unique_ptr CookieFactory
++    */
++    static std::unique_ptr<CookieFactory> create_from_secret(Secret const& secret);
++
++    /**
++    *   Contruction function used to create a CookieFactory as well as a secret.
++    *   The secret size must be no less then minimum_secret_size otherwise an expection will be thrown
++    *
++    *   \param [in]  secret_size  The size of the secret to create, must be larger then minimum_secret_size
++    *   \param [out] save_secret  The secret that was created.
++    *   \return                   A unique_ptr CookieFactory
++    */
++    static std::unique_ptr<CookieFactory> create_saving_secret(Secret& save_secret,
++                                                               unsigned secret_size = 2 * minimum_secret_size);
++
++    /**
++    *   Contruction function used to create a CookieFactory and a secret which it keeps internally.
++    *   The secret size must be no less then minimum_secret_size otherwise an expection will be thrown
++    *
++    *   \param [in]  secret_size  The size of the secret to create, must be larger then minimum_secret_size
++    *   \return                   A unique_ptr CookieFactory
++    */
++    static std::unique_ptr<CookieFactory> create_keeping_secret(unsigned secret_size = 2 * minimum_secret_size);
++
++    CookieFactory(CookieFactory const& factory) = delete;
++    CookieFactory& operator=(CookieFactory const& factory) = delete;
++    virtual ~CookieFactory() noexcept = default;
++
++    /**
++    *   Turns a timestamp into a MAC and returns a MirCookie.
++    *
++    *   \param [in] timestamp The timestamp
++    *   \return               MirCookie with the stored MAC and timestamp
++    */
++    virtual MirCookie timestamp_to_cookie(uint64_t const& timestamp) = 0;
++
++    /**
++    *   Checks that a MirCookie is a valid MirCookie.
++    *
++    *   \param [in] cookie  A created MirCookie
++    *   \return             True when the MirCookie is valid, False when the MirCookie is not valid
++    */
++    virtual bool attest_timestamp(MirCookie const& cookie) = 0;
++
++    static unsigned const minimum_secret_size = 8;
++
++protected:
++    CookieFactory() = default;
++};
++
++}
++}
++#endif // MIR_COOKIE_COOKIE_FACTORY_H_
 === added directory 'include/cookie/mir_toolkit'
 === added file 'include/cookie/mir_toolkit/cookie.h'
 --- include/cookie/mir_toolkit/cookie.h	1970-01-01 00:00:00 +0000
 +++ include/cookie/mir_toolkit/cookie.h	2015-09-17 09:09:28 +0000
@@ -0,0 +1,44 @@
++/*
++ * Copyright © 2015 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: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com>
++ */
++
++#ifndef MIR_TOOLKIT_COOKIE_FACTORY_H_
++#define MIR_TOOLKIT_COOKIE_FACTORY_H_
++
++#include <stdint.h>
++
++/**
++ * \addtogroup mir_toolkit
++ * @{
++ */
++/* This is C code. Not C++. */
++#ifdef __cplusplus
++extern "C" {
++#endif
++
++typedef struct MirCookie
++{
++    uint64_t timestamp;
++    uint64_t mac;
++} MirCookie;
++
++#ifdef __cplusplus
++}
++#endif
++/**@}*/
++
++#endif // MIR_TOOLKIT_COOKIE_FACTORY_H_
 === modified file 'include/server/mir/server.h'
 --- include/server/mir/server.h	2015-09-08 03:25:06 +0000
 +++ include/server/mir/server.h	2015-09-17 09:09:28 +0000
@@ -34,6 +34,10 @@
  namespace input { class CompositeEventFilter; class InputDispatcher; class CursorListener; class TouchVisualizer; class InputDeviceHub;}
  namespace logging { class Logger; }
  namespace options { class Option; }
++namespace cookie
++{
++using Secret = std::vector<uint8_t>;
++}
  namespace shell
+ {
  class DisplayLayout;
@@ -79,6 +83,13 @@
      /// This must remain valid while apply_settings() and run() are called.
      void set_command_line(int argc, char const* argv[]);
++    /// creates the CookieFactory from the given secret
++    /// This secret is used to generate timestamps that can be attested to by
++    /// libmircookie. Any process this secret is shared with can verify Mir-generated
++    /// cookies, or produce their own.
++    /// \note If not explicitly set, a random secret will be chosen.
++    void override_the_cookie_factory(mir::cookie::Secret const& secret);
++
      /// Applies any configuration options, hooks, or custom implementations.
      /// Must be called before calling run() or accessing any mir subsystems.
      void apply_settings();
@@ -381,6 +392,7 @@
      /// using the format "fd://%d".
      auto open_prompt_socket() -> Fd;
  /** @} */
++
  private:
      struct ServerConfiguration;
      struct Self;
 === modified file 'include/test/mir_test_framework/executable_path.h'
 --- include/test/mir_test_framework/executable_path.h	2015-06-22 03:04:56 +0000
 +++ include/test/mir_test_framework/executable_path.h	2015-09-17 09:09:28 +0000
@@ -26,6 +26,7 @@
  std::string executable_path();
  std::string library_path();
++std::string udev_recordings_path();
  std::string server_platform(std::string const& name);
  std::string client_platform(std::string const& name);
+ }
 === modified file 'src/CMakeLists.txt'
 --- src/CMakeLists.txt	2015-09-07 11:54:56 +0000
 +++ src/CMakeLists.txt	2015-09-17 09:09:28 +0000
@@ -1,6 +1,9 @@
  # We need MIRPLATFORM_ABI in both libmirplatform and the platform implementations.
  set(MIRPLATFORM_ABI 10)
++# Add the cookie implementation before exposing any APIs
++add_subdirectory(cookie/)
++
  # We need MIR_CLIENT_PLATFORM_PATH in both libmirclient and the platform
  # implementations
  set(MIR_CLIENT_PLATFORM_PATH
 === added directory 'src/cookie'
 === added file 'src/cookie/CMakeLists.txt'
 --- src/cookie/CMakeLists.txt	1970-01-01 00:00:00 +0000
 +++ src/cookie/CMakeLists.txt	2015-09-17 09:09:28 +0000
@@ -0,0 +1,48 @@
++set(PREFIX "${CMAKE_INSTALL_PREFIX}")
++set(EXEC_PREFIX "${CMAKE_INSTALL_PREFIX}")
++set(LIBDIR "${CMAKE_INSTALL_FULL_LIBDIR}")
++set(INCLUDEDIR "${CMAKE_INSTALL_PREFIX}/include")
++
++configure_file(
++  ${CMAKE_CURRENT_SOURCE_DIR}/mircookie.pc.in
++  ${CMAKE_CURRENT_BINARY_DIR}/mircookie.pc
++  @ONLY
++)
++
++include_directories(
++  ${PROJECT_SOURCE_DIR}/include/cookie
++  ${NETTLE_INCLUDE_DIRS}
++)
++
++set(MIRCOOKIE_ABI 1)
++set(symbol_map ${CMAKE_SOURCE_DIR}/src/cookie/symbols.map)
++
++add_library(mircookie SHARED
++
++  cookie_factory.cpp
++)
++
++set_target_properties(mircookie
++  PROPERTIES
++  SOVERSION ${MIRCOOKIE_ABI}
++  LINK_FLAGS "-Wl,--exclude-libs=ALL -Wl,--version-script,${symbol_map}"
++)
++
++target_link_libraries(mircookie
++  ${NETTLE_LDFLAGS} ${NETTLE_LIBS}
++)
++
++install(
++  TARGETS mircookie
++  LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
++  ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR})
++
++install(
++  DIRECTORY ${CMAKE_SOURCE_DIR}/include/cookie/mir
++  DESTINATION "include/mircookie"
++)
++
++install(
++  FILES ${CMAKE_CURRENT_BINARY_DIR}/mircookie.pc
++  DESTINATION ${CMAKE_INSTALL_LIBDIR}/pkgconfig
++)
 === added file 'src/cookie/cookie_factory.cpp'
 --- src/cookie/cookie_factory.cpp	1970-01-01 00:00:00 +0000
 +++ src/cookie/cookie_factory.cpp	2015-09-17 09:09:28 +0000
@@ -0,0 +1,161 @@
++/*
++ * Copyright © 2015 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: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com>
++ */
++
++#include "mir/cookie_factory.h"
++
++#include <algorithm>
++#include <random>
++#include <memory>
++#include <system_error>
++
++#include <nettle/hmac.h>
++#include <linux/random.h>
++#include <sys/types.h>
++#include <sys/stat.h>
++#include <fcntl.h>
++#include <sys/select.h>
++
++#include <boost/throw_exception.hpp>
++
++namespace
++{
++std::string const random_device_path{"/dev/random"};
++std::string const urandom_device_path{"/dev/urandom"};
++int const wait_seconds{30};
++}
++
++static mir::cookie::Secret get_random_data(unsigned size)
++{
++    mir::cookie::Secret buffer(size);
++    int random_fd;
++    int retval;
++    fd_set rfds;
++
++    struct timeval tv;
++    tv.tv_sec  = wait_seconds;
++    tv.tv_usec = 0;
++
++    if ((random_fd = open(random_device_path.c_str(), O_RDONLY)) == -1)
++    {
++        int error = errno;
++        BOOST_THROW_EXCEPTION(std::system_error(error, std::system_category(),
++                                                "open failed on device " + random_device_path));
++    }
++
++    FD_ZERO(&rfds);
++    FD_SET(random_fd, &rfds);
++
++    /* We want to block until *some* entropy exists on boot, then use urandom once we have some */
++    retval = select(random_fd + 1, &rfds, NULL, NULL, &tv);
++
++    /* We are done with /dev/random at this point, and it is either an error or ready to be read */
++    if (close(random_fd) == -1)
++    {
++        int error = errno;
++        BOOST_THROW_EXCEPTION(std::system_error(error, std::system_category(),
++                                                "close failed on device " + random_device_path));
++    }
++
++    if (retval == -1)
++    {
++        int error = errno;
++        BOOST_THROW_EXCEPTION(std::system_error(error, std::system_category(),
++                                                "select failed on file descriptor " + std::to_string(random_fd) +
++                                                " from device " + random_device_path));
++    }
++    else if (retval && FD_ISSET(random_fd, &rfds))
++    {
++        std::uniform_int_distribution<uint8_t> dist;
++        std::random_device rand_dev(urandom_device_path);
++
++        std::generate(std::begin(buffer), std::end(buffer), [&]() {
++            return dist(rand_dev);
++        });
++    }
++    else
++    {
++        BOOST_THROW_EXCEPTION(std::runtime_error("Failed to read from device: " + random_device_path +
++                                                 " after: " + std::to_string(wait_seconds) + " seconds"));
++    }
++
++    return buffer;
++}
++
++class CookieFactoryNettle : public mir::cookie::CookieFactory
++{
++public:
++    CookieFactoryNettle(mir::cookie::Secret const& secret)
++    {
++        if (secret.size() < minimum_secret_size)
++            BOOST_THROW_EXCEPTION(std::logic_error("Secret size " + std::to_string(secret.size()) + " is to small, require " +
++                                                   std::to_string(minimum_secret_size) + " or greater."));
++
++        hmac_sha1_set_key(&ctx, secret.size(), secret.data());
++    }
++
++    virtual ~CookieFactoryNettle() noexcept = default;
++
++    MirCookie timestamp_to_cookie(uint64_t const& timestamp) override
++    {
++        MirCookie cookie { timestamp, 0 };
++        calculate_mac(cookie);
++        return cookie;
++    }
++
++    bool attest_timestamp(MirCookie const& cookie) override
++    {
++        return verify_mac(cookie);
++    }
++
++private:
++    void calculate_mac(MirCookie& cookie)
++    {
++        hmac_sha1_update(&ctx, sizeof(cookie.timestamp), reinterpret_cast<uint8_t*>(&cookie.timestamp));
++        hmac_sha1_digest(&ctx, sizeof(cookie.mac), reinterpret_cast<uint8_t*>(&cookie.mac));
++    }
++
++    bool verify_mac(MirCookie const& cookie)
++    {
++        decltype(cookie.mac) calculated_mac;
++        uint8_t* message = reinterpret_cast<uint8_t*>(const_cast<decltype(cookie.timestamp)*>(&cookie.timestamp));
++        hmac_sha1_update(&ctx, sizeof(cookie.timestamp), message);
++        hmac_sha1_digest(&ctx, sizeof(calculated_mac), reinterpret_cast<uint8_t*>(&calculated_mac));
++
++        return calculated_mac == cookie.mac;
++    }
++
++    struct hmac_sha1_ctx ctx;
++};
++
++std::unique_ptr<mir::cookie::CookieFactory> mir::cookie::CookieFactory::create_from_secret(mir::cookie::Secret const& secret)
++{
++  return std::make_unique<CookieFactoryNettle>(secret);
++}
++
++std::unique_ptr<mir::cookie::CookieFactory> mir::cookie::CookieFactory::create_saving_secret(mir::cookie::Secret& save_secret,
++                                                                                             unsigned secret_size)
++{
++  save_secret = get_random_data(secret_size);
++  return std::make_unique<CookieFactoryNettle>(save_secret);
++}
++
++std::unique_ptr<mir::cookie::CookieFactory> mir::cookie::CookieFactory::create_keeping_secret(unsigned secret_size)
++{
++  auto secret = get_random_data(secret_size);
++  return std::make_unique<CookieFactoryNettle>(secret);
++}
 === added file 'src/cookie/mircookie.pc.in'
 --- src/cookie/mircookie.pc.in	1970-01-01 00:00:00 +0000
 +++ src/cookie/mircookie.pc.in	2015-09-17 09:09:28 +0000
@@ -0,0 +1,11 @@
++prefix=@PREFIX@
++exec_prefix=@EXEC_PREFIX@
++libdir=@LIBDIR@
++includedir=@INCLUDEDIR@/mircookie
++
++Name: mircookie
++Description: Mir client library
++Version: @MIR_VERSION@
++Requires.private: nettle
++Libs: -L${libdir} -lmircookie
++Cflags: -I${includedir}
 === added file 'src/cookie/symbols.map'
 --- src/cookie/symbols.map	1970-01-01 00:00:00 +0000
 +++ src/cookie/symbols.map	2015-09-17 09:09:28 +0000
@@ -0,0 +1,9 @@
++MIR_COOKIE_1 {
++ global:
++  extern "C++" {
++    mir::cookie::CookieFactory::create_from_secret*;
++    mir::cookie::CookieFactory::create_saving_secret*;
++    mir::cookie::CookieFactory::create_keeping_secret*;
++  };
++ local: *;
++};
 === modified file 'src/include/server/mir/default_server_configuration.h'
 --- src/include/server/mir/default_server_configuration.h	2015-09-08 03:25:06 +0000
 +++ src/include/server/mir/default_server_configuration.h	2015-09-17 09:09:28 +0000
@@ -41,6 +41,10 @@
  class SharedLibrary;
  class SharedLibraryProberReport;
++namespace cookie
++{
++class CookieFactory;
++}
  namespace dispatch
+ {
  class MultiplexingDispatchable;
@@ -186,7 +190,8 @@
      std::shared_ptr<DisplayChanger>         the_display_changer() override;
      std::shared_ptr<graphics::Platform>     the_graphics_platform() override;
      std::shared_ptr<input::InputDispatcher> the_input_dispatcher() override;
--    std::shared_ptr<EmergencyCleanup>  the_emergency_cleanup() override;
++    std::shared_ptr<EmergencyCleanup>       the_emergency_cleanup() override;
++    std::shared_ptr<cookie::CookieFactory>  the_cookie_factory() override;
      /**
       * Function to call when a "fatal" error occurs. This implementation allows
       * the default strategy to be overridden by --on-fatal-error-abort to force a
@@ -441,6 +446,7 @@
      CachedPtr<SharedLibraryProberReport> shared_library_prober_report;
      CachedPtr<shell::Shell> shell;
      CachedPtr<scene::ApplicationNotRespondingDetector> application_not_responding_detector;
++    CachedPtr<cookie::CookieFactory> cookie_factory;
  private:
      std::shared_ptr<options::Configuration> const configuration_options;
 === modified file 'src/include/server/mir/server_configuration.h'
 --- src/include/server/mir/server_configuration.h	2015-06-18 04:38:28 +0000
 +++ src/include/server/mir/server_configuration.h	2015-09-17 09:09:28 +0000
@@ -22,6 +22,10 @@
  namespace mir
+ {
++namespace cookie
++{
++class CookieFactory;
++}
  namespace compositor
+ {
  class Compositor;
@@ -75,6 +79,7 @@
      virtual std::shared_ptr<DisplayChanger> the_display_changer() = 0;
      virtual std::shared_ptr<graphics::Platform>  the_graphics_platform() = 0;
      virtual std::shared_ptr<EmergencyCleanup> the_emergency_cleanup() = 0;
++    virtual std::shared_ptr<cookie::CookieFactory> the_cookie_factory() = 0;
      virtual auto the_fatal_error_strategy() -> void (*)(char const* reason, ...) = 0;
      virtual std::shared_ptr<scene::ApplicationNotRespondingDetector> the_application_not_responding_detector() = 0;
 === modified file 'src/server/CMakeLists.txt'
 --- src/server/CMakeLists.txt	2015-09-07 11:54:56 +0000
 +++ src/server/CMakeLists.txt	2015-09-17 09:09:28 +0000
@@ -10,6 +10,7 @@
    ${PROJECT_SOURCE_DIR}/src/include/platform
    ${PROJECT_SOURCE_DIR}/src/include/client
    ${PROJECT_SOURCE_DIR}/src/include/server
++  ${PROJECT_SOURCE_DIR}/include/cookie
    ${GLIB_INCLUDE_DIRS}
+ )
  add_definitions(-DMIR_SERVER_INPUT_PLATFORM_VERSION="${MIR_SERVER_INPUT_PLATFORM_VERSION}")
@@ -95,7 +96,7 @@
    mirplatform
    mircommon
    mirprotobuf
--
++  mircookie
    xcursorloader-static
    ${GLog_LIBRARY}
 === modified file 'src/server/default_server_configuration.cpp'
 --- src/server/default_server_configuration.cpp	2015-06-17 05:20:42 +0000
 +++ src/server/default_server_configuration.cpp	2015-09-17 09:09:28 +0000
@@ -1,5 +1,5 @@
  /*
-- * Copyright © 2012-2014 Canonical Ltd.
++ * Copyright © 2012-2015 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,
@@ -24,6 +24,7 @@
  #include "mir/default_server_status_listener.h"
  #include "mir/emergency_cleanup.h"
  #include "mir/default_configuration.h"
++#include "mir/cookie_factory.h"
  #include "mir/logging/dumb_console_logger.h"
  #include "mir/options/program_option.h"
@@ -41,6 +42,8 @@
  #include "mir/scene/null_prompt_session_listener.h"
  #include "default_emergency_cleanup.h"
++#include <type_traits>
++
  namespace mc = mir::compositor;
  namespace geom = mir::geometry;
  namespace mf = mir::frontend;
@@ -51,6 +54,11 @@
  namespace msh = mir::shell;
  namespace mi = mir::input;
++namespace
++{
++    unsigned const secret_size{64};
++}
++
  mir::DefaultServerConfiguration::DefaultServerConfiguration(int argc, char const* argv[]) :
          DefaultServerConfiguration(std::make_shared<mo::DefaultConfiguration>(argc, argv))
+ {
@@ -173,6 +181,18 @@
          });
+ }
++std::shared_ptr<mir::cookie::CookieFactory> mir::DefaultServerConfiguration::the_cookie_factory()
++{
++    return cookie_factory(
++        []()
++        {
++            static_assert(secret_size >= mir::cookie::CookieFactory::minimum_secret_size,
++                          "Secret size is smaller then the minimum secret size");
++
++            return mir::cookie::CookieFactory::create_keeping_secret(secret_size);
++        });
++}
++
  auto mir::DefaultServerConfiguration::the_fatal_error_strategy()
  -> void (*)(char const* reason, ...)
+ {
 === modified file 'src/server/server.cpp'
 --- src/server/server.cpp	2015-09-08 03:25:06 +0000
 +++ src/server/server.cpp	2015-09-17 09:09:28 +0000
@@ -29,6 +29,7 @@
  #include "mir/main_loop.h"
  #include "mir/report_exception.h"
  #include "mir/run_mir.h"
++#include "mir/cookie_factory.h"
  // TODO these are used to frig a stub renderer when running headless
  #include "mir/compositor/renderer.h"
@@ -101,6 +102,7 @@
      std::weak_ptr<options::Option> options;
      std::string config_file;
      std::shared_ptr<ServerConfiguration> server_config;
++    std::shared_ptr<cookie::CookieFactory> cookie_factory;
      std::function<void()> init_callback{[]{}};
      int argc{0};
@@ -207,6 +209,15 @@
          return mir::DefaultServerConfiguration::the_renderer_factory();
+     }
++    auto the_cookie_factory() -> std::shared_ptr<cookie::CookieFactory> override
++    {
++        if (self->cookie_factory)
++        {
++            return self->cookie_factory;
++        }
++        return mir::DefaultServerConfiguration::the_cookie_factory();
++    }
++
      using mir::DefaultServerConfiguration::the_options;
      FOREACH_OVERRIDE(MIR_SERVER_CONFIG_OVERRIDE)
@@ -277,6 +288,12 @@
      self->argv = argv;
+ }
++void mir::Server::override_the_cookie_factory(mir::cookie::Secret const& secret)
++{
++    verify_setting_allowed(self->server_config);
++    self->cookie_factory = mir::cookie::CookieFactory::create_from_secret(secret);
++}
++
  void mir::Server::add_init_callback(std::function<void()> const& init_callback)
+ {
      auto const& existing = self->init_callback;
 === modified file 'src/server/symbols.map'
 --- src/server/symbols.map	2015-09-11 12:29:40 +0000
 +++ src/server/symbols.map	2015-09-17 09:09:28 +0000
@@ -135,6 +135,7 @@
      mir::ServerActionQueue::?ServerActionQueue*;
      mir::ServerActionQueue::ServerActionQueue*;
      mir::Server::add_configuration_option*;
++    mir::Server::override_the_cookie_factory*;
      mir::Server::add_emergency_cleanup*;
      mir::Server::add_init_callback*;
      mir::Server::apply_settings*;
@@ -590,6 +591,7 @@
      mir::DefaultServerConfiguration::the_buffer_stream_factory*;
      mir::DefaultServerConfiguration::the_clock*;
      mir::DefaultServerConfiguration::the_composite_event_filter*;
++    mir::DefaultServerConfiguration::the_cookie_factory*;
      mir::DefaultServerConfiguration::the_event_filter_chain_dispatcher*;
      mir::DefaultServerConfiguration::the_surface_input_dispatcher;
      mir::DefaultServerConfiguration::the_compositor*;
 === modified file 'tests/acceptance-tests/CMakeLists.txt'
 --- tests/acceptance-tests/CMakeLists.txt	2015-09-09 18:59:38 +0000
 +++ tests/acceptance-tests/CMakeLists.txt	2015-09-17 09:09:28 +0000
@@ -2,6 +2,8 @@
  include_directories(
    ${CMAKE_SOURCE_DIR}
++  ${PROJECT_SOURCE_DIR}/include/cookie
++  ${UMOCKDEV_INCLUDE_DIRS}
+ )
  set(
@@ -80,6 +82,7 @@
    mirclient-debug-extension
    mirserver
++  ${UMOCKDEV_LIBRARIES}
    ${CMAKE_THREAD_LIBS_INIT} # Link in pthread.
+ )
 === modified file 'tests/include/mir_test_framework/udev_environment.h'
 --- tests/include/mir_test_framework/udev_environment.h	2015-02-22 07:46:25 +0000
 +++ tests/include/mir_test_framework/udev_environment.h	2015-09-17 09:09:28 +0000
@@ -53,6 +53,32 @@
       */
      void add_standard_device(std::string const& name);
++    /**
++     * Load an ioctl recording for a UMockdev device
++     *
++     * Looks for a <tt>name</tt>.ioctl file recorded with umockdev-record --ioctl
++     * and adds it to the associated device in the testbed.
++     *
++     * The udev records for the device these ioctl records will be associated with
++     * must already exist in the testbed
++     *
++     * @param name The unadorned filename for the ioctl records to add
++     */
++    void load_device_ioctls(std::string const& name);
++
++    /**
++     * Load an evemu evdev recording for a UMockdev device
++     *
++     * Looks for a <tt>name</tt>.evemu file recorded with umockdev-record --evemu
++     * (or evemu-record) and associates it with the udev device it was recorded from.
++     *
++     * The udev records for the device this recording is associated with
++     * must already exist in the testbed
++     *
++     * @param name The unadorned filename for the ioctl records to add
++     */
++    void load_device_evemu(std::string const& name);
++
      UMockdevTestbed *testbed;
      std::string const recordings_path;
  };
 === modified file 'tests/integration-tests/CMakeLists.txt'
 --- tests/integration-tests/CMakeLists.txt	2015-09-09 18:59:38 +0000
 +++ tests/integration-tests/CMakeLists.txt	2015-09-17 09:09:28 +0000
@@ -4,6 +4,7 @@
    ${CMAKE_SOURCE_DIR}
    ${PROTOBUF_INCLUDE_DIRS}
    ${CMAKE_CURRENT_BINARY_DIR}
++  ${PROJECT_SOURCE_DIR}/include/cookie
    ${PROJECT_SOURCE_DIR}/src/include/platform
    ${PROJECT_SOURCE_DIR}/src/include/common
    ${PROJECT_SOURCE_DIR}/src/include/server
 === modified file 'tests/mir_test_framework/CMakeLists.txt'
 --- tests/mir_test_framework/CMakeLists.txt	2015-09-14 17:04:48 +0000
 +++ tests/mir_test_framework/CMakeLists.txt	2015-09-17 09:09:28 +0000
@@ -16,6 +16,8 @@
      -DMIR_SERVER_PLATFORM_PATH="${MIR_SERVER_PLATFORM_PATH}"
      -DMIR_CLIENT_PLATFORM_ABI_STRING="${MIR_CLIENT_PLATFORM_ABI}"
      -DMIR_SERVER_GRAPHICS_PLATFORM_ABI_STRING="${MIR_SERVER_GRAPHICS_PLATFORM_ABI}"
++    -DMIR_INSTALL_PREFIX="${CMAKE_INSTALL_PREFIX}"
++    -DMIR_BUILD_PREFIX="${CMAKE_BINARY_DIR}"
+     )
  add_library(mir-public-test-framework OBJECT
@@ -161,3 +163,5 @@
  install(TARGETS mirplatformgraphicsstub LIBRARY DESTINATION ${MIR_SERVER_PLATFORM_PATH})
  install(TARGETS mirclientplatformstub LIBRARY DESTINATION ${MIR_CLIENT_PLATFORM_PATH})
++
++add_subdirectory(udev_recordings/)
 === modified file 'tests/mir_test_framework/executable_path.cpp'
 --- tests/mir_test_framework/executable_path.cpp	2015-06-17 05:20:42 +0000
 +++ tests/mir_test_framework/executable_path.cpp	2015-09-17 09:09:28 +0000
@@ -45,6 +45,19 @@
      return executable_path() + "/../lib";
+ }
++std::string mir_test_framework::udev_recordings_path()
++{
++    std::string run_path     = executable_path() + "/udev_recordings";
++    std::string install_path = MIR_INSTALL_PREFIX"/share/udev_recordings";
++
++    if (boost::filesystem::exists(run_path))
++        return run_path;
++    else if (boost::filesystem::exists(install_path))
++        return install_path;
++
++    BOOST_THROW_EXCEPTION(std::runtime_error("Failed to find udev_recordings in standard search locations"));
++}
++
  std::string mir_test_framework::server_platform(std::string const& name)
+ {
      std::string libname{name};
 === modified file 'tests/mir_test_framework/udev_environment.cpp'
 --- tests/mir_test_framework/udev_environment.cpp	2015-02-22 07:46:25 +0000
 +++ tests/mir_test_framework/udev_environment.cpp	2015-09-17 09:09:28 +0000
@@ -38,7 +38,7 @@
  namespace mtf = mir_test_framework;
  mtf::UdevEnvironment::UdevEnvironment()
--    : recordings_path(mtf::executable_path() + "/udev_recordings")
++    : recordings_path(mtf::udev_recordings_path())
+ {
      testbed = umockdev_testbed_new();
+ }
@@ -109,4 +109,41 @@
+             }
+         }
+     }
++
++    auto script_filename = recordings_path + "/" + name + ".script";
++    if (stat(script_filename.c_str(), &sb) == 0)
++    {
++        if (S_ISREG(sb.st_mode) || S_ISLNK(sb.st_mode))
++        {
++            if (!umockdev_testbed_load_script(testbed, NULL, script_filename.c_str(), &err))
++            {
++                BOOST_THROW_EXCEPTION(std::runtime_error(std::string("Failed to load device recording: ") +
++                                                         err->message));
++            }
++        }
++    }
++}
++
++void mtf::UdevEnvironment::load_device_ioctls(std::string const& name)
++{
++    auto ioctls_filename = recordings_path + "/" + name + ".ioctl";
++
++    GError* err = nullptr;
++    if (!umockdev_testbed_load_ioctl(testbed, NULL, ioctls_filename.c_str(), &err))
++    {
++        BOOST_THROW_EXCEPTION(std::runtime_error(std::string("Failed to load ioctl recording: ") +
++                                                 err->message));
++    }
++}
++
++void mtf::UdevEnvironment::load_device_evemu(std::string const& name)
++{
++    auto evemu_filename = recordings_path + "/" + name + ".evemu";
++
++    GError* err = nullptr;
++    if (!umockdev_testbed_load_evemu_events(testbed, NULL, evemu_filename.c_str(), &err))
++    {
++        BOOST_THROW_EXCEPTION(std::runtime_error(std::string("Failed to load evemu recording: ") +
++                                                 err->message));
++    }
+ }
 === added file 'tests/mir_test_framework/udev_recordings/CMakeLists.txt'
 --- tests/mir_test_framework/udev_recordings/CMakeLists.txt	1970-01-01 00:00:00 +0000
 +++ tests/mir_test_framework/udev_recordings/CMakeLists.txt	2015-09-17 09:09:28 +0000
@@ -0,0 +1,3 @@
++file(GLOB UDEV_FILES *.umockdev *.ioctl *.evemu)
++
++install(FILES ${UDEV_FILES} DESTINATION ${CMAKE_INSTALL_PREFIX}/share/udev_recordings)
 === added file 'tests/mir_test_framework/udev_recordings/laptop-keyboard-hello.evemu'
 --- tests/mir_test_framework/udev_recordings/laptop-keyboard-hello.evemu	1970-01-01 00:00:00 +0000
 +++ tests/mir_test_framework/udev_recordings/laptop-keyboard-hello.evemu	2015-09-17 09:09:28 +0000
@@ -0,0 +1,272 @@
++# device /dev/input/event4
++# EVEMU 1.2
++# Input device name: "AT Translated Set 2 keyboard"
++# Input device ID: bus 0x11 vendor 0x01 product 0x01 version 0xab83
++# Supported events:
++#   Event type 0 (EV_SYN)
++#     Event code 0 (SYN_REPORT)
++#     Event code 1 (SYN_CONFIG)
++#     Event code 4 (FF_STATUS_STOPPED)
++#     Event code 17 ((null))
++#     Event code 20 ((null))
++#   Event type 1 (EV_KEY)
++#     Event code 1 (KEY_ESC)
++#     Event code 2 (KEY_1)
++#     Event code 3 (KEY_2)
++#     Event code 4 (KEY_3)
++#     Event code 5 (KEY_4)
++#     Event code 6 (KEY_5)
++#     Event code 7 (KEY_6)
++#     Event code 8 (KEY_7)
++#     Event code 9 (KEY_8)
++#     Event code 10 (KEY_9)
++#     Event code 11 (KEY_0)
++#     Event code 12 (KEY_MINUS)
++#     Event code 13 (KEY_EQUAL)
++#     Event code 14 (KEY_BACKSPACE)
++#     Event code 15 (KEY_TAB)
++#     Event code 16 (KEY_Q)
++#     Event code 17 (KEY_W)
++#     Event code 18 (KEY_E)
++#     Event code 19 (KEY_R)
++#     Event code 20 (KEY_T)
++#     Event code 21 (KEY_Y)
++#     Event code 22 (KEY_U)
++#     Event code 23 (KEY_I)
++#     Event code 24 (KEY_O)
++#     Event code 25 (KEY_P)
++#     Event code 26 (KEY_LEFTBRACE)
++#     Event code 27 (KEY_RIGHTBRACE)
++#     Event code 28 (KEY_ENTER)
++#     Event code 29 (KEY_LEFTCTRL)
++#     Event code 30 (KEY_A)
++#     Event code 31 (KEY_S)
++#     Event code 32 (KEY_D)
++#     Event code 33 (KEY_F)
++#     Event code 34 (KEY_G)
++#     Event code 35 (KEY_H)
++#     Event code 36 (KEY_J)
++#     Event code 37 (KEY_K)
++#     Event code 38 (KEY_L)
++#     Event code 39 (KEY_SEMICOLON)
++#     Event code 40 (KEY_APOSTROPHE)
++#     Event code 41 (KEY_GRAVE)
++#     Event code 42 (KEY_LEFTSHIFT)
++#     Event code 43 (KEY_BACKSLASH)
++#     Event code 44 (KEY_Z)
++#     Event code 45 (KEY_X)
++#     Event code 46 (KEY_C)
++#     Event code 47 (KEY_V)
++#     Event code 48 (KEY_B)
++#     Event code 49 (KEY_N)
++#     Event code 50 (KEY_M)
++#     Event code 51 (KEY_COMMA)
++#     Event code 52 (KEY_DOT)
++#     Event code 53 (KEY_SLASH)
++#     Event code 54 (KEY_RIGHTSHIFT)
++#     Event code 55 (KEY_KPASTERISK)
++#     Event code 56 (KEY_LEFTALT)
++#     Event code 57 (KEY_SPACE)
++#     Event code 58 (KEY_CAPSLOCK)
++#     Event code 59 (KEY_F1)
++#     Event code 60 (KEY_F2)
++#     Event code 61 (KEY_F3)
++#     Event code 62 (KEY_F4)
++#     Event code 63 (KEY_F5)
++#     Event code 64 (KEY_F6)
++#     Event code 65 (KEY_F7)
++#     Event code 66 (KEY_F8)
++#     Event code 67 (KEY_F9)
++#     Event code 68 (KEY_F10)
++#     Event code 69 (KEY_NUMLOCK)
++#     Event code 70 (KEY_SCROLLLOCK)
++#     Event code 71 (KEY_KP7)
++#     Event code 72 (KEY_KP8)
++#     Event code 73 (KEY_KP9)
++#     Event code 74 (KEY_KPMINUS)
++#     Event code 75 (KEY_KP4)
++#     Event code 76 (KEY_KP5)
++#     Event code 77 (KEY_KP6)
++#     Event code 78 (KEY_KPPLUS)
++#     Event code 79 (KEY_KP1)
++#     Event code 80 (KEY_KP2)
++#     Event code 81 (KEY_KP3)
++#     Event code 82 (KEY_KP0)
++#     Event code 83 (KEY_KPDOT)
++#     Event code 85 (KEY_ZENKAKUHANKAKU)
++#     Event code 86 (KEY_102ND)
++#     Event code 87 (KEY_F11)
++#     Event code 88 (KEY_F12)
++#     Event code 89 (KEY_RO)
++#     Event code 90 (KEY_KATAKANA)
++#     Event code 91 (KEY_HIRAGANA)
++#     Event code 92 (KEY_HENKAN)
++#     Event code 93 (KEY_KATAKANAHIRAGANA)
++#     Event code 94 (KEY_MUHENKAN)
++#     Event code 95 (KEY_KPJPCOMMA)
++#     Event code 96 (KEY_KPENTER)
++#     Event code 97 (KEY_RIGHTCTRL)
++#     Event code 98 (KEY_KPSLASH)
++#     Event code 99 (KEY_SYSRQ)
++#     Event code 100 (KEY_RIGHTALT)
++#     Event code 102 (KEY_HOME)
++#     Event code 103 (KEY_UP)
++#     Event code 104 (KEY_PAGEUP)
++#     Event code 105 (KEY_LEFT)
++#     Event code 106 (KEY_RIGHT)
++#     Event code 107 (KEY_END)
++#     Event code 108 (KEY_DOWN)
++#     Event code 109 (KEY_PAGEDOWN)
++#     Event code 110 (KEY_INSERT)
++#     Event code 111 (KEY_DELETE)
++#     Event code 112 (KEY_MACRO)
++#     Event code 113 (KEY_MUTE)
++#     Event code 114 (KEY_VOLUMEDOWN)
++#     Event code 115 (KEY_VOLUMEUP)
++#     Event code 116 (KEY_POWER)
++#     Event code 117 (KEY_KPEQUAL)
++#     Event code 118 (KEY_KPPLUSMINUS)
++#     Event code 119 (KEY_PAUSE)
++#     Event code 121 (KEY_KPCOMMA)
++#     Event code 122 (KEY_HANGEUL)
++#     Event code 123 (KEY_HANJA)
++#     Event code 124 (KEY_YEN)
++#     Event code 125 (KEY_LEFTMETA)
++#     Event code 126 (KEY_RIGHTMETA)
++#     Event code 127 (KEY_COMPOSE)
++#     Event code 128 (KEY_STOP)
++#     Event code 140 (KEY_CALC)
++#     Event code 142 (KEY_SLEEP)
++#     Event code 143 (KEY_WAKEUP)
++#     Event code 155 (KEY_MAIL)
++#     Event code 156 (KEY_BOOKMARKS)
++#     Event code 157 (KEY_COMPUTER)
++#     Event code 158 (KEY_BACK)
++#     Event code 159 (KEY_FORWARD)
++#     Event code 163 (KEY_NEXTSONG)
++#     Event code 164 (KEY_PLAYPAUSE)
++#     Event code 165 (KEY_PREVIOUSSONG)
++#     Event code 166 (KEY_STOPCD)
++#     Event code 172 (KEY_HOMEPAGE)
++#     Event code 173 (KEY_REFRESH)
++#     Event code 183 (KEY_F13)
++#     Event code 184 (KEY_F14)
++#     Event code 185 (KEY_F15)
++#     Event code 217 (KEY_SEARCH)
++#     Event code 226 (KEY_MEDIA)
++#   Event type 4 (EV_MSC)
++#     Event code 4 (MSC_SCAN)
++#   Event type 17 (EV_LED)
++#     Event code 0 (LED_NUML)
++#     Event code 1 (LED_CAPSL)
++#     Event code 2 (LED_SCROLLL)
++#   Event type 20 (EV_REP)
++# Properties:
++# N: AT Translated Set 2 keyboard
++# I: 0011 0001 0001 ab83
++# P: 00 00 00 00 00 00 00 00
++# B: 00 13 00 12 00 00 00 00 00
++# B: 01 fe ff ff ff ff ff ff ff
++# B: 01 ff ff ef ff df ff ff fe
++# B: 01 01 d0 00 f8 78 30 80 03
++# B: 01 00 00 00 02 04 00 00 00
++# B: 01 00 00 00 00 00 00 00 00
++# B: 01 00 00 00 00 00 00 00 00
++# B: 01 00 00 00 00 00 00 00 00
++# B: 01 00 00 00 00 00 00 00 00
++# B: 01 00 00 00 00 00 00 00 00
++# B: 01 00 00 00 00 00 00 00 00
++# B: 01 00 00 00 00 00 00 00 00
++# B: 01 00 00 00 00 00 00 00 00
++# B: 02 00 00 00 00 00 00 00 00
++# B: 03 00 00 00 00 00 00 00 00
++# B: 04 10 00 00 00 00 00 00 00
++# B: 05 00 00 00 00 00 00 00 00
++# B: 11 07 00 00 00 00 00 00 00
++# B: 12 00 00 00 00 00 00 00 00
++# B: 15 00 00 00 00 00 00 00 00
++# B: 15 00 00 00 00 00 00 00 00
++################################
++#      Waiting for events      #
++################################
++E: 0.000000 0004 0004 0028	# EV_MSC / MSC_SCAN             28
++E: 0.000000 0001 001c 0000	# EV_KEY / KEY_ENTER            0
++E: 0.000000 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
++E: 0.825945 0004 0004 0054	# EV_MSC / MSC_SCAN             54
++E: 0.825945 0001 0036 0001	# EV_KEY / KEY_RIGHTSHIFT       1
++E: 0.825945 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
++E: 1.101404 0004 0004 0054	# EV_MSC / MSC_SCAN             54
++E: 1.101404 0001 0036 0002	# EV_KEY / KEY_RIGHTSHIFT       2
++E: 1.101404 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
++E: 1.131449 0004 0004 0054	# EV_MSC / MSC_SCAN             54
++E: 1.131449 0001 0036 0002	# EV_KEY / KEY_RIGHTSHIFT       2
++E: 1.131449 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
++E: 1.161472 0004 0004 0036	# EV_MSC / MSC_SCAN             36
++E: 1.161472 0001 0024 0001	# EV_KEY / KEY_J                1
++E: 1.161472 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
++E: 1.221981 0004 0004 0036	# EV_MSC / MSC_SCAN             36
++E: 1.221981 0001 0024 0000	# EV_KEY / KEY_J                0
++E: 1.221981 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
++E: 1.241906 0004 0004 0054	# EV_MSC / MSC_SCAN             54
++E: 1.241906 0001 0036 0000	# EV_KEY / KEY_RIGHTSHIFT       0
++E: 1.241906 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
++E: 1.346773 0004 0004 0032	# EV_MSC / MSC_SCAN             32
++E: 1.346773 0001 0020 0001	# EV_KEY / KEY_D                1
++E: 1.346773 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
++E: 1.432213 0004 0004 0032	# EV_MSC / MSC_SCAN             32
++E: 1.432213 0001 0020 0000	# EV_KEY / KEY_D                0
++E: 1.432213 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
++E: 1.562060 0004 0004 0025	# EV_MSC / MSC_SCAN             25
++E: 1.562060 0001 0019 0001	# EV_KEY / KEY_P                1
++E: 1.562060 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
++E: 1.622477 0004 0004 0025	# EV_MSC / MSC_SCAN             25
++E: 1.622477 0001 0019 0000	# EV_KEY / KEY_P                0
++E: 1.622477 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
++E: 1.687305 0004 0004 0025	# EV_MSC / MSC_SCAN             25
++E: 1.687305 0001 0019 0001	# EV_KEY / KEY_P                1
++E: 1.687305 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
++E: 1.777807 0004 0004 0025	# EV_MSC / MSC_SCAN             25
++E: 1.777807 0001 0019 0000	# EV_KEY / KEY_P                0
++E: 1.777807 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
++E: 1.867577 0004 0004 0031	# EV_MSC / MSC_SCAN             31
++E: 1.867577 0001 001f 0001	# EV_KEY / KEY_S                1
++E: 1.867577 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
++E: 1.953019 0004 0004 0031	# EV_MSC / MSC_SCAN             31
++E: 1.953019 0001 001f 0000	# EV_KEY / KEY_S                0
++E: 1.953019 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
++E: 2.118004 0004 0004 0054	# EV_MSC / MSC_SCAN             54
++E: 2.118004 0001 0036 0001	# EV_KEY / KEY_RIGHTSHIFT       1
++E: 2.118004 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
++E: 2.348316 0004 0004 0002	# EV_MSC / MSC_SCAN             2
++E: 2.348316 0001 0002 0001	# EV_KEY / KEY_1                1
++E: 2.348316 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
++E: 2.489561 0004 0004 0002	# EV_MSC / MSC_SCAN             2
++E: 2.489561 0001 0002 0000	# EV_KEY / KEY_1                0
++E: 2.489561 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
++E: 2.523740 0004 0004 0054	# EV_MSC / MSC_SCAN             54
++E: 2.523740 0001 0036 0000	# EV_KEY / KEY_RIGHTSHIFT       0
++E: 2.523740 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
++E: 6.714286 0004 0004 0029	# EV_MSC / MSC_SCAN             29
++E: 6.714286 0001 001d 0001	# EV_KEY / KEY_LEFTCTRL         1
++E: 6.714286 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
++E: 6.989728 0004 0004 0029	# EV_MSC / MSC_SCAN             29
++E: 6.989728 0001 001d 0002	# EV_KEY / KEY_LEFTCTRL         2
++E: 6.989728 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
++E: 7.019767 0004 0004 0029	# EV_MSC / MSC_SCAN             29
++E: 7.019767 0001 001d 0002	# EV_KEY / KEY_LEFTCTRL         2
++E: 7.019767 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
++E: 7.049847 0004 0004 0029	# EV_MSC / MSC_SCAN             29
++E: 7.049847 0001 001d 0002	# EV_KEY / KEY_LEFTCTRL         2
++E: 7.049847 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
++E: 7.079889 0004 0004 0029	# EV_MSC / MSC_SCAN             29
++E: 7.079889 0001 001d 0002	# EV_KEY / KEY_LEFTCTRL         2
++E: 7.079889 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
++E: 7.109934 0004 0004 0029	# EV_MSC / MSC_SCAN             29
++E: 7.109934 0001 001d 0002	# EV_KEY / KEY_LEFTCTRL         2
++E: 7.109934 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
++E: 7.139943 0004 0004 0029	# EV_MSC / MSC_SCAN             29
++E: 7.139943 0001 001d 0002	# EV_KEY / KEY_LEFTCTRL         2
++E: 7.139943 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
++E: 7.170005 0004 0004 0023	# EV_MSC / MSC_SCAN             23
++E: 7.170005 0001 0017 0001	# EV_KEY / KEY_I                1
++E: 7.170005 0000 0000 0000	# ------------ SYN_REPORT (0) ----------
 === modified file 'tests/unit-tests/CMakeLists.txt'
 --- tests/unit-tests/CMakeLists.txt	2015-09-07 11:54:56 +0000
 +++ tests/unit-tests/CMakeLists.txt	2015-09-17 09:09:28 +0000
@@ -25,6 +25,7 @@
    ${GBM_INCLUDE_DIRS}
    ${UMOCKDEV_INCLUDE_DIRS}
++  ${PROJECT_SOURCE_DIR}/include/cookie
    ${PROJECT_SOURCE_DIR}/src/include/platform
    ${PROJECT_SOURCE_DIR}/src/include/server
    ${PROJECT_SOURCE_DIR}/src/include/client
@@ -67,6 +68,7 @@
    test_shared_library_prober.cpp
    test_lockable_callback.cpp
    test_module_deleter.cpp
++  test_mir_cookie.cpp
+ )
  if (MIR_TEST_PLATFORM STREQUAL "mesa-x11")
 === added file 'tests/unit-tests/test_mir_cookie.cpp'
 --- tests/unit-tests/test_mir_cookie.cpp	1970-01-01 00:00:00 +0000
 +++ tests/unit-tests/test_mir_cookie.cpp	2015-09-17 09:09:28 +0000
@@ -0,0 +1,105 @@
++/*
++ * Copyright © 2015 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: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com>
++ */
++
++#include "mir_toolkit/mir_client_library.h"
++#include "mir/cookie_factory.h"
++
++#include "mir_test_framework/headless_test.h"
++#include "mir_test_framework/connected_client_with_a_surface.h"
++#include "mir/test/doubles/wrap_shell_to_track_latest_surface.h"
++#include "mir/shell/shell_wrapper.h"
++#include "mir/test/validity_matchers.h"
++#include "mir/test/wait_condition.h"
++
++#include "boost/throw_exception.hpp"
++
++#include <gtest/gtest.h>
++
++namespace mtf = mir_test_framework;
++namespace mtd = mir::test::doubles;
++namespace msh = mir::shell;
++
++TEST(MirCookieFactory, attests_real_timestamp)
++{
++    std::vector<uint8_t> secret{ 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff, 0xde, 0x01 };
++    auto factory = mir::cookie::CookieFactory::create_from_secret(secret);
++
++    uint64_t mock_timestamp{0x322322322332};
++
++    auto cookie = factory->timestamp_to_cookie(mock_timestamp);
++
++    EXPECT_TRUE(factory->attest_timestamp(cookie));
++}
++
++TEST(MirCookieFactory, doesnt_attest_faked_timestamp)
++{
++    std::vector<uint8_t> secret{ 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff, 0xde, 0x01 };
++    auto factory = mir::cookie::CookieFactory::create_from_secret(secret);
++
++    MirCookie bad_client_no_biscuit{ 0x33221100, 0x33221100 };
++
++    EXPECT_FALSE(factory->attest_timestamp(bad_client_no_biscuit));
++}
++
++TEST(MirCookieFactory, timestamp_trusted_with_different_secret_doesnt_attest)
++{
++    std::vector<uint8_t> alice{ 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff, 0xde, 0x01 };
++    std::vector<uint8_t> bob{ 0x01, 0x02, 0x44, 0xd8, 0xee, 0x0f, 0xde, 0x01 };
++
++    auto alices_factory = mir::cookie::CookieFactory::create_from_secret(alice);
++    auto bobs_factory   = mir::cookie::CookieFactory::create_from_secret(bob);
++
++    uint64_t mock_timestamp{0x01020304};
++
++    auto alices_cookie = alices_factory->timestamp_to_cookie(mock_timestamp);
++    auto bobs_cookie = bobs_factory->timestamp_to_cookie(mock_timestamp);
++
++    EXPECT_FALSE(alices_factory->attest_timestamp(bobs_cookie));
++    EXPECT_FALSE(bobs_factory->attest_timestamp(alices_cookie));
++}
++
++TEST(MirCookieFactory, throw_when_secret_size_to_small)
++{
++    std::vector<uint8_t> bob(mir::cookie::CookieFactory::minimum_secret_size - 1);
++    EXPECT_THROW({
++        auto factory = mir::cookie::CookieFactory::create_from_secret(bob);
++    }, std::logic_error);
++}
++
++TEST(MirCookieFactory, saves_a_secret)
++{
++    std::vector<uint8_t> secret;
++    unsigned secret_size = 64;
++
++    mir::cookie::CookieFactory::create_saving_secret(secret, secret_size);
++
++    EXPECT_EQ(secret.size(), secret_size);
++}
++
++TEST(MirCookieFactory, timestamp_trusted_with_saved_secret_does_attest)
++{
++    uint64_t timestamp   = 23;
++    unsigned secret_size = 64;
++    std::vector<uint8_t> secret;
++
++    auto source_factory = mir::cookie::CookieFactory::create_saving_secret(secret, secret_size);
++    auto sink_factory   = mir::cookie::CookieFactory::create_from_secret(secret);
++    auto cookie = source_factory->timestamp_to_cookie(timestamp);
++
++    EXPECT_TRUE(sink_factory->attest_timestamp(cookie));
++}
 === modified file 'tools/update_package_abis.sh'
 --- tools/update_package_abis.sh	2015-08-07 05:46:34 +0000
 +++ tools/update_package_abis.sh	2015-09-17 09:09:28 +0000
@@ -16,6 +16,7 @@
      libmirplatform:MIRPLATFORM_ABI \
      libmirprotobuf:MIRPROTOBUF_ABI \
      libmirserver:MIRSERVER_ABI \
++    libmircookie:MIRCOOKIE_ABI \
      mir-client-platform-android:MIR_CLIENT_PLATFORM_ABI \
      mir-client-platform-mesa:MIR_CLIENT_PLATFORM_ABI \
      mir-platform-graphics-android:MIR_SERVER_GRAPHICS_PLATFORM_ABI \