Use AttestationVerifier in the Rust Client #4655

ipetr0v · 2024-01-16T12:12:11Z

This PR:

Adds an AttestationVerifier trait to the oak_attestation_verification
Adds an InsecureAttestationVerifier implementation that doesn't use reference values and is needed for testing

conradgrobler

Thanks

thmsbinder · 2024-01-17T11:04:24Z

oak_attestation_verification/src/verifier.rs

+
+impl AttestationVerifier for InsecureAttestationVerifier {
+    fn verify(&self, evidence: &Evidence, _: &Endorsements) -> anyhow::Result<DiceChainResult> {
+        verify_dice_chain(evidence).context("couldn't verify the DICE chain")


Why do we need AttestationVerifier, InsecureAttestationVerifier - couldn't we just expose the function, i.e. a variant of verify which doesn't have the reference_values parameter? My idea so far was to use only static functions in the interface.

Also, I have no idea where a SecureAttestationVerifier would fit into the pattern.

How about this:

pub fn verify_minimal(
now_utc_millis: i64,
evidence: &Evidence,
endorsements: &Endorsements,
) -> anyhow::Result < DiceChainResult > {
verify_dice_chain(evidence)
}

This is a shortcut to verify which behaves identically to an actual verification, but with assumed (but not existent) ReferenceValues skipping over every single dispensable step. This would be much clearer IMO.

Currently our client library accepts an implementation of the AttestationVerifier

And InsecureAttestationVerifier is just one of those implementations. If we provide a verify_minimal we would have to hardcode it into the client code.

The SecureAttestationVerifier will be just another implementation of the AttestationVerifier which may look like this:

struct SecureAttestationVerifier { reference_values: ReferenceValues, } impl SecureAttestationVerifier { fn create(reference_values: ReferenceValues) -> Self {...} } impl AttestationVerifier for SecureAttestationVerifier { fn verify(evidence, endorsements) -> { // Use self.reference_values } }

This interface also allows us to create specific verifiers (i.e. OakFunctionsAttestationVerifier) which will create reference values during the create method. So that our prod clients won't need to construct reference values themselves (if they want to rely of the default ones).

// Use only for testing, not in production.
pub fn verify_minimal(
evidence: &Evidence,
) -> anyhow::Result < DiceChainResult > {
verify_dice_chain(evidence)
}

(Alternative to the above)

couldn't we just expose the function, i.e. a variant of verify which doesn't have the reference_values parameter? My idea so far was to use only static functions in the interface.

This function will never be seen by our prod teams - they will only see the OakClient class that we provide to them. So it's OakClient that will need to either use the minimal verification for testing or use actual verification.

Yes, this interface is just there for prod teams' convenience - i.e. it a part of the client SDK and doesn't have to the the core logic of the attestation library.

I can move it into the oak_client crate. And make InsecureAttestationVerifier call the verify_minimal function.

Though we would still need to implement other verifiers (i.e. CustomAttestationVerifier that accepts any reference values, or application specific verifiers like OakFunctionsAttestationVerifier).

Where should we put those?

The same question about C++ and Java

Ideally they would belong to that module containing the present AttestationVerifier Rust structs, with all of its impl. However, the "custom" part seems to be the addition of a ReferenceValues proto instance which will always be hard-wired. That should be contained in the "custom" place, i.e. OakFunctionsAttestationVerifier would not exist (or it would exist in private somewhere in oak_functions_*.) OakFunctionsAttestationVerifier (if it needs to be named) would just be the CustomAttestationVerifier and add its static instance of ReferenceValues.

Assuming that CustomAttestationVerifier and SecureAttestationVerifier are the same thing.

For C++ and Java: the JNI interface would just connect/implement the static public verify functions (all variants), so the wrappers would be reimplemented in the respective language.

Moved InsecureAttestationVerifier to oak_client crate. But the AttestationVerifier is currently still in the oak_attestation_verification

Update: moved the trait to oak_client as well

thmsbinder · 2024-01-17T11:24:33Z

oak_attestation_verification/src/verifier.rs

+
+impl AttestationVerifier for InsecureAttestationVerifier {
+    fn verify(&self, evidence: &Evidence, _: &Endorsements) -> anyhow::Result<DiceChainResult> {
+        verify_dice_chain(evidence).context("couldn't verify the DICE chain")


// Use only for testing, not in production.
pub fn verify_minimal(
evidence: &Evidence,
) -> anyhow::Result < DiceChainResult > {
verify_dice_chain(evidence)
}

(Alternative to the above)

k-naliuka · 2024-01-17T12:43:22Z

oak_attestation_verification/src/verifier.rs

+        &self,
+        evidence: &Evidence,
+        endorsements: &Endorsements,
+    ) -> anyhow::Result<DiceChainResult>;


I thought we'd return AttestationResults?

I had this at first, after some discussion we ended up addressing that by trait, so we keep an idiomatic Rust version on the inside.

Example:

let result = verify(NOW_UTC_MILLIS, &evidence, &endorsements, &reference_values); let att_res = AttestationResults::from(&result);

Would this work with the JNI bindings?

I think current JNI bindings are relying on just the AttestationResults. But there is a wrapper layer between the JNI bindings and the actual verification code.

Yeah, it's definitely possible to do a conversion in the wrapper layer. I'm rather wondering about what the right interface for AttestationVerifier should be - do we want to expose DiceChainResult at all?

It's technically possible to make Java interface return AttestationResults and to do the conversion in the wrapper but it feels a bit weird in the Java interface and Rust trait diverge too much in terms of signature... WDYT?

I will rename to DiceChainResult, or address this in some other way, in a different PR.

tiziano88 · 2024-01-17T15:59:03Z

oak_functions_client/src/lib.rs

 }

 impl OakFunctionsClient {
-    pub async fn new(uri: &str) -> anyhow::Result<Self> {
+    pub async fn new<V: AttestationVerifier>(uri: &str, verifier: &V) -> anyhow::Result<Self> {


I don't think we need a generic here, a Box dyn would probably be a bit simpler (also in OakClient::create), but up to you.

Will it require always creating it as a Box?

I.e. Box::new(InsecureAttestationVerifier{})

I think so -- unless verifier: & dyn AttestationVerifier works?

Done

Made it verifier: &dyn AttestationVerifier

tiziano88 · 2024-01-17T15:59:52Z

oak_functions_client/src/lib.rs

        let channel = Channel::from_shared(uri.to_string())
            .context("couldn't create gRPC channel")?
            .connect()
            .await
            .context("couldn't connect via gRPC channel")?;
        let transport = GrpcStreamingTransport::new(StreamingSessionClient::new(channel));
-        let oak_client = OakClient::create(transport)
+        let oak_client = OakClient::create(transport, verifier)


BTW Why was this method called create instead of new?

I'm generally trying to call constructors that can fail as create to differentiate them from new that always succeeds

thmsbinder

Thanks!

thmsbinder · 2024-01-17T16:15:02Z

oak_attestation_verification/src/verifier.rs

+        &self,
+        evidence: &Evidence,
+        endorsements: &Endorsements,
+    ) -> anyhow::Result<DiceChainResult>;


I will rename to DiceChainResult, or address this in some other way, in a different PR.

Update Update Update Update

ipetr0v force-pushed the attestation_verifier_trait branch from cb6514a to 77d282e Compare January 16, 2024 12:12

ipetr0v changed the title ~~Use InsecureAttestationVerifier in the Rust Client~~ Use AttestationVerifier in the Rust Client Jan 16, 2024

ipetr0v force-pushed the attestation_verifier_trait branch from 24464bc to d6bb63b Compare January 17, 2024 10:26

ipetr0v marked this pull request as ready for review January 17, 2024 10:42

ipetr0v requested review from conradgrobler and thmsbinder January 17, 2024 10:46

conradgrobler approved these changes Jan 17, 2024

View reviewed changes

thmsbinder reviewed Jan 17, 2024

View reviewed changes

ipetr0v requested a review from k-naliuka January 17, 2024 11:22

thmsbinder requested changes Jan 17, 2024

View reviewed changes

k-naliuka reviewed Jan 17, 2024

View reviewed changes

ipetr0v requested review from thmsbinder and k-naliuka January 17, 2024 15:47

tiziano88 approved these changes Jan 17, 2024

View reviewed changes

thmsbinder approved these changes Jan 17, 2024

View reviewed changes

conradgrobler mentioned this pull request Jan 18, 2024

Remove empty AttestationEvidence from oak containers #4667

Closed

ipetr0v force-pushed the attestation_verifier_trait branch from 7bc636e to 5a38faf Compare January 18, 2024 10:45

ipetr0v added 11 commits January 19, 2024 10:43

Use InsecureAttestationVerifier in the Rust Client

b6214c1

Update Update Update Update

Update

9fb354d

Update

3e6b0f0

Update

7f777dc

Update

dd2ecf7

Update

5e5716d

Update

1214ff8

Update

5488dcc

Update

582f17c

Update

9e086f5

Update

7ee7f1b

ipetr0v force-pushed the attestation_verifier_trait branch from 7ba18fb to 7ee7f1b Compare January 19, 2024 10:43

ipetr0v merged commit b1e4abd into project-oak:main Jan 19, 2024
17 checks passed

ipetr0v deleted the attestation_verifier_trait branch January 19, 2024 12:39

ipetr0v mentioned this pull request Jan 26, 2024

Verify attestation in Rust client #4644

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use AttestationVerifier in the Rust Client #4655

Use AttestationVerifier in the Rust Client #4655

ipetr0v commented Jan 16, 2024 •

edited

Loading

conradgrobler left a comment

thmsbinder Jan 17, 2024

ipetr0v Jan 17, 2024 •

edited

Loading

ipetr0v Jan 17, 2024

thmsbinder Jan 17, 2024

ipetr0v Jan 17, 2024

ipetr0v Jan 17, 2024

ipetr0v Jan 17, 2024 •

edited

Loading

thmsbinder Jan 17, 2024 •

edited

Loading

ipetr0v Jan 17, 2024

ipetr0v Jan 17, 2024

thmsbinder Jan 17, 2024

k-naliuka Jan 17, 2024

thmsbinder Jan 17, 2024

ipetr0v Jan 17, 2024

k-naliuka Jan 17, 2024

thmsbinder Jan 17, 2024

tiziano88 Jan 17, 2024

ipetr0v Jan 17, 2024

tiziano88 Jan 17, 2024

ipetr0v Jan 17, 2024

tiziano88 Jan 17, 2024

ipetr0v Jan 17, 2024

thmsbinder left a comment

thmsbinder Jan 17, 2024

Use AttestationVerifier in the Rust Client #4655

Use AttestationVerifier in the Rust Client #4655

Conversation

ipetr0v commented Jan 16, 2024 • edited Loading

conradgrobler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ipetr0v Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ipetr0v Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

thmsbinder Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thmsbinder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ipetr0v commented Jan 16, 2024 •

edited

Loading

ipetr0v Jan 17, 2024 •

edited

Loading

ipetr0v Jan 17, 2024 •

edited

Loading

thmsbinder Jan 17, 2024 •

edited

Loading