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

issue 2117: disallow admin username #2378

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

bo17age
Copy link

@bo17age bo17age commented May 30, 2024

disallow admin username; use fallback username.
Related issue: #2117

@jandubois
Copy link
Member

This is unfortunately not going to be as simple as changing the regex. The changed default setting must be made conditional on the version of Lima that creates the VM, otherwise you are going to break existing instances. #2107 introduces code that will make this possible.

I'm also wondering if we special-case admin (purely for the benefit of Ubuntu), if we should disallow other well-known Linux user and group names as well? Random list: root, bin, adm, daemon, games, postmaster, guest, nobody, ntp, but there are plenty more. And then there are lots of common group names too.

@bo17age
Copy link
Author

bo17age commented May 31, 2024

This is unfortunately not going to be as simple as changing the regex. The changed default setting must be made conditional on the version of Lima that creates the VM, otherwise you are going to break existing instances. #2107 introduces code that will make this possible.

I'm also wondering if we special-case admin (purely for the benefit of Ubuntu), if we should disallow other well-known Linux user and group names as well? Random list: root, bin, adm, daemon, games, postmaster, guest, nobody, ntp, but there are plenty more. And then there are lots of common group names too.

Hi @jandubois ,
Thank you for your comment.
assume that the current user is admin, then they can not have any instance running.
For other Linux user and group, we can add up if there is an issue.

@jandubois
Copy link
Member

assume that the current user is admin, then they can not have any instance running.

This is not correct; only Ubuntu has a predefined admin group that conflicts; other templates run fine with an admin user.

@bo17age
Copy link
Author

bo17age commented May 31, 2024

assume that the current user is admin, then they can not have any instance running.

This is not correct; only Ubuntu has a predefined admin group that conflicts; other templates run fine with an admin user.

Thank you, I understand now.

@bo17age bo17age force-pushed the bugfix/issue-2117-username-admin-waiting-for-the-essential-requirement branch from a9b5a89 to b50caef Compare September 6, 2024 02:22
Signed-off-by: Linh Luong <[email protected]>
Signed-off-by: bo17age <[email protected]>
Signed-off-by: Linh Luong <[email protected]>
Signed-off-by: bo17age <[email protected]>
Signed-off-by: Linh Luong <[email protected]>
Signed-off-by: bo17age <[email protected]>
Signed-off-by: Linh Luong <[email protected]>
Signed-off-by: bo17age <[email protected]>
Signed-off-by: bo17age <[email protected]>
Signed-off-by: bo17age <[email protected]>
@bo17age bo17age force-pushed the bugfix/issue-2117-username-admin-waiting-for-the-essential-requirement branch from bff9ee3 to d5ab3b1 Compare September 6, 2024 04:17
@bo17age
Copy link
Author

bo17age commented Sep 6, 2024

Hi @jandubois, please help to review this PR again.

@jandubois
Copy link
Member

Hi @bo17age,

My thoughts around how this should be handled have evolved since you originally opened this PR. I thought you had abandoned it, which is why I didn't write them down earlier.

I think we should make the username configurable in lima.yaml:

user:
  name: null

And then FillDefaults should fill in the default user name according to our rules (set user.name to "lima" if the local user name doesn't look like a valid Linux user name). This is also where the check for the admin user would move.

If the lima.yaml already specifies a user name, then we accept it without further validation (so the user could force it to be admin because that would still work for non-Ubuntu images).

All other locations should use y.User.Name instead of calling osutil.LimaUser().

The user could then also add it to their override.yaml and always use lima or whatever as the user name and not use the host user name ever.

I want to use a nested user.name instead of a top-level username because we may want to allow the user to override other settings in the future, like the gid/uid. This will allow us to disable more Lima-specific configurations beyond what cloud-init does by default. @afbjorklund knows more about these requirements.

Note that I have not yet verified if all the locations that currently call osutil.LimaUser have easy access to the limayaml structure, so maybe this requires some additional refactoring.

Sorry for moving the goal-post at this late stage, but let me know if you are up for modifying your PR along those lines.

Alternatively I could review this PR as is, and then make the changes I described above in a follow-up PR myself.

@bo17age
Copy link
Author

bo17age commented Sep 12, 2024

Hi @jandubois ,
Thank you for your suggestion. Yes, I'm willing to make the change and will start soon.

@jandubois
Copy link
Member

Yes, I'm willing to make the change and will start soon.

Hi @bo17age,

Did you get a chance to work on this?

We are planning for a Lima 1.0 release later this month, or very early in November, and there will be some changes in default behaviour already. So it would be good to make the change for the admin user name at the same time.

Let me know if you think you cannot finish it by some time next week, and I can give it a try myself!

@jandubois
Copy link
Member

Let me know if you think you cannot finish it by some time next week, and I can give it a try myself!

Sorry, I meant: let me know if you can finish it yourself by next week, or if that is going to be too tight for you. In that case I can try to do it myself.

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.

2 participants