fix(settings): get message data hmac key setting to work with env variables

[brizental]

Fixes #109

Because we had underscores in the name of this setting, it wasn't getting overwritten by an FXA_EMAIL_ environment variable. At first I thought this was a bug with config-rs but it actually looks like a design choice, because they use underscores to signal nesting. For example:

  "authdb": {
    "baseuri": "http://127.0.0.1:8000/"
  }

Will get overwritten by the environment variable FXA_EMAIL_AUTHDB_BASEURI.

Anyways, to fix this I changed the message_id_hmac_key setting to not have any underscores. Since that was too long for just removing the underscores, I did a little nesting for the resulting environment variable to look nicer. Now, to overwrite the hmac key we can just use FXA_EMAIL_MESSAGEDATA_HMACKEY.

I also added that to our settings tests, so that we can make sure it works.

Finally, I remember @philbooth commenting that this is sensitive data that we should probably hide when logging. Should I add a deserializer macro to hide this?

r? @philbooth @vladikoff

---

[UPDATE]

Looking at config-rs source code made me wonder if this really isn't a bug... I filed an issue there (rust-cli/config-rs#73) and will try to investigate it.

[philbooth]

Fwiw, I maintain my position that we don't need nesting for this and could just have it at the top level (see discussion in #72 (comment)). Not sure what everyone else thinks though, so ignore me if you guys disagree.

[philbooth]

If we really want to keep the nesting though, maybe it should be inverted in this case. Make hmackeys the parent property and then we can have all the different instances that we imagine we want underneath it as children.

[brizental]

I agree that the nesting is a bit much, but I still think FXA_EMAIL_MESSAGEDATAHMACKEY is just too long. I do like the initial solution that you had for this, FXA_EMAIL_HMACKEY. I will take a look at the environment separator you mentioned and see if we can keep the underscores after all, if we don't, I will still remove the nesting :)

[philbooth]

Oh yeah, I definitely didn't mean FXA_EMAIL_MESSAGEDATAHMACKEY that's way too ugly. But FXA_EMAIL_HMACKEY is fine I think.

[philbooth]

(but I think @vladikoff disagrees, so let's see what he says)

[vladikoff]

I'm sold! I just want my ENV var :)

[philbooth]

Looking at config-rs source code made me wonder if this really isn't a bug

It's definitely not a bug in config-rs fwiw, I think you're supposed to use Environment::separator if you want to work round situations like this. It was just stupid oversight on my part when adding this property, compounded by not including a test for it. So thanks for adding that test here!

[philbooth]

r+, modulo whatever decision you guys want to make about dropping the messagedata nesting. I don't think it adds any value and would prefer to drop it on the basis of YAGNI.

[brizental]

I think you're supposed to use Environment::separator

So, apparently the config-rs crate is an outdated version of it compared to the repo. In the version that we point to (0.8.0) the behavior is to use underscore as a separator by default. The repo version doesn't set any default separator anymore (rust-cli/config-rs@536f52f).

This is how this affects us: if we keep using the version of config-rs we are using now, when that change comes it will break all our code, because we depend on the default underscore separator to overwrite all our nested settings. What we can do is set the separator even though it is already the default one and when the change comes our code will still work as expected. I think that is a good solution, if we all agree I'll send in a review for this PR.

Regarding having underscores in our settings names, if we are really gonna stick with using underscores as our default separator we can't do that, because whenever we have underscore config-rs will always assume that it is something nested inside something else. So, for example, FXA_EMAIL_MESSAGE_ID_HMAC_KEY will overwrite this setting:

{
   "message": {
      "id": {
         "hmac": {
            "key": "YOU SHOULD CHANGE THIS" # this will be overwritten by FXA_EMAIL_MESSAGE_ID_HMAC_KEY
         }
      }
}

and not what we are actually expecting it to overwrite, which is:

{
   "message_id_hmac_key": "YOU SHOULD CHANGE THIS" # this won't be overwritten
}

This is just something that is nice to know, because I think the solution for the problem that I introduced in this PR (just remove the underscores) is good enough and we shouldn't spend much more time on this. The other solution would be to use another separator to indicate nesting instead of underscores, but that would mean we would have to change ALL our environment variables for settings.

Anyways, let me know what you guys think about this, if we should use another separator or just stick with what we have now.

[philbooth]

Regarding having underscores in our settings names, if we are really gonna stick with using underscores as our default separator we can't do that

Fwiw, my recollection is that we made a conscious decision not to have underscores in setting names back when we did #11, although I can't find a comment about it in that PR so possibly I've misremembered. Anyway, whatever, let's keep on doing that is my thinking. The one you're fixing here was definitely accidental.

[vladikoff]

👍