Code review comment for lp:~mir-team/mir/attestable-timestamps-server

+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*;)