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

Fixup update framework permissions #123

Merged
merged 1 commit into from
May 13, 2022
Merged

Fixup update framework permissions #123

merged 1 commit into from
May 13, 2022

Conversation

mudler
Copy link
Contributor

@mudler mudler commented May 13, 2022

Otherwise fails for the permissions while committing changes

Signed-off-by: Ettore Di Giacinto edigiacinto@suse.com

Signed-off-by: Ettore Di Giacinto <edigiacinto@suse.com>
@mudler mudler enabled auto-merge (squash) May 13, 2022 16:17
@mudler mudler disabled auto-merge May 13, 2022 16:27
@mudler mudler merged commit 4239e72 into master May 13, 2022
@mudler mudler deleted the fixup_framework_update branch May 13, 2022 16:28
@mudler
Copy link
Contributor Author

mudler commented May 13, 2022

blargh still fails..locally didn't!?!

@Itxaka
Copy link
Contributor

Itxaka commented May 13, 2022

can reproduce:

# itxaka @ zeus in ~/projects/os2 on git:master x [19:26:33] 
$ ls -ltra framework/cos/system/oem             
ls: cannot access 'framework/cos/system/oem/.': Permission denied
ls: cannot access 'framework/cos/system/oem/..': Permission denied
ls: cannot access 'framework/cos/system/oem/07_live.yaml': Permission denied
ls: cannot access 'framework/cos/system/oem/06_recovery.yaml': Permission denied
ls: cannot access 'framework/cos/system/oem/05_network.yaml': Permission denied
ls: cannot access 'framework/cos/system/oem/08_boot_assessment.yaml': Permission denied
total 0
-????????? ? ? ? ?            ? 08_boot_assessment.yaml
-????????? ? ? ? ?            ? 07_live.yaml
-????????? ? ? ? ?            ? 06_recovery.yaml
-????????? ? ? ? ?            ? 05_network.yaml
d????????? ? ? ? ?            ? ..
d????????? ? ? ? ?            ? .

💩

weird, becuase it seems to have the proper perms??

$ sudo ls -ltra framework/cos/system/oem             
total 20
-rw------- 1 itxaka users 1063 May  5 18:54 06_recovery.yaml
-rw------- 1 itxaka users 5314 May  5 18:58 08_boot_assessment.yaml
drwxr-xr-x 1 itxaka users    6 May  5 18:58 ..
drw------- 1 itxaka users  132 May  5 18:58 .
-rw------- 1 itxaka users  603 May  5 19:00 07_live.yaml
-rw------- 1 itxaka users  581 May  5 19:00 05_network.yaml

@Itxaka
Copy link
Contributor

Itxaka commented May 13, 2022

goddammit, oem has no x attribute for some reason:

drw------- 1 itxaka users 132 May  5 18:58 oem

once given +x attributes, then you can list and add those files to git ¬_¬ I wonder if the package is broken?

@Itxaka
Copy link
Contributor

Itxaka commented May 13, 2022

yep, comes from the packages. I guess oem should have at least x permissions for the user/group so we can list files on it

@mudler
Copy link
Contributor Author

mudler commented May 13, 2022

perm wise that looks good, ideally we want only root to be able to look at those, which is inline with the perms that are coming from the package.. it is so annoying that as root you can add them that easily :D

@Itxaka
Copy link
Contributor

Itxaka commented May 13, 2022

No, it's wrong. It a dir doesn't have the execute permission for at least the user, then you can't ls its contents, can you? Compare it with the /system dir that gets extracted, that has the proper perms so you can list whatever is in there. So if you want to access the files on a directory, no matter if those files are 777, you won't get to access them if the dir they are in doesn't have the execute perm. So I'm guessing git is trying to list them and failing to do so, hence the failure to create a pr. Adding the +x perm to this dir before the PR is a must.

@Itxaka
Copy link
Contributor

Itxaka commented May 13, 2022

Damn, sorry for the text block, writing from a phone sucks lol

@mudler
Copy link
Contributor Author

mudler commented May 13, 2022

No, it's wrong. It a dir doesn't have the execute permission for at least the user, then you can't ls its contents, can you? Compare it with the /system dir that gets extracted, that has the proper perms so you can list whatever is in there. So if you want to access the files on a directory, no matter if those files are 777, you won't get to access them if the dir they are in doesn't have the execute perm. So I'm guessing git is trying to list them and failing to do so, hence the failure to create a pr. Adding the +x perm to this dir before the PR is a must.

yeap I see this, but, on the other hand this is completely acceptable from a system perspective. Do we want users beside root to list those content? I'm just not sure if adding +x here is the correct thing to do

@mudler
Copy link
Contributor Author

mudler commented May 13, 2022

@mudler
Copy link
Contributor Author

mudler commented May 13, 2022

so it looks like imho, the whole thing should have been:

  • 600 for the files INSIDE /system/oem/*
  • 700 for the folder

but on the spec I just see chmod -R 600, that might be slipped in. wdyt @Itxaka ?

@Itxaka
Copy link
Contributor

Itxaka commented May 13, 2022

Yep that seems right, I mean if we got read perms to the sir, we should have permission to enter IMHO, then file permissions should make sure that only owner or owner+group can read them

mudler pushed a commit that referenced this pull request Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants