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

out-of-tree attester/verifier instances support #25

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zhiminghufighting
Copy link
Collaborator

Fixes: #24
Signed-off-by: zhiminghufighting zhiming.hu@intel.com

@zhiminghufighting zhiminghufighting added the rfc Request for comments label Mar 2, 2022
@@ -21,6 +22,7 @@ static enclave_verifier_opts_t tdx_ecdsa_verifier_opts = {
.api_version = ENCLAVE_VERIFIER_API_VERSION_DEFAULT,
.flags = ENCLAVE_VERIFIER_OPTS_FLAGS_TDX,
.name = "tdx_ecdsa",
.oid = TDX_QUOTE_OID,
Copy link
Contributor

Choose a reason for hiding this comment

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

plz check the format.

@@ -31,6 +31,7 @@ typedef struct {
uint8_t api_version;
unsigned long flags;
const char name[ENCLAVE_VERIFIER_TYPE_NAME_SIZE];
const char oid[OID_LENGTH];
Copy link
Contributor

Choose a reason for hiding this comment

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

add a newline here.

la_attestation_evidence_t la;
tdx_attestation_evidence_t tdx;
snp_attestation_evidence_t snp;
tee_attestation_evidence_t evidence;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please change to

typedef struct {
	char type[ENCLAVE_ATTESTER_TYPE_NAME_SIZE];
	union {
		attestation_verification_report_t epid;
                struct {
                    uint8_t report[8192];
                    uint32_t report_len;
                };
	};
} attestation_evidence_t;

So we could avoid referring report with evidence->evidence.report. Now just use evidence->report. It looks more clear.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good advice!

return -ENCLAVE_ATTESTER_ERR_CPU_UNSUPPORTED;
}
}

enclave_attester_opts_t *new_opts = (enclave_attester_opts_t *)malloc(sizeof(*new_opts));
Copy link
Contributor

Choose a reason for hiding this comment

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

You should move this check to the corresponding instances' pre_init(), instead of simply removing them. In addition, ENCLAVE_ATTESTER_OPTS_FLAGS_SNP_GUEST and ENCLAVE_ATTESTER_OPTS_FLAGS_TDX_GUEST looks useless. You can remove them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Keep tee self-detect function in registering phase will block some instance to register. Consider the condition, if an app need to use some key generation function in sev-snp.so, but the app run in intel cpu, user will never have the opportunity to load the sev-snp.so. That's why remove the self-detect function as a registering gate-keeper.
I think user can invoke self-detect function to check the HW environment, but don't use self-detect as forcing gate-keeper in rats-tls.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't get your point. Why an user uses hardware agnostic key generation function from sev-snp? sev-snp only provides the support for its attester role or verifier role. If key generation function is so common that other instance wants to reuse it, does it make sense to move such a general function to the common part? In addition, your scenario actually exists? Does sev-snp instance really show enough versatility on key generation (not just a limited usage for serving itself)?

goto err;
if (!strcmp(cert_info->evidence.type, opts->name)) {
tee_attestation_evidence_t *evidence = &cert_info->evidence.evidence;
if (!x509_extension_add(cert, opts->oid, evidence->report, evidence->report_len))
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't any places where an OID is referred and then you replace such a loop to find the OID. Actually ctx->attester->opts->oid is the fast path to get it right?

typedef struct {
uint8_t report[8192];
uint32_t report_len;
} snp_attestation_evidence_t;
} tee_attestation_evidence_t;
Copy link
Contributor

Choose a reason for hiding this comment

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

This structure can be removed.

Fixes: inclavare-containers#24
Signed-off-by: zhiminghufighting <zhiming.hu@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfc Request for comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

out-of-tree attester/verifier instances support
2 participants