Update with crucible agent changes #689
Conversation
The crucible agent now requires clients to specify that a region should be created with the expectation that an upstairs will connect encrypted or not, along with X509 information for launching the corresponding downstairs instances with a TLS acceptor. The region in the DB now has the "encrypted" field, but the DiskCreate struct accepts an encryption key UUID. It's still a TODO to create a key, associate it with a region set, and launch any Upstairs appropriately.
| encrypted: region.encrypted(), | ||
| cert_pem: None, | ||
| key_pem: None, | ||
| root_pem: None, |
There was a problem hiding this comment.
Can we add a tracking issue for "adding support for disk encryption" to Nexus, if we aren't going to add it now?
(deferring the implementation is fine, I just wanna make sure we don't lose track of these spots where we're passing null values)
nexus/src/external_api/params.rs
Outdated
| @@ -177,6 +177,8 @@ pub struct DiskCreate { | |||
| pub snapshot_id: Option<Uuid>, /* TODO should be a name? */ | |||
| /** size of the Disk */ | |||
| pub size: ByteCount, | |||
| /** if present, on-disk blocks should be encrypted */ | |||
| pub encryption_key: Option<Uuid>, | |||
There was a problem hiding this comment.
So the type is a UUID, but what values should we actually be passing here?
From looking at the openapi/nexus.json file, it's clear that this is plumbed through the external API, but if I was a client of the Oxide API, I'd have no idea what values are valid to be used.
Is there another API where you expect callers to add encryption keys before they can start using encrypted disks? Is there some set of UUIDs that we'll make valid on the Oxide side of things?
There was a problem hiding this comment.
Is there another API where you expect callers to add encryption keys before they can start using encrypted disks? Is there some set of UUIDs that we'll make valid on the Oxide side of things?
I originally had this as an "encrypted" boolean, where the client would simply have to decide "encrypted or not", but changed it for this PR to expect what you're describing: a future endpoint where clients create keys, and choose which keys are used for which disks.
Now that I'm typing it all out, it may be better not to offer the choice of clients managing keys and which keys map to which internal consumers (i.e. disks), and to simply ask if users want on-disk blocks to be encrypted or not. That way the implementation details are left up to Nexus.
I'm also not sure if there are previous determinations on this topic or not. I couldn't find any existing encryption key code.
There was a problem hiding this comment.
I think we should dig into this, especially if we want to ensure "encrypted by default" remains easy.
If you want to punt the decision, and just bump the revision of Crucible in this PR, I think I'd be okay passing a None encryption key to Crucible internally. However, I'd want to avoid changing the external API until we need to - otherwise, console + friends will need to start plumbing this UUID field that we might not want.
Does that seem reasonable?
There was a problem hiding this comment.
So our prior plans here basically focused on the main thought: "Encryption of data at rest is just a feature of the product, it is not an option and it is not tied to something the user can control."
That is, in the public API there is no notion of keys or choice here. I think we'd want a very compelling use case to turn this around to bring it external. I would suggest dropping this field entirely. Even in a future world where there are keys that a customer might control, that'd still be an intermediate for volumes, but really here less is more and I'd recommend we avoid this path for public stuff entirely.
Even internally we don't want to have an option for crucible to be encryptionless as that's how we end up writing unencrypted data. Not saying we need to get there today, but something we'll want to get to.
There was a problem hiding this comment.
Just for additional context, support for customer-controlled keys was an explicit non-requirement from RFD 29.
There was a problem hiding this comment.
Just for additional context, support for customer-controlled keys was an explicit non-requirement from RFD 29.
Thanks, I missed this determination :( I'll update the PR.
Propolis: Update oximeter dependency to pull in automatic producer registration (#689) Propagate ReplaceResult up; return disk status (#687) Enable clippy warnings for lossless casts Update rustls deps for CVE-2024-32650 migration: refrain from offering all pages when possible (#682) Crucible: DTrace probes for IO on/off the network (#1284) Update oximeter dep to pull in automatic producer registration (#1279) Remove `ReadResponse` in favor of `RawReadResponse` (#1212) Fix typo in DTrace upstairs_info (#1276) replace needing no work should not be an error (#1275) Add some DTrace scripts to the package. (#1274) More Pantry updates for Region replacement (#1269) Send the correct task count for reconciliations (#1271) Raw extent cleanup (#1268)
Propolis: Update oximeter dependency to pull in automatic producer registration (#689) Propagate ReplaceResult up; return disk status (#687) Enable clippy warnings for lossless casts Update rustls deps for CVE-2024-32650 migration: refrain from offering all pages when possible (#682) Crucible: DTrace probes for IO on/off the network (#1284) Update oximeter dep to pull in automatic producer registration (#1279) Remove `ReadResponse` in favor of `RawReadResponse` (#1212) Fix typo in DTrace upstairs_info (#1276) replace needing no work should not be an error (#1275) Add some DTrace scripts to the package. (#1274) More Pantry updates for Region replacement (#1269) Send the correct task count for reconciliations (#1271) Raw extent cleanup (#1268) --------- Co-authored-by: Alan Hanson <alan@oxide.computer>
The crucible agent now requires clients to specify that a region should
be created with the expectation that an upstairs will connect encrypted
or not, along with X509 information for launching the corresponding
downstairs instances with a TLS acceptor.
The region in the DB now has the "encrypted" field, but the DiskCreate
struct accepts an encryption key UUID. It's still a TODO to create a
key, associate it with a region set, and launch any Upstairs
appropriately.