

One week of OpenSSL cleanup - ch0wn
http://undeadly.org/cgi?action=article&sid=20140418063443

======
bqe
They still haven't done the most critical thing: add tests. This is for two
reasons: the code has no tests now, and when you refactor, tests are important
to make sure you're not subtly introducing errors.

~~~
clr76
If openssl has no tests, what is it doing when you run "make test"?

------
jgrahamc
I'd love to see information about the actual security value of these changes.
A ton are just adjusting to KNF.

[http://anoncvs.estpak.ee/cgi-bin/cgit/openbsd-
src/log/lib/li...](http://anoncvs.estpak.ee/cgi-bin/cgit/openbsd-
src/log/lib/libssl)

~~~
Wilya
I don't think the changes they are doing at the moment have much security
value. They aren't working on fixing OpenSSL security flaws. They are
obliterating all the layers of cross-platform compatibility that make the code
more complex, and removing everything OpenBSD isn't likely to use.

They are transforming a local copy of openssl that tracks upstream changes to
their own fork, that probably won't reunite with the upstream version anytime
soon.

In the process, they are obviously likely to find some bugs (and they already
have fixed one or two harmless ones), but I think they are more interested in
building a clean fork for themselves than in fixing the upstream openssl.

~~~
jgrahamc
But they are doing stuff like this:

1\. Let's remove OPENSSL_cleanse: [http://anoncvs.estpak.ee/cgi-
bin/cgit/openbsd-src/commit/lib...](http://anoncvs.estpak.ee/cgi-
bin/cgit/openbsd-
src/commit/lib/libssl?id=6fec7be36425adfd2b48f3695d24a03f6287d4fe)

2\. Crap. Let's put it back again: [http://anoncvs.estpak.ee/cgi-
bin/cgit/openbsd-src/commit/lib...](http://anoncvs.estpak.ee/cgi-
bin/cgit/openbsd-
src/commit/lib/libssl?id=e51513a6c7d3dd499dd4a8a5767951ccfadab599)

That plus the 'tone' of the commits makes me wary of these changes.

~~~
clarry
Did you actually read the second diff you linked? They did not put anything
back again. They removed even more junk. It is all deletions.

I'm getting tired of all the people who complain about the humour, tone or
style in the commit messages. If you have serious concerns, why not read the
diffs and comment on the actual changes? This is how real problems get noticed
and corrected.

But some things were in fact removed and put back again, because it turns out
some ports depended on it. Sometimes you just don't notice what a few thousand
other applications depend on before you remove some code.

~~~
jgrahamc
You're right. I'm wrong about number 2. Mea culpa.

~~~
edwintorok
Also look at the manpage for explicit_bzero: [http://www.openbsd.org/cgi-
bin/man.cgi?query=bzero&sektion=3](http://www.openbsd.org/cgi-
bin/man.cgi?query=bzero&sektion=3)

It already does what OpenSSL_cleanse is meant to do, so using it makes sense.

------
doe88
I would have loved knowing their reasoning behind this decision, which I'm
sure it's very calculated, because the implications are huge. They're choosing
to switch from a simple packager role to a maintainer role, they will have to
always keep their fork in sync with upstream which if they try to apply every
changes will require a lot of work, or maybe they have decided on another
strategy e.g. freezing functionalities and applying only security patches, I
don't know I didn't look at the current types of clean-ups maybe that could
hint what their intentions are.

Regardless, I have mixed feeling about this, one month ago a lot of people
lauded openssl for not being vulnerable to the _goto fail_ saga while
criticizing Apple for not using openssl and now everyone is ready to ditch
openssl in a heartbeat for a flaw not related to crypto, not committed by core
developers. I'm not defending openssl, I would love a concerted effort by big
industry players for implementing a new SSL/TLS stack from scratch, maybe in a
safer language, but I also don't think it is a reason for taking quick,
disruptive decisions like this. It is also likely they had this move in mind
before this episode and this flaw was the last straw but they should have
exposed their plan, it would have been a bit better for us to understand their
reasoning.

~~~
higherpurpose
Maybe they should just fork it completely, keep only the most used stuff, and
call it OpenTLS - the "clean" alternative to OpenSSL. Then push for it to get
accepted in server software. It would be kind of like the
OpenOffice/LibreOffice situation.

Although if they're going to embark on such an endeavour, maybe it would be
best to also rewrite it completely in a memory safe language like Rust (but in
a year or two, after Rust becomes more mature).

~~~
edwintorok
It wouldn't help if they create another project based on OpenSSL code, because
that will inherit OpenSSL's license that everybody hates, and wouldn't
necessarily help other projects adopt it. It might also discourage other
people from looking at / contributing code, when they could just improve
another TLS library with a saner license that people actually want to use not
just due to legacy.

------
dfc
In case your cvs-fu is rusty FreshBSD[1] or OpenSSL Rampage[2] provide an easy
way of keeping an eye on the changes. One of my favorites so far is: "Remove
hacky workaround for Cray T3E," as in the debuted in 1995, Cray Research
T3E.[3]

    
    
      -/* Added for T3E, address-of fails on bit field (beckman@acl.lanl.gov) */
      -#ifndef BIT_FIELD_LIMITS
              memcpy(&server.sin_addr.s_addr, ip, 4);
      -#else
      -       memcpy(&server.sin_addr, ip, 4);
      -#endif
    

[^1]:
[http://freshbsd.org/search?project=openbsd&q=project%3Aopenb...](http://freshbsd.org/search?project=openbsd&q=project%3Aopenbsd+lib%2Flibssl%2F)

[^2]: [http://opensslrampage.org/](http://opensslrampage.org/)

[^3]:
[http://freshbsd.org/commit/openbsd/01f41ed5b37037b963c0de2c2...](http://freshbsd.org/commit/openbsd/01f41ed5b37037b963c0de2c25a84f35390455a2)

[^3]:
[https://en.wikipedia.org/wiki/Cray_T3E](https://en.wikipedia.org/wiki/Cray_T3E)

~~~
clarry
There's also [http://anoncvs.estpak.ee/cgi-bin/cgit/openbsd-
src/log/](http://anoncvs.estpak.ee/cgi-bin/cgit/openbsd-src/log/)

~~~
dfc
I hate cgit's log view, they should at least be honest and call it shortlog.
Most importantly the link you provide displays everything from src, I could
just browse the source-changes list if I wanted to see everything. The
freshbsd and rampage links allow you to see just the libssl changes.

------
clarry
There are tons of simple cleanups I'd love to do. But with all the activity
and people already working on it, I'm afraid I'd just be stepping on people's
toes if I started sending diffs now.

~~~
erickt
Stepping on each other's toes is just what happens with large projects. There
are tools to help deal with merge conflicts so I don't think you should hold
yourself back. OpenSSL needs all the eyes it can get right now.

