Support groups without password authentication

[huw0]

Presently the chart only supports groups where password authentication is used.

This removes the limitation so that groups can be used with any authentication type. My specific use case is to enable groups with certificate authentication. However, I've also found it beneficial to be able to set groups with no authentication which has been helpful for testing.

[huw0]

This PR has been open awhile and I can see that there are now some conflicts.
However I'd be grateful for a review before taking the time to resolve these? @losipiuk or @mosabua are you able to help here please?

[losipiuk]

I think this is fine. @nineinchnick opinion?

[nineinchnick]

Nit: it might be a bit confusing for users when they see this volume name (and the trino-password-authentication secret) with only groups; consider renaming this to something more generic, like file-authentication. This is just a suggestion, shouldn't block this PR.

[huw0]

Thanks for your feedback. I've amended the PR with the following changes:

• Rebased to main
• Renamed trino-password-authentication to trino-file-authentication. Since my initial PR there is now a conditional on the secret name to override the name where .auth.passwordAuthSecret has been set. I haven't renamed this value for backwards compatibility. Should this also be renamed?

Larger changes:

• The password file is now populated based on auth.passwordAuth alone rather than looking at whether password exists in server.config.authenticationType. This allows for reuse of the password config format with a different module if desired.

• Added a second commit to not set the group-provider.properties if this has also been set in coordinator.additionalConfigFiles. My use case for this is that I've created a custom module that extends the standard file based authentication and adds some additional groups from my corporate authentication provider. This allows re-use of the existing group configmap code, without forcing the default group provider.

[nineinchnick]

I haven't renamed this value for backwards compatibility. Should this also be renamed?

No, backward compatibility is more important.

[huw0]

Great thanks, I will leave as-is. Is this good to merge?

[jkoelker]

I think this broke auth.passwordAuthSecret since the file-authentication-volume never gets created now.

[huw0]

@jkoelker - Do you have a reproducer config you can share?

With auth.passwordAuthSecret set I've tested that if I have auth.groups and / or auto.passwordAuth then the secret is created and accessible in the coordinator.

[jkoelker]

@huw0 yep, here's a redacted version from my deployment:

  server:
    config:
      authenticationType: PASSWORD
  auth:
    passwordAuthSecret: trino-password-authentication
  additionalConfigProperties:
    - "internal-communication.shared-secret=shared-secret"
  ingress:
    enabled: true
    annotations:
      traefik.ingress.kubernetes.io/router.entrypoints: websecure
    hosts:
      - host: trino.my.cloud
        paths:
          - path: /
            pathType: Prefix
    tls:
      - secretName: trino-cert
        hosts:
          - trino.my.cloud

Adding the secret mount in will get it to work:

  coordinator:
    secretMounts:
      - name: trino-password-authentication
        secretName: trino-password-authentication
        path: /etc/trino/auth

[huw0]

ok, so I see the issue that setting config.authenticationType to PASSWORD then password-authenticator.properties refers to a password file that does not exist.

However, the password file is not present because no passwords are configured. Therefore, how are you using password authentication?

[jkoelker]

However, the password file is not present because no passwords are configured. Therefore, how are you using password authentication?

You just create a secret with a key password.db and the content and set the auth type. I personally use a sealed secret, so the contents are not in plain text anywhere other than on the cluster.

I think the fix is to just add Values.auth.passwordAuthSecret to the or condition that guards volume addition on the coordinator.

[huw0]

The fix you've suggested reverts to the previous behaviour and I'd be happy to raise a PR that does this (since this is after all my breakage!)

However this does not seem like an ideal fix?

The passwordAuthSecret sets the name of a secret that is managed by Trino. I'd expect this to be immutable, especially if paired with something like argocd?

Therefore should it not be the case that you maintain a separate secret outside of the trino chart?

In which case a better fix would be to modify password-authenticator.properties within the configmap-coordinator?

Either this should take a configurable path to the password file or when using a custom secret this file should be omitted and instead set through coordinator.additionalConfigFiles?

[jkoelker]

I'm not sure that's really the intention of passwordAuthSecret, there's no reason it has to be maintained by this chart, and I would advocate that not having the chart maintain it should be the preferred method of setting up password auth, as you can put the password.db in a sealed secret, which then makes it safe(er) to commit to source control, otherwise you end up having the raw password.db, potentially exposing the hashes.

This is the pattern in many other charts that have some sort of auth secret that they end up using, you can either let the chart manage it (like if you're just testing things out), or you can set the secret name where the chart will refer to, so an external service will manage the auth, which is much more suited for real world deployments.

Either way, I think it should just be documented, and if the choice is made that this chart must manage the secret insecurely, it should be strongly reccomended to not use password auth then.

[huw0]

@jkoelker does the fix here solve this for you?

[jkoelker]

I believe it will as i stated prior that adding the secret to the or condition would work. Do note that the linked commit also introduces a new requirement that the secret contain the group.db, which previously it did not. I would recommend either preserving the current and previous behavior, or introducing a new Values.auth.groupsAuthSecret, which users could set to the same secret if they so chose to have both files managed in the same manor.

[huw0]

Great - raised as #143