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

Cleanups from PR review #1813

Merged
merged 7 commits into from
Jun 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 46 additions & 27 deletions internal/builder/builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -563,12 +563,12 @@ func (b *Builder) Save(logger logging.Logger, creatorMetadata CreatorMetadata) e

func (b *Builder) addExplodedModules(kind string, logger logging.Logger, tmpDir string, image imgutil.Image, additionalModules []buildpack.BuildModule, layers dist.ModuleLayers) error {
collectionToAdd := map[string]moduleWithDiffID{}
splitAdditionalModules, errs := splitBuildModules(kind, tmpDir, additionalModules, logger)
toAdd, errs := explodeModules(kind, tmpDir, additionalModules, logger)
if len(errs) > 0 {
return e.Join(errs...)
}

for i, additionalModule := range splitAdditionalModules {
for i, additionalModule := range toAdd {
info, diffID, layerTar, module := additionalModule.module.Descriptor().Info(), additionalModule.diffID, additionalModule.tarPath, additionalModule.module

// check against builder layers
Expand Down Expand Up @@ -1143,15 +1143,19 @@ func sortKeys(collection map[string]moduleWithDiffID) []string {
return keys
}

// splitBuildModules takes the given Build Modules and concurrently read their internal tar files. In
// case a flattened module is found, it will split each flattened buildpack into an individual module and skip all the empty ones
// returns an explodedBuildModule array with the Build Module information but also the diffId and the tar file on disk
func splitBuildModules(kind, tmpDir string, additionalModules []buildpack.BuildModule, logger logging.Logger) ([]moduleWithDiffID, []error) {
// explodeModules takes a collection of build modules and concurrently reads their tar files.
// It assumes the modules were extracted with `buildpack.extractBuildpacks`, which when provided a flattened buildpack package containing N buildpacks,
// will return N modules: 1 module with a single tar containing ALL N buildpacks, and N-1 modules with empty tar files.
// As we iterate through the modules, in case a flattened module (tar containing all N buildpacks) is found,
// explodeModules will split the module into N modules, each with a single tar containing a single buildpack.
// In case a module with an empty tar file is found, it is ignored.
func explodeModules(kind, tmpDir string, additionalModules []buildpack.BuildModule, logger logging.Logger) ([]moduleWithDiffID, []error) {
modInfoChans := make([]chan modInfo, len(additionalModules))
for i := range modInfoChans {
modInfoChans[i] = make(chan modInfo, 1)
}

// Explode modules concurrently
for i, module := range additionalModules {
go func(i int, module buildpack.BuildModule) {
modTmpDir := filepath.Join(tmpDir, fmt.Sprintf("%s-%s", kind, strconv.Itoa(i)))
Expand All @@ -1166,44 +1170,59 @@ func splitBuildModules(kind, tmpDir string, additionalModules []buildpack.BuildM
}(i, module)
}

var result []moduleWithDiffID
var errs []error

// skip the flattened buildpacks that returned an empty tar file, their contents
// are included in a different Build Module that was split
// Iterate over modules sequentially, building up the result.
var (
result []moduleWithDiffID
errs []error
)
for i, module := range additionalModules {
mi := <-modInfoChans[i]
if mi.err != nil {
errs = append(errs, mi.err)
continue
}
// moduleTar could be an individual Buildpack or flattened Buildpack that writes an empty tar on disk
for _, moduleTar := range mi.moduleTars {
explodedMod := moduleWithDiffID{tarPath: moduleTar.Path()}
if len(mi.moduleTars) == 1 {
// This entry is an individual buildpack or extension, or a module with empty tar
moduleTar := mi.moduleTars[0]
diffID, err := dist.LayerDiffID(moduleTar.Path())
if err != nil {
errs = append(errs, errors.Wrapf(err, "calculating layer diffID for path %s", moduleTar.Path()))
continue
}
if diffID.String() == emptyTarDiffID {
logger.Debugf("%s %s is a component of a flattened buildpack that will be added elsewhere, skipping...", istrings.Title(kind), style.Symbol(moduleTar.Info().FullName()))
continue // we don't need to keep empty tars
continue // we don't need to keep modules with empty tars
}
explodedMod.diffID = diffID.String()
if moduleTar.Info().FullName() == fmt.Sprintf("%s@%s", module.Descriptor().EscapedID(), module.Descriptor().Info().Version) ||
moduleTar.Info().FullName() == module.Descriptor().Info().FullName() {
explodedMod.module = module
} else {
// we need to match the exploded modules with its corresponding BuildModule.
// this is important when flattened modules where included
for _, additionalModule := range additionalModules {
if namesMatch(additionalModule, moduleTar) {
explodedMod.module = additionalModule
break
result = append(result, moduleWithDiffID{
tarPath: moduleTar.Path(),
diffID: diffID.String(),
module: module,
})
} else {
// This entry is a flattened buildpack that was exploded, we need to add each exploded buildpack to the result in order
for _, moduleTar := range mi.moduleTars {
diffID, err := dist.LayerDiffID(moduleTar.Path())
if err != nil {
errs = append(errs, errors.Wrapf(err, "calculating layer diffID for path %s", moduleTar.Path()))
continue
}
explodedMod := moduleWithDiffID{
tarPath: moduleTar.Path(),
diffID: diffID.String(),
}
// find the module "info" for this buildpack - it could be the current module, or one of the modules with empty tars that was ignored
if namesMatch(module, moduleTar) {
explodedMod.module = module
} else {
for _, additionalModule := range additionalModules {
if namesMatch(additionalModule, moduleTar) {
explodedMod.module = additionalModule
break
}
}
}
result = append(result, explodedMod)
}
result = append(result, explodedMod)
}
}

Expand Down
2 changes: 1 addition & 1 deletion internal/builder/builder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -809,7 +809,7 @@ func testBuilder(t *testing.T, when spec.G, it spec.S) {
bpLayerString, err := json.Marshal(bpLayer)
h.AssertNil(t, err)

h.AssertNil(t, baseImage.SetLabel(
h.AssertNil(t, baseImage.SetLabel( // label builder as already having a buildpack with diffID `diffID`
dist.BuildpackLayersLabel,
string(bpLayerString),
))
Expand Down
122 changes: 61 additions & 61 deletions pkg/buildpack/buildpack.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"os"
"path"
"path/filepath"
"runtime"
"strings"

"github.com/BurntSushi/toml"
Expand All @@ -31,7 +30,6 @@ type BuildModule interface {
// timestamp and root UID/GID).
Open() (io.ReadCloser, error)
Descriptor() Descriptor
ContainsFlattenedModules() bool
}

type Descriptor interface {
Expand Down Expand Up @@ -67,17 +65,12 @@ func Flattened() BlobOption {
type buildModule struct {
descriptor Descriptor
Blob `toml:"-"`
flattened bool
}

func (b *buildModule) Descriptor() Descriptor {
return b.descriptor
}

func (b *buildModule) ContainsFlattenedModules() bool {
return b.flattened
}

// FromBlob constructs a buildpack or extension from a blob. It is assumed that the buildpack
// contents are structured as per the distribution spec (currently '/cnb/buildpacks/{ID}/{version}/*' or
// '/cnb/extensions/{ID}/{version}/*').
Expand All @@ -92,7 +85,6 @@ func FromBlob(descriptor Descriptor, blob Blob, ops ...BlobOption) BuildModule {
return &buildModule{
Blob: blob,
descriptor: descriptor,
flattened: blobOpts.flattened,
}
}

Expand Down Expand Up @@ -356,10 +348,6 @@ func ToLayerTar(dest string, module BuildModule) (string, error) {
}

func ToNLayerTar(dest string, module BuildModule) ([]ModuleTar, error) {
if !module.ContainsFlattenedModules() {
return handleSingleOrEmptyModule(dest, module)
}
Comment on lines -359 to -361
Copy link
Member Author

Choose a reason for hiding this comment

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

The recursive function can handle the case where the module contains a single buildpack


modReader, err := module.Open()
if err != nil {
return nil, errors.Wrap(err, "opening blob")
Expand All @@ -369,22 +357,40 @@ func ToNLayerTar(dest string, module BuildModule) ([]ModuleTar, error) {
tarCollection := newModuleTarCollection(dest)
tr := tar.NewReader(modReader)

// read the first header
header, err := tr.Next()
if err != nil {
if err == io.EOF {
return handleSingleOrEmptyModule(dest, module)
var (
header *tar.Header
forWindows bool
)

for {
header, err = tr.Next()

Check failure

Code scanning / CodeQL

Arbitrary file write during zip extraction ("zip slip")

Unsanitized archive entry, which may contain '..', is used in a [file system operation](1).
if err != nil {
if err == io.EOF {
return handleEmptyModule(dest, module)
}
return nil, err
}
if header.Name == "Files" {
forWindows = true
}
if strings.Contains(header.Name, `/cnb/buildpacks/`) || strings.Contains(header.Name, `\cnb\buildpacks\`) {
// Only for Windows, the first four headers are:
// - Files
// - Hives
// - Files/cnb
// - Files/cnb/buildpacks
// Skip over these until we find "Files/cnb/buildpacks/<buildpack-id>":
break
}
return nil, fmt.Errorf("failed to read first header '%s': %w", header.Name, err)
}

// the original version should be blank because the first header should look like "/cnb/buildpacks/<buildpack-id>"
// The header should look like "/cnb/buildpacks/<buildpack-id>"
// The version should be blank because the first header is missing <buildpack-version>.
origID, origVersion := parseBpIDAndVersion(header)
if origVersion != "" {
return nil, fmt.Errorf("first header '%s' contained unexpected version", header.Name)
}

if err := toNLayerTar(origID, origVersion, header, tr, tarCollection); err != nil {
if err := toNLayerTar(origID, origVersion, header, tr, tarCollection, forWindows); err != nil {
return nil, err
}

Expand All @@ -396,36 +402,14 @@ func ToNLayerTar(dest string, module BuildModule) ([]ModuleTar, error) {
return tarCollection.moduleTars(), nil
}

func toNLayerTar(origID, origVersion string, firstHeader *tar.Header, tr *tar.Reader, tc *moduleTarCollection) error {
func toNLayerTar(origID, origVersion string, firstHeader *tar.Header, tr *tar.Reader, tc *moduleTarCollection, forWindows bool) error {
toWrite := []*tar.Header{firstHeader}
if runtime.GOOS == "windows" && !strings.Contains(firstHeader.Name, "/cnb/buildpacks/") {
// On windows we can see the following headers before finding */cnb/buildpacks/<buildpack-id>/*
// Files
// Hives
// Files/cnb
// Files/cnb/buildpacks
for {
next, err := tr.Next()
if err != nil {
return err
}
if !strings.Contains(next.Name, "/cnb/buildpacks/") {
toWrite = append(toWrite, next)
} else {
origID, origVersion = parseBpIDAndVersion(next)
tc.buffer = toWrite
toWrite = []*tar.Header{next}
break
}
}
}

if origVersion == "" {
// the first header only contains the id - e.g., /cnb/buildpacks/<buildpack-id>,
// read the next header to get the version
secondHeader, err := tr.Next()
Fixed Show fixed Hide fixed

Check failure

Code scanning / CodeQL

Arbitrary file write during zip extraction ("zip slip")

Unsanitized archive entry, which may contain '..', is used in a [file system operation](1).
if err != nil {
return err
return fmt.Errorf("getting second header: %w; first header was %s", err, firstHeader.Name)
}
nextID, nextVersion := parseBpIDAndVersion(secondHeader)
if nextID != origID || nextVersion == "" {
Expand All @@ -440,16 +424,13 @@ func toNLayerTar(origID, origVersion string, firstHeader *tar.Header, tr *tar.Re
realFirstHeader.Name = filepath.ToSlash(filepath.Dir(firstHeader.Name))
toWrite = append([]*tar.Header{&realFirstHeader}, toWrite...)
}
if forWindows {
toWrite = append(windowsPreamble(), toWrite...)
}
mt, err := tc.get(origID, origVersion)
if err != nil {
return err
return fmt.Errorf("getting module from collection: %w", err)
}
for _, h := range tc.buffer {
if err := mt.writer.WriteHeader(h); err != nil {
return fmt.Errorf("failed to write header '%s': %w", h.Name, err)
}
}

for _, h := range toWrite {
if err := mt.writer.WriteHeader(h); err != nil {
return fmt.Errorf("failed to write header '%s': %w", h.Name, err)
Expand All @@ -463,31 +444,52 @@ func toNLayerTar(origID, origVersion string, firstHeader *tar.Header, tr *tar.Re
if err == io.EOF {
return nil
}
return err
return fmt.Errorf("getting next header: %w", err)
}
nextID, nextVersion := parseBpIDAndVersion(header)
if nextID != origID || nextVersion != origVersion {
// we found a new module, recurse
return toNLayerTar(nextID, nextVersion, header, tr, tc)
return toNLayerTar(nextID, nextVersion, header, tr, tc, forWindows)
}

err = mt.writer.WriteHeader(header)
if err != nil {
return errors.Wrapf(err, "failed to write header for '%s'", header.Name)
return fmt.Errorf("failed to write header for '%s': %w", header.Name, err)
}

buf, err := io.ReadAll(tr)
if err != nil {
return errors.Wrapf(err, "failed to read contents of '%s'", header.Name)
return fmt.Errorf("failed to read contents of '%s': %w", header.Name, err)
}

_, err = mt.writer.Write(buf)
if err != nil {
return errors.Wrapf(err, "failed to write contents to '%s'", header.Name)
return fmt.Errorf("failed to write contents to '%s': %w", header.Name, err)
}
}
}

func windowsPreamble() []*tar.Header {
return []*tar.Header{
{
Name: "Files",
Typeflag: tar.TypeDir,
},
{
Name: "Hives",
Typeflag: tar.TypeDir,
},
{
Name: "Files/cnb",
Typeflag: tar.TypeDir,
},
{
Name: "Files/cnb/buildpacks",
Typeflag: tar.TypeDir,
},
}
}

func parseBpIDAndVersion(hdr *tar.Header) (id, version string) {
// splitting "/cnb/buildpacks/{ID}/{version}/*" returns
// [0] = "" -> first element is empty or "Files" in windows
Expand All @@ -496,7 +498,7 @@ func parseBpIDAndVersion(hdr *tar.Header) (id, version string) {
// [3] = "{ID}"
// [4] = "{version}"
// ...
parts := strings.Split(hdr.Name, "/")
parts := strings.Split(strings.ReplaceAll(filepath.Clean(hdr.Name), `\`, `/`), `/`)
size := len(parts)
switch {
case size < 4:
Expand All @@ -510,7 +512,7 @@ func parseBpIDAndVersion(hdr *tar.Header) (id, version string) {
return id, version
}

func handleSingleOrEmptyModule(dest string, module BuildModule) ([]ModuleTar, error) {
func handleEmptyModule(dest string, module BuildModule) ([]ModuleTar, error) {
tarFile, err := ToLayerTar(dest, module)
if err != nil {
return nil, err
Expand Down Expand Up @@ -571,14 +573,12 @@ func newModuleTar(dest, id, version string) (moduleTar, error) {
type moduleTarCollection struct {
rootPath string
modules map[string]moduleTar
buffer []*tar.Header
}

func newModuleTarCollection(rootPath string) *moduleTarCollection {
return &moduleTarCollection{
rootPath: rootPath,
modules: map[string]moduleTar{},
buffer: []*tar.Header{},
}
}

Expand Down
4 changes: 0 additions & 4 deletions pkg/buildpack/buildpack_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -874,10 +874,6 @@ func (eb *errorBuildModule) Descriptor() buildpack.Descriptor {
return nil
}

func (eb *errorBuildModule) ContainsFlattenedModules() bool {
return eb.flattened
}

type expectedBuildpack struct {
id string
version string
Expand Down
Loading