-
Notifications
You must be signed in to change notification settings - Fork 77
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
Notary Server SGX Attestation Template #492
Conversation
Let's open a |
sgtm; i tried but i get a:
|
…ement feat: push hook action to generate a gramine sig for notary-server, add output to readme
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 work. See comments
.github/workflows/sgx-report.yml
Outdated
with: | ||
ref: ${{ github.ref }} |
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.
AFAIK this is the default
with: | |
ref: ${{ github.ref }} |
.github/workflows/sgx-report.yml
Outdated
- uses: awalsh128/cache-apt-pkgs-action@latest | ||
with: | ||
packages: rustc cargo gramine cmake clang gramine | ||
version: 1.1 |
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.
What is this version here?
.github/workflows/sgx-report.yml
Outdated
run: | | ||
echo "GITHUB_ENV: $GITHUB_ENV" | ||
echo "SGX_REPORT: $SGX_REPORT" | ||
- name: update README |
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.
We can avoid changing the readme and regex magic, If write the report to a file, add and commit it.
The README file can link to this file.
Similar to what projects to with checksum files.
What do you think?
I'll postpone reviewing the README file until we make a decision here.
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.
now it creates and uploads a signed-by github artifact
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.
Great stuff! got a few minor comments :)
on: | ||
create: | ||
branches: | ||
- 'release/*' |
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.
for releases, we don't use release/*
naming; but just tags, like https://github.com/tlsnotary/tlsn/blob/main/.github/workflows/ci.yml#L7-L8
|
||
jobs: | ||
create-gramine-attestation: | ||
uses: maceip/tlsn/.github/workflows/gramine-report.yml@sgx-attest |
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 shouldn't be maceip
's here right?
@@ -134,3 +134,8 @@ Axum is chosen as the framework to serve HTTP and WebSocket requests from the pr | |||
|
|||
#### WebSocket | |||
Axum's internal implementation of WebSocket uses [tokio_tungstenite](https://docs.rs/tokio-tungstenite/latest/tokio_tungstenite/), which provides a WebSocket struct that doesn't implement [AsyncRead](https://docs.rs/futures/latest/futures/io/trait.AsyncRead.html) and [AsyncWrite](https://docs.rs/futures/latest/futures/io/trait.AsyncWrite.html). Both these traits are required by the TLSN core libraries for the prover and the notary. To overcome this, a [slight modification](./src/service/axum_websocket.rs) of Axum's implementation of WebSocket is used, where [async_tungstenite](https://docs.rs/async-tungstenite/latest/async_tungstenite/) is used instead so that [ws_stream_tungstenite](https://docs.rs/ws_stream_tungstenite/latest/ws_stream_tungstenite/index.html) can be used to wrap on top of the WebSocket struct to get AsyncRead and AsyncWrite implemented. | |||
|
|||
## Reproduciple Builds & Attestation | |||
- We are using [Gramine](https://github.com/gramineproject) to generate SGX reports for notary-server; |
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.
- We are using [Gramine](https://github.com/gramineproject) to generate SGX reports for notary-server; | |
- We are using [Gramine](https://github.com/gramineproject) to generate SGX reports for notary-server. |
@@ -134,3 +134,8 @@ Axum is chosen as the framework to serve HTTP and WebSocket requests from the pr | |||
|
|||
#### WebSocket | |||
Axum's internal implementation of WebSocket uses [tokio_tungstenite](https://docs.rs/tokio-tungstenite/latest/tokio_tungstenite/), which provides a WebSocket struct that doesn't implement [AsyncRead](https://docs.rs/futures/latest/futures/io/trait.AsyncRead.html) and [AsyncWrite](https://docs.rs/futures/latest/futures/io/trait.AsyncWrite.html). Both these traits are required by the TLSN core libraries for the prover and the notary. To overcome this, a [slight modification](./src/service/axum_websocket.rs) of Axum's implementation of WebSocket is used, where [async_tungstenite](https://docs.rs/async-tungstenite/latest/async_tungstenite/) is used instead so that [ws_stream_tungstenite](https://docs.rs/ws_stream_tungstenite/latest/ws_stream_tungstenite/index.html) can be used to wrap on top of the WebSocket struct to get AsyncRead and AsyncWrite implemented. | |||
|
|||
## Reproduciple Builds & Attestation |
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.
## Reproduciple Builds & Attestation | |
## Reproducible Builds & Attestation |
## Reproduciple Builds & Attestation | ||
- We are using [Gramine](https://github.com/gramineproject) to generate SGX reports for notary-server; | ||
- Each release of tlsn will include a build artifact attested by github, which includes the gramine signature. | ||
- if you build and run the notary-server with gramine, you should get the same mr_enclave hash as in our release artifact. |
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.
- if you build and run the notary-server with gramine, you should get the same mr_enclave hash as in our release artifact. | |
- If you build and run the notary-server with gramine, you should get the same `mr_enclave` hash as in our release artifact. |
execute_install_scripts: true | ||
- name: Set PATH | ||
run: echo "export PATH=\$PATH:/usr/local/bin:/usr/bin" >> $GITHUB_ENV | ||
- name: generate manifest and sig |
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.
do we not need to cd
into notary/server
first?
name: generate a gramine signature, which can be verified later | ||
|
||
on: | ||
workflow_call: |
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.
is there a reason why we making this modular to be called from another workflow, instead of everything in one workflow file?
|
||
ARCH_LIBDIR ?= /lib/$(shell $(CC) -dumpmachine) | ||
|
||
SELF_EXE = target/release/notary-server |
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.
the target/release/notary-server
file is actually located in the parent folder: notary/target/release
, instead of notary/server/target/release
— will this affect anything?
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.
the latest repo reorg has shifted the target folder to the top level of the repo
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.
Great stufff! BTW SO SORRY EVERYONE haha i didn't realise each of my previous comments was an independent review that might end up spamming everyone's inbox :(
|
||
# Note that we're compiling in release mode regardless of the DEBUG setting passed | ||
# to Make, as compiling in debug mode results in an order of magnitude's difference in | ||
# performance that makes testing by running a benchmark with ab painful. The primary goal |
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.
question: does ab
here mean ab testing?
$(GRAMINE) notary-server | ||
.PHONY: clean | ||
clean: | ||
$(RM) -rf *.token *.sig *.manifest.sgx *.manifest result-* OUTPUT |
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.
question: does this command run in notary/server directory? or somewhere else?
{ uri = "file:fixture/notary/notary.key" }, | ||
{ uri = "file:fixture/notary/notary.pub" }, | ||
{ uri = "file:fixture/auth/whitelist.csv" }, | ||
{ uri = "file:fixture/tls/notary.crt" }, |
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.
doesn't tls/notary.key
also need to be included since it's the TLS key?
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.
tls/notary.key
not needed anymore as hosted notary server doesnt use it
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.
same goes for whitelist.csv
and notary.crt
; though for config.yaml
, notary.key
and notary.pub
— we need to make sure they are the same files used by the hosted notary server
{ uri = "file:fixture/notary/notary.key" }, | ||
{ uri = "file:fixture/notary/notary.pub" }, | ||
{ uri = "file:fixture/auth/whitelist.csv" }, | ||
{ uri = "file:fixture/tls/notary.crt" }, |
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.
tls/notary.key
not needed anymore as hosted notary server doesnt use it
{ uri = "file:fixture/notary/notary.key" }, | ||
{ uri = "file:fixture/notary/notary.pub" }, | ||
{ uri = "file:fixture/auth/whitelist.csv" }, | ||
{ uri = "file:fixture/tls/notary.crt" }, |
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.
same goes for whitelist.csv
and notary.crt
; though for config.yaml
, notary.key
and notary.pub
— we need to make sure they are the same files used by the hosted notary server
edmm_enable = true | ||
|
||
allowed_files = [ | ||
"file:fixture/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.
we dont need this anymore probably? since hosted notary server don't use it
|
||
ARCH_LIBDIR ?= /lib/$(shell $(CC) -dumpmachine) | ||
|
||
SELF_EXE = target/release/notary-server |
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.
the latest repo reorg has shifted the target folder to the top level of the repo
mounts = [ | ||
{ path = "/lib", uri = "file:{{ gramine.runtimedir() }}" }, | ||
{ path = "{{ arch_libdir }}", uri = "file:{{ arch_libdir }}" }, | ||
{ path = "/fixture", uri = "file:fixture" }, |
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.
we dont need this anymore probably? since hosted notary server don't use it
sgx attestation
this pr adds two files to the notary-server:a gramine manifest template
a Makefile
this PR sets the stage for SGX specific concepts and scaffolding before we deploy on SGX hardware
The SGX Report
The SGX report contains a few values, one of which is MR_ENCLAVE.
This PR also adds a push-hook that generates a new SGX report for that commit, and modifies the project's README.md with the updated SGX Report:
How To Create an SGX Report without SGX:
steps to test this out (gramine is a pain to build on macos, use a *nix:
install gramine; for ubuntu it's (copypasta this whole block):
ensure you have the tlsn deps
make
gramine-sgx-gen-private-key
gramine-sgx-sign -v --manifest notary-server.manifest --output notary-server.sgx
gramine-sgx-sigstruct-view notary-server.sig
if all went well you should see something like the following:
make start-gramine-server
SGX=1 make start-gramine-server
*.sig, *.manifest$, and *.sgx need to get added to .gitignore