containers / buildah

A tool that facilitates building OCI images.
https://buildah.io
Apache License 2.0
7.41k stars 782 forks source link

Containerfile `--chmod +x` not working in `podman` #4614

Closed WesselAtWork closed 1 year ago

WesselAtWork commented 1 year ago

/kind bug

Description

--chmod in the container files seems to only work with the chmod octal permissions (e.g. 755) and doesn't work with short chmod directives like +x.

Steps to reproduce the issue:

  1. Create Containerfile
    COPY --chmod='+x' src/somecmd /bin/somecmd
  2. Run podman build .

Describe the results you received:

Error: error building at STEP "COPY --chmod=+x src/somecmd /bin/somecmd": Error parsing chmod +x

Describe the results you expected:

Expected it to work similiar to docker with BUILDKIT enabled which builds the Containerfile correctly.

DOCKER_BUILDKIT=1 docker build . -f ./Containerfile

Version: 20.10.12

**Output of podman version

Client:       Podman Engine
Version:      4.4.1
API Version:  4.4.1
Go Version:   go1.19.5
Git Commit:   34e8f3933242f2e566bbbbf343cf69b7d506c1cf
Built:        Wed Feb  8 23:08:06 2023
OS/Arch:      windows/amd64

Server:       Podman Engine
Version:      4.2.1
API Version:  4.2.1
Go Version:   go1.18.5
Built:        Wed Sep  7 21:58:19 2022
OS/Arch:      linux/amd64
rhatdan commented 1 year ago

Does this syntax COPY --chmod='+x' src/somecmd /bin/somecmd Work on Docker?

WesselAtWork commented 1 year ago

Yes it does. You need to enable build-kit with DOCKER_BUILDKIT=1

For me this isn't such a big issue. Using 755 is acceptable. Did take my a little to figure out what was wrong though, my first Googling only showed using the +x. Eventually I noticed that 755 worked fine.

rhatdan commented 1 year ago

Interested in opening a PR for this?

@nalind @flouthoc Would this be easy to add?

WesselAtWork commented 1 year ago

Thinking about it, I would actually love to dig into the go code and learn how buildah functions! Investigating the chmod spec for the +x forms of the octal syntax also sounds cool! I can also imagine setting up test cases with bats to compare the --chmod functionality with the unix chmod command... Sadly I won't have too much free time in the next month or two.

For now I'll try to make some time but I won't be able to properly look at it until at least 2 months from now.

WesselAtWork commented 1 year ago

Probably involve creating a converter/checker type function to take a MaybeChmod and interpret the content. Should then return what os.FileMode() expects to see.

https://github.com/containers/buildah/blob/02db353d2beb30a28340cb7267f10adacdca9a70/add.go#L279-L286

I'm thinking:

 if options.Chmod != "" { 
    p, err := lib.ParseChmod(options.Chmod) //lib placholder
    if err != nil { 
        return fmt.Errorf("parsing chmod %q: %w", options.Chmod, err) 
    } 
    perm := os.FileMode(p)
    chmodDirsFiles = &perm
 }

The only issue then is getting the flag spec of what is allowed and not. Having a little trouble finding the full scope though.

WesselAtWork commented 1 year ago

Maybe drop it down a step?

if options.Chmod != "" { 
  perm, err := lib.ParseChmod(options.Chmod) //lib placholder
  if err != nil { 
    return fmt.Errorf("parsing chmod %q: %w", options.Chmod, err) 
  } 
  chmodDirsFiles = &perm
}

Then ParseChmod would be something like:

func ParseChmod(maybeChmod string) (FileMode, error) {
  //parse parse parse...
  return os.FileMode(p), nil
}

Not sure what is a better.

WesselAtWork commented 1 year ago

I think I ran into a limitation of the stdlib

I found a very helpful spec doc for the chmod formats. The different formats are called Symbolic and Absolute. Absolute is the 0755 format and Symbolic is the uo+rw format.

The symbolic format has 3 directives:

Meaning you can specify things like:

The problem is that os.FileMode only deals in the absolute forms.

No biggy. That just means we have to stat the file beforehand and manually add or subtract the relevant bits.

Problem is that most things seem to assume Absolute mode.

For instance: here where you are setting the mode in the header of a tar archive for a remote source:

https://github.com/containers/buildah/blob/02db353d2beb30a28340cb7267f10adacdca9a70/add.go#L143-L154

The documentation for tar.Header doesn't mention adding or subtracting from the mode specified, so I have to assume it's absolute.

What makes this even more annoying is that the tar command has a --mode flag that accepts Symbolic form chmod arguments.

Might be possible to do something with the reader.Next() ?

For forward compatibility, users that retrieve a Header from Reader.Next, mutate it in some ways, and then pass it back to Writer.WriteHeader should do so by creating a new Header and copying the fields that they are interested in preserving.


The local case I am unsure. https://github.com/containers/buildah/blob/02db353d2beb30a28340cb7267f10adacdca9a70/add.go#L510-L523

The copier code is big (I'd need more time to dig into it) but it looks to be possible (maybe). For instance in the directory case:

https://github.com/containers/buildah/blob/02db353d2beb30a28340cb7267f10adacdca9a70/copier/copier.go#L1572-L1584

You are already stat-ing the directory so applying additions and subtractions shouldn't be too difficult. e.g.

mode := ChmodVariable.apply(st.Mode())

This would require an non-insignificant amount of refactoring as far as I can tell. Everywhere you are passing a naked os.FileMode or uint64/unit32 for the permisison mode, you'd need to replace with a new type. A custom Chmod type, that has some kind of .apply() method you could use to "apply" it to another mode. This would probably require special handling in the case of directories. You'd have to manually iterate through the directory and stat the files to individually apply the modes.


If you know more about the code base or I missed something with the file mode please let me know. It doesn't feel like it should be this difficult.

nalind commented 1 year ago

It might not reject the syntax, but are we sure it works as intended? When I try it with 20.10.23 locally, the builder seems to just ignore "--chmod=+x" or "--chmod=u+wx".

github-actions[bot] commented 1 year ago

A friendly reminder that this issue had no activity for 30 days.

rhatdan commented 1 year ago

Since we heard no further feedback and this does not seem to work on Docker, closing.

WesselAtWork commented 1 year ago

Hi all.

Just wanted to apologise for letting this die, got busy with other things.

Also wow that was a miss on my part. I just half checked that docker "worked" with the --chmod=+x flag, missing that, without specifying anything, the file I used already had eXecute permissions. Trying with -x or with other combinations doesn't work and I should have maybe been a little more through.

Additionally, it seems docker with buildkit is actually ignoring the flag

CACHED [2/2] COPY --chmod=+w ./sometext.sh /bin/somecmd 

here I have changed from +x to +w and it used the cached layer which is not how that is supposed to work.

I've been trying to understand how I even though this was possible in docker in the first place. I think I found a SO that suggested using it but googling I can't find it again. It seems to just have popped into my head and I was convinced it was possible :P.

Feel free to open again if docker changes in the future.

WesselAtWork commented 1 year ago

Oh I actually did screw around with a helper library. GOLANG doesn't natively understand Chmod in the symbolic format and I couldn't find any "off the shelf" libraries to help (historically you should take this with a grain of salt :P). Feel free to look at this if you need to implement in the future.

I haven't really worked with GOLANG before so I apologise for the what-ever-the-heck pattern I tried to follow here. The most usefull thing from this should be the REGEX and the documentation which was the most difficult to obtain.

REGEX (single): ^(u?g?o?|a)([-+=])(\b)(r?w?x?X?s?t?u?g?o?)$ REGEX (mutiple): ^((u?g?o?|a)([-+=])(\b)(r?w?x?X?s?t?u?g?o?),?)+$

CHMOD documentation

chmod.go ```go package libs import ( "fmt" "os" "regexp" "strconv" ) // Intention: // The idea is that a MaybeChmod is user input or input that you are unsure about. // You can convert that UNSURE input to a SURE unit or properly handle it if it isn't. // The VALID forms can be used by developers but are unchecked and will break stuff if you give it bad input. type ( MaybeChmod string //Represents an string that COULD BE a valid chmod form ChmodAbsoluteForm uint32 //Represents a VALID chmod absolute uint32 `Parameter` ChmodSymbolicForm string //Represents a VALID chmod symbolic string `Parameter` ChmodForm interface { Chmod() Chmod //turns a VALID chmod form into it's workable object } //Represents a VALID chmod form Chmod interface { Apply(c ChmodAbsoluteForm) uint32 //applies chmod description ApplyToInt16(c uint16) uint16 //applies chmod description to [int16] ApplyToUInt32(c uint32) uint32 //applies chmod description to [unit32] ApplyToUnint64(c uint64) uint64 //applies chmod description to [unit64] ApplyToFileMode(c os.FileMode) os.FileMode //applies chmod description to [filemode] String() string //Returns FRENDLY representation string } //Represents a chmod unit ) type ChmodAbsolute struct { ChmodAbsoluteForm } // VALID Absolute Chmod unit type ChmodSybolicMask uint32 // Symbolic Mask Parameter type ChmodSymbolic struct { ChmodSymbolicForm ChmodSybolicAddition ChmodSybolicMask ChmodSybolicSubtraction ChmodSybolicMask ChmodSybolicAbsolute ChmodSybolicMask } // VALID Symbolic Chmod unit const ( ChmodMaxAbsoluteValue uint32 = 0o7777 //maximum allowed chmod absolute value ChmodMinAbsoluteValue uint32 = 0o0000 //minimum allowed chmod absolute value ChmodParameterRegex = `^(u?g?o?|a)([-+=])(\b)(r?w?x?X?s?t?u?g?o?)$` ChmodParametersRegex = `^((u?g?o?|a)([-+=])(\b)(r?w?x?X?s?t?u?g?o?),?)+$` ChmodSymbolicDoc = "README.md" //where to find information on the symbolic form ) var ( ChmodParameterPatern = regexp.MustCompile(ChmodParametersRegex) //should match a single parameter set; where the `,` have been split out ChmodParametersPatern = regexp.MustCompile(ChmodParametersRegex) //should match full list of parameters ) // https://ss64.com/osx/chmod.html // SYMBOLIC MODE MODE /* # Form `[who...][[+-=][perm...]...][,...]` who - a combination of the letters `ugoa` controls which users' access to the file will be changed: u The User who owns it g other users in the file's Group o Other users not in the file's group a All users, this is equivalent to (ugo) If none of these are given, the effect is as if (a) were given, but bits that are set in the umask are not affected. +-= The operator '+' causes the permissions selected to be added to the existing permissions of each file; '-' causes them to be removed; and '=' causes them to be the only permissions that the file has. if = is specified with no who then all (owner, group and other) will be cleared. perm - The letters 'rwxXstugo' select the new permissions for the affected users: r Read w Write x Execute/search (or access for directories) X Execute/search only if the file is a directory or already has execute permission for some user s Set user or group ID on execution t The sticky bit u User permission g Group permission o Other permission (users not in the file's group) # Symbolic Mode Examples Deny execute permission to everyone: $ `chmod a-x file` Allow read permission to everyone: $ `chmod a+r file` Make a file readable and writable by the group and others: $ `chmod go+rw file` Make a shell script executable by the user/owner $ `chmod u+x myscript.sh` Allow everyone to read, write, and execute the file and turn on the set group-ID: $ `chmod =rwx,g+s file` */ // NUMERIC MODE /* From one to four octal digits [oooo] Any omitted digits are assumed to be leading zeros. [0ooo] | Bit Name | Position | Valid Values | Directory Context | |----|----|----|----| | Atribute Bit | [x000] | 4 (User ID), 2 (Group ID), 1 (Sticky) | None | | User (Owner) Bit | [0x00] | 4 (Read), 2 (Write), 1 (Exec) | 1 (Owner User Search) | | Group Bit | [00x0] | 4 (Read), 2 (Write), 1 (Exec) | 1 (Group Search) | | Others Bit | [000x] | 4 (Read), 2 (Write), 1 (Exec) | 1 (Others Search) | The octal (0-7) value is calculated by adding up the values for each digit User (rwx) = 4+2+1 = 7 Group (rx) = 4+1 = 5 World (rx) = 4+1 = 5 chmod mode = 0755 # Numeric Mode Examples: Allow read permission to everyone: $ chmod 444 file Allow everyone to read, and execute the file: $ chmod 755 file Make a file readable by anyone and writable by the owner only: $ chmod 644 file Make a file readable and writable by the group and others: $ chmod 066 file */ //----[MaybeChmod]---- // tests if [MaybeChmod] is empty func (mch MaybeChmod) isEmpty() bool { return mch == "" } // converts a [MaybeChmod] to the [ChmodSymbolicForm] func (mch MaybeChmod) chmodSymbolicForm() (ChmodForm, error) { //addtional validation? if !ChmodParametersPatern.MatchString(string(mch)) { return nil, fmt.Errorf("value '%s' was not in the correct chmod-symbolic format. Please see %s for information on the correct format", mch, ChmodSymbolicDoc) } return ChmodSymbolicForm(mch), nil } // tests: // succeed: u+r ; +r ; +rw ; a+r ; +u ; +rwx ; u+x,g+r,o+x ; u-x ; a=rwx ; +rwxXstugo (unsure) // fail: + ; ua+r (all was spesified) ; r ; w ; u+lol ; a+ ; lol+x ; uu+x ; g+rr // converts a [MaybeChmod] to the [ChmodAbsoluteForm] func (mch MaybeChmod) chmodAbsoluteForm() (ChmodForm, error) { //addtional validation? ui64, err := strconv.ParseUint(string(mch), 8, 32) if err != nil { return nil, fmt.Errorf("absolute: %w", err) //TODO: what is the top level tag? } ui32 := uint32(ui64) if !(ui32 <= ChmodMaxAbsoluteValue && ui32 >= ChmodMinAbsoluteValue) { //min will never be false. put here for completeness return nil, fmt.Errorf("absolute: value is not in valid range: %04o <= '%04o' >= %04o", ChmodMaxAbsoluteValue, ui32, ChmodMinAbsoluteValue) } return ChmodAbsoluteForm(ui32), nil } // might return chmod form func (mch MaybeChmod) MaybeChmodForm() (ChmodForm, error) { if mch.isEmpty() { return nil, fmt.Errorf("maybeChmod.MaybeChmod: empty value") } chmodform, aerr := mch.chmodAbsoluteForm() if chmodform != nil { //if we have a return, it means the input was in symbolic format return chmodform, nil } chmodform, serr := mch.chmodSymbolicForm() if chmodform != nil { //if we have a return, it means the input was in absolute format return chmodform, nil } //if we get here it means it was in neither format. we don't know what the problem is, so we send back both errors err := fmt.Errorf("[%v, %v]", serr, aerr) return nil, fmt.Errorf("maybeChmod.MaybeChmod: %w", err) } // might return chmod func (mch MaybeChmod) MaybeChmod() (Chmod, error) { chmodform, err := mch.MaybeChmodForm() if err != nil { return nil, fmt.Errorf("MaybeChmod: %w", err) } return chmodform.Chmod(), nil } //----[ChmodAbsoluteForm]---- func (caf ChmodAbsoluteForm) FileMode() os.FileMode { return os.FileMode(caf) } func (caf ChmodAbsoluteForm) Chmod() Chmod { return ChmodAbsolute{ ChmodAbsoluteForm: caf, } } // should print the friendly representation of the absolute chmod 4chars leftpadded with 0 func (caf ChmodAbsoluteForm) String() string { return fmt.Sprintf("%04o", caf) } //----[ChmodSymbolicForm]---- func (csf ChmodSymbolicForm) Chmod() Chmod { //TODO: parse parse parse return ChmodSymbolic{ ChmodSymbolicForm: csf, } } func (csf ChmodSymbolicForm) String() string { return string(csf) } //----[ChmodAbsolute]---- // should always overide func (ca ChmodAbsolute) Apply(c ChmodAbsoluteForm) uint32 { return uint32(ca.ChmodAbsoluteForm) } func (ca ChmodAbsolute) ApplyToInt16(c uint16) uint16 { return uint16(ca.ChmodAbsoluteForm) } func (ca ChmodAbsolute) ApplyToUInt32(c uint32) uint32 { return uint32(ca.ChmodAbsoluteForm) } func (ca ChmodAbsolute) ApplyToUnint64(c uint64) uint64 { return uint64(ca.ChmodAbsoluteForm) } func (ca ChmodAbsolute) ApplyToFileMode(c os.FileMode) os.FileMode { return ca.ChmodAbsoluteForm.FileMode() } func (ca ChmodAbsolute) String() string { return ca.ChmodAbsoluteForm.String() } //----[ChmodMask]---- func (cm ChmodSybolicMask) String() string { return fmt.Sprintf("%04o", cm) } func (cm ChmodSybolicMask) PrettyString() string { //todo: pretty string return cm.String() } //----[ChmodSymbolic]---- // should "apply" (overide, add, subtract) the different parts of the form to the given value func (cs ChmodSymbolic) Apply(c ChmodAbsoluteForm) uint32 { ret := uint32(c) return cs.ApplyToUInt32(ret) } func (cs ChmodSymbolic) ApplyToInt16(c uint16) uint16 { ret := c ret |= uint16(cs.ChmodSybolicAddition) ret &^= uint16(cs.ChmodSybolicSubtraction) ret |= uint16(cs.ChmodSybolicAbsolute) //what is the order of operations here? does absolute overide subtractions? return uint16(ret) } func (cs ChmodSymbolic) ApplyToUInt32(c uint32) uint32 { ret := c ret |= uint32(cs.ChmodSybolicAddition) ret &^= uint32(cs.ChmodSybolicSubtraction) ret |= uint32(cs.ChmodSybolicAbsolute) //what is the order of operations here? does absolute overide subtractions? return uint32(ret) } func (cs ChmodSymbolic) ApplyToUnint64(c uint64) uint64 { ret := c ret |= uint64(cs.ChmodSybolicAddition) ret &^= uint64(cs.ChmodSybolicSubtraction) ret |= uint64(cs.ChmodSybolicAbsolute) //what is the order of operations here? does absolute overide subtractions? return uint64(ret) } func (cs ChmodSymbolic) ApplyToFileMode(c os.FileMode) os.FileMode { ui32 := uint32(c) applied := cs.ApplyToUInt32(ui32) return os.FileMode(applied) } func (cs ChmodSymbolic) String() string { return cs.ChmodSymbolicForm.String() } ```

I think I was going to test out the bitwise operations but never got that far. The apply stuff is also very very WIP and actually wrong and I should stay in the model as long as possible, this was done quickly to test things out and should definitely be changed if you want to use this.

This was WIP so take it with a grain of salt.

I think you use this by:

chm, err := MaybeChmod("some input")
if err != null {
  //tell someone they did it worng
}
//chmod is now valid and can be used without checking
fmt.printf("%s", chm.String)
fmt.printf("%s", chm.Apply( ChmodAbsoluteForm(0o0555) )