Re: [Lprng-devel] IPP server for lprng

[Bernhard R. L. <br...@de...>]

* amdg <am...@e-...> [101111 13:03]:
> I have made an implementation of IPP 1.1 server into LPRng daemon. The IPP
> implementation is almost rfc-conforming except IPP subscriptions, which
> would require more changes in LPD daemon (I hope some volunteer would do
> it).

Very minor issue: the patch triples the number of lines with space
or tab as last character. This can be fixed easily when we import
it.

Minor Issue: The patch introduces some compiler warnings:
---------------------------------------------------------

common/lpd_dispatch.c:151: warning: unused variable ‘p’
trivial to fix

common/lpd_dispatch.c:287: warning: ‘noreturn’ function does return
This seems to only be a missing NORETURN in the Service_ipp prototype.

common/ipp.c:742: warning: unused variable ‘d’
common/ipp.c:845: warning: unused variable ‘r’
common/ipp.c:1303: warning: label ‘unauth’ defined but not used
common/ipp.c:1180: warning: unused variable ‘ath’
common/ipp_proc.c:497: warning: unused variable ‘username’
common/ipp_proc.c:1970: warning: unused variable ‘username’
common/ipp_proc.c:1967: warning: unused variable ‘jobnum’
common/ipp_aux.c:1077: warning: label ‘done’ defined but not used
Harmless unused stuff, too, I guess. Easy to fix.

common/ipp_proc.c:3190: warning: ‘sx’ may be used uninitialized in this function
false positive

Issue: library dependencies
---------------------------

The patch adds dependencies for pam, shadow and crypt libraries
unconditionally. It never needs them all at the same time, so it rather
should only include things those it will use (and not pull in those it
does not use). Ideally with some option for the user to choose what to
use.

Pulling in pam unconditionally does not make be happy at all. I think
we should guard this code with an option disabled by default and only
check and pull in those dependencies if it is enabled.

Issue: including prototypes for system functions
------------------------------------------------
+#ifdef HAVE_CRYPT
+       char *crypt(const char *key, const char *salt);
+#endif

That is not acceptable in my eyes.

Issue: incomplete testing in configure
--------------------------------------
The code uses shadow.h but does not check in configure.
If there is no pam and getpwnam/getspnam but crypt, one will
get errors compiling. They should already be in configure.

leaking ssl details
-------------------
I do not like the ssl functions being visible via auth_ssl
where the patch adds them.
I'd rather have it in some special header file to reduce the
impact of that stuff.

Same with ipp.h, that also leaks ssl implementation details
into multiple other compilation units.

incompatible with authentication methods as plugins
---------------------------------------------------
When compiled with ssl and plugins, it will fail to link,
as the ssl stuff is only in a plugin. If this is guarded by
some option, that combination has to be rejected by configure
or some code has to be moved around in that case.

security considerations
-----------------------
I've not yet had the time to look into the code itself. It's quite
a bit to review it thoroughly. I'm a bit concerned it accesses the
system user database and passwords, so it opens a way to brute-force
user passwords. I guess if this is guarded by some option and that
option has a proper warning message in configure that should not be
an issue.

testing
-------
Anyone has an ipp client so this can be tested?

	Bernhard R. Link