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 1 commit
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
3 changes: 3 additions & 0 deletions pkg/rotator/rotator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,9 @@ func injectCertToApiService(apiService *unstructured.Unstructured, certPem []byt
if !found {
return errors.New("`spec` field not found in APIService")
}
if err := unstructured.SetNestedField(apiService.Object, false, "spec", "insecureSkipTLSVerify"); err != nil {
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious to get some of the maintainers thoughts on whether this actually has to happen here, in cert-controller. If cert-controller is going to be used with APIService, presumably we are going to inject a caBundle so why not set this field outside of cert-controller to begin with ?

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 problem we have faced with is that when you set insecureSkipTLSVerify: true , you have to set explicitly insecureSkipTLSVerify: false to remove the value.
As insecureSkipTLSVerify: false is the default value, k8s api removes it automatically and CD tools like ArgoCD/Flux enter on permanent out-of-sync status.
kedacore/charts#550

In this scenario, we have some options:

  • Adding a step to remove the field manually (breaking the helm upgrade process)
  • Adding explicitly the value in helm chart, producing out-of-sync on CD tools
  • Updating insecureSkipTLSVerify before setting caBundle

Maybe I've missed something and there is any other option

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused here. You're saying that having insecureSkipTLSVerify: false in the Helm chart causes CI/CD pipelines to detect drift because the API server strips default values, correct?

I'm curious what makes cert-controller different in this regard, since it's merely another thing essentially running kubectl apply. Would cert-controller also be subject to the same default-stripping issue, leaving you right back where you began?

Or is the objective to remove the state from the Helm chart such that there is no longer any check for drift for this value?

Modifying controller code to meet the needs of CI/CD code does seem a bit backwards. I am also wary of potentially manually overriding user intent on what could be a very salient field (is there some reason why insecureSkipTLSVerify is set? Will un-setting it cause an outage? Can we be sure the answer will be identical for all projects that consume cert-controller?)

As for other options, maybe a post-install hook on the Helm chart to modify the value?

Here is an example of a post-install hook G8r uses:

https://github.com/open-policy-agent/gatekeeper/blob/master/charts/gatekeeper/templates/namespace-post-install.yaml

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'm a bit confused here. You're saying that having insecureSkipTLSVerify: false in the Helm chart causes CI/CD pipelines to detect drift because the API server strips default values, correct?

Yes, as it's the default value, it is removed automatically. If you deploy an apiservice with explicitly set insecureSkipTLSVerify: false and then you get it from the cluster, you will see that your set value insecureSkipTLSVerify: false has been chopped from the manifest. This triggers out-of-sync on CD systems like ArgoCD.
You can just try with this:

apiVersion: apiregistration.k8s.io/v1
kind: APIService
metadata:
  labels:
    app.kubernetes.io/name: v1beta1.external.metrics.k8s.io
    app.kubernetes.io/version: latest
    app.kubernetes.io/part-of: keda-operator
  name: v1beta1.custom.metrics.k8s.io
# nosemgrep: yaml.kubernetes.security.skip-tls-verify-service.skip-tls-verify-service
spec:
  service:
    name: keda-metrics-apiserver
    namespace: keda
  group: external.metrics.k8s.io
  version: v1beta1
  groupPriorityMinimum: 100
  versionPriority: 100
  insecureSkipTLSVerify: false

I'm curious what makes cert-controller different in this regard, since it's merely another thing essentially running kubectl apply. Would cert-controller also be subject to the same default-stripping issue, leaving you right back where you began?

Yes, it's removed exactly in the same way, but if it's cert-controller, we don't need to set it in the helm chart, so external tools aren't affected.

Or is the objective to remove the state from the Helm chart such that there is no longer any check for drift for this value?

The objective is to not have to explicitly set the value to default value in general. (not only from helm but from raw manifests)

Modifying controller code to meet the needs of CI/CD code does seem a bit backwards. I am also wary of potentially manually overriding user intent on what could be a very salient field (is there some reason why insecureSkipTLSVerify is set? Will un-setting it cause an outage? Can we be sure the answer will be identical for all projects that consume cert-controller?)

Actually, if cert-controller tries to patch the apiservice to add the caBundle but the apiservice has set insecureSkipTLSVerify: true, cert-controller will fail because insecureSkipTLSVerify: true and caBundle are mutually exclusive. If the value has been set to true and then cert-controller has been enabled, cert-controller won't work at all, printing errors all the time due to this mutual exclusivity.

As for other options, maybe a post-install hook on the Helm chart to modify the value?

We don't need this feature anymore (nor for helm nor for raw yamls) as we just needed it during some releases after the migration from random certs generated by the pods to real certs, stored in Kubernetes and managed by cert-controller/cert-manager. I mean, we don't need this feature anymore for KEDA xD

I've just pushed it instead of just closing the PR because this project is awesome and has helped us a lot and I assume that we aren't the only one who faces with this and the code was already done.

I can extend any other doubt that you have, said that, if you think that this doesn't make sense, we can close the PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, if cert-controller tries to patch the apiservice to add the caBundle but the apiservice has set insecureSkipTLSVerify: true, cert-controller will fail because insecureSkipTLSVerify: true and caBundle are mutually exclusive. If the value has been set to true and then cert-controller has been enabled, cert-controller won't work at all, printing errors all the time due to this mutual exclusivity.

This makes sense, but if a user has intentionally skipped TLS verification, will cert-controller overriding it break their use case? If so, consumers would be unable to use that version of cert-controller due to the mandatory override. It comes down to whether its better for cert-controller to return errors or for it to override user intent without context as to why it was set.

I've just pushed it instead of just closing the PR because this project is awesome and has helped us a lot and I assume that we aren't the only one who faces with this and the code was already done.

I very much appreciate the consideration and am glad the project has helped you!

I can extend any other doubt that you have, said that, if you think that this doesn't make sense, we can close the PR

I think if the override was optional, such that projects could opt-in, I would be less concerned. This would probably boil down to adding a new option to the rotator:

cert-controller/pkg/rotator/rotator.go

Lines 219 to 259 in b1a35eb

// CertRotator contains cert artifacts and a channel to close when the certs are ready.
type CertRotator struct {
reader SyncingReader
writer client.Writer
SecretKey types.NamespacedName
CertDir string
CAName string
CAOrganization string
DNSName string
ExtraDNSNames []string
IsReady chan struct{}
Webhooks []WebhookInfo
// FieldOwner is the optional fieldmanager of the webhook updated fields.
FieldOwner string
RestartOnSecretRefresh bool
ExtKeyUsages *[]x509.ExtKeyUsage
// RequireLeaderElection should be set to true if the CertRotator needs to
// be run in the leader election mode.
RequireLeaderElection bool
// CaCertDuration sets how long a CA cert will be valid for.
CaCertDuration time.Duration
// ServerCertDuration sets how long a server cert will be valid for.
ServerCertDuration time.Duration
// RotationCheckFrequency sets how often the rotation is executed
RotationCheckFrequency time.Duration
// LookaheadInterval sets how long before the certificate is renewed
LookaheadInterval time.Duration
// CertName and Keyname override certificate path
CertName string
KeyName string
certsMounted chan struct{}
certsNotMounted chan struct{}
wasCAInjected *atomic.Bool
caNotInjected chan struct{}
// testNoBackgroundRotation doesn't actually start the rotator in the background.
// This should only be used for testing.
testNoBackgroundRotation bool
}

Which defaults to off so that behavior would be backwards compatible for users on upgrade.

Definitely up to you if the extra work is worth your time. I apologize for the slow review cycle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kindly reminder @maxsmythe 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we probably want to test both cases, but I hear you about not wanting to propagate the expected assertion everywhere.

Maybe we can use functional options to make this a backwards-compatible change to testWebhook().

Functional options blog: https://commandcenter.blogspot.com/2014/01/self-referential-functions-and-design.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

f**k! I missed your comment 🤦
I'll review the PR this week 😄

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 added another split test for this specific feature. PTAL when you have some time @maxsmythe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kindly reminder @maxsmythe 😄

if err := unstructured.SetNestedField(apiService.Object, base64.StdEncoding.EncodeToString(certPem), "spec", "caBundle"); err != nil {
return err
}
Expand Down
18 changes: 18 additions & 0 deletions 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,24 @@ func TestReconcileWebhook(t *testing.T) {
},
},
},
{
"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
Loading