lprng-devel Mailing List for LPRng

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

* 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

Bernhard could you take a look ?

It sounds interessting.

re,
 wh


Am 10.11.2010 23:19, schrieb amdg:
> 
> 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).
> 
> Although I do not have time for further development, it would be a shame to
> left my two-years work without commit. I hope people could enjoy it.
> 
> You can find the patch and user documentation on
> http://jointlab.upol.cz/michale/projects/lprng-ipp .
> 
> Václav Michálek
> 
> 
> 
> This body part will be downloaded on demand.
> 
> 
> 
> This body part will be downloaded on demand.

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

Although I do not have time for further development, it would be a shame to
left my two-years work without commit. I hope people could enjoy it.

You can find the patch and user documentation on
http://jointlab.upol.cz/michale/projects/lprng-ipp .

Václav Michálek