You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/09/01 09:43:49 UTC

[GitHub] [solr-operator] thelabdude commented on a change in pull request #312: Configuration options to support an additional client TLS cert in addition to the server certificate

thelabdude commented on a change in pull request #312:
URL: https://github.com/apache/solr-operator/pull/312#discussion_r699689067



##########
File path: controllers/solrcloud_controller.go
##########
@@ -960,80 +950,160 @@ func (r *SolrCloudReconciler) indexAndWatchForTLSSecret(mgr ctrl.Manager, ctrlBu
 
 	return ctrlBuilder.Watches(
 		&source.Kind{Type: &corev1.Secret{}},
-		&handler.EnqueueRequestsFromMapFunc{
-			ToRequests: handler.ToRequestsFunc(func(a handler.MapObject) []reconcile.Request {
-				foundClouds := &solr.SolrCloudList{}
-				listOps := &client.ListOptions{
-					FieldSelector: fields.OneTermEqualSelector(".spec.solrTLS.pkcs12Secret", a.Meta.GetName()),
-					Namespace:     a.Meta.GetNamespace(),
-				}
-				err := r.List(context.TODO(), foundClouds, listOps)
-				if err != nil {
-					return []reconcile.Request{}
-				}
+		r.findSolrCloudByFieldValueFunc(field),
+		builder.WithPredicates(predicate.ResourceVersionChangedPredicate{})), nil
+}
 
-				requests := make([]reconcile.Request, len(foundClouds.Items))
-				for i, item := range foundClouds.Items {
-					requests[i] = reconcile.Request{
-						NamespacedName: types.NamespacedName{
-							Name:      item.GetName(),
-							Namespace: item.GetNamespace(),
-						},
-					}
-				}
-				return requests
-			}),
-		},
+func (r *SolrCloudReconciler) indexAndWatchForClientTLSSecret(mgr ctrl.Manager, ctrlBuilder *builder.Builder) (*builder.Builder, error) {
+	field := ".spec.solrClientTLS.pkcs12Secret"
+	if err := mgr.GetFieldIndexer().IndexField(context.TODO(), &solr.SolrCloud{}, field, func(rawObj runtime.Object) []string {
+		// grab the SolrCloud object, extract the used configMap...
+		solrCloud := rawObj.(*solr.SolrCloud)
+		if solrCloud.Spec.SolrClientTLS == nil || solrCloud.Spec.SolrClientTLS.PKCS12Secret == nil {
+			return nil
+		}
+		// ...and if so, return it
+		return []string{solrCloud.Spec.SolrClientTLS.PKCS12Secret.Name}
+	}); err != nil {
+		return ctrlBuilder, err
+	}
+
+	return ctrlBuilder.Watches(
+		&source.Kind{Type: &corev1.Secret{}},
+		r.findSolrCloudByFieldValueFunc(field),
 		builder.WithPredicates(predicate.ResourceVersionChangedPredicate{})), nil
 }
 
+func (r *SolrCloudReconciler) findSolrCloudByFieldValueFunc(field string) *handler.EnqueueRequestsFromMapFunc {
+	return &handler.EnqueueRequestsFromMapFunc{
+		ToRequests: handler.ToRequestsFunc(func(a handler.MapObject) []reconcile.Request {
+			foundClouds := &solr.SolrCloudList{}
+			listOps := &client.ListOptions{
+				FieldSelector: fields.OneTermEqualSelector(field, a.Meta.GetName()),
+				Namespace:     a.Meta.GetNamespace(),
+			}
+			err := r.List(context.TODO(), foundClouds, listOps)
+			if err != nil {
+				return []reconcile.Request{}
+			}
+
+			requests := make([]reconcile.Request, len(foundClouds.Items))
+			for i, item := range foundClouds.Items {
+				requests[i] = reconcile.Request{
+					NamespacedName: types.NamespacedName{
+						Name:      item.GetName(),
+						Namespace: item.GetNamespace(),
+					},
+				}
+			}
+			return requests
+		}),
+	}
+}
+
 // Ensure the TLS config is ready, such as verifying the TLS secret exists, to enable TLS on SolrCloud pods
-func (r *SolrCloudReconciler) reconcileTLSConfig(instance *solr.SolrCloud) (*util.TLSConfig, error) {
-	tls := &util.TLSConfig{}
-	tls.InitContainerImage = instance.Spec.BusyBoxImage
-	tls.Options = instance.Spec.SolrTLS
+func (r *SolrCloudReconciler) reconcileTLSConfig(instance *solr.SolrCloud) (*util.TLSCerts, error) {
+	tls := util.TLSCertsForSolrCloud(instance)
 
 	// Has the user configured a secret containing the TLS cert files that we need to mount into the Solr pods?
-	if instance.Spec.SolrTLS.PKCS12Secret != nil {
+	serverCert := tls.ServerConfig.Options
+	if serverCert.PKCS12Secret != nil {
 		// Ensure one or the other have been configured, but not both
-		if instance.Spec.SolrTLS.MountedServerTLSDir != nil {
-			return nil, fmt.Errorf("invalid TLS config, either supply `solrTLS.pkcs12Secret` or `solrTLS.mountedServerTLSDir` but not both")
+		if serverCert.MountedTLSDir != nil {
+			return nil, fmt.Errorf("invalid TLS config, either supply `solrTLS.pkcs12Secret` or `solrTLS.mountedTLSDir` but not both")
 		}
 
-		foundTLSSecret, err := r.verifyTLSSecretConfig(instance.Spec.SolrTLS.PKCS12Secret.Name, instance.Namespace, instance.Spec.SolrTLS.KeyStorePasswordSecret)
+		foundTLSSecret, err := r.verifyTLSSecretConfig(serverCert.PKCS12Secret.Name, instance.Namespace, serverCert.KeyStorePasswordSecret)
 		if err != nil {
 			return nil, err
 		} else {
-			// We have a watch on secrets, so will get notified when the secret changes (such as after cert renewal)
-			// capture the hash of the secret and stash in an annotation so that pods get restarted if the cert changes
-			if instance.Spec.SolrTLS.RestartOnTLSSecretUpdate {
-				if tlsCertBytes, ok := foundTLSSecret.Data[util.TLSCertKey]; ok {
-					tls.CertMd5 = fmt.Sprintf("%x", md5.Sum(tlsCertBytes))
-				} else {
-					return nil, fmt.Errorf("%s key not found in TLS secret %s, cannot watch for updates to"+
-						" the cert without this data but 'solrTLS.restartOnTLSSecretUpdate' is enabled",
-						util.TLSCertKey, foundTLSSecret.Name)
+			if serverCert.RestartOnTLSSecretUpdate {
+				tls.ServerConfig.CertMd5, err = r.computeCertMd5(foundTLSSecret)
+				if err != nil {
+					return nil, err
 				}
+				tls.ServerConfig.CertMd5Annotation = util.SolrTlsCertMd5Annotation
 			}
 
-			if _, ok := foundTLSSecret.Data[instance.Spec.SolrTLS.PKCS12Secret.Key]; !ok {
+			if _, ok := foundTLSSecret.Data[serverCert.PKCS12Secret.Key]; !ok {
 				// the keystore.p12 key is not in the TLS secret, indicating we need to create it using an initContainer
-				tls.NeedsPkcs12InitContainer = true
+				tls.ServerConfig.NeedsPkcs12InitContainer = true
 			}
 		}
 
-		if instance.Spec.SolrTLS.TrustStoreSecret != nil {
+		if serverCert.TrustStoreSecret != nil {
 			// verify the TrustStore secret is configured correctly
-			passwordSecret := instance.Spec.SolrTLS.TrustStorePasswordSecret
-			if passwordSecret == nil {
-				passwordSecret = instance.Spec.SolrTLS.KeyStorePasswordSecret
+			err = r.verifyTruststoreConfig(serverCert, instance.Namespace)
+			if err != nil {
+				return nil, err
+			}
+		} else {
+			// does the supplied keystore secret also contain the truststore?
+			if _, ok := foundTLSSecret.Data[util.DefaultPkcs12TruststoreFile]; ok {
+				// there's a truststore in the supplied TLS secret
+				serverCert.TrustStoreSecret = &corev1.SecretKeySelector{
+					LocalObjectReference: corev1.LocalObjectReference{Name: foundTLSSecret.Name},
+					Key:                  util.DefaultPkcs12TruststoreFile,
+				}
+				serverCert.TrustStorePasswordSecret = serverCert.KeyStorePasswordSecret
 			}
-			_, err := r.verifyTLSSecretConfig(instance.Spec.SolrTLS.TrustStoreSecret.Name, instance.Namespace, passwordSecret)
+		}
+
+		// is there a client TLS config too?
+		if tls.ClientConfig != nil {
+			if tls.ClientConfig.Options.PKCS12Secret == nil {
+				// cannot mix options with the client cert, if the server cert comes from a secret, so too must the client, not a mountedTLSDir
+				return nil, fmt.Errorf("invalid TLS config, the 'solrClientTLS.pkcs12Secret' option is required when using a secret for server cert")
+			}
+
+			clientOpts := tls.ClientConfig.Options
+			// shouldn't configure a client cert if it's the same as the server cert
+			if clientOpts.PKCS12Secret == tls.ServerConfig.Options.PKCS12Secret {
+				return nil, fmt.Errorf("invalid TLS config, the 'solrClientTLS.pkcs12Secret' option should not be the same as the 'solrTLS.pkcs12Secret'")

Review comment:
       Probably not an issue for Solr but seemed like that would be a mis-configuration / accident by the user since it wouldn't be sensible to use the same cert for the client & server (you don't need a separate client in that case) so I felt raising an error would help draw attention to a mistake.

##########
File path: controllers/solrprometheusexporter_controller.go
##########
@@ -434,53 +458,133 @@ func (r *SolrPrometheusExporterReconciler) buildSecretWatch(secretField string,
 		builder.WithPredicates(predicate.ResourceVersionChangedPredicate{})), nil
 }
 
-func (r *SolrPrometheusExporterReconciler) reconcileTLSConfig(prometheusExporter *solrv1beta1.SolrPrometheusExporter) (*util.TLSConfig, error) {
-	opts := prometheusExporter.Spec.SolrReference.SolrTLS
-
-	tls := &util.TLSConfig{}
-	tls.InitContainerImage = prometheusExporter.Spec.BusyBoxImage
-	tls.Options = opts
+// Reconcile the various options for configuring TLS for the exporter
+// The exporter is a client to Solr pods, so can either just have a truststore so it trusts Solr certs
+// Or it can have its own client auth cert when Solr mTLS is required
+func (r *SolrPrometheusExporterReconciler) reconcileTLSConfig(prometheusExporter *solrv1beta1.SolrPrometheusExporter) (*util.TLSCerts, error) {
+	tls := util.TLSCertsForExporter(prometheusExporter)
+	opts := tls.ClientConfig.Options
 
 	if opts.PKCS12Secret != nil {

Review comment:
       yeah, let me take another stab on consolidating the reconcile loop around this, definitely a lot of similarity that can be reused.

##########
File path: controllers/util/solr_tls_util.go
##########
@@ -27,171 +27,217 @@ import (
 )
 
 const (
-	SolrTlsCertMd5Annotation    = "solr.apache.org/tlsCertMd5"
-	DefaultKeyStorePath         = "/var/solr/tls"
-	DefaultWritableKeyStorePath = "/var/solr/tls/pkcs12"
-	TLSCertKey                  = "tls.crt"
-	DefaultTrustStorePath       = "/var/solr/tls-truststore"
-	InitdbPath                  = "/docker-entrypoint-initdb.d"
-	DefaultPkcs12KeystoreFile   = "keystore.p12"
-	DefaultPkcs12TruststoreFile = "truststore.p12"
-	DefaultKeystorePasswordFile = "keystore-password"
+	SolrTlsCertMd5Annotation       = "solr.apache.org/tlsCertMd5"
+	SolrClientTlsCertMd5Annotation = "solr.apache.org/tlsClientCertMd5"
+	DefaultKeyStorePath            = "/var/solr/tls"
+	DefaultClientKeyStorePath      = "/var/solr/client-tls"
+	DefaultWritableKeyStorePath    = "/var/solr/tls/pkcs12"
+	TLSCertKey                     = "tls.crt"
+	DefaultTrustStorePath          = "/var/solr/tls-truststore"
+	DefaultClientTrustStorePath    = "/var/solr/client-tls-truststore"
+	InitdbPath                     = "/docker-entrypoint-initdb.d"
+	DefaultPkcs12KeystoreFile      = "keystore.p12"
+	DefaultPkcs12TruststoreFile    = "truststore.p12"
+	DefaultKeystorePasswordFile    = "keystore-password"
 )
 
-// Holds Options from the user config as well as other config properties determined during reconciliation
+// Helper struct for holding server and/or client cert config
+// This struct is intended for internal use only and is only exposed outside the package so that the controllers can access
+type TLSCerts struct {
+	// Server cert config
+	ServerConfig *TLSConfig
+	// Client cert config
+	ClientConfig *TLSConfig
+	// Image used for initContainers that help configure the TLS settings
+	InitContainerImage *solr.ContainerImage
+}
+
+// Holds TLS options from the user config as well as other config properties determined during reconciliation
 // This struct is intended for internal use only and is only exposed outside the package so that the controllers can access
 type TLSConfig struct {
 	// TLS options provided by the user in the CRD definition
 	Options *solr.SolrTLSOptions
 	// Flag to indicate if we need to convert the provided keystore into the p12 format needed by Java
 	NeedsPkcs12InitContainer bool
-	// The MD5 hash of the TLS cert, used for restarting Solr pods after the cert updates if so desired
+	// The MD5 hash of the cert, used for restarting Solr pods after the cert updates if so desired
 	CertMd5 string
-	// Image used for initContainers that help configure the TLS settings
-	InitContainerImage *solr.ContainerImage
+	// The annotation varies based on the cert type (client or server)
+	CertMd5Annotation string
+	// The paths vary based on whether this config is for a client or server cert
+	KeystorePath   string
+	TruststorePath string
+}
+
+// Get a TLSCerts struct for reconciling TLS on a SolrCloud
+func TLSCertsForSolrCloud(instance *solr.SolrCloud) *TLSCerts {
+	tls := &TLSCerts{
+		ServerConfig: &TLSConfig{
+			Options:        instance.Spec.SolrTLS.DeepCopy(),
+			KeystorePath:   DefaultKeyStorePath,
+			TruststorePath: DefaultTrustStorePath,
+		},
+		InitContainerImage: instance.Spec.BusyBoxImage,
+	}
+	if instance.Spec.SolrClientTLS != nil {
+		tls.ClientConfig = &TLSConfig{
+			Options:        instance.Spec.SolrClientTLS.DeepCopy(),
+			KeystorePath:   DefaultClientKeyStorePath,
+			TruststorePath: DefaultClientTrustStorePath,
+		}
+	}
+	return tls
+}
+
+// Get a TLSCerts struct for reconciling TLS on an Exporter
+func TLSCertsForExporter(prometheusExporter *solr.SolrPrometheusExporter) *TLSCerts {
+	// when using mounted dir option, we need a busy box image for our initContainers
+	bbImage := prometheusExporter.Spec.BusyBoxImage
+	if bbImage == nil {
+		bbImage = &solr.ContainerImage{
+			Repository: solr.DefaultBusyBoxImageRepo,
+			Tag:        solr.DefaultBusyBoxImageVersion,
+			PullPolicy: solr.DefaultPullPolicy,
+		}
+	}
+	return &TLSCerts{
+		ClientConfig: &TLSConfig{
+			Options:        prometheusExporter.Spec.SolrReference.SolrTLS.DeepCopy(),
+			KeystorePath:   DefaultKeyStorePath,
+			TruststorePath: DefaultTrustStorePath,
+		},
+		InitContainerImage: bbImage,
+	}
 }
 
 // Enrich the config for a SolrCloud StatefulSet to enable TLS, either loaded from a secret or
 // a directory on the main pod containing per-pod specific TLS files. In the latter case, the "mounted dir"
 // typically comes from an external agent (such as a cert-manager extension) or CSI driver that injects the
 // pod-specific TLS files using mutating web hooks
-func (tls *TLSConfig) enableTLSOnSolrCloudStatefulSet(stateful *appsv1.StatefulSet) {
+func (tls *TLSCerts) enableTLSOnSolrCloudStatefulSet(stateful *appsv1.StatefulSet) {
+	serverCert := tls.ServerConfig
+
 	// Add the SOLR_SSL_* vars to the main container's environment
-	tls.enableTLSOnPodTemplate(&stateful.Spec.Template)
+	mainContainer := &stateful.Spec.Template.Spec.Containers[0]
+	mainContainer.Env = append(mainContainer.Env, serverCert.serverEnvVars()...)
+	// Was a client cert mounted too? If so, add the client env vars to the main container as well
+	if tls.ClientConfig != nil {
+		mainContainer.Env = append(mainContainer.Env, tls.ClientConfig.clientEnvVars()...)
+	}
+
+	if serverCert.Options.PKCS12Secret != nil {
+		// Cert comes from a secret, so setup the pod template to mount the secret
+		serverCert.mountTLSSecretOnPodTemplate(&stateful.Spec.Template, "")
 
-	// volumes and mounts for TLS when using the mounted dir option
-	if tls.Options.MountedServerTLSDir != nil {
+		// mount the client certificate from a different secret (at different mount points)
+		if tls.ClientConfig != nil && tls.ClientConfig.Options.PKCS12Secret != nil {
+			tls.ClientConfig.mountTLSSecretOnPodTemplate(&stateful.Spec.Template, "client-")
+		}
+	} else if serverCert.Options.MountedTLSDir != nil {
 		// the TLS files come from some auto-mounted directory on the main container
-		tls.mountInitDbIfNeeded(stateful)
+		mountInitDbIfNeeded(stateful)
 		// use an initContainer to create the wrapper script in the initdb
 		stateful.Spec.Template.Spec.InitContainers = append(stateful.Spec.Template.Spec.InitContainers, tls.generateTLSInitdbScriptInitContainer())
 	}
 }
 
 // Enrich the config for a Prometheus Exporter Deployment to allow the exporter to make requests to TLS enabled Solr pods
-func (tls *TLSConfig) enableTLSOnExporterDeployment(deployment *appsv1.Deployment) {
+func (tls *TLSCerts) enableTLSOnExporterDeployment(deployment *appsv1.Deployment) {
+	clientCert := tls.ClientConfig
+
 	// Add the SOLR_SSL_* vars to the main container's environment
-	mainContainer := tls.enableTLSOnPodTemplate(&deployment.Spec.Template)
+	mainContainer := &deployment.Spec.Template.Spec.Containers[0]
+	mainContainer.Env = append(mainContainer.Env, clientCert.clientEnvVars()...)
+	mainContainer.Env = append(mainContainer.Env, corev1.EnvVar{Name: "SOLR_SSL_CHECK_PEER_NAME", Value: strconv.FormatBool(clientCert.Options.CheckPeerName)})
 
 	// the exporter process doesn't read the SOLR_SSL_* env vars, so we need to pass them via JAVA_OPTS
-	tls.appendTLSJavaOptsToEnv(mainContainer)
-
-	// volumes and mounts for TLS when using the mounted dir option
-	if tls.Options.MountedServerTLSDir != nil {
-		tls.mountTLSWrapperScriptAndInitContainer(deployment)
+	appendJavaOptsToEnv(mainContainer, clientCert.clientJavaOpts())
+
+	if clientCert.Options.PKCS12Secret != nil || clientCert.Options.TrustStoreSecret != nil {
+		// Cert comes from a secret, so setup the pod template to mount the secret
+		clientCert.mountTLSSecretOnPodTemplate(&deployment.Spec.Template, "")
+	} else if clientCert.Options.MountedTLSDir != nil {
+		// volumes and mounts for TLS when using the mounted dir option
+		clientCert.mountTLSWrapperScriptAndInitContainer(deployment, tls.InitContainerImage)
 	}
 }
 
-// Configures the TLS env vars, pod annotations, volumes, and pkcs12 initContainer on a Pod template spec
-// for enabling TLS on either StatefulSet or Deployment
-func (tls *TLSConfig) enableTLSOnPodTemplate(template *corev1.PodTemplateSpec) *corev1.Container {
-	// Add the SOLR_SSL_* vars to the main container's environment
+// Configures a pod template (either StatefulSet or Deployment) to mount the TLS files from a secret
+func (tls *TLSConfig) mountTLSSecretOnPodTemplate(template *corev1.PodTemplateSpec, namePrefix string) *corev1.Container {
 	mainContainer := &template.Spec.Containers[0]
-	mainContainer.Env = append(mainContainer.Env, tls.envVars()...)
 
-	if tls.Options.PKCS12Secret != nil {
-		// the TLS files are mounted from a secret, setup the volumes and mounts
-		template.Spec.Volumes = append(template.Spec.Volumes, tls.volumes()...)
-		mainContainer.VolumeMounts = append(mainContainer.VolumeMounts, tls.volumeMounts()...)
-
-		// track the MD5 of the TLS cert (from secret) to trigger restarts if the cert changes
-		if tls.Options.RestartOnTLSSecretUpdate && tls.CertMd5 != "" {
-			if template.Annotations == nil {
-				template.Annotations = make(map[string]string, 1)
-			}
-			template.Annotations[SolrTlsCertMd5Annotation] = tls.CertMd5
-		}
+	// the TLS files are mounted from a secret, setup the volumes and mounts
+	vols, mounts := tls.volumesAndMounts(namePrefix)
+
+	// We need an initContainer to convert a TLS cert into the pkcs12 format Java wants (using openssl)
+	// but openssl cannot write to the /var/solr/tls directory because of the way secret mounts work
+	// so we need to mount an empty directory to write pkcs12 keystore into
+	if tls.NeedsPkcs12InitContainer {
+		vols = append(vols, corev1.Volume{Name: "pkcs12", VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}})
+		mounts = append(mounts, corev1.VolumeMount{Name: "pkcs12", ReadOnly: false, MountPath: DefaultWritableKeyStorePath})
+		pkcs12InitContainer := tls.generatePkcs12InitContainer(mainContainer.Image, mainContainer.ImagePullPolicy, mounts)
+		template.Spec.InitContainers = append(template.Spec.InitContainers, pkcs12InitContainer)
+	}
+	template.Spec.Volumes = append(template.Spec.Volumes, vols...)
+	mainContainer.VolumeMounts = append(mainContainer.VolumeMounts, mounts...)
 
-		// use an initContainer to convert the keystore to p12 format if needed
-		if tls.NeedsPkcs12InitContainer {
-			pkcs12InitContainer := tls.generatePkcs12InitContainer(mainContainer.Image, mainContainer.ImagePullPolicy)
-			template.Spec.InitContainers = append(template.Spec.InitContainers, pkcs12InitContainer)
+	// track the MD5 of the TLS cert (from secret) to trigger restarts if the cert changes
+	if tls.Options.RestartOnTLSSecretUpdate && tls.CertMd5 != "" {
+		if template.Annotations == nil {
+			template.Annotations = make(map[string]string, 1)
 		}
+		template.Annotations[tls.CertMd5Annotation] = tls.CertMd5
 	}
 
 	return mainContainer
 }
 
 // Get a list of volumes for the keystore and optionally a truststore loaded from a TLS secret
-func (tls *TLSConfig) volumes() []corev1.Volume {
+func (tls *TLSConfig) volumesAndMounts(namePrefix string) ([]corev1.Volume, []corev1.VolumeMount) {
 	optional := false
 	defaultMode := int32(0664)
-	vols := []corev1.Volume{
-		{
-			Name: "keystore",
+	vols := []corev1.Volume{}
+	mounts := []corev1.VolumeMount{}
+	keystoreSecretName := ""
+
+	opts := tls.Options
+	if opts.PKCS12Secret != nil {
+		keystoreSecretName = opts.PKCS12Secret.Name
+		volName := namePrefix + "keystore"

Review comment:
       the prefix is either "" or "client-" ... I'll fix it with a helper method on `tls.Options`




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org