DEV Community: Dimitri Merejkowsky

Classes vs Types (or Yet Another Reason To Learn Rust)

Dimitri Merejkowsky — Thu, 23 Jun 2022 10:11:47 +0000

Introduction

Note: this is both a follow-up to the do classes suck or do classes suck articles, and yet another reason why you should learn Rust, so you should have read at least one of them in order to understand the context for this post.

Back to the spec

As a reminder, here’s our spec:

When robots come off the factory floor, they have no name.
The first time a robot boots, a random name is generated.
Every once in a while, the robots are reset to their factory settings, and the next time they boot, they get a new random name.

Using Rust

The goal of Rust is to allow to code to be 3 things at once: correct, fast, and expressive. This is no easy task and that’s why Rust is a bit complicated to learn (which is not such a bad thing in my opinion, but I digress).

Anyway, what’s interesting with the Rust programming language is that we can express the spec in such a way that invalid states lead to compilation errors and without using anything but structs and methods.

Here’s how. Since the specs talks about names that may or may not exist, we’re going to use two different types - one for the unnamed robots, and one for the robots that have a name. To be precise, those are called structs:

// We wrap the robot name inside a struct
pub struct NamedRobot {
  name: String,
}

// We create a brand new type for "robots that don't have names"
pub struct UnnamedRobot;

// Note : interestingly, this struct costs *nothing* to allocate and is
// known as a zero-sized type (ZST) in Rust parlance.

Then we can express the transitions between allowed states as public methods on those types (inside a impl block) or as free functions

Note: that most of the bodies are omitted here, but you can find tho whole source code on github

// Note: this is a private implementation detail, so no
// `pub` here!
fn generate_random_name() -> String { /* ... */ }

pub fn new_robot() -> UnnamedRobot { /* ... */ }

impl UnnamedRobot {
    // Note: emit a compiler warning if users call start() without
    // using the return value
    #[must_use]
    pub fn start(self) -> NamedRobot {
      let name = generate_random_name();
      NamedRobot { name }
    }
}

impl NamedRobot {
    pub fn name(&self) -> &str { /* ... */ }

    pub fn stop(&self) { /* ... */ }

    pub fn start(&self) { /* ... */ }

    // Note: taking ownership of `self` here!
    pub fn reset(self) -> UnnamedRobot { /* ... */ }
}

Valid code compiles of course:

let robot = new_robot();
let robot = robot.start();
let name = robot.name();
println!("New robot with name: {name}");

And invalid code won’t compile. For instance:

let robot = new_robot();
let name = robot.name()
// Error: method name() not found for UnnamedRobot

You can reset a robot, restart it and get a new name:

let robot = new_robot();
let robot = robot.start();
let name1 = robot.name().to_string();

let robot = robot.reset();

let robot = robot.start();
let name2 = robot.name().to_string();
assert_ne!(name1, name2);

You may wondering why we need .to_string() here. Well, let me explain.

Owning and borrowing

I mentioned earlier that reset takes ownership of the robot.

That’s because the method uses self as first parameter.

In contrast, the name method uses &self and borrows the robot.

This matters because in addition to having a rich type system as we saw above, the Rust compiler also enforce rules about ownership.

Here are those rules:

Each value in Rust has an owner.
There can only be one owner at a time.
At any given time, you can have either one mutable reference or any number of immutable references.

What this means is that when the robot is started, you can access the robot name any time you want, as long as you don’t try to modify it. But, once reset() is called, the value has been moved, and thus you can no longer access the robot name.

Let’s this in practice:

let robot = new_robot();
let robot = robot.start();
let name1 = robot.name().to_string();
let name2 = robot.name().to_string();
robot.reset();
let name3 = robot.name();

// Error: value `robot` moved during called to reset reset()

If Rust had allowed this code to compile, we would have a name3 variable after the robot has been reset, which is not allowed by our spec!

Note that this also means we can’t have name1 and name2 be merely string references (&str).

That’s why we use the .to_string() method, which gives us as a mutable copy (a String) for the robot name.

Conclusion

If you learn Rust, you’ll find that:

There’s more to Object-Oriented Programming than just classes
The borrow checker, when used well, can prevent a bunch of mistakes at compile time.

Side note: I used two different types to make a point. You will find a more idiomatic version of the code in the aptly named “idiomatic” branch on GitHub.

The again show again how the borrow checker rules prevent invalid states at compile time, this time because some methods (like start or reset) use &mut self and other just &self (like name). It also shows the “new type” pattern, in order to enforce robot name format at runtime.

I'd love to hear what you have to say, so please feel free to leave a comment below, or check out my contact page for more ways to get in touch with me.

Is Rust worth learning? Part 1: logs and secrets

Dimitri Merejkowsky — Tue, 04 Jan 2022 10:11:47 +0000

Originally published on my blog

A language that doesn’t affect the way you think about programming, is not worth knowing. – Alan J. Perlis - Yale University

To see if Rust is worth trying, then, let me tell you a story - based on real events.

Logs and secrets

Let’s say you are writing a Java application that needs to make HTTP calls on an external API.

To do this, you have a client class that implements a call method.

public class Client {
  private void authenticate(String appSecret) {
    //
  }

  private void doRequest(String url) {
    //
  }

  public void call(String appSecret, String url) {
    authenticate(appSecret);
    doRequest(url);
  }
}

Note that you need do send an app secret along the url to make the call, hence the separate authenticate() method.

You also have a Config record to hold the configuration of the application:

public record Config(String appSecret, String url) {
}

Finally, you have a main class that reads the values from the environment, builds a Config and a Client instances and uses it to make calls:

public class App {
  public static void main(String[] args) {
    String appSecret = System.getenv("APP_SECRET");
    String url = System.getenv("API_URL");
    Config config = new Config(appSecret, url);

    Client client = new Client();
    client.call(config.appSecret(), config.url());
  }
}

Since logs are important, you also add a call to logger.info() when making the call:

public void call(String appSecret, String url) {
  authenticate(appSecret);
  logger.info("Making the call"); // <- new
  doRequest(url)
}

A vulnerability happens

You push the code into production, and some time later you get assigned to the following bug in the issue tracker:

BUG: APP_SECRET found in the logs
SEVERITY : critical
PRIORITY : high

The logs sent by the application at startup contains the
APP_SECRET

...
INFO: config: Config[appSecret=s3cret, url=https://api.dev]
...
INFO: Making the call

So you take a look at the code base, and sure enough, you find the problem: someone from an other team added a log containing the contents of the Config class:

// SomewhereElse.java

logger.info("config:" + config.toString());

Sure, the bug is easy to fix. You can just remove the call to logger.info.

But after thinking about it more, you decide to also override the toString() method of the Config struct so that the app secret is always redacted:

public record Config(...) {
+ @Override
+ public String toString() {
+ return String.format("Config[appSecret:REDACTED, url:%s]", this.url);
+ }
}

There. You can mark the bug as “fixed” and move to something else, like take a look at this new programming language called Rust …

A few hours later, here’s what you learned.

Ownership

First, Rust has this notion of ownership: every piece of data must have exactly one owner. By default, data is moved and can’t be used after the move.

Here’s an example:

struct StringOwner {
   inner: String, // <- note: fields are private by default
}

fn main() {
  let s = String::from("hello"); // Creates a new string

  let owner = StringOwner { inner: s }; // Move the string into
                                        // the 'StringOwner' struct

  println!("{}", s); // Compile error: cannot use the string after it's moved
}

Second, if you don’t want to move the value, you have to “borrow” it.

fn borrow_the_string(s: &String) { // < Note the '&'
   //
}

fn main() {
  let s = String::from("hello"); // Creates a new string

  borrow_the_string(&s); // Note the '&'

  println!("{}", s); // OK
}

This is a immutable (or shared) borrow: you won’t be able to modify the string.

You’ve also learned about mutable (or exclusive) borrows. They would allow you to modify the string, but they are not relevant for this story.

The trait system

You also learned that classes don’t exist in Rust. Instead, Rust uses traits, and adds methods to structs using impl blocks:

/* in stuff.rs */

// This says that the Stuffer trait
// contains a method named do_stuff
pub trait Stuffer {
   fn do_stuff(&self);
}

struct Foo {
  // ...
}

// This says that Foo implements the Stuffer trait,
// and than compilation will fail if do_stuff() is not present
// or has not the proper signature
impl Stuffer for Foo {
  fn do_stuff(&self) {
    // Implementation here
  }
}

The trait must be in the scope where you are using it:

use stuff::Foo;

let foo = Foo::new():
foo.do_stuff(); // Error: Stuffer trait is not in scope


use stuff::Foo;
use stuff::Stuffer; // <- new

let foo = Foo::new():
foo.do_stuff(); // OK: Stuffer trait is in scope

A rewrite

So, feeling adventurous, you decide to try and re-implement your application in Rust, just to feel like how the code would look like and if you can find a better way of handling the security issue you had to fix in Java.

You start with the Config class:

struct Config {
  url: String,
  app_secret: String,
}

Then you add the Client class:

struct Client {
  // ...
}

impl Client {
  fn authenticate(&self, app_secret: String) {
    // ...
  }

  fn do_request(&self, url: String) {
    // ...
  }

  fn call(app_secret: String, url: String) {
      self.authenticate(app_secret);
      info!("{}", "Making the call");
      self.do_request(url);
  }
}

And finally, the main() function:

fn main() {

    let app_secret = std::env::var("APP_SECRET").unwrap();
    let url = std::env::var("API_URL").unwrap();

    let config = Config { app_secret, url };

    let client = Client;
    client.call(config.app_secret, config.url);
}

“Well, that was easy” you think. “I wonder what people mean when they’re talking about ‘fighting the borrow checker’”.

Then you try to use client.call a second time:

fn main() {

    let client = Client;
    client.call(config.app_secret, config.url);
    // ...
    client.call(config.app_secret, config.url); // <- new
}

error[E0382]: use of moved value: `config.app_secret`
  --> src/main.rs:27:17
   |
23 | client.call(config.app_secret, config.url);
   | ----------------- value moved here
...
27 | client.call(config.app_secret, config.url);
   | ^^^^^^^^^^^^^^^^^ value used here after move

“Oh, right”, I need to “borrow” the app_secret and the url in the client if I want to be able to keep using the config struct.

So you change the code to be like this instead:

  impl Client {
- fn call(&self, app_secret: String, url: String) {
+ fn call(&self, app_secret: &str, url: &str) {
          //
      }

  }

- client.call(config.app_secret, config.url);
+ client.call(&config.app_secret, &config.url);

There. Now the client borrows the app secret and the url and the code compiles and runs fine.

Feeling confident, you try and reproduce the logging issue.

First, you add a #[derive(Debug)] annotation on top of the Config struct:

+ #[derive(Debug)]
  struct Config {

  }

And then you add a log displaying the contents of the config struct:

  fn main() {
    let config = Config { app_secret, url };
+ info!("config: {:?}", &config);
   }
}

This code works because Rust knows how to print debug representations of strings and booleans and the derive(Debug) annotation can automatically generates the code to print a debug representation of the Config struct.

Indeed, the code compile, and, sure enough, the secret shows up in the log:

[INFO] config: Config { app_secret: "s3cret", url: "https://api.dev" }
[INFO] Making the call to https://api.dev

OK, you’ve reproduced the problem. But can you find a better solution this time?

Time to think

You’ve heard good things about Rust type system, so you wonder: “Would it be possible to solve the problem by adding a new type?”.

You start with a SecretString struct:

struct SecretString {
    inner: String,
}

impl SecretString {
    fn new(inner: String) -> Self {
        Self { inner }
    }
}

The constructor takes ownership of the string, which is a good thing, because it means the inner value can no longer be accessed once the SecretString struct is created:

let secret_value = std::env::var("APP_SECRET").unwrap();
let app_secret = SecretString::new(secret_value):

dbg!(secret_value); // < does not compile!

Then you change the type of app_secret in Config from String to SecretString:

struct Config {
- app_secret: String,
+ app_secret: SecretString,
}

Which means you have to get the inner value when authenticating the client:

impl Client {
- fn call(&self, app_secret: &str, url: &str) {
- self.authenticate(&app_secret);
+ fn call(&self, app_secret: &SecretString, url: &str) {
+ self.authenticate(app_secret.inner);
+ }

}

You are then faced with 2 compile errors:

First, the Debug macro no longer works:

error[E0277]: `SecretString` doesn't implement `std::fmt::Debug`
  --> src/main.rs:26:5
   |
24 | #[derive(Debug)]
   | ----- in this derive macro expansion
25 | struct Config {
26 | app_secret: SecretString,
   | ^^^^^^^^^^^^^^^^^^^^^^^^ `SecretString` cannot be formatted using `{:?}`
   |
   = help: the trait `std::fmt::Debug` is not implemented for `SecretString`

To fix this, you implement the debug trait yourself:

use std::fmt::Debug;
// ^-- new - remember: traits needs to be in scope
// in order to use them

impl Debug for SecretString {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        write!(f, "REDACTED")
    }
}

Second, you can’t use the inner field directly in the call() method:

error[E0616]: field `inner` of struct `SecretString` is private
  --> src/client.rs:13:39
   |
13 | let secret_value = app_secret.inner;
   | ^^^^^ private field

So you add a public method to access the inner value:

impl SecretString {
    pub fn expose_value(&self) -> &str {
        &self.inner
    }
}

And you fix the client code:

pub fn call(&self, app_secret: &SecretString, url: &str) {
    let secret_value = app_secret.expose_value(); // <- new!
    self.authenticate(&secret_value);
}

Going further

Then you think back about the rule about traits having to be in scope, and you decide to move the expose_value method into a trait:

pub trait ExposeSecret {
    fn expose_value(&self) -> &str;
}

impl ExposeSecret for SecretString {
// ^-- used to be `impl SecretString`

    fn expose_value(&self) -> &str {
        &self.inner
    }
}

And then the compiler says:

error[E0599]: no method named `expose_value` found for reference
`&SecretString` in the current scope
  --> src/client.rs:13:39
   |
13 | let secret_value = app_secret.expose_value();
   | ^^^^^^^^^^^^

   = help: items from traits can only be used if the trait is in scope
   = help: the following trait is implemented but not in scope; perhaps add a
           `use` for it:

use crate::secrets::ExposeSecret;

So you comply:

use crate::secrets::ExposeSecret; // <- new!
use crate::secrecy::SecretString;

There - the code compiles, and it’s now pretty hard to leak the app secret:

Indeed, if a value has been wrapped in the SecretString struct, anyone attempting to access it must:

call a method that sounds dangerous (expose_value())
import a trait that also sounds dangerous (secrets::ExposeSecret)

It’s also very easy to edit the code for safety: just list the usages of the ExposeSecret trait.

Conclusion

I hope you found this post interesting: it shows how you can use some unique features of the Rust programming language to tackle an old issue in a novel way.

I mentioned at the beginning that the this post is based on true stories, so here are my two sources of inspiration:

The issue of a secret value exposed by mistake is based on an actual security vulnerability I reported to the Miniflux maintainers. You can see the details in the miniflux repository.
The SecretString struct is based on a real crate, called secrecy.

Thanks for taking this journey with me, and if you’re a reading this while trying to fix the log4j security vulnerability, you have my sincere sympathy :)

Cheers!

I'd love to hear what you have to say, so please feel free to leave a comment below, or check out my contact page for more ways to get in touch with me.

Merge first, ask questions later - Optimistic Merging in practice

Dimitri Merejkowsky — Tue, 14 Sep 2021 14:06:46 +0000

Introduction - Pessimistic Merging

If you’ve contributed to an open-source project on GitHub or GitLab, or written code for a company, you’re probably familiar with the concept of “Pessimistic Merging”. You write a patch (or a pull request or a merge request), but it is not merged (or applied) right away. Instead, you need to wait for human approval to be given and/or for continuous integration to pass.

I can think of two reasons why this strategy is often the one which is used:

It’s been used by really big projects (like the Linux kernel) for quite some time
It’s kind of the default way to collaborate on platforms like GitHub or GitLab

There are even tools like Gerrit which work by specifying a list of constraints before a patch can be merged ¹ - which is used among other things by Google’s Android Open Source Project or the Qt framework.

There’s also the fact that it allows to enforce some rules. Used in combination with source control software (like Git), Pessimistic Merging can ensure:

A “clean” git history (from the commit messages to their contents to the size of the patches)
A consistent coding style
A test suite that always pass on the main development branch
A code that is composed only of patches that have been reviewed and approved by an arbitrary number of people
… and so on

And indeed, there are some projects where this matters a lot - in a corporate environment, or in a “sensitive” project like a browser, a database engine or an operating system …

That being said, we rarely talk about the downsides of Pessimistic Merging - which is why you probably never thought that this concept needed a name.

Issues with Pessimistic Merging

For this, I’ll let you read the article which prompted me to write this one: Why Optimistic Merging Works Better².

The idea behind Optimistic Merging is very simple: you merge the patch first, and then deal with issues like broken builds, code style violation and other problems.

Sure, this means that the main development branch of your Git repository may now contain commits that do not fully work, code that does not always compile, or commits that only fix code styling errors.

But for a small or medium-sized project, that does nothing too security-sensitive, and for which maintainers and collaborators are hard to find - is it such a bad deal?

On this topic, see also Aleksey Kladov’s piece on Two Kinds of Code Review (which is the reason I learned about Optimistic Merging in the first place and wanted to try it out), where he explains how pull requests are reviewed and merged for the rust-analyzer project.

A note before I continue: they are plenty of cases where using Pessimistic Merging is the best strategy - here I’m just describing an alternate strategy you may find interesting.

With that out of the way, let’s now look how Optimistic Merging works for me in practice.

My workflow for Optimistic Merging

Setting the stage

I’m the maintainer of a project called “foo”, written in Python, and hosted in GitHub.

The project uses flake8 as a linter to catch small mistakes, the code is formatted with black, and it has has some tests (of course). It is also very easy to run the linters, the tests and the code formatter from the developer machine (this will matter later on).

Here comes a developer I never heard of (let’s call them Mallory) - and they open their first ever merge request to the GitHub repository.

Merging Mallory’s changes

The first step is for me to get the proposed changes on my machine and to prepare a list of notes for later feedback - a simple text file is enough.

Since it’s a PR on GitHub, I can do something like this, where “some-branch” is the source ref of Mallory’s PR:

$ git remote add mallory git@github.com:mallory/foo
$ git fetch mallory
From github.com:mallory/foo
* [new branch] some-branch -> mallory/some-branch

Then I rebase some-branch on top of the main branch. This allows me to make sure the commits can be merged without conflicts.

During the rebase I look at each commit diff and I take notes.

Since everything is on my machine, it’s trivial to amend Mallory’s commits if a linter complains or to run black if I need to.

I can also choose to run the tests locally if I fear a commit may have caused a regression.

Finally, if there’s a small issue (like a misleading comment) I can fix it right away! There’s no need to go through the hassle of:

telling Mallory they did something wrong,
waiting for them to update the merge request,
re-starting the process from scratch.

When all of this is done, it’s time to push Mallory’s branch. I re-run the linters and the tests just to be sure, then I push the code and leave a message in the PR discussion telling Mallory the changes have been merged.

Looking at the interaction from Mallory’s point of view

It’s very likely that Mallory was not paid to write the patch, (this is a medium-size open-source project, after all) but they still worked on it and thought their work was good enough to open a pull request.

They probably don’t know much about the project, and it’s likely they don’t know everything about Git, Python, flake8 or black. And it’s even possible they know nothing about testing!

Yet, the first reply they got for the maintainer is “Thanks! Your commits have been merged”. Boom - the collaboration is already working - what a great start!

Contrast with a typical response when using Pessimistic Merging, which would look like this:

Thank you for your contribution, but the CI does not pass (test_foo_bar4 is failing and there are some lint errors). Also, please merge or rebase your work on top of the main branch so that the merge is fast-forward.

… followed by a bunch of nitpicks about the merge request.

After the merge

Anyway, now it’s time to put my notes to good use.

If there are some trivial changes I had to do, I can just tell Mallory:

It was a good idea to refactor the do_stuff() method inside the Foo class, but you did not adapt the doc string, which I’ve done in commit …

Again, we’re working together here, and I’m willing to bet Mallory is more likely to watch their docstrings next time they write a new patch :)

If there was some non-trivial changes (like a missed opportunity for a refactoring), I can just create an issue with a task list for instance.

I can also start a pull request and have Mallory review it.

Conclusion

To be frank, I’ve just started implementing this strategy for my open-source projects, so it’s a bit early to draw any conclusions. That being said, so far it has been going great, and I wanted to share the idea of Optimistic Merging right way.

I’ll let you know how it goes in the future. Until then, happy coding!

I'd love to hear what you have to say, so please feel free to leave a comment below, or check out my contact page for more ways to get in touch with me.

Fun fact: to do this, Gerrit embeds an implementation of the Prolog Language ↩
If you have trouble with opening this link, make sure you are using the http:// URL, the https:// version is unfortunately broken. ↩

Two awesome Rust warnings

Dimitri Merejkowsky — Tue, 02 Feb 2021 13:35:21 +0000

Originally published on my blog.

Introduction

If you’ve ever used a compiler (or any other tool) that can produce warnings about your code, you may be used to messages that are hard to understand, confusing or just plain wrong.

But today I want to show you that good compiler warnings do exist and how they can make your code more correct.

The incorrect server API

The first example comes from code I wrote to test the Rust implementation of the Tanker client SDK. You don’t need to know precisely how Tanker works to follow this example - let’s just say the client needs a “Tanker permanent identity” from an identity server to start a session. ¹

In my case, the identity server was already written in Go using the Gin-Gonic framework.

Here’s what the implementation of the sign_in route looked like:

// This struct is used by various Gin handlers
type UserResponse struct {
    UserID string `json:"user_id"`
    Email string `json:"email"`
    PermanentIdentity string `json:"tanker_permanent_identity"`
}

// handler for the /sign_in route
func (this *Server) signIn(c *gin.Context) {
    var body SignInBody
    jsonErr := c.BindJSON(&body)
    if jsonErr != nil {
        c.JSON(400, gin.H{"error": jsonErr.Error()})
        return
    }

    email := body.Email
    password := body.Password

    // Not shown: check email and password, fetch user from the db

    userResponse := UserResponse{
        Email: user.Email,
        UserID: user.UserID,
        PermanentIdentity: user.PermanentIdentity,
    }

    c.JSON(200, userResponse)
}

To parse the JSON response from the server in the Rust client, I used serde, like this:

#[derive(Deserialize)]
struct UserResponse {
    user_id: String,
    email: String,
    tanker_permanent_identity: String,
}

impl Client {
    pub async fn sign_in(&mut self) -> Result<()> {
        let email = ...;
        let password = ...;

        let mut data = HashMap::new();
        data.insert(String::from("email"), email);
        data.insert(String::from("password"), password);

        let response = self.client.post("/sign_in", data).await?;
        let user_response = response.json::<UserResponse>().await?;

        self.start_tanker(&user_response).await?;
    }

    async fn start_tanker(&self, user_response: &UserResponse) -> Result<()> {
        let private_identity = &user_response.tanker_permanent_identity;
        let email = &user_response.email;
        let status = self.tanker.start(&private_identity).await?;
        // Not shown: verify identity by email when required

        Ok(())
   }
}

When I first compiled the code, I got this warning:

Warning: field is never read: `user_id`
  --> src/notepad.rs:24:5
   |
24 | user_id: String,
   | ^^^^^^^^^^^^^^^
   |
   = note: `#[warn(dead_code)]` on by default

This made sense: as you can see, to start a Tanker session we only need the email and tanker_permanent_identity fields of the JSON response returned by the server. In particular, we don’t need the user ID at all.

What’s interesting with this example is that at Tanker, we already wrote six other clients for the server in question (using Python, Go, Java, Javascript, Objective-C and Ruby) and that was the first time we got an indication that our JSON API was wasting bandwidth by sending information that was actually not needed!

The useless mutation

The second example comes from a personal project - a week ago I started implementing an interpreter in Rust, following the instructions from the Writing an Interpreter in Go, by Throsten Ball.

As I worked on the the lexer, I wrote the following code:

use std::iter::Peekable;
use std::str::CharIndices;

#[derive(Debug)]
pub(crate) struct Lexer<'a> {
    code: &'a str,
    iter: Peekable<CharIndices<'a>>,
}

impl<'a> Lexer<'a> {
    pub fn new(code: &'a str) -> Self {
        Self {
            iter: code.char_indices().peekable(),
            code,
        }
    }

    // ...

    /// Try to read an identifier starting at start_pos
    fn read_identifier(&mut self, start_pos: usize) -> &'a str {
        // end_pos is either `None` if we reached the end of the input code,
        // or Some(index) if we reached a character that is not part of
        // of a valid identifier

        let mut end_pos = Some(start_pos);
        loop {
            // Is the next character valid?
            let peek = self.iter.peek();
            match peek {
                Some((next_pos, next_c)) if Self::is_valid_ident(*next_c) => {
                    // Yes: advance next_pos
                    end_pos = Some(*next_pos);
                    self.iter.next();
                }
                Some((next_pos, _)) => {
                    // No: advance next_pos and exit the loop
                    end_pos = Some(*next_pos);
                    break;
                }
                None => {
                    // There's no next character : we've reached the end
                    // of the input code
                    end_pos = None;
                    break;
                }
            }
        }

        match end_pos {
            // end_pos is None: return all the code from start_pos to the end
            None => &self.code[start_pos..],
            // end_pos is not None: return the code from start_pos to end_pos
            Some(index) => &self.code[start_pos..index],
        }
    }
}

Here’s what the compiler had to say:

warning: value assigned to `end_pos` is never read
   --> src/lexer.rs:131:13
    |
131 | let mut end_pos = Some(start_pos);
    | ^^^^^^^^^^^
    |
    = note: `#[warn(unused_assignments)]` on by default
    = help: maybe it is overwritten before being read?

warning: value assigned to `end_pos` is never read
   --> src/lexer.rs:136:21
    |
136 | end_pos = Some(*next_pos);
    | ^^^^^^^
    |
    = help: maybe it is overwritten before being read?

The second warning was the easiest to fix: we keep setting end_pos toSome(*next_pos) but never read it until the loop exits. So we can just remove the line that sets end_pos before calling self.iter.next():

    fn read_identifier(&mut self, start_pos: usize) -> &'a str {
        let mut end_pos = Some(start_pos);

        loop {
              match peek {
- Some((next_pos, next_c)) if Self::is_valid_ident(*next_c) => {
- end_pos = Some(*next_pos);
+ Some((_, next_c)) if Self::is_valid_ident(*next_c) => {
                      self.iter.next();
                  }
                  Some((next_pos, _)) => {
                      end_pos = Some(*next_pos);
                      break;
                  }
                  None => {
                      end_pos = None;
                      break;
                  }
              }
        }

But the compiler was still unhappy:

warning: value assigned to `end_pos` is never read
   --> src/lexer.rs:131:13
    |
131 | let mut end_pos = Some(start_pos);
    | ^^^^^^^^^^^
    |
    = note: `#[warn(unused_assignments)]` on by default
    = help: maybe it is overwritten before being read?

This took me a while to realize, but the compiler is telling me that I can declare the end_pos binding but not initialize it right away:

    fn read_identifier(&mut self, start_pos: usize) -> &'a str {
- let mut end_pos = Some(start_pos);
+ let end_pos;
        loop {
              match peek {
                  Some((_, next_c)) if Self::is_valid_ident(*next_c) => {
                      end_pos = Some(*next_pos);
                      self.iter.next();
                  }
                  Some((next_pos, _)) => {
                      end_pos = Some(*next_pos);
                      break;
                  }
                  None => {
                      end_pos = None;
                      break;
                  }
              }
        }

I love this : the end_pos binding is no longer mut, and can only be assign a value in two cases: either in the None arm of the match peek block, or to end_pos when the peek character is not valid.

It’s much less likely for the code to be buggy!

Conclusion

I hope those two examples gave you an idea of what it’s like to write code using a language that has such a focus on correctness and how nice it is to have useful warnings.

For now, Rust is the only language I’ve used which gave me this feeling of “the compiler has my back”, but I’m sure they are others - please tell me bellow if you know one.

And if this gave you a motivation to try and learn Rust, go for it!

I'd love to hear what you have to say, so please feel free to leave a comment below, or check out my contact page for more ways to get in touch with me.

More info in the Tanker documentation ↩

dmerej.info: now on the Gemini space

Dimitri Merejkowsky — Thu, 19 Nov 2020 16:10:22 +0000

Hi there,

Just a quick note to let you know my personal website is now available on the Gemini space too.

Get a Gemini client and see you at gemini://dmerej.info!

But Why?

Good question! - the answer is only available on the Gemini version of the blog, though …

The quest for a better hiring process

Dimitri Merejkowsky — Wed, 22 Jul 2020 15:20:36 +0000

The quest for a better hiring process

I’ve been a software engineer for about ten years, and here’s something I learned: hiring software engineers is hard. One of the main challenges is to get the most information about the candidates in the minimum amount of time. You want to know if they will be a good fit for your team, and you want to know it quickly, without wasting their time.

In this article, I’ll go through a list of the various techniques I’ve seen, describe the one we are using at Tanker, and why it works best.

A tour of existing techniques

The whiteboard

Let’s start with the infamous whiteboard. You ask the candidates to write some code from scratch on a whiteboard to solve a given problem. This can give you an idea of how they think or how they approach new challenges, but bear in mind the whiteboard can get very intimidating for some developers, so they may appear less bright than usual.

All in all, the whiteboard is a peek into the reasoning abilities of the candidate. Still, it doesn’t give any information about the ability to work in a team, understand a new codebase, or their skills with a given programming language.

The homework

Another technique is to ask the candidate to implement a project “from scratch” at home. This technique avoids some of the above pitfalls, and you can quickly gauge the candidate’s level of competence.

Note that you only get to see how they work alone and when writing code from scratch. Moreover, either it means a big-time investment for the candidate or the project is too small to correctly judge their skills.

None of these techniques match our needs: we are a team with a flat organization in which each member has the right to contribute to important decisions. Besides, we already have a pretty large codebase, so future recruits will mostly work on existing code.

For these reasons, we use a refactoring exercise, a technique I learned about during an episode of Ruby Tapas presented by Nickolas Means.

As the name implies, the candidates will have to refactor some code. They also will have to demonstrate other abilities like reasoning about new features and dealing with tests.

Here’s how it works.

The Refactoring Exercise

Before meeting the candidate

First, take a simple problem and implement it. The code does not have to be very long, but you should make sure there’s room for improvement: leave out some duplication, use functions with lots of parameters, or other anti-patterns. However, do use features you want the candidate to be familiar with.

Then write some basic tests and think about a new feature to add, but don’t implement it. This will be important later on.

Running the exercise

Remember, your goal is to gather the most valuable information about the candidates: you want to see how they work when they are at their best.

For instance, it’s crucial to tell the candidate about the refactoring exercise, so it’s not a surprise for them to make sure they bring their own development machine.

When the candidate arrives, show them the code and tell them about the problem it’s solving.

At this point, the code should be in a state that makes it hard to implement the new feature without refactoring.

Then announce the new requirements and wait. Make it clear that the code should be production-ready.

Do they ask for existing tests?
Do they try and refactor first?

Your goal is to see if they think about those “good practices” without a prompt from your end. But if they start writing the new code directly, it’s not a red flag per se.

You do want to see how they refactor and use tests, though, so if they skip all of the above, just gently nudge them in the right direction with questions like “Is there something you want to do before implementing the new feature?” or remarks like: “By the way, there are some tests if you think that’s helpful.”

Your goal is to keep them comfortable, so don’t make them think they have failed if they did not ask about tests or refactoring right away. It happens.

The rest is straightforward: watch the candidate do the refactor, implement the solution, and add tests. Note that the whole session should last about 30 minutes.

What running the exercise tells you

Let’s recap what you’ve learned during the refactoring session:

You have a pretty good idea about the candidates’ skills and knowledge in your programming language
You can see how they make decisions about adding features, adding tests, and architecture of their code
You make sure that they can convert requirements into code
You know how it would be like to pair-program with them

At Tanker, we’ve been using this technique for quite some time, and it has worked very well. It gives us a good idea of how candidates will do all day, every day — and all of that in 30 minutes — not bad!

Interested in joining us? Have a look at our job openings.

Automating version number updates: what could go wrong?

Dimitri Merejkowsky — Thu, 11 Jun 2020 09:35:17 +0000

Automating version number updates: what could go wrong?

Say you need to update (bump) your software. It’s currently at version 1.2, all the required changes have been merged, and it’s time to publish version 1.3. That’s really easy, right? Change the version in one file, commit, tag, and push. Done!

I thought that too once, but the truth is that it’s harder than it looks — let me tell you my story.

Releasing software for Softbank robotics

Our story begins in 2008, when I was release manager at Softbank Robotics.

I had two big projects to release: Choregraphe, a desktop GUI to control the NAO and Pepper robots, and qiBuild, a command-line application to ease C++ development.

For qiBuild, I had three files to patch because the code version number was hard-coded in several places (setup.py, __init__.py, and docs/conf.py).

On the other hand, for Choregraphe the version number was hard-coded in the top CMakeLists.txt file only, but there was quite a bit of code to “forward” the version number from the top CMake file down to the version.hpp and main.cpp files.

Both solutions had their pros and cons but I could not decide which one was best. Since I was pretty sure I was not the first one to have encountered this issue, I started to look around for better solutions.

Trying out bumpversion

The way bumpversion works is a nice combination of the two approaches I’ve mentioned before:

First, keep the hard-coded version number in as many files as required
Second, add a dedicated configuration file (.bumpversion.cfg) containing the current version and the aforementioned list of files.

Then, when bumping the project from version X to version Y:

Iterate over the file list
Replace all occurrences of X with Y, including in the configuration file itself.

So, I started with the qiBuild project and added the following configuration file:

# in .bumpversion.cfg
[bumpversion]
current_version = 1.0.1
commit = True
tag = True

[bumpversion:file:setup.py]

[bumpversion:file:qiBuild/__init__.py]

[bumpversion:file:docs/conf.py]

Since I had to bump qiBuild from 1.0.1 to 1.0.2, I ran:

bumpversion patch

A commit was automatically made along with a matching tag:

$ git show
commit ec50897893ce4ecfb1debaa1df266ae4c555f45b (HEAD -> master, tag: v1.0.2)
Author: Dimitri Merejkowsky <dimitri.merejkowsky@tanker.io>
Date: Wed May 20 16:58:27 2020 +0200

Bump version: 1.0.1 → 1.0.2
diff --git a/.bumpversion.cfg b/.bumpversion.cfg
-------- a/.bumpversion.cfg
+++ b/.bumpversion.cfg
 [bumpversion]
-current_version = 1.0.1
+current_version = 1.0.2
 commit = True
 tag = True

diff --git a/qiBuild/__init__.py b/qiBuild/__init__.py
-------- a/qiBuild/__init__.py
+++ b/qiBuild/__init__.py
-__version__ = “1.0.1”
+__version__ = “1.0.2”

diff --git a/setup.py b/setup.py
-------- a/setup.py
+++ b/setup.py
 setup(
    name="qiBuild",
-   version="1.0.1",
+   version="1.0.2",
     )

diff --git a/docs/conf.py b/docs/conf.py
-------- a/docs/conf.py
+++ b/docs/conf.py
project = "qiBuild"
- version="1.0.1",
+ version="1.0.2",

Great! Now I just had to run python setup.py sdist upload and the 1.0.2 release was published on pypi.org.

For the NAOqi project, it worked very well too.

I could delete a bunch of CMake and preprocessor code and replace it with just one line of C++ code:

// in naoqi/version.hpp
static std::string const NAOQI_VERSION = "2.3.0";

This time the .bumpversion.cfgfile looked like this:

[bumpversion]
current_version = 2.3.0
files = include/naoqi/version.hpp

Now bumping naoQI’s version could be done with a command similar to the one used to bump qiBuild.

Brilliant! So, what went wrong?

Wandering off-track

The problem can be described as being “too smart by half” and has to do with the command line syntax of bumpversion.

Indeed, bumpversion is smart enough to bump various “parts” of the version number, namely the major, minor, and patch components used in the semver spec.

Here are some examples, assuming that the current version is 1.2.3:

bumpversion patch : 1.2.3 -> 1.2.4
bumpversion minor : 1.2.3 -> 1.3.0
bumpversion major : 1.2.3 -> 2.0.0

We were using semver for qiBuild and NAOqi too at the beginning — but sometimes semver is not enough.

Let’s continue our story. When qiBuild got more usage and the software team grew, publishing qiBuild releases started becoming… scary.

New features were added, refactorings were made and qiBuild had become an essential tool for all the developers in the software team (100 of them) — I was getting nervous about what would happen if I shipped a buggy release.

So, with the help of members of my team, I decided to start making release candidates. That way, a few brave colleagues could help me catch bugs before everyone upgraded to the latest stable version.

Since Python developers had already come up with a version scheme that allowed for release candidates, I started using that. See PEP 440 for details. Basically, I could add a rcX suffix after the patch part.

And… it turned out that doing so was far from trivial. Why?

Well, bumpversion assumes you are using semver and, if you don’t, you need to specify a custom regex:

parse = (?P<major>\d+) 
 (?P<minor>\d+) 
 (\.(?P<patch>\d+))? .
 ((?P<release>rc)(?P<rel_num>\d+))?

But it does not stop there.

Now you need a way to tell bumpversion how to go from 2.0.0rc1 to 2.0.0rc2 (if the release candidate had bugs) or from 2.0.0rc2 to 2.0.1 (if it did not).

So you add even more configuration, but it’s still not enough:

serialize =
 {major}.{minor}
 {major}.{minor}.{patch}
 {major}.{minor}{release}{rel_num}

[bumpversion:part:release]
values =
 a
 b
 rc

You can find all the gory details in the GitHub issue but, in short, it was a show stopper.

I believe that bumpversion is still not used at Softbank Robotics to this day, and, as far as I know, bumping qiBuild still needs version numbers to be fixed manually in several files.

But our story does not end here — that would be a rather sad ending!

Arriving at Tanker

In March 2016, I handed in my resignation letter at Softbank and, three months later, I started my current job at Tanker.

In short, Tanker sells a product that can be used for end-to-end encryption, as well as client-side anonymization, packaged in open source SDKs (see our website for details).

Anyway, given we were releasing software and, because of my past experience, I was also in charge of release management at Tanker.

As you might expect, Tanker also had hard-coded version numbers and chunks of code whose only role was “forwarding” the version number from one file to another.

“This is exactly the same problem I had last time!”, I thought. So, I took a look at bumpversion again — but even after all this time the bug I opened was still not fixed.

That’s when I realized there were two big problems with bumpversion which would be pretty hard to fix without rewriting a lot of code.

First, it’s very hard to reliably identify “parts” of version numbers. Semantics can vary from one version scheme to the next. Even comparing version numbers is a hard task, but guessing how to bump from a release candidate to a stable one is near impossible.

Secondly, there are some hidden defaults at play which make understanding what’s going on under the hood pretty hard.

In other words, bumpversion was “too clever by half”.

So, what to do? Well, rewrite from scratch of course! (It turned out to be a good idea after all — otherwise, I would not brag about it here :P)

The birth of tbump

On December 7, 2017, I started working on a rewrite called tbump. My goal was to keep bumpversion’s good ideas and to fix its shortcomings.

Here are the main differences between tbump and bumpversion:

There are no hard-coded defaults: you must specify how the git message and the tag name will be formatted, and you also need to specify a regular expression to define the version scheme.
Instead of specifying what part of the current version you want to update, you need to pass the whole new version.

Back to our example — to go from 2.1.3 to 2.1.4 you run tbump 2.1.4 instead of bumpversion patch.

Those differences come with a price.

First, since there is no hard-coded default it’s harder to use tbump out of the box.

However, this one was easy to fix : I added an init command to generate a tbump.toml file automatically. Instead of having to read the docs, users can read the generated file and get started quickly.

Secondly, since one has to specify the new version instead of a segment one wants to bump it’s easy to make mistakes, like going from 1.0.3 to 1.0.5 instead of 1.0.4.

That’s where it gets interesting.

You see, I was pretty annoyed by some aspects of the bumpversion UX, especially when trying to tweak the configuration file.

Just watch:

$ bumpversion patch --dry-run
<nothing>
$ bumpversion patch --verbose --dry-run
current_version=1.0.2
commit=True
tag=True
new_version=1.0.3

Now look at what tbump --dry-run does:

$ tbump --dry-run 1.0.3
:: Bumping from 1.0.2 to 1.0.3
=> Would patch these files
- setup.py:3 version="1.0.2",
+ setup.py:3 version="1.0.3",
- foo.py:1 __version__ = "1.0.2"
+ foo.py:1 __version__ = "1.0.3"
- tbump.toml:2 current = "1.0.2"
+ tbump.toml:2 current = "1.0.3"
=> Would run these git commands
$ git add --update
$ git commit --message Bump to 1.0.3
$ git tag --annotate --message v1.0.3 v1.0.3
$ git push origin master
$ git push origin v1.0.3

The new version is all over the place, you can’t miss it, and at the same time, you see exactly what is going on.

The output is similar without the — dry-run option, except changes are actually applied and git commands are run.

And then I realized that every time I was bumping something, I would be following the same pattern:

Run tbump $NEW_VERSION --dry-run
Check if everything was OK by reading the output
If not, back to Step 1
Otherwise, run tbump $NEW_VERSION

This looks like an algorithm — and what do we do with algorithms? We implement them!

So here’s what the entry point of tbump looks like:

# (simplified)
def main():
    bump(dry_run=True)
    answer = input("Looking good (y/n)?")

    if answer != "y":
       sys.exit("Canceled by user")
    else:
       bump(dry_run=False)

That way you get a chance to catch any mistake in the new version just before it’s too late :)

Early adoption

In January 2018 tbump 1.0 was out (and I had been using tbump to bump itself since the 0.2 release).

I published the package on pypi.org and I started using it both for Tanker projects and my own ones.

It worked great! My fellow developers told me they liked the UX so I was pretty happy.

But there was a critical flaw in tbump’s design that would come back to bite us pretty hard in the future. Let’s see how.

yarn workspaces

Quite early in the development of our Javascript SDK, we knew we’d be using a mono-repo.

For instance, we wanted to support both Node.JS (for fast-running tests) and the browser (which is the primary use of the SDK).

So right off, we knew we would have at least three packages: @tanker/core, @tanker/client-browser, and @tanker/client-node.

It did not make sense to have different version numbers for those three packages, so here’s what we ended up with:

// In core/package.json
{
  "name": "@tanker/core",
  "version": "1.2.0",
  // ...
  "dependencies": {
    "libsodium-wrappers": "^0.5.1",
    // ...
  }
}

// In client-browser/package.json
{
  "name": "@tanker/client-browser",
  "version": "1.2.0",
   // ...
  "dependencies": {
    "@tanker/core": "1.2.0",
    // ...
   }
}

// In client-node/package.json
{
 "name": "@tanker/client-node",
 "version": "1.2.0",
  // ...
 "dependencies": {
   "@tanker/core": "1.2.0",
   // ...
  }
}

When bumping to a new version, we needed to patch the line that contains the “version” key in the top metadata:

[[file]]
src = "packages/core/package.json"
search = '"version": "{current_version}"'

[[file]]
src = "packages/client-node/package.json"
search = '"version": "{current_version}"'

[[file]]
src = "packages/client-node/package.json"
search = '"@tanker/crypto": "{current_version}"'

Note the search line: we did not want to blindly replace all occurrences of the new version in packages.json — if a line declares a third-party dependency that happens to have the same version number as the current one, it should not get patched!

Speaking of dependencies, we also needed to patch the line that specifies the version of @tanker/core used by @tanker/client-browser and @tanker/client-node:

[[file]]
src = "packages/client-browser/package.json"
search = '"@tanker/core": "{current_version}"'

[[file]]
src = "packages/client-node/package.json"
search = '"@tanker/core": "{current_version}"'

That’s a total of four blocks of configuration. Then we extracted a @tanker/crypto package from @tanker/core, and added two new blocks of configuration:

[[file]]
# Bump version of @tanker/crypto
src = "packages/crypto/package.json"
search = '"@tanker/core": "{current_version}"'

[[file]]
# Bump version for @tanker/core too — it depends on @tanker/crypto!
src = "packages/core/package.json"
search = '"@tanker/crypto": "{current_version}"'

Unbeknownst to us, that was the start of a slippery slope: every time we added a new package in the workspace, we’d have to add two blocks of configuration for this package, and one for every package that depended on it.

This is a famous anti-pattern, and before you can say “I see a polynomial complexity!”, we ended up with this kind of monstrosity: a 200 hundred lines configuration file!

Saved by my co-workers

Luckily I’m working with a team whose members never hesitate to share (constructive) criticism — and who often take matters into their own hands.

So, when faced with the task of trying to edit this gigantic configuration file just to add a new package, one of them decided to fix the root cause of the problem and submitted a pull request for tbump named “Stars and Dots”. Here are the two main changes it implemented:

Allow regular expressions in search strings: we could now use @tanker/.* instead of specifying the whole name of the dependency (@tanker/core, @tanker/crypto, and so on)
Allow using glob patterns to specify file names. Instead of having one block per file, we could now specify a whole bunch of files using packages/*/packages.json as a glob pattern.

Clever name for a clever pull request, don’t you think?

This pull request was of, course, quickly reviewed and merged by yours truly, and all the nasty blocks in the tbump.toml file were replaced with just two:

[[file]]
src = "packages/**/package.json"
search = '"version": "{current_version}"'

[[file]]
src = "packages/**/package.json"
search = '"@tanker/[^"]+": "{current_version}"'

Note that it remains more or less the same to this day, even if the sdk-js git repository now contains about 30 different packages :)

The cherry on the cake

Before I conclude, I’d like to mention a feature of tbump called hooks. It’s the ability to run arbitrary commands before patching the files or after the commit is made and pushed.

Hooks can run before the files are patched and the commit is made, or after the new commit and new tag have been pushed and are defined in the tbump.toml file.

You can find examples of this in tbump’s own configuration file:

[[before_commit]]
name = "Check Changelog"
cmd = "grep -q {new_version} Changelog.rst"

[[after_push]]
name = "Publish to pypi"
cmd = "tools/publish.sh"

Conclusion

tbump is now the deployment tool for Tanker.

We often use “before commit” hooks to perform various checks (like verifying that the version to be published is mentioned in the changelog), or to make sure that generated files are up-to-date (like yarn.lock for instance).

We also use “after push” hooks as “executable documentation” to specify how to publish new releases (like using poetry publish in case of a Python package).

So, what did we learn?

First, pay attention to algorithmic complexity, even in configuration files, take care in designing a good UX, even for command-line tools,
Second, if you can afford an extra pair of eyes to a given problem, don’t hesitate to ask.
Finally, there’s a battle-tested rewrite of bumpversion with a pretty good UX and nice features waiting for you on pypi.org. Feel free to try it out and tell us what you think.

Cheers!

PS: We’re hiring software engineers. If the way we work sounds interesting, have a look at the hack challenge available on https://www.tanker.io/rabbit/ and don’t hesitate to reach out!

"It's a waste of time!"

Dimitri Merejkowsky — Sun, 31 May 2020 09:01:53 +0000

Originally published on my blog.

I have a confession to make : every time I heard the phrase "It's a waste of time!", I get a little angry.

I've heard this answer countless times, often after suggestions like:

"Let's write tests before writing production code"
"Let's write documentation before writing code"
"Let's refactor before implementing the new feature"
"Let's discuss the API once more before shipping the next release."

There are several reasons for why I don't like this answer.

Why? It's all about knowledge.

Time spent vs time saved

First, it's easy to see the time spent.

Yes, we spent one week writing and reviewing the new documentation. Yes, we spent three days refactoring the code and we did not implement any feature. Yes, we had two one-hour meetings before getting consensus on the new API and shipping the next release.

But it's really hard to see the time saved.

Who knows what horrible bug we would have to fix in production if we skipped writing those tests? Who knows how much breaking changes we would have made because the API was not good enough? Who knows how much time we would have spent implementing the next big feature if we skipped the refactoring?

Learning about the problem

Second, you may have noticed that in all the situations I mentioned above, it's all about gaining more knowledge about the problem before jumping to the implementation.

In other terms, by saying "It's a waste of time", you assume you know enough about the problem to start working on the implementation right away.

Trying to convince others

Most of the time, I can't convince people who tell me that I'm suggesting is a waste of time.

Instead, they skip what I think was a critical preparatory step, and then I watch them spend a ton of time rectifying the situation afterwards - it's frustrating.

Fortunately, after they've repeated the same mistake enough times, they tend to finally listen to me. But I'd very much like to not have to go through this painful process every time.

I'm not sure what to do about it, though. Maybe it's just human nature?

To make matters worse sometimes writing the code is actually a good way to gain this precious knowledge in a way that neither tests nor documentation nor brainstorms could have done.

Questions for you

What do you think about all this? How do you react when confronted to the "It' a waste of time!" argument?

Feel free to share your ideas below, and see you next time!

Tips for better Python tests

Dimitri Merejkowsky — Sat, 25 Apr 2020 14:20:47 +0000

Originally published on my blog.

Today, I'd like to share with you a list of small tips to help you write better tests when using Python. Note that the tips often improve both the readability of the test implementation, and of the failure messages (which is pretty important too).

Use pytest

Pytest does a great job in generating very good error messages from your assertions, without the need for anything other than the assert statement.

Take this assertion for instance:

assert foo(x) == bar(y)

Here's what the failure will look like when using pytest if the assertion fails:

    def test_foobar():
        x = 2
> assert foo(x) == bar(x)
E assert 4 == 5
E + where 4 = foo(2)
E + and 5 = bar(2)

test.py:11: AssertionError

Note how much information you get about what went wrong, like the entire body of the function up to the assertion that failed, or when the values that were compared came from.

It gets even better - take this other assertion:

assert get_message() == "this is what I expected"

And note the nice, detailed diff between the two strings that gets printed:

    def test_get_message():
>         assert get_message() == "this is what I expected"

E AssertionError: assert 'this is what I expected' == 'this is what I got'
E - this is what I expected
E ? ^^^^^ --
E + this is what I got
E ? ^^^

A more complex example

For the rest of this post, let's assume you are testing a sync_folders() function that can synchronize a remote folder with a local one.

Here's one of the tests you wrote:

def test_can_update_local_file(remote, local):
    local_file = local / "a.txt"
    local_file.write_text("old contents")
    new_contents = "new contents"
    remote.add_file("a.txt", contents=new_contents)

    sync_folders(remote, local)

    actual_contents = local_file.read_text()
    assert actual_contents == new_contents

Quick tip: I usually split my tests into three arrange /act / assert parts and I visualize them using vertical space.

Add context to assertions

Did you know you can add a string at the end of the assert statement?

def test_sync(remote, local):
    ...

    actual_contents = local_file.read_text()
-   assert actual_contents == new_contents"
+   assert actual_contents == new_contents, "a.txt should have been updated"

This way, instead of having to read this:

E AssertionError: assert 'old contents\n' == 'new contents'
E - old contents
E + new contents

you get this message, which contains a clue about where the diff actually comes from:

E AssertionError: a.txt should have been updated
E assert 'old contents\n' == 'new contents'
E - old contents
E + new contents

Reduce noise

You can also improve the signal over noise ratio by using pytest.fail instead of the assert statement:

- assert actual_contents == new_contents", "a.txt should have been updated"
+ if actual_contents != new_contents:
+     pytest.fail("a.txt should have been updated")

    if actual_contents != new_contents:
>        pytest.fail("a.txt should have been updated")
E        Failed: a.txt should have been updated

Be careful when using this technique, because it may hinder the debugging of failing tests.

Use custom assertion helpers

Finally, don't hesitate to factorize code about assertions, for instance in a test/helpers.py file:

def assert_was_updated(path, contents):
    ...

def assert_was_created(path):
    ...

# and so on

Use docstrings to describe tests scenarios

This is my favorite tip ever: if you are testing something complex, add a human-readable description of the test inside the docstring.

Still using our test_can_update_local_file example:

def test_can_update_local_file():
    """ Scenario:
    * Create a file named a.txt in the local folder with
      "old" contents
    * Add a new version of the `a.txt` file in the remote folder
    * Synchronize the remote and local folders
    * Check that `a.txt` has been updated
    """
    ....

There are two advantages to this approach:

It can be helpful to have a human-readable description of what the test is supposed to be doing when reading the implementation of the code - like any docstring
Since pytest displays the entire block of the function block that caused the assertion to fail, you get a reminder of what the test was about when reading the failure message.

Reflections on the last tip

You can stop reading there if you want, but I thought it would be interesting to know how I end up using docstrings in my test code - especially since for a long time, I was convinced that docstrings were useless if the test implementation was clear enough¹!

What changed my mind? In two words: code review. let me elaborate.

Getting feedback

I've had the chance to get my Python test code reviewed by some teammates who did not know pytest very well but were used to frameworks like Mocha or Cucumber. They help me realize this simple truth: using only function names and implementation (in other words, code) to express all the subtlety of what the tests are about cannot be enough - kind of obvious when you say it like that, right?

But in this case code review can only see you what needs to be improved, but not always how.

So I did what I had to: I took a closer look at those other frameworks.

Getting to know other frameworks

Here's an implementation of our test using Mocha:

describe('sync', function() {
  it('syncs a remote file', function() {
    remote.addFile('a.txt', { contents: newContents });

    syncFolders(remote, local);

    const actualContents = local.join('a.txt').readText();
    assert.equal(actualContents, newContents);
  });
});

And here's an implementation using Cucumber:

# in synchronization.feature
Feature: Synchronization

Scenario: file updated remotely
  Given there is a local file 'a.txt' containing "old_contents"
  Given there is a remote file 'a.txt' containing "new_contents"
  When I synchronize the folders
  Then the local file a.txt contains "new_contents"

# in synchronization.rb
Given(/there is a local file '{word}' containing "{string}"/ do |path, contents|
  open(path, 'w') do |f|
      f.puts contents
  end
end

When(/I synchronize the folder/) do
  sync_folders(@local, @remote)
end

# ...

Quite different styles, right?

Finding a middle ground

And there you have it: I came up with using docstrings with pytest because it was a nice middle ground between those two approaches.

With Mocha, you write short descriptions in it() and describe() and there's no such thing as a "test name".
With Cucumber, you write long and detailed text using the English natural languages.

I prefer the docstring solution to the ones above because:

docstrings can be as long as you want (you rarely see descriptions larger than one line in Mocha tests)
contrary to Cucumber, you are not forced to use any formatting or syntax in your description, and they live right next to the accompanying code

Conclusion

So what did we learn?

Being reviewed helps to keep your code readable - but you already knew that, right?
Exploring new languages and frameworks gives you insights on how to improve your code
If you are in a team that contains people who use different languages and frameworks than yours and you get them to review your code, it will lead to even better code because you'll be combining the two effects from above!

In other terms, consider increasing the diversity of your teams, and don't hesitate to explore new things 😎.

I'd love to hear what you have to say, so please feel free to leave a comment below, or check out my contact page for more ways to get in touch with me.

I also believed good code did not need comments - fortunately, I read this article. ↩

symlinks and .so files on linux - what you need to know

Dimitri Merejkowsky — Sat, 18 Apr 2020 14:03:19 +0000

Originally published on my blog.

The issue

I've seen this happen countless times.

First, a Linux user asks for help about an error looking like this:

Error when running `bar`: `libfoo.so.5: no such file or directory`

Then, someone who's had the same issue suggests creating a symlink from libfoo.so.6 to libfoo.so.5.

And then I come across the dialog some time later and I start crying.

Why do I cry - it looks like the problem is solved, right?

the bar program seems to run just fine;
the change was made with just one command (sudo ln) and can be undone easily (just remove the symlink);
there was already a symlink from libfoo.so to libfoo.so.6 anyway!

Well, despite the appearances, the problem is not solved, and doing this is a terrible idea in general - like planting a ticking time bomb in the street you live or shooting yourself in the foot because you've got a plantar wart.

But to understand why we need to talk about the language C, shared libraries, soname bumps, Linux distributions, and package management.

And because I love telling stories, you, dear reader, will be the heroin this one.

Solving the Ultimate Question

Let's assume a group of people you don't really know (let's called them_The Experts_), wrote a piece of C code than can get the answer to the Ultimate Question of Life, the Universe, and Everything.

For obvious reasons, they want to keep the source code private, so here'swhat they did:

They wrote a header file containing the get_answer declaration named answer.h¹

// in answer.h
#pragma once
#ifdef __cplusplus
extern "C" {
#endif

char* get_answer();

#ifdef __cplusplus
}
#endif

They created a shared library called libanswer.so from their source file (answer.cin this case):

$ gcc -shared libanswer.so answer.c

That way, everyone who needs to get the answer to the Ultimate Question of Life, the Universe, and Everything can buy the libanswer.so compiled library and the answer.h header and call the get_answer() function - let's see how.

Using the library from The Experts

You've bought the libanswer.so and answer.h files from The Experts and have put them next to a file you've wrote named print-answer.c:

// in print-answer.c
#include <stdio.h>
#include <answer.h>

int main() {
  char* answer = get_answer();
  printf("The answer is %s\n", answer);
  return 0;
}

You were told that gcc can compile C code and link against shared libraries if you put them on the command line, so you try and run this:

$ gcc libanswer.so print-answer.c -o print-answer

But it does not work, and you get the following error message:

print-answer.c:2:10: fatal error: answer.h: No such file or directory

Wait a minute - the answer.h file is right there - what does that mean, “No such file or directory”?.

After a bit of research, you discover that gcc uses a list of paths called the “include path” where it looks for headers. You check on your machine, and sure enough, /usr/include/ is one of the elements of this list, and stdio.his in /usr/include/stdio.h, which explains why gcc did not complain about the first include.

To fix the compilation error, you add the -I . option to the command line so that current directory is added to the list of include paths:

$ gcc -I . libanswer.so print-answer.c -o print-answer

And then it compiles.

Now you try and run the print-answer program, but you get a new error message:

$ ./print-answer
 error while loading shared libraries: libanswer.so
 cannot open shared object file: No such file or directory

It's the same error message as in the introduction and the operating system is lying to us. The file libanswer.so is right there! What's happening there?

After more investigation, you figure it out: when you compiled the print-answer executable, there was a small piece of binary inside it that recorded the name of the .so file it was linked against. You can check it by running the readelf command and display the dynamic section of your program:

$ readelf -d ./print-answer
Dynamic section at offset 0x2de8 contains 27 entries:
  Tag Type Name/Value
 0x0000000000000001 (NEEDED) Shared library: [libanswer.so]
 0x0000000000000001 (NEEDED) Shared library: [libc.so.6]
 ...

In a way, the print-answer program “knows” that it needs libanswer.so to run. Crucially, it does not know nor cares about where libanswer.so really is.²

Then, when you run ./print-answer, the operating system sees the name of the shared library in the dynamic section and tries to locate it. Like gcc, it finds libc.so.6 by itself (in/usr/lib/libc.so.6 for instance) - but it's unable to find the libanswer.so shared library in the current directory.

Fortunately, you can fix that by using a special environment variable called LD_LIBRARY_PATH:

$ LD_LIBRARY_PATH=. ./print-answer
The answer is 42

This time, the operating system looks for libanswer.so in the current directory, finds it, and when required, invokes the code for the get_answer() function from the shared library.

That gets you thinking - you did not have to do any of this for print-answer to find thelibc.so library and the stdio.h header.

What if it was possible to compile libanswer.so once and for all?
And what if there was a way to compile the print-answer.c source file without having to copy/paste the header and the shared library, and remember all the various gcc options?

Becoming a packager

Let's assume The Experts realized that their business model was not going to work and decided to publish their source files for free instead. Yay open source!

Here's the contents of their answer.c file:

#include <answer.h>
#include <stdio.h>
#include <string.h>

char * get_answer() {
  // Disappointing, I know
  int r = 6 * 7;
  char buf[3];
  snprintf(buf, 3, "%d", r);
  return strdup(buf);
}

Since you are an Arch Linux user, you decide to lookup documentation about the pacman package managerand how generate and publish Arch Linux packages³.

After a while, you manage to write a working PGKBUILD for libanswer:

pkgname=libanswer
pkgver=1.0
pkgrel=1
pkgdesc="Answer the Ultimate Question"
arch=('x86_64')
...

build() {
  gcc -I . -shared answer.c -o libanswer.so
}

package ()
{
  mkdir -p $pkgdir/usr/{include,lib}
  install answer.h $pkgdir/usr/include/
  install libanswer.so $pkgdir/usr/lib
}

Then you build the package with makepkg

$ makepkg
==> Making package: libanswer 1.0-1
...
==> Finished making: libanswer 1.0-1

Everything goes well, and you now have a file named libanswer-1.0-1-x86_64.pkg.tar.xznext to your PGKBUILD.

You install the package with pacman:

$ sudo pacman -U libanswer-1.0-1-x86_64.pkg.tar.xz

Then you find out that you can compile and run print-answer.c from anywhere in the system using the following commands:

$ gcc print-answer.c -lanswer -o ./print-answer
$ ./print-answer
The answer is 42

This time you need only the name of the library (the part without thelib prefix and the .so suffix) after the -l option. You do not have to worry about include paths or use the LD_LIBRARY_PATH environment variable - neat!

What you've accomplished is called packaging the answer library, and its what package maintainers do - well done.

Impressed by your packaging skills⁴, the Arch Linux maintainers allow you to publish your package in official repositories, which means everyone using Arch Linux is now able to install the libanswer package in just one command!

A simple change

A few days later, The Experts release a new version of their library (1.1), containing a nice optimization:

char* get_answer() {
  return strdup("42");
}

Since you are the maintainer of the libanswer package, you quickly update the PKGBUILD and publish a new release.

-pkgver=1.0
+pkgver=1.1
 pkgrel=1
 pkgdesc="Answers the Ultimate Question"
 arch=('x86_64')
 source=(...)

You build and install the package on your machine and check it still works:

$ makepg
$ sudo pacman -U libanswer-1.1-1-x86_64.pkg.tar.xz

And then you publish the new version of the libanswer package.

That's where Linux distributions really shine (and the whole reason we use shared libraries in the first place). Once the new package is published, any Arch Linux user who installs it will get the latest version of libanswer.so in their system, and all the programs that were linked against it will use the latest version - this is especially important if the new version contains a security bug fix for instance.

Another change

A week later, The Experts realize they don't really need to return a string from the get_answer() function, and that a simple int would suffice.

So they modify both their header and source files:

- char* get_answer();
+ int get_answer();

#include <answer.h>

int get_answer() {
  return 42;
}

And they publish a release note:

# The answer project

## Version 2.0

* Change the return type of the `get_answer()` function from `char*` to `int`.

## Version 1.1

* This release implements performance optimizations.

## Version 1.0

* First public release!

Upon hearing the good news, you download the latest sources of the project, and you modify your print-answer.c source file to use the latest version of the library:

#include <stdio.h>
#include <answer.h>

int main() {
  int answer = get_answer();
  printf("The answer is %d\n", answer);
}

You compile everything and check the code still works:

$ gcc -I . -shared answer.c -o libanswer.so
$ gcc -I . libanswer.so print-answer.c -o ./print-answer
$ LD_LIBRARY_PATH=. ./print-answer
The answer is 42

Time to publish the v2!

-pkgver=1.0
+pkgver=2.0
 pkgrel=1
 pkgdesc="Answers the Ultimate Question"
 arch=('x86_64')
 source=(...)


$ makepg
$ sudo pacman -U libanswer-2.0-1-x86_64.pkg.tar.xz

Easy as pie - satisfied, you publish the new package and go to bed.

When shit hits the fan

The next morning you receive the following e-mail:

Subject : latest libanswer package update broke the display-answer-pp program

Hello,

I'm using `display-answer-pp` version 0.4, and when I updated
`libanswer` to the version 2.0, I got the following error:

./display-answer-pp
zsh: segmentation fault (core dumped) ./display-answer-pp

Downgrading the `libanswer` package fixes the problem. Please advise.

Signed: Bob

Welcome to the joys of packaging!

You've never heard of the display-answer-pp package - what is going on?

After a bit of research, you find out that someone wrote a display-answer-pp program using your libanswer package and published it on the official Arch Linux repositories a few days ago.

Here's what the code for display-answer-pp looks like - it's a single C++ file:

#include <answer.h>
#include <iostream>

int main() {
    auto answer = get_answer();
    std::cout << "The answer is: " << answer << std::endl;
    return 0;
}

So what happened?

Well, the problem is that you forgot to coordinate with your fellow packagers!

You see, when display-answer-pp was being packaged, the get_answer() function was returning a char*. When you published answer version 2.0, the code for the function get_answer() started returning anint instead.

So when Bob updated libanswer after having installed display-answer-pp, and tried to re-run the program, all hell break loose, because the compiled C++ code expected a pointer to a string and got an int instead.

In other terms, the application binary interface (or ABI for short) of the libanswer library broke.

You break it, you fix it

Unfortunately, there's only one way to fix an ABI breakage: you need to recompile everything that was linked against the old version of the library.

That's one of the main issues package maintainers have to solve. They need two very different features when it comes to libraries updates:

If the new version of the library is ABI-compatible with the previous one, end-users should be able to get it by simply updating the one package that contains it.
If not, they need to make sure none of the programs that depend on the library break when the update is made.

The Arch Way

Here's how Arch maintainers solve this problem. In our example, they would have published libanswer v2 in a special repository named “staging”. Then, they would have rebuild every package that depends on libanswer (so both print-answer and display-answer-pp) and pushed those to the staging repository.

Finally, after a period of testing, they would have moved libanswer, print-answer and display-answer-pp in the official repositories in one swift update. They would have used a to do list like this one to coordinate the packaging tasks.

This means that if you try and update libanswer without upgrading every other package that depends on it, you risk breaking your installation - and that's why partial updates are not supported on Arch Linux.

The Debian Way

Debian maintainers use another strategy. When they package a library, they include its version number in the name of the package. What's more, they have a separate development package that contains the files required for compiling programs that use the library. They also use a compilation trick called the soname option.

Let's see how this works.

First, they tell gcc to use the correct soname option when linking the library⁵

$ gcc -I . answer.c -shared -Wl,-soname=libanswer.so.1 -o libanswer.so.1

They use the outcome of the build to generate two packages:

libanswer-dev, that contains a symlink libanswer.so -> libanswer.so.1 and the answer.h header
and libanswer1, that contains only the libanswer.so.1 file

Then, they build display-answer-pp for the first time, using the libanswer-dev package:

$ g++ -lanswer display-answer.cpp -o ./display-answer-pp

Because libanswer.so was built with the -soname=libanswer.so.1 option, the display-answer-pp binary now knows that it needs libanswer.so.1 at runtime:

$ readelf -d display-answer-pp
Tag Type Name/value
0x0000000000000001 (NEEDED) Shared library: [libanswer.so.1]
...

When the v2 version of libanswer is published by The Experts, they rebuild the shared library with an appropriate soname:

$ gcc -I . answer.c -shared -Wl,-soname=libanswer.so.2 -o libanswer.so.2

They use the outcome of the build to update the libanswer-dev package and to create a brand new libanswer2 package.

At this point:

libanswer-dev contains a symlink libanswer.so -> libanswer.so.2 and the updated answer.h header
libanswer2 contains only the libanswer.so.2 file

Then, they build display-answer-pp for the second time, using the updated libanswer-dev package:

$ g++ -lanswer display-answer.cpp -o ./display-answer

This time, display-answer-pp knows that it needs libanswer.so.2 at runtime:

$ readelf -d display-answer-pp
Tag Type Name/value
0x0000000000000001 (NEEDED) Shared library: [libanswer.so.2]
...

Let's sum up:

Both libanswer1 and libanswer2 packages can co-exist in the same system since they contain different filenames
The libanswer-dev package does not need to be installed fordisplay-answer-pp to run since the program contain the versioned soname in its dynamic section
If you want to rebuild a program when a new version of the library is out, you just install the latest libanswer-dev package - the command used for compilation does not have to change at all!

Clever, right?

Because of this technique, the update from libanswer v1 to libanswerv2 is often called a soname bump - the update from v1 to v1.1 is just a regular update.

Quite often, a backward incompatible update correspond to the first digit of the soname to be updated. ⁶

Conclusion

Now you know - different versions of a library have various sonames, and the symlinks are carefully crafted by distribution maintainers.

So, when you create a symlink yourself, you are taking a huge risk, especially when creating links between libraries that have a different leading digit in their soname!

As we saw, using the incorrect library at runtime can cause crashes, and if you break an essential binary (like bash for instance), you may no longer be able to log in, which means your only choice may be to re-install your whole system from scratch. This is not theoretical by the way: it happened to me years ago, and I still remember it to this day.

So, what to do if you get this error?

Error when running `bar`: `libfoo.so.5: no such file or directory`

If bar comes from a package on your distribution, update all packages and then open a bug report if the problem persists
If not, try and re-compile bar - possibly after updating the libfoo-dev package.

But please, please do not create a symlink from libfoo.so.6 tolibfoo.so.5 or suggest this solution to someone else. Together, let'sput an end to this bad, bad, advice. Thank you!

I'd love to hear what you have to say, so please feel free to leave a comment below, or check out my contact page for more ways to get in touch with me.

If you're puzzled by the __cplusplus stuff don't worry - it's just so that the answer library is usable from C++ too. ↩
By the way, it needs libc.so because the compiled code for printf lives there. ↩
You're using Arch Linux because you're cool and you want to learn - good for you! ↩
Reminder: it's a story I made up! ↩
Actually, this is often done automatically by whatever build system the library is using ↩
Sadly it's not always the case: lua and libpng are notable exceptions. ↩

Taking Mastodon's security to the next level - part 2: Exchange encrypted messages

Dimitri Merejkowsky — Tue, 19 Nov 2019 10:36:16 +0000

Introduction

This is the second article in a 2-part series of blog posts that describe our endeavor to add end-to-end encryption to Mastodon: if you haven’t already, please read Part 1: Encrypt your toots first.
In the rest of this article, we’ll refer to the Javascript code responsible for managing the UI as the client, and the Ruby on Rails code as the server.

We left on a bit of a cliffhanger - we’d managed to encrypt direct messages in the client, but hadn’t yet sent them to the server.

Actually, sending encrypted messages to the server instead of plain text messages will lead to all sorts of interesting challenges and we’ll learn even more about Mastodon’s internals than we did in the first post.

Adding an encrypted field in the database

Since we are encrypting only direct messages, it seems like a good idea to add an encrypted boolean in the database. That way, we’ll know whether statuses are encrypted or not before attempting to decrypt them.

So here’s the plan:

The client should send an encrypted boolean to the server when calling the api/v1/statuses route during the composition of direct messages
The server should store the encrypted status contents in the database, along with an encrypted boolean
The server should send the encrypted text along with the encrypted boolean back to the client.

Let’s write a new migration and migrate the db:

# db/migrate/20190913090225_add_encrypted_to_statuses.rb
class AddEncryptedToStatuses < ActiveRecord::Migration[5.2]
  def change
      add_column :statuses, :encrypted, :bool
  end
end

$ rails db:setup

Then fix the controller:

# app/controllers/api/v1/statuses_controller.rb
class Api::V1::StatusesController < Api::BaseController
  def create
    @status = PostStatusService.new.call(
                current_user.account,
                # ...
                encrypted: status_params[:encrypted])
  end

  def status_params
    params.permit(
       # ...
       :encrypted)
  end

end

Note that the controller deals only with validating the JSON request; the actual work of saving the statuses in the database is done by a service instead, so we need to patch this class as well:

# app/services/post_status_service.rb
class PostStatusService < BaseService
# ...
  def call(account, options = {})
    @encrypted = @options[:encrypted] || false
    # …
    process_status!


  end

  def process_status!
      ApplicationRecord.transaction do
      @status = @account.statuses.create!(status_attributes)
    end
  end


  def status_attributes
    # Map attributes to a list of kwargs suitable for create!
    {
       # …
       :encrypted: @encrypted
   }.compact
  end
end

Let’s write a test to make sure the PostStatus service properly persists encrypted messages:

# spec/services/post_status_service_spec.rb
it 'can create a new encrypted status' do
  account = Fabricate(:account)
  text = "test status update"
  status = subject.call(account, text: text, encrypted: true)
  expect(status).to be_persisted
  expect(status.text).to eq text
  expect(status.encrypted).to be_truthy
end

OK, it passes!

We can now use the new PostStatus API from the client code:

// app/javascript/mastodon/actions/compose.js


export function submitCompose(routerHistory) {
  let shouldEncrypt = getState().getIn(['compose', 'shouldEncrypt'], false);
  let status = getState().getIn(['compose', 'text'], '');

  if (shouldEncrypt) {
   status = await tankerService.encrypt(status);
  }

  api(getState).post('/api/v1/statuses', {
    //
    status,
    encrypted: shouldEncrypt
  });
}

We can check that this works by composing a direct message:

And then checking in the database:

rails db
# select encrypted, text from statuses order by id desc;
encrypted | text
----------+---------------------------------
 t        | A4qYtb2RBWs4vTvF8Z4fpEYy402IvfMZQqBckhOaC7DLHzw…

Looks like it’s working as expected, so it’s time to go the other way around - sending the encrypted boolean from the server to the client.

Displaying encrypted messages in the UI

This time we need to change the status serializer:

# app/serializers/rest/status_serializer.rb
class REST::StatusSerializer < ActiveModel::Serializer
  attributes :id, :created_at, :in_reply_to_id, :in_reply_to_account_id,
             # ...
             :encrypted
end

The Javascript code that fetches the status from the Rails API does not have to change.

That being said, we still want to make it clear in the UI whether the message is encrypted or not - this is useful for debugging.

So let’s update the StatusContent component to display a padlock icon next to any encrypted message:

// app/javascript/mastodon/components/status_content.js
render() {
  const encrypted = status.get('encrypted');
  let contentHtml;
  if (encrypted) {
    contentHtml = '<i class="fa fa-lock" aria-hidden="true"></i>&nbsp;' \
      + status.get('contentHtml');
  } else {
    contentHtml = status.get('contentHtml');
  }

  const content = { __html: contentHtml };
  return (
     // ...
     <div ...>
       dangerouslySetInnerHTML={content} 
     </div>
  );
}

Hooray, it works! We’re ready to call decrypt now.

Decrypt messages

First things first, let’s patch the TankerService to deal with decryption:

// app/javascript/mastodon/tanker/index.js
export default class TankerService {
  // ...

  decrypt = async (encryptedText) => {
    await this.lazyStart();

    const encryptedData = fromBase64(encryptedText);
    const clearText = await this.tanker.decrypt(encryptedData);
    return clearText;
  }
}

Now we’re faced with a choice. There are indeed several ways to decrypt statuses in the client code. For simplicity’s sake, we’ll patch the processStatus function which is called for each message returned from the server:

// app/javascript/mastodon/actions/importer/index.js
async function processStatus(status) {
  // …
  if (status.encrypted) {
    const { id, content } = status;

    // `content` as returned by the server has a <p> around it, so
    // clean that first
    const encryptedText = content.substring(3, content.length-4);
    const clearText = await tankerService.decrypt(encryptedText);
    const clearHtml = `<p>${clearText}</p>`
    dispatch(updateStatusContent(id, clearText, clearHtml));
  }

}

Note that we call an udpateStatusContent action to update the status after it has been decrypted.

I won’t go through the implementation of the updateStatusContent action and reducers as they’re pretty standard.

Anyway, we can check that our patch works by logging in as Alice, and then sending a message to ourselves:

Exchanging private messages

Being able to send encrypted messages to oneself is quite impressive, but I don’t think we should stop there :)

Let’s create a new account for Bob, and look at what happens when Alice sends a message containing @bob - this is known as a mention:

Normally, Bob should get a notification because he was sent a direct message, but this is not the case.

Clearly there is something to fix there.

After digging into the code, here's what I found out: notifications about direct messages are generated by a class named ProcessMentionsService.

Here’s the relevant part of the code:

class ProcessMentionsService < BaseService
  def call(status)
      status.text.gsub(Account::MENTION_RE) do |match|
         mentionned_account = ...
         # …
         mentions <<  \\
           mentionned_account.mentions(...).first_or_create(states)
       end

       mentions.each { create_notification(mention) }
  end
end

We can see that the server looks for @ mentions in the status text using regular expression matches and then builds a list of Mention instances.

Then something interesting happens:

# app/services/process_mentions_services.rb
class ProcessMentionsService < BaseService
   # …
   def create_notification(mention)
    mentioned_account = mention.account

    if mentioned_account.local?
      LocalNotificationWorker.perform_async(
        mentioned_account.id, 
        mention.id, 
        mention.class.name)
    elsif mentioned_account.activitypub?
       ActivityPub::DeliveryWorker.perform_async(
         activitypub_json, 
         mention.status.account_id, 
         mentioned_account.inbox_url)
     end
  end
end

So the server triggers a task from the LocalNotificationWorker if the mentioned account is local to the instance. It turns out this will later use the websocket server we discovered in Part 1 to send a notification to the client.

Side note here: if the mentioned account is not local to the instance, an Activity Pub delivery worker is involved. This is at the heart of the Mastodon mechanism: each instance can either send messages across local users, or they can use the ActivityPub protocol to send notifications across to another instance.

Back to the task at hand: it’s clear now that if the status is encrypted by the time it’s processed by the server, nothing will match and no notification will be created. That’s why Bob didn’t get any notification when we tried sending a direct message from Alice to Bob earlier.

Thus we need to process the @ mentions client-side, then send a list of mentions next to the encrypted status to the server:

//app/javascript/mastodon/actions/compose.js


export function submitCompose(routerHistory) {
// ...
  let mentionsSet = new Set();
  if (shouldEncrypt) {
    // Parse mentions from the status
    let regex = /@(\S+)/g;
    let match;
    while ((match = regex.exec(status)) !== null) {
      // We want the first group, without the leading '@'
      mentionsSet.add(match[1]);
    }

  const mentions = Array.from(mentionsSet);
  api(getState).post('/api/v1/statuses', {
    status,
    mentions,
    encrypted,
  });

}

As we did for the encrypted boolean, we have to allow the mentions key in the statuses controller and forward the mentions array to the PostStatus service:

class Api::v1::StatusesController < Api::BaseController
  def status_params
    params.permit(
      :status,
      # ...
      :encypted,
      mentions: [])
  end


  def create
    @status = PostStatusService.new.call(
                current_user.account,                                                         
                encrypted: status_param[:encrypted],
                mentions: status_params[:mentions])
end

In the PostStatus service we forward the mentions to the ProcessMentions service using a username key in an option hash:

# app/services/post_status_service.rb
class PostStatusService < BaseService
  def process_status!
    process_mentions_service.call(@status, { usernames: @mentions })
  end
end

And, finally, in the ProcessMentions service, we convert usernames into real accounts and create the appropriate mentions:

# app/services/process_mentions_service.rb
class ProcessMentionsService < BaseService
  def call(status, options = {})
    if @status.encrypted?
      usernames = options[:usernames] || []
      usernames.each do |username|
        account = Account.find_by!(username: username)
        mentions << Mention.create!(status: @status, account:account)
      end
   else
     # same code as before
   end
end

Now we can try encrypting the following status: @bob I have a secret message for you and check that Bob gets the notification.

But when Bob tries to decrypt Alice’s message, it fails with a resource ID not found error message: this is because Alice never told Tanker that Bob had access to the encrypted message.

For Bob to see a message encrypted by Alice, Alice must provide Bob’s public identity when encrypting the status. We still have some code to write, because in Part 1 we created and stored only private tanker identities. Luckily, the tanker-identity Ruby gem contains a get_public_identity function to convert private identities to public ones.

So the plan becomes:

Add a helper function to access public identities from rails
When rendering the initial-state from the server, add public identities to the serialized accounts.
In the client code, fetch public identities of the recipients of the encrypted statuses
Instead of calling encrypt with no options, call tanker.encrypt( resource, { shareWithUsers: identities }) where identities is an array of public identities

Good thing we are already parsing the @ mentions client-side :)

Sending public identities in the initial state

First we adapt our TankerIdentity class so we can convert a private identity to a public one:

# app/lib/tanker_identity.rb
def self.get_public_identity(private_identity)
  Tanker::Identity.get_public_identity(private_identity)
end

Then we add the tanker_public_identity attribute to the User class:

class User < ApplicationRecord
  def tanker_public_identity
    TankerIdentity::get_public_identity tanker_identity
  end
end

We tell the Account class to delegate the tanker_public_identity method to the inner user attribute.

# app/models/use.rb
class Account < ApplicationRecord
  delegate :email,
           :unconfirmed_email,
           :current_sign_in_ip,
           :current_sign_in_at,
           ...
           :tanker_public_identity,
           to: user,
           prefix: true
end

We adapt the account serializer:

# app/serializers/rest/account_serializer.rb
class REST::AccountSerializer < ActiveModel::Serializer 
   attributes :id, :username, 
              # ...:
              :tanker_public_identity


def tanker_public_identity
    return object.user_tanker_public_identity
end

And now the client can access the Tanker public identities of the mentioned accounts in the initial state.

Sharing encrypted messages

We can now collect the identities from the state and use them in the call to tanker.encrypt():

export function submitCompose(routerHistory) {
  // ...

  let identities = [];
  const knownAccounts = getState().getIn(['accounts']).toJS();
  for (const id in knownAccounts) {
    const account = knownAccounts[id];
    if (mentionsSet.has(account.username)) {
       identities.push(account.tanker_public_identity);
     }
   }

  // …
  const encryptedData = await tankerService.encrypt(
                                clearText, 
                                { shareWithUsers: identities });

  api(getState).post('/api/v1/statuses', {
  // ...
  });
}

Let’s see what happens after this code change. This time, when Bob clicks on the notification, he sees Alice's decrypted message:

Done!

What did we learn?

We discovered how notifications are handled in Mastodon
We found out that some server-side processing needed to be moved client-side, as is expected when client-side encryption is used.
We implemented a fully working end-to-end encryption feature for Mastodon’s direct messages, making sure direct message can be read only by the intended recipients

If you are curious, here are some statistics about the number of changes we had to write, excluding generated files:

$ git diff --stat \
   :(exclude)yarn.lock \
  :(exclude)Gemfile.lock \
  :(exclude)db/schema.rb
 41 files changed, 360 insertions(+), 40 deletions(-)

Future Work

Reminder: this is a proof of concept, and many things could be improved. Here’s a list of problems and hints about their solutions.

Improve status decryption

We are violating an implicit property of the messages in Mastodon: they are supposed to be immutable, as shown by the fact that until our patch, no action was able to change the contents of the statuses.

We probably would have to refactor the client code a bit to not violate this property, with the added benefit that the UI will no longer “flicker” when statuses go from encrypted base64 strings to clear text.

Improving the identity verification flow

We should remove the @tanker/verification-ui package and instead introduce tanker identity verification inside the existing authentication flow.

You can check out the Starting a Tanker session section of Tanker’s documentation for more details.

Provide alternative verification methods

You may have noticed that the identity verification currently works by having Tanker and Mastodon servers holding some secrets. Also, the email provider of the users can, in theory, intercept the email containing the verification code.

If this concerns you, please note that instead of using email-based verification, we could use another verification method called the verification key. You can read more about that in the Alternative verification methods section of the Tanker documentation.

Please do note that in this case, users are in charge of their verification key and will not be able to access any of their encrypted resources if they lose it.

We could implement both verification methods and let users choose between the two during onboarding.

Implement pre-registration sharing

The code assumes all users sending or receiving direct messages already have a Tanker identity registered. This can also be solved by using a Tanker feature called Pre-registration sharing.

Make encryption work across instances

Finally, our implementation works only if the sender and receiver of the direct messages are on the same instance - we need to make encryption work with the ActivityPub protocol.

I have a few ideas but fixing it seems non-trivial. Still, it would make for a pretty nice challenge :)

Conclusion

Thanks for reading this far. Writing the patch was a nice experience: Mastodon’s source code is clean and well-organized. You can browse the changes on the pull request on GitHub.

I hope this gave you an idea of the possibilities offered by Tanker. If you’d like to use Tanker in your own application, please get in touch with us.

Feel free to leave a comment below and give us your feedback!

Classes Rock

Dimitri Merejkowsky — Sat, 09 Nov 2019 19:26:27 +0000

This is the second article in a 2-part series of blog posts that examine the complicated relationships between the real world and object-oriented code. If you haven’t already, you should the first part: classes suck first.

Let’s recap: we used a Robot class to implement the client specifications. It worked well, until we tried to model thestart and stop methods with a mutable state. We concluded that blindly using classes (with methods and mutable attributes)to represent real-world objects was a mistake.

Taking a second look at the code

Let’s look at the code again (minus the stop method), along with the spec:

When robots come off the factory floor, they have no name:

def __init__ (self):
    self._name = None

The first time you boot them up, a random name is generated:

def start(self):
    if self._name is None:
        self._name = generate_name()

When a robot is reset, its name gets wiped:

def reset(self):
    self._name = None

So we were modeling real-world objects with our class after all: the mistake we made was trying to model the robot. But using code to model the specification worked!

If you’re not convinced, consider this: the stop() method is empty because the specification said nothing about what happens when the robot is stopped.

If the specification said something like the robot cannot be reset when it isstarted, then using a stopped attribute may have been a good idea.

Encapsulation

So far we talked only about methods and attributes. There’s another aspect of programming with classes I’d like to examine next: the concept of encapsulation, or how to separate “private” implementation and “public” interface.

Basically, there are some details of a class that matter to the people who wrote the class but that users of said class should not know about.

What does this have to do with the real world? Everything! Let me show you.

A single line telling a whole story

In Python, to signify to users of a class that an attribute is not part of the “public” interface, you prefix it with an underscore, like this:

class Robot:
    def __init__ (self):
        self._name = None

By the way, there are already two pieces of information here:

robot names are empty at first
the name likely changes over time (otherwise it would probably have been passed as a parameter in the constructor)

But that’s again about the specification. What does it mean to have _name as a private attribute?

Working in teams

Let’s imagine we are writing software inside a big company with several teams. Reading the specification again, it’s clear that the client is extremely worried about robot names: each and every line of the specification talks about it! If there is a bug affecting the robot name, the client is going to get pissed.

And that’s where using encapsulation makes sense: by “hiding” the name attribute from the users of the Robot class, we are actually covering our bases. If someone from another team writes robot._name = "something-else",the client will get mad (the robot name no longer has the correct format), but we can tell him it’s not our fault: the other team should not have broken the encapsulation without asking us first!

Conclusion

So, which is it? Do classes rock or suck at modeling the real world?

Well, it all depend of what you mean by “the real world”. My advice to you is:

clarify specifications as much as possible
write code that closely resemble the specification
use encapsulation when necessary to protect yourself against misuses of your code

When used correctly, classes are a great way to accomplish all of the above, butthey are also easy to misuse.

Finally, let’s go back at our original question: “is modeling the real worldin code worth it?“.

Well, if it allows you to implement the specifications correctly and makesthings easier for you and your team mates, then yes, it’s definitely worth it.

It’s hard, but trying and solve this challenge is what I love about programming.

Cheers!