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

Allow pre-definition of user passwords #44

Merged
merged 6 commits into from
Mar 12, 2021
Merged

Allow pre-definition of user passwords #44

merged 6 commits into from
Mar 12, 2021

Conversation

coro
Copy link
Contributor

@coro coro commented Mar 8, 2021

This PR allows a user to specify a Secret which contains a password to set on the User. The controller looks for the username & password under the keys username & password in the Secret. This Secret must be in the same namespace as the User, in order to mitigate privilege escalation.

Note that this import only happens upon creation - once the User is created, the import Secret is ignored. A user must be deleted & recreated in order to change a password or username until #35 is played.

This closes #36

Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed

Summary Of Changes

This PR adds several new fields to allow import of passwords from a created Secret:

apiVersion: v1
kind: Secret
metadata:
  name: credentials-secret
  namespace: testing
type: Opaque
data:
  username: bXktc2VjcmV0LXVzZXI=
  password: d2h5YXJleW91bG9va2luZ2hlcmU=
---
apiVersion: rabbitmq.com/v1alpha1
kind: User
metadata:
  name: import-user
  namespace: testing
spec:
  rabbitmqClusterReference:
    name: rmq
    namespace: rabbitmq-system
  importCredentialsSecret:
    name: credentials-secret

coro added 3 commits March 8, 2021 17:52
@coro coro marked this pull request as ready for review March 9, 2021 14:31
@coro
Copy link
Contributor Author

coro commented Mar 9, 2021

cc @n3wscott as I believe this was one of the knative requirements.

Copy link
Contributor

@ChunyiLyu ChunyiLyu left a comment

Choose a reason for hiding this comment

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

What we have discussed:

  1. Import password and username from secret
  2. Delete user.spec.name; if name is not present and no import secret provided, randomly generates username and password pair
  3. two example manifests about how to create users: randomly generated username/password and pre defined password and username in docs/examples/users

api/v1alpha1/user_types.go Outdated Show resolved Hide resolved
system_tests/user_system_tests.go Outdated Show resolved Hide resolved
@Zerpet Zerpet self-requested a review March 11, 2021 17:29
Copy link
Contributor

@ChunyiLyu ChunyiLyu left a comment

Choose a reason for hiding this comment

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

LGTM. There are several places in the controller which a failure was only returned but not logged or recorded as event. But I want to approve the PR so we can merge it soon and avoid merge conflicts. Logging and publish events for all errors can come in a different PR.

@coro coro merged commit cbab30b into main Mar 12, 2021
@coro coro deleted the import-password branch March 12, 2021 11:31
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.

Allow pre-defined passwords to be used for created User objects
3 participants