sylabs / sif Goto Github PK

Currently, NewDescriptorInput does not place the resulting object in an object group. The vast majority of the case, objects are placed in the "default" group (group 1.) We should make this the default behaviour.

A new build constraint format was introduced in Go 1.17, and in the SIF codebase in #124. This format must exist alongside the old format (// +build) until the minimum Go version supported by SIF understands the new format.

Once Go 1.17 is the minimum supported version, remove the old format build constraints.

The golang.org/x/crypto/openpgp package is now deprecated (see https://golang.org/issue/44226 for details.) The integrity package currently uses this (both v1 and v2.) We should consider whether to migrate to a different package.

The Descriptor type currently uses type embedding to expose the fields/methods of rawDescriptor. The exported fields in rawDescriptor reveal implementation details that are best left unexported. This needs to be addressed prior to the v2 stable release.

At present, the descriptor capacity of an image created via CreateContainer or CreateContainerAtPath is hard coded to 48. There may be use cases where it makes sense to support more (or fewer) descriptors in a given image.

Add a CreateOpt to customize the descriptor capacity.

We should utilize the functional options pattern to provide a way to extend (*FileImage).SetPrimPart.

Passing zero to OptObjectAlignment() causes a panic to occur:

panic: runtime error: integer divide by zero [recovered]
        panic: runtime error: integer divide by zero

Fix the code such that OptObjectAlignment(0) does not panic, and behaves equivalently to OptObjectAlignment(1).

The sif package currently defines its own HashType. The integrity package translates crypto.Hash values to/from HashType values. We should treat HashType as an implementation detail, and switch to type crypto.Hash in the exported API.

Methods that return a time.Time in the SIF API currently return values with the location set to UTC. By contrast, the time package generally returns values in the local time.

I think it'd be better if we returned local time values, and left the decision of whether to convert to UTC to the caller.

func readableSize returns invalid human-readable sizes for values >= 1 PiB. For example:

readableSize(1) = "1"
readableSize(1024 * 1024 * 1024 * 1024) = "1 TiB"
readableSize(1024 * 1024 * 1024 * 1024 * 1024) = "1"

A SIF in excess of 1 PiB is certainly extreme. But we should probably handle this case gracefully.

We should utilize the functional options pattern to provide a way to extend (*FileImage).AddObject.

Reproducible builds are important as they provide an independently-verifiable path from source to binary code (ref).

The goreleaser configuration follows some of the recommendations for reproducible builds available here. We do not currently make use of the -trimpath flag, though arguably formal releases are always built by continuous integration, and thus use a consistent directory structure.

On the other hand the mage build is not currently reproducible, as demonstrated by running the build twice from the same commit:

$ mage install
$ sha256sum ~/go/bin/siftool 
d959a6f5bc4e17353df582ce60b5ae5cb2a50a85210c9e6f061a25774c54c348  /home/adam/go/bin/siftool
$ mage install
$ sha256sum ~/go/bin/siftool 
f6a1b211003de62897ebc0fb476694945a81b9bdfa31d027670caaf96047a200  /home/adam/go/bin/siftool

I think it might make sense to make the siftool build reproducible, and unify the build flags used between goreleaser and mage.

The integrity.VerifyCallback type passed to integrity.OptVerifyCallback uses the type integrity.VerifyResult interface.

As mentioned here, use of an interface type limits future extension, since an addition to the interface is a breaking change.

We should return a concrete type instead of an interface to support future extension of the API.

DeleteObject currently accepts a flags field that accepts DelZero and/or DelCompact options. We should add a type DeleteOpt that mirrors CreateOpt, for consistency and clarity.

The Short usage strings for siftool all begin with an upper case letter, but the practice in Cobra internally is to start these with lowercase (see completion command below). For consistency, we should update the siftool commands to be consistent.

$ siftool help
A set of commands are provided to display elements such as the SIF global
header, the data object descriptors and to dump data objects. It is also
possible to modify a SIF file via this tool via the add/del commands.

Usage:
  siftool [command]

Available Commands:
  add         Add a data object to a SIF file
  completion  generate the autocompletion script for the specified shell
  del         Delete a specified object descriptor and data from SIF file
  dump        Extract and output data objects from SIF files
  header      Display SIF global headers
  help        Help about any command
  info        Display detailed information of object descriptors
  list        List object descriptors from SIF files
  new         Create a new empty SIF image file
  setprim     Set primary system partition
  version     Print version information

Flags:
  -h, --help   help for siftool

Use "siftool [command] --help" for more information about a command.

sif.CreateContainer currently accepts a sif.CreateInfo:

With this pattern, users must understand the internals of SIF to effectively call CreateContainer. It would be better if these were defaulted, with potential overrides supplied via the functional options pattern. For example:

• Launchstr: default to sif.HdrLaunch
• Sifversion: default to sif.HdrVersion
• ID: default to a generated ID (perhaps the public API can avoid a UUID package at all, to prevent issues such as #7).
• InputDescr: default to empty list, allow DescriptorInput via functional option.

sif.CreateContainer and *FileImage.AddObject currently accept a sif.DescriptorInput:

This struct is unwieldy, requiring knowledge of the implementation to use properly. For example, it is possible to provide Fp, and/or Data, but it's not obvious in what circumstances each is required.

I suggest we move to a functional options approach to create a DescriptorInput, and make the following changes:

• Datatype: provide explicitly.
• Groupid: default to zero (unused), override via functional option.
• Link: default to zero (unused), override via functional option.
• Size: remove? This can be calculated while reading underlying data object.
• Alignment: default to memory page alignment (?), override via functional option.
• Fname: default to empty, override via functional option?
• Fp: provide explicitly?
• Data: remove, redundant with Fp?
• Image: remove, unused?
• Descr: remove, unused?
• Extra: default to empty, override via functional option for specific Datatype.

func readableSize does not correctly round size values. For example, readableSize(2047) = "1KB".

The V1 API had sif.LoadContainerReader(), which allowed a user to do some operations on a SIF file from a source that did not support all of ReadWriter interface.

There is one use case where this is particularly useful, and that is inspecting the header and/or descriptors at the start of a SIF file as it streams by (from an http request, for example.)

I think we could accommodate this by exposing a type similar to bytes.Buffer that implements ReadWriter interface. There may be other options worth exploring as well...

At present, images created via CreateContainer or CreateContainerAtPath are hard coded with the launch script "#!/usr/bin/env run-singularity\n". There may be use cases where it makes sense to use a different launch script, or disable the launch script altogether. For example, Singularity plugins utilize the SIF format, but the Singularity launch script is not appropriate for this use case.

Add a CreateOpt to customize the launch script.

We should automate release publishing using goreleaser.

There are a few formatting functions that print various information contained in a FileImage:

• func (*FileImage) FmtDescrInfo
• func (*FileImage) FmtDescrList
• func (*FileImage) FmtHeader

As far as I'm aware, these are used exclusively by the internal/app/siftool package. I believe we should mark these functions as deprecated in the v1 API, and make a breaking change to remove these functions from pkg/sif in the v2 API.

type ReadWriter is a large interface. For the v2 API, we should see if it can be trimmed down.

The v1 module has a direct dependency on an unmaintained module which has a documented weakness as described in GHSA-4gh8-x3vv-phhg. Although a workaround is available, it requires manual intervention.

Once v2 is released, we should include a module deprecation comment instructing users to upgrade to github.com/sylabs/sif/v2.

(Descriptor).GetData and (Descriptor).GetReader currently require a *FileImage to be passed to obtain data associated with a Descriptor. This is inconvenient as a user, because it requires a Descriptor and FileImage to be passed around.

We should embed the necessary information inside GetData and GetReader, and remove these parameters.

There are currently no unit tests for the internal/app/siftool package. Adding unit tests here will support more rapid development of this portion of the code.

CreateContainer creates a new SIF container at a specified path, and returns a *FileImage:

Unfortunately, the returned *FileImage is not usable, because its Fp field is closed by the time CreateContainer returns. Most operations on fimg return os.ErrClosed.

Use sebdah/goldie for golden file testing, to replace custom code that does the same thing:

Data objects in a SIF are aligned by default, which is achieved with zero or more padding bytes before the data object. The number of padding bytes can be calculated by subtracting Size from SizeWithPadding.

In any given SIF image, data objects are included following a data object descriptor section. You can see this using siftool:

$ siftool header test/images/one-group.sif 
Launch:   #!/usr/bin/env run-singularity
Version:  01
Arch:     386
ID:       6ecc76b7-a497-4f7f-9ebd-8da2a04c6be1
Ctime:    2020-05-22 19:30:59 +0000 UTC
Mtime:    2020-05-22 19:30:59 +0000 UTC
Dfree:    46
Dtotal:   48
Descoff:  4096
Descrlen: 28080
Dataoff:  32768
Datalen:  4100

In this image, the data object descriptor section starts at offset 4096, and the data objects start at offset 32768.

The data objects in this image are as follows:

$ siftool list test/images/one-group.sif 
Container id: 6ecc76b7-a497-4f7f-9ebd-8da2a04c6be1
Created on:   2020-05-22 19:30:59 +0000 UTC
Modified on:  2020-05-22 19:30:59 +0000 UTC
----------------------------------------------------
Descriptor list:
ID   |GROUP   |LINK    |SIF POSITION (start-end)  |TYPE
------------------------------------------------------------------------------
1    |1       |NONE    |32768-32772               |FS (Raw/System/386)
2    |1       |NONE    |36864-36868               |FS (Squashfs/*System/386)

By default, data objects are added with a default alignment of 4096. We can see that in this image, with objects starting at offset 32768 and 36864. In order to achieve this, both objects are preceded by padding. With a bit of math, we can see that the object descriptor section ends at offset Descoff + Descrlen = 4096 + 28080 = 32176. So, the padding is as follows:

• ID 1: 32768 - 32176 = 592 bytes of padding
• ID 2: 36864 - 32772 = 4092 bytes of padding

Unfortunately, this is not reflected in Size from SizeWithPadding fields of the first descriptor:

• ID 1: SizeWithPadding - Size = 4 - 4 = 0 bytes of padding
• ID 2: SizeWithPadding - Size = 4096 - 4 = 4092 bytes of padding

This actually doesn't appear to have any material impact on accessing data objects within a given SIF image, since only Offset and Size fields in the rawDescriptor are used for this purpose. Regardless, I believe we should fix the code to correctly record data object padding.

The ability to construct a SIF image in a reproducible way has been raised as a requirement. Specifically, what is meant by reproducible here is that, given identical input(s) to CreateContainer or CreateContainerAtPath, identical SIFs should be produced.

This is technically possible in the V2 API already, but requires use of several options:

• OptCreateWithID, to ensure the UUID in the header is consistent
• OptCreateWithTime, to ensure that the header timestamps are consistent
• OptObjectTime, to ensure that the object descriptor timestamps are consistent

While this works, it forces the user to select consistent ID and time values, which are not likely meaningful.

I propose we add an OptCreateReproducible that defaults the ID and time values to sensible defaults.

type Descriptor struct contains UID/GID values:

These are populated when a data object is created by calling user.Current:

UID/GID values are by their nature system specific, and thus these seem to be of limited utility in a container format that is meant to support mobility. I cannot find a single use of these in SingularityCE, nor any other projects using the github.com/sylabs/sif module.

I suggest we deprecate these in github.com/sylabs/sif. In github.com/sylabs/sif/v2, I suggest we set these values to zero, and remove them from the exported API.

SIF's Go Version Compatibility statement makes it clear that this module aims to support the two most recent stable versions of Go.

At the moment, the code in v1 and master can be built using Go 1.15:

go1.15.15 build ./...

But it actually can't be tested. In v1:

$ go1.15.15 test ./...
# github.com/sylabs/sif/pkg/sif [github.com/sylabs/sif/pkg/sif.test]
pkg/sif/create_test.go:61:12: undefined: os.CreateTemp
pkg/sif/create_test.go:160:12: undefined: os.CreateTemp
?       github.com/sylabs/sif/cmd/siftool       [no test files]
?       github.com/sylabs/sif/internal/app/siftool      [no test files]
ok      github.com/sylabs/sif/pkg/integrity     0.154s
FAIL    github.com/sylabs/sif/pkg/sif [build failed]
?       github.com/sylabs/sif/pkg/siftool       [no test files]
FAIL

And master:

$ go1.15.15 test ./...
# github.com/sylabs/sif/v2/internal/app/siftool [github.com/sylabs/sif/v2/internal/app/siftool.test]
internal/app/siftool/modif_test.go:24:13: undefined: os.CreateTemp
internal/app/siftool/modif_test.go:79:15: undefined: os.CreateTemp
internal/app/siftool/modif_test.go:104:13: undefined: os.CreateTemp
internal/app/siftool/modif_test.go:131:13: undefined: os.CreateTemp
# github.com/sylabs/sif/v2/pkg/siftool [github.com/sylabs/sif/v2/pkg/siftool.test]
pkg/siftool/new_test.go:24:15: undefined: os.CreateTemp
pkg/siftool/siftool_test.go:22:13: undefined: os.CreateTemp
# github.com/sylabs/sif/v2/pkg/sif [github.com/sylabs/sif/v2/pkg/sif.test]
pkg/sif/create_test.go:59:13: undefined: os.CreateTemp
pkg/sif/create_test.go:116:12: undefined: os.CreateTemp
# github.com/sylabs/sif/v2/pkg/integrity [github.com/sylabs/sif/v2/pkg/integrity.test]
pkg/integrity/metadata_test.go:26:12: undefined: os.ReadFile
pkg/integrity/metadata_test.go:70:15: undefined: os.ReadFile
pkg/integrity/metadata_test.go:77:15: undefined: os.ReadFile
pkg/integrity/sign_test.go:434:16: undefined: os.CreateTemp
?       github.com/sylabs/sif/v2/cmd/siftool    [no test files]
FAIL    github.com/sylabs/sif/v2/internal/app/siftool [build failed]
FAIL    github.com/sylabs/sif/v2/pkg/integrity [build failed]
FAIL    github.com/sylabs/sif/v2/pkg/sif [build failed]
FAIL    github.com/sylabs/sif/v2/pkg/siftool [build failed]
FAIL

To ensure it's always possible to build/test against the two most recent stable versions of Go, let's verify that explicitly in CI.

When a SIF is loaded from a non-local filesystem using LoadContainer or LoadContainerFp, a SIGBUS may occur when an I/O issue occurs in the underlying storage.

The reason for this is that the SIF module memory maps the file when loading with LoadContainer/LoadContainerFp. When combined with non-local storage, mmap is particularly risky. A SIGBUS can be raised any time an underlying I/O operation fails. Since accessing SIF images from non-local storage is a common use case in SingularityCE, we can't ignore this risk.

In theory, the SIGBUS could be caught with a signal handler, and a scheme could be devised to safely recover. In practice though, the github.com/sylabs/sif module is generally imported into applications which may already define a signal handler. Since these are global, there would need to be some scheme devised to coordinate, and in the end this would be quite cumbersome and clunky.

As an alternative, I propose we look at whether we really need to use memory mapping in the first place. On the surface, it wouldn't appear that we're getting much benefit from it.

There are instances where we need to search by default group. We should add a const for that.

Remove deprecated fields from type FileImage. Remove any other unnecessary fields, and provide accessors as necessary.

The README.md contains instructions on how to get the module:

As of v2.0.0-beta.1, that command returns an error:

$ go get github.com/sylabs/sif/v2
github.com/sylabs/sif/v2: no Go source files
package github.com/sylabs/sif/v2: build constraints exclude all Go files in /home/adam/src/sif

It looks like this will resolve itself in a future version of Go (golang/go#29268), but in the meantime we should fix our docs.

The primary purpose of the pkg/siftool package is to reduce CLI code duplication between cmd/siftool and the SingularityCE code base. This function currently exposes several functions that return a *cobra.Command. This API could be reduced to a single call that adds the SIF command hierarchy to an existing *cobra.Command. Something like:

func AddCommands(*cobra.Command, ...Option) error

The siftool header command includes a Magic field:

$ siftool header pkg/integrity/testdata/images/one-group.sif
Launch:   #!/usr/bin/env run-singularity

Magic:    SIF_MAGIC
Version:  01
Arch:     386
ID:       6ecc76b7-a497-4f7f-9ebd-8da2a04c6be1
Ctime:    2020-05-22 19:30:59 +0000 UTC
Mtime:    2020-05-22 19:30:59 +0000 UTC
Dfree:    46
Dtotal:   48
Descoff:  4096
Descrlen: 27KB
Dataoff:  32768
Datalen:  4KB

Magic is a low level implementation detail which must contain "SIF_MAGIC" for the SIF to be loaded in the first place, and cannot be set/modified by the user in the first place. I would argue it can be removed from the siftool header command output, which will also reduce the API surface.

The SIF code base currently has test images in two locations in the repo:

• pkg/integrity/testdata/images/
• pkg/sif/testdata/

It would be nice to centralize these, and provide code that can generate existing images, and generate new images as required. pkg/integrity/testdata/gen_sifs.go contains Go code to do this for the images in pkg/integrity, but it is skipped by tools such as go build and golangci-lint as it's in a testdata directory.

I propose we move this code to its own internal package, and place the resulting images nearby.

Several portions of the integrity package return object IDs rather than object descriptors. A quick survey of usage shows that in the vast majority of cases, the descriptor is immediately obtained by the caller. We should just return the descriptor directly, where applicable.

The repo currently uses a shell script to build that calls out to external tools on the build host (cat, echo, git, sed). We should consider using mage to build/install, so that all build dependencies are versioned in the go.mod.

The sif API currently has three different funcs to load a container:

• func LoadContainer(filename string, rdonly bool) (FileImage, error)
• func LoadContainerFp(fp ReadWriter, rdonly bool) (fimg FileImage, err error)
• func LoadContainerReader(b *bytes.Reader) (fimg FileImage, err error)

In conjunction with #75, we should consolidate these into a single function, and have it return a *FileImage to be consistent with CreateContainer.

(*Descriptor).GetReadSeeker currently returns an io.ReadSeeker, but current usage could be satisfied by the simpler io.Reader interface.

With the introduction of GetReader suggested by #14, I believe we should consider a breaking change to remove GetReadSeeker in the v2 API.

(*Descriptor).GetReadSeeker currently returns an io.ReadSeeker, but I cannot find any usage of it that would not be satisfied by the simpler io.Reader interface. In the interest of simplifying the underlying implementation in a major version bump, I think we should consider deprecating GetReadSeeker and introducing:

func (d *Descriptor) GetReader(fimg *FileImage) io.Reader

This change can be introduced in a minor version, which will provide time before the planned removal of GetReadSeeker in a v2 API.

Running go test causes an unclean working directory. If you start with a clean checkout:

$ git status
On branch master
Your branch is up to date with 'upstream/master'.

nothing to commit, working tree clean

Run the unit tests:

$ go test ./...
?       github.com/sylabs/sif/cmd/siftool       [no test files]
?       github.com/sylabs/sif/internal/app/siftool      [no test files]
ok      github.com/sylabs/sif/pkg/integrity     0.157s
ok      github.com/sylabs/sif/pkg/sif   0.047s
?       github.com/sylabs/sif/pkg/siftool       [no test files]

And pkg/sif/testdata/testcontainer.sif has been modified:

$ git status
On branch master
Your branch is up to date with 'upstream/master'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   pkg/sif/testdata/testcontainer.sif

no changes added to commit (use "git add" and/or "git commit -a")

We should ensure that the unit tests clean up after themselves.

The SIF implementation currently specifies string values for various CPU architectures:

Users of the github.com/sylabs/sif/pkg/sif package receive these via *Descriptor.GetArch, and pass them via *DescriptorInput.SetPartExtra.

The Go runtime exposes the current CPU architecture via runtime.GOARCH.

The SIF package provides sif.GetGoArch and sif.GetSIFArch for this purpose, but it would be easier for users if they could deal directly with GOARCH as exposed by the Go runtime.

I believe we should switch to use native Go architecture strings in relevant APIs, and remove GetGoArch/GetSIFArch from the API.

The cmd/siftool and pkg/siftool packages contain duplicate CLI implementations. We should DRY the code by refactoring cmd/siftool to use pkg/siftool.

(*Descriptor).GetData does not currently return a detailed error, instead returning a nil result when an error is encountered. This leads to code that does not explicitly handle errors at all, for example:

While (*Descriptor).GetReadSeeker can be used to obtain a detailed error, a io.ReadSeeker is less convenient than a []byte in some use cases.

I believe we should consider a breaking change in the function signature to:

func (*Descriptor) GetData(*FileImage) ([]byte, error)

Each Descriptor has an Extra field that includes extra metadata for certain data types. There are a number of methods to get individual values from this field:

• GetFormatType
• GetMessageType
• GetEntity
• GetEntityString
• GetHashType
• GetArch
• GetFsType
• GetPartType

These eight functions apply to three message types. In general, it is necessary to call more than one accessor for a given data type to do anything useful with the data object. This results in redundant parsing, and needlessly verbose code.

I propose consolidation of these into three methods in the v2 API:

• GetCryptoMessageMetadata: consolidates GetFormatType and GetMessageType
• GetPartitionMetadata: consolidates GetArch, GetFormatType, and GetMessageType
• GetSignatureMetadata: consolidates GetEntity, GetEntityString and GetHashType

The github.com/satori/go.uuid module used by this project does not appear to be actively maintained (ref).

We should consider switching to the github.com/gofrs/uuid package, or some other suitable alternative.