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.

Sign up for GitHub

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

Jump to bottom

feat : Add support to customize the developer account password (#2359) #4451

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat : Add support to customize the developer account password (#2359) #4451

wants to merge 2 commits into from

Conversation

rohanKanojia
Copy link
Contributor

@rohanKanojia rohanKanojia commented Nov 11, 2024 •
edited
Loading

Fixes: Issue #2539

Relates to: Issue #2539

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change
  • Chore (non-breaking change which doesn't affect codebase;
    test, version modification, documentation, etc.)

Checklist

  • I have read the contributing guidelines
  • My code follows the style guidelines of this project
  • I Keep It Small and Simple: The smaller the PR is, the easier it is to review and have it merged
  • I use conventional commits in my commit messages
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I tested my code on specified platforms
    • Linux
    • Windows
    • MacOS

Solution/Idea

  • Generate developer password instead of hardcoded developer string as password
  • Add config option to set developer password for cluster developer-password

Proposed changes

crc would no longer have developer as user password. It would be generated each time cluster is created (just like kubeadmin password). If user wants to override it, they can use developer-password configuration option in CRC config.

Testing

  • After starting CRC cluster user would see a random string as password value instead of `developer
  • If user has specified developer-password configuration option in CRC config, then that value is used in developer password:
    $ crc config set developer-password mypassword
    $ crc start
    [...]
    INFO Adding crc-admin and crc-developer contexts to kubeconfig...
    Started the OpenShift cluster.
    
    The server is accessible via web console at:
      https://console-openshift-console.apps-crc.testing
    
    Log in as user:
      Username: developer
      Password: mypassword

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Nov 11, 2024

Hi @rohanKanojia. Thanks for your PR.

I'm waiting for a crc-org member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@rohanKanojia rohanKanojia force-pushed the pr/issue2539 branch 4 times, most recently from 6e72835 to 2246b1e Compare November 12, 2024 15:52
@rohanKanojia rohanKanojia marked this pull request as ready for review November 13, 2024 04:08
@openshift-ci openshift-ci bot requested review from cfergeau and gbraad November 13, 2024 04:08
@anjannath
Copy link
Member

/ok-to-test

@rohanKanojia rohanKanojia force-pushed the pr/issue2539 branch 2 times, most recently from 1d67db9 to bf8050f Compare November 18, 2024 08:22
@praveenkumar
Copy link
Member

looks like prow/security is not liking this https://prow.ci.openshift.org/job-history/gs/test-platform-results/pr-logs/directory/pull-ci-crc-org-crc-main-security (recent jobs are green but failed for this)

@rohanKanojia
Copy link
Contributor Author

@praveenkumar : How can I reproduce this failure locally? I'm not able to get much idea about what's wrong by looking at logs:

{"component":"entrypoint","error":"wrapped process failed: exit status 1","file":"sigs.k8s.io/prow/pkg/entrypoint/run.go:84","func":"sigs.k8s.io/prow/pkg/entrypoint.Options.internalRun","level":"error","msg":"Error executing test process","severity":"error","time":"2024-11-18T08:45:42Z"}
ERRO[2024-11-18T08:45:43Z] Some steps failed:                           
ERRO[2024-11-18T08:45:43Z] 
  * could not run steps: step security failed: "security" test steps failed: "security" pod "security-openshift-ci-security-snyk-scan" failed: could not watch pod: the pod ci-op-m0wwywcn/security-openshift-ci-security-snyk-scan failed after 44s (failed containers: test): ContainerFailed one or more containers exited

Could it be possible that it's an intermittent failure?

@cfergeau
Copy link
Contributor

The failure is

[Medium] Use of Hardcoded Credentials
   ID: 3373e961-2410-402e-8db9-9c0b595d4e62 
   Path: pkg/crc/config/settings.go, line 32 
   Info: Do not hardcode passwords in code. Found hardcoded saved in DeveloperPassword.

which corresponds to

	DeveloperPassword        = "developer-password"

I think @albfan did some work on snyk's false positives recently (?)

@rohanKanojia
Copy link
Contributor Author

@cfergeau: Thanks for checking. We discussed this in a crc internal Slack chat and decided to add an exception for this particular rule.

@praveenkumar
Copy link
Member

Without this PR the developer user have developer password and it doesn't have a way to change it. There are users who might be using this as scripted way and consuming this well known password. Now with this change either user before hand set the required password or they will get a random password, this looks like a UX change. I think what we should do is have default password set to be developer but provide option to user to change if they want this way the UX is not going to change and existing scripts/automation wouldn't fail. @crc-org/crc-team wdyt?

@rohanKanojia
Copy link
Contributor Author

@praveenkumar : Maybe we can add a configuration named useLegacyDeveloperPassword that would be true by default.

@praveenkumar
Copy link
Member

useLegacyDeveloperPassword

@rohanKanojia I don't think it is good to have multiple config option just for developer user.

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Dec 13, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign cfergeau for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cfergeau
cfergeau reviewed Dec 20, 2024
View reviewed changes
pkg/crc/cluster/kubeadmin_password.go Outdated
kubeAdminPasswordFile := constants.GetKubeAdminPasswordPath()
// GenerateUserPassword creates and put updated password to ~/.crc/machine/crc/ directory
func GenerateUserPassword(passwordFile string, user string) error {
logging.Infof("Generating new password for the %s user", user)
kubeAdminPassword, err := GenerateRandomPasswordHash(23)
Copy link
Contributor

Choose a reason for hiding this comment

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

This could now be named password or such, this is no longer necessarily a kubeadmin password.

pkg/crc/cluster/kubeadmin_password.go Outdated

kubeAdminPassword, err := GetKubeadminPassword()
// UpdateUserPassword updates the htpasswd secret
func UpdateUserPassword(ctx context.Context, ocConfig oc.Config, newKubeAdminPassword string, newDeveloperPassword string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd use the plural here as there are multiple passwords involved UpdateUserPasswords

pkg/crc/cluster/kubeadmin_password.go Outdated
kubeAdminPassword, err := GetKubeadminPassword()
// UpdateUserPassword updates the htpasswd secret
func UpdateUserPassword(ctx context.Context, ocConfig oc.Config, newKubeAdminPassword string, newDeveloperPassword string) error {
credentials, err := resolveUserPassword(newKubeAdminPassword, newDeveloperPassword, constants.GetKubeAdminPasswordPath(), constants.GetDeveloperPasswordPath())
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, resolveUserPasswords

pkg/crc/cluster/kubeadmin_password.go
@@ -50,7 +50,7 @@ func UpdateUserPassword(ctx context.Context, ocConfig oc.Config, newKubeAdminPas
return nil
}

logging.Infof("Changing the password for the kubeadmin user")
logging.Infof("Changing the password for the users")
expected, err := getHtpasswd(credentials, externals)
Copy link
Contributor

Choose a reason for hiding this comment

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

Did this belong in the previous commit?

pkg/crc/cluster/kubeadmin_password.go
@@ -60,7 +60,7 @@ func UpdateUserPassword(ctx context.Context, ocConfig oc.Config, newKubeAdminPas
"-n", "openshift-config", "--type", "merge"}
_, stderr, err = ocConfig.RunOcCommandPrivate(cmdArgs...)
if err != nil {
return fmt.Errorf("Failed to update kubeadmin password %v: %s", err, stderr)
return fmt.Errorf("failed to update user passwords %v: %s", err, stderr)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question?

pkg/crc/config/settings.go Outdated
@@ -134,6 +135,8 @@ func RegisterSettings(cfg *Config) {

cfg.AddSetting(KubeAdminPassword, "", validateString, SuccessfullyApplied,
"User defined kubeadmin password")
cfg.AddSetting(DeveloperPassword, "", validateString, SuccessfullyApplied,
Copy link
Contributor

Choose a reason for hiding this comment

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

What if we use "developer" here as the default value (second parameter to AddSetting), wouldn't this make the second commit redundant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was added in second commit: e9a11fc

Did you mean we can avoid third commit by adding developer as default value?

@rohanKanojia
feat : Add config option to set developer password for cluster (crc-o…
04d07f4 
…rg#2539)

Add option to set developer password for the user.

```
$ crc config set developer-password mypassword
$ crc start
[...]
INFO Adding crc-admin and crc-developer contexts to kubeconfig...
Started the OpenShift cluster.

The server is accessible via web console at:
  https://console-openshift-console.apps-crc.testing

Log in as user:
  Username: developer
  Password: mypassword
```

Signed-off-by: Rohan Kumar <[email protected]>
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Dec 21, 2024

@rohanKanojia: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/security 04d07f4 link false /test security
ci/prow/integration-crc 04d07f4 link true /test integration-crc
ci/prow/e2e-crc 04d07f4 link true /test e2e-crc

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cfergeau cfergeau cfergeau left review comments

@gbraad gbraad Awaiting requested review from gbraad

Assignees
No one assigned
Labels
ok-to-test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rohanKanojia @anjannath @praveenkumar @cfergeau