[barbican-kms-plugin] Pass KeyId to EncryptResponse

[GlassOfWhiskey]

What this PR does / why we need it:

The KMSv2 API requires to pass the remote KeyID in the EncryptResponse object. Indeed, the documentation of the EncryptResponse ProtoBuf object says:

message EncryptResponse {
    // The encrypted data.
    // ciphertext must satisfy the following constraints:  
    // 1. The ciphertext is not empty.  
    // 2. The ciphertext is less than 1 kB.
    bytes ciphertext = 1;
    // The KMS key ID used to encrypt the data. This must always refer to the KMS KEK and not any local KEKs that may be in use.
    // This can be used to inform staleness of data updated via value.Transformer.TransformFromStorage.
    // keyID must satisfy the following constraints:
    // 1. The keyID is not empty.
    // 2. The size of keyID is less than 1 kB.
    string key_id = 2;
    // Additional metadata to be stored with the encrypted data.
    // This data is stored in plaintext in etcd. KMS plugin implementations are responsible for pre-encrypting any sensitive data.
    // Annotations must satisfy the following constraints:
    //  1. Annotation key must be a fully qualified domain name that conforms to the definition in DNS (RFC 1123).
    //  2. The size of annotations keys + values is less than 32 kB.
    map<string, bytes> annotations = 3;
}

This PR implements this requirement in the Barbican KMS Plugin.

Release note:

Propagate KeyID in the EncryptResponse object as per KMSv2 API spec

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: GlassOfWhiskey / name: Iacopo Colonnelli (50266a6bb4c098b4a130fabb649a2d01b3f9008e)

[k8s-ci-robot]

Welcome @GlassOfWhiskey!

It looks like this is your first PR to kubernetes/cloud-provider-openstack 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/cloud-provider-openstack has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. :smiley:

[k8s-ci-robot]

Hi @GlassOfWhiskey. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[dulek]

/ok-to-test

[dulek]

Please fix the release note in the PR description and best if you'll use the repo template.

[dulek]

What are the symptoms of not sending the keyId?

[GlassOfWhiskey]

What are the symptoms of not sending the keyId?

The ValidateKeyID check fails and the KMS does not encrypt secrets.

[GlassOfWhiskey]

Please fix the release note in the PR description and best if you'll use the repo template.

Added

[GlassOfWhiskey]

Anything else to do here?

[dulek]

/lgtm

I'd like to leave the final approval to someone who has more experience with KMS. @kayrus? @zetaab?

[zetaab]

I do not have any exp with kms in openstack

[GlassOfWhiskey]

Hi @dulek , Do we really need to wait so much time to progress on a 1-line PR that enables a plugin to finally work as expected?

[dulek]

Hi @dulek , Do we really need to wait so much time to progress on a 1-line PR that enables a plugin to finally work as expected?

Unfortunately yes, we tend to try to have at least two reviews before merging and personally I never used Barbican KMS, so don't feel confident enough to accept this. Also there's not Barbican KMS CI either.

I'm trying to deploy this now myself, once I confirm this works as expected I'll merge the patch. It'll probably be tomorrow morning.

[dulek]

@GlassOfWhiskey: So after quite a lot of fighting I deployed this on my env and tried version with and without your patch. Peaking into the DB I can see that in both cases data is saved encrypted and also K8s is able to decrypt it. I understand the protobuf definition mismatch here, but I'd like to actually see the error you're describing. How do I get to this?

[GlassOfWhiskey]

Hi @dulek, I was using this plugin with Kubernetes v1.29.0. Without passing the keyID object, the secrets were not encrypted inside etcd and the kube-apiserver logs were reporting the keyID is empty error generated by the ValidateKeyID method in the kmsv2 API.

This is the EncryptionConfiguration file I used:

apiVersion: v1
kind: EncryptionConfig
resources:
- providers:
  - kms:
      apiVersion: v2
      endpoint: unix:///root/kms/kms.sock
      name: barbican
      timeout: 10s
  - identity: {}
  resources:
  - secrets

And this is the DeamonSet I used to run the barbican-kms-plugin:

apiVersion: apps/v1
kind: DaemonSet
metadata:
  labels:
    cdk-addons: 'true'
    cdk-restart-on-ca-change: 'true'
    k8s-app: barbican-kms
  name: barbican-kms
  namespace: kube-system
spec:
  selector:
    matchLabels:
      k8s-app: barbican-kms
  template:
    metadata:
      labels:
        k8s-app: barbican-kms
    spec:
      containers:
      - args:
        - /bin/barbican-kms-plugin
        - --socketpath=$(KMS_ENDPOINT)
        - --cloud-config=$(CLOUD_CONFIG)
        env:
        - name: CLOUD_CONFIG
          value: /etc/config/cloud.conf
        - name: KMS_ENDPOINT
          value: /root/kms/kms.sock
        image: alphaunito/barbican-kms-plugin:v1.29.0
        livenessProbe:
          exec:
            command:
            - ls
            - $(KMS_ENDPOINT)
          failureThreshold: 5
          initialDelaySeconds: 10
          periodSeconds: 60
          timeoutSeconds: 10
        name: barbican-kms
        volumeMounts:
        - mountPath: /etc/config
          name: cloud-config-volume
        - mountPath: /root/kms/
          name: socket-dir
      hostNetwork: true
      nodeSelector:
        node-role.kubernetes.io/control-plane: ''
      serviceAccountName: cloud-controller-manager
      tolerations:
      - effect: NoSchedule
        key: node.cloudprovider.kubernetes.io/uninitialized
        value: 'true'
      - effect: NoSchedule
        key: node-role.kubernetes.io/master
      - effect: NoSchedule
        key: node-role.kubernetes.io/control-plane
      volumes:
      - name: cloud-config-volume
        secret:
          secretName: cloud-config
      - hostPath:
          path: /root/kms/
          type: DirectoryOrCreate
        name: socket-dir
  updateStrategy:
    type: RollingUpdate

The alphaunito/barbican-kms-plugin:v1.29.0 container is a custom version of the barbican-kms-plugin container that I created to test this patch, but this is the only different between it and the master branch of this repository

[dulek]

/approve

Okay, I think I observed the problem.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dulek

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• ~~OWNERS~~ [dulek]

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[dulek]

#2561 is also a result of this investigation.