Repository: osbuild/osbuild-composer
Pull request #71 (2019-10-31 17:53:30)

blueprint: don't create group with the same as one of the users


Patch:
diff --git a/internal/blueprint/ami_output.go b/internal/blueprint/ami_output.go
index 7335ce5e0..c2bd79ea2 100644
--- a/internal/blueprint/ami_output.go
+++ b/internal/blueprint/ami_output.go
@@ -21,7 +21,7 @@ func (t *amiOutput) translate(b *Blueprint) *pipeline.Pipeline {
 	excludedPackages := [...]string{
 		"dracut-config-rescue",
 	}
-	p := getCustomF30PackageSet(packages[:], excludedPackages[:])
+	p := getCustomF30PackageSet(packages[:], excludedPackages[:], b)
 	addF30FixBlsStage(p)
 	addF30LocaleStage(p)
 	addF30FSTabStage(p)
diff --git a/internal/blueprint/blueprint.go b/internal/blueprint/blueprint.go
index 78521e6a7..b68b306cc 100644
--- a/internal/blueprint/blueprint.go
+++ b/internal/blueprint/blueprint.go
@@ -25,7 +25,7 @@ type Blueprint struct {
 	Version        string          `json:"version,omitempty"`
 	Packages       []Package       `json:"packages"`
 	Modules        []Package       `json:"modules"`
-	Groups         []Package       `json:"groups"`
+	Groups         []Group         `json:"groups"`
 	Customizations *Customizations `json:"customizations,omitempty"`
 }
 
@@ -35,6 +35,11 @@ type Package struct {
 	Version string `json:"version,omitempty"`
 }
 
+// A group specifies an package group.
+type Group struct {
+	Name string `json:"name"`
+}
+
 type output interface {
 	translate(b *Blueprint) *pipeline.Pipeline
 	getName() string
@@ -90,3 +95,12 @@ func FilenameFromType(outputFormat string) (string, string, error) {
 
 	return "", "", &InvalidOutputFormatError{outputFormat}
 }
+
+func (p Package) ToNameVersion() string {
+	// Omit version to prevent all packages with prefix of name to be installed
+	if p.Version == "*" {
+		return p.Name
+	}
+
+	return p.Name + "-" + p.Version
+}
diff --git a/internal/blueprint/customizations.go b/internal/blueprint/customizations.go
index f5f58eab7..44243a5db 100644
--- a/internal/blueprint/customizations.go
+++ b/internal/blueprint/customizations.go
@@ -1,7 +1,9 @@
 package blueprint
 
 import (
+	"github.com/osbuild/osbuild-composer/internal/crypt"
 	"github.com/osbuild/osbuild-composer/internal/pipeline"
+	"log"
 	"strconv"
 	"strings"
 )
@@ -110,9 +112,13 @@ func (c *Customizations) customizeGroup(p *pipeline.Pipeline) {
 		return
 	}
 
-	// TODO: lorax-composer doesn't create group with the same name as user if both specified
 	groups := make(map[string]pipeline.GroupsStageOptionsGroup)
 	for _, group := range c.Group {
+		if userCustomizationsContainUsername(c.User, group.Name) {
+			log.Println("group with name ", group.Name, " was not created, because user with same name was defined!")
+			continue
+		}
+
 		groupData := pipeline.GroupsStageOptionsGroup{}
 		if group.GID != nil {
 			gid := strconv.Itoa(*group.GID)
@@ -161,8 +167,16 @@ func (c *Customizations) customizeUserAndSSHKey(p *pipeline.Pipeline) error {
 
 	users := make(map[string]pipeline.UsersStageOptionsUser)
 	for _, user := range c.User {
-		// TODO: only hashed password are currently supported as an input
-		// plain-text passwords should be also supported due to parity with lorax-composer
+
+		if user.Password != nil && !crypt.PasswordIsCrypted(*user.Password) {
+			cryptedPassword, err := crypt.CryptSHA512(*user.Password)
+			if err != nil {
+				return err
+			}
+
+			user.Password = &cryptedPassword
+		}
+
 		userData := pipeline.UsersStageOptionsUser{
 			Groups:      user.Groups,
 			Description: user.Description,
@@ -305,3 +319,13 @@ func findKeysForUser(sshKeyCustomizations []SSHKeyCustomization, user string) (k
 	}
 	return
 }
+
+func userCustomizationsContainUsername(userCustomizations []UserCustomization, name string) bool {
+	for _, usr := range userCustomizations {
+		if usr.Name == name {
+			return true
+		}
+	}
+
+	return false
+}
diff --git a/internal/blueprint/disk_output.go b/internal/blueprint/disk_output.go
index 65509f523..6c8b24066 100644
--- a/internal/blueprint/disk_output.go
+++ b/internal/blueprint/disk_output.go
@@ -17,7 +17,7 @@ func (t *diskOutput) translate(b *Blueprint) *pipeline.Pipeline {
 	excludedPackages := [...]string{
 		"dracut-config-rescue",
 	}
-	p := getCustomF30PackageSet(packages[:], excludedPackages[:])
+	p := getCustomF30PackageSet(packages[:], excludedPackages[:], b)
 	addF30LocaleStage(p)
 	addF30FSTabStage(p)
 	addF30GRUB2Stage(p, b.getKernelCustomization())
diff --git a/internal/blueprint/ext4_output.go b/internal/blueprint/ext4_output.go
index 928f16c73..ef263cf36 100644
--- a/internal/blueprint/ext4_output.go
+++ b/internal/blueprint/ext4_output.go
@@ -16,7 +16,7 @@ func (t *ext4Output) translate(b *Blueprint) *pipeline.Pipeline {
 	excludedPackages := [...]string{
 		"dracut-config-rescue",
 	}
-	p := getCustomF30PackageSet(packages[:], excludedPackages[:])
+	p := getCustomF30PackageSet(packages[:], excludedPackages[:], b)
 	addF30LocaleStage(p)
 	addF30GRUB2Stage(p, b.getKernelCustomization())
 	addF30FixBlsStage(p)
diff --git a/internal/blueprint/f30_helpers.go b/internal/blueprint/f30_helpers.go
index e53a8059e..ae73e9d9b 100644
--- a/internal/blueprint/f30_helpers.go
+++ b/internal/blueprint/f30_helpers.go
@@ -89,7 +89,7 @@ func getF30Pipeline() *pipeline.Pipeline {
 	return p
 }
 
-func getCustomF30PackageSet(packages []string, excludedPackages []string) *pipeline.Pipeline {
+func getCustomF30PackageSet(packages []string, excludedPackages []string, blueprint *Blueprint) *pipeline.Pipeline {
 	p := &pipeline.Pipeline{
 		BuildPipeline: getF30BuildPipeline(),
 	}
@@ -104,6 +104,20 @@ func getCustomF30PackageSet(packages []string, excludedPackages []string) *pipel
 	for _, pkg := range excludedPackages {
 		options.ExcludePackage(pkg)
 	}
+
+	// handle extra packages, modules (currently synonym to packages) and groups
+	for _, pkg := range blueprint.Packages {
+		options.AddPackage(pkg.ToNameVersion())
+	}
+
+	for _, pkg := range blueprint.Modules {
+		options.AddPackage(pkg.ToNameVersion())
+	}
+
+	for _, group := range blueprint.Groups {
+		options.AddPackage(group.Name)
+	}
+
 	p.AddStage(pipeline.NewDNFStage(options))
 	return p
 }
diff --git a/internal/blueprint/openstack_output.go b/internal/blueprint/openstack_output.go
index 333271b9a..292582696 100644
--- a/internal/blueprint/openstack_output.go
+++ b/internal/blueprint/openstack_output.go
@@ -21,7 +21,7 @@ func (t *openstackOutput) translate(b *Blueprint) *pipeline.Pipeline {
 	excludedPackages := [...]string{
 		"dracut-config-rescue",
 	}
-	p := getCustomF30PackageSet(packages[:], excludedPackages[:])
+	p := getCustomF30PackageSet(packages[:], excludedPackages[:], b)
 	addF30LocaleStage(p)
 	addF30FSTabStage(p)
 	addF30GRUB2Stage(p, b.getKernelCustomization())
diff --git a/internal/blueprint/qcow2_output.go b/internal/blueprint/qcow2_output.go
index 0ff1587b7..fdb1c832c 100644
--- a/internal/blueprint/qcow2_output.go
+++ b/internal/blueprint/qcow2_output.go
@@ -22,7 +22,7 @@ func (t *qcow2Output) translate(b *Blueprint) *pipeline.Pipeline {
 		"gobject-introspection",
 		"plymouth",
 	}
-	p := getCustomF30PackageSet(packages[:], excludedPackages[:])
+	p := getCustomF30PackageSet(packages[:], excludedPackages[:], b)
 	addF30LocaleStage(p)
 	addF30FSTabStage(p)
 	addF30GRUB2Stage(p, b.getKernelCustomization())
diff --git a/internal/blueprint/tar_output.go b/internal/blueprint/tar_output.go
index 352fd62ee..b2694685e 100644
--- a/internal/blueprint/tar_output.go
+++ b/internal/blueprint/tar_output.go
@@ -16,7 +16,7 @@ func (t *tarOutput) translate(b *Blueprint) *pipeline.Pipeline {
 	excludedPackages := [...]string{
 		"dracut-config-rescue",
 	}
-	p := getCustomF30PackageSet(packages[:], excludedPackages[:])
+	p := getCustomF30PackageSet(packages[:], excludedPackages[:], b)
 	addF30LocaleStage(p)
 	addF30GRUB2Stage(p, b.getKernelCustomization())
 	addF30FixBlsStage(p)
diff --git a/internal/blueprint/vhd_output.go b/internal/blueprint/vhd_output.go
index 2ae21bfad..96166ed38 100644
--- a/internal/blueprint/vhd_output.go
+++ b/internal/blueprint/vhd_output.go
@@ -20,7 +20,7 @@ func (t *vhdOutput) translate(b *Blueprint) *pipeline.Pipeline {
 	excludedPackages := [...]string{
 		"dracut-config-rescue",
 	}
-	p := getCustomF30PackageSet(packages[:], excludedPackages[:])
+	p := getCustomF30PackageSet(packages[:], excludedPackages[:], b)
 	addF30LocaleStage(p)
 	addF30FSTabStage(p)
 	addF30GRUB2Stage(p, b.getKernelCustomization())
diff --git a/internal/blueprint/vmdk_output.go b/internal/blueprint/vmdk_output.go
index 212b351fa..decd0a4e9 100644
--- a/internal/blueprint/vmdk_output.go
+++ b/internal/blueprint/vmdk_output.go
@@ -18,7 +18,7 @@ func (t *vmdkOutput) translate(b *Blueprint) *pipeline.Pipeline {
 	excludedPackages := [...]string{
 		"dracut-config-rescue",
 	}
-	p := getCustomF30PackageSet(packages[:], excludedPackages[:])
+	p := getCustomF30PackageSet(packages[:], excludedPackages[:], b)
 	addF30LocaleStage(p)
 	addF30FSTabStage(p)
 	addF30GRUB2Stage(p, b.getKernelCustomization())
diff --git a/internal/crypt/crypt.go b/internal/crypt/crypt.go
new file mode 100644
index 000000000..48adce4a7
--- /dev/null
+++ b/internal/crypt/crypt.go
@@ -0,0 +1,49 @@
+package crypt
+
+import (
+	"crypto/rand"
+	"math/big"
+	"strings"
+)
+
+func CryptSHA512(phrase string) (string, error) {
+	const SHA512SaltLength = 16
+
+	salt, err := genSalt(SHA512SaltLength)
+
+	if err != nil {
+		return "", nil
+	}
+
+	hashSettings := "$6$" + salt
+	return crypt(phrase, hashSettings)
+}
+
+func genSalt(length int) (string, error) {
+	saltChars := "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789./"
+
+	b := make([]byte, length)
+
+	for i := range b {
+		runeIndex, err := rand.Int(rand.Reader, big.NewInt(int64(len(saltChars))))
+		if err != nil {
+			return "", err
+		}
+		b[i] = saltChars[runeIndex.Int64()]
+	}
+
+	return string(b), nil
+}
+
+func PasswordIsCrypted(s string) bool {
+	// taken from lorax src: src/pylorax/api/compose.py:533
+	prefixes := [...]string{"$2b$", "$6$", "$5$"}
+
+	for _, prefix := range prefixes {
+		if strings.HasPrefix(s, prefix) {
+			return true
+		}
+	}
+
+	return false
+}
diff --git a/internal/crypt/crypt_impl.go b/internal/crypt/crypt_impl.go
new file mode 100644
index 000000000..088cf4462
--- /dev/null
+++ b/internal/crypt/crypt_impl.go
@@ -0,0 +1,85 @@
+// Copied from https://github.com/amoghe/go-crypt/blob/b3e291286513a0c993f7c4dd7060d327d2d56143/crypt_r.go
+// Original sources are under MIT license:
+// The MIT License (MIT)
+//
+// Copyright (c) 2015 Akshay Moghe
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in all
+// copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+// SOFTWARE.
+//
+// Package crypt provides wrappers around functions available in crypt.h
+//
+// It wraps around the GNU specific extension (crypt_r) when it is available
+// (i.e. where GOOS=linux). This makes the go function reentrant (and thus
+// callable from concurrent goroutines).
+package crypt
+
+import (
+	"unsafe"
+)
+
+/*
+	#cgo LDFLAGS: -lcrypt
+
+	// this is needed for Ubuntu
+	#define _GNU_SOURCE
+
+	#include <stdlib.h>
+	#include <string.h>
+	#include <crypt.h>
+
+	char *gnu_ext_crypt(char *pass, char *salt) {
+		char *enc = NULL;
+		char *ret = NULL;
+		struct crypt_data data;
+		data.initialized = 0;
+
+		enc = crypt_r(pass, salt, &data);
+		if(enc == NULL) {
+			return NULL;
+		}
+
+		ret = (char *)malloc((strlen(enc)+1) * sizeof(char)); // for trailing null
+		strcpy(ret, enc);
+		ret[strlen(enc)]= '\0';
+
+	 return ret;
+	}
+*/
+import "C"
+
+// Crypt provides a wrapper around the glibc crypt_r() function.
+// For the meaning of the arguments, refer to the package README.
+func crypt(pass, salt string) (string, error) {
+	c_pass := C.CString(pass)
+	defer C.free(unsafe.Pointer(c_pass))
+
+	c_salt := C.CString(salt)
+	defer C.free(unsafe.Pointer(c_salt))
+
+	c_enc, err := C.gnu_ext_crypt(c_pass, c_salt)
+	if c_enc == nil {
+		return "", err
+	}
+	defer C.free(unsafe.Pointer(c_enc))
+
+	// Return nil error if the string is non-nil.
+	// As per the errno.h manpage, functions are allowed to set errno
+	// on success. Caller should ignore errno on success.
+	return C.GoString(c_enc), nil
+}
diff --git a/internal/store/store.go b/internal/store/store.go
index 9472760ef..2438b6d43 100644
--- a/internal/store/store.go
+++ b/internal/store/store.go
@@ -306,7 +306,7 @@ func (s *Store) GetBlueprint(name string, bp *blueprint.Blueprint, changed *bool
 		bp.Modules = []blueprint.Package{}
 	}
 	if bp.Groups == nil {
-		bp.Groups = []blueprint.Package{}
+		bp.Groups = []blueprint.Group{}
 	}
 	if bp.Version == "" {
 		bp.Version = "0.0.0"
@@ -337,7 +337,7 @@ func (s *Store) GetBlueprintCommitted(name string, bp *blueprint.Blueprint) bool
 		bp.Modules = []blueprint.Package{}
 	}
 	if bp.Groups == nil {
-		bp.Groups = []blueprint.Package{}
+		bp.Groups = []blueprint.Group{}
 	}
 	if bp.Version == "" {
 		bp.Version = "0.0.0"


Test patch:
diff --git a/internal/crypt/crypt_test.go b/internal/crypt/crypt_test.go
new file mode 100644
index 000000000..4a29404b8
--- /dev/null
+++ b/internal/crypt/crypt_test.go
@@ -0,0 +1,43 @@
+package crypt
+
+import (
+	"testing"
+)
+
+func Test_crypt_PasswordIsCrypted(t *testing.T) {
+
+	tests := []struct {
+		name     string
+		password string
+		want     bool
+	}{
+		{
+			name:     "bcrypt",
+			password: "$2b$04$123465789012345678901uac5A8egfBuZVHMrDZsQzR96IqNBivCy",
+			want:     true,
+		}, {
+			name:     "sha256",
+			password: "$5$1234567890123456$v.2bOKKLlpmUSKn0rxJmgnh.e3wOKivAVNZmNrOsoA3",
+			want:     true,
+		}, {
+			name:     "sha512",
+			password: "$6$1234567890123456$d.pgKQFaiD8bRiExg5NesbGR/3u51YvxeYaQXPzx4C6oSYREw8VoReiuYZjx0V9OhGVTZFqhc6emAxT1RC5BV.",
+			want:     true,
+		}, {
+			name:     "scrypt",
+			password: "$7$123456789012345", //not actual hash output from scrypt
+			want:     false,
+		}, {
+			name:     "plain",
+			password: "password",
+			want:     false,
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			if got := PasswordIsCrypted(test.password); got != test.want {
+				t.Errorf("PasswordIsCrypted() =%v, want %v", got, test.want)
+			}
+		})
+	}
+}
diff --git a/internal/weldr/api_test.go b/internal/weldr/api_test.go
index 783a736b8..c786a1a26 100644
--- a/internal/weldr/api_test.go
+++ b/internal/weldr/api_test.go
@@ -78,7 +78,37 @@ func sendHTTP(api *weldr.API, external bool, method, path, body string) *http.Re
 	}
 }
 
-func testRoute(t *testing.T, api *weldr.API, external bool, method, path, body string, expectedStatus int, expectedJSON string) {
+// this function serves to drop fields that shouldn't be tested from the unmarshalled json objects
+func dropFields(obj interface{}, fields ...string) {
+	switch v := obj.(type) {
+	// if the interface type is a map attempt to delete the fields
+	case map[string]interface{}:
+		for i, field := range fields {
+			if _, ok := v[field]; ok {
+				delete(v, field)
+				// if the field is found remove it from the fields slice
+				if len(fields) < i-1 {
+					fields = append(fields[:i], fields[i+1:]...)
+				} else {
+					fields = fields[:i]
+				}
+			}
+		}
+		// call dropFields on the remaining elements since they may contain a map containing the field
+		for _, val := range v {
+			dropFields(val, fields...)
+		}
+	// if the type is a list of interfaces call dropFields on each interface
+	case []interface{}:
+		for _, element := range v {
+			dropFields(element, fields...)
+		}
+	default:
+		return
+	}
+}
+
+func testRoute(t *testing.T, api *weldr.API, external bool, method, path, body string, expectedStatus int, expectedJSON string, ignoreFields ...string) {
 	resp := sendHTTP(api, external, method, path, body)
 	if resp == nil {
 		t.Skip("This test is for internal testing only")
@@ -119,6 +149,9 @@ func testRoute(t *testing.T, api *weldr.API, external bool, method, path, body s
 		return
 	}
 
+	dropFields(reply, ignoreFields...)
+	dropFields(expected, ignoreFields...)
+
 	if !reflect.DeepEqual(reply, expected) {
 		t.Errorf("%s: reply != expected:\n   reply: %s\nexpected: %s", path, strings.TrimSpace(string(replyJSON)), expectedJSON)
 		return
@@ -273,15 +306,16 @@ func TestCompose(t *testing.T) {
 		Body           string
 		ExpectedStatus int
 		ExpectedJSON   string
+		IgnoreFields   []string
 	}{
-		{true, "POST", "/api/v0/compose", `{"blueprint_name": "http-server","compose_type": "tar","branch": "master"}`, http.StatusBadRequest, `{"status":false,"errors":[{"id":"UnknownBlueprint","msg":"Unknown blueprint name: http-server"}]}`},
-		{false, "POST", "/api/v0/compose", `{"blueprint_name": "test","compose_type": "tar","branch": "master"}`, http.StatusOK, `*`},
+		{true, "POST", "/api/v0/compose", `{"blueprint_name": "http-server","compose_type": "tar","branch": "master"}`, http.StatusBadRequest, `{"status":false,"errors":[{"id":"UnknownBlueprint","msg":"Unknown blueprint name: http-server"}]}`, []string{"build_id"}},
+		{false, "POST", "/api/v0/compose", `{"blueprint_name": "test","compose_type": "tar","branch": "master"}`, http.StatusOK, `{"status": true}`, []string{"build_id"}},
 	}
 
 	for _, c := range cases {
 		api := weldr.New(repo, packages, nil, store.New(nil))
 		sendHTTP(api, c.External, "POST", "/api/v0/blueprints/new", `{"name":"test","description":"Test","packages":[{"name":"httpd","version":"2.4.*"}],"version":"0.0.0"}`)
-		testRoute(t, api, c.External, c.Method, c.Path, c.Body, c.ExpectedStatus, c.ExpectedJSON)
+		testRoute(t, api, c.External, c.Method, c.Path, c.Body, c.ExpectedStatus, c.ExpectedJSON, c.IgnoreFields...)
 		sendHTTP(api, c.External, "DELETE", "/api/v0/blueprints/delete/test", ``)
 	}
 }
@@ -293,8 +327,9 @@ func TestComposeQueue(t *testing.T) {
 		Body           string
 		ExpectedStatus int
 		ExpectedJSON   string
+		IgnoreFields   []string
 	}{
-		{"GET", "/api/v0/compose/queue", ``, http.StatusOK, `*`}, // TODO: we need a way to verify we actually get the expected json here
+		{"GET", "/api/v0/compose/queue", ``, http.StatusOK, `{"new":[{"blueprint":"test","version":"0.0.0","compose_type":"tar","image_size":0,"queue_status":"WAITING"}],"run":[{"blueprint":"test","version":"0.0.0","compose_type":"tar","image_size":0,"queue_status":"RUNNING"}]}`, []string{"id", "job_created", "job_started"}},
 	}
 
 	if len(os.Getenv("OSBUILD_COMPOSER_TEST_EXTERNAL")) > 0 {
@@ -318,7 +353,8 @@ func TestComposeQueue(t *testing.T) {
 		sendHTTP(api, false, "POST", "/api/v0/compose", `{"blueprint_name": "test","compose_type": "tar","branch": "master"}`)
 		job = s.PopCompose()
 		s.UpdateCompose(job.ComposeID, "FAILED")
-		testRoute(t, api, false, c.Method, c.Path, c.Body, c.ExpectedStatus, c.ExpectedJSON)
+
+		testRoute(t, api, false, c.Method, c.Path, c.Body, c.ExpectedStatus, c.ExpectedJSON, c.IgnoreFields...)
 		sendHTTP(api, false, "DELETE", "/api/v0/blueprints/delete/test", ``)
 	}
 }