apple / cups

Apple CUPS Sources
https://www.cups.org
Apache License 2.0
1.91k stars 461 forks source link

auth-info-required - ipp backend #4371

Closed michaelrsweet closed 10 years ago

michaelrsweet commented 10 years ago

Version: 1.5.3 CUPS.org User: jove

I don't want gui authentification dialog to show when I specify username:password@ in the printer uri in printers.conf.

However in ipp.c line 2544 password_cb auth_info_required is set to "username,password" unconditionally.

This leads to setting auth-info-required printer attribute and gui shows dialog next time. Even if this attribute is changed manually to "none", after each authentification it is reset back to "username,password". This is very annoying.

Commenting this line out solved the problem. While I understand a motivation behind putting it there, it is not wise to assume that credentials come from gui.

Other possible interpretation is that backend is right about setting auth-info-required="username,password", but gui needs to check if the credentials are already provided in uri and do not ask the user in that case.

For me the fixing the backend was easied than figuring out the behaviour of (possibly many different) guis.

michaelrsweet commented 10 years ago

CUPS.org User: mike

Jozef,

Thanks for reporting this. Obviously we can't use your patch as-is, since it breaks non-hardcoded usernames and passwords, but I will investigate an alternate fix.

Hardcoding the usernames and passwords isn't generally recommended (and the IPP and HTTP URI RFCs frown on it, with the latest HTTP RFCs outlawing it outright), so in the future we may have to provide a different method for providing hardcoded credentials...

michaelrsweet commented 10 years ago

CUPS.org User: jove

If password_cb is the only way of getting the password into the cups library, then both user supplied and stored passwords have to pass through it. If I auth_info_required needs to be set for user supplied passwords then this has to be done if and only if the password was indeed supplied by user, thus we need a flag and check for it in password_cb before setting auth_info_required.

Tomorrow I'll try do the opposite as a quick and possibly acceptable fix: set a flag if credentials come from uri and don't set auth_info_required in password_cb in that case.

As for the username and password in uri it has been standard http and ftp feature for ages until recently when users got stupid enough not to recognize it when misused for phishing attacks. Then IE stopped supporting it and Firefox warns about it with scary messagebox.

In cups it seems to be supported for smb as well. Provisions are already in place for striping it from logs and user visible strings. I suppose devising another mechanism would be a long term endeavor. I am really happy I found it (mostly) working although it is not mentioned anywhere. Ideal case would be of course authentication by client certificate. Any news on status of this in cups?

michaelrsweet commented 10 years ago

CUPS.org User: mike

We'll be looking at fixing this in 1.7.3.

michaelrsweet commented 10 years ago

CUPS.org User: mike

Fixed in Subversion repository.

michaelrsweet commented 10 years ago

"patch.patch":

diff --git a/backend/ipp.c b/backend/ipp.c index 2700c23..8038edb 100644 --- a/backend/ipp.c +++ b/backend/ipp.c @@ -2541,7 +2541,7 @@ password_cb(const char prompt) / I - Prompt (not used) */

michaelrsweet commented 10 years ago

"patch2.patch":

diff --git a/backend/ipp.c b/backend/ipp.c index 2700c23..930380b 100644 --- a/backend/ipp.c +++ b/backend/ipp.c @@ -94,6 +94,8 @@ static char username[256] = "", /* Username for device URI / *password = NULL; / Password for device URI */ +static int have_auth_info_from_uri = 0;

@@ -2541,7 +2544,10 @@ password_cb(const char prompt) / I - Prompt (not used) */

michaelrsweet commented 10 years ago

"str4371.patch":

Index: backend/ipp.c

--- backend/ipp.c (revision 11885) +++ backend/ipp.c (working copy) @@ -91,8 +91,10 @@ "job-state", "job-state-reasons" }; -static int job_canceled = 0; +static int job_canceled = 0, /* Job cancelled? */