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

Add base-passwd and generate certificate bundle #3

Merged
merged 2 commits into from
Jun 17, 2022

Conversation

woky
Copy link

@woky woky commented Jun 14, 2022

No description provided.

Copy link
Collaborator

@cjdcordeiro cjdcordeiro left a comment

Choose a reason for hiding this comment

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

it lgtm.

I have 2 non-blocking comments though:

  1. (see inline comment about var names)
  2. in the base-passwd DEB, it actually is a preinst script who populates those files in /etc, manually (like printing a text snippet). The files in /usr/share are only used in postinst, by what I assume is a sort of backup procedure. So the comment/question is: aren't we being untruthful to the DEB by ignoring the preinst?

slices/base-passwd.yaml Outdated Show resolved Hide resolved
@woky
Copy link
Author

woky commented Jun 14, 2022

in the base-passwd DEB, it actually is a preinst script who populates those files in /etc, manually (like printing a text snippet). The files in /usr/share are only used in postinst, by what I assume is a sort of backup procedure. So the comment/question is: aren't we being untruthful to the DEB by ignoring the preinst?

@cjdcordeiro We are still truthful, see:

The preinst string is created from passwd.master.

That being said, I don't think it makes sense to be truthful here. I don't think most of these make sense in 99% of container images we wish to have:

...
games:*:5:60:games:/usr/games:/usr/sbin/nologin
man:*:6:12:man:/var/cache/man:/usr/sbin/nologin
lp:*:7:7:lp:/var/spool/lpd:/usr/sbin/nologin
mail:*:8:8:mail:/var/mail:/usr/sbin/nologin
news:*:9:9:news:/var/spool/news:/usr/sbin/nologin
uucp:*:10:10:uucp:/var/spool/uucp:/usr/sbin/nologin
proxy:*:13:13:proxy:/bin:/usr/sbin/nologin
...

FYI: I'm not sure why the ca-certificates script now works but I think it's because of the relative output dir issue that was just fixed in chisel. I may have edited the command line I put into the issue where I originally used relative path.

@cjdcordeiro
Copy link
Collaborator

thanks for the info on the base-passwd's preinst 👍

no further comments then.

@woky woky assigned woky and niemeyer and unassigned niemeyer and woky Jun 16, 2022
@woky
Copy link
Author

woky commented Jun 17, 2022

@niemeyer could you please merge this one if you have no objections?

Copy link

@niemeyer niemeyer left a comment

Choose a reason for hiding this comment

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

Just some nitpicking, and one relevant question:

@@ -28,7 +28,7 @@ slices:

tmp:
contents:
/tmp/:
/tmp/: { mode: 01777 }

Choose a reason for hiding this comment

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

This shouldn't be required, as it's the actual mode of the underlying directory. I think we have a bug about umask getting in the way during creation. Is that what you're seeing? Why was the explicit mode added here?

Copy link
Author

Choose a reason for hiding this comment

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

I though there wasn't bug but that the directory had wrong permissions in the archive since the postinst script explicitly adjusted its permissions. But I've just checked it's correct and indeed it sounds like a bug with umask handling. Reported here: canonical/chisel#9

/usr/share/ca-certificates/mozilla/*: {until: mutate}
mutate: |
certs_dir = "/usr/share/ca-certificates/mozilla/"
certs = [ content.read(certs_dir + path) for path in content.list(certs_dir) ]

Choose a reason for hiding this comment

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

Nitpick: I think it's more conventional in Python to not have spaces padding the edges of list comprehensions. Is that true? I don't mind too much the details of our conventions, as long as it's consistent, but since we're just starting we should try to borrow some typical Python style.

Copy link
Author

Choose a reason for hiding this comment

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

Ack. Fixed.

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.

3 participants