Hey there,

This looks really good, thanks for the contribution! Do you mind just amending your commit message to be the format that is documented here?

http://www.port389.org/docs/389ds/contributing.html#getting-the-patch-ready

Otherwise, I'd be happy to merge it. @mhonek can you review and sanity check this for me?

Thanks!

As I don't have a separate ticket to reference, should I use this PR number instead?

https://pagure.io/389-ds-base/issue/51042

I just opened it for you

Incidentally, the motivation for this change is tied to https://bugs.archlinux.org/task/54887, we might want to drop perl from the base system by removing this script. See also https://github.com/openssl/openssl/pull/10123#issuecomment-541223256 where an openssl developer suggested it might be removed upstream.

I realize that either way perl is needed directly for 389-ds-base, this is more about the correctness of which program to use.

It's all good, we want to remove perl anyway, so this helps us towards that objective. Thank you!

Great, so I can freely mention that in the commit message. :D

Great, that looks good to me. I'll let @mhonek have a review to be sure, but otherwise I think this is good to merge! Thank you very much :)

This will break tests on RHEL 7, because openssl rehash is not available there. We’re using lib389 from master branch to run tests on all versions of RHEL and Fedora, so it needs to be compatible.
Could you please make a check for c_rehash presence and use it if it’s available?

This will break tests on RHEL 7, because openssl rehash is not available there. We’re using lib389 from master branch to run tests on all versions of RHEL and Fedora, so it needs to be compatible.
Could you please make a check for c_rehash presence and use it if it’s available?

I had the same though. Try with c_rehash and fall back to openssl rehash. That way the Perl dependency can go away on newer distros.

Indeed, openssl rehash was added in OpenSSL_1_1_0 which was only 3.5 years ago -- it's many places, but not everywhere.

Idea:

-from subprocess import check_output, run, PIPE
+from subprocess import check_output, run, DEVNULL, PIPE

        rehash = ['/usr/bin/openssl', 'rehash']
        if run(rehash + ['-h'], stdout=DEVNULL, stderr=DEVNULL).returncode != 0:
            rehash = ['/usr/bin/c_rehash']
        cmd =  cmd + [self._certdb]

openssl rehash -h should exit 1 if the sub-command is invalid.

That explains why I never use openssl rehash in the past, I always used c_rehash, and why this started with c_rehash.

Normally it wouldn't be an issue to be swapping as iirc sle 15 and rhel 8 both have openssl rehash, but I think that RH still use master lib389 to test 1.3.x 389-ds, so it matters to make that work.

Perhaps the way to handle this is in nss_ssl.py is to break out a function for rehash, and then have the three locations call that, and that one function can do a check such as if c_rehash then use, else try openssl rehash.

Is that reasonable?

openssl rehash -h should exit 1 if the sub-command is invalid.

@eschwartz Funny thing is it doesn't. :) One of things I hate about openssl. I would just go with something like try: ...c_rehash... except CalledProcessError: ...openssl rehash.... And as @firstyear suggested, let's have it broken out to a separate function (or maybe (and maybe as a static method) in NssSsl to have it properly located, not lost in utils somewhere).

And yes, the master branch is used to test everything, including CentOS/RHEL 7 where c_rehash is not present, as Viktor already stated.

Funny thing is it doesn't. :) One of things I hate about openssl.

Arghhhhhh!

Foiled by older openssl versions again. This does work on my openssl 1.1.1g, but I won't bother hunting down when it was added since the only thing that matters is the test environment doesn't have it. Can't depend on rehash to exist everywhere, and can't even depend on checking to see if it exists, whyyyyyyyyyy do you do this openssl...

I'll add a function that tries to use c_rehash and uses openssl rehash if that fails.

In rpm/389-ds-base.spec.in, what is the preferred way to depend on openssl-perl for versions of openssl<1.1.0?

@eschwartz I think that's hard to say, for 389-ds-base.spec.in, that's mainly used by testers and developers, so we can have the openssl-perl there, but the downstream distros will have to adjust based on their needs. IIRC unless you use some if defines with rpm macros, and even then that seems really complex ....

So what do you think of this? Two commits instead of one, because that lets the second commit be used with git revert when it comes time to drop the compatibility shim.

Yeah I'm happy with that, I'll let @mhonek and @vashirov comment too since they had some things to say too :)

Thanks again for your patience with this!

This needs to be a method call, like... self.openssl_rehash(self._certdb). The same in other two places, too.

And this needs to contain the self parameter. Or, you just make it into a static method as I mentioned it earlier.

We don't have self, so it cannot be here. But since the NssSsl uses the log from line 42, we can use it here as well. The same for the two below, of course.

And please, try to test before posting updates to see if it works at least for you. Otherwise we get these sometimes needless iterations. Thanks.

If I don't have c_rehash installed and openssl is <1.1, then it fails twice in both try and except blocks, and no exception is raised:

DEBUG:lib389.nss_ssl:nss cmd: /usr/bin/certutil -L -n Self-Signed-CA -d /etc/dirsrv/ssca -a
DEBUG:lib389.nss_ssl:nss cmd: /usr/bin/c_rehash /etc/dirsrv/ssca
DEBUG:lib389.nss_ssl:failed to run c_rehash, falling back to:
DEBUG:lib389.nss_ssl:nss cmd: /usr/bin/openssl rehash /etc/dirsrv/ssca

Could you please ensure that if both tools fail, we get a proper error? Otherwise code silently continues and the issue is hidden.

Thanks!

There's simply no way to figure out "why" it fails on old versions of openssl.

I've checked out openssl 1.1.0 and it has both rehash and proper return codes.
I've checked out openssl 0.9.8.zh and its openssl version command exists with stable behavior to the new versions. Hopefully this will work everywhere to check whether the rehash app exists.

See new revision.

Looks better to me, I'll let @vashirov comment though. But wow, thanks for your patience, this really got out of hand!

Missing c_rehash is handled correctly now, thanks!
@eschwartz, could you please squash both commits and rebase to master?
Thank you!

Sure.

I had it as two commits so far, because I wanted to leave open the possibility of using git revert to make it obvious how to remove support for older openssl versions. But I can of course combine them into one.

@eschwartz Yeah, leave it as two. Your strategy here to allow revert is correct. Sorry about the confusion.

Note that I just updated the docstring for openssl_rehash() while rebasing, since I forgot to commit it while amending my previous push, but there are no functional changes.

All good. Thank you so much for your patience with this!

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/4094

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.