From da4330e14351c6a7b234f845b18f17769c97223e Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Fri, 22 Mar 2024 11:57:05 -0400 Subject: [PATCH] Add endpointaddr.ParseFromURL helper, WebhookAuthenticator handle additional IPv6 cases --- .../webhookcachefiller/webhookcachefiller.go | 43 +-- .../webhookcachefiller_test.go | 278 +++++++++++++++--- internal/endpointaddr/endpointaddr.go | 40 ++- internal/endpointaddr/endpointaddr_test.go | 171 ++++++++++- 4 files changed, 466 insertions(+), 66 deletions(-) diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go index b96c0c754c..7991284871 100644 --- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go +++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go @@ -113,9 +113,9 @@ func (c *webhookCacheFillerController) Sync(ctx controllerlib.Context) error { var errs []error certPool, pemBytes, conditions, tlsBundleOk := c.validateTLSBundle(specCopy.TLS, conditions) - endpointURL, conditions, endpointOk := c.validateEndpoint(specCopy.Endpoint, conditions) + endpointHostPort, conditions, endpointOk := c.validateEndpoint(specCopy.Endpoint, conditions) okSoFar := tlsBundleOk && endpointOk - conditions, tlsNegotiateErr := c.validateTLSNegotiation(certPool, endpointURL, conditions, okSoFar) + conditions, tlsNegotiateErr := c.validateConnectionProbe(certPool, endpointHostPort, conditions, okSoFar) errs = append(errs, tlsNegotiateErr) okSoFar = okSoFar && tlsNegotiateErr == nil @@ -271,7 +271,7 @@ func newWebhookAuthenticator( return webhookA, conditions, nil } -func (c *webhookCacheFillerController) validateTLSNegotiation(certPool *x509.CertPool, endpointURL *url.URL, conditions []*metav1.Condition, prereqOk bool) ([]*metav1.Condition, error) { +func (c *webhookCacheFillerController) validateConnectionProbe(certPool *x509.CertPool, endpointHostPort *endpointaddr.HostPort, conditions []*metav1.Condition, prereqOk bool) ([]*metav1.Condition, error) { if !prereqOk { conditions = append(conditions, &metav1.Condition{ Type: typeConnectionProbeValid, @@ -282,30 +282,22 @@ func (c *webhookCacheFillerController) validateTLSNegotiation(certPool *x509.Cer return conditions, nil } - // dial requires domain, IPv4 or IPv6 w/o protocol - endpointHostPort, err := endpointaddr.Parse(endpointURL.Host, 443) - if err != nil { - // we have already validated the endpoint with url.Parse(endpoint) in c.validateEndpoint() - // so there is no reason to have a parsing error here. - c.log.Error("error parsing endpoint", err) - } - - conn, dialErr := c.tlsDialerFunc("tcp", endpointHostPort.Endpoint(), &tls.Config{ + conn, err := c.tlsDialerFunc("tcp", endpointHostPort.Endpoint(), &tls.Config{ MinVersion: tls.VersionTLS12, // If certPool is nil then RootCAs will be set to nil and TLS will use the host's root CA set automatically. RootCAs: certPool, }) - if dialErr != nil { + if err != nil { errText := "cannot dial server" - msg := fmt.Sprintf("%s: %s", errText, dialErr.Error()) + msg := fmt.Sprintf("%s: %s", errText, err.Error()) conditions = append(conditions, &metav1.Condition{ Type: typeConnectionProbeValid, Status: metav1.ConditionFalse, Reason: reasonUnableToDialServer, Message: msg, }) - return conditions, fmt.Errorf("%s: %w", errText, dialErr) + return conditions, fmt.Errorf("%s: %w", errText, err) } // this error should never be significant @@ -348,10 +340,10 @@ func (c *webhookCacheFillerController) validateTLSBundle(tlsSpec *auth1alpha1.TL return rootCAs, pemBytes, conditions, true } -func (c *webhookCacheFillerController) validateEndpoint(endpoint string, conditions []*metav1.Condition) (*url.URL, []*metav1.Condition, bool) { +func (c *webhookCacheFillerController) validateEndpoint(endpoint string, conditions []*metav1.Condition) (*endpointaddr.HostPort, []*metav1.Condition, bool) { endpointURL, err := url.Parse(endpoint) if err != nil { - msg := fmt.Sprintf("%s: %s", "spec.endpoint URL is invalid", err.Error()) + msg := fmt.Sprintf("%s: %s", "spec.endpoint URL cannot be parsed", err.Error()) conditions = append(conditions, &metav1.Condition{ Type: typeEndpointURLValid, Status: metav1.ConditionFalse, @@ -360,9 +352,10 @@ func (c *webhookCacheFillerController) validateEndpoint(endpoint string, conditi }) return nil, conditions, false } + // handles empty string and other issues as well. if endpointURL.Scheme != "https" { - msg := fmt.Sprintf("spec.endpoint %s has invalid scheme, require 'https'", endpoint) + msg := fmt.Sprintf("spec.endpoint URL %s has invalid scheme, require 'https'", endpoint) conditions = append(conditions, &metav1.Condition{ Type: typeEndpointURLValid, Status: metav1.ConditionFalse, @@ -372,13 +365,25 @@ func (c *webhookCacheFillerController) validateEndpoint(endpoint string, conditi return nil, conditions, false } + endpointHostPort, err := endpointaddr.ParseFromURL(endpointURL, 443) + if err != nil { + msg := fmt.Sprintf("%s: %s", "spec.endpoint URL is not valid", err.Error()) + conditions = append(conditions, &metav1.Condition{ + Type: typeEndpointURLValid, + Status: metav1.ConditionFalse, + Reason: reasonInvalidEndpointURL, + Message: msg, + }) + return nil, conditions, false + } + conditions = append(conditions, &metav1.Condition{ Type: typeEndpointURLValid, Status: metav1.ConditionTrue, Reason: reasonSuccess, Message: "endpoint is a valid URL", }) - return endpointURL, conditions, true + return &endpointHostPort, conditions, true } func (c *webhookCacheFillerController) updateStatus( diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go index c92860390b..657472bdb0 100644 --- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go +++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go @@ -149,9 +149,6 @@ func TestController(t *testing.T) { localhostURL, err := url.Parse(hostAsLocalhostWebhookServer.URL) require.NoError(t, err) - hostAs127001WebhookServerURL, err := url.Parse(hostAs127001WebhookServer.URL) - require.NoError(t, err) - badEndpointInvalidURL := "https://.café .com/café/café/café/coffee" badEndpointNoHTTPS := "http://localhost" @@ -307,6 +304,16 @@ func TestController(t *testing.T) { Message: "cannot dial server: tls: failed to verify certificate: x509: cannot validate certificate for 127.0.0.1 because it doesn't contain any IP SANs", } } + sadConnectionProbeValidWithMessage := func(time metav1.Time, observedGeneration int64, msg string) metav1.Condition { + return metav1.Condition{ + Type: "ConnectionProbeValid", + Status: "False", + ObservedGeneration: observedGeneration, + LastTransitionTime: time, + Reason: "UnableToDialServer", + Message: msg, + } + } happyEndpointURLValid := func(time metav1.Time, observedGeneration int64) metav1.Condition { return metav1.Condition{ @@ -325,17 +332,28 @@ func TestController(t *testing.T) { ObservedGeneration: observedGeneration, LastTransitionTime: time, Reason: "InvalidEndpointURL", - Message: fmt.Sprintf(`spec.endpoint URL is invalid: parse "%s": invalid character " " in host name`, issuer), + Message: fmt.Sprintf(`spec.endpoint URL cannot be parsed: parse "%s": invalid character " " in host name`, issuer), } } - sadEndpointURLValidHTTPS := func(issuer string, time metav1.Time, observedGeneration int64) metav1.Condition { + sadEndpointURLValidHTTPS := func(endpoint string, time metav1.Time, observedGeneration int64) metav1.Condition { return metav1.Condition{ Type: "EndpointURLValid", Status: "False", ObservedGeneration: observedGeneration, LastTransitionTime: time, Reason: "InvalidEndpointURLScheme", - Message: fmt.Sprintf(`spec.endpoint %s has invalid scheme, require 'https'`, issuer), + Message: fmt.Sprintf(`spec.endpoint URL %s has invalid scheme, require 'https'`, endpoint), + } + } + + sadEndpointURLValidWithMessage := func(time metav1.Time, observedGeneration int64, msg string) metav1.Condition { + return metav1.Condition{ + Type: "EndpointURLValid", + Status: "False", + ObservedGeneration: observedGeneration, + LastTransitionTime: time, + Reason: "InvalidEndpointURL", + Message: msg, } } @@ -425,7 +443,8 @@ func TestController(t *testing.T) { } }, wantCacheEntries: 1, - }, { + }, + { name: "Sync: changed WebhookAuthenticator: loop will update timestamps only on relevant statuses", syncKey: controllerlib.Key{Name: "test-name"}, webhooks: []runtime.Object{ @@ -482,7 +501,8 @@ func TestController(t *testing.T) { } }, wantCacheEntries: 1, - }, { + }, + { name: "Sync: valid WebhookAuthenticator with CA: will complete sync loop successfully with success conditions and ready phase", syncKey: controllerlib.Key{Name: "test-name"}, webhooks: []runtime.Object{ @@ -647,7 +667,8 @@ func TestController(t *testing.T) { } }, wantCacheEntries: 0, - }, { + }, + { name: "validateEndpoint: parsing error (spec.endpoint URL has invalid scheme, requires https) will fail sync loop, will write failed and unknown status conditions, but will not enqueue a resync due to user config error", syncKey: controllerlib.Key{Name: "test-name"}, webhooks: []runtime.Object{ @@ -692,7 +713,59 @@ func TestController(t *testing.T) { wantCacheEntries: 0, }, { - name: "validateTLSNegotiation: CA does not validate serving certificate for host, the dialer will error, will fail sync loop, will write failed and unknown status conditions, but will not enqueue a resync due to user config error", + name: "validateEndpoint: should error if endpoint cannot be parsed", + syncKey: controllerlib.Key{Name: "test-name"}, + tlsDialerFunc: func(network string, addr string, config *tls.Config) (*tls.Conn, error) { + return nil, errors.New("IPv6 test fake error") + }, + webhooks: []runtime.Object{ + &auth1alpha1.WebhookAuthenticator{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-name", + }, + Spec: auth1alpha1.WebhookAuthenticatorSpec{ + Endpoint: "https://[0:0:0:0:0:0:0:1]:69999/some/fake/path", + TLS: &auth1alpha1.TLSSpec{ + CertificateAuthorityData: base64.StdEncoding.EncodeToString(caForLocalhostAs127001.Bundle()), + }, + }, + }, + }, + wantActions: func() []coretesting.Action { + updateStatusAction := coretesting.NewUpdateAction(webhookAuthenticatorGVR, "", &auth1alpha1.WebhookAuthenticator{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-name", + }, + Spec: auth1alpha1.WebhookAuthenticatorSpec{ + Endpoint: "https://[0:0:0:0:0:0:0:1]:69999/some/fake/path", + TLS: &auth1alpha1.TLSSpec{ + CertificateAuthorityData: base64.StdEncoding.EncodeToString(caForLocalhostAs127001.Bundle()), + }, + }, + Status: auth1alpha1.WebhookAuthenticatorStatus{ + Conditions: conditionstestutil.Replace( + allHappyConditionsSuccess("https://[0:0:0:0:0:0:0:1]:69999/some/fake/path", frozenMetav1Now, 0), + []metav1.Condition{ + sadEndpointURLValidWithMessage(frozenMetav1Now, 0, `spec.endpoint URL is not valid: invalid port "69999"`), + sadReadyCondition(frozenMetav1Now, 0), + unknownConnectionProbeValid(frozenMetav1Now, 0), + unknownAuthenticatorValid(frozenMetav1Now, 0), + }, + ), + Phase: "Error", + }, + }) + updateStatusAction.Subresource = "status" + return []coretesting.Action{ + coretesting.NewListAction(webhookAuthenticatorGVR, webhookAuthenticatorGVK, "", metav1.ListOptions{}), + coretesting.NewWatchAction(webhookAuthenticatorGVR, "", metav1.ListOptions{}), + updateStatusAction, + } + }, + wantCacheEntries: 0, + }, + { + name: "validateConnectionProbe: CA does not validate serving certificate for host, the dialer will error, will fail sync loop, will write failed and unknown status conditions, but will not enqueue a resync due to user config error", syncKey: controllerlib.Key{Name: "test-name"}, webhooks: []runtime.Object{ &auth1alpha1.WebhookAuthenticator{ @@ -732,9 +805,9 @@ func TestController(t *testing.T) { }, // No unit test for system roots. We don't test the JWTAuthenticator's use of system roots either. // We would have to find a way to mock out roots by adding a dummy cert in order to test this - // { name: "validateTLSNegotiation: TLS bundle not provided should use system roots to validate server cert signed by a well-known CA",}, + // { name: "validateConnectionProbe: TLS bundle not provided should use system roots to validate server cert signed by a well-known CA",}, { - name: "validateTLSNegotiation: 404 endpoint on a valid server will still validate server certificate, will complete sync loop successfully with success conditions and ready phase", + name: "validateConnectionProbe: 404 endpoint on a valid server will still validate server certificate, will complete sync loop successfully with success conditions and ready phase", syncKey: controllerlib.Key{Name: "test-name"}, webhooks: []runtime.Object{ &auth1alpha1.WebhookAuthenticator{ @@ -775,8 +848,9 @@ func TestController(t *testing.T) { } }, wantCacheEntries: 1, - }, { - name: "validateTLSNegotiation: localhost hostname instead of 127.0.0.1 should still dial correctly as dialer should handle hostnames as well as IPv4", + }, + { + name: "validateConnectionProbe: localhost hostname instead of 127.0.0.1 should still dial correctly as dialer should handle hostnames as well as IPv4", syncKey: controllerlib.Key{Name: "test-name"}, webhooks: []runtime.Object{ &auth1alpha1.WebhookAuthenticator{ @@ -815,16 +889,17 @@ func TestController(t *testing.T) { } }, wantCacheEntries: 1, - }, { - name: "validateTLSNegotiation: localhost IPv6 address instead of 127.0.0.1 should still dial correctly as dialer should handle addresses", + }, + { + name: "validateConnectionProbe: IPv6 address with port: should call dialer func with correct arguments", syncKey: controllerlib.Key{Name: "test-name"}, tlsDialerFunc: func(network string, addr string, config *tls.Config) (*tls.Conn, error) { - // We are dialing a different server here since CI doesn't have the linux IPv6 stack installed. - // This test proves that IPv6 addresses are parsed & handled correctly before we call tls.Dial in production code. - return tls.Dial(network, hostAs127001WebhookServerURL.Host, &tls.Config{ - MinVersion: tls.VersionTLS12, - RootCAs: caForLocalhostAs127001.Pool(), - }) + assert.Equal(t, "tcp", network) + assert.Equal(t, "[0:0:0:0:0:0:0:1]:4242", addr) + assert.True(t, caForLocalhostAs127001.Pool().Equal(config.RootCAs)) + assert.Equal(t, uint16(tls.VersionTLS12), config.MinVersion) + + return nil, errors.New("IPv6 test fake error to skip real dial in prod code, this is actually success") }, webhooks: []runtime.Object{ &auth1alpha1.WebhookAuthenticator{ @@ -832,22 +907,67 @@ func TestController(t *testing.T) { Name: "test-name", }, Spec: auth1alpha1.WebhookAuthenticatorSpec{ - Endpoint: fmt.Sprintf("%s:%s", "https://[0:0:0:0:0:0:0:1]", hostAs127001WebhookServerURL.Port()), + Endpoint: "https://[0:0:0:0:0:0:0:1]:4242/some/fake/path", TLS: &auth1alpha1.TLSSpec{ CertificateAuthorityData: base64.StdEncoding.EncodeToString(caForLocalhostAs127001.Bundle()), }, }, }, }, - wantLogs: []map[string]any{ - { - "level": "info", - "timestamp": "2099-08-08T13:57:36.123456Z", - "logger": "webhookcachefiller-controller", - "message": "added new webhook authenticator", - "endpoint": fmt.Sprintf("%s:%s", "https://[0:0:0:0:0:0:0:1]", hostAs127001WebhookServerURL.Port()), - "webhook": map[string]interface{}{ - "name": "test-name", + wantActions: func() []coretesting.Action { + updateStatusAction := coretesting.NewUpdateAction(webhookAuthenticatorGVR, "", &auth1alpha1.WebhookAuthenticator{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-name", + }, + Spec: auth1alpha1.WebhookAuthenticatorSpec{ + Endpoint: "https://[0:0:0:0:0:0:0:1]:4242/some/fake/path", + TLS: &auth1alpha1.TLSSpec{ + CertificateAuthorityData: base64.StdEncoding.EncodeToString(caForLocalhostAs127001.Bundle()), + }, + }, + Status: auth1alpha1.WebhookAuthenticatorStatus{ + Conditions: conditionstestutil.Replace( + allHappyConditionsSuccess("https://[0:0:0:0:0:0:0:1]:4242/some/fake/path", frozenMetav1Now, 0), + []metav1.Condition{ + sadConnectionProbeValidWithMessage(frozenMetav1Now, 0, "cannot dial server: IPv6 test fake error to skip real dial in prod code, this is actually success"), + sadReadyCondition(frozenMetav1Now, 0), + unknownAuthenticatorValid(frozenMetav1Now, 0), + }, + ), + Phase: "Error", + }, + }) + updateStatusAction.Subresource = "status" + return []coretesting.Action{ + coretesting.NewListAction(webhookAuthenticatorGVR, webhookAuthenticatorGVK, "", metav1.ListOptions{}), + coretesting.NewWatchAction(webhookAuthenticatorGVR, "", metav1.ListOptions{}), + updateStatusAction, + } + }, + wantSyncLoopErr: testutil.WantExactErrorString(`cannot dial server: IPv6 test fake error to skip real dial in prod code, this is actually success`), + wantCacheEntries: 0, + }, + { + name: "validateConnectionProbe: IPv6 address without port: should call dialer func with correct arguments", + syncKey: controllerlib.Key{Name: "test-name"}, + tlsDialerFunc: func(network string, addr string, config *tls.Config) (*tls.Conn, error) { + assert.Equal(t, "tcp", network) + assert.Equal(t, "[0:0:0:0:0:0:0:1]:443", addr, "should add default port when port not provided") + assert.True(t, caForLocalhostAs127001.Pool().Equal(config.RootCAs)) + assert.Equal(t, uint16(tls.VersionTLS12), config.MinVersion) + + return nil, errors.New("IPv6 test fake error to skip real dial in prod code, this is actually success") + }, + webhooks: []runtime.Object{ + &auth1alpha1.WebhookAuthenticator{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-name", + }, + Spec: auth1alpha1.WebhookAuthenticatorSpec{ + Endpoint: "https://[0:0:0:0:0:0:0:1]/some/fake/path", + TLS: &auth1alpha1.TLSSpec{ + CertificateAuthorityData: base64.StdEncoding.EncodeToString(caForLocalhostAs127001.Bundle()), + }, }, }, }, @@ -857,14 +977,21 @@ func TestController(t *testing.T) { Name: "test-name", }, Spec: auth1alpha1.WebhookAuthenticatorSpec{ - Endpoint: fmt.Sprintf("%s:%s", "https://[0:0:0:0:0:0:0:1]", hostAs127001WebhookServerURL.Port()), + Endpoint: "https://[0:0:0:0:0:0:0:1]/some/fake/path", TLS: &auth1alpha1.TLSSpec{ CertificateAuthorityData: base64.StdEncoding.EncodeToString(caForLocalhostAs127001.Bundle()), }, }, Status: auth1alpha1.WebhookAuthenticatorStatus{ - Conditions: allHappyConditionsSuccess(fmt.Sprintf("%s:%s", "https://[0:0:0:0:0:0:0:1]", hostAs127001WebhookServerURL.Port()), frozenMetav1Now, 0), - Phase: "Ready", + Conditions: conditionstestutil.Replace( + allHappyConditionsSuccess("https://[0:0:0:0:0:0:0:1]/some/fake/path", frozenMetav1Now, 0), + []metav1.Condition{ + sadConnectionProbeValidWithMessage(frozenMetav1Now, 0, "cannot dial server: IPv6 test fake error to skip real dial in prod code, this is actually success"), + sadReadyCondition(frozenMetav1Now, 0), + unknownAuthenticatorValid(frozenMetav1Now, 0), + }, + ), + Phase: "Error", }, }) updateStatusAction.Subresource = "status" @@ -874,9 +1001,11 @@ func TestController(t *testing.T) { updateStatusAction, } }, - wantCacheEntries: 1, - }, { - name: "validateTLSNegotiation: localhost as IP address 127.0.0.1 should still dial correctly as dialer should handle hostnames as well as IPv4 and IPv6 addresses", + wantSyncLoopErr: testutil.WantExactErrorString(`cannot dial server: IPv6 test fake error to skip real dial in prod code, this is actually success`), + wantCacheEntries: 0, + }, + { + name: "validateConnectionProbe: localhost as IP address 127.0.0.1 should still dial correctly as dialer should handle hostnames as well as IPv4 and IPv6 addresses", syncKey: controllerlib.Key{Name: "test-name"}, webhooks: []runtime.Object{ &auth1alpha1.WebhookAuthenticator{ @@ -914,8 +1043,9 @@ func TestController(t *testing.T) { } }, wantCacheEntries: 1, - }, { - name: "validateTLSNegotiation: CA for example.com, serving cert for example.com, but endpoint 127.0.0.1 will fail to validate certificate and will fail sync loop and will report failed and unknown conditions and Error phase, but will not enqueue a resync due to user config error", + }, + { + name: "validateConnectionProbe: CA for example.com, serving cert for example.com, but endpoint 127.0.0.1 will fail to validate certificate and will fail sync loop and will report failed and unknown conditions and Error phase, but will not enqueue a resync due to user config error", syncKey: controllerlib.Key{Name: "test-name"}, webhooks: []runtime.Object{ &auth1alpha1.WebhookAuthenticator{ @@ -956,7 +1086,65 @@ func TestController(t *testing.T) { }, wantCacheEntries: 0, wantSyncLoopErr: testutil.WantExactErrorString(`cannot dial server: tls: failed to verify certificate: x509: cannot validate certificate for 127.0.0.1 because it doesn't contain any IP SANs`), - }, { + }, + { + name: "validateConnectionProbe: IPv6 address without port or brackets: should succeed since IPv6 brackets are optional without port", + syncKey: controllerlib.Key{Name: "test-name"}, + tlsDialerFunc: func(network string, addr string, config *tls.Config) (*tls.Conn, error) { + assert.Equal(t, "tcp", network) + assert.Equal(t, "[0:0:0:0:0:0:0:1]:443", addr) + assert.True(t, caForLocalhostAs127001.Pool().Equal(config.RootCAs)) + assert.Equal(t, uint16(tls.VersionTLS12), config.MinVersion) + + return nil, errors.New("IPv6 test fake error to skip real dial in prod code, this is actually success") + }, + webhooks: []runtime.Object{ + &auth1alpha1.WebhookAuthenticator{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-name", + }, + Spec: auth1alpha1.WebhookAuthenticatorSpec{ + Endpoint: "https://0:0:0:0:0:0:0:1/some/fake/path", + TLS: &auth1alpha1.TLSSpec{ + CertificateAuthorityData: base64.StdEncoding.EncodeToString(caForLocalhostAs127001.Bundle()), + }, + }, + }, + }, + wantActions: func() []coretesting.Action { + updateStatusAction := coretesting.NewUpdateAction(webhookAuthenticatorGVR, "", &auth1alpha1.WebhookAuthenticator{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-name", + }, + Spec: auth1alpha1.WebhookAuthenticatorSpec{ + Endpoint: "https://0:0:0:0:0:0:0:1/some/fake/path", + TLS: &auth1alpha1.TLSSpec{ + CertificateAuthorityData: base64.StdEncoding.EncodeToString(caForLocalhostAs127001.Bundle()), + }, + }, + Status: auth1alpha1.WebhookAuthenticatorStatus{ + Conditions: conditionstestutil.Replace( + allHappyConditionsSuccess("https://0:0:0:0:0:0:0:1/some/fake/path", frozenMetav1Now, 0), + []metav1.Condition{ + sadConnectionProbeValidWithMessage(frozenMetav1Now, 0, "cannot dial server: IPv6 test fake error to skip real dial in prod code, this is actually success"), + sadReadyCondition(frozenMetav1Now, 0), + unknownAuthenticatorValid(frozenMetav1Now, 0), + }, + ), + Phase: "Error", + }, + }) + updateStatusAction.Subresource = "status" + return []coretesting.Action{ + coretesting.NewListAction(webhookAuthenticatorGVR, webhookAuthenticatorGVK, "", metav1.ListOptions{}), + coretesting.NewWatchAction(webhookAuthenticatorGVR, "", metav1.ListOptions{}), + updateStatusAction, + } + }, + wantSyncLoopErr: testutil.WantExactErrorString(`cannot dial server: IPv6 test fake error to skip real dial in prod code, this is actually success`), + wantCacheEntries: 0, + }, + { name: "updateStatus: called with matching original and updated conditions: will not make request to update conditions", syncKey: controllerlib.Key{Name: "test-name"}, webhooks: []runtime.Object{ @@ -990,7 +1178,8 @@ func TestController(t *testing.T) { } }, wantCacheEntries: 1, - }, { + }, + { name: "updateStatus: called with different original and updated conditions: will make request to update conditions", syncKey: controllerlib.Key{Name: "test-name"}, webhooks: []runtime.Object{ @@ -1041,7 +1230,8 @@ func TestController(t *testing.T) { } }, wantCacheEntries: 1, - }, { + }, + { name: "updateStatus: when update request fails: error will enqueue a resync", syncKey: controllerlib.Key{Name: "test-name"}, configClient: func(client *pinnipedfake.Clientset) { @@ -1149,7 +1339,7 @@ func TestController(t *testing.T) { require.NoError(t, err) } actualLogLines := stringutil.SplitByNewline(log.String()) - require.Equal(t, len(actualLogLines), len(tt.wantLogs), "log line count should be correct") + require.Equal(t, len(tt.wantLogs), len(actualLogLines), "log line count should be correct") for logLineNum, logLine := range actualLogLines { require.NotNil(t, tt.wantLogs[logLineNum], "expected log line should never be empty") diff --git a/internal/endpointaddr/endpointaddr.go b/internal/endpointaddr/endpointaddr.go index fd9277658c..5c77c80d89 100644 --- a/internal/endpointaddr/endpointaddr.go +++ b/internal/endpointaddr/endpointaddr.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package endpointaddr implements parsing and validation of "[:]" strings for Pinniped APIs. @@ -7,7 +7,9 @@ package endpointaddr import ( "fmt" "net" + "net/url" "strconv" + "strings" "k8s.io/apimachinery/pkg/util/validation" ) @@ -38,7 +40,7 @@ func (h *HostPort) Endpoint() string { // - ":" (IPv4 address with port) // - "[]:" (IPv6 address with port, brackets are required) // -// If the input does not not specify a port number, then defaultPort will be used. +// If the input does not specify a port number, then defaultPort will be used. func Parse(endpoint string, defaultPort uint16) (HostPort, error) { // Try parsing it both with and without an implicit port 443 at the end. host, port, err := net.SplitHostPort(endpoint) @@ -69,3 +71,37 @@ func Parse(endpoint string, defaultPort uint16) (HostPort, error) { return HostPort{Host: host, Port: uint16(integerPort)}, nil } + +// ParseFromURL wraps Parse but specifically takes a url.URL instead of an endpoint string. +// ParseFromURL differs from Parse in that a URL will contain a protocol, and IPv6 addresses +// may or may not be wrapped in brackets (but require them when a port is provided): +// +// - "https://" (DNS hostname) +// - "https://" (IPv4 address) +// - "https://" (IPv6 address) +// - "https://[]" (IPv6 address without port, brackets should be used but are not strictly required) +// - "https://:" (DNS hostname with port) +// - "https://:" (IPv4 address with port) +// - "https://[]:" (IPv6 address with port, brackets are required) +// +// If the input does not specify a port number, then defaultPort will be used. +// +// The rfc for literal IPv6 addresses in URLs indicates that brackets +// - must be used when a port is provided +// - should be used when a port is not provided, but does not indicate "must" +// +// Since url.Parse does not inspect the host, it will accept IPv6 hosts without +// brackets and without port, which may result in errors that are not immediately obvious. +// Therefore, this helper will normalize the bracketed use case. Note that this is +// because ParseFromURL returns a HostPort which has an Endpoint() method which will +// return a properly constructed URL with brackets when appropriate. +// +// See RFC: https://datatracker.ietf.org/doc/html/rfc2732#section-2 +func ParseFromURL(u *url.URL, defaultPort uint16) (HostPort, error) { + host := u.Host + if strings.HasPrefix(host, "[") && strings.HasSuffix(host, "]") { + host = strings.TrimPrefix(strings.TrimSuffix(host, "]"), "[") + } + fmt.Printf("url.Host: %s, parsed host: %s \n", u.Host, host) + return Parse(host, defaultPort) +} diff --git a/internal/endpointaddr/endpointaddr_test.go b/internal/endpointaddr/endpointaddr_test.go index 736df3121b..adf6bb0b72 100644 --- a/internal/endpointaddr/endpointaddr_test.go +++ b/internal/endpointaddr/endpointaddr_test.go @@ -1,12 +1,14 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package endpointaddr import ( + "net/url" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestParse(t *testing.T) { @@ -180,3 +182,170 @@ func TestParse(t *testing.T) { }) } } + +func TestParseFromURL(t *testing.T) { + t.Parallel() + for _, tt := range []struct { + name string + input string + defaultPort uint16 + expectErr string + expect HostPort + // HostPort.Endpoint() returns a properly constructed endpoint. The normalization provided by ParseFromURL() + // expects that the resulting HostPort.Endpoint() will be called to normalize several special cases, especially + // for IPv6. + expectEndpoint string + }{ + // First set of valid passthrough tests to Parse() + // Matches the above test table, minus any test that would not url.Parse(input) properly + { + name: "plain IPv4", + input: "http://127.0.0.1", + defaultPort: 443, + expect: HostPort{Host: "127.0.0.1", Port: 443}, + expectEndpoint: "127.0.0.1:443", + }, + { + name: "IPv4 with port", + input: "http://127.0.0.1:8443", + defaultPort: 443, + expect: HostPort{Host: "127.0.0.1", Port: 8443}, + expectEndpoint: "127.0.0.1:8443", + }, + { + name: "IPv4 in brackets with port", + input: "http://[127.0.0.1]:8443", + defaultPort: 443, + expect: HostPort{Host: "127.0.0.1", Port: 8443}, + expectEndpoint: "127.0.0.1:8443", + }, + { + name: "IPv4 as IPv6 in brackets with port", + input: "http://[::127.0.0.1]:8443", + defaultPort: 443, + expect: HostPort{Host: "::127.0.0.1", Port: 8443}, + expectEndpoint: "[::127.0.0.1]:8443", + }, + { + name: "IPv6 with port", + input: "http://[2001:db8::ffff]:8443", + defaultPort: 443, + expect: HostPort{Host: "2001:db8::ffff", Port: 8443}, + expectEndpoint: "[2001:db8::ffff]:8443", + }, + { + name: "plain hostname", + input: "http://host.example.com", + defaultPort: 443, + expect: HostPort{Host: "host.example.com", Port: 443}, + expectEndpoint: "host.example.com:443", + }, + { + name: "plain hostname with dash", + input: "http://host-dev.example.com", + defaultPort: 443, + expect: HostPort{Host: "host-dev.example.com", Port: 443}, + expectEndpoint: "host-dev.example.com:443", + }, + { + name: "hostname with port", + input: "http://host.example.com:8443", + defaultPort: 443, + expect: HostPort{Host: "host.example.com", Port: 8443}, + expectEndpoint: "host.example.com:8443", + }, + { + name: "hostname in brackets with port", + input: "http://[host.example.com]:8443", + defaultPort: 443, + expect: HostPort{Host: "host.example.com", Port: 8443}, + expectEndpoint: "host.example.com:8443", + }, + { + name: "hostname without dots", + input: "http://localhost", + defaultPort: 443, + expect: HostPort{Host: "localhost", Port: 443}, + expectEndpoint: "localhost:443", + }, + { + name: "hostname and port without dots", + input: "http://localhost:8443", + defaultPort: 443, + expect: HostPort{Host: "localhost", Port: 8443}, + expectEndpoint: "localhost:8443", + }, + { + name: "http://invalid empty string", + input: "", + defaultPort: 443, + expectErr: `host "" is not a valid hostname or IP address`, + }, + { + name: "invalid host with underscores", + input: "http://___.example.com:1234", + defaultPort: 443, + expectErr: `host "___.example.com" is not a valid hostname or IP address`, + }, + { + name: "invalid host with uppercase", + input: "http://HOST.EXAMPLE.COM", + defaultPort: 443, + expectErr: `host "HOST.EXAMPLE.COM" is not a valid hostname or IP address`, + }, + // new tests for new functionality + { + name: "IPv6 with brackets but without port will strip brackets to create HostPort{}, which will add brackets when HostPort.Endpoint() is called", + input: "http://[2001:db8::ffff]", + defaultPort: 443, + expect: HostPort{Host: "2001:db8::ffff", Port: 443}, + expectEndpoint: "[2001:db8::ffff]:443", + }, + { + name: "IPv6 without brackets and without port will create HostPort{}, which will add brackets when HostPort.Endpoint() is called", + input: "http://2001:db8::1234", + defaultPort: 443, + expect: HostPort{Host: "2001:db8::1234", Port: 443}, + expectEndpoint: "[2001:db8::1234]:443", + }, + { + name: "IPv6 without brackets and without port with path create HostPort{}, which will add brackets when HostPort.Endpoint() is called", + input: "https://0:0:0:0:0:0:0:1/some/fake/path", + defaultPort: 443, + expect: HostPort{Host: "0:0:0:0:0:0:0:1", Port: 443}, + expectEndpoint: "[0:0:0:0:0:0:0:1]:443", + }, + { + name: "IPv6 with mismatched leading bracket will err on bracket", + input: "https://[[::1]/some/fake/path", + defaultPort: 443, + expect: HostPort{Host: "[[::1]", Port: 443}, + expectEndpoint: "[[::1]:443", + expectErr: `address [[::1]:443: unexpected '[' in address`, + }, + { + name: "IPv6 with mismatched trailing brackets will err on port", + input: "https://[::1]]/some/fake/path", + defaultPort: 443, + expect: HostPort{Host: "[::1]]", Port: 443}, + expectEndpoint: "[::1]]:443", + expectErr: `address [::1]]:443: missing port in address`, + }, + } { + tt := tt + t.Run(tt.name, func(t *testing.T) { + urlToProcess, err := url.Parse(tt.input) + require.NoError(t, err, "ParseFromURL expects a valid url.URL, parse errors here are not valuable") + + got, err := ParseFromURL(urlToProcess, tt.defaultPort) + if tt.expectErr == "" { + assert.NoError(t, err) + assert.Equal(t, tt.expect, got) + assert.Equal(t, tt.expectEndpoint, got.Endpoint()) + } else { + assert.EqualError(t, err, tt.expectErr) + assert.Equal(t, HostPort{}, got) + } + }) + } +}