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.

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

VAULT-6936 support bound service account namespace selector #218

Merged
merged 21 commits into from
Dec 12, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
2 changes: 2 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ vault-image:
setup-integration-test: teardown-integration-test vault-image
kind --name $(KIND_CLUSTER_NAME) load docker-image hashicorp/vault:dev
kubectl --context="kind-$(KIND_CLUSTER_NAME)" create namespace test
kubectl --context="kind-$(KIND_CLUSTER_NAME)" label namespaces test target=integration-test other=label
helm upgrade --install vault vault --repo https://helm.releases.hashicorp.com --version=0.25.0 \
--kube-context="kind-$(KIND_CLUSTER_NAME)" \
--wait --timeout=5m \
Expand All @@ -63,6 +64,7 @@ setup-integration-test: teardown-integration-test vault-image
--set server.extraArgs="-dev-plugin-dir=/vault/plugin_directory"
kubectl --context="kind-$(KIND_CLUSTER_NAME)" apply --namespace=test -f integrationtest/vault/tokenReviewerServiceAccount.yaml
kubectl --context="kind-$(KIND_CLUSTER_NAME)" apply -f integrationtest/vault/tokenReviewerBinding.yaml
kubectl --context="kind-$(KIND_CLUSTER_NAME)" apply -f integrationtest/vault/namespaceControllerBinding.yaml
kubectl --context="kind-$(KIND_CLUSTER_NAME)" wait --namespace=test --for=condition=Ready --timeout=5m pod -l app.kubernetes.io/name=vault

.PHONY: teardown-integration-test
Expand Down
9 changes: 8 additions & 1 deletion backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,12 @@ type kubeAuthBackend struct {
// review. Mocks should only be used in tests.
reviewFactory tokenReviewFactory

// nsValidateFactory is used to configure the strategy for validating
// namespace properties (currently labels). Currently, the only options
// are using the kubernetes API or mocking the validation. Mocks should
// only be used in tests.
nsValidateFactory namespaceValidateFactory

// localSATokenReader caches the service account token in memory.
// It periodically reloads the token to support token rotation/renewal.
// Local token is used when running in a pod with following configuration
Expand Down Expand Up @@ -133,7 +139,8 @@ func Backend() *kubeAuthBackend {
// Set the default TLSConfig
tlsConfig: getDefaultTLSConfig(),
// Set the review factory to default to calling into the kubernetes API.
reviewFactory: tokenReviewAPIFactory,
reviewFactory: tokenReviewAPIFactory,
nsValidateFactory: namespaceValidateAPIFactory,
}

b.Backend = &framework.Backend{
Expand Down
5 changes: 1 addition & 4 deletions backend_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ func Test_kubeAuthBackend_updateTLSConfig(t *testing.T) {
for idx, config := range tt.configs {
t.Run(fmt.Sprintf("config-%d", idx), func(t *testing.T) {
if config.localCACert != "" {
if err := os.WriteFile(localFile, []byte(config.localCACert), 0600); err != nil {
if err := os.WriteFile(localFile, []byte(config.localCACert), 0o600); err != nil {
t.Fatalf("failed to write local file %q", localFile)
}
t.Cleanup(func() {
Expand Down Expand Up @@ -324,7 +324,6 @@ func Test_kubeAuthBackend_initialize(t *testing.T) {
err := b.initialize(ctx, tt.req)
if tt.wantErr && err == nil {
t.Errorf("initialize() error = %v, wantErr %v", err, tt.wantErr)

}

if !reflect.DeepEqual(err, tt.expectErr) {
Expand Down Expand Up @@ -442,7 +441,6 @@ func Test_kubeAuthBackend_runTLSConfigUpdater(t *testing.T) {
err := b.runTLSConfigUpdater(ctx, tt.storage, tt.horizon)
if tt.wantErr && err == nil {
t.Errorf("runTLSConfigUpdater() error = %v, wantErr %v", err, tt.wantErr)

}

if !reflect.DeepEqual(err, tt.expectErr) {
Expand Down Expand Up @@ -506,7 +504,6 @@ func assertTLSConfigEquals(t *testing.T, actual, expected *tls.Config) {
t.Errorf("updateTLSConfig() actual MinVersion = %v, expected MinVersion %v",
actual.MinVersion, expected.MinVersion)
}

}

func assertValidTransport(t *testing.T, b *kubeAuthBackend, expected *tls.Config) {
Expand Down
4 changes: 2 additions & 2 deletions caching_file_reader.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
package kubeauth

import (
"io/ioutil"
"os"
"sync"
"time"
)
Expand Down Expand Up @@ -58,7 +58,7 @@ func (r *cachingFileReader) ReadFile() (string, error) {
r.l.Lock()
defer r.l.Unlock()

buf, err := ioutil.ReadFile(r.path)
buf, err := os.ReadFile(r.path)
if err != nil {
return "", err
}
Expand Down
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -75,4 +75,5 @@ require (
k8s.io/utils v0.0.0-20230406110748-d93618cff8a2 // indirect
sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect
sigs.k8s.io/structured-merge-diff/v4 v4.2.3 // indirect
sigs.k8s.io/yaml v1.3.0 // indirect
)
1 change: 1 addition & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -332,3 +332,4 @@ sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd/go.mod h1:B8JuhiUyNFVKdsE8h6
sigs.k8s.io/structured-merge-diff/v4 v4.2.3 h1:PRbqxJClWWYMNV1dhaG4NsibJbArud9kFxnAMREiWFE=
sigs.k8s.io/structured-merge-diff/v4 v4.2.3/go.mod h1:qjx8mGObPmV2aSZepjQjbmb2ihdVs8cGKBraizNC69E=
sigs.k8s.io/yaml v1.3.0 h1:a2VclLzOGrwOHDiV8EfBGhvjHvP46CtW5j6POvhYGGo=
sigs.k8s.io/yaml v1.3.0/go.mod h1:GeOyir5tyXNByN85N/dRIT9es5UQNerPYEKK56eTBm8=
17 changes: 17 additions & 0 deletions helpers.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package kubeauth

import (
"net/http"
"strings"
)

func setRequestHeader(req *http.Request, bearer string) {
bearer = strings.TrimSpace(bearer)

// Set the JWT as the Bearer token
req.Header.Set("Authorization", bearer)

// Set the MIME type headers
req.Header.Set("Content-Type", "application/json")
req.Header.Set("Accept", "application/json")
}
5 changes: 2 additions & 3 deletions integrationtest/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@ module github.com/hashicorp/vault-plugin-auth-kubernetes/integrationtest
go 1.20

require (
github.com/hashicorp/go-version v1.6.0
github.com/hashicorp/vault/api v1.9.2
k8s.io/api v0.27.3
k8s.io/apimachinery v0.27.3
k8s.io/client-go v0.27.3
)

Expand Down Expand Up @@ -56,8 +57,6 @@ require (
gopkg.in/inf.v0 v0.9.1 // indirect
gopkg.in/yaml.v2 v2.4.0 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
k8s.io/api v0.27.3 // indirect
k8s.io/apimachinery v0.27.3 // indirect
k8s.io/klog/v2 v2.90.1 // indirect
k8s.io/kube-openapi v0.0.0-20230501164219-8b0f38b5fd1f // indirect
k8s.io/utils v0.0.0-20230209194617-a36077c30491 // indirect
Expand Down
2 changes: 0 additions & 2 deletions integrationtest/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,6 @@ github.com/hashicorp/go-secure-stdlib/strutil v0.1.2 h1:kes8mmyCpxJsI7FTwtzRqEy9
github.com/hashicorp/go-secure-stdlib/strutil v0.1.2/go.mod h1:Gou2R9+il93BqX25LAKCLuM+y9U2T4hlwvT1yprcna4=
github.com/hashicorp/go-sockaddr v1.0.2 h1:ztczhD1jLxIRjVejw8gFomI1BQZOe2WoVOu0SyteCQc=
github.com/hashicorp/go-sockaddr v1.0.2/go.mod h1:rB4wwRAUzs07qva3c5SdrY/NEtAUjGlgmH/UkBUC97A=
github.com/hashicorp/go-version v1.6.0 h1:feTTfFNnjP967rlCxM/I9g701jU+RN74YKx2mOkIeek=
github.com/hashicorp/go-version v1.6.0/go.mod h1:fltr4n8CU8Ke44wwGCBoEymUuxUHl09ZGVZPK5anwXA=
github.com/hashicorp/golang-lru v0.5.0/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8=
github.com/hashicorp/golang-lru v0.5.1/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8=
github.com/hashicorp/hcl v1.0.0 h1:0Anlzjpi4vEasTeNFn2mLJgTSwt0+6sfsiTG8qcWGx4=
Expand Down
25 changes: 25 additions & 0 deletions integrationtest/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,14 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

const (
matchLabelsKeyValue = `{
"matchLabels": {
"target": "integration-test"
}
}`
)

// Set the environment variable INTEGRATION_TESTS to any non-empty value to run
// the tests in this package. The test assumes it has available:
// - A Kubernetes cluster in which:
Expand Down Expand Up @@ -149,6 +157,23 @@ func TestSuccessWithTokenReviewerJwt(t *testing.T) {
}
}

func TestSuccessWithNamespaceLabels(t *testing.T) {
roleConfigOverride := map[string]interface{}{
"bound_service_account_names": "vault",
"bound_service_account_namespace_selector": matchLabelsKeyValue,
}
client, cleanup := setupKubernetesAuth(t, "vault", nil, roleConfigOverride)
defer cleanup()

_, err := client.Logical().Write("auth/kubernetes/login", map[string]interface{}{
"role": "test-role",
"jwt": createToken(t, "vault", nil),
})
if err != nil {
t.Fatalf("Expected successful login but got: %v", err)
}
}

func TestFailWithBadTokenReviewerJwt(t *testing.T) {
client, cleanup := setupKubernetesAuth(t, "vault", map[string]interface{}{
"kubernetes_host": "https://kubernetes.default.svc.cluster.local",
Expand Down
26 changes: 26 additions & 0 deletions integrationtest/vault/namespaceControllerBinding.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
name: test-namespacelister-account-binding
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: ClusterRole
name: system:controller:namespace-controller
subjects:
- kind: ServiceAccount
name: test-token-reviewer-account
namespace: test
---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
name: test-namespacelister-account-binding-vault
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: ClusterRole
name: system:controller:namespace-controller
subjects:
- kind: ServiceAccount
name: vault
namespace: test

118 changes: 118 additions & 0 deletions namespace_validate.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
package kubeauth

import (
"context"
"encoding/json"
"errors"
"fmt"
"io"
"net/http"
"strings"

v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
k8s_yaml "k8s.io/apimachinery/pkg/util/yaml"
)

// This exists so we can use a mock namespace validation when running tests
type namespaceValidator interface {
ValidateLabels(context.Context, *http.Client, string, string) (bool, error)
}

type namespaceValidateFactory func(*kubeConfig) namespaceValidator

// This is the real implementation that calls the kubernetes API
type namespaceValidateAPI struct {
config *kubeConfig
}

func namespaceValidateAPIFactory(config *kubeConfig) namespaceValidator {
return &namespaceValidateAPI{
config: config,
}
}

func (v *namespaceValidateAPI) ValidateLabels(ctx context.Context, client *http.Client, namespace string, namespaceSelector string) (bool, error) {
labelSelector, err := makeLabelSelector(namespaceSelector)
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't look like we are guarding against empty namespace, nor empty namespaceSelector. I think we might want to return an error if either one is empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The guarding step happens in path_role.go where bound_service_account_namespaces and bound_service_account_namespace_selector are set. With bound_service_account_namespace_selector, the PR allows either but not both of these fields to be empty.

// Verify namespaces is not empty unless selector is set
if len(role.ServiceAccountNamespaces) == 0 && role.ServiceAccountNamespaceSelector == "" {
return logical.ErrorResponse("%q can not be empty if %q is not set", "bound_service_account_namespaces", "bound_service_account_namespace_selector"), nil
}

ValidateLabels(...) is used in path_login.go where the request's token namespace meets the namespace condition. I think we don't need the guarding step here as path_login.go only references those fields.

valid := false
if role.ServiceAccountNamespaceSelector != "" {
if valid, err = b.namespaceValidatorFactory(config).validateLabels(ctx,
client, sa.namespace(), role.ServiceAccountNamespaceSelector); err != nil {
return nil, err
}
}
if !valid && len(role.ServiceAccountNamespaces) == 0 {
return nil, logical.CodedError(http.StatusForbidden, "namespace not authorized")
}
if !valid && (len(role.ServiceAccountNamespaces) > 1 || role.ServiceAccountNamespaces[0] != "*") {
if !strutil.StrListContainsGlob(role.ServiceAccountNamespaces, sa.namespace()) {
return nil, logical.CodedError(http.StatusForbidden, "namespace not authorized")
}
}

Let me know if I misunderstood your point anywhere or how I can improve the guarding step.

if err != nil {
return false, err
}
nsLabels, err := v.getNamespaceLabels(ctx, client, namespace)
if err != nil {
return false, err
}
selector, err := metav1.LabelSelectorAsSelector(&labelSelector)
if err != nil {
return false, err
}
return selector.Matches(labels.Set(nsLabels)), nil
}

func makeLabelSelector(selector string) (metav1.LabelSelector, error) {
labelSelector := metav1.LabelSelector{}
decoder := k8s_yaml.NewYAMLOrJSONDecoder(strings.NewReader(selector), len(selector))
err := decoder.Decode(&labelSelector)
if err != nil {
return labelSelector, err
}
return labelSelector, nil
}

func (v *namespaceValidateAPI) getNamespaceLabels(ctx context.Context, client *http.Client, namespace string) (map[string]string, error) {
url := fmt.Sprintf("%s/api/v1/namespaces/%s", strings.TrimSuffix(v.config.Host, "/"), namespace)
req, err := http.NewRequestWithContext(ctx, "GET", url, nil)
if err != nil {
return nil, err
}

// Use the configured TokenReviewer JWT as the bearer
if v.config.TokenReviewerJWT == "" {
return nil, errors.New("namespace lookup failed: TokenReviewer JWT needs to be configured to use namespace selectors")
}
bearer := fmt.Sprintf("Bearer %s", v.config.TokenReviewerJWT)
setRequestHeader(req, bearer)

resp, err := client.Do(req)
if err != nil {
return nil, err
}
body, err := io.ReadAll(resp.Body)
if err != nil {
return nil, err
}
if resp.StatusCode != 200 {
return nil, fmt.Errorf("failed to get namespace (code %d): %s", resp.StatusCode, body)
}
ns := v1.Namespace{}

err = json.Unmarshal(body, &ns)
if err != nil {
return nil, err
}
return ns.Labels, nil
}

type mockNamespaceValidator struct {
labels map[string]string
}

func mockNamespaceValidateFactory(labels map[string]string) namespaceValidateFactory {
return func(config *kubeConfig) namespaceValidator {
return &mockNamespaceValidator{
labels: labels,
}
}
}

func (v *mockNamespaceValidator) ValidateLabels(ctx context.Context, client *http.Client, namespace string, namespaceSelector string) (bool, error) {
labelSelector, err := makeLabelSelector(namespaceSelector)
if err != nil {
return false, err
}
selector, err := metav1.LabelSelectorAsSelector(&labelSelector)
if err != nil {
return false, err
}
return selector.Matches(labels.Set(v.labels)), nil
}
11 changes: 5 additions & 6 deletions path_config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ package kubeauth
import (
"context"
"crypto"
"io/ioutil"
"os"
"reflect"
"testing"
Expand All @@ -16,7 +15,7 @@ import (
)

func setupLocalFiles(t *testing.T, b logical.Backend) func() {
cert, err := ioutil.TempFile("", "ca.crt")
cert, err := os.CreateTemp("", "ca.crt")
if err != nil {
t.Fatal(err)
}
Expand All @@ -26,7 +25,7 @@ func setupLocalFiles(t *testing.T, b logical.Backend) func() {
}
cert.Close()

token, err := ioutil.TempFile("", "token")
token, err := os.CreateTemp("", "token")
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -473,7 +472,7 @@ func TestConfig_LocalJWTRenewal(t *testing.T) {
defer cleanup()

// Create temp file that will be used as token.
f, err := ioutil.TempFile("", "renewed-token")
f, err := os.CreateTemp("", "renewed-token")
if err != nil {
t.Error(err)
}
Expand All @@ -490,7 +489,7 @@ func TestConfig_LocalJWTRenewal(t *testing.T) {
token2 := "after-renewal"

// Write initial token to the temp file.
err = ioutil.WriteFile(f.Name(), []byte(token1), 0o644)
err = os.WriteFile(f.Name(), []byte(token1), 0o644)
if err != nil {
t.Error(err)
}
Expand Down Expand Up @@ -522,7 +521,7 @@ func TestConfig_LocalJWTRenewal(t *testing.T) {
}

// Write new value to the token file to simulate renewal.
err = ioutil.WriteFile(f.Name(), []byte(token2), 0o644)
err = os.WriteFile(f.Name(), []byte(token2), 0o644)
if err != nil {
t.Error(err)
}
Expand Down
Loading