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

Add new ctrdtaskapi package for shim task API support. #1485

Merged
merged 3 commits into from
Sep 8, 2022

Conversation

anmaxvl
Copy link
Contributor

@anmaxvl anmaxvl commented Aug 20, 2022

Add new typeurl registered data structure to represent
additional security policy constraint fragments, which
can be passed as part of shim's task Update request.

Change behavior of UpdateContainerConstraints to return
an error when an invalid resource is passed.

Make sure hcsshim can consume the new resource as part
of task Update request handling and new GCS protocol
message can be properly accepted by the guest.

Signed-off-by: Maksim An maksiman@microsoft.com

@anmaxvl anmaxvl requested a review from a team as a code owner August 20, 2022 00:30
@anmaxvl
Copy link
Contributor Author

anmaxvl commented Aug 20, 2022

@KenGordon this is how fragments will be eventually presented in GCS. FYI @SeanTAllen @matajoh

@KenGordon
Copy link
Collaborator

What does crtd stand for? I am guessing this is crtd task api? Jst so I can keep it in my head better that a random string

@anmaxvl
Copy link
Contributor Author

anmaxvl commented Aug 20, 2022

What does crtd stand for? I am guessing this is crtd task api? Jst so I can keep it in my head better that a random string

short for ContainerD 😄

Copy link
Contributor

@helsaawy helsaawy left a comment

Choose a reason for hiding this comment

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

The actual InjectFragment implementations is coming soon, I imagine?

@helsaawy helsaawy self-assigned this Aug 22, 2022
@anmaxvl
Copy link
Contributor Author

anmaxvl commented Aug 22, 2022

The actual InjectFragment implementations is coming soon, I imagine?

yup

pkg/ctrdtaskapi/update.go Outdated Show resolved Hide resolved
@katiewasnothere
Copy link
Contributor

We already have the ExtendedTask grpc service meant to extend the TaskAPI for our shim. How does this relate? I feel like it's slightly confusing

@anmaxvl
Copy link
Contributor Author

anmaxvl commented Aug 23, 2022

We already have the ExtendedTask grpc service meant to extend the TaskAPI for our shim. How does this relate? I feel like it's slightly confusing

We need to pass in a new resource type when sending a container update request. This package is meant to have those additional resource definitions. Adding new rpc call to ExtendedTask didn't seem necessary in this case.

internal/uvm/update_uvm.go Outdated Show resolved Hide resolved
Fragment string `json:"fragment,omitempty"`
// Annotations hold arbitrary additional information that can be used to
// (e.g.) provide more context about Fragment.
Annotations map[string]string `json:"annotations,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Do we have an expectation of what this will be used for? Pretty sure the overall update request already takes annotations, so rather we not just add it onto everything for future proofing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We briefly discussed with @KenGordon about potentially sending a "media-type", so that the guest would be able to process different content, e.g., tar, raw bytes etc. But I agree that we can add it later if we ever have a need to extend supported fragment formats

Copy link
Member

Choose a reason for hiding this comment

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

Is the fragment format today a raw string of Rego? Do we know of cases where that will need to change in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as far as I know yes, it's a raw string of Rego. @KenGordon or @matajoh can we get your input here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

At this level the payload is a COSE_Sign1 document base64 encoded so we can check the signatures inside the UVM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, right. I should've known this 🤦‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to rename this field to reflect this? Or at least add a comment stating it's a COSE_Sign1 doc with base64 encoding?

Copy link
Member

@kevpar kevpar left a comment

Choose a reason for hiding this comment

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

Left a few comments

@anmaxvl anmaxvl force-pushed the ctrdtaskapi-fragment branch 2 times, most recently from 7434f36 to 5d9408d Compare September 8, 2022 18:23
Copy link
Member

@kevpar kevpar left a comment

Choose a reason for hiding this comment

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

LGTM

Add new typeurl registered data structure to represent
additional security policy constraint fragments, which
can be passed as part of shim's task Update request.

Change behavior of UpdateContainerConstraints to return
an error when an invalid resource is passed.

Make sure hcsshim can consume the new resource as part
of task Update request handling and new GCS protocol
message can be properly accepted by the guest.

Signed-off-by: Maksim An <maksiman@microsoft.com>
Signed-off-by: Maksim An <maksiman@microsoft.com>
Signed-off-by: Maksim An <maksiman@microsoft.com>
@anmaxvl anmaxvl merged commit e374b8d into microsoft:main Sep 8, 2022
@anmaxvl anmaxvl deleted the ctrdtaskapi-fragment branch September 8, 2022 20:36
@SeanTAllen
Copy link
Contributor

I did a little jig when I saw this was merged.

anmaxvl added a commit to anmaxvl/hcsshim that referenced this pull request Sep 14, 2022
)

Add new typeurl registered data structure to represent
additional security policy constraint fragments, which
can be passed as part of shim's task Update request.

Rename `UpdateContainerConstraints` to `Update` and
change the behavior to return an error when an invalid
resource is passed.

Make sure hcsshim can consume the new resource as part
of task Update request handling and new GCS protocol
message can be properly accepted by the guest.

Signed-off-by: Maksim An <maksiman@microsoft.com>
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.

None yet

7 participants