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

Jump to bottom

feat: Add ca to check the validation of the member clusters' server certificate #4183

Merged
merged 1 commit into from
Oct 26, 2023
Merged

feat: Add ca to check the validation of the member clusters' server certificate #4183

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
15 changes: 9 additions & 6 deletions pkg/registry/cluster/storage/proxy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,9 @@ func TestProxyREST_Connect(t *testing.T) {
return &clusterapis.Cluster{
ObjectMeta: metav1.ObjectMeta{Name: name},
Spec: clusterapis.ClusterSpec{
APIEndpoint: s.URL,
ImpersonatorSecretRef: &clusterapis.LocalSecretReference{Namespace: "ns", Name: "secret"},
APIEndpoint: s.URL,
ImpersonatorSecretRef: &clusterapis.LocalSecretReference{Namespace: "ns", Name: "secret"},
InsecureSkipTLSVerification: true,
},
}, nil
},
Expand Down Expand Up @@ -109,8 +110,9 @@ func TestProxyREST_Connect(t *testing.T) {
return &clusterapis.Cluster{
ObjectMeta: metav1.ObjectMeta{Name: name},
Spec: clusterapis.ClusterSpec{
APIEndpoint: s.URL,
ImpersonatorSecretRef: &clusterapis.LocalSecretReference{Namespace: "ns", Name: "secret"},
APIEndpoint: s.URL,
ImpersonatorSecretRef: &clusterapis.LocalSecretReference{Namespace: "ns", Name: "secret"},
InsecureSkipTLSVerification: true,
},
}, nil
},
Expand All @@ -134,8 +136,9 @@ func TestProxyREST_Connect(t *testing.T) {
return &clusterapis.Cluster{
ObjectMeta: metav1.ObjectMeta{Name: name},
Spec: clusterapis.ClusterSpec{
APIEndpoint: s.URL,
ImpersonatorSecretRef: &clusterapis.LocalSecretReference{Namespace: "ns", Name: "secret"},
APIEndpoint: s.URL,
ImpersonatorSecretRef: &clusterapis.LocalSecretReference{Namespace: "ns", Name: "secret"},
InsecureSkipTLSVerification: true,
},
}, nil
},
Expand Down
14 changes: 12 additions & 2 deletions pkg/registry/cluster/storage/storage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"net/http"
"net/url"

corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apiserver/pkg/registry/generic"
Expand Down Expand Up @@ -58,7 +59,7 @@ func NewStorage(scheme *runtime.Scheme, kubeClient kubernetes.Interface, secretL
statusStore.UpdateStrategy = statusStrategy
statusStore.ResetFieldsStrategy = statusStrategy

clusterRest := &REST{store}
clusterRest := &REST{secretLister, store}
return &ClusterStorage{
Cluster: clusterRest,
Status: &StatusREST{&statusStore},
Expand All @@ -72,6 +73,7 @@ func NewStorage(scheme *runtime.Scheme, kubeClient kubernetes.Interface, secretL

// REST implements a RESTStorage for Cluster.
type REST struct {
secretLister listcorev1.SecretLister
*genericregistry.Store
}

Expand All @@ -85,7 +87,15 @@ func (r *REST) ResourceLocation(ctx context.Context, name string) (*url.URL, htt
return nil, nil, err
}

return proxy.Location(cluster)
secretGetter := func(ctx context.Context, namespace, name string) (*corev1.Secret, error) {
return r.secretLister.Secrets(namespace).Get(name)
}
tlsConfig, err := proxy.GetTlsConfigForCluster(ctx, cluster, secretGetter)
if err != nil {
return nil, nil, err
}

return proxy.Location(cluster, tlsConfig)
}

func (r *REST) getCluster(ctx context.Context, name string) (*clusterapis.Cluster, error) {
Expand Down
73 changes: 60 additions & 13 deletions pkg/util/proxy/proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package proxy
import (
"context"
"crypto/tls"
"crypto/x509"
"errors"
"fmt"
"net/http"
Expand All @@ -25,9 +26,16 @@ import (
clusterapis "github.com/karmada-io/karmada/pkg/apis/cluster"
)

type SecretGetterFunc func(context.Context, string, string) (*corev1.Secret, error)

// ConnectCluster returns a handler for proxy cluster.
func ConnectCluster(ctx context.Context, cluster *clusterapis.Cluster, proxyPath string, secretGetter func(context.Context, string, string) (*corev1.Secret, error), responder registryrest.Responder) (http.Handler, error) {
location, proxyTransport, err := Location(cluster)
func ConnectCluster(ctx context.Context, cluster *clusterapis.Cluster, proxyPath string, secretGetter SecretGetterFunc, responder registryrest.Responder) (http.Handler, error) {
tlsConfig, err := GetTlsConfigForCluster(ctx, cluster, secretGetter)
if err != nil {
return nil, err
}

location, proxyTransport, err := Location(cluster, tlsConfig)
if err != nil {
return nil, err
}
Expand All @@ -37,20 +45,20 @@ func ConnectCluster(ctx context.Context, cluster *clusterapis.Cluster, proxyPath
return nil, fmt.Errorf("the impersonatorSecretRef of cluster %s is nil", cluster.Name)
}

secret, err := secretGetter(ctx, cluster.Spec.ImpersonatorSecretRef.Namespace, cluster.Spec.ImpersonatorSecretRef.Name)
impersonateTokenSecret, err := secretGetter(ctx, cluster.Spec.ImpersonatorSecretRef.Namespace, cluster.Spec.ImpersonatorSecretRef.Name)
if err != nil {
return nil, err
}

impersonateToken, err := getImpersonateToken(cluster.Name, secret)
impersonateToken, err := getImpersonateToken(cluster.Name, impersonateTokenSecret)
if err != nil {
return nil, fmt.Errorf("failed to get impresonateToken for cluster %s: %v", cluster.Name, err)
}

return newProxyHandler(location, proxyTransport, cluster, impersonateToken, responder)
return newProxyHandler(location, proxyTransport, cluster, impersonateToken, responder, tlsConfig)
}

func newProxyHandler(location *url.URL, proxyTransport http.RoundTripper, cluster *clusterapis.Cluster, impersonateToken string, responder registryrest.Responder) (http.Handler, error) {
func newProxyHandler(location *url.URL, proxyTransport http.RoundTripper, cluster *clusterapis.Cluster, impersonateToken string,
responder registryrest.Responder, tlsConfig *tls.Config) (http.Handler, error) {
return http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
requester, exist := request.UserFrom(req.Context())
if !exist {
Expand All @@ -76,7 +84,7 @@ func newProxyHandler(location *url.URL, proxyTransport http.RoundTripper, cluste
location.RawQuery = req.URL.RawQuery

upgradeDialer := NewUpgradeDialerWithConfig(UpgradeDialerWithConfig{
TLS: &tls.Config{InsecureSkipVerify: true}, //nolint:gosec
TLS: tlsConfig,
Proxier: http.ProxyURL(proxyURL),
PingPeriod: time.Second * 5,
Header: ParseProxyHeaders(cluster.Spec.ProxyHeader),
Expand All @@ -94,14 +102,46 @@ func NewThrottledUpgradeAwareProxyHandler(location *url.URL, transport http.Roun
return proxy.NewUpgradeAwareHandler(location, transport, wrapTransport, upgradeRequired, proxy.NewErrorResponder(responder))
}

func GetTlsConfigForCluster(ctx context.Context, cluster *clusterapis.Cluster, secretGetter SecretGetterFunc) (*tls.Config, error) {
// The secret is optional for a pull-mode cluster, if no secret just returns a config with root CA unset.
if cluster.Spec.SecretRef == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Can you remind me when the secret would not exist?

Copy link
Member Author

@jwcesign jwcesign Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

From my point of view, it couldn't be empty. But I saw it's an optional field in the struct comment. Or I delete this field with kubectl.

Copy link
Member

Choose a reason for hiding this comment

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

For Pull mode cluster, the karmada-agent may not set the secret, see the usage from karmada-agent:

      --report-secrets strings                                                                                                                                                                     
                The secrets that are allowed to be reported to the Karmada control plane during registering. Valid values are 'KubeCredentials', 'KubeImpersonator' and 'None'. e.g
                'KubeCredentials,KubeImpersonator' or 'None'. (default [KubeCredentials,KubeImpersonator])

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if cluster.Spec.SecretRef == nil {
// The secret is optional for a pull-mode cluster, if no secret just returns a config with root CA unset.
if cluster.Spec.SecretRef == nil {

Maybe we can have a comment here.

return &tls.Config{
MinVersion: tls.VersionTLS13,
// Ignore false positive warning: "TLS InsecureSkipVerify may be true. (gosec)"
// Whether to skip server certificate verification depends on the
// configuration(.spec.insecureSkipTLSVerification, defaults to false) in a Cluster object.
InsecureSkipVerify: cluster.Spec.InsecureSkipTLSVerification, //nolint:gosec
}, nil
}
caSecret, err := secretGetter(ctx, cluster.Spec.SecretRef.Namespace, cluster.Spec.SecretRef.Name)
if err != nil {
return nil, err
}
caBundle, err := getClusterCABundle(cluster.Name, caSecret)
if err != nil {
return nil, fmt.Errorf("failed to get CA bundle for cluster %s: %v", cluster.Name, err)
}

caCertPool := x509.NewCertPool()
caCertPool.AppendCertsFromPEM([]byte(caBundle))
return &tls.Config{
RootCAs: caCertPool,
MinVersion: tls.VersionTLS13,
// Ignore false positive warning: "TLS InsecureSkipVerify may be true. (gosec)"
// Whether to skip server certificate verification depends on the
// configuration(.spec.insecureSkipTLSVerification, defaults to false) in a Cluster object.
InsecureSkipVerify: cluster.Spec.InsecureSkipTLSVerification, //nolint:gosec
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove the nolint:gosec now?

Same as above.

Copy link
Member Author

@jwcesign jwcesign Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If delete it, there is a lint error: cluster.Spec.InsecureSkipTLSVerification may be true.

But it's allowed to be true(if users want to do it).

Copy link
Member

Choose a reason for hiding this comment

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

Before:

pkg/util/proxy/proxy.go:109:24: G402: TLS InsecureSkipVerify set true. (gosec)
			InsecureSkipVerify: true,

After:

pkg/util/proxy/proxy.go:109:24: G402: TLS InsecureSkipVerify may be true. (gosec)
			InsecureSkipVerify: cluster.Spec.InsecureSkipTLSVerification,

we might need a comment to explain it.

}, nil
}

// Location returns a URL to which one can send traffic for the specified cluster.
func Location(cluster *clusterapis.Cluster) (*url.URL, http.RoundTripper, error) {
func Location(cluster *clusterapis.Cluster, tlsConfig *tls.Config) (*url.URL, http.RoundTripper, error) {
location, err := constructLocation(cluster)
if err != nil {
return nil, nil, err
}

proxyTransport, err := createProxyTransport(cluster)
proxyTransport, err := createProxyTransport(cluster, tlsConfig)
if err != nil {
return nil, nil, err
}
Expand All @@ -122,12 +162,11 @@ func constructLocation(cluster *clusterapis.Cluster) (*url.URL, error) {
return uri, nil
}

func createProxyTransport(cluster *clusterapis.Cluster) (*http.Transport, error) {
func createProxyTransport(cluster *clusterapis.Cluster, tlsConfig *tls.Config) (*http.Transport, error) {
var proxyDialerFn utilnet.DialFunc
proxyTLSClientConfig := &tls.Config{InsecureSkipVerify: true} // #nosec
trans := utilnet.SetTransportDefaults(&http.Transport{
DialContext: proxyDialerFn,
TLSClientConfig: proxyTLSClientConfig,
TLSClientConfig: tlsConfig,
})

if proxyURL := cluster.Spec.ProxyURL; proxyURL != "" {
Expand Down Expand Up @@ -162,6 +201,14 @@ func getImpersonateToken(clusterName string, secret *corev1.Secret) (string, err
return string(token), nil
}

func getClusterCABundle(clusterName string, secret *corev1.Secret) (string, error) {
caBundle, found := secret.Data[clusterapis.SecretCADataKey]
if !found {
return "", fmt.Errorf("the CA bundle of cluster %s is empty", clusterName)
}
return string(caBundle), nil
}

func skipGroup(group string) bool {
switch group {
case user.AllAuthenticated, user.AllUnauthenticated:
Expand Down
5 changes: 3 additions & 2 deletions pkg/util/proxy/proxy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,9 @@ func TestConnectCluster(t *testing.T) {
cluster: &clusterapis.Cluster{
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "cluster"},
Spec: clusterapis.ClusterSpec{
APIEndpoint: s.URL,
ImpersonatorSecretRef: &clusterapis.LocalSecretReference{Namespace: "ns", Name: "secret"},
APIEndpoint: s.URL,
ImpersonatorSecretRef: &clusterapis.LocalSecretReference{Namespace: "ns", Name: "secret"},
InsecureSkipTLSVerification: true,
},
},
secretGetter: func(_ context.Context, ns string, name string) (*corev1.Secret, error) {
Expand Down
Loading