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

[Enhancement]: Enable root password configuration at startup #33058

Closed
1 task done
timheuer opened this issue May 14, 2024 · 37 comments
Closed
1 task done

[Enhancement]: Enable root password configuration at startup #33058

timheuer opened this issue May 14, 2024 · 37 comments
Labels
kind/enhancement Issues or changes related to enhancement stale indicates no udpates for 30 days

Comments

@timheuer
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

What would you like to be added?

When using standalone, it would be helpful to set the root password to something other than default using an environment value and/or config. like MILVUS_ROOT_PASSWORD. This would enable the standalone container to start in a more secure manner and configurable, rather than having to start a Milvus instance, then have a separate client use the default admin password, and update itself. Saving a step.

Why is this needed?

The default password for root is well documented. Not having a way to change this in config for startup of the standalone leaves an opening for even authenticated commands to just guess at the default (it's the equivalent of admin/admin). Other containers I've worked with enable setting the root password as a configurable aspect in environment variables so that it is secure at startup.

Anything else?

No response

@timheuer timheuer added the kind/enhancement Issues or changes related to enhancement label May 14, 2024
@ckrapu-nv
Copy link

The suggestion from @timheuer would also reduce the frequency of questions like issues/33495.

@xiaofan-luan
Copy link
Collaborator

put password in config or file seems to be a very dangerous behaviour.
Any best practice on a k8s cluster?

@timheuer
Copy link
Author

timheuer commented Jun 4, 2024

A few other db providers do this. Environment variables over file-based configs

Copy link

stale bot commented Jul 7, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
Rotten issues close after 30d of inactivity. Reopen the issue with /reopen.

@stale stale bot added the stale indicates no udpates for 30 days label Jul 7, 2024
@timheuer
Copy link
Author

timheuer commented Jul 7, 2024

Any chance of considering a design here?

@eerhardt
Copy link

Is the Milvus docker container supposed to be used in a production environment? If so, what are the best practices for locking it down so only authorized apps can make requests?

@stale stale bot removed the stale indicates no udpates for 30 days label Jul 15, 2024
@timheuer
Copy link
Author

The docs only seem to hint at 'change the password' as a solution, but this isn't ideal either versus having a config-based setup that is not a globally-known default. We'd prefer a design consistent with other container runtimes/apps that follow patterns where an ENV variable can be set with a password that upon startup the container knows to use that instead of a default, if set. Something like COMMON_SECURITY_ROOT_TOKEN. For example Qdrant, which uses an API key approach, enables defining the API key in an environment variable and clients need that to connect, but there is no default that is known. It is set at the container ENV level and used upon startup.

@xiaofan-luan
Copy link
Collaborator

@timheuer
Agreed that use ENV or configs is for initial setup is an option.

I thought we just need to change a little bit code here

the default username password is a constant at
constant.go
UserRoot = "root"
DefaultRootPassword = "Milvus"

we need to change it to component_param.go as a config similar to AuthorizationEnabled

anyone want to take this feature?

@timheuer
Copy link
Author

@xiaofan-luan Not a Go developer so the repo is not entirely intuitive to me and I've had onramp issues already with the devcontainer, but with your pointers, is it as simple as this? (of course no tests changed)?

timheuer@98ce4cb

Is anyone on core team able to make the change to ensure better security for the container?

@xiaofan-luan
Copy link
Collaborator

@SimFG could you have on this?

@SimFG
Copy link
Contributor

SimFG commented Jul 17, 2024

@timheuer I looked at your commit and it basically meets the requirements. I will modify it slightly and submit a PR.

@xiaofan-luan
Copy link
Collaborator

@timheuer I looked at your commit and it basically meets the requirements. I will modify it slightly and submit a PR.

You are quick!

sre-ci-robot pushed a commit that referenced this issue Jul 17, 2024
@timheuer
Copy link
Author

Amazing thank you @xiaofan-luan and @SimFG! How long from a merge to when it shows up in a docker image we can start consuming. And I assume the pattern to follow of setting an env to COMMON_SECURITY_DEFAULTROOTPASSWORD would set it (as well in yaml). I'd love to give it a spin on a docker image and make the necessary changes then in our platform.

@SimFG
Copy link
Contributor

SimFG commented Jul 18, 2024

@timheuer The PR has been merged into the master branch, and will be picked to the 2.4 branch today. What version of milvus are you using now? If you want to include this PR, you can only use the latest master dev image.

sre-ci-robot pushed a commit that referenced this issue Jul 18, 2024
default root password
- issue: #33058
- pr: #34752

set log level
- issue: #34756
- pr: #34757

---------

Signed-off-by: SimFG <[email protected]>
@timheuer
Copy link
Author

Thanks @SimFG I confirmed functionality with master-20240718-c8bf6c8a-amd64 setting env variable COMMON_SECURITY_DEFAULTROOTPASSWORD and works. When might we expect this to get into 2.4-latest or later (as it looks like that is days old and wouldn't have this)?

@SimFG
Copy link
Contributor

SimFG commented Jul 19, 2024

@timheuer We should release version 2.4.7 next week, which will have this improvement

@timheuer
Copy link
Author

Thanks @SimFG! Can you help me understand your docker tagging though? 2.4-latest I would have assumed is the same as at least v2.4.6, but the hashes are different (and release date of 2.4-latest is a week older than v2.4.6)
2.4-latest: https://hub.docker.com/layers/milvusdb/milvus/2.4-latest/images/sha256-a3d2832d6980b01899dde756dab6f97171dc5d16b2f8d3dda2629b4ff95ad696?context=explore
v2.4.6: https://hub.docker.com/layers/milvusdb/milvus/v2.4.6/images/sha256-dd31ca45f5faeb5cbd665ca72a360367a542d4800d23377eeb06f7620c351017?context=explore

@xiaofan-luan
Copy link
Collaborator

@SimFG
please also modify the document as well

@SimFG
Copy link
Contributor

SimFG commented Jul 22, 2024

@timheuer 2.4-latest is expected to be the dev image of the latest 2.4 branch, but due to some problems with the internal pipeline, there may be some problems with the update. Version 2.4.6 is our official version for the public

@timheuer
Copy link
Author

Thanks for update @SimFG -- you mentioned that 2.4-latest is a 'dev image' -- I kinda expected it to be a semver production image of 2.4.6? is that not a correct understanding? Is there a better doc that describes the tags? We've been trying to snap to major.minor with our dependencies and ideally would follow major.minor-latest tagging.

@SimFG
Copy link
Contributor

SimFG commented Jul 24, 2024

@timheuer no, the 2.4-latest image is not recommended to use, it has not been fully tested and is only used for internal testing

@timheuer
Copy link
Author

Thanks @SimFG it sounds like if we want to snap to validated images we'll have to use major.minor.revision tagged docker images then as a best practice?

@SimFG
Copy link
Contributor

SimFG commented Jul 25, 2024

@timheuer
Copy link
Author

Thanks @SimFG I'll await the 2.4.7 official image to make our product change.

I'd ask that you may reconsider container tagging here. By definition it would seem odd that 2.3-latest is not meant for production when it has "latest" tagging which is fairly standard for meaning, well, the latest. Having something that is tagged v2.4.7 and that does not match 2.4-latest seems odd as well. I'd encourage re-thinking the release/tag strategy to adhere to more common expectations of semver + tagging as I would expect anything -latest to include the revision latest of the major.minor. If something is meant for dev-only, then using -dev-latest or something would be better as well. Thanks for considering.

@SimFG
Copy link
Contributor

SimFG commented Jul 30, 2024

@timheuer I quite agree with your view. @xiaofan-luan what do you think?

@xiaofan-luan
Copy link
Collaborator

For milvus, every 2.4-branch/2.3-brnch is ready to production.

but we recommend to use release tag in production because for each of the release QA team will run a full regression and we don't have guarantee on branches.

Generally speaking, we conurrently maintain multiple developing branch and only tags is the actaul release version. As you said, 2.4 branch is 2.4-dev-latest actually

@timheuer
Copy link
Author

timheuer commented Aug 2, 2024

So @xiaofan-luan if I understand you correctly, there is not a 'release' docker image that is semver-compliant. You can't grap a release 2.x image and expect it to float to any 2.x.y updates. And your policy is that consumers should take explicit release versions (2.x.y)?

@xiaofan-luan
Copy link
Collaborator

Exactly. The 2.4 branch functions as the development branch for 2.4. We continuously merge bug fixes and small features into this branch. Once we believe it has reached a stable state and includes all intended features, the maintainer team will create a 2.x.y tag. This tagged version will then be released using all the deployment methods.

Mean time, master works as the dev branch of 2.5. Once 2.5.0 released, master moved to 2.6 latest dev and 2.5 branch becomes active maintained branch.

We maintain 3 different branch at the same:
2.3 - mainly critical bugfix, stable release
2.4 - small features, active bug fixes, stable release with latest features
master - all patches included, 2.5 release candidate

@timheuer
Copy link
Author

timheuer commented Aug 5, 2024

Thank you for the explanation @xiaofan-luan. I think I'm mostly concerned about the tagged Docker images on Docker Hub and not about the branching strategy. I know that it would appear they are tied 1:1, but as a consumer of the Docker images, I'm not concerned myself with source-branch strategy.

My main feedback is there is a tagged Docker image that is called 2.4-latest and there is also a tagged Docker image with 2.4.6 that is NOT a part of the 2.4-latest (the hashes don't match). So to the consumer, something labeled 'latest' is indeed, not matching the latest. This doesn't match other Docker image tagging strategies hence my suggestion that perhaps you consider that and follow SemVer tagging.

I now understand your method as of now though and will NOT use -latest images and instead we'll have to snap to explicit major.minor.revision images which will only get updated when we ship release updates ourselves, rather than follow major.minor.

@xiaofan-luan
Copy link
Collaborator

2.4 latest is the daily build image that mainly for test purpose and user who need instant fix

@timheuer
Copy link
Author

timheuer commented Aug 6, 2024

I understand now @xiaofan-luan I just disagree with the tagging strategy based on how others in the Docker ecosystem do it. Per this strategy there is no production image that is major.minor I can rely on and continue to get production updates. I had incorrectly assumed that latest was a production latest tag, and it is not. I'd again ask you to reconsider your tagging strategy, but at least I understand it now that there is no major.minor production images.

Using RabbitMQ (https://hub.docker.com/_/rabbitmq/tags?page=&page_size=&ordering=&name=3.13) as another example,
latest == 3.13 == 3.13.6

I'll await the official 2.4.7 image that we can change to and snap to that as new minimum for us.

@Alirexaa
Copy link

Alirexaa commented Aug 8, 2024

@timheuer, 2.4.7 image released. I will open an issue in Aspire for this.

@timheuer
Copy link
Author

timheuer commented Aug 8, 2024

Nice. Verified it works for me. @SimFG seems good to close this I think then.

@ycyin
Copy link

ycyin commented Aug 28, 2024

On initial startup, the environment variable COMMON_SECURITY_DEFAULTROOTPASSWORD works for me. But if already have data (the /var/lib/milvus directory is not empty), how to change the password of the root account, and is there any other design for this?

@xiaofan-luan
Copy link
Collaborator

you can use modify password to update root password

@ycyin
Copy link

ycyin commented Aug 29, 2024

you can use modify password to update root password

I only found a way to modify it through the RPC interface, and now there is no way to modify it through the command line?
Now: https://milvus.io/docs/rbac.md

client.update_password(
    user_name='user_1',
    old_password='P@ssw0rd',
    new_password='P@ssw0rd123'
)

Expect to have a command-line tool, like 'delete user`:

update user ...

I commited an issue in the CLI repository,zilliztech/milvus_cli#87

Copy link

stale bot commented Sep 29, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
Rotten issues close after 30d of inactivity. Reopen the issue with /reopen.

@stale stale bot added the stale indicates no udpates for 30 days label Sep 29, 2024
@stale stale bot closed this as completed Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Issues or changes related to enhancement stale indicates no udpates for 30 days
Projects
None yet
Development

No branches or pull requests

7 participants