-
Notifications
You must be signed in to change notification settings - Fork 84
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
SecureComms: Add Support for activating using InitData #2072
base: main
Are you sure you want to change the base?
SecureComms: Add Support for activating using InitData #2072
Conversation
8630009
to
206ce0f
Compare
cc: @bpradipt |
Install KBS and test SecureComms with KBS Based on confidential-containers#2072 which should be merged first Signed-off-by: David Hadas <david.hadas@gmail.com>
Install KBS and test SecureComms with KBS Based on confidential-containers#2072 which should be merged first Signed-off-by: David Hadas <david.hadas@gmail.com>
Install KBS and test SecureComms with KBS Based on confidential-containers#2072 which should be merged first Signed-off-by: David Hadas <david.hadas@gmail.com>
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.
Some initial comments. It would be good to fix the typos in the commit message too.
@@ -52,6 +54,8 @@ func load(path string, obj interface{}) error { | |||
return fmt.Errorf("failed to decode a Agent Protocol Forwarder config file file: %s: %w", path, err) | |||
} | |||
|
|||
logger.Printf("succesful loading config from %s\n", path) |
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 this supposed to say "successfully loaded config..."?
@@ -114,7 +114,7 @@ func (cfg *daemonConfig) Setup() (cmd.Starter, error) { | |||
flags.BoolVar(&secureComms, "secure-comms", false, "Use SSH to secure communication between cluster and peer pods") | |||
flags.StringVar(&secureCommsInbounds, "secure-comms-inbounds", "", "Inbound tags for secure communication tunnels") | |||
flags.StringVar(&secureCommsOutbounds, "secure-comms-outbounds", "", "Outbound tags for secure communication tunnels") | |||
flags.StringVar(&secureCommsKbsAddr, "secure-comms-kbs", "kbs-service.kbs-operator-system:8080", "Address of a KBS Service for Secure-Comms") | |||
flags.StringVar(&secureCommsKbsAddr, "secure-comms-kbs", "kbs-service.trustee-operator-system:8080", "Address of a KBS Service for Secure-Comms") |
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 trustee operator namespace changes should be in a separate commit with an explanation that it's triggered by the change in the trustee-operator project
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.
I've just noticed a bunch of these change are in #2073. Is this PR supposed to depend on that one?
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.
Yes - #2073 should be merged first
```sh | ||
kubectl get secrets -n trustee-operator-system | ||
NAME TYPE DATA AGE | ||
kbs-auth-public-key Opaque 1 28h | ||
kbs-client Opaque 1 28h | ||
``` |
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's the reason for this command being added?
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.
I've just seen this code is under #2065 now. What's going on with these PR and their duplication of code?
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.
This is a slight improvement to the SecureComms doc which shows the correct result after following the instructions trustee operator and following the recommendation: "Make sure to uncomment the secret generation as recommended for both public and private key (kbs-auth-public-key
and kbs-client
secrets). "
We can add it to #2073 if it will make things clearer
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.
I prefer to leave this extra documentation detail as is, although it also appears in #2065 - unless someone finds this very disturbing.
kubectl -n confidential-containers-system get cm peer-pods-cm -o yaml | sed "s/SECURE_COMMS: \"false\"/SECURE_COMMS: \"true\"/"|kubectl apply -f - | ||
``` | ||
|
||
Set InitData to point KBC services to IP address 127.0.0.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.
Should this have a heading like Build a podvm that enforces Secure-Comms (Optional)
section does as it's an alternative to 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.
Some expansion of the explanation of what this is doing and why would be nice.
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.
I will be adapting the documentation based on this comment. Please reconsider the next version.
@@ -114,7 +114,7 @@ func (cfg *daemonConfig) Setup() (cmd.Starter, error) { | |||
flags.BoolVar(&secureComms, "secure-comms", false, "Use SSH to secure communication between cluster and peer pods") | |||
flags.StringVar(&secureCommsInbounds, "secure-comms-inbounds", "", "Inbound tags for secure communication tunnels") | |||
flags.StringVar(&secureCommsOutbounds, "secure-comms-outbounds", "", "Outbound tags for secure communication tunnels") | |||
flags.StringVar(&secureCommsKbsAddr, "secure-comms-kbs", "kbs-service.kbs-operator-system:8080", "Address of a KBS Service for Secure-Comms") | |||
flags.StringVar(&secureCommsKbsAddr, "secure-comms-kbs", "kbs-service.trustee-operator-system:8080", "Address of a KBS Service for Secure-Comms") |
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.
I've just noticed a bunch of these change are in #2073. Is this PR supposed to depend on that one?
```sh | ||
kubectl get secrets -n trustee-operator-system | ||
NAME TYPE DATA AGE | ||
kbs-auth-public-key Opaque 1 28h | ||
kbs-client Opaque 1 28h | ||
``` |
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.
I've just seen this code is under #2065 now. What's going on with these PR and their duplication of code?
206ce0f
to
1a5d0a9
Compare
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.
I understand that kata is proceeding with an init-data approach that is passed from the runtime to the agent via a SetInitData()
RPC call for the upcoming CoCo release.
This looks like a CAA-specific extension to InitData. If the project adopts kata's init-data approach, would this still work?
Add apj.json to InitData apf.josn includes a secure-comms key used to activate secure comms (if not already activated using an agent-protocol-forwarder.service flag) Signed-off-by: David Hadas <david.hadas@gmail.com>
1a5d0a9
to
914490c
Compare
@huoqifeng, can you please review this PR as it extends your work in #1895 |
@mkulke, thanks for pointing this,
This PR extends #1895 which introduced a CAA-specific extension to InitData. Here we added configuration of the APF to the already merged configuration of AA and CDH. Regardless, it seems that the |
Can you elaborate on why a SetInitData() RPC wouldn't work for CAA? I think I POC'd that approach when the init-data design was being discussed to make sure it would work and I didn't run into issues. My understanding is that agent-protocol-forwarder, daemon.json and user-data are implementation details of CAA that are responsible for setting the stage to allow runtime <=> agent communication and kata should ideally not be concerned about those. |
In your prototype did you keep the guest-components managed by systemd, or the kata-agent? It seems to be that in if the kata-agent isn't managing them and they are started before the kata-agent then relying on config from kata-agent endpoint doesn't really make sense as we then have this weird undefined start until the kata is up and running and receiving the setInitData request. In fairness I'm still not sure why bare-metal doesn't adopt the approach of systemd managing the processes like the peer pod does anyway to avoid this either, so I'm obviously missing something. |
To add to what @stevenhorsman indicated, we use attestation when connecting CAA to APF in SecureComms. I would assume that allowing someone to change the InitData (using the kata agent RPC) after attestation was already done and after keys delivered to the podvm, is not what we want to do. Am I missing something? |
I would have to dig a bit to find the details, but I think in the PoC code kata-agent was talking via ttRPC to attestation-agent to update the configuration, so it was indeed half-initialized. however, that endpoint is about to be removed as I understand, though. somehow AA will now have to perform a binding to the TEE itself (e.g. compare host-data, extend an init-data PCR). |
I understand this applies if you use the secure communications feature, but in a default CAA installation there is no attestation ceremony before runtime <=> agent communication is established, afaik? |
See:
Add
apj.json
to InitDataapf.josn
includes ansc
key used to activate secure comms (if not already activated using an agent-protocol-forwarder.service flag)