Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Undeprecate OpenSSL_version_num #7853

Closed
wants to merge 2 commits into from
Closed

Conversation

@vdukhovni
Copy link

vdukhovni commented Dec 9, 2018

The OpenSSL_version_num() function returns at runtime the
OPENSSL_VERSION_NUMBER of the compiled OpenSSL library. This is a
used and useful interface, and should not (at least yet) be
deprecated, we just introduced the new versioning schema, it seems
too early to deprecate the old.

@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Dec 9, 2018

+1 from me for @vdukhovni's request because it makes sense. But I won't formally approve the pull request because I'd prefer if @levitte himself would approve.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Dec 9, 2018

You do understand, I hope, that a deprecation in 3.0.0 means removal at the earliest in 4.0.0.

It seems to me that running two versioning calculations (not scheme, the scheme has changed regardless of what number you pull) without marking one as "this is the old stuff" isn't sensible. That is basically what deprecation does.

@vdukhovni

This comment has been minimized.

Copy link
Author

vdukhovni commented Dec 9, 2018

You do understand, I hope, that a deprecation in 3.0.0 means removal at the earliest in 4.0.0.

Yes. I almost put a "I understand that ..." comment to that effect in the commit message.
Still I don't think there's any compelling reason to remove the function, there's so little to be gained by doing it.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Dec 10, 2018

One of the uses was to check forward compatibility by checking that what people inferred to be the major number (using 0xfff00000L as mask) is the same in macro and number returned by the library. That mask is no longer correct, even though harmless. Taking out just the major number is sufficient, and there are much easier macros and library functions to do so.

Also, please don't say "remove", that's a false argument, at least for now. It will have to be removed at the latest at version 8.0.0. I would prefer earlier, but... Either way, I strongly disagree with this PR.

If you were protesting actual removal, I would understand much better.

@vdukhovni

This comment has been minimized.

Copy link
Author

vdukhovni commented Dec 10, 2018

The function is harmless, there are no security issues with using it, and worst case some application might be too cautious and issue warnings about potential ABI incompatibility where none are needed.

The cost of deprecation (and eventual removal) is to break some old code that would otherwise just work. So I don't think deprecation is warranted in this case. Since we expose the compile-time OPENSSL_VERSION_NUMBER, we should also expose the equivalent runtime version, but document that this is perhaps typically not what you want.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Dec 10, 2018

Hmmm, OPENSSL_VERSION_NUMBER is also deprecated... but in the future, i.e. starting with version 4.0.0. I have no idea why I made that difference, it certainly wasn't a conscious move.

However, that might be an acceptable compromise. Instead of removing deprecation entirely, make it a future deprecation for 4.0.0. I'll have an easier time accepting that. Does that work for you?

@vdukhovni

This comment has been minimized.

Copy link
Author

vdukhovni commented Dec 10, 2018

If push comes to shove, I could perhaps live with a 4.0.0 base for deprecation, but I still don't see that the benefit outweighs the costs. I'll sit back and see what others have to say...

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Dec 10, 2018

We (well, at least I) want people to start using the new macros and functions. People often don't unless you somehow tell them that the old is falling out of use (and support). If you know of other ways to do send a clear signal, by all means, please let us know.

@paulidale

This comment has been minimized.

Copy link
Contributor

paulidale commented Dec 10, 2018

I'd like to see outdated APIs deprecated in a meaningful way, there is lots of cruft in the code and this is one way to slowly clean it up.

I'm not too concerned if these are tagged as deprecated in 3.0.0 or 4.0.0. I'd ever so slightly favour the former but not by enough to matter. What I'd not like, is a perpetual deferring of the decision to deprecate. I.e. we should set a release where they will be tagged and stick to it.

@vdukhovni

This comment has been minimized.

Copy link
Author

vdukhovni commented Dec 10, 2018

In this case I think that "carrot" rather than "stick" works well enough. The new functions are contracted to properly reflect ABI stability, rather than provide a version number for the application to decode in an ad hoc manner. So applications should want to use the new approach when ABI versioning is what they want.

On the other hand, for example, in Postfix we serialize session objects to external shared storage, and given that SSL_SESSION is opaque, I don't think I can necessarily expect d2i_SSL_SESSION to invert i2d_SSL_SESSION across software versions. Or if not that reason, then there can be other reasons to obtain the specific numeric version.

So the carrot is documentation, blog posts, ... and since (I believe that) the old API does no harm, I see no reason to deprecate it at all, mostly makes users jump through hoops porting.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Dec 10, 2018

Heh... "if you don't take the carrot, you will eventually get the stick"

I'm down with that 😉

@petrovr

This comment has been minimized.

Copy link

petrovr commented Dec 11, 2018

I know that some projects use macros similar to OPENSSL_VERSION_NUMBER to decide at compile time which function to use. This is easiest way to write compatible code but is hard to maintain - project code full with #if FOO_VERSION > NNNN cannot be refactored, updated and so one.
Testing external libraries for available functionality and using "port" library (wrapper functions) ensures readable and easy maintainable code.

Deprecation does not mean that OpenSSL functionality will not be available. I don't think that OS vendors will distribute package with API compatibility flag activated.

-1 for this patch.

@DDvO

This comment has been minimized.

Copy link
Contributor

DDvO commented Jan 15, 2019

There are deeper and more general problems with touching the functions that yield OpenSSL runtime version information (such as making OpenSSL_version_num() inaccessible) and potentially changing/deprecating OPENSSL_VERSION_NUMBER.

In case the version of libcrypto/libssl that is actually used at runtime is different from the OpenSSL version that has been referred to statically during compiling code that uses OpenSSL, crashes can happen and it can be quite hard for people to find out why.
As a remedy for such situations I've added version checking code like the following to libraries and applications dynamically linked to OpenSSL:

if (OpenSSL_version_num() != OPENSSL_VERSION_NUMBER) {
    fprintf(stderr, "OpenSSL runtime version 0x%lx does not match 0x%lx used for compiling %s\n",
            OpenSSL_version_num(), OPENSSL_VERSION_NUMBER, __FILE__);
    exit(EXIT_FAILURE);
}

Now comes the catch:
The version retrieval function OpenSSL_version_num() itself has changed its name (from originally SSLeay()) and is going to disappear, and even worse, whether to use SSLeay() or OpenSSL_version_num() or something new as needed since version 3.0.0-pre:

#if OPENSSL_VERSION_NUMBER < 0x10100000L
#define OpenSSL_version_num SSLeay
#elif OPENSSL_VERSION_NUMBER >= 0x30000000L
#define OpenSSL_version_num() ((unsigned long) \
                               ((OPENSSL_version_major()<<28) \
                               |(OPENSSL_version_minor()<<20) \
                               |(OPENSSL_version_patch()<< 4) \
                               |_OPENSSL_VERSION_PRE_RELEASE ))
#endif

can only be decided by consulting OPENSSL_VERSION_NUMBER - which in turn may give not the right version in case a different OpenSSL version gets loaded at runtime :-(

I see no other solution for this tricky issue than keeping OpenSSL_version_num() function, and also SSLeay() as long as 1.0.2 is being supported, as proper (i.e., non-macro) functions.

@t8m

This comment has been minimized.

Copy link
Member

t8m commented Jan 15, 2019

As a remedy for such situations I've added version checking code like the following to libraries and applications dynamically linked to OpenSSL:

    if (OpenSSL_version_num() != OPENSSL_VERSION_NUMBER) {
        fprintf(stderr, "OpenSSL runtime version 0x%lx does not match version 0x%lx used for compiling %s\n",
                OpenSSL_version_num(), OPENSSL_VERSION_NUMBER, __FILE__);
        exit(EXIT_FAILURE);
    }

Huh, that is clearly incorrect version check - it will not allow any update of the openssl shared libraries to a newer version although they are fully ABI compatible. It would make sense many years ago when the ABI compatibility was not really maintained on releases but this is definitely no longer the case.

@DDvO

This comment has been minimized.

Copy link
Contributor

DDvO commented Jan 15, 2019

@t8m, thanks for your comment. You are right in the sense that the above check is too strict - API changes should affect only the major and minor version, not the (letter) release/patch. Yet apart from that, functions can appear and disappear and change their arguments and might even change their semantics, which can lead to crashes on (major/minor) version mismatch.
So any improvements on the details of the above example check do not affect the point I made above.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 1, 2019

I had forgotten about this... so nothing has really happened. I still see no compelling reason to undeprecate this, but can see the deprecation move to version 4 instead of version 3. I would approve that.

@kroeckx

This comment has been minimized.

Copy link
Member

kroeckx commented Sep 1, 2019

@vdukhovni

This comment has been minimized.

Copy link
Author

vdukhovni commented Sep 1, 2019

I had forgotten about this... so nothing has really happened. I still see no compelling reason to undeprecate this, but can see the deprecation move to version 4 instead of version 3. I would approve that.

The compelling reason is that I and many others maintain software packages that use this interface for diagnostic purposes, and capriciously removing it is disrespectful to users. The users who misuse this interface and not use the newer options will pay the price at some point, but there is no reason to remove this interface.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 1, 2019

... capriciously removing it ...

[rephrasing a heated comment]
I admit getting irritated by the constant reformulation of deprecation as removal, when these aren't the same thing. Sure, it's a future removal, and we give warning ahead... and with that forewarning, I fail to understand "capricious", as the intent is exactly the opposite.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 1, 2019

... but there is no reason to remove this interface.

Oh yes there is. The misuse that @kroeckx reports is exactly a reason to deprecate this interface.

@vdukhovni

This comment has been minimized.

Copy link
Author

vdukhovni commented Sep 1, 2019

I think where we diverge is on the question of what constitutes grounds for deprecation. For me, misuse by some is not a sufficient reason to deprecate an interface. My take is that we deprecate interfaces when impose a burden on maintenance or future evolution of the library. This interface is not a burden to maintain, and we can easily continue to provide it. And therefore, IMHO we should. I do not see a compelling need to break compatibility, even given some misuse.

@kroeckx

This comment has been minimized.

Copy link
Member

kroeckx commented Sep 1, 2019

@vdukhovni

This comment has been minimized.

Copy link
Author

vdukhovni commented Sep 1, 2019

Since we're changing our version scheme, where we change the major from "1.1" to "3", you can consider that to be an API change, and the mask you used for older versions might not be correct anymore. The manpage doesn't even explain that.

I don't see anything incompatible here. It'll continue to work.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 1, 2019

[shrug]
Of course, if no one approves this, the status remains quo.
And if no one changes the deprecation of either OPENSSL_VERSION_NUMBER or OpenSSL_version_num, the mismatch remains.
Nice standstill...

@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Sep 1, 2019

I don't see anything incompatible here. It'll continue to work.

Well, not forever. There is a hard limit at major version number 15:

/* Synthesize OPENSSL_VERSION_NUMBER with the layout 0xMNN00PPSL */
# ifdef OPENSSL_VERSION_PRE_RELEASE
# define _OPENSSL_VERSION_PRE_RELEASE 0x0L
# else
# define _OPENSSL_VERSION_PRE_RELEASE 0xfL
# endif
# define OPENSSL_VERSION_NUMBER \
( (OPENSSL_VERSION_MAJOR<<28) \
|(OPENSSL_VERSION_MINOR<<20) \
|(OPENSSL_VERSION_PATCH<<4) \
|_OPENSSL_VERSION_PRE_RELEASE )

However, this hard deadline still leaves enough room for a fair compromise between you two IMHO. For example, one could schedule

  • deprecation in major version 4 and
  • removal in major version 6.

This would give users more than enough time to adopt their code to the new versioning scheme.

@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Sep 1, 2019

Version checks at runtime by the application - whether carried out correctly or not - are not the right way to go IMHO. Ensuring ABI compatibility for a fixed major version and linking the application to a fixed major version number via soname should render runtime checks obsolete.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 1, 2019

Agreed... But we haven't been very good with that before, so I can understand the scepticism

@DDvO

This comment has been minimized.

Copy link
Contributor

DDvO commented Sep 2, 2019

One cannot rely on the runtime system nor on the lib file name to ensure the right major version is loaded at runtime.

@t-j-h

This comment has been minimized.

Copy link
Member

t-j-h commented Sep 2, 2019

Fixed version names and fixed linkage aren't the right solution. That requires unnecessary pain in many places.

It is possible to write explicit runtime dynamic linking code that works across versions so applications can cross the major version boundary and still work.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 2, 2019

-1

I think we need a general discussion on deprecation. We have very obviously quite different ideas about how, when and why it should or shouldn't be done.

This discussion should preferably happen on the project list, not just within the OMC

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 2, 2019

Thinking of depreciation as a harmless no-op isn't correct in my view.

I don't, but I object to the rethorics that make it sound like immediate removal.

@DDvO

This comment has been minimized.

Copy link
Contributor

DDvO commented Sep 2, 2019

No matter how mild the removal of the version detection function is named ('deprecation' etc.), understood, or implemented, from the moment where the function is not actually available serious backward compatibility issues with older application code occur.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 5, 2019

In the conversation on openssl-project, @dwmw2's response (that someone kindly chose to let in...) resonates with me.

So ok, with that argumentation, I give in. My previous -1 doesn't apply any more.

@t-j-h
t-j-h approved these changes Sep 5, 2019
Copy link
Member

t-j-h left a comment

With @levitte removing the -1 I am approving this.

@t-j-h t-j-h added the approval: done label Sep 5, 2019
@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 5, 2019

Unfortunately, this PR is incomplete, the macro OPENSSL_VERSION_NUMBER is still wrapped with # if !OPENSSL_API_4. If OpenSSL_version_num() gets undeprecated, then so must that macro. Or did I miss something?

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 5, 2019

You were a bit too fast there, @t-j-h 😉

@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Sep 5, 2019

Unfortunately, this PR is incomplete, the macro OPENSSL_VERSION_NUMBER is still wrapped with # if !OPENSSL_API_4. If OpenSSL_version_num() gets undeprecated, then so must that macro. Or did I miss something?

I was just going to write the same thing. Again, you beat me by seconds ;-)

@t-j-h

This comment has been minimized.

Copy link
Member

t-j-h commented Sep 5, 2019

It can be fixed - but it doesn't actually need to be fixed as it works for master with this PR.
If someone wants to add more, feel free. But this change actually fixes the stated problem.
This undoes deprecation at API==3, the other is removal at API==4 which is a different (future) situation.

@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Sep 5, 2019

I don't see anything incompatible here. It'll continue to work.

Well, not forever. There is a hard limit at major version number 15:

I agree with the necessity of postponing the deprecation, but I'd still like to emphasize that deprecation will be inevitable at some time in the feature, the latest in

<ETA of major version 15> - 5 years

Because the version numbering will not just continue to work forever.

@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Sep 5, 2019

This undoes deprecation at API==3, the other is removal at API==4 which is a different (future) situation.

I'm with @levitte, I'd prefer a complete and consistent solution.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 5, 2019

This undoes deprecation at API==3, the other is removal at API==4

(I'm going to point out, every time, when you all get deprecation and removal confused... the confusion is understandable, because the "api" level isn't exactly intuitive, as API==3, which does indicate a deprecation in version 3, also removes the declaration in the corresponding version when the --api options is used with that value... but that's a special configuration for folks that want to live on the bleeding edge)

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 5, 2019

but I'd still like to emphasize that deprecation will be inevitable at some time in the feature, the latest in

Heh, so we need an OPENSSL_API_14 and corresponding DEPRECATEDIN_15? That can be done, but that certainly is for another PR

@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Sep 5, 2019

because the "api" level isn't exactly intuitive

That's why I prefer the no-deprecated configure option, because it's much simpler to comprehend: I say "remove the deprecated stuff" and that's exactly what I get. ;-)

@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Sep 5, 2019

but I'd still like to emphasize that deprecation will be inevitable at some time in the feature, the latest in

Heh, so we need an OPENSSL_API_14 and corresponding DEPRECATEDIN_15? That can be done, but that certainly is for another PR

You forgot about the 5 year grace period. I'd prefer deprecation of the version number in major version 9 and removal in major version 10 five years later. That should be far enough in the future.

@mspncp

This comment has been minimized.

Copy link
Contributor

mspncp commented Sep 5, 2019

Yes, in another pr.

@t-j-h

This comment has been minimized.

Copy link
Member

t-j-h commented Sep 5, 2019

Don't read my short hand notation indicating that I think this is what the code does as I know what the code does (and I think what the code does could be done differently - but what is there is what is there).

And I do think what we do isn't the right logical way of looking at it in terms of its structure - it is somewhat unusual.

We should have the concept of selecting a specific API level (and hence what is actually available) entirely separate from how deprecation warnings are handled and mixing the two together does create a mess that you have to think about when looking at the code - and a lot of people look at the code and have to think about it because it is handled strangely.

If I want to basically say I want to avoid any APIs that aren't in the supported set at a given level then I should be able to do so simply and not have an API macro that uses a value that is different from the macro that is the deprecation markers.

And the issue is that removal happens separately - and with a time period of removal the whole logic being expressed in the header files is incorrect - in that the removal is a separate control that isn't linked to the next largest major version (i.e. if we elected to have more than one major version change in a five year period the current approach doesn't work).

But again - if someone wants to add the extra change to this PR then do so - it isn't required to achieve the stated objective of this PR.

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 5, 2019

So what I'm hearing is that 'no-deprecated' should have a completely separate treatment from the whole api selection thing. That's food for thought, sure enough.

(for another pr)

@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 5, 2019

That being said, if we're removing the OPENSSL_API_3 wrapper around the definition of OpenSSL_version_num, then the OPENSSL_API_4 around the definition of OPENSSL_VERSION_NUMBER should be removed as well. That's simply a question of consistency.

@t-j-h

This comment has been minimized.

Copy link
Member

t-j-h commented Sep 5, 2019

I have no objection to an update to do that.
And consider my approval also applies to that change if you want to add it.

Viktor Dukhovni and others added 2 commits Dec 9, 2018
The OpenSSL_version_num() function returns at runtime the
OPENSSL_VERSION_NUMBER of the compiled OpenSSL library.  This is a
used and useful interface, and should not (at least yet) be
deprecated, we just introduced the new versioning schema, it seems
too early to deprecate the old.
... and OPENSSL_VERSION_NUMBER
@levitte levitte force-pushed the vdukhovni:undeprecate-ovn branch from 96f9a8e to 561d5bd Sep 5, 2019
@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 5, 2019

Seeing that this has been around for so long, I decided to add the commit the resolves my issue. While I was at it, I rebased on top of a fresher master.

I'll take the time to wait for the CIs, and if they are happy, I'll take @t-j-h's pre-approval and merge.

levitte added a commit that referenced this pull request Sep 5, 2019
The OpenSSL_version_num() function returns at runtime the
OPENSSL_VERSION_NUMBER of the compiled OpenSSL library.  This is a
used and useful interface, and should not (at least yet) be
deprecated, we just introduced the new versioning schema, it seems
too early to deprecate the old.

Reviewed-by: Tim Hudson <tjh@openssl.org>
Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from #7853)
@levitte

This comment has been minimized.

Copy link
Member

levitte commented Sep 5, 2019

CIs are happy. Merged. Thanks for all your patience!

7e8c338 Undeprecate OpenSSL_version_num and OPENSSL_VERSION_NUMBER

@levitte levitte closed this Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.