Skip to content

Commit

Permalink
Remove unnecessary context for authz (#6193)
Browse files Browse the repository at this point in the history
<!--
Before you open the request please review the following guidelines and
tips to help it be more easily integrated:

 - Describe the scope of your change - i.e. what the change does.
 - Describe any known limitations with your change.
- Please run any tests or examples that can exercise your modified code.

 Thank you for contributing!
 -->

### Description of the change

<!-- Describe the scope of your change - i.e. what the change does. -->
We no longer need to be passing the context down and around for
authorization, as headers are used now.

This PR isn't complete - I've got a bunch of TODOs that I need to
followup.

### Benefits

<!-- What benefits will be realized by the code change? -->

### Possible drawbacks

<!-- Describe any known limitations with your change -->

### Applicable issues

<!-- Enter any applicable Issues here (You can reference an issue using
#) -->

- ref #6013

### Additional information

<!-- If there's anything else that's important and relevant to your pull
request, mention that information here.-->

---------

Signed-off-by: Michael Nelson <minelson@vmware.com>
  • Loading branch information
absoludity committed Apr 17, 2023
1 parent ce6d4e0 commit 7b06c61
Show file tree
Hide file tree
Showing 29 changed files with 163 additions and 226 deletions.
26 changes: 4 additions & 22 deletions cmd/kubeapps-apis/core/plugins/v1alpha1/plugins.go
Expand Up @@ -23,7 +23,6 @@ import (
"github.com/vmware-tanzu/kubeapps/pkg/kube"
"google.golang.org/grpc"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/metadata"
"google.golang.org/grpc/status"
"k8s.io/client-go/rest"
"k8s.io/client-go/tools/clientcmd"
Expand Down Expand Up @@ -317,10 +316,10 @@ func createConfigGetter(serveOpts core.ServeOptions, clustersConfig kube.Cluster
func createConfigGetterWithParams(inClusterConfig *rest.Config, serveOpts core.ServeOptions, clustersConfig kube.ClustersConfig) (core.KubernetesConfigGetter, error) {
// return the closure function that takes the context, but preserving the required scope,
// 'inClusterConfig' and 'config'
return func(ctx context.Context, headers http.Header, cluster string) (*rest.Config, error) {
return func(headers http.Header, cluster string) (*rest.Config, error) {
log.V(4).Infof("+clientGetter.GetClient")
var err error
token, err := extractToken(ctx, headers)
token, err := extractToken(headers)
if err != nil {
return nil, status.Errorf(codes.Unauthenticated, "invalid authorization metadata: %v", err)
}
Expand Down Expand Up @@ -351,28 +350,11 @@ func createConfigGetterWithParams(inClusterConfig *rest.Config, serveOpts core.S
}

// extractToken returns the token passed through the gRPC request in the
// "authorization" metadata in the context (improbable-eng grpc) or headers
// (connect gRPC)
// It is equivalent to the "Authorization" usual HTTP 1 header
// "authorization" metadata, in the headers for connect gRPC.
// For instance: authorization="Bearer abc" will return "abc"
func extractToken(ctx context.Context, headers http.Header) (string, error) {
func extractToken(headers http.Header) (string, error) {
bearerToken := headers.Get("Authorization")

if bearerToken == "" {
// per https://github.com/vmware-tanzu/kubeapps/issues/3560
// extractToken() to raise an error if there is no metadata with the context.
// note, the caller will wrap this as a codes.Unauthenticated status
md, ok := metadata.FromIncomingContext(ctx)
if !ok {
return "", fmt.Errorf("missing authorization metadata")
}

// metadata is always lowercased
if len(md["authorization"]) > 0 {
bearerToken = md["authorization"][0]
}
}

if len(bearerToken) > 0 {
if strings.HasPrefix(bearerToken, "Bearer ") {
return strings.TrimPrefix(bearerToken, "Bearer "), nil
Expand Down
51 changes: 4 additions & 47 deletions cmd/kubeapps-apis/core/plugins/v1alpha1/plugins_test.go
Expand Up @@ -20,7 +20,6 @@ import (
plugins "github.com/vmware-tanzu/kubeapps/cmd/kubeapps-apis/gen/core/plugins/v1alpha1"
"github.com/vmware-tanzu/kubeapps/pkg/kube"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/metadata"
"google.golang.org/grpc/status"
"k8s.io/client-go/rest"
)
Expand Down Expand Up @@ -287,51 +286,27 @@ func createTestFS(t *testing.T, filenames []string) fstest.MapFS {
func TestExtractToken(t *testing.T) {
testCases := []struct {
name string
contextKey string
contextValue string
headers http.Header
expectedToken string
expectedErr error
}{
{
name: "it returns the expected token without error for a valid 'authorization' metadata value",
contextKey: "authorization",
contextValue: "Bearer abc",
expectedToken: "abc",
expectedErr: nil,
},
{
name: "it returns the expected token without error for a valid 'Authorization' header",
contextKey: "",
contextValue: "",
headers: http.Header{"Authorization": []string{"Bearer abc"}},
expectedToken: "abc",
expectedErr: nil,
},
{
name: "it returns no token with an error if the 'authorization' metadata value is invalid",
contextKey: "authorization",
contextValue: "Bla",
expectedToken: "",
expectedErr: fmt.Errorf("malformed authorization metadata"),
},
{
name: "it returns no token and expected error if the 'authorization' is empty",
contextKey: "",
contextValue: "",
expectedToken: "",
expectedErr: fmt.Errorf("missing authorization metadata"),
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
context := context.Background()
context = metadata.NewIncomingContext(context, metadata.New(map[string]string{
tc.contextKey: tc.contextValue,
}))

token, err := extractToken(context, tc.headers)
token, err := extractToken(tc.headers)

if tc.expectedErr != nil && err != nil {
if got, want := err.Error(), tc.expectedErr.Error(); !cmp.Equal(want, got) {
Expand Down Expand Up @@ -374,54 +349,36 @@ func TestCreateConfigGetterWithParams(t *testing.T) {
testCases := []struct {
name string
cluster string
contextKey string
contextValue string
headers http.Header
expectedAPIHost string
expectedErrMsg error
}{
{
name: "it creates the config for the default cluster when passing a valid value for the authorization metadata",
contextKey: "authorization",
contextValue: "Bearer abc",
headers: http.Header{"Authorization": []string{"Bearer abc"}},
expectedAPIHost: DefaultK8sAPI,
expectedErrMsg: nil,
},
{
name: "it doesn't create the config and throws a grpc error when passing an invalid authorization metadata",
contextKey: "authorization",
contextValue: "Bla",
headers: http.Header{"Authorization": []string{"Bla"}},
expectedErrMsg: status.Errorf(codes.Unauthenticated, "invalid authorization metadata: malformed authorization metadata"),
},
{
name: "it doesn't create the config and throws a grpc error for the default cluster when no authorization metadata is passed",
contextKey: "",
contextValue: "",
expectedAPIHost: DefaultK8sAPI,
expectedErrMsg: status.Errorf(codes.Unauthenticated, "invalid authorization metadata: missing authorization metadata"),
},
{
name: "it doesn't create the config and throws a grpc error for the other cluster",
contextKey: "",
contextValue: "",
cluster: OtherClusterName,
expectedAPIHost: OtherK8sAPI,
expectedErrMsg: status.Errorf(codes.Unauthenticated, "invalid authorization metadata: missing authorization metadata"),
},
{
name: "it creates the config for the default cluster when passing a valid value for the authorization headers",
headers: http.Header{"Authorization": []string{"Bearer token-value"}},
expectedAPIHost: DefaultK8sAPI,
expectedErrMsg: nil,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
ctx := metadata.NewIncomingContext(context.Background(), metadata.New(map[string]string{
tc.contextKey: tc.contextValue,
}))

serveOpts := core.ServeOptions{
ClustersConfigPath: "/config.yaml",
PinnipedProxyURL: "http://example.com",
Expand All @@ -431,7 +388,7 @@ func TestCreateConfigGetterWithParams(t *testing.T) {
t.Fatalf("in %s: fail creating the configGetter: %+v", tc.name, err)
}

restConfig, err := configGetter(ctx, tc.headers, tc.cluster)
restConfig, err := configGetter(tc.headers, tc.cluster)
if tc.expectedErrMsg != nil && err != nil {
if got, want := err.Error(), tc.expectedErrMsg.Error(); !cmp.Equal(want, got) {
t.Errorf("in %s: mismatch (-want +got):\n%s", tc.name, cmp.Diff(want, got))
Expand Down
4 changes: 2 additions & 2 deletions cmd/kubeapps-apis/core/serveopts.go
Expand Up @@ -37,5 +37,5 @@ type GatewayHandlerArgs struct {

// KubernetesConfigGetter is a function type used throughout the apis server so
// that call-sites don't need to know how to obtain an authenticated client, but
// rather can just pass the request context and the cluster to get one.
type KubernetesConfigGetter func(ctx context.Context, headers http.Header, cluster string) (*rest.Config, error)
// rather can just pass the headers and the cluster to get one.
type KubernetesConfigGetter func(headers http.Header, cluster string) (*rest.Config, error)
Expand Up @@ -29,7 +29,7 @@ import (
)

func (s *Server) getChartInCluster(ctx context.Context, headers http.Header, key types.NamespacedName) (*sourcev1.HelmChart, error) {
client, err := s.getClient(ctx, headers, key.Namespace)
client, err := s.getClient(headers, key.Namespace)
if err != nil {
return nil, err
}
Expand Down
10 changes: 5 additions & 5 deletions cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/release.go
Expand Up @@ -42,7 +42,7 @@ var (

// namespace maybe "", in which case releases from all namespaces are returned
func (s *Server) listReleasesInCluster(ctx context.Context, headers http.Header, namespace string) ([]helmv2.HelmRelease, error) {
client, err := s.getClient(ctx, headers, namespace)
client, err := s.getClient(headers, namespace)
if err != nil {
return nil, err
}
Expand All @@ -63,7 +63,7 @@ func (s *Server) listReleasesInCluster(ctx context.Context, headers http.Header,
}

func (s *Server) getReleaseInCluster(ctx context.Context, headers http.Header, key types.NamespacedName) (*helmv2.HelmRelease, error) {
client, err := s.getClient(ctx, headers, key.Namespace)
client, err := s.getClient(headers, key.Namespace)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -350,7 +350,7 @@ func (s *Server) newRelease(ctx context.Context, headers http.Header, packageRef
// per https://github.com/vmware-tanzu/kubeapps/pull/3640#issuecomment-949315105
// the helm release CR to also be created in the target namespace (where the helm
// release itself is currently created)
client, err := s.getClient(ctx, headers, targetName.Namespace)
client, err := s.getClient(headers, targetName.Namespace)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -460,7 +460,7 @@ func (s *Server) updateRelease(ctx context.Context, headers http.Header, package
// even other changes made by the user.
rel.Status = helmv2.HelmReleaseStatus{}

client, err := s.getClient(ctx, headers, packageRef.Context.Namespace)
client, err := s.getClient(headers, packageRef.Context.Namespace)
if err != nil {
return nil, err
}
Expand All @@ -482,7 +482,7 @@ func (s *Server) updateRelease(ctx context.Context, headers http.Header, package
}

func (s *Server) deleteRelease(ctx context.Context, headers http.Header, packageRef *corev1.InstalledPackageReference) error {
client, err := s.getClient(ctx, headers, packageRef.Context.Namespace)
client, err := s.getClient(headers, packageRef.Context.Namespace)
if err != nil {
return err
}
Expand Down
Expand Up @@ -686,7 +686,7 @@ func TestCreateInstalledPackage(t *testing.T) {
}

// check expected HelmReleass CRD has been created
if ctrlClient, err := s.clientGetter.ControllerRuntime(context.Background(), http.Header{}, s.kubeappsCluster); err != nil {
if ctrlClient, err := s.clientGetter.ControllerRuntime(http.Header{}, s.kubeappsCluster); err != nil {
t.Fatal(err)
} else {
key := types.NamespacedName{Namespace: tc.request.TargetContext.Namespace, Name: tc.request.Name}
Expand Down Expand Up @@ -902,7 +902,7 @@ func TestUpdateInstalledPackage(t *testing.T) {
}
ctx := context.Background()
var actualRel helmv2.HelmRelease
if ctrlClient, err := s.clientGetter.ControllerRuntime(ctx, http.Header{}, s.kubeappsCluster); err != nil {
if ctrlClient, err := s.clientGetter.ControllerRuntime(http.Header{}, s.kubeappsCluster); err != nil {
t.Fatal(err)
} else if err = ctrlClient.Get(ctx, key, &actualRel); err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -990,7 +990,7 @@ func TestDeleteInstalledPackage(t *testing.T) {
}
ctx := context.Background()
var actualRel helmv2.HelmRelease
if ctrlClient, err := s.clientGetter.ControllerRuntime(ctx, http.Header{}, s.kubeappsCluster); err != nil {
if ctrlClient, err := s.clientGetter.ControllerRuntime(http.Header{}, s.kubeappsCluster); err != nil {
t.Fatal(err)
} else if err = ctrlClient.Get(ctx, key, &actualRel); !errors.IsNotFound(err) {
t.Errorf("mismatch expected, NotFound, got %+v", err)
Expand Down
10 changes: 5 additions & 5 deletions cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/repo.go
Expand Up @@ -101,7 +101,7 @@ func (s *Server) getRepoInCluster(ctx context.Context, headers http.Header, key
// whether or not the caller hasAccessToNamespace(). We can just pass the caller
// context into Get() and if the caller isn't allowed, Get will raise an error, which is what we
// want
client, err := s.getClient(ctx, headers, key.Namespace)
client, err := s.getClient(headers, key.Namespace)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -285,7 +285,7 @@ func (s *Server) newRepo(ctx context.Context, request *connect.Request[corev1.Ad

if fluxRepo, err := newFluxHelmRepo(name, description, typ, url, interval, secret, passCredentials, provider); err != nil {
return nil, err
} else if client, err := s.getClient(ctx, request.Header(), name.Namespace); err != nil {
} else if client, err := s.getClient(request.Header(), name.Namespace); err != nil {
return nil, err
} else if err = client.Create(ctx, fluxRepo); err != nil {
return nil, statuserror.FromK8sError("create", "HelmRepository", name.String(), err)
Expand Down Expand Up @@ -378,7 +378,7 @@ func (s *Server) repoSummaries(ctx context.Context, headers http.Header, ns stri
// error should be raised, as opposed to returning an empty list with no error
var repoList sourcev1.HelmRepositoryList
var client ctrlclient.Client
if client, err = s.getClient(ctx, headers, ns); err != nil {
if client, err = s.getClient(headers, ns); err != nil {
return nil, err
} else if err = client.List(ctx, &repoList); err != nil {
return nil, statuserror.FromK8sError("list", "HelmRepository", "", err)
Expand Down Expand Up @@ -499,7 +499,7 @@ func (s *Server) updateRepo(ctx context.Context, repoRef *corev1.PackageReposito
// even other changes made by the user.
repo.Status = sourcev1.HelmRepositoryStatus{}

if client, err := s.getClient(ctx, request.Header(), key.Namespace); err != nil {
if client, err := s.getClient(request.Header(), key.Namespace); err != nil {
return nil, err
} else if err = client.Update(ctx, repo); err != nil {
return nil, statuserror.FromK8sError("update", "HelmRepository", key.String(), err)
Expand All @@ -525,7 +525,7 @@ func (s *Server) updateRepo(ctx context.Context, repoRef *corev1.PackageReposito
}

func (s *Server) deleteRepo(ctx context.Context, headers http.Header, repoRef *corev1.PackageRepositoryReference) error {
client, err := s.getClient(ctx, headers, repoRef.Context.Namespace)
client, err := s.getClient(headers, repoRef.Context.Namespace)
if err != nil {
return err
}
Expand Down
10 changes: 5 additions & 5 deletions cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/repo_auth.go
Expand Up @@ -63,7 +63,7 @@ func (s *Server) handleRepoSecretForCreate(
// in that order because to set an owner ref you need object (i.e. repo) UID, which you only get
// once the object's been created
// create a secret first, if applicable
if typedClient, err := s.clientGetter.Typed(ctx, headers, s.kubeappsCluster); err != nil {
if typedClient, err := s.clientGetter.Typed(headers, s.kubeappsCluster); err != nil {
return nil, false, err
} else if secret, err = typedClient.CoreV1().Secrets(repoName.Namespace).Create(ctx, secret, metav1.CreateOptions{}); err != nil {
return nil, false, statuserror.FromK8sError("create", "secret", secret.GetGenerateName(), err)
Expand Down Expand Up @@ -94,7 +94,7 @@ func (s *Server) handleRepoSecretForUpdate(
return nil, false, false, status.Errorf(codes.InvalidArgument, "Package repository cannot mix referenced secrets and user provided secret data")
}

typedClient, err := s.clientGetter.Typed(ctx, headers, s.kubeappsCluster)
typedClient, err := s.clientGetter.Typed(headers, s.kubeappsCluster)
if err != nil {
return nil, false, false, err
}
Expand Down Expand Up @@ -188,7 +188,7 @@ func (s *Server) validateUserManagedRepoSecret(
var secret *apiv1.Secret
if secretRef != "" {
// check that the specified secret exists
if typedClient, err := s.clientGetter.Typed(ctx, headers, s.kubeappsCluster); err != nil {
if typedClient, err := s.clientGetter.Typed(headers, s.kubeappsCluster); err != nil {
return nil, err
} else if secret, err = typedClient.CoreV1().Secrets(repoName.Namespace).Get(ctx, secretRef, metav1.GetOptions{}); err != nil {
return nil, statuserror.FromK8sError("get", "secret", secretRef, err)
Expand Down Expand Up @@ -255,7 +255,7 @@ func (s *Server) setOwnerReferencesForRepoSecret(
repo *sourcev1.HelmRepository) error {

if repo.Spec.SecretRef != nil && secret != nil {
if typedClient, err := s.clientGetter.Typed(ctx, headers, s.kubeappsCluster); err != nil {
if typedClient, err := s.clientGetter.Typed(headers, s.kubeappsCluster); err != nil {
return err
} else {
secretsInterface := typedClient.CoreV1().Secrets(repo.Namespace)
Expand Down Expand Up @@ -285,7 +285,7 @@ func (s *Server) getRepoTlsConfigAndAuth(ctx context.Context, headers http.Heade
if s == nil || s.clientGetter == nil {
return nil, nil, status.Errorf(codes.Internal, "unexpected state in clientGetterHolder instance")
}
typedClient, err := s.clientGetter.Typed(ctx, headers, s.kubeappsCluster)
typedClient, err := s.clientGetter.Typed(headers, s.kubeappsCluster)
if err != nil {
return nil, nil, err
}
Expand Down

0 comments on commit 7b06c61

Please sign in to comment.