Repository: code-ready/crc
Pull request #2392 (2021-06-01 11:35:02)

Create VM auth files right after driver.Create()
crc console command is available if the VM exists. 
When running crc start, there is a time frame where the VM exists but the kubeadmin password doesn't exist in the instance directory.

Now, kubeadmin password, ssh key and kubeconfig files are written right after the VM is created (driver.Create())
Right after, there is a call to `api.SetExists()`, which unlock functions like crc console.

Also,
* Refactor kubeadmin password generation to be done at creation time only
* htpasswd is changed only if needed by comparing the htpasswd file in the cluster and the wanted passwords.


openshift-ci[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/code-ready/crc/pull/2392#" title="Author self-approved">guillaumerose</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=code-ready%2Fcrc).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/code-ready/crc/blob/master/OWNERS)~~ [guillaumerose]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
openshift-ci[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/code-ready/crc/pull/2392#pullrequestreview-677519627" title="Approved">cfergeau</a>*, *<a href="https://github.com/code-ready/crc/pull/2392#" title="Author self-approved">guillaumerose</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=code-ready%2Fcrc).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/code-ready/crc/blob/master/OWNERS)~~ [cfergeau,guillaumerose]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/pkg/crc/cluster/kubeadmin_password.go b/pkg/crc/cluster/kubeadmin_password.go
index ee8ea1e80..cc221e31a 100644
--- a/pkg/crc/cluster/kubeadmin_password.go
+++ b/pkg/crc/cluster/kubeadmin_password.go
@@ -1,6 +1,8 @@
 package cluster
 
 import (
+	"bufio"
+	"bytes"
 	"crypto/rand"
 	"encoding/base64"
 	"fmt"
@@ -11,49 +13,63 @@ import (
 	"github.com/code-ready/crc/pkg/crc/constants"
 	"github.com/code-ready/crc/pkg/crc/logging"
 	"github.com/code-ready/crc/pkg/crc/oc"
-	crcos "github.com/code-ready/crc/pkg/os"
 	"golang.org/x/crypto/bcrypt"
 )
 
-// UpdateKubeAdminUserPassword does following
-// - Create and put updated kubeadmin password to ~/.crc/machine/crc/kubeadmin-password
-// - Update the htpasswd secret
-func UpdateKubeAdminUserPassword(ocConfig oc.Config, kubeAdminPassword string) error {
-	var err error
+// GenerateKubeAdminUserPassword creates and put updated kubeadmin password to ~/.crc/machine/crc/kubeadmin-password
+func GenerateKubeAdminUserPassword() error {
+	logging.Infof("Generating new password for the kubeadmin user")
 	kubeAdminPasswordFile := constants.GetKubeAdminPasswordPath()
-	if crcos.FileExists(kubeAdminPasswordFile) {
-		logging.Debugf("kubeadmin password has already been updated")
-		return nil
+	kubeAdminPassword, err := GenerateRandomPasswordHash(23)
+	if err != nil {
+		return fmt.Errorf("Cannot generate the kubeadmin user password: %w", err)
 	}
+	return ioutil.WriteFile(kubeAdminPasswordFile, []byte(kubeAdminPassword), 0600)
+}
 
-	if kubeAdminPassword == "" {
-		logging.Infof("Generating new password for the kubeadmin user")
-		kubeAdminPassword, err = GenerateRandomPasswordHash(23)
-		if err != nil {
-			return fmt.Errorf("Cannot generate the kubeadmin user password: %w", err)
+// UpdateKubeAdminUserPassword updates the htpasswd secret
+func UpdateKubeAdminUserPassword(ocConfig oc.Config, newPassword string) error {
+	if newPassword != "" {
+		logging.Infof("Overriding password for kubeadmin user")
+		if err := ioutil.WriteFile(constants.GetKubeAdminPasswordPath(), []byte(strings.TrimSpace(newPassword)), 0600); err != nil {
+			return err
 		}
 	}
 
-	hashDeveloperPasswd, err := hashBcrypt("developer")
+	kubeAdminPassword, err := GetKubeadminPassword()
 	if err != nil {
-		return err
+		return fmt.Errorf("Cannot generate the kubeadmin user password: %w", err)
+	}
+	credentials := map[string]string{
+		"developer": "developer",
+		"kubeadmin": kubeAdminPassword,
 	}
 
-	hashKubeAdminPasswd, err := hashBcrypt(kubeAdminPassword)
+	given, _, err := ocConfig.RunOcCommandPrivate("get", "secret", "htpass-secret", "-n", "openshift-config", "-o", `jsonpath="{.data.htpasswd}"`)
+	if err != nil {
+		return err
+	}
+	ok, externals, err := compareHtpasswd(given, credentials)
 	if err != nil {
 		return err
 	}
-	base64Data := getBase64(hashDeveloperPasswd, hashKubeAdminPasswd)
+	if ok {
+		return nil
+	}
 
+	logging.Infof("Changing the password for the kubeadmin user")
+	expected, err := getHtpasswd(credentials, externals)
+	if err != nil {
+		return err
+	}
 	cmdArgs := []string{"patch", "secret", "htpass-secret", "-p",
-		fmt.Sprintf(`'{"data":{"htpasswd":"%s"}}'`, base64Data),
+		fmt.Sprintf(`'{"data":{"htpasswd":"%s"}}'`, expected),
 		"-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 ioutil.WriteFile(kubeAdminPasswordFile, []byte(kubeAdminPassword), 0600)
+	return nil
 }
 
 func GetKubeadminPassword() (string, error) {
@@ -110,7 +126,64 @@ func hashBcrypt(password string) (hash string, err error) {
 	return string(passwordBytes), nil
 }
 
-func getBase64(developerUserPassword, adminUserPassword string) string {
-	s := fmt.Sprintf("developer:%s\nkubeadmin:%s", developerUserPassword, adminUserPassword)
-	return base64.StdEncoding.EncodeToString([]byte(s))
+func getHtpasswd(credentials map[string]string, externals []string) (string, error) {
+	var ret []string
+	ret = append(ret, externals...)
+	for username, password := range credentials {
+		hash, err := hashBcrypt(password)
+		if err != nil {
+			return "", err
+		}
+		ret = append(ret, fmt.Sprintf("%s:%s", username, hash))
+	}
+	return base64.StdEncoding.EncodeToString([]byte(strings.Join(ret, "\n"))), nil
+}
+
+// source https://github.com/openshift/oauth-server/blob/04985077512fec241a5170074bf767c23592d7e7/pkg/authenticator/password/htpasswd/htpasswd.go
+func compareHtpasswd(given string, credentials map[string]string) (bool, []string, error) {
+	decoded, err := base64.StdEncoding.DecodeString(given)
+	if err != nil {
+		return false, nil, err
+	}
+	scanner := bufio.NewScanner(bytes.NewReader(decoded))
+
+	var externals []string
+	found := 0
+	for scanner.Scan() {
+		line := scanner.Text()
+		if len(line) == 0 {
+			continue
+		}
+		parts := strings.SplitN(line, ":", 2)
+		if len(parts) != 2 {
+			continue
+		}
+		username := parts[0]
+		password := parts[1]
+
+		if expectedPassword, ok := credentials[username]; ok {
+			ok, err := testBCryptPassword(expectedPassword, password)
+			if err != nil || !ok {
+				continue
+			}
+			found++
+		} else {
+			externals = append(externals, line)
+		}
+	}
+	if found != len(credentials) {
+		return false, externals, nil
+	}
+	return true, externals, nil
+}
+
+func testBCryptPassword(password, hash string) (bool, error) {
+	err := bcrypt.CompareHashAndPassword([]byte(hash), []byte(password))
+	if err == bcrypt.ErrMismatchedHashAndPassword {
+		return false, nil
+	}
+	if err != nil {
+		return false, err
+	}
+	return true, nil
 }
diff --git a/pkg/crc/machine/config/config.go b/pkg/crc/machine/config/config.go
index 57bcb7857..7bc423b01 100644
--- a/pkg/crc/machine/config/config.go
+++ b/pkg/crc/machine/config/config.go
@@ -14,6 +14,7 @@ type MachineConfig struct {
 	ImageSourcePath string
 	ImageFormat     string
 	SSHKeyPath      string
+	KubeConfig      string
 
 	// HyperKit specific configuration
 	KernelCmdLine string
diff --git a/pkg/crc/machine/start.go b/pkg/crc/machine/start.go
index c8b3be9f0..8c9c33177 100644
--- a/pkg/crc/machine/start.go
+++ b/pkg/crc/machine/start.go
@@ -179,6 +179,7 @@ func (client *client) Start(ctx context.Context, startConfig types.StartConfig)
 			KernelCmdLine:   crcBundleMetadata.GetKernelCommandLine(),
 			Initramfs:       crcBundleMetadata.GetInitramfsPath(),
 			Kernel:          crcBundleMetadata.GetKernelPath(),
+			KubeConfig:      crcBundleMetadata.GetKubeConfigPath(),
 		}
 		if err := createHost(libMachineAPIClient, machineConfig); err != nil {
 			return nil, errors.Wrap(err, "Error creating machine")
@@ -280,9 +281,6 @@ func (client *client) Start(ctx context.Context, startConfig types.StartConfig)
 	if err := updateSSHKeyPair(sshRunner); err != nil {
 		return nil, errors.Wrap(err, "Error updating public key")
 	}
-	if err := copyKubeconfig(client.name, crcBundleMetadata); err != nil {
-		return nil, errors.Wrap(err, "Error copying kubeconfig file")
-	}
 
 	// Trigger disk resize, this will be a no-op if no disk size change is needed
 	if err := growRootFileSystem(sshRunner); err != nil {
@@ -526,6 +524,17 @@ func createHost(api libmachine.API, machineConfig config.MachineConfig) error {
 		return fmt.Errorf("Error in driver during machine creation: %s", err)
 	}
 
+	logging.Info("Generating new SSH Key pair...")
+	if err := crcssh.GenerateSSHKey(constants.GetPrivateKeyPath()); err != nil {
+		return fmt.Errorf("Error generating ssh key pair: %v", err)
+	}
+	if err := cluster.GenerateKubeAdminUserPassword(); err != nil {
+		return errors.Wrap(err, "Error generating new kubeadmin password")
+	}
+	if err := copyKubeconfig(machineConfig.Name, machineConfig); err != nil {
+		return errors.Wrap(err, "Error copying kubeconfig file")
+	}
+
 	if err := api.SetExists(vm.Name); err != nil {
 		return fmt.Errorf("Failed to record VM existence: %s", err)
 	}
@@ -568,18 +577,6 @@ func addNameServerToInstance(sshRunner *crcssh.Runner, ns string) error {
 }
 
 func updateSSHKeyPair(sshRunner *crcssh.Runner) error {
-	if _, err := os.Stat(constants.GetPrivateKeyPath()); err != nil {
-		if !os.IsNotExist(err) {
-			return err
-		}
-
-		// Generate ssh key pair
-		logging.Info("Generating new SSH Key pair...")
-		if err := crcssh.GenerateSSHKey(constants.GetPrivateKeyPath()); err != nil {
-			return fmt.Errorf("Error generating ssh key pair: %v", err)
-		}
-	}
-
 	// Read generated public key
 	publicKey, err := ioutil.ReadFile(constants.GetPublicKeyPath())
 	if err != nil {
@@ -600,7 +597,7 @@ func updateSSHKeyPair(sshRunner *crcssh.Runner) error {
 	return err
 }
 
-func copyKubeconfig(name string, crcBundleMetadata *bundle.CrcBundleInfo) error {
+func copyKubeconfig(name string, machineConfig config.MachineConfig) error {
 	kubeConfigFilePath := filepath.Join(constants.MachineInstanceDir, name, "kubeconfig")
 	if _, err := os.Stat(kubeConfigFilePath); err == nil {
 		return nil
@@ -609,7 +606,7 @@ func copyKubeconfig(name string, crcBundleMetadata *bundle.CrcBundleInfo) error
 	// Copy Kubeconfig file from bundle extract path to machine directory.
 	// In our case it would be ~/.crc/machines/crc/
 	logging.Info("Copying kubeconfig file to instance dir...")
-	err := crcos.CopyFileContents(crcBundleMetadata.GetKubeConfigPath(),
+	err := crcos.CopyFileContents(machineConfig.KubeConfig,
 		kubeConfigFilePath,
 		0644)
 	if err != nil {


Test patch:
diff --git a/pkg/crc/cluster/kubeadmin_password_test.go b/pkg/crc/cluster/kubeadmin_password_test.go
new file mode 100644
index 000000000..524d7be31
--- /dev/null
+++ b/pkg/crc/cluster/kubeadmin_password_test.go
@@ -0,0 +1,58 @@
+package cluster
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestCompareHtpasswdWithOneUsername(t *testing.T) {
+	htpasswd, err := getHtpasswd(map[string]string{"username": "password1"}, []string{})
+	assert.NoError(t, err)
+
+	ok, _, err := compareHtpasswd(htpasswd, map[string]string{"username": "password1"})
+	assert.NoError(t, err)
+	assert.True(t, ok)
+
+	ok, _, err = compareHtpasswd(htpasswd, map[string]string{"username": "password2"})
+	assert.NoError(t, err)
+	assert.False(t, ok)
+
+	ok, _, err = compareHtpasswd(htpasswd, map[string]string{"other": "password1"})
+	assert.NoError(t, err)
+	assert.False(t, ok)
+
+	ok, _, err = compareHtpasswd(htpasswd, map[string]string{"username1": "password1", "username2": "password2"})
+	assert.NoError(t, err)
+	assert.False(t, ok)
+}
+
+func TestCompareHtpasswdWithTwoUsernames(t *testing.T) {
+	htpasswd, err := getHtpasswd(map[string]string{"username1": "password1", "username2": "password2"}, []string{})
+	assert.NoError(t, err)
+
+	ok, _, err := compareHtpasswd(htpasswd, map[string]string{"username1": "password1", "username2": "password2"})
+	assert.NoError(t, err)
+	assert.True(t, ok)
+
+	ok, _, err = compareHtpasswd(htpasswd, map[string]string{"username1": "password1", "username2": "password3"})
+	assert.NoError(t, err)
+	assert.False(t, ok)
+}
+
+func TestCompareFalseWithCustomEntries(t *testing.T) {
+	wanted := map[string]string{"username": "password"}
+
+	htpasswd, err := getHtpasswd(map[string]string{"username": "wrong", "external1": "external1", "external2": "external2"}, []string{})
+	assert.NoError(t, err)
+
+	ok, externals, err := compareHtpasswd(htpasswd, wanted)
+	assert.NoError(t, err)
+	assert.False(t, ok)
+
+	htpasswd, err = getHtpasswd(wanted, externals)
+	assert.NoError(t, err)
+	ok, _, err = compareHtpasswd(htpasswd, map[string]string{"username": "password", "external1": "external1", "external2": "external2"})
+	assert.NoError(t, err)
+	assert.True(t, ok)
+}