-
-
Notifications
You must be signed in to change notification settings - Fork 8
Description
Part of #809.
The SNI hostname checks appear to exist so that accidentally hitting the wrong node cannot happen.
We have opened an upstream issue and PR to allow to make this configurable:
- Upstream issue: https://issues.apache.org/jira/browse/NIFI-14858
- Upstream PR (rejected) NIFI-14858: Make SNI checking configurable apache/nifi#10201
- Docker image patch: Backport NIFI-14858 to NiFi 2.4.0 docker-images#1225
- Documentation change: docs: Document workaround to disable SNI checks #834
The upstream PR has been rejected but we still believe it is the correct solution and does not have a noticeable impact on security. And it is opt-in so we'll allow disabling SNI.
Tasks:
- Make SNI configurable (see above)
- Update NiFi documentation: https://docs.stackable.tech/home/stable/nifi/troubleshooting/#_http_error_400_invalid_sni
-
Consider adding a CRD flag to disable the SNI check-> Was considered, we decided against it
Related material:
- https://medium.com/@chnzhoujun/how-to-resolve-sni-issue-when-upgrading-to-nifi-2-0-907e07d465c5
- https://docs.stackable.tech/home/stable/nifi/troubleshooting/#_http_error_400_invalid_sni
- https://issues.apache.org/jira/browse/NIFI-14353
- https://issues.apache.org/jira/browse/NIFI-14209
- https://issues.apache.org/jira/browse/NIFI-14858
CRD Decision
As mentioned above, we want to be able to configure the SNI check being imposed by NiFi. Per default, it is enabled in NiFi (which is using the default Jetty configuration)
Additional information:
- The patch to NiFi introduced a breaking change for calls being made to IP addresses, since SNI is not applied there. This was fixed with Fix NIFI-14858 patch docker-images#1231
- Depending on the accepted proposal the patch needs either adjustment or we need to set the default to
false
forsniRequired
(maybe irrelevant now, with the fix in Fix NIFI-14858 patch docker-images#1231) - The relevant
sniRequired
field we modified in the patch and are setting in the potential new CRD fields is for the check after the TLS handshake. So technically not part of the SNI for TLS, just another check to improve error outputs, see https://jetty.org/docs/jetty/12/operations-guide/protocols/index.html#ssl-sni
Proposal 1
clusterConfig:
tls:
sniRequired: false # default false
sniHostCheck: true # default true
The first proposal is based on the patch we apply for NiFi. Naming of fields was taken from the Jetty configuration and it was sorted under tls, since SNI is a TLS extension. Default values are set to reflect the default NiFi/Jetty configuration of those values.
Proposal 2
clusterConfig:
tls:
sniHostCheck: true # default true
The second proposal was shortened to only include the sniHostCheck
field but requires an adjustment to the above patch.
In the NiFi code a SecureRequestCustomizer
is instantiated with its default values (false
for sniRequired
and true
for sniHostCheck
). In our patch we change the default behavior and set sniRequired
to true, which we then need to be able to set to false
again. If we would remove the code changing the sniRequired
value to true
, we could just make use of the default behavior and not having to disable it per config/CRD in the first place.
Proposal 3
No CRD change
As suggested by @NickLarsenNZ, disabling the SNI check might not be necessary in most cases if stackabletech/secret-operator#630 gets implemented. For the remaining unlikely possibility that the user still needs to disable the check, they can do so with configOverrides
.
Metadata
Metadata
Assignees
Labels
Type
Projects
Status
Status