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

Change cupsd.conf to force authentication when accessing administration #518

Merged
merged 1 commit into from
Dec 7, 2022

Conversation

AZero13
Copy link
Contributor

@AZero13 AZero13 commented Nov 3, 2022

With the current default configuration, someone can request to Find New Printers without being authenticated and with no way to be authenticated. Without authentication cups does not detects printers on the network. The person might not realize that it's because authentication is missing. It seems natural to always offer to authenticate when accessing the administration page.

Co-Authored-By: Dominic Mayers dominic.mayers@meditationstudies.org

@AZero13 AZero13 force-pushed the admin branch 2 times, most recently from 948af0b to b1b90de Compare November 9, 2022 17:48
@AZero13 AZero13 force-pushed the admin branch 2 times, most recently from 18d108a to 806d72a Compare November 17, 2022 14:25
@AZero13
Copy link
Contributor Author

AZero13 commented Nov 17, 2022

@michaelrsweet Could we merge this one too please

@michaelrsweet
Copy link
Member

@AtariDreams I'm not the only developer for the CUPS project, and in particular @zdohnal is the release manager for the CUPS 2.4.x series... As he also works for a Linux distribution, I'd like him to review this PR and make sure it doesn't cause problems.

@AZero13
Copy link
Contributor Author

AZero13 commented Nov 17, 2022

Sure thing.

tillkamppeter
tillkamppeter previously approved these changes Nov 17, 2022
@AZero13 AZero13 force-pushed the admin branch 5 times, most recently from 89623f1 to 6eb8c2f Compare November 26, 2022 17:50
@AZero13 AZero13 force-pushed the admin branch 5 times, most recently from 637adda to 7daf3b8 Compare December 2, 2022 13:51
With the current default configuration, anyone can request to Find New Printers without being authenticated and with no way to be authenticated. Without authentication cups does not detects printers on the network. The person might not realize that it's because authentication is missing. It seems natural to always offer to authenticate when accessing the administration page.

Co-Authored-By: Dominic Mayers <dominic.mayers@meditationstudies.org>
@AZero13
Copy link
Contributor Author

AZero13 commented Dec 6, 2022

@zdohnal Thoughts?

@AZero13
Copy link
Contributor Author

AZero13 commented Dec 6, 2022

@michaelrsweet Does this mean you can merge this PR or?

@zdohnal
Copy link
Member

zdohnal commented Dec 7, 2022

Hi all,

I'm still cleaning up things in Fedora/RHEL after my sickness, but I briefly looked into this PR.

In general any change in configuration file is bigger or smaller problem for migrations (either between single CUPS version updates or between OS version's migrations), especially ones which depend on being in a specific scope in the config. Because the proposed change is of this kind, I would prefer the same behavior as Add Printer has - the user is asked for authentication once he clicks on the button Add Printer (but in our case he would click on Find New Printers).

IIUC this authentication prompt is brought by cupsGetDevices() from do_am_printer() in cgi-bin/admin.c, which is not used in do_list_printers() used for finding new printers. The function uses IPP requests for getting permanent queues and then for getting devices instead of calling CUPS API, so its usage could get the same behavior as it is for adding a printer - but this has to be done a separate function, which will be called only if we want find new printers (we don't want to hide printer's list under authentication...)

So my plan is:

  • find out whether I can create a scriptlet-fu for migration with the proposed change and let you know then,

or (if the migration can't be done by a script during upgrade and it would require an external migration solution like LEAPP)

  • introduce a do_find_new_printers() which will start with auth prompt.

Once I'm done with my script research, I'll clear the review request and let you know how we can proceed.

@zdohnal
Copy link
Member

zdohnal commented Dec 7, 2022

Plus I've tried to check whether other clients (like cupsenable etc.) will be affected by the change, since they send requests to /admin/ resource path, but it doesn't seem to change for them - the tools require the authentication even now.

@zdohnal
Copy link
Member

zdohnal commented Dec 7, 2022

Ok, I've come with the following:

grep -m 1 -A 20 "^\s*<Location /admin>" /etc/cups/cupsd.conf | grep -m 1 -B 20 "^\s*</Location>" | grep -E '^\s*AuthType|^\s*Require' &> /dev/null || sed -i '/^\s*<Location \/admin>/a\  AuthType Default\n  Require user @SYSTEM' /etc/cups/cupsd.conf

which will get you location /admin scope in case it is not longer than 20 lines (I accounted some possible specific Allow/Deny lines, so it should cover most of use case).

So Ack to the PR.

Copy link
Member

@zdohnal zdohnal left a comment

Choose a reason for hiding this comment

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

LGTM once migration script is applied.

@zdohnal zdohnal merged commit 9704c6c into OpenPrinting:master Dec 7, 2022
@zdohnal
Copy link
Member

zdohnal commented Dec 8, 2022

A little bit better command, where colleague helped me and taught me new things about sed. Additionally the command does a backup of config (in case the script goes wrong) - here I use .rpmsave, but other distros can use a different one:

sed -ne '/^\s*<Location \/admin>/ { :loop; /<\/Location>/ ! {N; b loop}; p }' /etc/cups/cupsd.conf \
| grep -E '^\s*(AuthType|Require)' &> /dev/null || cp /etc/cups/cupsd.conf{,.rpmsave} && \
sed -i '/^\s*<Location \/admin>/a\  AuthType Default\n  Require user @SYSTEM' /etc/cups/cupsd.conf

I hope it is useful for someone.

@AZero13 AZero13 deleted the admin branch January 26, 2023 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority-medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants