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

Allow TCB levels to be accepted besides UpToDate #344

Merged
merged 2 commits into from
Jan 10, 2023

Conversation

OverOrion
Copy link
Contributor

Proposed changes

This PR aims to give users the ability to accept different TCB levels. This can be useful in cases where the grace period has ended but for some reason (e.g. the BIOS update has not been relased yet) SWHardening could not take place.

Additional info

Currently a user needs to set both the command line flags and in the manifest as well. The command line flag was necessary for me as I could not find a way to extract the manifest values and inject them to the call site without major refactoring. Could you please give me some hints regarding this?

I was unable to run E2E tests via Kubernetes deployment because it seemed to me that some package (ego? era? not sure) were using the upstream version instead of my patched branch (I was using the provided Dockerfile in redis-gramine). This is likely a fault on my side, but wanted to mention it nevertheless. I would appreciate it if you could run E2E on it before merging.

@netlify
Copy link

netlify bot commented Jan 4, 2023

Deploy Preview for marblerun-docs canceled.

Name Link
🔨 Latest commit e399f62
🔍 Latest deploy log https://app.netlify.com/sites/marblerun-docs/deploys/63bbfd98e8f74200085cb6a7

@CLAassistant
Copy link

CLAassistant commented Jan 4, 2023

CLA assistant check
All committers have signed the CLA.

@m1ghtym0 m1ghtym0 added the enhancement New feature or request label Jan 4, 2023
@m1ghtym0
Copy link
Member

m1ghtym0 commented Jan 4, 2023

Thank you @OverOrion!
This is a feature we had on the roadmap, perfect timing with your PR:-)

Copy link
Member

@daniel-weisse daniel-weisse left a comment

Choose a reason for hiding this comment

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

Changes look good to me, thanks
Just noted down some small style changes

Regarding having to set both CLI flags and manifest:
This is OK, and imo the correct way to implement this.

cli/cmd/certificateChain.go Outdated Show resolved Hide resolved
cli/cmd/certificateIntermediate.go Outdated Show resolved Hide resolved
cli/cmd/certificateRoot.go Outdated Show resolved Hide resolved
cli/cmd/manifestGet.go Outdated Show resolved Hide resolved
cli/cmd/util.go Outdated Show resolved Hide resolved
coordinator/quote/ert.go Outdated Show resolved Hide resolved
coordinator/quote/ertvalidator/ertvalidator.go Outdated Show resolved Hide resolved
@daniel-weisse
Copy link
Member

I was unable to run E2E tests via Kubernetes deployment because it seemed to me that some package (ego? era? not sure) were using the upstream version instead of my patched branch (I was using the provided Dockerfile in redis-gramine).

The dockerfile builds the premain from latest upstream source.

If you want to use a premain build from your changes instead, you can adjust the Dockerfile to pull from your repo

4c4
< RUN git clone https://github.com/edgelesssys/marblerun.git /marblerun
---
> RUN git clone --branch tcb-allow-list-pr https://github.com/OverOrion/marblerun.git /marblerun

Alternatively, you could also just copy the built premain into the container directly, instead of building it in docker:

37c37
< COPY --from=build-premain /premain/build/premain-libos /gramine/CI-Examples/redis/
---
> COPY ./premain-libos /gramine/CI-Examples/redis/

@OverOrion
Copy link
Contributor Author

Changes look good to me, thanks Just noted down some small style changes

Regarding having to set both CLI flags and manifest: This is OK, and imo the correct way to implement this.

Thank you for the quick review, I have addressed them and did a force push (it seemed like the way how it is expected to be done in previous PRs).

coordinator/quote/ert.go Outdated Show resolved Hide resolved
coordinator/quote/ert.go Outdated Show resolved Hide resolved
@daniel-weisse daniel-weisse merged commit c537b74 into edgelesssys:master Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants