Re: [Lprng-devel] IPP server for lprng

[walter h. <wh...@bf...>]

thx for the deep analyses !

we can try cups-lpr as IPP client.

@amdg:
can you please comment on the unused variables ?
Simply lazyness or what else ?


Am 12.11.2010 16:52, schrieb Bernhard R. Link:
> * 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.


crypt is quit common these days it is ok to fail if not provided.
same for shadow.

> 
> 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.
> 

IMHO as rule of the thumb plugins should never import libs.



> 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.

i think this i a minor as it could be done with any tool that access /etc/passwd

re,
 wh



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