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

Add single-user authentication #278

Merged
merged 35 commits into from
Aug 21, 2023
Merged

Add single-user authentication #278

merged 35 commits into from
Aug 21, 2023

Conversation

O-Bezimo
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
API breaks? no
Deprecations? no
Related tickets fixes #X, partially #Y, mentioned in #Z
License Apache 2.0

What's in this PR?

Our objective is to add the Single-User authentication in addition to the OIDC and LDAP authentication methods.

We added a SingleUserConfiguration object in the NifiCluster ressource.
The options are the following:

  • Enabled boolean: set to true to add the according single-user configuration blocs in the nifi.properties template and the login_identity_provider template.
  • SecretRef SecretReference: reference to a kubernetes secret containing the desired username and password.
  • AuthorizerEnabled bool: set to true to use the single-user-authorizer. Set to false to use the default managed-authorizer.

We also added the reference to the SingleUserConfiguration in the helm folder.

Why?

If neither OIDC or LDAP are configured, nifi forces a certificate authentication on all HTTP/HTTPS certificates.
We need another option so that we can have functionnal healthchecks on our loadbalancer generated by the ingress-controller.

Additional context

We tested the code with an integration cluster hosted on gcp and telepresence as a proxy for our modified operator.

We also proofed some error cases:

  • if SingleUserConfiguration.Enabled is false and SingleUserConfiguration.AuthorizerEnabled is true: don't configure the authorizer
  • if SecretRef is empty: use auto generated username and password that would appear in the logs

Checklist

  • Implementation tested
  • Error handling code meets the guideline
  • Logging code meets the guideline
  • User guide and development docs updated (if needed)
  • Append changelog with changes

@O-Bezimo O-Bezimo marked this pull request as ready for review July 31, 2023 14:11
Copy link
Contributor

@juldrixx juldrixx left a comment

Choose a reason for hiding this comment

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

Please delete the versionned documentation, the next version has not yet been decided.

And add the appropriate changes to the "unversioned" documentation to reflect your changes.

README.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated
## v1.2.0

### Added

- [PR #258](https://github.com/konpyutaika/nifikop/pull/141) - **[Helm Chart]** Upgraded helm-deployed HPA to v2 and added flowPosition to NiFiDataflow.
- [PR #258](https://github.com/konpyutaika/nifikop/pull/141) - **[Operator]** Upgraded helm-deployed HPA to v2 and added flowPosition to NiFiDataflow.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- [PR #258](https://github.com/konpyutaika/nifikop/pull/141) - **[Operator]** Upgraded helm-deployed HPA to v2 and added flowPosition to NiFiDataflow.
- [PR #258](https://github.com/konpyutaika/nifikop/pull/141) - **[Helm Chart]** Upgraded helm-deployed HPA to v2 and added flowPosition to NiFiDataflow.

This change should not have occurred, and it may be necessary to rebase the master.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified accordingly

CHANGELOG.md Outdated
Comment on lines 13 to 18
## v1.3.0

### Added

- [PR #278](https://github.com/konpyutaika/nifikop/pull/278) - **[Operator]** Added the single-user-authentication method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
## v1.3.0
### Added
- [PR #278](https://github.com/konpyutaika/nifikop/pull/278) - **[Operator]** Added the single-user-authentication method.
```suggestion
## Unreleased
### Added
- [PR #278](https://github.com/konpyutaika/nifikop/pull/278) - **[Operator]** Added the single-user-authentication method.
### Changed
### Fixed Bugs
### Deprecated
### Removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modified accordingly

@juldrixx juldrixx requested a review from mh013370 July 31, 2023 14:36
@juldrixx
Copy link
Contributor

@mh013370 Should secret keys be supplied in the operator configuration or forced as now?

@O-Bezimo
Copy link
Contributor Author

O-Bezimo commented Aug 1, 2023

@mh013370 Should secret keys be supplied in the operator configuration or forced as now?

Secret keys are supplied if singleUserConfiguration->enabled is true and if singleUserConfiguration->secretRef is not given in the nificluster.yml configuration. If enabled is true and secretRef is declared and exists in the cluster with a username and password, the operator will use these instead as credentials and add it in the login_identity_providers file.

@mh013370
Copy link
Member

mh013370 commented Aug 2, 2023

@mh013370 Should secret keys be supplied in the operator configuration or forced as now?

@mh013370 Should secret keys be supplied in the operator configuration or forced as now?

Secret keys are supplied if singleUserConfiguration->enabled is true and if singleUserConfiguration->secretRef is not given in the nificluster.yml configuration. If enabled is true and secretRef is declared and exists in the cluster with a username and password, the operator will use these instead as credentials and add it in the login_identity_providers file.

If SingleUserConfiguration.SecretRef is optional, then it should be expressed as such in the spec by using a pointer (allowing you to check for nil) and adding the // +optional kubebuilder annotation.

@mh013370
Copy link
Member

mh013370 commented Aug 2, 2023

Why are there a whole new set of unversioned docs?

@O-Bezimo
Copy link
Contributor Author

O-Bezimo commented Aug 2, 2023

@mh013370 Should secret keys be supplied in the operator configuration or forced as now?

@mh013370 Should secret keys be supplied in the operator configuration or forced as now?

Secret keys are supplied if singleUserConfiguration->enabled is true and if singleUserConfiguration->secretRef is not given in the nificluster.yml configuration. If enabled is true and secretRef is declared and exists in the cluster with a username and password, the operator will use these instead as credentials and add it in the login_identity_providers file.

If SingleUserConfiguration.SecretRef is optional, then it should be expressed as such in the spec by using a pointer (allowing you to check for nil) and adding the // +optional kubebuilder annotation.

Thank you for your answer, the modification have been made

@O-Bezimo
Copy link
Contributor Author

O-Bezimo commented Aug 3, 2023

@mh013370 Should secret keys be supplied in the operator configuration or forced as now?

@mh013370 Should secret keys be supplied in the operator configuration or forced as now?

Secret keys are supplied if singleUserConfiguration->enabled is true and if singleUserConfiguration->secretRef is not given in the nificluster.yml configuration. If enabled is true and secretRef is declared and exists in the cluster with a username and password, the operator will use these instead as credentials and add it in the login_identity_providers file.

If SingleUserConfiguration.SecretRef is optional, then it should be expressed as such in the spec by using a pointer (allowing you to check for nil) and adding the // +optional kubebuilder annotation.

A pointer is added to Secret ref and the empty checks are made with nil as of now. An optional annotation is added and the same is made to a secretKeys struct allowing the final user to change the username and password keys. They will default to "username" and "password" if nil and the according information are added in the documentation and in the helm folder.

@O-Bezimo
Copy link
Contributor Author

O-Bezimo commented Aug 3, 2023

Why are there a whole new set of unversioned docs?

I switched the 1.3 versioned docs to unversioned which was a mistake. Since then I deleted it and moved the modifications in ./site/docs.

api/v1/nificluster_types.go Show resolved Hide resolved
api/v1/nificluster_types.go Show resolved Hide resolved
helm/nifi-cluster/templates/nifi-cluster.yaml Outdated Show resolved Hide resolved
helm/nifi-cluster/values.yaml Outdated Show resolved Hide resolved
pkg/resources/templates/config/authorizers.go Outdated Show resolved Hide resolved
pkg/resources/templates/config/authorizers.go Outdated Show resolved Hide resolved
@konpyutaika konpyutaika deleted a comment from gitguardian bot Aug 11, 2023
@juldrixx juldrixx self-requested a review August 11, 2023 14:32
@gitguardian
Copy link

gitguardian bot commented Aug 14, 2023

⚠️ GitGuardian has uncovered 2 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request
GitGuardian id Secret Commit Filename
- Generic Password b2f5f75 pkg/resources/nifi/pod.go View secret
- Generic Password 0978cc6 pkg/resources/nifi/pod.go View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

helm/nifi-cluster/values.yaml Outdated Show resolved Hide resolved
helm/nifi-cluster/values.yaml Outdated Show resolved Hide resolved
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.

None yet

4 participants