certificatesigningrequests/v1beta1 to v1

[joelddiaz]

Issue #, if available:

Description of changes:

Kube 1.22 drops support for certificatesigningrequests/v1beta1. Update vendor/k8s.io/client-go and migrate to certificatesigningrequests/v1.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[2uasimojo]

@jqmichael would you be able to (find someone to) review this? Thanks!

[danquack]

is there any updates on this?

[2uasimojo]

@micahhausler @nckturner ?

[danquack]

@jyotimahapatra @wongma7 ?

[jim-barber-he]

I tried this in a new 1.22.2 Kubernetes cluster and it wasn't quite enough.
With this patch the CSRs for the pod-identity-webhook are now created (where as before they failed to be created at all).
However when you go to approve them they end up in an Approved,Failed state:

NAME        AGE   SIGNERNAME                      REQUESTOR                                                REQUESTEDDURATION   CONDITION
csr-7hfzn   29s   kubernetes.io/kubelet-serving   system:serviceaccount:kube-system:pod-identity-webhook   <none>              Approved,Failed
csr-p5rkd   19s   kubernetes.io/kubelet-serving   system:serviceaccount:kube-system:pod-identity-webhook   <none>              Approved,Failed
csr-sl426   24s   kubernetes.io/kubelet-serving   system:serviceaccount:kube-system:pod-identity-webhook   <none>              Approved,Failed

When you view the status of the failed CSRs the have:

  - lastTransitionTime: "2021-10-26T00:46:18Z"
    lastUpdateTime: "2021-10-26T00:46:18Z"
    message: subject organization is not system:nodes
    reason: SignerValidationFailure
    status: "True"
    type: Failed

This page: https://kubernetes.io/docs/reference/access-authn-authz/certificate-signing-requests/#kubernetes-signers
shows that when the kubernetes.io/kubelet-serving signer is used, the Common name for the certificate requests must begin with system:node: and have an Organization set to ["system:nodes"].
I made the following change to main.go at the top of the repository and now everything works.

diff --git a/main.go b/main.go
index 41b7d86..b80159c 100644
--- a/main.go
+++ b/main.go
@@ -152,7 +152,10 @@ func main() {
 
        if *inCluster {
                csr := &x509.CertificateRequest{
-                       Subject: pkix.Name{CommonName: fmt.Sprintf("%s.%s.svc", *serviceName, *namespaceName)},
+                       Subject: pkix.Name{
+                               CommonName:   fmt.Sprintf("system:node:%s.%s.svc", *serviceName, *namespaceName),
+                               Organization: []string{"system:nodes"},
+                       },
                        DNSNames: []string{
                                fmt.Sprintf("%s", *serviceName),

Feel free to add the fix to this PR (and adjust if necessary, I'm only a beginner at golang).

[danopia]

Kubernetes v1.23 is out now, so this project is now 2 minors behind the latest Kubernetes release.

The fact that the system:node: prefix is required means that something is being done incorrectly I believe.. the CSR API is intended for grabbing certificates that the whole cluster trusts, but webhooks don't need that level of certificate. They just need to have their caBundle put into the MutatingWebhookConfiguration resource. It's still good IMO to get csr/v1 merged to work on newer Kubernetes but there will be future work to be a better Kubernetes citizen and not need kubectl certificate approve just to register a webhook.

[h3poteto]

but webhooks don't need that level of certificate. They just need to have their caBundle put into the MutatingWebhookConfiguration resource.

I agree. I think it's good to support cert-manager based deployments in the future. Refs: #94

[h3poteto]

With this patch the CSRs for the pod-identity-webhook are now created (where as before they failed to be created at all).
However when you go to approve them they end up in an Approved,Failed state:

I have the same issue, and it is solved by that change. The system:node prefix is required as long as we continue to use the kubernetes.io/kubelet-serving as a signer.
So could you please add that change in this PR, @joelddiaz ? Or shall I do it with a new PR?

[wongma7]

Yes it seems like requesting kubelet-serving with system:node prefix is the path of least resistance for maintaining roughly the same functionality/behavior in 1.22 as if legacy-unknown were allowed. So I am inclined to merge this with the system:node prefix change. I understand the sentiment about it being "wrong", it does feel a bit to me like abuse of the API since obviously the webhook is not a kubelet. But it seems also like other solutions require third party software and/or more significant changes.

It might be "wrong" for the webhook to be getting its own serving cert in the first place...it is mimicking what kubelet does. From what I have seen, it is more common a pattern to have the cert generation be the responsibility of the deployer as a prerequisite to webhook deployment. So for example, another aws maintained webhook https://github.com/kubernetes-sigs/aws-iam-authenticator has an optional step where certs are generated before deployment. As mentioned in #94 if the webhook could just read the cert from file and reload then it could be ignorant of how certs are generated, by cert-manager, manually or whatever.

Another complication is that AWS EKS does NOT run the kubernetes default signer. So having the webhook create a kublet-serving cert might not work on EKS. But EKS 1.22 is not released yet so that's not really a problem yet

[h3poteto]

But EKS 1.22 is not released yet so that's not really a problem yet

Oh, sure...

[wongma7]

😂 don't worry, it's on AWS's radar to fix the problem before release.

If you have time to submit a PR with the change if @joelddiaz is not available (to be fair, we took very long to review this PR!) I can approve it. Thanks

[wongma7]

Actually I have time to submit a PR now, will link to it in a minute and appreciate reviews. thx.

[wongma7]

#133

[wongma7]

#133 is up for review , (sorry for noise, I sent previous comment with just hte link )

and btw when I said 'not a problem' yet I meant for EKS users specifically. Obviously this repository has a wider audience and I'll try to prioritize the cert-manager fix even if EKS doesn't necessarily officially support that as an add-on or whatever.

[wongma7]

Solution I'm thinking of is (cross-posting from #133 (comment) for awareness, apologies again for all the noise):

• if --in-cluster=true then you must provide signerName (and common name and org, as new arguments). So for non-EKS users they can set these to kubelet-serving and system:node accordingly. And for other distros that have their custom signer they might support other signerNames and such.
• if --in-cluster=false then Use certwatcher to support mounting cert-manager certificates. #134 should fix the bug where cert never gets reloaded if it's renewed so that fixes the cert-manager use-case

[danquack]

Looks like AWS released EKS 1.22 3 days ago, but noticed this did not make it. Is a different work around for getting this deployed with 1.22? Not sure if you would know @wongma7

[baalajimaestro]

Hi, is there an update or any workaround for this? Looking to update my cluster to 1.22.

[danopia]

It looks like the certificates API was switched to v1 in #134 which also updated client-go to 0.23 (huge diff though, so hard to see all that changed)

I believe cert-manager is the recommended way forward now, mentioned in README:

After version v0.3.0, --in-cluster=true no longer works and is deprecated. Please use --in-cluster=false and manage the cluster certificate with cert-manager or some other external certificate provisioning system. This is because certificates using the legacy-unknown signer are no longer signed when using the v1 certificates API.

And associated PR #139 adds the YAML for this deployment style.

[baalajimaestro]

I clearly don't know how this got added to my cluster infact, I do have cert-manager, but this specific thing got installed in my cluster. Any idea what might bring this up?

[clayrosenthal]

Any update on this? The solution below seems pretty reasonable to me

Solution I'm thinking of is (cross-posting from #133 (comment) for awareness, apologies again for all the noise):

* if --in-cluster=true then you must provide signerName (and common name and org, as new arguments). So for non-EKS users they can set these to kubelet-serving and system:node accordingly. And for other distros that have their custom signer they might support other signerNames and such.

* if --in-cluster=false then [Use certwatcher to support mounting cert-manager certificates. #134](https://github.com/aws/amazon-eks-pod-identity-webhook/pull/134) should fix the bug where cert never gets reloaded if it's renewed so that fixes the cert-manager use-case

[danopia]

Any update on this? The solution below seems pretty reasonable to me

I understand that everything's in place for --in-cluster=false. The deployment YAML in https://github.com/aws/amazon-eks-pod-identity-webhook/tree/master/deploy has been updated to use cert-manager with --in-cluster=false and I've been running this configuration in production for two years.