security/openssl: Enable ASM by default
ClosedPublic

Authored by allanjude on Feb 7 2017, 9:03 PM.

Details

Summary
security/openssl: Enable ASM by default

  - Enable ASM option
  - By extension this enables AES-NI [1]
  - Order OPTIONS_DEFAULT alphabetically
  - Switch to using @sample [2]

PR: 216559 [2]
Reported by:	dtestke [1]
Submitted by:  Franco Fichtner <franco@opnsense.org> [2]

Diff Detail

Repository
rP FreeBSD ports repository
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7264
Build 7432: arc lint + arc unit
brnrd retitled this revision from to security/openssl: Enable ASM by default.Feb 7 2017, 9:03 PM
brnrd updated this object.
brnrd edited the test plan for this revision. (Show Details)
brnrd added a subscriber: dteske.
brnrd updated this object.Feb 7 2017, 9:04 PM
brnrd added a reviewer: dteske.Feb 7 2017, 9:07 PM
brnrd removed a subscriber: dteske.
brd added a reviewer: asomers.Feb 7 2017, 9:11 PM
brnrd updated this revision to Diff 24851.Feb 7 2017, 9:13 PM

Fix pkg-message for new @sample

brnrd updated this object.Feb 7 2017, 9:15 PM

I think we need a "PORTREVISION= 1"

mat added a comment.Feb 7 2017, 11:17 PM

Are the new *.so files 100% compatible with the old ones if one enables ASM ? Or does this need a shlib version bump ?

security/openssl/pkg-plist
1707

This feels like a bad idea.
If this openssl.cnf is there, does openssl behave the exact same way than it was without it ? Because if the answer is less than 100% yes, do not do it.

asomers requested changes to this revision.Feb 8 2017, 1:11 AM

Apart from the PORTREVISION issue, it LGTM. Please bump PORTREVISION.

security/openssl/pkg-plist
1707

I tested it, and it's fine. If openssl.cnf is already there, then updating the port does _not_ replace it.

This revision now requires changes to proceed.Feb 8 2017, 1:11 AM
emaste added a subscriber: emaste.Feb 8 2017, 2:07 AM
brnrd marked 2 inline comments as done.Feb 8 2017, 11:14 AM
In D9480#195981, @mat wrote:

Are the new *.so files 100% compatible with the old ones if one enables ASM ? Or does this need a shlib version bump ?

The ASM defines mask the C-routines and enable the optimized assembly replacements.

There's a bit more to it I've learned. Various commits referring to ASM:

Unfortunately the commit messages aren't very informative... It seems to me that people hitting bugs had not rebuilt their ports correctly or were mixing base and ports SSL libs (via sasl, krb, curl, ...)
Given that:

  1. Base OpenSSL has ASM enabled by default
  2. There are no reports on broken ports with base OpenSSL pertaining to ASM
  3. There are reports that ASM does work (e.g. PR19788 #49)
  4. ASM is enabled by default upstream
  5. Insecure options are enabled by default in port but not upstream

I propose the following:

  1. Enable ASM by default as soon as possible

And as a separate action/commit

  1. Announce upcoming change on ML
  2. Bump .so version
  3. Disable MD2 and SSLv2
  4. Add entry to UPDATING
OptionDefault PortComment
asmonforced offPerformance impacted
ec_nistpoffforced onconfigure can't detect
gmpoffforced off
jpakeoffdefault off
krb5offdefault off
libunboundoffdefault off
md2offforced onInsecure
rc5offforced off
rfc3779offforced off
sctpoffforced on
sharedoffforced on
ssl-traceoffdefault off
ssl2offforced onInsecure
ssl3forced onpresumed on by default
static-engineonoffautomatic by "shared" option
storeoffforced off
threadsforced on
unit-testoffdefault off
weak-ciphersoffforced off
zliboffforced off
zlib-dynamicoffforced off
brnrd updated this revision to Diff 24857.Feb 8 2017, 11:18 AM

Bump PORTREVISION

mat added inline comments.Feb 8 2017, 11:27 AM
security/openssl/pkg-plist
1707

Well, of course if there is an existing one it will not be replaced, it is what @sample does. My question was "does openssl behaves the same way if the provided openssl.cnf is here or not ?"

allanjude accepted this revision.Feb 8 2017, 5:36 PM
allanjude added a reviewer: allanjude.
allanjude added a subscriber: allanjude.

Works as expected for me. We have been using the ASM option in production since the start of 2016.

brnrd marked an inline comment as done.Feb 8 2017, 7:45 PM
brnrd added inline comments.
security/openssl/pkg-plist
1707

Not in all circumstances, it sets defaults for files, directories, and DN. The openssl command works without an openssl.cnf but will emit a warning it is missing.

I believe the default @sample behaviour is what's required here.

dteske added a comment.Feb 9 2017, 1:43 AM

Bumping the .so revision puts a big onus on some vendors to recompile their software for almost zero reason.

The ASM enabled .so is absolutely compatible with the ASM disabled .so -- there should be no need to bump the .so revision.

Most notably, people like nginx whom provide a paid-for product called "nginx-plus" available as binary-package only from their own public pkg server will have to recompile their binaries and issue a new package to the world if you bump the .so revision.

dteske accepted this revision.Feb 9 2017, 2:26 AM
dteske added a comment.Feb 9 2017, 2:34 AM

Tried closing this revision but because asomers blocked it, phabricator tells me it can't be closed.

Can someone else try to close this review?

allanjude commandeered this revision.Feb 9 2017, 2:40 AM
allanjude accepted this revision.
allanjude edited reviewers, added: brnrd; removed: allanjude.
allanjude added a reviewer: allanjude.
allanjude marked an inline comment as done.
allanjude removed a reviewer: asomers.
This revision is now accepted and ready to land.Feb 9 2017, 2:41 AM
allanjude closed this revision.Feb 9 2017, 2:41 AM

committed as rP433671

julian added a subscriber: julian.Feb 9 2017, 4:25 AM

As long as features are DISABLED and not removed (e.g. at $JOB we need MD2) then ok.
I think we should make two versions of this port: openssl and openssl-insecure, which has features like sslv2 and MD2 enabled for people who need that functionality. (e.g. for talking to old machines or handling old certificates etc.)
maybe with a different .so number. (just an idea).

brnrd added a comment.Feb 9 2017, 12:20 PM

As long as features are DISABLED and not removed (e.g. at $JOB we need MD2) then ok.
I think we should make two versions of this port: openssl and openssl-insecure, which has features like sslv2 and MD2 enabled for people who need that functionality. (e.g. for talking to old machines or handling old certificates etc.)
maybe with a different .so number. (just an idea).

You may be interested in D9483 "security/openssl-unsafe: New port"