Refactor text reference values matching

[k-naliuka]

Allow literal string comparison and make regex optional

[tiziano88]

What implications does this have on the BUILD files?

[k-naliuka]

What implications does this have on the BUILD files?

We can enable the feature in crate_features in the BUILD rule - when the feature is disabled the dependency is not necessary. Defined both targets in the BUILD rule as a proof of concept

[tiziano88]

are both config guards needed?

[k-naliuka]

#[cfg(feature = "regex")] allows using the Regex type only when the feature is enabled, the if cfg!(feature = "regex") condition prevents the unreachable code warning for returning the error - but perhaps there is a more elegant way to do it?

[timonvo]

I think another option could be to define two verify_regex functions, one for when the feature is on, one for when it is off. Something like:

#[cfg(feature = "regex")]
fn verify_regex(
    actual: &str,
    regex: &oak_proto_rust::oak::attestation::v1::Regex,
) -> anyhow::Result<()> {
  // the real implementation
}

#[cfg(not(feature = "regex"))]
fn verify_regex(
    _actual: &str,
    _regex: &oak_proto_rust::oak::attestation::v1::Regex,
) -> anyhow::Result<()> {
  Err(anyhow::anyhow!("verification of regex values not supported"))
}

[k-naliuka]

Oh, that's really nice, thanks!

[tiziano88]

I think you can mark the message itself as deprecated with an annotation.

[k-naliuka]

Looks like to deprecate an entire message an option needs to be used

[tiziano88]

Why did they become optional though? I think they should be kept identical to what they were before. I mean, it's probably fine, but I would avoid unnecessary changes if possible. Also, being message types, I don't think there is an actual difference on the wire -- you can already set them to None, even without the optional, or did I misunderstand what optional actually does in proto3?

[k-naliuka]

Oops. an artifact from playing around with optional fields. Reverted.

[tiziano88]

I guess this may become repeated in the future, right?

[k-naliuka]

If it will changing this field to repeated will be rather painful, but if we don't already have a use case for multiple values I'd rather leave it as is... I guess if this becomes necessary we can add another oneof option, e.g., MultiStringLiteral. WDYT?

[timonvo]

For parity with BinaryReferenceValue, which has a Digests submessage which lists multiple valid digests (only of which has to match), I would actually lean towards using multiple string values here, as Tiziano suggests.

We're currently operating under the assumption that we can create one ReferenceValues proto to verify a set of possibly TEE configurations, and due to the multiple allowable digests per layer this works right now, but if the kernel command line were to vary across configurations that would break down with this current "single literal" approach.

[k-naliuka]

Hmmm, do we have a story about how to match a particular kernel cmd line to a particular binary to define an acceptable TEE configuration? Or would you expect to define a set of binaries + a set of command lines where any combination is acceptable?

[timonvo]

That's currently the assumption we're making, yeah, that we don't have to strictly verify specific configurations. E.g. that given:

• layer 1: A or B is allowed
• layer 2: C or D is allowed

means that all of the following configurations are allowable:

• A && C
• A && D
• B && C
• B && D

as opposed to only allowing, say, A && C and B && D. I don't think we have a way to specify such allowable strict configurations within a single ReferenceValues proto, but we could always try verifying using a list of ReferenceValues until the first one matches to achieve that kind of more strict checking. I don't think the commit really changes that situation much, given that we already have this issue with the other BinaryReferenceValues matchers.

[timonvo]

For parity with BinaryReferenceValue, which has a Digests submessage which lists multiple valid digests (only of which has to match), I would actually lean towards using multiple string values here, as Tiziano suggests.

We're currently operating under the assumption that we can create one ReferenceValues proto to verify a set of possibly TEE configurations, and due to the multiple allowable digests per layer this works right now, but if the kernel command line were to vary across configurations that would break down with this current "single literal" approach.

[timonvo]

Maybe we should clarify that these fields are not just kept for compatibility, but also being completely ignored now by the verifier.

And maybe for good measure the verifier should actually fail if any of these values are still being specified. That would avoid all confusion about the meaning of these fields in cases where someone erroneously specifies both the new and old values.

[k-naliuka]

Failing verification sounds scary at this point but expanded the comment

[timonvo]

I think another option could be to define two verify_regex functions, one for when the feature is on, one for when it is off. Something like:

#[cfg(feature = "regex")]
fn verify_regex(
    actual: &str,
    regex: &oak_proto_rust::oak::attestation::v1::Regex,
) -> anyhow::Result<()> {
  // the real implementation
}

#[cfg(not(feature = "regex"))]
fn verify_regex(
    _actual: &str,
    _regex: &oak_proto_rust::oak::attestation::v1::Regex,
) -> anyhow::Result<()> {
  Err(anyhow::anyhow!("verification of regex values not supported"))
}

[timonvo]

Note that if we do move to a list of allowed string values, I'd name this StringLiterals (plural), again to be consistent with the Digests (also plural) message name.