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

[tools/RA-TLS] Copy quote from X.509 cert into a separate object #1947

Merged
merged 1 commit into from
Jul 24, 2024

Conversation

dimakuv
Copy link

@dimakuv dimakuv commented Jul 24, 2024

Description of the changes

Previously, extract_quote_and_verify_pubkey() returned a pointer to the SGX quote located inside the X.509 certificate. This is a confusing pattern, so this commit introduces a copy operation, to copy the SGX quote into a newly allocated object whose ownership is passed to the callers of this func.

How to test this PR?

CI is enough.


This change is Reviewable

@dimakuv dimakuv marked this pull request as ready for review July 24, 2024 07:11
Copy link
Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 5 files reviewed, all discussions resolved, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel)

a discussion (no related file):
FYI: extracted from #1039.



tools/sgx/ra-tls/ra_tls_common.h line 27 at r1 (raw file):

    { 0x06, 0x09, 0x2A, 0x86, 0x48, 0x86, 0xF8, 0x4D, 0x8A, 0x39, (N) }
static const uint8_t g_quote_oid[] = OID(0x06);
static const size_t g_quote_oid_size = sizeof(g_quote_oid);

FYI: This change is a bit unrelated, but I tucked it into this PR for simplicity.

Just like this whole PR, this change is also extracted from #1039.

@dimakuv dimakuv mentioned this pull request Jul 24, 2024
5 tasks
kailun-qin
kailun-qin previously approved these changes Jul 24, 2024
Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel)

mkow
mkow previously approved these changes Jul 24, 2024
Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dimakuv)


tools/sgx/ra-tls/ra_tls_verify_common.c line 243 at r1 (raw file):

    /* quote returned by find_oid_in_cert_extensions() is a pointer somewhere inside of the X.509
     * cert object; let's copy it into a newly allocated object to correctly track ownership */

(this is not required for correct ownership tracing, it just makes it more straightforward)

Suggestion:

let's copy it into a newly allocated object to make tracing ownership easier

Copy link
Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dimakuv)


tools/sgx/ra-tls/ra_tls_verify_common.c line 243 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

(this is not required for correct ownership tracing, it just makes it more straightforward)

Will do during rebase.

Previously, `extract_quote_and_verify_pubkey()` returned a pointer to
the SGX quote located inside the X.509 certificate. This is a confusing
pattern, so this commit introduces a copy operation, to copy the SGX
quote into a newly allocated object whose ownership is passed to the
callers of this func.

Signed-off-by: Dmitrii Kuvaiskii <[email protected]>
@dimakuv dimakuv dismissed stale reviews from mkow and kailun-qin via 139f642 July 24, 2024 18:05
@dimakuv dimakuv force-pushed the dimakuv/ra-tls-extract-quote-obj branch from 978f880 to 139f642 Compare July 24, 2024 18:05
Copy link
Author

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @kailun-qin and @mkow)


tools/sgx/ra-tls/ra_tls_verify_common.c line 243 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Will do during rebase.

Done.

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel) (waiting on @mkow)

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@mkow mkow merged commit 139f642 into master Jul 24, 2024
17 of 18 checks passed
@mkow mkow deleted the dimakuv/ra-tls-extract-quote-obj branch July 24, 2024 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants