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.

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

Option to merge the JVM truststore with user-supplied truststore #461

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

thelabdude
Copy link
Contributor

@thelabdude thelabdude commented Jul 28, 2022

Fixes #390 ~ by allowing the JVM cacerts to get merged in with the user-supplied truststore (Let's Encrypt's CA is in the cacerts for modern JVM)

Users can now configure the TLS options to merge the JVM's truststore with the truststore for their certs using:

spec:
  ...
  solrTLS:
    ...
    trustStoreSecret:
      name: dev-selfsigned-cert-tls
      key: truststore.p12
    mergeJavaTrustStore: "$JAVA_HOME/lib/security/cacerts"

The path given in mergeJavaTrustStore option must exist on the Solr docker image! Thus, if users customize their Solr image, this path may be different.

Behind the scenes, this creates an additional initContainer that merges the two truststores together and then points the env var to the "merged" truststore. The actual merging is done with keytool.

For server TLS:

- name: SOLR_SSL_TRUST_STORE
  value: /var/solr/tls-merged/truststore.p12

By pointing SOLR_SSL_TRUST_STORE env var at the merged truststore, we're ensured that all the other initContainers and liveness probes continue to work (as they just use the env var to resolve this path).

Added a few simple unit tests and tested manually in my cluster.

For Prom exporter, the config would be:

spec:
  solrReference:
    ...
    solrTLS:
      ...
      mergeJavaTrustStore: "$JAVA_HOME/lib/security/cacerts"

Which results in the exporter container getting configed with env var:

- name: SOLR_SSL_CLIENT_TRUST_STORE
  value: /var/solr/tls-merged/truststore.p12

Copy link
Contributor

@HoustonPutman HoustonPutman left a comment

Choose a reason for hiding this comment

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

Would you be able to add something to the docs about this as well?

Also a changelog entry would be much appreciated 🙂

// Path on the Solr image to your JVM's truststore to merge with an external truststore.
// If supplied, Solr will be configured to use the merged truststore.
// The truststore for the JVM in the default Solr image is: $JAVA_HOME/lib/security/cacerts
MergeJavaTruststore string `json:"mergeJavaTrustStore,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add the // +optional tag for both of these new options? Just for consistency sake

MergeJavaTruststore string `json:"mergeJavaTrustStore,omitempty"`

// Password for the Java truststore to merge; defaults to "changeit"
MergeJavaTruststorePass string `json:"mergeJavaTrustStorePass,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a secret reference? How bad is it to have this in plain text?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seemed like overkill to me since it's the truststore pass for the JVM which is most likely "changeit" and isn't used by Solr ... that said, we can pull from a secret too ... doubt many would ever use this option.

Copy link
Contributor

Choose a reason for hiding this comment

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

ahhh ok, didn't understand it was unusual to change it. Sounds good

mergeMount := &corev1.VolumeMount{Name: volName, ReadOnly: false, MountPath: mountPath}
mounts = append(mounts, *mergeMount)

return &corev1.Container{
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we possibly use the default initContainer resources?

You can see how it was done in this PR: https://github.com/apache/solr-operator/pull/451/files#diff-653faf42eabedf3285e433f247c993282f035ee70781d151f8c8d68fee2621a3R618-R649

// Path on the Solr image to your JVM's truststore to merge with an external truststore.
// If supplied, Solr will be configured to use the merged truststore.
// The truststore for the JVM in the default Solr image is: $JAVA_HOME/lib/security/cacerts
MergeJavaTruststore string `json:"mergeJavaTrustStore,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work with mountedTLSDir?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typically, mountedTLSDir will have a csi driver volume and corresponding mount on the mainContainer, which would get used by the merge initContainer, so the init container would get the truststore file. However, that might cause double creation of the cert for each pod, once for the initContainer and once for the main container, so this would likely put a lot of pressure on the Cert issuer. So probably safer to say this feature is not supported with mountedTLSDir option for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, nvm all that volumesAndMounts doesn't include vols & mounts for the mountedTLSDir option. So this option is not going to work with mountedTLSDir.

So maybe we just punt on this feature for 0.6 and solve it using an init-db script instead? There's already some code in place for mounting a script into the init-db.

@thelabdude thelabdude marked this pull request as draft August 2, 2022 13:19
@thelabdude
Copy link
Contributor Author

I think we should hold off on this solution for 0.6 as now I'm thinking the better approach would be to use the init-db script approach built into the Solr image. The problem with the current solution in this PR is it doesn't work with the mountedTLSDir option. Originally I was thinking the volumes and mounts would be available to the merge initContainer but that's not the case. I think the init-db approach will actually be fairly easy to implement except for the Solr exporter. Unfortunately, I won't have time to work on this for a bit and I don't want to hold up 0.6

@HoustonPutman
Copy link
Contributor

Sounds good Tim. Thanks for putting in the effort on this though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

auto certificate renewal with restartOnTLSSecretUpdate and cert-manager fails
2 participants