-
Notifications
You must be signed in to change notification settings - Fork 34
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
base: master
Are you sure you want to change the base?
out-of-tree attester/verifier instances support #25
Conversation
@@ -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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plz check the format.
40123b8
to
b57b7f5
Compare
@@ -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]; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)) |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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 <[email protected]>
Fixes: #24
Signed-off-by: zhiminghufighting [email protected]