Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TQ opertaions #225

Merged
merged 1 commit into from
Sep 10, 2024
Merged

TQ opertaions #225

merged 1 commit into from
Sep 10, 2024

Conversation

labbott
Copy link
Contributor

@labbott labbott commented Aug 27, 2024

No description provided.

@andrewjstone andrewjstone self-requested a review September 6, 2024 20:18
@@ -49,13 +52,17 @@ impl HostRotHeader {
)]
#[repr(u32)]
pub enum HostToRotCommand {
/// Returns the certificate chain associated with the RoT
/// Returns the certificate chain associated with the RoT-M
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure adding the -M is relevant here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was trying to differentiate between the chain returned via GetCertificates vs GetTqCertificates but I'll drop it

GetCertificates,
/// Returns the measurement log
GetMeasurementLog,
/// Calculates sign(sha3_256(hubpack(measurement_log) | nonce))
/// and returns the result.
Attest,
/// Returns the certificate chain associated with TQ
GetTqCertificates,
/// Signs a sha3_256 message with the TQ key
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TqSign looks like it will sign any 32 byte value. I understand that we only send it sha3_256 hashes but we may not want to give folks the impression that there is some magic happening that prevents this function from signing something that isn't a digest.

@@ -229,6 +229,26 @@ impl PkiPathSignatureVerifier {
}
}

pub fn verify_signature(
alias: &Certificate,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alias here is misleading since this could be any certificate. Probably got refactored from code that was checking the signature on an attestation?

pub const MAX_DATA_LEN: usize = NONCE_SIZE;

static_assertions::const_assert!(SHA3_256_DIGEST_SIZE == 32);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels weird. Isn't this a tautology?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had some massive brain farts while working on the code and intended this to be related to the size of the hash passed to TqSign (I wasted several hours thinking the size of sha256 was 64 bytes..). You're right this is not actually useful so I'll remove it.

/// Right now `Attest` is the only command that takes data (nonce)
/// Right now `Attest` and `TqSign` are the only commands that take data
/// argumenets. They happen to be the same length but to be extra cautious
/// add a static assertion.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've removed the assertion so we should probably remove the comment about it. That said we may be able to do more here to ensure we set MAX_DATA_LEN appropriately w/ the two possible message sizes. cmp::max can't be used to create a const value but there are some weird tricks like:

const fn const_max(a: usize, b: usize) -> usize {
    [a, b][(a < b) as usize]
}

I think that may make something like const MAX_DATA_LEN: usize = const_max(NONCE_SIZE, SHA3_256_DIGEST_SIZE) possible? Casting a bool as usize is a bit hacky but it gets us the right array index. Hacked an example up in the playground

This lets the upper layers perform signing operations
@labbott labbott merged commit 3cc953c into main Sep 10, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants