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

Building with BoringSSL should set ossl110, etc. #1944

Open
davidben opened this issue Jun 3, 2023 · 4 comments
Open

Building with BoringSSL should set ossl110, etc. #1944

davidben opened this issue Jun 3, 2023 · 4 comments

Comments

@davidben
Copy link
Contributor

davidben commented Jun 3, 2023

In C, BoringSSL defines OPENSSL_VERSION_NUMBER to match OpenSSL 1.1.1 because, by default, you're expected to use the code you use for OpenSSL 1.1.1. Occasionally things vary from the target version, so we have OPENSSL_IS_BORINGSSL, but the default is to match 1.1.1.

rust-openssl's initial BoringSSL port wasn't done right. Rather than following this standard pattern, it defaults to giving BoringSSL the code for the very oldest OpenSSL! This has caused a lot of mess over the short period of time this port has existed:

We're still not done. The code below was deprecated years before the port existed, and will break very soon. Also it means rust-openssl loses this fix.
https://github.com/sfackler/rust-openssl/blob/master/openssl/src/rsa.rs#L584
https://github.com/sfackler/rust-openssl/blob/master/openssl/src/dsa.rs#L317

I'll upload a PR to do a point fix there, but that is merely fixing a symptom of a structural flaw in the port. The true fix is for rust-openssl to follow the supported pattern in C. The default for OpenSSL derivatives should be to target the OpenSSL version they advertise.

That'll simplify a lot of the cfg(ossl110, boringssl) lines. Though we'll need to clean up some tech debt in the process:

  • Adding cfg(not(boringssl)) on features we don't support (bindings libraries are, alas, a pain point because they tend to expose every feature that exists, even if not used in practice)
  • In some places, we'll probably need to add some compatibility functions to BoringSSL
  • There's some mess with __fixed_rust suffixes on function callbacks which we may need to replicate in bssl-sys. (The old names have been deprecated for a while... is it time to unwind that mess?)

(We also have our own BORINGSSL_API_VERSION define, but that matters less if you only target one BoringSSL. Though @maurer may care about this due to Android.)

CC @maurer @alex @reaperhulk

@alex
Copy link
Collaborator

alex commented Jun 3, 2023

Yes, I agree we should default to treating BoringSSL as its "equivalent" OpenSSL and opt things out as required, instead of the reverse. I had this on my TODO list and then... forgot about it, because I'm an irresponsible person.

@davidben
Copy link
Contributor Author

davidben commented Jun 3, 2023

Oh we should also resolve the bssl-sys situation. Though I think that's blocked on the Android side, as they haven't yet updated to bindgen 0.65. (@maurer, see b/279198502 internally.)

davidben added a commit to davidben/rust-openssl that referenced this issue Jun 3, 2023
The RSA and DSA changes will be needed to avoid build breakage soon. The
others are mostly tidying up. There's another place around BIO that we'd
ideally also switch over, but that depends on resolving the
__fixed_rust mess first.

This addresses a symptom of sfackler#1944, but not the root cause.
@davidben
Copy link
Contributor Author

davidben commented Jun 3, 2023

I'm not sure what way you all want to express it in the build (figure I'll leave that to you), but hacking the cfgs in, I see a few compat functions we'll want on the BoringSSL side. I'll take a pass adding in some of those and we can iterate from there.

@davidben
Copy link
Contributor Author

davidben commented Jun 4, 2023

Playing with this a bit, the other issue is rust-openssl should still pick up osslconf = "OPENSSL_NO_FOO" in BoringSSL and LibreSSL. Both forks set those to disable features, but rust-openssl is ignoring it.

davidben added a commit to davidben/rust-openssl that referenced this issue Jun 4, 2023
Setting ossl110 in the BoringSSL build (see sfackler#1944) causes rust-openssl
to expect OCB support. However, OpenSSL already has a feature guard for
OCB, which BoringSSL sets. rust-openssl just isn't honoring it.

This fixes building against an OpenSSL built with ./config no-ocb
davidben added a commit to davidben/rust-openssl that referenced this issue Jun 4, 2023
The RSA and DSA changes will be needed to avoid build breakage soon. The
others are mostly tidying up. There's another place around BIO that we'd
ideally also switch over, but that depends on resolving the
__fixed_rust mess first.

This addresses a symptom of sfackler#1944, but not the root cause.
davidben added a commit to davidben/rust-openssl that referenced this issue Jun 4, 2023
BoringSSL defines OpenSSL-compatible OPENSSL_NO_* values to aid porting.
This removes a ton of boringssl cfg lines that weren't actually
necessary. This addresses part of issue sfackler#1944, though not the version
number half.
davidben added a commit to google/boringssl that referenced this issue Jun 7, 2023
OpenSSL 1.1.x renamed these functions with an OPENSSL_ prefix.
Unfortunately, rust-openssl uses these, losing type-safety, rather than
the type-safe macros. It currently expects the old, unprefixed names due
to a different bug
(sfackler/rust-openssl#1944), but to fix that,
we'll need to align with the OpenSSL names.

To keep the current version of rust-openssl working, I've preserved the
old names that rust-openssl uses, but we should clear these out.

Bug: 499
Change-Id: I3be56a54ef503620b92ce8154fafd46b2906ae63
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60505
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
andrewhop pushed a commit to andrewhop/aws-lc that referenced this issue Oct 16, 2023
OpenSSL 1.1.x renamed these functions with an OPENSSL_ prefix.
Unfortunately, rust-openssl uses these, losing type-safety, rather than
the type-safe macros. It currently expects the old, unprefixed names due
to a different bug
(sfackler/rust-openssl#1944), but to fix that,
we'll need to align with the OpenSSL names.

To keep the current version of rust-openssl working, I've preserved the
old names that rust-openssl uses, but we should clear these out.

Bug: 499
Change-Id: I3be56a54ef503620b92ce8154fafd46b2906ae63
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60505
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
(cherry picked from commit 99d3c228344ccad4aa29e9ecc2d0f618c9a2b384)
andrewhop pushed a commit to andrewhop/aws-lc that referenced this issue Oct 16, 2023
OpenSSL 1.1.x renamed these functions with an OPENSSL_ prefix.
Unfortunately, rust-openssl uses these, losing type-safety, rather than
the type-safe macros. It currently expects the old, unprefixed names due
to a different bug
(sfackler/rust-openssl#1944), but to fix that,
we'll need to align with the OpenSSL names.

To keep the current version of rust-openssl working, I've preserved the
old names that rust-openssl uses, but we should clear these out.

Bug: 499
Change-Id: I3be56a54ef503620b92ce8154fafd46b2906ae63
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60505
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
(cherry picked from commit 99d3c228344ccad4aa29e9ecc2d0f618c9a2b384)
andrewhop pushed a commit to andrewhop/aws-lc that referenced this issue Oct 20, 2023
OpenSSL 1.1.x renamed these functions with an OPENSSL_ prefix.
Unfortunately, rust-openssl uses these, losing type-safety, rather than
the type-safe macros. It currently expects the old, unprefixed names due
to a different bug
(sfackler/rust-openssl#1944), but to fix that,
we'll need to align with the OpenSSL names.

To keep the current version of rust-openssl working, I've preserved the
old names that rust-openssl uses, but we should clear these out.

Bug: 499
Change-Id: I3be56a54ef503620b92ce8154fafd46b2906ae63
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60505
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
(cherry picked from commit 99d3c228344ccad4aa29e9ecc2d0f618c9a2b384)
andrewhop pushed a commit to andrewhop/aws-lc that referenced this issue Oct 20, 2023
OpenSSL 1.1.x renamed these functions with an OPENSSL_ prefix.
Unfortunately, rust-openssl uses these, losing type-safety, rather than
the type-safe macros. It currently expects the old, unprefixed names due
to a different bug
(sfackler/rust-openssl#1944), but to fix that,
we'll need to align with the OpenSSL names.

To keep the current version of rust-openssl working, I've preserved the
old names that rust-openssl uses, but we should clear these out.

Bug: 499
Change-Id: I3be56a54ef503620b92ce8154fafd46b2906ae63
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60505
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
(cherry picked from commit 99d3c228344ccad4aa29e9ecc2d0f618c9a2b384)
andrewhop pushed a commit to aws/aws-lc that referenced this issue Oct 20, 2023
OpenSSL 1.1.x renamed these functions with an OPENSSL_ prefix.
Unfortunately, rust-openssl uses these, losing type-safety, rather than
the type-safe macros. It currently expects the old, unprefixed names due
to a different bug
(sfackler/rust-openssl#1944), but to fix that,
we'll need to align with the OpenSSL names.

To keep the current version of rust-openssl working, I've preserved the
old names that rust-openssl uses, but we should clear these out.

Bug: 499
Change-Id: I3be56a54ef503620b92ce8154fafd46b2906ae63
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60505
Reviewed-by: Bob Beck <bbe@google.com>
Auto-Submit: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
(cherry picked from commit 99d3c228344ccad4aa29e9ecc2d0f618c9a2b384)
davidben added a commit to davidben/rust-openssl that referenced this issue May 1, 2024
This is yet another symptom of
sfackler#1944.

Ideally rust-openssl would use the type-safe wrappers, which are the
actual public APIs. Right now, rust-openssl's STACK_OF(T) handling is a
safety regression from C. This PR doesn't address the safety issue, only
works around the BoringSSL porting issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants