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

AA: handle multiline content in log events #615

Conversation

mkulke
Copy link
Contributor

@mkulke mkulke commented Jul 16, 2024

The NELR spec doesn't prescribe a format for the content payload of an event. However, since a line break is semantic in the log file, the content cannot be a multi-line string:

content = "one two\nthree four five"
               |
               v
my-domain my-operation one two
three four five

The result would be ambiguous. This change enforces this assertion. Also added tests for the event log and moved the init log line generation to the eventlog module (as the digest calc is already there).

@mkulke mkulke requested a review from jialez0 as a code owner July 16, 2024 13:17
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.

Nice catch. Besides line separators, spaces ' ' are also need to be carefully handled. In the original PR on AS side https://github.com/confidential-containers/trustee/pull/408/files#diff-aa0f7de1cbbcb4efaafaaa30a4b065987cf9d16f1fa01cfdb4e05c1323a1c59eR27 I didnot care about this. Let me fix this issue

attestation-agent/attestation-agent/src/lib.rs Outdated Show resolved Hide resolved
@mkulke
Copy link
Contributor Author

mkulke commented Jul 17, 2024

Nice catch. Besides line separators, spaces ' ' are also need to be carefully handled. In the original PR on AS side https://github.com/confidential-containers/trustee/pull/408/files#diff-aa0f7de1cbbcb4efaafaaa30a4b065987cf9d16f1fa01cfdb4e05c1323a1c59eR27 I didnot care about this. Let me fix this issue

Indeed. the first two fields have similar problems, maybe there should be a formal spec document for the log file in the repo, defines which characters are legal, are multiple INIT statements legal, etc...

The NELR spec doesn't prescribe a format for the `content` payload of an
event. However, since a line break is semantic in the log file, the
content cannot be a multi-line string:

```
content = "one two\nthree four five"
               |
               v
my-domain my-operation one two
three four five
```

The result would be ambiguous. This change enforces this assertion.

Also added tests for the event log and moved the init log line
generation to the eventlog module (as the digest calc is already there)

Signed-off-by: Magnus Kulke <magnuskulke@microsoft.com>
@mkulke mkulke force-pushed the mkulke/handle-multiline-content-in-eventlog branch from 906abec to 88c7d34 Compare July 17, 2024 08:12
@mkulke mkulke requested a review from Xynnn007 July 17, 2024 09:24
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

@mkulke mkulke merged commit 4619b4b into confidential-containers:main Jul 17, 2024
5 checks passed
@mkulke mkulke deleted the mkulke/handle-multiline-content-in-eventlog branch July 17, 2024 12:04
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.

2 participants