Mir

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

Revision history for this message
Chris Halse Rogers (raof) wrote :

> 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

« Back to merge proposal