carvel-dev / vendir

Easy way to vendor portions of git repos, github releases, helm charts, docker image contents, etc. declaratively
https://carvel.dev/vendir
Apache License 2.0
279 stars 50 forks source link

`vendir` should retain directory permissions #186

Closed hoegaarden closed 1 year ago

hoegaarden commented 1 year ago

Describe the problem/challenge you have vendir by default seems to generate directories with 0700 permissions. When I want to allow other users access to the vendired directories, I can e.g. do a chmod 0750 on them. This however will be set back to 0700 on the next vendir sync again.

example shell session ```console $ cat vendir.yml apiVersion: vendir.k14s.io/v1alpha1 kind: Config directories: - path: vendir_vendor contents: - path: sub_dir http: url: https://github.com/vmware-tanzu/carvel-vendir $ vendir sync Fetching: vendir_vendor + sub_dir (http from https://github.com/vmware-tanzu/carvel-vendir) Lock config apiVersion: vendir.k14s.io/v1alpha1 directories: - contents: - http: {} path: sub_dir path: vendir_vendor kind: LockConfig Succeeded $ find . -type d -ls 133086 4 drwxr-xr-x 3 me me 4096 Sep 28 18:09 . 133089 4 drwx------ 3 me me 4096 Sep 28 18:09 ./vendir_vendor 133094 4 drwx------ 2 me me 4096 Sep 28 18:09 ./vendir_vendor/sub_dir $ chmod 0750 vendir_vendor{,/sub_dir} $ find . -type d -ls 133086 4 drwxr-xr-x 3 me me 4096 Sep 28 18:09 . 133089 4 drwxr-x--- 3 me me 4096 Sep 28 18:09 ./vendir_vendor 133094 4 drwxr-x--- 2 me me 4096 Sep 28 18:09 ./vendir_vendor/sub_dir $ vendir sync Fetching: vendir_vendor + sub_dir (http from https://github.com/vmware-tanzu/carvel-vendir) Lock config apiVersion: vendir.k14s.io/v1alpha1 directories: - contents: - http: {} path: sub_dir path: vendir_vendor kind: LockConfig Succeeded $ find . -type d -ls 133086 4 drwxr-xr-x 3 me me 4096 Sep 28 18:09 . 133091 4 drwx------ 3 me me 4096 Sep 28 18:09 ./vendir_vendor 133098 4 drwx------ 2 me me 4096 Sep 28 18:09 ./vendir_vendor/sub_dir $ ```

Describe the solution you'd like

I'd prefer vendir not to mess with my permissions on the directories; or, alternatively, allow me to configure the permissions for the directories and paths in the config.

Anything else you would like to add: -


Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

πŸ‘ "I would like to see this addressed as soon as possible" πŸ‘Ž "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help working on this issue.

joaopapereira commented 1 year ago

As @kumaritanushree pointed out in slack this sounds like an interesting property to add to the configuration. In your example it would be something like:

apiVersion: vendir.k14s.io/v1alpha1
kind: Config
directories:
  - path: vendir_vendor
    contents:
      - path: sub_dir
        permission: 755
        http:
          url: https://github.com/vmware-tanzu/carvel-vendir

Do you think this option would help in your use case?

hoegaarden commented 1 year ago

That would help, but I think we'd need the same for the "outer directory":

apiVersion: vendir.k14s.io/v1alpha1
kind: Config
directories:
  - path: vendir_vendor
    permission: 755  #! <-- this
    contents:
      - path: sub_dir
        permission: 755
        http:
          url: https://github.com/vmware-tanzu/carvel-vendir

Ideally, I think I'd (in addition?) want vendir to use the same permissions I've set to the directories, because I e.g. ran chmod after the first sync. Now I have no clue about the implementation and how feasible/doable that would be (e.g. collecting the permissions before running the sync / overwriting the directories) and one might also argue that this makes it "unpredictable". So ... :shrug:

joaopapereira commented 1 year ago

Make sense do you think it is useful to have permissions on both levels or only the main would be enough?

want vendir to use the same permissions I've set to the directories

Do you mean that if you set the permission on the top level to 744 all the files and directories underneath it would have the same 744 permission? If we do this the result will always be the same, so I would say you can predict the outcome. My main concern would be that this could remove executable bit from files that are executable, or would add write to a file that should not be writable and could cause problems to the applications that are going to consume this folder.

hoegaarden commented 1 year ago

think it is useful to have permissions on both levels or only the main would be enough?

I think there might be cases where you want them do be different. E.g. allowing access to one path for your unpriv'ed code scanner but not allowing access to the other path, because there are somewhat sensitive things in there? For that, I'd still need access to the directory be somewhat open, but have means to limit access to the path? E.g.:

vendir_vendor: 0755
  sub_dir_code: 0755
  sub_dir_somewhat_sensitive: 0700

Somewhat of a contrived example, but I could see something like that in "real life". :shrug: Would this be an option / good approach: permissions set on the directories.path should be the default on the directories.contents.path, with the option to explicitly set / overwrite them on the directories.contents.path individually, too.

apiVersion: vendir.k14s.io/v1alpha1
kind: Config
directories:
  - path: vendir_vendor
    permission: 754  #! <-- this
    contents:
      - path: sub_dir
        #! no permissions set, so will be defaulted to `0754` from it's parent
        http:
          url: https://github.com/vmware-tanzu/carvel-vendir
      - path: my_dirty_little_screts
        permissions: 0700  #! noone except myself should be allowed
        http:
          url: https://github.com/vmware-tanzu/carvel-vendir

want vendir to use the same permissions I've set to the directories

Do you mean that if you set the permission on the top level to 744 all the files and directories underneath it would have the same 744 permission? If we do this the result will always be the same, so I would say you can predict the outcome. My main concern would be that this could remove executable bit from files that are executable, or would add write to a file that should not be writable and could cause problems to the applications that are going to consume this folder.

What I meant is: If I do a chmod on vendir_vendor or vendir_vendor/sub_dir, vendir should keep those permissions. It would need to inspect those directories before setting up it's temporary directories on vendir sync, it would need to use those collected permissions for vendir_vendor and vendir_vendor/sub_dir. This means that runnning vendir with the same config on my machine and your machine might lead to different outcomes. So I guess the approach in the config file would be more predictable. FWIW: I don't want vendir to go in and do a "chmod -R ..." on everything. Permissions of the things it pulls in should probably be dictated by the dependency itself (e.g. if I remove the executable bit on a file in the git repo, it should also be removed on the file after I have pulled in the repo via vendir_sync.

Does that make somewhat sense?

joaopapereira commented 1 year ago

I think I understand your example for the multi-layer permissions and makes sense to me.

What I meant is: If I do a chmod on vendir_vendor or vendir_vendor/sub_dir, vendir should keep those permissions. It would need to inspect those directories before setting up it's temporary directories on vendir sync, it would need to use those collected permissions for vendir_vendor and vendir_vendor/sub_dir. This means that runnning vendir with the same config on my machine and your machine might lead to different outcomes. So I guess the approach in the config file would be more predictable. FWIW: I don't want vendir to go in and do a "chmod -R ..." on everything. Permissions of the things it pulls in should probably be dictated by the dependency itself (e.g. if I remove the executable bit on a file in the git repo, it should also be removed on the file after I have pulled in the repo via vendir_sync.

Does that make somewhat sense?

Let me see if I understand what you said here. You have 2 different use cases here:

Is my description of these 2 scenarios what you mentioned above?

Also, my assumption is that defining the permissions in the configuration will only affect the folder itself and the permission of the files underneath them would be kept as they were in the origin location.

hoegaarden commented 1 year ago

On your local machine you did a vendir sync and afterward, you decided to change permission on your file on disk. The next time you do a vendir sync you would like that file to keep the permission you set in your machine. Is this correct?

That would be one option, in addition or as alternative to having the permission settings in the config file.

Someone changes the permission of a file on a git repository, so the next time you do a vendir sync you want that new permission to be set on your local file correctly. Is this correct?

Yeah, kinda. But let's ignore that, because: that is already happening, to my knowledge (at least for the permission bits that git is tracking) and because in this issue I am solely concerned about the two outer directories (.directories[].path & .directories[].contents[].path). I just brought it up, to say "please do no't chmod -R the whole content". Again, I am only concerned about the outer directories and their permissions. Nothing else.

joaopapereira commented 1 year ago

On your local machine you did a vendir sync and afterward, you decided to change permission on your file on disk. The next time you do a vendir sync you would like that file to keep the permission you set in your machine. Is this correct?

That would be one option, in addition or as alternative to having the permission settings in the config file.

This use case I believe is somewhat complicated because vendir would have to keep the permission information when it downloaded the first time, and before sync happens would have to compare again to make sure it knows what changed so that it can apply after the sync. Not sure if this complexity makes sense for vendir. My assumption is that if you had to change the permissions locally maybe it would be better if it is done in the source so that no one else has to change it themselves.

About the second use case, indeed it is what happens right now.

Let me see if the following use cases match what we want from this feature

Use case 1: As a user when using the configuration in example 1 the folder vendir_vendor will be created with permission 754 and vendir_vendor/sub_dir as well. Nevertheless, vendir_vendor/sub_dir/path will keep the permissions that come from the origin.

# example 1
apiVersion: vendir.k14s.io/v1alpha1
kind: Config
directories:
  - path: vendir_vendor
    permission: 754  #! <-- this
    contents:
      - path: sub_dir
        http:
          url: https://github.com/vmware-tanzu/carvel-vendir

Use case 2: As a user when using the configuration in example 2 the folder vendir_vendor will be created with permission 700 and vendir_vendor/sub_dir will be created with permission 754. Nevertheless vendir_vendor/sub_dir/path will keep the permissions that come from the origin

# example 2
apiVersion: vendir.k14s.io/v1alpha1
kind: Config
directories:
  - path: vendir_vendor
    contents:
      - path: sub_dir
        permission: 754
        http:
          url: https://github.com/vmware-tanzu/carvel-vendir

Use case 3: As a user when using the configuration in example 3 the folder vendir_vendor will be created with permission 755 and vendir_vendor/sub_dir will be created with permission 754 and vendir_vendor/other_dir will be created with permission 755. Nevertheless vendir_vendor/sub_dir/path will keep the permissions that come from the origin

# example 3
apiVersion: vendir.k14s.io/v1alpha1
kind: Config
directories:
  - path: vendir_vendor
    permission: 755
    contents:
      - path: sub_dir
        permission: 754
        http:
          url: https://github.com/vmware-tanzu/carvel-vendir
      - path: other_dir
        http:
          url: https://github.com/vmware-tanzu/carvel-imgpkg

Use case 4: As a user when using the configuration in example 4 and previously I did chmod 0755 vendir_vendor/sub_dir when doing a new vendir sync the permission of the folder vendir_vendor/sub_dir will be reverted to 754.

# example 4
apiVersion: vendir.k14s.io/v1alpha1
kind: Config
directories:
  - path: vendir_vendor
    contents:
      - path: sub_dir
        permission: 754
        http:
          url: https://github.com/vmware-tanzu/carvel-vendir
github-actions[bot] commented 1 year ago

This issue is being marked as stale due to a long period of inactivity and will be closed in 5 days if there is no response.

joaopapereira commented 1 year ago

@hoegaarden do you think the above use cases match your needs?

hoegaarden commented 1 year ago

Sorry for the long pause. Got side track there just a little bit ... ;)

@hoegaarden do you think the above use cases match your needs?

Yeah, your use cases laid out in https://github.com/vmware-tanzu/carvel-vendir/issues/186#issuecomment-1267132937 are exactly what I am after. This is what I'd want but obviously wasn't able to clearly express the earlier convo.

Thanks!

joaopapereira commented 1 year ago

Cool.

Let us know if this is blocking your work and want us to try and prioritize this feature higher. Also if you are interested in giving it a go and sending a PR we are more than happy to review it or even help you create it.

hoegaarden commented 1 year ago

Also if you are interested in giving it a go and sending a PR we are more than happy to review

@joaopapereira please check #199 and let me know what you all think about it. Thanks!

hoegaarden commented 1 year ago

@joaopapereira With #199 being merged, could you also have a look at https://github.com/vmware-tanzu/carvel/pull/604? Once this is merged, I think this issue can be closed.

hoegaarden commented 1 year ago

/close as both #199 and https://github.com/vmware-tanzu/carvel/pull/604 are merged. Thanks!