confidential-containers / cloud-api-adaptor

Ability to create Kata pods using cloud provider APIs aka the peer-pods approach
Apache License 2.0
44 stars 71 forks source link

enable kbs cert for cdh #1875

Open wyuany opened 1 week ago

wyuany commented 5 days ago

Hi @mkulke, thank you very much! All the comments are addressed.

mkulke commented 4 days ago

Hi @mkulke, thank you very much! All the comments are addressed.

thanks for accommodating. I think we are missing the cert property in the aa.toml file for the kbs tokens, too.

In terms of testing it would be good to have this tested as part of the existing e2e test suite.

~I have been testing w/ self-signed certs for IPs, because in the current tests, the kbs deployment is exposed via node-port. Sadly self-signed IP certificates do not work well in the rustls stack, I only got those working when building AA + CDH w/ openssl.~ edit: see below

mkulke commented 4 days ago

The problem with self-signed certs is that rustls is a bit more strict than openssl and others, so if we want to specify the kbs cert directly, we have to mark it as CA:false

openssl req -x509 -out kbs.crt -keyout kbs.key -newkey rsa:2048 -nodes -sha256 \
      -subj '/CN=kbs.coco' \
      --addext "subjectAltName=IP:10.200.24.4" \
      --addext "basicConstraints=CA:FALSE"
mkulke commented 4 days ago

The problem with self-signed certs is that rustls is a bit more strict than openssl and others, so if we want to specify the kbs cert directly, we have to mark it as CA:false

openssl req -x509 -out kbs.crt -keyout kbs.key -newkey rsa:2048 -nodes -sha256 \
      -subj '/CN=kbs.coco' \
      --addext "subjectAltName=IP:10.200.24.4" \
      --addext "basicConstraints=CA:FALSE"

At the moment, the kbs deployment manifests hardcode http-only, so we can do the changes to the e2e tests in a follow-up PR. Please add a description to the PR, I'll trigger e2e tests.

wyuany commented 3 days ago

@mkulke Thank you very much for the details. I'll add description and address the e2e test in a new PR. There are lots of commits of this PR. Let me merge these commits into one.

mkulke commented 3 days ago

a couple of additional comments.

In general I'm not convinced that it's the best approach to specify the plain text of the cert as --kbc-cert "----BEGIN CERTIFICATE-----\n ..." maybe others can chime in

I tried something like this, so we can still specify a block of text in the peerpod configmap. we can write it into a file earlier and then pass the filename. In main.go we then need read the file into a []byte and pass it around. sounds like ceremonial back-and-forth, but it's probably better than having to deal with a large unreadable cmdline for the process.

[[ "${KBS_CERT}" ]] && echo -n "${KBS_CERT}" > /run/kbs.crt
[[ -f /run/kbs.crt ]] && optionals+="-kbs-cert-file /run/kbs.crt "