Mir

Merge lp:~mir-team/mir/lazy-cookie-secret-creation into lp:mir

Proposed by Brandon Schaefer on 2016-02-26
Status: Work in progress
Proposed branch: lp:~mir-team/mir/lazy-cookie-secret-creation
Merge into: lp:mir
Diff against target: 88 lines (+20/-9)
3 files modified
src/cookie/authority.cpp (+18/-7)
tests/acceptance-tests/test_server_startup.cpp (+1/-1)
tests/unit-tests/test_mir_cookie.cpp (+1/-1)
To merge this branch: bzr merge lp:~mir-team/mir/lazy-cookie-secret-creation
Reviewer Review Type Date Requested Status
Daniel van Vugt Disapprove on 2016-02-29
Mir CI Bot continuous-integration Needs Fixing on 2016-02-27
Cemil Azizoglu (community) 2016-02-26 Needs Fixing on 2016-02-26
Review via email: mp+287372@code.launchpad.net

Commit message

Delay creating the secret so we dont block on /dev/random on boot.

Description of the change

Delay creating the secret so we dont block on /dev/random on boot.

Tested by:

You can check the entropy pool:
watch -n 0.1 cat /proc/sys/kernel/random/entropy_avail

PrePatch (ie. bug)
In a ssh session or tty1 "cat /dev/random" // eats up all the entropy
log into unity8 (Black screen and nothing!)

With this patch:
cat /dev/random again
log into unity8 (while smashing on the keys/mouse to attempt to get the secret created)
login works!

Note there was also discussion to remove create_saving, which we can remove. Just wanted to create this quick fix with out breaking API/ABI of the libcookie. Ill be creating a branch soon to remove that create function + a function that returns the secret.

To post a comment you must log in.
3347. By Brandon Schaefer on 2016-02-26

* Enable the regession tests

Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3346
https://mir-jenkins.ubuntu.com/job/mir-ci/424/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/231
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/255
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/247
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/247
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/238
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/238/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/238
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/238/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/238
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/238/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/238
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/238/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/238
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/238/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/424/rebuild

review: Approve (continuous-integration)
Cemil Azizoglu (cemil-azizoglu) wrote :

16 + BOOST_THROW_EXCEPTION(std::logic_error("Secret size " + std::to_string(passed_secret.size()) + " is to small, require " +

1. Line too long.
2. s/"to small"/"too small"

review: Needs Fixing
3348. By Brandon Schaefer on 2016-02-26

* Fix throw

Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:3347
https://mir-jenkins.ubuntu.com/job/mir-ci/425/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/233/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/257
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/249
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/249
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/240/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/240/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/240
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/240/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/240
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/240/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/240/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/425/rebuild

review: Needs Fixing (continuous-integration)
Mir CI Bot (mir-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Daniel van Vugt (vanvugt) wrote :

You're only deferring the problem and not solving it. This branch simply moves the hang/crash to a later location where the regression tests don't cover it any more.

So you need new regression tests to cover the new code here, and to prove that at no stage during executing does Mir hang indefinitely or crash due to lack of entropy.

Needs fixing: No test coverage

also Disapprove: The best you can achieve this with approach is to move the hang+crash to a later location. And that's still not acceptable.

review: Disapprove

Unmerged revisions

3348. By Brandon Schaefer on 2016-02-26

* Fix throw

3347. By Brandon Schaefer on 2016-02-26

* Enable the regession tests

3346. By Brandon Schaefer on 2016-02-26

* Delay making the secret if no one wants one right away

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/cookie/authority.cpp'
2--- src/cookie/authority.cpp 2016-01-29 08:18:22 +0000
3+++ src/cookie/authority.cpp 2016-02-26 22:51:56 +0000
4@@ -123,13 +123,15 @@
5 class AuthorityNettle : public mir::cookie::Authority
6 {
7 public:
8- AuthorityNettle(mir::cookie::Secret const& secret)
9+ AuthorityNettle() = default;
10+
11+ explicit AuthorityNettle(mir::cookie::Secret const& passed_secret)
12 {
13- if (secret.size() < minimum_secret_size)
14- BOOST_THROW_EXCEPTION(std::logic_error("Secret size " + std::to_string(secret.size()) + " is to small, require " +
15+ if (passed_secret.size() < minimum_secret_size)
16+ BOOST_THROW_EXCEPTION(std::logic_error("Secret size " + std::to_string(passed_secret.size()) +
17+ " is too small, require " +
18 std::to_string(minimum_secret_size) + " or greater."));
19-
20- hmac_sha1_set_key(&ctx, secret.size(), secret.data());
21+ set_secret(passed_secret);
22 }
23
24 virtual ~AuthorityNettle() noexcept = default;
25@@ -137,6 +139,9 @@
26
27 std::unique_ptr<mir::cookie::Cookie> make_cookie(uint64_t const& timestamp) override
28 {
29+ if (secret.empty())
30+ set_secret(get_random_data(optimal_secret_size()));
31+
32 return std::make_unique<mir::cookie::HMACCookie>(timestamp, calculate_cookie(timestamp), mir::cookie::Format::hmac_sha_1_8);
33 }
34
35@@ -183,6 +188,12 @@
36 }
37
38 private:
39+ void set_secret(mir::cookie::Secret const& passed_secret)
40+ {
41+ secret = passed_secret;
42+ hmac_sha1_set_key(&ctx, secret.size(), secret.data());
43+ }
44+
45 std::vector<uint8_t> calculate_cookie(uint64_t const& timestamp)
46 {
47 std::vector<uint8_t> mac(mac_byte_size);
48@@ -204,6 +215,7 @@
49 }
50
51 struct hmac_sha1_ctx ctx;
52+ mir::cookie::Secret secret;
53 };
54
55 size_t mir::cookie::Authority::optimal_secret_size()
56@@ -227,6 +239,5 @@
57
58 std::unique_ptr<mir::cookie::Authority> mir::cookie::Authority::create()
59 {
60- auto secret = get_random_data(optimal_secret_size());
61- return std::make_unique<AuthorityNettle>(secret);
62+ return std::make_unique<AuthorityNettle>();
63 }
64
65=== modified file 'tests/acceptance-tests/test_server_startup.cpp'
66--- tests/acceptance-tests/test_server_startup.cpp 2016-02-26 03:37:46 +0000
67+++ tests/acceptance-tests/test_server_startup.cpp 2016-02-26 22:51:56 +0000
68@@ -68,7 +68,7 @@
69 });
70 }
71
72-TEST(ServerStartupReliability, DISABLED_can_start_with_low_entropy)
73+TEST(ServerStartupReliability, can_start_with_low_entropy)
74 { // Regression test for LP: #1536662 and LP: #1541188
75 using namespace ::testing;
76
77
78=== modified file 'tests/unit-tests/test_mir_cookie.cpp'
79--- tests/unit-tests/test_mir_cookie.cpp 2016-02-26 03:37:46 +0000
80+++ tests/unit-tests/test_mir_cookie.cpp 2016-02-26 22:51:56 +0000
81@@ -119,7 +119,7 @@
82 Ge(mir::cookie::Authority::minimum_secret_size));
83 }
84
85-TEST(MirCookieAuthority, DISABLED_given_low_entropy_does_not_hang_or_crash)
86+TEST(MirCookieAuthority, given_low_entropy_does_not_hang_or_crash)
87 { // Regression test for LP: #1536662 and LP: #1541188
88 using namespace testing;
89

Subscribers

People subscribed via source and target branches