wbrawner/toolbox
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Increase the validation coverage for the container & release options

Browse source 
Currently, the container name and release are only validated if they
were specified as command line options.  Neither the value of release
in the configuration file nor the container name generated from an
image are validated.

There's also a lot of repeated code in the command front-ends to
validate the container name and release.  This opens the door for
mistakes.  Any adjustment to the code must be repeated elsewhere, and
there are subtle interactions and overlaps between the validation code
and the code to resolve container and image names.

It's worth noting that the container and image name resolution happens
for both the command line and configuration file options, and generates
the container name from the image when necessary.

Therefore, validating everything while resolving cleans up the command
front-ends and increases the coverage of the validation.

This introduces the use of sentinel error values and custom error
implementations to identify the different errors that can occur while
resolving the container and images, so that they can be appropriately
shown to the user.

https://github.com/containers/toolbox/pull/1101
This commit is contained in:
Debarshi Ray 2022-07-31 10:49:03 +02:00
parent b5474bff84
commit 8ca5611942
10 changed files with 150 additions and 66 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

24
src/cmd/create.go
View file

@ -147,28 +147,12 @@ func create(cmd *cobra.Command, args []string) error {
containerArg = "--container"
}
if container != "" {
if !utils.IsContainerNameValid(container) {
err := createErrorInvalidContainer(containerArg)
return err
}
}
var release string
if createFlags.release != "" {
var err error
release, err = utils.ParseRelease(createFlags.distro, createFlags.release)
if err != nil {
hint := err.Error()
err := createErrorInvalidRelease(hint)
return err
}
}
container, image, release, err := utils.ResolveContainerAndImageNames(container,
container, image, release, err := resolveContainerAndImageNames(container,
containerArg,
createFlags.distro,
createFlags.image,
release)
createFlags.release)
if err != nil {
return err
}

21
src/cmd/enter.go
View file

@ -100,27 +100,18 @@ func enter(cmd *cobra.Command, args []string) error {
if container != "" {
defaultContainer = false
if !utils.IsContainerNameValid(container) {
err := createErrorInvalidContainer(containerArg)
return err
}
}
var release string
if enterFlags.release != "" {
defaultContainer = false
var err error
release, err = utils.ParseRelease(enterFlags.distro, enterFlags.release)
if err != nil {
hint := err.Error()
err := createErrorInvalidRelease(hint)
return err
}
}
container, image, release, err := utils.ResolveContainerAndImageNames(container, enterFlags.distro, "", release)
container, image, release, err := resolveContainerAndImageNames(container,
containerArg,
enterFlags.distro,
"",
enterFlags.release)
if err != nil {
return err
}

2
src/cmd/rootMigrationPath.go
View file

@ -60,7 +60,7 @@ func rootRunImpl(cmd *cobra.Command, args []string) error {
return nil
}
container, image, release, err := utils.ResolveContainerAndImageNames("", "", "", "")
container, image, release, err := resolveContainerAndImageNames("", "", "", "", "")
if err != nil {
return err
}

21
src/cmd/run.go
View file

@ -100,24 +100,10 @@ func run(cmd *cobra.Command, args []string) error {
if runFlags.container != "" {
defaultContainer = false
if !utils.IsContainerNameValid(runFlags.container) {
err := createErrorInvalidContainer("--container")
return err
}
}
var release string
if runFlags.release != "" {
defaultContainer = false
var err error
release, err = utils.ParseRelease(runFlags.distro, runFlags.release)
if err != nil {
hint := err.Error()
err := createErrorInvalidRelease(hint)
return err
}
}
if len(args) == 0 {
@ -131,7 +117,12 @@ func run(cmd *cobra.Command, args []string) error {
command := args
container, image, release, err := utils.ResolveContainerAndImageNames(runFlags.container, runFlags.distro, "", release)
container, image, release, err := resolveContainerAndImageNames(runFlags.container,
"--container",
runFlags.distro,
"",
runFlags.release)
if err != nil {
return err
}

50
src/cmd/utils.go
View file

@ -81,6 +81,17 @@ func createErrorInvalidContainer(containerArg string) error {
return errors.New(errMsg)
}
func createErrorInvalidImageForContainerName(container string) error {
var builder strings.Builder
fmt.Fprintf(&builder, "invalid argument for '--image'\n")
fmt.Fprintf(&builder, "Container name %s generated from image is invalid.\n", container)
fmt.Fprintf(&builder, "Container names must match '%s'.\n", utils.ContainerNameRegexp)
fmt.Fprintf(&builder, "Run '%s --help' for usage.", executableBase)
errMsg := builder.String()
return errors.New(errMsg)
}
func createErrorInvalidRelease(hint string) error {
var builder strings.Builder
fmt.Fprintf(&builder, "invalid argument for '--release'\n")
@ -101,6 +112,45 @@ func getUsageForCommonCommands() string {
return usage
}
func resolveContainerAndImageNames(container, containerArg, distroCLI, imageCLI, releaseCLI string) (
string, string, string, error,
) {
container, image, release, err := utils.ResolveContainerAndImageNames(container,
distroCLI,
imageCLI,
releaseCLI)
if err != nil {
var errContainer *utils.ContainerError
var errParseRelease *utils.ParseReleaseError
if errors.As(err, &errContainer) {
if errors.Is(err, utils.ErrContainerNameInvalid) {
if containerArg == "" {
panicMsg := fmt.Sprintf("unexpected %T without containerArg: %s", err, err)
panic(panicMsg)
}
err := createErrorInvalidContainer(containerArg)
return "", "", "", err
} else if errors.Is(err, utils.ErrContainerNameFromImageInvalid) {
err := createErrorInvalidImageForContainerName(errContainer.Container)
return "", "", "", err
} else {
panicMsg := fmt.Sprintf("unexpected %T: %s", err, err)
panic(panicMsg)
}
} else if errors.As(err, &errParseRelease) {
err := createErrorInvalidRelease(errParseRelease.Hint)
return "", "", "", err
} else {
return "", "", "", err
}
}
return container, image, release, nil
}
// showManual tries to open the specified manual page using man on stdout
func showManual(manual string) error {
manBinary, err := exec.LookPath("man")

1
src/meson.build
View file

@ -20,6 +20,7 @@ sources = files(
'cmd/utils.go',
'pkg/podman/podman.go',
'pkg/shell/shell.go',
'pkg/utils/errors.go',
'pkg/utils/utils.go',
'pkg/version/version.go',
)

44
src/pkg/utils/errors.go Normal file
View file

@ -0,0 +1,44 @@
/*
* Copyright © 2022 Red Hat Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package utils
import (
"fmt"
)
type ContainerError struct {
Container string
Image string
Err error
}
type ParseReleaseError struct {
Hint string
}
func (err *ContainerError) Error() string {
errMsg := fmt.Sprintf("%s: %s", err.Container, err.Err)
return errMsg
}
func (err *ContainerError) Unwrap() error {
return err.Err
}
func (err *ParseReleaseError) Error() string {
return err.Hint
}

38
src/pkg/utils/utils.go
View file

@ -117,6 +117,10 @@ var (
var (
ContainerNameDefault string
ErrContainerNameFromImageInvalid = errors.New("container name generated from image is invalid")
ErrContainerNameInvalid = errors.New("container name is invalid")
)
func init() {
@ -605,16 +609,9 @@ func ShortID(id string) string {
return id
}
func ParseRelease(distro, release string) (string, error) {
func parseRelease(distro, release string) (string, error) {
if distro == "" {
distro = distroDefault
if viper.IsSet("general.distro") {
distro = viper.GetString("general.distro")
}
}
if _, supportedDistro := supportedDistros[distro]; !supportedDistro {
distro = distroFallback
panic("distro not specified")
}
distroObj, supportedDistro := supportedDistros[distro]
@ -636,11 +633,11 @@ func parseReleaseFedora(release string) (string, error) {
releaseN, err := strconv.Atoi(release)
if err != nil {
logrus.Debugf("Parsing release %s as an integer failed: %s", release, err)
return "", errors.New("The release must be a positive integer.")
return "", &ParseReleaseError{"The release must be a positive integer."}
}
if releaseN <= 0 {
return "", errors.New("The release must be a positive integer.")
return "", &ParseReleaseError{"The release must be a positive integer."}
}
return release, nil
@ -648,17 +645,17 @@ func parseReleaseFedora(release string) (string, error) {
func parseReleaseRHEL(release string) (string, error) {
if i := strings.IndexRune(release, '.'); i == -1 {
return "", errors.New("The release must be in the '<major>.<minor>' format.")
return "", &ParseReleaseError{"The release must be in the '<major>.<minor>' format."}
}
releaseN, err := strconv.ParseFloat(release, 32)
if err != nil {
logrus.Debugf("Parsing release %s as a float failed: %s", release, err)
return "", errors.New("The release must be in the '<major>.<minor>' format.")
return "", &ParseReleaseError{"The release must be in the '<major>.<minor>' format."}
}
if releaseN <= 0 {
return "", errors.New("The release must be a positive number.")
return "", &ParseReleaseError{"The release must be a positive number."}
}
return release, nil
@ -730,6 +727,11 @@ func ResolveContainerAndImageNames(container, distroCLI, imageCLI, releaseCLI st
}
}
release, err := parseRelease(distro, release)
if err != nil {
return "", "", "", err
}
if imageCLI == "" {
image = getDefaultImageForDistro(distro, release)
@ -765,6 +767,14 @@ func ResolveContainerAndImageNames(container, distroCLI, imageCLI, releaseCLI st
if tag != "" {
container = container + "-" + tag
}
if !IsContainerNameValid(container) {
return "", "", "", &ContainerError{container, image, ErrContainerNameFromImageInvalid}
}
} else {
if !IsContainerNameValid(container) {
return "", "", "", &ContainerError{container, "", ErrContainerNameInvalid}
}
}
logrus.Debug("Resolved container and image names")

2
src/pkg/utils/utils_test.go
View file

@ -150,7 +150,7 @@ func TestParseRelease(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
release, err := ParseRelease(tc.inputDistro, tc.inputRelease)
release, err := parseRelease(tc.inputDistro, tc.inputRelease)
if tc.ok {
assert.NoError(t, err)

13
test/system/101-create.bats
View file

@ -56,6 +56,19 @@ teardown() {
assert_line --index 2 "Run 'toolbox --help' for usage."
}
@test "create: Try to create a container with invalid custom image ('ßpeci@l.Nam€')" {
local image="ßpeci@l.Nam€"
run $TOOLBOX create --image "$image"
assert_failure
assert_line --index 0 "Error: invalid argument for '--image'"
assert_line --index 1 "Container name $image generated from image is invalid."
assert_line --index 2 "Container names must match '[a-zA-Z0-9][a-zA-Z0-9_.-]*'."
assert_line --index 3 "Run 'toolbox --help' for usage."
assert [ ${#lines[@]} -eq 4 ]
}
@test "create: Create a container with a distro and release options ('fedora'; f32)" {
pull_distro_image fedora 32