fix: Weave GitOps username is hard-coded #2086

yitsushi · 2022-05-04T09:58:26Z

Create addition RoleBinding and ClusterRoleBinding based on the
username, and keep the original wego-admin there to maintain backward
compatibility.

DevMode with Tilt still uses wego-admin as a hard-coded user,
potentially we can even remove the DevUser maybe.

DevMode is used only to set DevUser on HMACTokenSignerVerifier and
the user is used only return with a static claim on Verify().

Additional changes:

Split up the admin-user.yaml into logical parts like
- Credentials as they will be created only once.
- Role and ClustreRole as they will be created only once
- Two files for RoleBinding and ClusterRoleBinding (as described above)

Fixes #2001

yitsushi · 2022-05-04T09:58:32Z

ozamosi

Changes to the chart will (just like documentation) be deployed straight to our end users. I believe this change would break all existing users, hence my change request.

Additionally, when you change the chart, you need to change the chart version, or the deploy becomes weird and we create 2 different "editions" of the same chart version.

yitsushi · 2022-05-04T10:14:15Z

Additionally, when you change the chart, you need to change the chart version, or the deploy becomes weird and we create 2 different "editions" of the same chart version.

I know that part, first I wanted some review if the approach is acceptable.

Changes to the chart will (just like documentation) be deployed straight to our end users. I believe this change would break all existing users, hence my change request.

When the Helm Chart is applied, the RoleBinding will be changed. I don't see why would it break. If they log in with an adminUser, the username/password pair will not change, the underlying RoleBinding will change, but the change will just use the same value as defined in adminUser (the one they use to log in), the only requirement would be to restart the server pod, but that will happen because of the code (and with that the container image) changes.

yitsushi · 2022-05-04T10:15:37Z

Tested workflow:

Used main while for dev-cluster
Log in as dev/dev and it shows wego-admin
Switched to this branch
Wait...
Log in as dev/dev and it shows dev and I see the same content

ozamosi · 2022-05-04T10:16:13Z

but that will happen because of the code (and with that the container image) changes.

No - merging this PR won't deploy core in any customer cluster. It will deploy the role binding. We can only do the chart change as part of a release.

yitsushi · 2022-05-04T10:17:14Z

No - merging this PR won't deploy core in any customer cluster. It will deploy the role binding. We can only do the chart change as part of a release.

That makes sense. So this can be merged only before we cut a new release?

Unfortunately I can't split it up code and chart changes. At least I don't see how can I do that.

Callisto13 · 2022-05-04T10:18:07Z

dejavu

I think this is something we do not want. It was a deliberate choice to have the k8s object User to always be called wego admin. I had a PR very similar to this a while ago (cannot find it anywhere) and was told that we do not want this. Basically the argument was that if we let it be configurable to this level, people can create any number of static users and we don't want to become an identity provider.
So basically, even though it feels "broken" to you and me, there is only ever one "user", regardless of what people think they are setting.

yitsushi · 2022-05-04T10:19:28Z

Closing this PR based on the discussion above.

Callisto13 · 2022-05-04T10:21:35Z

the fact that we keep having these discussions, because it looks so weird to use and to implement, is... illuminating 😁

Callisto13 · 2022-05-04T10:25:24Z

This is the closest thing to a "summary issue" i can find. It has all the links to various discussions around this. I may write it up fully so we have an easy link in future for when this inevitably comes up again #1787

ozamosi · 2022-05-04T10:29:07Z

Unfortunately I can't split it up code and chart changes. At least I don't see how can I do that.

Add 2 users right now, and add a ticket to remove the old one after the next release, I suppose.

for when this inevitably comes up again

Given this has been reported about one time for every person who's tried to use this, could we just fix it instead? "We don't want this" isn't accurate - we clearly all do.

Callisto13 · 2022-05-04T10:35:31Z

you'd have to take it up with @davidstauffer

yitsushi · 2022-05-04T11:00:02Z

Given this has been reported about one time for every person who's tried to use this, could we just fix it instead? "We don't want this" isn't accurate - we clearly all do.

To be fair it's kind of documented:

Maybe we just have to highlight the fact that the username + password pair is only for authentication, the logged in user will be wego-admin.

yitsushi · 2022-05-04T13:47:47Z

if we let it be configurable to this level, people can create any number of static users and we don't want to become an identity provider.

Technically with this approach they still can have only one user, because we read the user/pass from a single and hard-coded Secret, so no way they can create more than one user without changing the mechanism used to fetch auth credentials from Secret.

JamWils · 2022-05-04T14:28:47Z

Alright, I think we should reopen this since it is a topic that keeps coming up.

Let's not break existing users so do what needs to be done.
The name can be configurable via helm chart values and it defaults to wego-admin.
This is the only role binding we provide in the chart. This is effectively our one user. We can't stop users from setting up more users then there already are, but that is on them. They could do it now if they wanted to by uploading the correct yaml along side the helm release.

yitsushi · 2022-05-04T15:39:46Z

Why the HelmChart test fails... updated the chart version :/

ozamosi

🎉

Not sure about the failed check - I added it, and it's clearly not doing what it should, so I need to debug it. I'm happy for you to ignore it and merge regardless.

ozamosi · 2022-05-04T16:07:47Z

Fixed the chart check in #2094

Fixes #2001

username, and keep the original `wego-admin` there to maintain backward compatibility. `DevMode` with Tilt still uses `wego-admin` as a hard-coded user, potentially we can even remove the `DevUser` maybe. `DevMode` is used only to set `DevUser` on `HMACTokenSignerVerifier` and the user is used only return with a static claim on `Verify()`. Additional changes: * Split up the admin-user.yaml into logical parts like * Credentials as they will be created only once. * Role and ClustreRole as they will be created only once * Two files for RoleBinding and ClusterRoleBinding (as described above)

Callisto13 · 2022-05-05T08:52:07Z

They could do it now if they wanted to by uploading the correct yaml along side the helm release.

Should we add something in docs (in another pr) that if ppl want to add more users, it is at their own risk and we are doing (will always do) the bare minimum when it comes to being an identity provider, and that they should def use oidc?

yitsushi · 2022-05-05T11:45:02Z

They could do it now if they wanted to by uploading the correct yaml along side the helm release.

How can they add more users?

The code behind the scene will be still the same and it will check only the cluster-user-auth secret. I don't see any options to tell the system "use that secret too".

Right now I think the only way people can use multiple users is OIDC.

they can create Roles and RoleBindings, but login user will be only one without OIDC.

yitsushi added the bug Something isn't working label May 4, 2022

yitsushi requested review from rokshana-b and Callisto13 May 4, 2022 09:59

ozamosi suggested changes May 4, 2022

View reviewed changes

yitsushi closed this May 4, 2022

yitsushi mentioned this pull request May 4, 2022

Weave GitOps username is hard-coded #2001

Closed

10 tasks

yitsushi reopened this May 4, 2022

yitsushi marked this pull request as draft May 4, 2022 14:46

yitsushi mentioned this pull request May 4, 2022

feat: Show the same username on the UI as the user logged in #2087

Closed

yitsushi requested a review from ozamosi May 4, 2022 15:34

yitsushi marked this pull request as ready for review May 4, 2022 15:39

ozamosi approved these changes May 4, 2022

View reviewed changes

yitsushi added 2 commits May 5, 2022 08:05

fix: Weave GitOps username is hard-coded

707145c

Fixes #2001

bump chart version

0e462dd

yitsushi force-pushed the 2001-username-on-the-ui branch from 5f753e5 to 6b9fb36 Compare May 5, 2022 08:05

yitsushi merged commit 53d4bbd into main May 5, 2022

foot mentioned this pull request May 6, 2022

Update the cluster-auth-secret docs username, and login instructions, to wego-admin weaveworks/weave-gitops-enterprise#706

Closed

This was referenced May 12, 2022

Updates for 0.0.0-this-is-a-test-please-ignore #2151

Closed

Updates for 0.8.1-rc.3 #2158

Merged

Updates for 0.8.1-rc.4 #2169

Merged

yitsushi mentioned this pull request May 19, 2022

Publish helm charts only when the chart version is changed, rather than every time the chart is changed #2180

Closed

This was referenced May 26, 2022

Updates for 0.8.1-rc.5 #2213

Merged

Updates for 0.8.1-rc.6 #2248

Merged

Updates for 0.8.1-rc.7 #2258

Merged

Updates for 0.8.1 #2262

Merged

weave-gitops-bot mentioned this pull request Jun 8, 2022

Updates for 0.9.0-rc.1 #2291

Merged

makkes deleted the 2001-username-on-the-ui branch December 9, 2022 09:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Weave GitOps username is hard-coded #2086

fix: Weave GitOps username is hard-coded #2086

yitsushi commented May 4, 2022 •

edited

Loading

yitsushi commented May 4, 2022 •

edited

Loading

ozamosi left a comment

yitsushi commented May 4, 2022

yitsushi commented May 4, 2022

ozamosi commented May 4, 2022

yitsushi commented May 4, 2022 •

edited

Loading

Callisto13 commented May 4, 2022 •

edited

Loading

yitsushi commented May 4, 2022

Callisto13 commented May 4, 2022 •

edited

Loading

Callisto13 commented May 4, 2022 •

edited

Loading

ozamosi commented May 4, 2022

Callisto13 commented May 4, 2022

yitsushi commented May 4, 2022

yitsushi commented May 4, 2022

JamWils commented May 4, 2022

yitsushi commented May 4, 2022 •

edited

Loading

ozamosi left a comment

ozamosi commented May 4, 2022

Callisto13 commented May 5, 2022

yitsushi commented May 5, 2022 •

edited

Loading

fix: Weave GitOps username is hard-coded #2086

fix: Weave GitOps username is hard-coded #2086

Conversation

yitsushi commented May 4, 2022 • edited Loading

yitsushi commented May 4, 2022 • edited Loading

ozamosi left a comment

Choose a reason for hiding this comment

yitsushi commented May 4, 2022

yitsushi commented May 4, 2022

ozamosi commented May 4, 2022

yitsushi commented May 4, 2022 • edited Loading

Callisto13 commented May 4, 2022 • edited Loading

yitsushi commented May 4, 2022

Callisto13 commented May 4, 2022 • edited Loading

Callisto13 commented May 4, 2022 • edited Loading

ozamosi commented May 4, 2022

Callisto13 commented May 4, 2022

yitsushi commented May 4, 2022

yitsushi commented May 4, 2022

JamWils commented May 4, 2022

yitsushi commented May 4, 2022 • edited Loading

ozamosi left a comment

Choose a reason for hiding this comment

ozamosi commented May 4, 2022

Callisto13 commented May 5, 2022

yitsushi commented May 5, 2022 • edited Loading

yitsushi commented May 4, 2022 •

edited

Loading

yitsushi commented May 4, 2022 •

edited

Loading

yitsushi commented May 4, 2022 •

edited

Loading

Callisto13 commented May 4, 2022 •

edited

Loading

Callisto13 commented May 4, 2022 •

edited

Loading

Callisto13 commented May 4, 2022 •

edited

Loading

yitsushi commented May 4, 2022 •

edited

Loading

yitsushi commented May 5, 2022 •

edited

Loading