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

fix: Ensure that insecureSkipTLSVerify is false before setting caBundle on APIService #160

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

fix: Ensure that insecureSkipTLSVerify is false before setting caBundle on APIService #160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
711ab42
feat: Ensure that insecureSkipTLSVerify is false before setting caBun…
JorTurFer Nov 21, 2023
b6658e1
add a comment for the test case
JorTurFer Nov 30, 2023
ab23e8f
Merge branch 'master' into set-insecure
ritazh Jan 23, 2024
944182b
make replace optional
JorTurFer Feb 7, 2024
64b274a
add a test
JorTurFer Mar 31, 2024
b6466d0
Merge branch 'master' into set-insecure
JorTurFer Mar 31, 2024
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
20 changes: 15 additions & 5 deletions pkg/rotator/rotator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@ func AddRotator(mgr manager.Manager, cr *CertRotator) error {
needLeaderElection: cr.RequireLeaderElection,
refreshCertIfNeededDelegate: cr.refreshCertIfNeeded,
fieldOwner: cr.FieldOwner,
removeInsecureSkipTLSVerify: cr.RemoveInsecureSkipTLSVerify,
}
if err := addController(mgr, reconciler); err != nil {
return err
Expand Down Expand Up @@ -247,6 +248,9 @@ type CertRotator struct {
// CertName and Keyname override certificate path
CertName string
KeyName string
// RemoveInsecureSkipTLSVerify sets if InsecureSkipTLSVerify has to
// be removed from apiservices during the patch process
RemoveInsecureSkipTLSVerify bool

certsMounted chan struct{}
certsNotMounted chan struct{}
Expand Down Expand Up @@ -387,7 +391,7 @@ func (cr *CertRotator) refreshCerts(refreshCA bool, secret *corev1.Secret) error
return nil
}

func injectCert(updatedResource *unstructured.Unstructured, certPem []byte, webhookType WebhookType) error {
func injectCert(updatedResource *unstructured.Unstructured, certPem []byte, webhookType WebhookType, removeInsecureSkipTLSVerify bool) error {
switch webhookType {
case Validating:
return injectCertToWebhook(updatedResource, certPem)
Expand All @@ -396,7 +400,7 @@ func injectCert(updatedResource *unstructured.Unstructured, certPem []byte, webh
case CRDConversion:
return injectCertToConversionWebhook(updatedResource, certPem)
case APIService:
return injectCertToApiService(updatedResource, certPem)
return injectCertToApiService(updatedResource, certPem, removeInsecureSkipTLSVerify)
case ExternalDataProvider:
return injectCertToExternalDataProvider(updatedResource, certPem)
}
Expand Down Expand Up @@ -442,14 +446,19 @@ func injectCertToConversionWebhook(crd *unstructured.Unstructured, certPem []byt
return nil
}

func injectCertToApiService(apiService *unstructured.Unstructured, certPem []byte) error {
func injectCertToApiService(apiService *unstructured.Unstructured, certPem []byte, removeInsecureSkipTLSVerify bool) error {
_, found, err := unstructured.NestedMap(apiService.Object, "spec")
if err != nil {
return err
}
if !found {
return errors.New("`spec` field not found in APIService")
}
if removeInsecureSkipTLSVerify {
if err := unstructured.SetNestedField(apiService.Object, false, "spec", "insecureSkipTLSVerify"); err != nil {
return err
}
}
if err := unstructured.SetNestedField(apiService.Object, base64.StdEncoding.EncodeToString(certPem), "spec", "caBundle"); err != nil {
return err
}
Expand Down Expand Up @@ -733,6 +742,7 @@ type ReconcileWH struct {
ctx context.Context
secretKey types.NamespacedName
webhooks []WebhookInfo
removeInsecureSkipTLSVerify bool
wasCAInjected *atomic.Bool
needLeaderElection bool
refreshCertIfNeededDelegate func() (bool, error)
Expand Down Expand Up @@ -778,7 +788,7 @@ func (r *ReconcileWH) Reconcile(ctx context.Context, request reconcile.Request)
artifacts, err := buildArtifactsFromSecret(secret)
if err != nil {
crLog.Error(err, "secret is not well-formed, cannot update webhook configurations")
return reconcile.Result{}, nil
return reconcile.Result{}, err
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've found this error too. If the reconciliation has failed, we should reschedule it manually or just return the error for auto rescheduling it

}

// Ensure certs on webhooks
Expand Down Expand Up @@ -826,7 +836,7 @@ func (r *ReconcileWH) ensureCerts(certPem []byte) error {
}

log.Info("Ensuring CA cert", "name", webhook.Name, "gvk", gvk)
if err := injectCert(updatedResource, certPem, webhook.Type); err != nil {
if err := injectCert(updatedResource, certPem, webhook.Type, r.removeInsecureSkipTLSVerify); err != nil {
log.Error(err, "Unable to inject cert to webhook.")
anyError = err
continue
Expand Down
193 changes: 192 additions & 1 deletion pkg/rotator/rotator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,28 @@ func TestReconcileWebhook(t *testing.T) {
},
},
},
{
// As InsecureSkipTLSVerify: true isn't compatible with caBundle
// we cover that the process sets InsecureSkipTLSVerify: false.
// If there is any issue patching InsecureSkipTLSVerify the test fails
// due InsecureSkipTLSVerify & caBundle incompatibility
"apiservice-insecure-tls", APIService, nil, []string{"spec", "caBundle"}, &apiregistrationv1.APIService{
ObjectMeta: metav1.ObjectMeta{
Name: "v1alpha2.example.com",
},
Spec: apiregistrationv1.APIServiceSpec{
Group: "example.com",
GroupPriorityMinimum: 1,
Version: "v1alpha2",
VersionPriority: 1,
InsecureSkipTLSVerify: true,
JorTurFer marked this conversation as resolved.
Show resolved Hide resolved
Service: &apiregistrationv1.ServiceReference{
Namespace: "kube-system",
Name: "example-api",
},
},
},
},
{
"externaldataprovider", ExternalDataProvider, nil, []string{"spec", "caBundle"}, &externaldatav1beta1.Provider{
TypeMeta: metav1.TypeMeta{
Expand Down Expand Up @@ -379,7 +401,8 @@ func TestReconcileWebhook(t *testing.T) {
Type: tt.webhookType,
},
},
FieldOwner: fieldOwner,
FieldOwner: fieldOwner,
RemoveInsecureSkipTLSVerify: true,
}
wh, ok := tt.webhookConfig.DeepCopyObject().(client.Object)
if !ok {
Expand Down Expand Up @@ -564,6 +587,174 @@ func TestNamespacedCache(t *testing.T) {
wg.Wait()
}

// Verifies that the rotator doesn't remove InsecureSkipTLSVerify when it's not flagged
func TestRemoveInsecureSkipTLSVerify(t *testing.T) {
testCases := []struct {
name string
apiservice apiregistrationv1.APIService
removeInsecureSkipTLSVerify bool
expectedResult bool
}{
{
name: "remove-enabled-and-insecure-active",
apiservice: apiregistrationv1.APIService{
ObjectMeta: metav1.ObjectMeta{
Name: "v1alpha1.verify.com",
},
Spec: apiregistrationv1.APIServiceSpec{
Group: "verify.com",
GroupPriorityMinimum: 1,
Version: "v1alpha1",
VersionPriority: 1,
InsecureSkipTLSVerify: true,
Service: &apiregistrationv1.ServiceReference{
Namespace: "kube-system",
Name: "example-api",
},
},
},
removeInsecureSkipTLSVerify: true,
expectedResult: true,
},
{
name: "remove-disabled-and-insecure-active",
apiservice: apiregistrationv1.APIService{
ObjectMeta: metav1.ObjectMeta{
Name: "v1alpha2.verify.com",
},
Spec: apiregistrationv1.APIServiceSpec{
Group: "verify.com",
GroupPriorityMinimum: 1,
Version: "v1alpha2",
VersionPriority: 1,
InsecureSkipTLSVerify: true,
Service: &apiregistrationv1.ServiceReference{
Namespace: "kube-system",
Name: "example-api",
},
},
},
removeInsecureSkipTLSVerify: false,
expectedResult: false,
},
{
name: "remove-disabled-and-insecure-inactive",
apiservice: apiregistrationv1.APIService{
ObjectMeta: metav1.ObjectMeta{
Name: "v1alpha3.verify.com",
},
Spec: apiregistrationv1.APIServiceSpec{
Group: "verify.com",
GroupPriorityMinimum: 1,
Version: "v1alpha3",
VersionPriority: 1,
InsecureSkipTLSVerify: false,
Service: &apiregistrationv1.ServiceReference{
Namespace: "kube-system",
Name: "example-api",
},
},
},
removeInsecureSkipTLSVerify: false,
expectedResult: true,
},
{
name: "remove-enabled-and-insecure-inactive",
apiservice: apiregistrationv1.APIService{
ObjectMeta: metav1.ObjectMeta{
Name: "v1alpha4.verify.com",
},
Spec: apiregistrationv1.APIServiceSpec{
Group: "verify.com",
GroupPriorityMinimum: 1,
Version: "v1alpha4",
VersionPriority: 1,
InsecureSkipTLSVerify: false,
Service: &apiregistrationv1.ServiceReference{
Namespace: "kube-system",
Name: "example-api",
},
},
},
removeInsecureSkipTLSVerify: true,
expectedResult: true,
},
}

for _, tt := range testCases {
ctx, cancelFunc := context.WithCancel(context.Background())
defer cancelFunc()
g := gomega.NewWithT(t)
mgr := setupManager(g)
c := mgr.GetClient()
key := types.NamespacedName{Namespace: tt.name, Name: "cert"}
createSecret(ctx, g, c, key)

rotator := &CertRotator{
SecretKey: key,
RemoveInsecureSkipTLSVerify: tt.removeInsecureSkipTLSVerify,
FieldOwner: "test",
Webhooks: []WebhookInfo{
{
Name: tt.apiservice.Name,
Type: APIService,
},
},
}
err := AddRotator(mgr, rotator)
g.Expect(err).NotTo(gomega.HaveOccurred(), "adding rotator")

wg := StartTestManager(ctx, mgr, g)

// The reader (cache) will be initialized in AddRotator.
g.Expect(rotator.reader).ToNot(gomega.BeNil())

// Wait for it to populate
if !rotator.reader.WaitForCacheSync(ctx) {
t.Fatal("waiting for cache to populate")
}

// Wait for certificates to generated
ensureCertWasGenerated(ctx, g, c, key)

wh := &tt.apiservice
err = c.Create(ctx, wh)
g.Expect(err).NotTo(gomega.HaveOccurred(), "creating apiservice")

// Sleep 1 seconds to ensure the operation
time.Sleep(1 * time.Second)

whu := &unstructured.Unstructured{}
err = c.Scheme().Convert(wh, whu, nil)
g.Expect(err).NotTo(gomega.HaveOccurred(), "cannot convert to webhook to unstructured")

key = client.ObjectKeyFromObject(whu)

if err := c.Get(ctx, key, whu); err != nil {
t.Fatal("recovering wh")
}

webhooks := extractWebhooks(g, whu, nil)
for _, w := range webhooks {
_, found, err := unstructured.NestedFieldNoCopy(w.(map[string]interface{}), []string{"spec", "caBundle"}...)
if err != nil {
t.Fatal("error checking the caBundle status")
}

if found && !tt.expectedResult {
t.Fatal("caBundle has been found but not found is the expected result")
}
if !found && tt.expectedResult {
t.Fatal("caBundle hasn¡t been found but found is the expected result")
}
}

cancelFunc()
wg.Wait()
}

}

func ensureCertWasGenerated(ctx context.Context, g *gomega.WithT, c client.Reader, key types.NamespacedName) {
var secret corev1.Secret
g.Eventually(func() bool {
Expand Down
Loading