-
-
Notifications
You must be signed in to change notification settings - Fork 0
feat: TLS support #55
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
base: main
Are you sure you want to change the base?
Conversation
| pub struct OpenSearchTls { | ||
| /// Affects client connections and internal transport connections. | ||
| /// This setting controls: | ||
| /// - If TLS encryption is used at all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If secret_class is not set, then TLS is not just disabled but the cluster does not start anymore:
org.opensearch.bootstrap.StartupException: java.lang.IllegalStateException: failed to load plugin class [org.opensearch.security.OpenSearchSecurityPlugin]
at org.opensearch.bootstrap.OpenSearch.init(OpenSearch.java:172) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.bootstrap.OpenSearch.execute(OpenSearch.java:159) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.common.cli.EnvironmentAwareCommand.execute(EnvironmentAwareCommand.java:110) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.cli.Command.mainWithoutErrorHandling(Command.java:138) ~[opensearch-cli-3.1.0.jar:3.1.0]
at org.opensearch.cli.Command.main(Command.java:101) ~[opensearch-cli-3.1.0.jar:3.1.0]
at org.opensearch.bootstrap.OpenSearch.main(OpenSearch.java:125) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.bootstrap.OpenSearch.main(OpenSearch.java:91) ~[opensearch-3.1.0.jar:3.1.0]
Caused by: java.lang.IllegalStateException: failed to load plugin class [org.opensearch.security.OpenSearchSecurityPlugin]
at org.opensearch.plugins.PluginsService.loadPlugin(PluginsService.java:881) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.plugins.PluginsService.loadBundle(PluginsService.java:820) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.plugins.PluginsService.loadBundles(PluginsService.java:615) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.plugins.PluginsService.<init>(PluginsService.java:229) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.node.Node.<init>(Node.java:536) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.node.Node.<init>(Node.java:464) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.bootstrap.Bootstrap$5.<init>(Bootstrap.java:249) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.bootstrap.Bootstrap.setup(Bootstrap.java:249) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.bootstrap.Bootstrap.init(Bootstrap.java:411) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.bootstrap.OpenSearch.init(OpenSearch.java:168) ~[opensearch-3.1.0.jar:3.1.0]
... 6 more
Caused by: java.lang.reflect.InvocationTargetException
at java.base/jdk.internal.reflect.DirectConstructorHandleAccessor.newInstance(DirectConstructorHandleAccessor.java:74) ~[?:?]
at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:502) ~[?:?]
at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:486) ~[?:?]
at org.opensearch.plugins.PluginsService.loadPlugin(PluginsService.java:872) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.plugins.PluginsService.loadBundle(PluginsService.java:820) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.plugins.PluginsService.loadBundles(PluginsService.java:615) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.plugins.PluginsService.<init>(PluginsService.java:229) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.node.Node.<init>(Node.java:536) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.node.Node.<init>(Node.java:464) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.bootstrap.Bootstrap$5.<init>(Bootstrap.java:249) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.bootstrap.Bootstrap.setup(Bootstrap.java:249) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.bootstrap.Bootstrap.init(Bootstrap.java:411) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.bootstrap.OpenSearch.init(OpenSearch.java:168) ~[opensearch-3.1.0.jar:3.1.0]
... 6 more
Caused by: org.opensearch.OpenSearchException: No SSL configuration found
at org.opensearch.security.ssl.SslSettingsManager.loadConfigurations(SslSettingsManager.java:128) ~[?:?]
at org.opensearch.security.ssl.SslSettingsManager.buildSslContexts(SslSettingsManager.java:96) ~[?:?]
at org.opensearch.security.ssl.SslSettingsManager.<init>(SslSettingsManager.java:83) ~[?:?]
at org.opensearch.security.ssl.OpenSearchSecuritySSLPlugin.<init>(OpenSearchSecuritySSLPlugin.java:248) ~[?:?]
at org.opensearch.security.OpenSearchSecurityPlugin.<init>(OpenSearchSecurityPlugin.java:347) ~[?:?]
at java.base/jdk.internal.reflect.DirectConstructorHandleAccessor.newInstance(DirectConstructorHandleAccessor.java:62) ~[?:?]
at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:502) ~[?:?]
at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:486) ~[?:?]
at org.opensearch.plugins.PluginsService.loadPlugin(PluginsService.java:872) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.plugins.PluginsService.loadBundle(PluginsService.java:820) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.plugins.PluginsService.loadBundles(PluginsService.java:615) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.plugins.PluginsService.<init>(PluginsService.java:229) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.node.Node.<init>(Node.java:536) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.node.Node.<init>(Node.java:464) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.bootstrap.Bootstrap$5.<init>(Bootstrap.java:249) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.bootstrap.Bootstrap.setup(Bootstrap.java:249) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.bootstrap.Bootstrap.init(Bootstrap.java:411) ~[opensearch-3.1.0.jar:3.1.0]
at org.opensearch.bootstrap.OpenSearch.init(OpenSearch.java:168) ~[opensearch-3.1.0.jar:3.1.0]
... 6 more
If the security plugin is enabled, then
TLS is optional for the REST layer and mandatory for the transport layer.
I would not allow to disable the security plugin, especially if it was enabled before, because then the security index becomes unprotected and confidential data could be leaked.
If it should be possible to disable TLS for the REST layer, then separate SecretClasses are required for the REST and transport layer.
| #[derive(Clone, Debug, Default, Deserialize, Eq, JsonSchema, PartialEq, Serialize)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct OpenSearchClusterConfig { | ||
| pub tls: OpenSearchTls, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other operators set a comment and default to the SecretClass tls, e.g. https://github.com/stackabletech/kafka-operator/blob/25.7.0/rust/operator-binary/src/crd/mod.rs#L158-L162.
| v1alpha1::NodeRole::RemoteClusterClient, | ||
| ])), | ||
| requested_secret_lifetime: Some( | ||
| Duration::from_str("15d").expect("should be a valid duration"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should default to 1 day like mentioned in the comment.
| /// Maximum length of annotation values | ||
| pub const MAX_ANNOTATION_LENGTH: usize = 253; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Annotation values are not restricted by their length.
| } | ||
|
|
||
| attributed_string_type! { | ||
| TlsSecretClassName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use the type SecretClassName instead of TlsSecretClassName. TLS is only the backend used by the SecretClass, but also k8sSearch could be used to reference TLS certificates. We can only check that a given string is a valid name for a SecretClass. We cannot check if the SecretClass actually will exist and what its backend will be. We should not define too specific types if we cannot validate them.
| ClusterName::from_str_unsafe("my-opensearch-cluster"), | ||
| NamespaceName::from_str_unsafe("default"), | ||
| uuid!("0b1e30e6-326e-4c1a-868d-ad6598b49e8b"), | ||
| OpenSearchClusterConfig::default(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By using the default here, which means no TLS, there exists not a single unit test which tests this feature.
| #[snafu(display("failed to set tls secret class"))] | ||
| ParseTlsSecretClassName { source: crate::framework::Error }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused
| pub name: ClusterName, | ||
| pub namespace: NamespaceName, | ||
| pub uid: Uid, | ||
| pub cluster_config: OpenSearchClusterConfig, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If OpenSearchClusterConfig is extended in the future, then it will probably contain unvalidated entries. If the whole structure is added now to ValidatedCluster, then it will have to be removed again and all the TLS related code has to be refactored.
Additionally, OpenSearchClusterConfig contains vector_aggregator_config_map_name which is already contained in role_group_configs.logging.vector_container.vector_aggregator_config_map_name.
I would directly add the OpenSearchTls structure here.
| - name: tls | ||
| mountPath: /stackable/opensearch/config/tls | ||
| readOnly: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not required
| - name: tls | ||
| ephemeral: | ||
| volumeClaimTemplate: | ||
| metadata: | ||
| annotations: | ||
| secrets.stackable.tech/class: tls | ||
| spec: | ||
| storageClassName: secrets.stackable.tech | ||
| accessModes: | ||
| - ReadWriteOnce | ||
| resources: | ||
| requests: | ||
| storage: "1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The documentation is missing.
- A changelog entry is missing.
- If it should be possible to disable TLS on the HTTP port, then an integration test is required for this case.
Description
spec.clusterConfig.tls.secretClassto CRDPart of #61
Definition of Done Checklist
Author
-
requestedSecretLifetimewas already decided in decision: Make autoTls certificate lifetime configurable issues#673.- SecretClass
Reviewer
Acceptance
type/deprecationlabel & add to the deprecation scheduletype/experimentallabel & add to the experimental features tracker