Provide builder for KBS Protocol Wrapper

[mkulke]

Currently KbsProtocolWrapper bootstraps itself. It detects a TEE and attempts to create an Attester object. There are situations in which it is desirable to inject an Attester implementation from the caller side (e.g. we want to wrap get_evidence() for a given TEE in an IPC call).

(discussion here, for more context)

This is made possible by providing a builder for the wrapper. Existing code should work without modifications. The builder requires a tee and accepts an attester object optionally.

let wrapper = KbsProtocolWrapperBuilder::new().with_tee(Tee::Snp).build();
# or
let wrapper = KbsProtocolWrapperBuilder::new()
  .with_tee(Tee::Tdx)
  .with_attester(brought_my_own)
  .build();

There are some drive-by fixes, as the type KBSProtocolWrapper currently doesn't reflect certain invariants:

• nonce should not default to an empty string, we want to bail out if it's unset
• attester and tee_key otoh shouldn't be wrapped in Options, the wrapper will not work w/o them.
• tee_key can be passed by ref

[Xynnn007]

Why we use generic types here? I saw builder in projects like reqwest and in-toto uses the following, which seems more straightforward

pub struct KbsProtocolWrapperBuilder {
    tee: Option<..>,
}

impl KbsProtocolWrapperBuilder {
    pub fn with_tee(mut self, tee: ..) -> Self {
        self.tee = Some(tee);
        self
    } 
}

[mkulke]

Why we use generic types here? I saw builder in projects like reqwest and in-toto uses the following, which seems more straightforward

pub struct KbsProtocolWrapperBuilder {
    tee: Option<..>,
}

impl KbsProtocolWrapperBuilder {
    pub fn with_tee(mut self, tee: ..) -> Self {
        self.tee = Some(tee);
        self
    } 
}

Yeah, this is a more pragmatic way to create builders. If you call .build() and the builder struct has Option<...> types, you need to perform runtime checks to verify tee is actually set (e.g. return Result<KbsProtocolWrapper> or panic even).

If you use encode the requirements via generics the verification happens at compile time, you are not able to call build() unless you called .with_tee().

The type-level verification is admittedly more verbose, and I just noticed we already need to return a Result<KbsProtocolWrapper> from .build(), so i'd be fine with changing this to a builder w/ Option<...> properties. What do you prefer?

[Xynnn007]

Generic types actually will save runtime. I'm ok with both. However there is a naive question from myside. I think the main aim of the PR is to avoid creating an Attester from the TEE detection heuristic inside new() of KbsProtocolWrapper, but to receive an outer Attester parameter. If so, why do not directly add a function with the signature

pub fn new(attester: BoxedAttester) -> Self {
    Self {
        attester,
        ...
    }
}

[mkulke]

Generic types actually will save runtime. I'm ok with both. However there is a naive question from myside. I think the main aim of the PR is to avoid creating an Attester from the TEE detection heuristic inside new() of KbsProtocolWrapper, but to receive an outer Attester parameter. If so, why do not directly add a function with the signature

pub fn new(attester: BoxedAttester) -> Self {
    Self {
        attester,
        ...
    }
}

you mean pub fn new_with_attester() or simply replacing the existing pub fn new() function?

if we can do that, the builder pattern is too much. it would maybe be useful if we would want to inject an HTTP client (for unit testing or having a generic RPC client), but we can also add it later.

[Xynnn007]

LGTM. cc @jialez0 please take a look if it is ok to you

[Xynnn007]

Hi @mkulke Can you rebase the PR?

[Xynnn007]

@mkulke Well. Seems that some coupled code was broken by this PR, s.t. Kbc::new() which is called in a unit test

[mkulke]

@mkulke Well. Seems that some coupled code was broken by this PR, s.t. Kbc::new() which is called in a unit test

oh, that's a surprise. i'll look into that.

[mkulke]

@mkulke Well. Seems that some coupled code was broken by this PR, s.t. Kbc::new() which is called in a unit test

oh, that's a surprise. i'll look into that.

Ok, apparently those unit tests construct a kbs protocol wrapper w/ an attester: None as a side effect. The PR removed the Option wrapping, as having an attester in a kbc client is an invariant (i.e. clients w/o attester don't work) => tests fail w/ TEE not supported.

The tests in question are URI parsing tests however, so they don't mind a non-functional kbc client.

Refactoring the tests/code to untangle uri parsing from the kbc client and unit testing just that is probably the right fix long-term. The unit tests atm perform IO side-effects as they detect the tee which adds delay and faux errors to the log.

Short term I see 2 ways to address that:

• Restoring the Option wrapping for Attester
• set AA_SAMPLE_ATTESTER_TEST=1 in the unit test, so we'll get a proper a Attester trait object.

I don't particularly like either option, but restoring the Option wrapper is probably the most sensible option in the scope of this PR.

[fitzthum]

There are situations in which it is desirable to inject an Attester implementation from the caller side (e.g. we want to wrap get_evidence() for a given TEE in an IPC call).

I am missing something at the high level. When would we want to use an Attester other than the one that we would currently create with the TEE detection heuristic?

[mkulke]

There are situations in which it is desirable to inject an Attester implementation from the caller side (e.g. we want to wrap get_evidence() for a given TEE in an IPC call).

I am missing something at the high level. When would we want to use an Attester other than the one that we would currently create with the TEE detection heuristic?

One scenario would be a kbs-client which is not able to access the TEE's evidence-retrieval facilities directly (like in peer-pods where the network is segmented), without having to replicate the kbs client logic. Another scenario would be the injection of a mock attester in a unit test for the RCAR logic.