Use HKDF-derived node secrets rather than a textfile

VERIFIED FIXED

Status

Cloud Services
Server: Token
P3
normal
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: rfkelly, Assigned: rfkelly)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa?])

Attachments

(3 attachments)

(Assignee)

Description

4 years ago
Per IRL discussion with Ops and SecOps, we plan to simplify the way shared secrets are managed between tokenserver and syncstorage.  Rather than maintaining a file of name=>secret mappings, we will use a single list of master secrets and HKDF-derive a corresponding list of node-specific secrets for each node.

The tokenserver holds the master secrets and derived node-specific secrets on demand.  Each storage node holds only its derived secrets, which are pushed onto it from the admin box at deploy time.

We need:

  * code in mozsvc to do the deriving etc
  * code in tokenserver to make it use the above
  * docs updates to describe the new scheme
(Assignee)

Comment 1

4 years ago
Created attachment 8367787 [details] [diff] [review]
mozsvc-derived-secrets.diff

Here's part 1, adding support for this to mozsvc, which I think came out pretty cleanly.

I've extended our existing secrets-handling code with two new subclasses.  "FixedSecrets" directly uses a list of secrets for any node, and "DerivedSecrets" does the node-specific HKDF-derivation.  This should make it easy to plug these into our existing code without too much churn.

I've also changed the mozsvc.user class to load this as a plugin rather than the special-case handling it was doing before.  This is what will make the sync storage servers able to consume tokens signed with these secrets, via a configuration like:

    [hawkauth]
    secrets.backend = mozsvc.secrets.FixedSecrets
    secrets.secrets = <derived-secret-1> <derived-secret-2>

Given a master secret of length N, we derive the node-specific secret as:

    info = "services.mozilla.com/mozsvc/v1/node_secret/" + node_name
    HKDF-SHA256( ikm=HEX(master_secret), salt=None, info=info, size=N )

This gives us a node-specific secret of length N, tied to both the node-name and to this specific usage context via the HKDF info parameter.
Attachment #8367787 - Flags: review?(telliott)
Attachment #8367787 - Flags: feedback?(dchan)
(Assignee)

Comment 2

4 years ago
(By the way, I want to keep the existing file-based Secrets class even if we don't plan to use it right away.  It will be useful if we want to do db-level sharding by having each storage box host multiple nodes, as we can use it to manage the derived secrets for just that set of nodes.)
(Assignee)

Comment 3

4 years ago
Created attachment 8367794 [details] [diff] [review]
tokenserver-derived-secrets.diff

Part 2, plugging it into the tokenserver.  All interfaces remain the same so it's just a matter of tweaking how the settings are loaded.

(We could remove some text-file-specific helper functions from the tokenserver codebase, but that can be done separately).
Attachment #8367794 - Flags: review?(telliott)
(Assignee)

Comment 4

4 years ago
For rotation of the master_secret, the procedure would look something like this (all done from the admin box)

  1) Generate the new master secret, but keep the old one as well for now

  2) For each storage node, derive both the new and old node-specific secrets
     and push them out, so that its config file looks like this:

       [hawkauth]
       secrets.backend = mozsvc.secrets.FixedSecrets
       secrets.secrets = <old-derived-node-secret-as-hex> <new-derived-node-secret-as-hex>

     Restart it.  It is now able to accept tokens signed with either secret.

  3) For each tokenserver webhead, update it with the new master secret, removing
     the old one.  Its config file will look like:

       [tokenserver]
       secrets.backend = mozsvc.secrets.DerivedSecrets
       secrets.master_secrets = <new-master-secret-as-hex>

     Restart it.  It now generates tokens signed with the new derived secrets.

  4) Discard the old master secret.

  5) Wait for one token expiration period, e.g. five minutes.

  6) For each storage node, derive just the new node-specific secret and push
     it out, so that its config file looks like this:

       [hawkauth]
       secrets.backend = mozsvc.secrets.FixedSecrets
       secrets.secrets = <new-derived-node-secret-as-hex>

     Restart it.  It no longer accepts tokens signed with the old secret.


Does this sound sensible?  We can get rid of the "restart it" steps as future development work.  Actually uwsgi/gunicorn probably give us a SIGHUP-based alternative for free already, letting us do this without any user-visible downtime.
Comment on attachment 8367787 [details] [diff] [review]
mozsvc-derived-secrets.diff

Review of attachment 8367787 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozsvc/secrets.py
@@ +28,3 @@
>  from collections import defaultdict
>  
> +from tokenlib.utils import HKDF

Should this be a dependency if you're using Secrets or FixedSecrets?
Attachment #8367787 - Flags: review?(telliott) → review+
Attachment #8367794 - Flags: review?(telliott) → review+
(Assignee)

Comment 6

4 years ago
> Should this be a dependency if you're using Secrets or FixedSecrets?

We already depend on tokenlib for other stuff in mozsvc anyway, so I'm not worried about it too much here.

Updated

4 years ago
Blocks: 951382
Whiteboard: [qa?]
(Assignee)

Comment 8

4 years ago
Created attachment 8373099 [details] [diff] [review]
mozsvc-secret-management.diff

Adding some simple command-line utils to help ops manage the secrets.  With the 'mozservices' package installed on the admin box, we can do this to generate a new master secret:

    python -m mozsvc.secrets new

And so this to derive the node-specific secret from a given master secret:

    python -m mozsvc.secrets derivce <master_secret> <node_name>

Bob, will this be enough for our initial scripting setup?
Attachment #8373099 - Flags: review?(telliott)
Attachment #8373099 - Flags: feedback?(bobm)

Comment 9

4 years ago
Comment on attachment 8367787 [details] [diff] [review]
mozsvc-derived-secrets.diff

Review of attachment 8367787 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozsvc/secrets.py
@@ +144,5 @@
> +        self._master_secrets = master_secrets
> +
> +    def get(self, node):
> +        hkdf_params = {
> +            "salt": None,

Ideally we would use a non-empty salt to make a bruteforce attack more difficult, though we are okay for this use case. The derived key is never exposed outside of the network and is only used to sign/check authtoken requests.

An attacker would need to compromise HKDF_INFO_NODE_SECRET to generate their own valid authTokens. A salt wouldn't help much since an attacker with access to HKDF_INFO_NODE_SECRET should also know any salt value we use.
Attachment #8367787 - Flags: feedback?(dchan) → feedback+
Comment on attachment 8373099 [details] [diff] [review]
mozsvc-secret-management.diff

I believe it will be.
Attachment #8373099 - Flags: feedback?(bobm) → feedback+
Comment on attachment 8373099 [details] [diff] [review]
mozsvc-secret-management.diff

Review of attachment 8373099 [details] [diff] [review]:
-----------------------------------------------------------------

If it gets any more complicated, you'll want to split off the pieces in to their own functions, but it's good for now.
Attachment #8373099 - Flags: review?(telliott) → review+
(Assignee)

Comment 12

4 years ago
> Ideally we would use a non-empty salt to make a bruteforce attack more difficult,
> though we are okay for this use case. 

For completeness, I'll also note that the salt input to HKDF is only used in the initial "extract" stage, where the entropy in the input secret is concentrated and distributed into a uniform pseudo-random key.  Since we're starting with a full-entropy master secret, according to RFC 5869 Section 3.3 we could safely skip the entire extract stage without weakening our security.
Is this considered Fixed, or is the commit above for review?
(Assignee)

Comment 15

4 years ago
Not fixed yet; we need to work with Benson and Bob to get this into stage and give it at least a light loadtest.
Well I think this can be closed now :)
(Assignee)

Comment 17

4 years ago
If this is happily working in stage, let's leave it open while I write some explicit docs on the secret-generation scheme and then I'll close it out.
Priority: -- → P3
Already deployed in Stage and Prod.
:rfkelly to add some docs.
Status: NEW → ASSIGNED
(Assignee)

Comment 19

4 years ago
Documentation updated in tokenserver repo, and on the wiki at https://wiki.mozilla.org/CloudServices/Sagrada/TokenServer#Secrets
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
nice docs!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.