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.

Sign up for GitHub

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

[release-v3.23] Auto pick #5910: Use TokenRequest API instead of calico-nodes service account #6218

Merged
merged 1 commit into from
Jun 14, 2022
Merged

[release-v3.23] Auto pick #5910: Use TokenRequest API instead of calico-nodes service account #6218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions app-policy/config/install/05-calico.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,13 @@ rules:
- get
- list
- watch
- apiGroups: [""]
resources:
- serviceaccounts/token
resourceNames:
- calico-node
verbs:
- create
---

apiVersion: rbac.authorization.k8s.io/v1beta1
Expand Down
8 changes: 8 additions & 0 deletions calico/_includes/charts/calico/templates/calico-node-rbac.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,14 @@ rules:
- pods/status
verbs:
- patch
# Used for creating service account tokens to be used by the CNI plugin
- apiGroups: [""]
resources:
- serviceaccounts/token
resourceNames:
- calico-node
Copy link

Choose a reason for hiding this comment

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

If the role containing this rule is also bound to the calico-node service account, it allows calico-node to mint tokens for calico-node. This creates a potential security issue where an attacker could steal a token and prevent their access from ever expiring by sending periodic TokenRequests for the same service account whose token they stole. Instead, it would be better to grant calico-node permission to request tokens for a separate service account that the CNI is intended to run as. For example, grant calico-node permission to mint tokens for a calico-node-cni service account, and grant calico-node-cni the permissions that the CNI needs. That way, no tokens have the ability to self-perpetuate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, I agree. I hadn't spotted this particular issue but already have a ticket tracking this enhancement for other reasons as well: #5921

verbs:
- create
# Calico monitors various CRDs for config.
- apiGroups: ["crd.projectcalico.org"]
resources:
Expand Down
8 changes: 8 additions & 0 deletions calico/getting-started/kubernetes/hardway/install-node.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,14 @@ rules:
- pods/status
verbs:
- patch
# Used for creating service account tokens to be used by the CNI plugin
- apiGroups: [""]
resources:
- serviceaccounts/token
resourceNames:
- calico-node
verbs:
- create
# Calico monitors various CRDs for config.
- apiGroups: ["crd.projectcalico.org"]
resources:
Expand Down
4 changes: 2 additions & 2 deletions cni-plugin/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,8 +207,8 @@ pkg/install/install.test: pkg/install/*.go

.PHONY: test-install-cni
## Test the install
test-install-cni: image pkg/install/install.test
cd pkg/install && CONTAINER_NAME=$(CNI_PLUGIN_IMAGE):latest-$(ARCH) ./install.test
test-install-cni: run-k8s-apiserver image pkg/install/install.test
cd pkg/install && CONTAINER_NAME=$(CNI_PLUGIN_IMAGE):latest-$(ARCH) CERTS_PATH=$(CERTS_PATH) ./install.test

###############################################################################
# CI/CD
Expand Down
12 changes: 11 additions & 1 deletion cni-plugin/pkg/install/install.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import (

"github.com/projectcalico/calico/libcalico-go/lib/logutils"
"github.com/projectcalico/calico/libcalico-go/lib/names"
"github.com/projectcalico/calico/node/pkg/cni"
)

type config struct {
Expand Down Expand Up @@ -476,7 +477,16 @@ contexts:
user: calico
current-context: calico-context`

data = strings.Replace(data, "TOKEN", kubecfg.BearerToken, 1)
clientset, err := cni.BuildClientSet()
if err != nil {
logrus.WithError(err).Fatal("Unable to create client for generating CNI token")
}
tr := cni.NewTokenRefresher(clientset, cni.NamespaceOfUsedServiceAccount(), cni.DefaultServiceAccountName)
tu, err := tr.UpdateToken()
if err != nil {
logrus.WithError(err).Fatal("Unable to create token for CNI kubeconfig")
}
data = strings.Replace(data, "TOKEN", tu.Token, 1)
data = strings.Replace(data, "__KUBERNETES_SERVICE_PROTOCOL__", getEnv("KUBERNETES_SERVICE_PROTOCOL", "https"), -1)
data = strings.Replace(data, "__KUBERNETES_SERVICE_HOST__", getEnv("KUBERNETES_SERVICE_HOST", ""), -1)
data = strings.Replace(data, "__KUBERNETES_SERVICE_PORT__", getEnv("KUBERNETES_SERVICE_PORT", ""), -1)
Expand Down
80 changes: 80 additions & 0 deletions cni-plugin/pkg/install/install_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
package install

import (
"context"
"fmt"
"io/ioutil"
"math/rand"
Expand All @@ -13,6 +14,11 @@ import (
. "github.com/onsi/gomega"

log "github.com/sirupsen/logrus"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/tools/clientcmd"
"k8s.io/client-go/tools/clientcmd/api"
)

var expectedDefaultConfig string = `{
Expand Down Expand Up @@ -79,13 +85,16 @@ func runCniContainer(tempDir string, binFolderWriteable bool, extraArgs ...strin
}
args := []string{
"run", "--rm", "--name", name,
"--net=host",
"-e", "SLEEP=false",
"-e", "KUBERNETES_SERVICE_HOST=127.0.0.1",
"-e", "KUBERNETES_SERVICE_PORT=6443",
"-e", "KUBERNETES_NODE_NAME=my-node",
"-e", "KUBECONFIG=/home/user/certs/kubeconfig",
"-v", tempDir + "/bin:" + binFolder,
"-v", tempDir + "/net.d:/host/etc/cni/net.d",
"-v", tempDir + "/serviceaccount:/var/run/secrets/kubernetes.io/serviceaccount",
"-v", os.Getenv("CERTS_PATH") + ":/home/user/certs",
}
args = append(args, extraArgs...)
image := os.Getenv("CONTAINER_NAME")
Expand All @@ -99,6 +108,54 @@ func runCniContainer(tempDir string, binFolderWriteable bool, extraArgs ...strin
return err
}

func createKubernetesClient() *kubernetes.Clientset {
certsPath := os.Getenv("CERTS_PATH")
if len(certsPath) == 0 {
Fail("CERTS_PATH env variable not set")
}
kubeconfigPath := certsPath + "/kubeconfig"
kubeconfigData, err := ioutil.ReadFile(kubeconfigPath)
if err != nil {
Fail(fmt.Sprintf("Failed to read kubeconfig file: %v", err))
}
// The client certificate/key do not necessarily reside in the location specified by kubeconfig => patch it directly
config, err := clientcmd.Load(kubeconfigData)
if err != nil {
Fail(fmt.Sprintf("Failed to load kubeconfig: %v", err))
}
certificate, err := ioutil.ReadFile(certsPath + "/admin.pem")
if err != nil {
Fail(fmt.Sprintf("Failed to read admin client certificate: %v", err))
}
key, err := ioutil.ReadFile(certsPath + "/admin-key.pem")
if err != nil {
Fail(fmt.Sprintf("Failed to read admin client key: %v", err))
}

overrides := &clientcmd.ConfigOverrides{
AuthInfo: api.AuthInfo{
ClientCertificate: "",
ClientCertificateData: certificate,
ClientKey: "",
ClientKeyData: key,
},
}
adminAuthInfo := config.AuthInfos["admin"]
adminAuthInfo.ClientCertificate = ""
adminAuthInfo.ClientCertificateData = certificate
adminAuthInfo.ClientKey = ""
adminAuthInfo.ClientKeyData = key
kubeconfig, err := clientcmd.NewDefaultClientConfig(*config, overrides).ClientConfig()
if err != nil {
Fail(fmt.Sprintf("Failed to create kubeconfig: %v", err))
}
clientset, err := kubernetes.NewForConfig(kubeconfig)
if err != nil {
Fail(fmt.Sprintf("Could not create kubernetes client: %v", err))
}
return clientset
}

var _ = Describe("CNI installation tests", func() {
var tempDir string
BeforeEach(func() {
Expand Down Expand Up @@ -158,6 +215,29 @@ PuB/TL+u2y+iQUyXxLy3
if err != nil {
Fail(fmt.Sprintf("Failed to write k8s CA file for test: %v", err))
}

// Create namespace file for token refresh
k8sNamespace := []byte("kube-system")
var namespaceFile = fmt.Sprintf("%s/serviceaccount/namespace", tempDir)
err = ioutil.WriteFile(namespaceFile, k8sNamespace, 0755)
if err != nil {
Fail(fmt.Sprintf("Failed to write k8s namespace file: %v", err))
}

// Create calico-node service account
serviceAccount := &v1.ServiceAccount{
ObjectMeta: metav1.ObjectMeta{
Name: "calico-node",
},
}
_, err = createKubernetesClient().CoreV1().ServiceAccounts("kube-system").Create(context.Background(), serviceAccount, metav1.CreateOptions{})
Expect(err).NotTo(HaveOccurred())
})

AfterEach(func() {
// Cleanup calico-node service account
err := createKubernetesClient().CoreV1().ServiceAccounts("kube-system").Delete(context.Background(), "calico-node", metav1.DeleteOptions{})
Expect(err).NotTo(HaveOccurred())
})

Context("Install with default values", func() {
Expand Down
4 changes: 2 additions & 2 deletions node/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,10 +292,10 @@ fv: run-k8s-apiserver
-v $(CERTS_PATH):/home/user/certs \
-e KUBECONFIG=/go/src/github.com/projectcalico/calico/hack/test/certs/kubeconfig \
-e ETCD_ENDPOINTS=http://$(LOCAL_IP_ENV):2379 \
$(CALICO_BUILD) ginkgo -cover -r -skipPackage vendor pkg/lifecycle/startup pkg/allocateip $(GINKGO_ARGS)
$(CALICO_BUILD) ginkgo -cover -r -skipPackage vendor pkg/lifecycle/startup pkg/allocateip pkg/cni $(GINKGO_ARGS)

# Skip packages containing FV tests.
UT_PACKAGES_TO_SKIP?=pkg/lifecycle/startup,pkg/allocateip,pkg/status
UT_PACKAGES_TO_SKIP?=pkg/lifecycle/startup,pkg/allocateip,pkg/status,pkg/cni
.PHONY: ut
ut:
$(DOCKER_GO_BUILD) sh -c '$(GIT_CONFIG_SSH) ginkgo -r -skipPackage=$(UT_PACKAGES_TO_SKIP) $(GINKGO_ARGS)'
Expand Down
Loading