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

ensure secret names are unique and rfc1123 compliant #138

Merged
merged 6 commits into from Jan 28, 2021

Conversation

kwmonroe
Copy link
Member

@kwmonroe kwmonroe commented Jan 14, 2021

Add a random string to the generated secret names to ensure they are unique for a cluster. Names also need to be rfc1123 compliant:

https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-subdomain-names

Fixes LP 1911445

@kwmonroe kwmonroe marked this pull request as draft January 14, 2021 16:02
@kwmonroe
Copy link
Member Author

As noted in the linked bug, admins could create a user with non-rfc1123 data (like an uppercase letter). The charm would use that as part of the k8s secret name, which would cause an error in secret creation:

$ juju run-action --wait kubernetes-master/0 user-create name=Bob
unit-kubernetes-master-0:
  UnitId: kubernetes-master/0
  id: "31"
  results:
    Stderr: |
      The Secret "Bob-token-auth" is invalid: metadata.name: Invalid value: "Bob-token-auth": a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')
    Stdout: |
      Cluster "juju-cluster" set.
      Property "users" unset.
      User "Bob" set.
      Context "juju-context" modified.
      Switched to context "juju-context".
    kubeconfig: juju scp kubernetes-master/0:/home/ubuntu/Bob-kubeconfig .
    msg: User "Bob" created.
    users: admin, system:kube-controller-manager, system:kube-proxy, system:node:juju-2a8f8a-10,
      system:node:juju-2a8f8a-11, system:node:juju-2a8f8a-12, system:kube-scheduler,
      system:monitoring, Bob
  status: completed

$ kubectl get secrets -n kube-system --kubeconfig config | grep -i bob || echo "not found"
not found

With this fix, the charm will ensure the secret name is valid so it can be created:

$ juju run-action --wait kubernetes-master/0 user-create name=Bob
unit-kubernetes-master-0:
  UnitId: kubernetes-master/0
  id: "33"
  results:
    Stdout: |
      Cluster "juju-cluster" set.
      Property "users" unset.
      User "Bob" set.
      Context "juju-context" modified.
      Switched to context "juju-context".
    kubeconfig: juju scp kubernetes-master/0:/home/ubuntu/Bob-kubeconfig .
    msg: User "Bob" created.
    users: admin, system:kube-controller-manager, system:kube-proxy, system:node:juju-2a8f8a-10,
      system:node:juju-2a8f8a-7, system:node:juju-2a8f8a-11, system:node:juju-2a8f8a-8,
      system:node:juju-2a8f8a-12, system:node:juju-2a8f8a-9, system:kube-scheduler,
      system:monitoring, Bob
  status: completed

$ kubectl get secrets -n kube-system --kubeconfig config | grep -i bob || echo "not found"
bob-l1xrzi1bv7-token-auth                        juju.is/token-auth                    4      95s

@kwmonroe kwmonroe marked this pull request as ready for review January 14, 2021 17:22
@kwmonroe
Copy link
Member Author

kwmonroe commented Jan 14, 2021

Initially, i didn't bother to validate the name parameter for the user_create action since create_secret now ensures valid secret names in the charm. However, if someone added a name that contained our secret delimiter (::), it would cause problems.

I can't think of a good use case to allow anything but alphanum, '@', '-', and '.' for the name parameter, so the latest commit enforces that.

actions.yaml Show resolved Hide resolved
description: Username for the new user
description: |
Username for the new user. This value must only contain alphanumeric
characters, '@', '-' or '.'.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should : be allowed, too? I ask because : is a prominent character used in a lot of user names, as seen from user-list:

$ juju run-action --wait kubernetes-master/0 user-list
unit-kubernetes-master-0:
  UnitId: kubernetes-master/0
  id: "16"
  results:
    users: admin, Bob, bob@foo, bob, system:kube-controller-manager, system:kube-proxy,
      system:node:ip-172-31-47-88, system:node:ip-172-31-8-169, system:node:ip-172-31-21-142,
      system:kube-scheduler, system:monitoring
  status: completed
  timing:
    completed: 2021-01-15 20:08:43 +0000 UTC
    enqueued: 2021-01-15 20:08:41 +0000 UTC
    started: 2021-01-15 20:08:42 +0000 UTC

which, as a dumb user, makes me think I might be able to create a user named custom:bob, but no:

$ juju run-action --wait kubernetes-master/0 user-create name=custom:bob
unit-kubernetes-master-0:
  UnitId: kubernetes-master/0
  id: "20"
  message: User name may only contain alphanumeric characters, '@', '-' or '.'
  results:
    users: admin, Bob, bob@foo, bob, system:kube-controller-manager, system:kube-proxy,
      system:node:ip-172-31-47-88, system:node:ip-172-31-8-169, system:node:ip-172-31-21-142,
      system:kube-scheduler, system:monitoring
  status: failed
  timing:
    completed: 2021-01-15 20:12:14 +0000 UTC
    enqueued: 2021-01-15 20:12:08 +0000 UTC
    started: 2021-01-15 20:12:13 +0000 UTC

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh right, you said:

However, if someone added a name that contained our secret delimiter (::), it would cause problems.

So if we allow : then that problem returns.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it could work to allow : if we updated auth-webhook's delimiter handling to split on only the right-most occurrence of ::.

I don't think this is a show stopper though.

@Cynerva
Copy link
Contributor

Cynerva commented Jan 15, 2021

This LGTM. But I would be happy to see some consideration given to the mixed messages coming from user-list and user-create regarding the : character.

…-rfc1123 chars do not start the name; drop unnecessary suffix since the generator ensures a valid end char
@kwmonroe
Copy link
Member Author

This LGTM. But I would be happy to see some consideration given to the mixed messages coming from user-list and user-create regarding the : character.

Thanks for the feedback. I'm moving user-list around a bit in lp 1906732, so i'll tweak the action wording and/or double check the allowance of : in the pr for that.

@kwmonroe kwmonroe merged commit 39b24d5 into master Jan 28, 2021
@kwmonroe kwmonroe deleted the lp1911445-uniq-secret branch January 28, 2021 15:07
Cynerva pushed a commit that referenced this pull request Feb 19, 2021
* add rfc1123 helper and ensure secret id is compliant

* unit test for generate_rfc1123

* tweak secret_id format

* validate name during user_create

* ensure generate_rfc1123 starts/ends with alphanum

* adjust secret name to start with "auth-" to ensure usernames with non-rfc1123 chars do not start the name; drop unnecessary suffix since the generator ensures a valid end char
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants