-
Notifications
You must be signed in to change notification settings - Fork 230
Conversation
@AyadiAmen can you test that this feature is disabled by default via the |
@banzo i'll test that later today and i'lll let you know asap. |
Does the readiness probe still work with this? We have been going down the path of ldap auth and found that the initial admin identity needs to be the node in order to use the secured nifi api. |
@iammoen yes it works, after the last commit you can pass your “Initial Admin Identity” in the variable auth.ldap.admin in the |
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.
Hello @AyadiAmen, thanks for your work on this feature :) ! Can you see my comments before we can merge?
When it is fixed, I will try your new feature on my laptop.
#### Configure the cluster security: | ||
|
||
- **Ldap**: Enable ldap to secure the cluster and add user/password authentication. when ldap is enabled make sure to change the variables `properties.isSecure` and `properties.clusterSecure` to **true** and set `properties.httpPort` to **null** and `properties.httpsPort` to **9443**. | ||
- Also to use ldap you need to set the **namesapce** and the **release-name** you're going to use in advance respectivly in `properties.namespace` and `properties.release` . |
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 release name can be retrieved by .Release.Name so the user doesn't need to add it in values file.
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.
Why the namespace name is needed?
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.
Also I propose to add ldap in a sub-section as it's one of the possible auth method. You can also say that it's mandatory for nifi to enable the ssl to use an auth method.
configs/authorizers.xml
Outdated
<property name="Manager DN">{{.Values.auth.ldap.admin}}</property> | ||
<property name="Manager Password">{{.Values.auth.ldap.pass}}</property> | ||
<property name="TLS - Keystore">/opt/nifi/nifi-current/conf/localhost/keystore.jks</property> | ||
<property name="TLS - Keystore Password">keystorePasswdfadi</property> |
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.
It should be possible to pass all these values via the values.yaml. You can nevertheless set ldap.enabled to false and add default values for the ldap propreties.
configs/login-identity-providers.xml
Outdated
<property name="TLS - Client Auth"></property> | ||
<property name="TLS - Protocol"></property> | ||
<property name="TLS - Shutdown Gracefully"></property> | ||
<property name="Authentication Strategy">SIMPLE</property> |
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.
All these values should be pass via the values.yaml
configs/nifi.properties
Outdated
@@ -144,6 +146,18 @@ nifi.sensitive.props.algorithm=PBEWITHMD5AND256BITAES-CBC-OPENSSL | |||
nifi.sensitive.props.provider=BC | |||
nifi.sensitive.props.additional.keys= | |||
|
|||
{{if .Values.auth.ldap.enabled}} | |||
nifi.security.keystore=/opt/nifi/nifi-current/conf/{{.Values.properties.release}}-nifi-0.{{.Values.properties.release}}-nifi-headless.{{.Values.properties.namespace}}.svc.cluster.local/keystore.jks |
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 passing nifi-0
it will not work for a cluster of nifi. you can retrieve this value with hostname -f, and the variable should be injected in the propreties file.
@@ -144,6 +146,18 @@ nifi.sensitive.props.algorithm=PBEWITHMD5AND256BITAES-CBC-OPENSSL | |||
nifi.sensitive.props.provider=BC | |||
nifi.sensitive.props.additional.keys= | |||
|
|||
{{if .Values.auth.ldap.enabled}} | |||
nifi.security.keystore=/opt/nifi/nifi-current/conf/{{.Values.properties.release}}-nifi-0.{{.Values.properties.release}}-nifi-headless.{{.Values.properties.namespace}}.svc.cluster.local/keystore.jks | |||
nifi.security.keystoreType=jks |
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.
via values.yaml
values.yaml
Outdated
@@ -17,8 +17,8 @@ image: | |||
# pullSecret: myRegistrKeySecretName | |||
|
|||
securityContext: | |||
runAsUser: 1000 | |||
fsGroup: 1000 | |||
runAsUser: 0 |
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 acceptable! we can't use root user
values.yaml
Outdated
isSecure: false # switch to true if the cluster is secured ( if you're using Ldap for example ) | ||
webProxyHost: | ||
webHttpsHost: | ||
release: nifi # if you're using the secured cluster, provide the release-name here before deploying. |
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.
use hostname -f
in your scripts, so the user do not need to use these variables.
values.yaml
Outdated
needClientAuth: false | ||
provenanceStorage: "8 GB" | ||
siteToSite: | ||
secure: false | ||
secure: 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.
should be false by default?
httpPort: 8080 # switch to null ( 8080 by default )if the cluster is secured ( if you're using Ldap ) | ||
httpsPort: null # switch to 9443 ( null by default ) if the cluster is secured ( if you're using Ldap ) | ||
clusterPort: 8443 | ||
clusterSecure: false # switch to true if the cluster is secured ( if you're using Ldap ) |
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.
sould be usable even if we don't use ldap auth
type: NodePort | ||
httpPort: 8080 | ||
httpsPort: 443 | ||
type: LoadBalancer |
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.
-> NodePort
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.
there's an issue to access nifi when it runs in a secured cluster, accessing it with NodePort is not possible for and there might be a solution with LoadBalener.
searchFilter: CN=john | ||
admin: cn=admin,dc=example,dc=com | ||
searchFilter: (objectClass=*) | ||
UserIdentityAttribute: cn |
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.
Should this variable be lower camel case like all the other ones?
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 confirm @AyadiAmen SSL, UserIdentity, AuthStrategy, IdentityStrategy, AuthExpiration should be lower case.
<property name="TLS - Shutdown Gracefully"></property> | ||
<property name="Referral Strategy">FOLLOW</property> | ||
<property name="Authentication Strategy">{{.Values.auth.ldap.AuthStrategy}}</property> | ||
<property name="Manager DN">{{.Values.auth.ldap.admin}}</property> |
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.
was also thinking that maybe this should be a different ldap user besides the one for initial admin? Since this one will be used to bind to the ldap server it would probably be a service account of some kind whereas the initial admin would be likely to be a proper user?
Not to hijack this PR, but but i've also created my own PR #76 that configures TLS support (using nifi-toolkit) and OIDC authentication (tested using adfs). |
@makeacode once #76 merged, we can use this PR to work on the ldap auth :) |
theoretically once TLS is enabled the auth should be pretty simple....theoretically. |
Practically it's too quite simple. there are few quirks(like adding proper permissions manually in UI) but this may be because of our configuration changes and nature of hybrid nifi cluster. |
Thanks for your feedback @piotron! Could you share your configuration setup? It would help to improve the Readme part/section about ldap integration with nifi. |
We setup nifi with ldap and it seems to work: we can see that nifi and openldap communicate together in their respective logs. But we face a new error. We can't reach the UI as described in #72... Any ideas? |
Current work is on branch feature: https://github.com/cetic/helm-nifi/tree/feature/atuh |
@alexnuttinck you have this issue with browser OR nifi/minifi agents(We noticed that MiNiFi agent's cannot proxy through loadbalancer properly and had to do workaround with proxy setup) Have you set up webProxyHost properly(including nifi listening port nifi.test:9443 ?)
then your entry for webProxyHost also should contain domain:port because statefulset does not set it Other case your UI cannot open port 443 because of security settings. Ports below 1024 needs to have specific capabilities to open ports for non-root user. |
@piotron thanks for the reply! This is a browser issue. Ok, I didn't setup the port for the webProxyHost, only the domain. Let's have a try. |
Thank you @alexnuttinck and @piotron , i tried nifi.test:9443 but it didn't work, i even tried to pass nifi.test:9443 in webProxyHost and still not working, the only way i can reach the UI now is via forwarding the port 9443 and using localhost. |
@AyadiAmen could you past whole list of allowed domains? it should show all allowed ports and combinations for access. |
Valid host headers are [empty] or: |
@AyadiAmen and your webProxyHost is exactly |
@piotron i tried with webProxyHost: nifi.test, webProxyHost: nifi.test:9443 and webProxyHost: nifi.test, nifi.test:9443 with the branches feature/atuh and feature/ldap |
@AyadiAmen if possible please share values.yml(ommiting internal data ofc) you used, and overrides. |
|
@AyadiAmen please try this file
and call https://nifi.test:9443 |
@piotron still not accessible :/ |
@AyadiAmen this may be wild guess now but try to disable ingress and return it to default. because now this is only thing that differs. |
@piotron and access it how without ingress ? i'll try anyway but i think the ingress should the solution here |
@AyadiAmen Maybe Ingress is a way to go but for now we managed to get it to work with separate LoadBalancer service this way it presents with it's own certificates. Our setup is K3S + Metallb |
@AyadiAmen I'm definitely not a nifi expert but when i was working on adding TLS support it seemed that nifi wants/needs to do the termination itself and it cannot be done at the ingress level (unless ingress can do some trickery i'm not aware of). I used EKS for my development/testing and exposed nifi through a LoadBalancer and set the url i wanted to access it by to webProxyHost. After that I just added a dns CNAME to point the webProxyHost value to the load balancer url so all the values would match up. |
What is this status of this ldap support as the currently implementation is a bit limiting especially when upgrading the chart as it wipes my configuration |
Done in #107. I close this PR. |
What this PR does / why we need it:
Which issue this PR fixes
This pull request adds the connection to ldap server for authentication.
Special notes for your reviewer:
Please check the comments in mentioned issue ( #45 ) for a better understand of all the changes and configurations.
Checklist
[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]