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

kbs: improvements to quickstart and misc #324

Merged

Conversation

wainersm
Copy link
Member

I'm trying to catch up with latest changes of KBS project by following the quickstart. From within a fresh Ubuntu 22.04 VM (non TEE), I could run the instructions for the KBS on background check mode but not before fixing some instructions. I took that opportunity to have some improvements to the doc.

@wainersm wainersm added the documentation Improvements or additions to documentation label Feb 12, 2024
Copy link
Member

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

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

Nice. I made a couple minor comments.


As an alternative, you can build it with the `sample_only` feature which allow
to mock attestation, thus, you will be able to perform some operations (e.g.
get resources) even by running outside of an TEE:
Copy link
Member

Choose a reason for hiding this comment

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

To be clear, the sample attester is always enabled and you can always use the kbs-client outside of a TEE. This flag means that no other attesters are available so you cannot do a real TEE attestation, but you don't have to worry about the dependencies that some of the hw attesters have.

Btw if we build the kbs-client in the CI we probably want to use this feature (or add a new one that disables the resource endpoint completely).

Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, we do not need CLI_FEATURES flag. And we can always build the client-tool with feature default. This will support both TEE and non-TEE cases. Right?

Copy link
Member

Choose a reason for hiding this comment

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

The default features support all the use cases on all the platforms. For the tests in the CoCo CI, we only use the client for configuration. So we will use the sample_only feature there so that we don't have to install the SGX dependencies.

So yes, no need for any additional feature, although maybe at some point we could introduce features to enable/disable the config and resource endpoints separately. Not a high priority.

Copy link
Member Author

Choose a reason for hiding this comment

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

hi @Xynnn007 @fitzthum , sorry, I misunderstood it completely. I built the default feature and run kbs-client on a non-TEE environment, and I was getting a attester related error (unfortunately I didn't save the logs). Then I built the client with sample_only feature and it magically worked out. Thus, I assumed (wrongly) sample_only feature should be used on non-TEE environments.

Now I updated my main branch and I no longer see the error with kbs-client built with default...probably I was hitting a bug solved recently.

I will re-phrase the sentences.

kbs/quickstart.md Outdated Show resolved Hide resolved
kbs/sample_policies/README.md Outdated Show resolved Hide resolved
It seems obvious to mentioning on quickstart that rust is required to
build the kbs. Not so obvious is the dependency to Golang in order to
compile the OPA engine.

Signed-off-by: Wainer dos Santos Moschetta <[email protected]>
The `make install-kbs` should have admin priviledges to write to
/usr/local/bin.

Signed-off-by: Wainer dos Santos Moschetta <[email protected]>
Added the `cli` and `install-cli` Make targets to, respectively,
build the kbs-client and install into the system.

If you want to build it with alternative features then you should
overwrite the `CLI_FEATURES` variable, for example:

$ make cli CLI_FEATURES=sample_only

Some documents were update to mention the new targets.

Signed-off-by: Wainer dos Santos Moschetta <[email protected]>
Update the instruction to start the kbs binary with sudo as it requires
higher priviledges and in background.

Signed-off-by: Wainer dos Santos Moschetta <[email protected]>
By default kbs listen at port 8080.

Signed-off-by: Wainer dos Santos Moschetta <[email protected]>
@wainersm wainersm force-pushed the quickstart_improvements branch from 51aee2c to 2334ce9 Compare February 19, 2024 21:05
@wainersm
Copy link
Member Author

Updated with following changes:

  • the allow_all.rego policy file was missing a empty line at end of the file. I noticed that if it doesn't have empty lines at first and last line of the file then rego engineer simply fails to parse it. Is it expected?
  • Update the instruction to install rust. The packaged rustc on Ubuntu 22.04 no longer builds the kbs (some recent change probably)
  • Re-phrased the explanation of sample_only feature of client. Looks better now, @fitzthum @Xynnn007 ?

@fitzthum
Copy link
Member

the allow_all.rego policy file was missing a empty line at end of the file. I noticed that if it doesn't have empty lines at first and last line of the file then rego engineer simply fails to parse it. Is it expected?

The Rego engine is really finicky about this sort of thing. In theory I don't think this should be required, but for now it is.

Copy link
Member

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

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

LGTM.

Do you want to add in a deny_all policy as well before we merge? Could come in handy.

Copy link
Member

@Xynnn007 Xynnn007 left a comment

Choose a reason for hiding this comment

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

lgtm. the CI error could be fixed by #331

@wainersm
Copy link
Member Author

LGTM.

Do you want to add in a deny_all policy as well before we merge? Could come in handy.

Good idea. Let me do it.

Created the sample_policies directory to hold exampe of policy files. An
allow_all.rego and deny_all.rego files are added.

Signed-off-by: Wainer dos Santos Moschetta <[email protected]>
@wainersm wainersm force-pushed the quickstart_improvements branch from 2334ce9 to 7556827 Compare February 20, 2024 13:08
@wainersm
Copy link
Member Author

@fitzthum added the deny_all.rego policy. Oddly this policy requires no empty line at the end of the file... :D

I tested it here, I could apply but didn't check it was really denying all.

@fitzthum
Copy link
Member

Oddly this policy requires no empty line at the end of the file

Yeah we sort of have an issue for this actually. We will probably switch to regorus (rust OPA engine soon) and hopefully that will resolve.

@fitzthum fitzthum merged commit 6c32b58 into confidential-containers:main Feb 20, 2024
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants