Minor changes/fixes #6
Conversation
|
Thank you for your interest and your work. I'll check it later with consideration. |
|
Hi, Here is my analysis about this pull request. The message was vague on purpose, for security reasons. The aim is to avoid an attacker from understanding all the mechanisms behind the frontend. (the fact that it is an underlying cracklib library for example) agreed agreed agreed ppm is now compiled by default with cracklib in LTB packages. As it is a new feature, I don't think it is a good idea to activate it by default in the configuration file. Normal users shouldn't see OpenLDAP logs anyway, should they? Thank you for reporting this, as it is a bug in the module!
Brilliant
The cracklib library is not linked, which indeed results in a bug. However, why also link to libber and libldap? Have you tried without it? For example, with the following:
Interesting... Do you have an explanation on why ppm.so and LIBS need to be rejected to the end of the line? ppm should stay generic. (as you may have seen, LTB community provides packages both for Debian and Red-Hat) @tdb would you mind being mentionned in the README as: |
|
On Thu, Jul 20, 2017 at 03:51:28PM +0000, davidcoutadeur wrote:
b52db61
The message was vague on purpose, for security reasons. The aim is to
avoid an attacker from understanding all the mechanisms behind the
frontend. (the fact that it is an underlying cracklib library for
example)
318255f
ppm is now compiled by default with cracklib in LTB packages. As it is
a new feature, I don't think it is a good idea to activate it by
default in the configuration file.
5014f76
Normal users shouldn't see OpenLDAP logs anyway, should they?
As the verbose output is only displayed in case of password change, I
suppose it is not a bad idea to activate debug by default.
I've dropped these three commits. Maybe I'll use them locally, but
you're probably right that they don't need upstreaming to you.
08646ea
Thank you for reporting this, as it is a bug in the module!
Though, I have different opinions on parts of the patch:
-Wl,-rpath=.
Brilliant
$(CC) $(LDAP_INC) $(LDAP_LIBS) -shared -o ppm.so ppm.o $(LIBS)
The cracklib library is not linked, which indeed results in a bug.
However, why also link to libber and libldap? Have you tried without
it? For example, with the following:
$(CC) $(LDAP_INC) -shared -o ppm.so ppm.o $(CRACK_LIB)
$(CC) $(LDAP_INC) $(LDAP_LIBS) -Wl,-rpath=. -o ppm_test ppm_test.c
ppm.so $(LIBS)
ppm_test uses ber_memfree. It could probably just use free instead?
Interesting... Do you have an explanation on why ppm.so and LIBS need
to be rejected to the end of the line?
I'm not sure what you mean by "rejected to the end of the line",
probably just the wrong words? If you mean why did I move them to the
end of the line, then it was because the linker requires dependencies to
come after the source file.
1468620
ppm should stay generic. (as you may have seen, LTB community provides
packages both for Debian and Red-Hat)
The default CONFIG and LIBDIR would only be useful on Ubuntu...
Idem for LDAP_LIB / LDAP_INC: the Makefile is supposed to be adapted
for the appropriate environment. LTB is packaging passing the correct
variables, for example:
make "CONFIG=/usr/local/openldap/etc/openldap/ppm.conf"
"LDAP_INC=-I../include -I../servers/slapd"
Ok - that's a good idea, I can just override when building. So I dropped
this commit.
@tdb would you mind being mentionned in the README as:
2015 - tdb - Tim Bishop - contribution on some compilation improvements
No problem. But it's 2017 :-)
Tim.
…--
Tim Bishop
http://www.bishnet.net/tim/
PGP Key: 0x6C226B37FDF38D55
|
ppm_test has always been using libber. That's why I have linked it in the Makefile.
Yes: why move them to the end of the line. Noted for 2017 :) |
|
On Thu, Jul 20, 2017 at 10:08:21AM -0700, davidcoutadeur wrote:
> ppm_test uses ber_memfree. It could probably just use free instead?
ppm_test has always been using libber. That's why I have linked it in the Makefile.
Sorry, I got muddled about which lines we're talking about.
You're right - ppm_test needs linking against $(LIBS), which it did
before. All I did was change the order of the line.
However, I wanted - and still want - the main program to be as short and efficient as possible.
My point is that I don't think ppm.so needs to be linked to libldap
and libber because slapd is in a way already linked to them. Can you
confirm it is also working with you without this linking?
Fair enough, that's reasonable. It is only dynamic linking though, so it
doesn't really change ppm.so much. I suppose it depends on whether you
think the fact that ppm.so uses functions in the openldap libraries
means it should link the openldap libraries. I thought it should, but
you don't. It doesn't really matter.
Linking against just $(CRACK_LIB) seems to be sufficient:
$(CC) $(LDAP_INC) -shared -o ppm.so ppm.o $(CRACK_LIB)
> I'm not sure what you mean by "rejected to the end of the line",
> probably just the wrong words? If you mean why did I move them to
> the end of the line, then it was because the linker requires
> dependencies to come after the source file.
Yes: why move them to the end of the line.
So you suspect the way I did it was a tolerance in my build environment (especialy gcc actually)?
I'm using gcc too, so I'm puzzled why it behaves differently. If I put
them before ppm_test.c in the command line I get:
/tmp/cc7QADTv.o: In function `main':
ppm_test.c:(.text+0x88): undefined reference to `check_password'
ppm_test.c:(.text+0xbf): undefined reference to `ber_memfree'
collect2: error: ld returned 1 exit status
Tim.
…--
Tim Bishop
http://www.bishnet.net/tim/
PGP Key: 0x6C226B37FDF38D55
|
This way it can be run outside of the test rig and report whether the password is good, and include the error if not.
On Ubuntu 16.04 I had trouble making this compile. There was one notable change required - the libraries to link (ppm.so and LIBS) need to be given after the file being compiled. It also seems helpful to link ppm.so against the required libraries, otherwise how will slapd know to find libcrack? Finally, I added -Wl,-rpath=. to ppm_test meaning it can be run directly without needing to set the library path.
| @@ -40,13 +40,13 @@ TESTS=./unit_tests.sh | |||
| all: ppm ppm_test | |||
|
|
|||
| ppm_test: | |||
| $(CC) $(LDAP_INC) $(LDAP_LIBS) $(LIBS) ppm.so -o ppm_test ppm_test.c | |||
| $(CC) $(LDAP_INC) $(LDAP_LIBS) -Wl,-rpath=. -o ppm_test ppm_test.c ppm.so $(LIBS) | |||
There was a problem hiding this comment.
Wondering if -Wl,rpath=. might be better as -Wl,rpath=.:$(LIBDIR)?
|
I am not a big fan of having both relative and absolute path, as it could bring confusion between the really loaded libraries. |
|
Thanks for the final feedback and the merge! |
Here are a number of changes/fixes that I needed to get ppm working on Ubuntu with the OS slapd. If there's anything useful here please feel free to cherry pick the bits you want, or let me know and I can reshape the pull request.