[flihp]

This work is intended to simplify the machinery behind the existing sled-agent RoT interface & to bring sled-agent up to date w/ some reorganization & renaming done in the dice-util repo. The hard work was already done in oxidecomputer/dice-util#360. This PR just brings sled-agent up to date allowing it to benefit from the newly async trait & impls.

[jmpesp]

To check my assumptions: RotAttestor now takes care of spawning the necessary background task(s)? Does that mean RotError::Shutdown can still come from the functions called on guard.attest?

[luqmana]

Any reason not to generate a random nonce here?

[flihp]

I went back and forth on this. TL;DR simplicity won out and as written, this tests all of he parts that we care about here.

Some of the logic behind the choice:
There's nothing bad about creating the qualifying data from a random nonce here, but for the purposes of this test we don't really care about freshness or even appraising the attestation. All we care about is that the machinery in sled-agent behaves the way we expect. Verifying the attestation here serves as a means to check the integrity of the data we get back from the plumbing, but that's it. There are tests in the dice-util repo where we try to mimic the "real world" as much as possible and we use a random nonce there. I didn't think there was much use recreating those tests here.

All that said the cost associated with dipping into the RNG is pretty low so I'm not really attached to either approach. LMK if you have a preference?

[luqmana]

Do we plan to also change the API-visible names as well?

[flihp]

At this point I avoided doing so because the change is just cosmetic. I've got a few topic branches where I'm working out how to add support for the HeliosRot and that will require API changes. It seemed logical to do that in a single PR ... or maybe just leave this part of the API alone? I'm not fully convinced this is the right approach so suggestions / opinions are welcome.

[flihp]

in reply to @jmpesp from #11674 (review)

To check my assumptions: RotAttestor now takes care of spawning the necessary background task(s)?

Yes: https://github.com/oxidecomputer/dice-util/blob/main/oxide-rot/src/ipcc.rs#L39

Does that mean RotError::Shutdown can still come from the functions called on guard.attest?

As written now I believe the answer is 'no'. AFAIK: RotError::Shutdown was produced in response to errors from the oneshot receive channel used by the http endpoint to get responses from the long running background task / RotAttestationTask :

omicron/sled-agent/src/rot.rs

Lines 157 to 163 in eb2e28a

	pub async fn get_measurement_log(
	&self,
	) -> Result<MeasurementLog, RotError> {
	let (tx, rx) = oneshot::channel();
	self.tx.try_send(AttestationMessage::GetMeasurementLog(tx))?;
	Ok(rx.await.map_err(|_| RotError::Shutdown)??)
	}

and

omicron/sled-agent/src/rot.rs

Lines 201 to 208 in eb2e28a

	pub fn launch(
	log: &Logger,
	attest_config: &AttestConfig,
	) -> Result<RotAttestationHandle, RotError> {
	let (task, handle) = Self::new(log, attest_config)?;
	tokio::task::spawn_blocking(move || task.run());
	Ok(handle)
	}

This doesn't give us much info about what happened to the task, but it means that the task that should respond is not going to and Shutdown seems reasonable there. The use of spawn_blocking has been moved into a much shorter lived task (one request) here:

https://github.com/oxidecomputer/dice-util/blob/994c3fcdb7e3897283f5082a892b67db397c0738/oxide-rot/src/ipcc.rs#L39-L47

This code now immediately calls await on the JoinHandle returned by spawn_blocking and that's the only place where an error may occur that is at least similar to the RotError::Shutdown. In this code however we do JoinHandle.await.expect(..) and that will cause that code to panic on a JoinError. AFAIK this only happens if the task spawned panics and that's ... impossible?

All that said, instead of expecting, we could turn the JoinError into something akin to RotError::Shutdown (that should probably be mapped to an HTTP internal error?) if we think that's the right thing to do. But as currently written, the Shutdown variant of RotError is no longer useful.