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

(SIMP-10739) Fix auth issues #91

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

greatflyingsteve
Copy link

@greatflyingsteve greatflyingsteve commented Aug 12, 2022

The secrets file for an rsync module must be owned by the same user that the daemon is running as, but was owned by the user and group that the rsync module was using for file ownership. This fixes that.

Additionally, if auth_users or user_pass variables were not Undef but just empty, as would be expected if an iterable they were meant to be based on had been handled incorrectly by accident, the result was an entirely anonymous rsync module rather than an rsync module with no allowed users. In addition, if the user_pass parameter was given, a secrets file would be generated with valid contents, but the rsync module would not contain an auth users list or any reference to the secrets file, resulting in an anonymous-access rsync module if only one of the two available means of passing authentication data (but not both) was given. In this case the auth_users array was entirely ignored, meaning that it was impossible to pass a mix of users with generated passwords and users with specified passwords.

The auth_users param can now be either an Array of allowed users (with all passwords generated by simplib::passgen()) or a Hash of allowed user keys to their password values, and any passwords that are empty or undef will be generated using simplib::passgen(). This paves the way for entirely doing away with the user_pass array and using only one clear paramter for all user specification. Unit test coverage has also been significantly expanded, including tests for secrets file ownership when the section had a non-root uid/gid.

These changes have been validated by live tests on my own hardware. Hopefully the acceptance tests also work.

SIMP-10739 #close

The secrets file for an rsync module must be owned by the same user that
the daemon is running as, but was owned by the user and group that the
rsync module was using for file ownership.  This fixes that.
Additionally, if auth_users or user_pass variables were not Undef but
just empty, as would be expected if an iterable they were meant to be
based on had been handled incorrectly by accident, the result was an
entirely anonymous rsync module rather than an rsync module with no
allowed users.  In addition, if user_pass parameter was given, a secrets
file would be generated with valid contents, but the rsync module would
not contain an auth users list or any reference to the secrets file,
resulting in an anonymous-access rsync module if only one of the two
available means of passing authentication data (but not both) was given.
In this case the auth_users array was entirely ignored, meaning that it
was impossible to pass a mix of users with generated passwords and
users with specified passwords.  The auth_users param can now be either
an Array of allowed users (with all passwords generated by
simplib::passgen()) or a Hash of allowed user keys to their password
values, and any passwords that are empty or undef will be generated
using simplib::passgen().  This paves the way for entirely doing away
with the user_pass array and using only one clear paramter for all user
specification.  Unit test coverage has also been significantly expanded.
@op-ct op-ct changed the title Simp 10739 fix auth issues (SIMP-10739) fix auth issues Aug 12, 2022
@op-ct op-ct changed the title (SIMP-10739) fix auth issues (SIMP-10739) Fix auth issues Aug 12, 2022
Updated the acceptance tests (which I cannot run, so gosh I hope they
work) to use the newer convention for user/password specification.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

2 participants