LG-6948: Store key name in AttemptEvent redis key

[n1zyy]

Why: To allow for eventual support for key rotation, where we'd want to indicate which key was used to encrypt a particular event.

Commentary

I don't completely love this, but I think we should go with this unless someone has a better idea.

The schema describes us as returning something like this:

{
  "sets": {
    "4cf27640-5b92-4b10-9c96-40e4e3f1f183": {
        "KEYID": "ENCRYPTED_VALUE"
    },
    "8ae5ed6b-5efd-423d-8afa-cbd82a9cf439": {
        "KEYID2": "ENCRYPTED_VALUE"
    }
  }
}

Where KEYID or KEYID2 would be the key ID used to encrypt the event.

Redis seems to only support string hash values, so I ended up encoding this in Redis as key_id:jti for the key and a simple jwe as the value. We then swap it out to the format indicated in the schema when reading it from Redis.

The goal of this story is only to get the returned data supporting this, not to actually support multiple keys. That will probably some changes to how keys are stored in addition to the code changes.

[zachmargolis]

do we want to extract out the key_id as a property on the event now too?

[n1zyy]

This story is dragging on (my fault), but having just implemented this, I don't like it and think I want to change the model a bit.

tl;dr, it doesn't make sense to me to set key id but not be able to set the key itself. It's logical enough in from_jwe that we'd capture that information, but if you were to then call key_id you'd get a key id that might not match what event_data_encryption_key returns.

Right now it's implemented in the yaml like so:

  irs_attempt_api_public_key: change-me-pls
  irs_attempt_api_public_key_id: key1

This seems easy enough for currently only supporting one key. If we're going to keep this, I think we should not extract the key_id parameter as proposed in this comment.

But I feel like what we actually want is something like:

  irs_attempt_api_public_keys:
    key1: change-me-pls

Then, setting key_id in code would cause the correct public key to be looked up.

What do you think, @zachmargolis et al.? Should I go ahead with that?

[zachmargolis]

honestly, I'm not sure I follow anymore so I trust your call

[n1zyy]

OK. I'm going to propose leaving this as-is for now and not explicitly capturing key_id as an attribute inside AttemptEvent.

What I proposed with application.yml.default changes turns out to not be quite how IdentityConfig works, and feels a little too close to implementing actual multi-key support. (Jack also pointed out that it would probably make more sense to implement storage of that in the database when the time comes.)

[n1zyy]

I'm not thrilled with this living in the controller, but we're getting a hash out of Redis and it's just doing some light data munging. 😐

[mitchellhenke]

I think one typo?

Otherwise looks good!