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

Update to accommodate for new XTT changes #46

Merged
merged 2 commits into from
Dec 12, 2018

Conversation

kathrynfejer
Copy link
Contributor

These changes will be necessary when XTT cuts a new release since the server_id and expiry have been taken out, and the NVRAM handles and functions for initializing the sapi and tcti contexts have been added to the XTT library.

This fixes #44 because the xaptum-tpm updates will allow us to read variable length data from NVRAM.

@kathrynfejer kathrynfejer self-assigned this Nov 30, 2018
Copy link
Contributor

@drbild drbild left a comment

Choose a reason for hiding this comment

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

Glad to see these changes being made. Thanks!

Could you please split this into multiple commits:

  1. remove server id and expiry checks
  2. use XTT functions to read from NVRAM
  3. handle variable-length TLS certificates (unless the changes in commit 2 implicitly fix this as well).

I've some comments inline. In particular, I think the usages of xtt_read_object should be replaced with a new, higher-level API in XTT. I won't block merging of this PR while waiting on that, but we should discuss with Zane.

src/xtt.c Outdated Show resolved Hide resolved
src/xtt.c Outdated Show resolved Hide resolved
src/xtt.c Outdated Show resolved Hide resolved
Copy link
Contributor

@drbild drbild left a comment

Choose a reason for hiding this comment

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

Code changes look good! Now we need to update the required version of XTT in CMakeLists.txt and README. That's blocked until @zanebeckwith cuts an XTT release with these changes.

@zanebeckwith
Copy link
Contributor

Sorry for the delay @kathrynfejer. I've now released a new XTT (v0.10.1)

Copy link
Contributor

@drbild drbild left a comment

Choose a reason for hiding this comment

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

Please update the required version of XTT in CMakeLists.txt and the README.

@drbild drbild merged commit a612cde into xaptum:master Dec 12, 2018
@kathrynfejer kathrynfejer deleted the change-nvram-read branch December 20, 2018 19:11
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.

Length of TLS Root Cert is hard-coded
3 participants