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

Release 1.2.1 #32

Merged
merged 3 commits into from
Oct 8, 2021
Merged

Release 1.2.1 #32

merged 3 commits into from
Oct 8, 2021

Conversation

geofft
Copy link
Collaborator

@geofft geofft commented Oct 7, 2021

Bump nix dependency to 0.21.2 to pick up fix for nix-rust/nix#1541 aka RUSTSEC-2021-0119, memory corruption when using initgroups on a user in more than 16 groups

Also add an integration regression test.

@geofft geofft force-pushed the release-1.2.1 branch 5 times, most recently from 71f4120 to 4737815 Compare October 8, 2021 00:29
@geofft
Copy link
Collaborator Author

geofft commented Oct 8, 2021

OK, finally got the failing test working: https://github.com/twosigma/nsncd/runs/3833571070

I originally tried to be lazy and add it to /etc/group, but reliably detecting "did nsncd crash" ended up being hard (I think we were racing the nsncd process in the middle of crashing, or something?). So I instead just went with extending our existing whatami module to dynamically create am_i_foo users and make it so that this user is in 20 groups if the current process name is foo. So we check for the group membership of am_i_nsncd, which works iff we routed the lookup through nsncd.

@geofft geofft marked this pull request as ready for review October 8, 2021 00:43

enum nss_status
_nss_whatami_getpwnam_r(const char *name, struct passwd *result, char *buffer, size_t buflen, int *errnop)
{
if (strcmp(name, "whatami") == 0) {
if (strcmp(name, "whatami") == 0 || strncmp(name, "am_i_", 5) == 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

since we're accepting more names, should we set result->pw_name = name? it probably doesn't matter for this test but maybe we won't regret it later.

Copy link
Collaborator

@leifwalsh leifwalsh left a comment

Choose a reason for hiding this comment

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

I have...questions...about the implementation of _nss_whatami_initgroups_dyn, but also, if the test works it works and maybe that's okay.

Let me know if you'd like me to comment all over that function, or just merge.

@geofft
Copy link
Collaborator Author

geofft commented Oct 8, 2021

I'm going to merge (and do an internal release) because both of these are comments on the test code, but definitely happy to accept comments on the test code. I am not sure I have a lot of answers for questions though :)

@geofft geofft merged commit 10639ad into main Oct 8, 2021
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