-
-
Notifications
You must be signed in to change notification settings - Fork 49
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
Handle cases when auth groups are unset #354
base: v1.4.x
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs work.
pappl/client-ipp.c
Outdated
@@ -141,6 +141,9 @@ _papplClientProcessIPP( | |||
client->printer = NULL; | |||
client->job = NULL; | |||
|
|||
if ((attr = ippFindAttribute(client->request, "requesting-user-name", IPP_TAG_NAME)) != NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
pappl/client-auth.c
Outdated
@@ -64,6 +64,10 @@ papplClientIsAuthorized( | |||
if (!client) | |||
return (HTTP_STATUS_BAD_REQUEST); | |||
|
|||
if (_papplSystemGroupIsEmpty(client->system->admin_group) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm uncomfortable with this change, need to think about the security implications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
.
pappl/printer-ipp.c
Outdated
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we don't limit authentication to printing operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
pappl/printer-ipp.c
Outdated
@@ -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)) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't the place for that sort of detemination.
pappl/printer-ipp.c
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ALL printers operations except Get-Printer-Attributes MUST be authenticated when the print group is set.
pappl/system-accessors.c
Outdated
// | ||
// '_papplSystemGroupIsEmpty()' - Check if group is empty | ||
// | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 apappl_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).
2568851
to
239e243
Compare
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.
239e243
to
39e0bb3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments.
// | ||
|
||
bool // O - `true` if empty, `false` if set | ||
_papplClientGroupIsEmpty(const char *group) // I - Group name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also not an operation on a client object. Why not just look for a NULL or empty string?
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.