Skip to content

New AuthenticationClass provider: static #494

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

Closed
6 tasks
Tracked by #293
sbernauer opened this issue Oct 18, 2022 · 15 comments · Fixed by stackabletech/documentation#323
Closed
6 tasks
Tracked by #293

New AuthenticationClass provider: static #494

sbernauer opened this issue Oct 18, 2022 · 15 comments · Fixed by stackabletech/documentation#323
Assignees
Labels
priority/high release-note/action-required Denotes a PR that introduces potentially breaking changes that require user action. type/feature-new

Comments

@sbernauer
Copy link
Member

sbernauer commented Oct 18, 2022

We have multiple products (e.g. Trino) that can take a list of usernames + passwords and authenticate users against that list (see https://trino.io/docs/current/security/password-file.html)
Trino e.g. takes a list of bcrypt passwords.

We need an standardized way to express such list of usernames + passwords. We already have the standardized AuthenticationClass, so we want a new provider 'static'

apiVersion: authentication.stackable.tech/v1alpha1
kind: AuthenticationClass
metadata:
  name: simple-trino-users
spec:
  provider:
    static:
      userCredentialsSecret:
        name: simple-trino-users-secret # mandatory, needs to be in same Namespace as product
---
apiVersion: v1
kind: Secret
metadata:
  name: simple-trino-users-secret
  namespace: default
type: kubernetes.io/opaque
stringData:
  admin: admin
  alice: superpass
  bob: secret

The only possible problem is that the AuthenticationClass is cluster-scoped and Secrets are namespaced.
It might be the case that the current solution has the downside that the userCredentialsSecret needs to be in the same Namespace as the Product using it (Secrets can't be mounted cross-namespace). A solution could be the cluster-scoped SecretClass but that introduces complexity

  • We store the credentials as plain text within the Secret. Some products need the credentials as plain text, others hashed. To achieve a commons mechanism we need to store the credentials in plain text.
  • The secret gets mounted as files. The entrypoint of the product Pod collects them together in the format accepted by the product. If hashing is need it hashes as well. If it makes sense, parts are moved to operator-rs.
  • Restart-controller is enabled and should work as normal
  • OPTIONAL: Some product allow hot-reloading the credentials (e.g. Trino). In this case no restart shut be done, i think the secret should update automatically. This would need additional functionality in restart controller to white- or blacklist certain volumes.
  • If hot-reloading is not implemented, follow-up ticket is created.
  • Documentation exists (ideally also for tls, see this comment but if that becomes too much we can split this in an extra ticket
@nightkr
Copy link
Member

nightkr commented Nov 1, 2022

This is tricky... Ideally we'd only store the passwords in a hashed form, but that assumes that every service uses the same hashing scheme. Additionally, PBKDFs are nondeterministic and slow, so we'd need a strategy of some kind to figure out when to regenerate downstream credential files...

@sbernauer
Copy link
Member Author

Yeah, i think we have multiple products such as Superset admin user that need the credentials in plain text.

@sbernauer sbernauer moved this from Refinement: Waiting for to Refinement: In Progress in Stackable Engineering Nov 4, 2022
@sbernauer
Copy link
Member Author

sbernauer commented Nov 4, 2022

The problem with consistent hashing remains. I see the following (non-exhausting) options:

  1. Mount the secrets as files and hash stuff in the product entrypoint. Restart controller should work normally. Code to generate entrypoint command should be moved into operator-rs.
    • Actually e.g. Trino can hot-reload this lists, so no restart needed
      • Maybe this needs new functionality in restart controller, because we want it to restart when ldap credentials change, but not if user credentials change. Something like restarter.stackable.tech/volumeBlacklist or similar
  2. Commons operator watches the credentials secret and automatically creates a new secret or adds keys to the secret containing the hashed variants. This might make things easier, but the commons-op needs to handle user credentials which we try to avoid.

@lfrancke
Copy link
Member

lfrancke commented Nov 4, 2022

Just to keep it in mind: One result of your refinement could be to just not do it now
If this turns out to be much more effort than expected (more than 1 week) we might skip it for now as we have more important things to work on.

@sbernauer
Copy link
Member Author

Further refined the ticket, would present it after daily (thus blocking it).
I think this ticket only introduces things in operator-rs (and subsequentially commons-operator). Should be easily doable in a week (don't quote me on that :D).

@sbernauer sbernauer moved this from Refinement: In Progress to Refinement Acceptance: Waiting for in Stackable Engineering Nov 7, 2022
@sbernauer
Copy link
Member Author

Presented after daily

@lfrancke
Copy link
Member

lfrancke commented Nov 8, 2022

This doesn't need docs because it's only in the framework for now, right?

@lfrancke lfrancke moved this from Refinement Acceptance: Waiting for to Ready for Development in Stackable Engineering Nov 8, 2022
@sbernauer
Copy link
Member Author

I would say this needs docs. We should document tls provider as well as it's missing
Docs reside within commons-operator currently (https://docs.stackable.tech/commons-operator/stable/authenticationclass.html) but should be moved to the new Concepts section IMHO

@fhennig
Copy link
Contributor

fhennig commented Nov 8, 2022

I've made a ticket stackabletech/documentation#311

@siegfriedweber
Copy link
Member

OpenSearch can use a configuration file with statically defined users, see https://github.com/opensearch-project/security/blob/2.4/config/internal_users.yml:

admin:
  hash: "$2a$12$VcCDgh2NDk07JGN0rjGbM.Ad41qVR/YFJcgHp0UGns5JDymv..TOG"
  reserved: true
  backend_roles:
  - "admin"
  description: "Demo admin user"

If the secret for the AuthenticationClass would allow arbitrary metadata per user then the internal users for OpenSearch could also be generated.

@sbernauer
Copy link
Member Author

Do you already have a idea how we can represent this in k8s Secrets?

@siegfriedweber
Copy link
Member

@sbernauer and I talked about integrating metadata into the secret. We concluded to implement the ticket as it is for now because a custom structure in the secret would be difficult to use and the OpenSearch use case is not urgent.

@fhennig fhennig moved this from Ready for Development to Refinement: In Progress in Stackable Engineering Nov 15, 2022
@fhennig fhennig moved this from Refinement: In Progress to Ready for Development in Stackable Engineering Nov 15, 2022
@sbernauer sbernauer moved this from Ready for Development to Development: In Progress in Stackable Engineering Nov 16, 2022
@sbernauer sbernauer self-assigned this Nov 16, 2022
@sbernauer sbernauer moved this from Development: In Progress to Development: Waiting for Review in Stackable Engineering Dec 2, 2022
bors bot pushed a commit that referenced this issue Dec 2, 2022
@sbernauer sbernauer moved this from Development: Waiting for Review to Development: In Progress in Stackable Engineering Dec 2, 2022
@Pscheidl
Copy link
Contributor

Pscheidl commented Dec 4, 2022

Passwords are best stored encrypted, not hashed. Hashing doesn't work for multiple reasons (e.g. salting not possible).

This is a solved problem: sealed secrets. The solution is a little bit heavyweight, but I think this could solve your problem ? Either mimic their solution, or use sealed secrets directly ? The controller is available separately from the rest of their stack. Making your controller watch for sealed secrets (label them ?) would do the trick ?

@sbernauer
Copy link
Member Author

sbernauer commented Dec 5, 2022

Hi @Pscheidl thanks for your suggestion!
First off let's talk about our foundation: Most of the products need the credentials in plain text, some in some sort of hashed form. I personally would say, I don't need them to be hashed, but we need to hash them for e.g. Trino.
The current plan is that users need to provided Secrets, which than get mounted into the Product Pods. The Stackable operators itself don't need to read the Secrets, which improves the number of participants with access to the credentials (also no accidentally logging etc.). If desired, users can use sealed secrets checked into Git and deploy them via CI/CD. The sealed-secrets-controller is than able to convert the SealedSecret to a plain Secret, which than can be mounted into Trino.
The nice thing about having Secrets as a shared interface is that users can decide if the want to use SealedSecrets or vanilla Secrets. We should be able to support both.

My questions would be, if that aligns with your understanding of the current approach? Do you have any additional requirements or ideas?

@Pscheidl
Copy link
Contributor

Pscheidl commented Dec 7, 2022

Hello @sbernauer , definitely no additional ideas, if sealed secrets can be used with the current approach, then there's nothing more to wish for 😉 .

@sbernauer sbernauer moved this from Development: In Progress to Development: Waiting for Review in Stackable Engineering Dec 8, 2022
@sbernauer sbernauer reopened this Dec 13, 2022
@maltesander maltesander moved this from Development: Waiting for Review to Development: In Review in Stackable Engineering Dec 13, 2022
@sbernauer sbernauer moved this from Development: In Review to Development: Done in Stackable Engineering Dec 13, 2022
@lfrancke lfrancke moved this from Development: Done to Done in Stackable Engineering Dec 13, 2022
@lfrancke lfrancke added the release-note/action-required Denotes a PR that introduces potentially breaking changes that require user action. label Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/high release-note/action-required Denotes a PR that introduces potentially breaking changes that require user action. type/feature-new
Projects
Archived in project
6 participants