Opened 4 months ago

Closed 6 days ago

#30190 closed defect (fixed)

Do not warn about compatible OpenSSL upgrades

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: openssl ux usability 035-backport 040-backport dgoulet-merge
Cc: Actual Points: 0.2
Parent ID: Points: 0.2
Reviewer: nickm Sponsor:

Description

From https://github.com/torproject/tor/pull/951

When releasing OpenSSL patch-level maintenance updates,
we do not want to rebuild binaries using it.
And since they guarantee ABI stability, we do not have to.

Without this patch, warning messages were produced
that confused users:
https://bugzilla.opensuse.org/show_bug.cgi?id=1129411

Child Tickets

Change History (16)

comment:1 Changed 4 months ago by teor

I think we've had issues in the past where OpenSSL has promised version compatibility, but failed to deliver.

I've assigned review to nickm, because he knows about our issues with OpenSSL library and header versions.

comment:2 Changed 4 months ago by teor

Status: newneeds_review

comment:3 Changed 4 months ago by bmwiedemann

subscribing here

comment:4 Changed 4 months ago by nickm

Keywords: ux usability 035-backport? 040-backport? added
Status: needs_reviewneeds_revision

Thanks for the patch!

Teor is right -- I'm really glad that openssl is aiming for ABI compatibility now, but they are still have enough complexity going on that I'm not confident that they won't mess it up in the future.

With that in mind, I'd suggest a middle-ground message. Let's log at NOTICE instead of INFO or WARN, and let's say something like "We compiled with OpenSSL X and we're running with OpenSSL Y. These two versions should be binary compatible." That way if people do run into bugs some time they can report that message.

Minor bookkeeping: let's base this on maint-0.3.5 rather than master so that we can backport it to LTS. We'll also need a "changes" file as described in doc/HACKING/CodingStandards.md.

comment:5 Changed 4 months ago by bmwiedemann

I revised the patch as proposed and targeted it at maint-0.3.5

also gave it a quick round of testing and got the notice.

comment:6 Changed 4 months ago by nickm

Looks better -- one problem is that the lintChanges.py script says that your changes file is not in the requested format, so CI is failing. Could you make sure that make check and make check-local both pass?

comment:7 Changed 4 months ago by bmwiedemann

I could use some help there. It wants a ref of an old change.
git blame gave me commit 3e3ec7 referencing issue 17549
but commit 7607ad2 seems to be the original source
and then how to find the release from that?

comment:8 Changed 4 months ago by bmwiedemann

I think I got it right now.

comment:9 Changed 4 months ago by nickm

Status: needs_revisionneeds_review

Sorry for the delay -- I've been on vacation. I'll try to look at this again soon.

comment:10 Changed 4 months ago by nickm

LGTM now; thank you!

comment:11 Changed 4 months ago by nickm

Status: needs_reviewmerge_ready

comment:12 Changed 4 months ago by nickm

Keywords: dgoulet-merge added

comment:13 Changed 4 months ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

Merged!

comment:14 Changed 2 months ago by teor

Actual Points: 0.2
Keywords: 035-backport 040-backport added; 035-backport? 040-backport? removed
Milestone: Tor: 0.4.1.x-finalTor: 0.4.0.x-final
Resolution: fixed
Status: closedreopened

comment:15 Changed 2 months ago by teor

Status: reopenedmerge_ready

We missed a backport on this ticket, because we closed it after merging to master.

comment:16 Changed 6 days ago by teor

Milestone: Tor: 0.4.0.x-finalTor: 0.3.5.x-final
Resolution: fixed
Status: merge_readyclosed

Backported to 0.3.5.
Merged with the other 0.3.5 and 0.4.0 backports on 2019-08-12.

Note: See TracTickets for help on using tickets.