Handle cases when auth groups are unset #354

zdohnal · 2024-03-18T16:23:01Z

Currently, pappl based applications require authentication every time for any request if authentication module is set, even if authentication group(s) is set to None.

With this PR, admin can set admin and print group to None. Every admin action is allowed if admin group is None. For printing jobs, Validate-Job, Print-Job, Create-Job are allowed, Send-Document if the requesting client is owner of the job - value of requesting-user-name is used as default value,
in case authorization field does not contain it.

This allows smooth printing to the printer application via CUPS when printer application uses PAM module to protect against simple attacks.

michaelrsweet

Needs work.

michaelrsweet · 2024-03-18T17:18:24Z

pappl/client-ipp.c

@@ -141,6 +141,9 @@ _papplClientProcessIPP(
 client->printer = NULL;
 client->job = NULL;

+ if ((attr = ippFindAttribute(client->request, "requesting-user-name", IPP_TAG_NAME)) != NULL)


No, client->username is the AUTHENTICATED username, not whatever the client sends in the "requesting-user-name" attribute. There are places where we want to fallback on "requesting-user-name", but this isn't how to do it.

Yes, I saw this later in the code - I meant this assignment as a fallback for purpose of this code:

806 if (code == HTTP_STATUS_CONTINUE && client->job && client->job->username && strcmp(client->username, client->job->username)) 807 { 808 // Not the owner, try authorizing with admin group... 809 code = _papplClientIsAuthorizedForGroup(client, true, client->system->admin_group, client->system->admin_gid); 810 }

where, even if the user is really owner of the job, client->username is empty, so the request gets to trying authenticate with admin group (which is kind of off-topic for job related operations IMHO).

What I had in mind is to simulate similar settings as default CUPS has - authentication required for admin tasks, printing operations (except for Send-Document) requires no authentication by default.

Does the fallback make sense in _papplClientIsAuthorizedForGroup() if we don't find authorization field?

michaelrsweet · 2024-03-18T17:19:25Z

pappl/client-auth.c

@@ -64,6 +64,10 @@ papplClientIsAuthorized(
 if (!client)
 return (HTTP_STATUS_BAD_REQUEST);

+ if (_papplSystemGroupIsEmpty(client->system->admin_group) &&


I'm uncomfortable with this change, need to think about the security implications.

AFAIK it has the same implications as the current default in printer applications - any user can do all admin tasks. But I can add check here whether there is an authentication service and move it to _papplClientIsAuthorizedForGroup().

michaelrsweet · 2024-03-18T17:19:58Z

pappl/printer-ipp.c

@@ -38,6 +38,7 @@ static void ipp_get_jobs(pappl_client_t *client);
 static void ipp_get_printer_attributes(pappl_client_t *client);
 static void ipp_hold_new_jobs(pappl_client_t *client);
 static void ipp_identify_printer(pappl_client_t *client);
+static bool ipp_is_printing_op(pappl_client_t *client);


No, we don't limit authentication to printing operations.

Ok, now I see the function is combining two things - finding out whether the operation is within the expected set and checking the usernames in case of Send-Document, which should not happen. I basically wanted to simulate the default CUPS behavior for printing related operations, so I implemented it in one function, which was to be used only in this use case.

I've thrown it away for now.

michaelrsweet · 2024-03-18T17:20:20Z

pappl/printer-ipp.c

@@ -785,6 +786,10 @@ bool // O - `true` on success, `false` on failure
 _papplPrinterIsAuthorized(
 pappl_client_t *client) // I - Client
 {
+ if (_papplSystemGroupIsEmpty(client->printer->print_group) && httpAddrIsLocalhost(httpGetAddress(client->http)) &&


This isn't the place for that sort of detemination.

michaelrsweet · 2024-03-18T17:20:54Z

pappl/printer-ipp.c

@@ -2365,3 +2370,22 @@ valid_job_attributes(

 return (valid);
 }
+
+
+static bool // O - `true` if the operation is related to print job and valid, `false` if not


ALL printers operations except Get-Printer-Attributes MUST be authenticated when the print group is set.

michaelrsweet · 2024-03-18T17:21:35Z

pappl/system-accessors.c

+//
+// '_papplSystemGroupIsEmpty()' - Check if group is empty
+//
+


Empty or NULL group, NOT the "none" group (which is a valid group name and yes people do use that name...)

Also, _papplSystemGroupIsEmpty implies it operates on a pappl_system_t object. Better to have a different name if this is to be generic or provide the equivalent _papplPrinterXXX function for the print group.

Empty or NULL group, NOT the "none" group (which is a valid group name and yes people do use that name...)

Ok, never knew it exists :D .

Also, _papplSystemGroupIsEmpty implies it operates on a pappl_system_t object. Better to have a different name if this is to be generic or provide the equivalent _papplPrinterXXX function for the print group.

I've made it _papplClientGroupIsEmpty() for now, since it is used when authenticating client request (we don't use printer-private.h in client-auth.c).

Currently, pappl based applications require authentication every time for any request if authentication module is set, even if authentication group(s) is set to None. With this PR, admin can set admin and print group to None. If the connection is local, authentication group is none and username is missing, `requesting-user-name` is used during authorization. This allows smooth printing to the printer application via CUPS when printer application uses PAM module to protect against simple attacks.

michaelrsweet

See comments.

michaelrsweet · 2024-03-22T13:46:42Z

pappl/client-accessors.c

+//
+
+bool // O - `true` if empty, `false` if set
+_papplClientGroupIsEmpty(const char *group) // I - Group name


Also not an operation on a client object. Why not just look for a NULL or empty string?

michaelrsweet · 2024-03-22T13:48:34Z

pappl/printer-ipp.c

 http_status_t code = _papplClientIsAuthorizedForGroup(client, true, client->printer->print_group, client->printer->print_gid);

- if (code == HTTP_STATUS_CONTINUE && client->job && client->job->username && strcmp(client->username, client->job->username))
+ // In case of local connections, use requesting-user-name attribute as username later if client username is missing


No, we want an authenticated username. The alternative (if authentication is enabled) would be to lookup the UID via peer credentials. I don't want to rely on requesting-user-name if authentication is enabled.

michaelrsweet requested changes Mar 18, 2024

View reviewed changes

michaelrsweet self-assigned this Mar 18, 2024

michaelrsweet added bug Something isn't working priority-low labels Mar 18, 2024

michaelrsweet added this to the Stable milestone Mar 18, 2024

michaelrsweet mentioned this pull request Mar 18, 2024

Handle cases when auth groups are unset #353

Open

zdohnal force-pushed the auth-print-group-none-1.4 branch from 2568851 to 239e243 Compare March 19, 2024 16:09

zdohnal force-pushed the auth-print-group-none-1.4 branch from 239e243 to 39e0bb3 Compare March 19, 2024 16:14

zdohnal requested a review from michaelrsweet March 19, 2024 16:21

michaelrsweet requested changes Mar 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle cases when auth groups are unset #354

Handle cases when auth groups are unset #354

zdohnal commented Mar 18, 2024

michaelrsweet left a comment

michaelrsweet Mar 18, 2024

zdohnal Mar 19, 2024

michaelrsweet Mar 18, 2024

zdohnal Mar 19, 2024

michaelrsweet Mar 18, 2024

zdohnal Mar 19, 2024

michaelrsweet Mar 18, 2024

michaelrsweet Mar 18, 2024

michaelrsweet Mar 18, 2024

zdohnal Mar 19, 2024

michaelrsweet left a comment

michaelrsweet Mar 22, 2024

michaelrsweet Mar 22, 2024

Handle cases when auth groups are unset #354

Are you sure you want to change the base?

Handle cases when auth groups are unset #354

Conversation

zdohnal commented Mar 18, 2024

michaelrsweet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelrsweet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment