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.

Sign up for GitHub

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

Jump to bottom

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
@davidben
Fix some deprecated patterns when using BoringSSL
de5ae9c 
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 davidben mentioned this issue Jun 3, 2023
Merged
@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
@davidben
Honor OPENSSL_NO_OCB if OpenSSL was built this way
e476f9a 
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 davidben mentioned this issue Jun 4, 2023
Merged
davidben added a commit to davidben/rust-openssl that referenced this issue Jun 4, 2023
@davidben
Fix some deprecated patterns when using BoringSSL
5283d7c 
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
@davidben
Pick up OPENSSL_NO_* with BoringSSL
931eb46 
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 davidben mentioned this issue Jun 4, 2023
Closed
davidben added a commit to google/boringssl that referenced this issue Jun 7, 2023
@davidben
Prefix the private stack functions
99d3c22 
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>
@hawkw hawkw mentioned this issue Oct 2, 2023
Draft
andrewhop pushed a commit to andrewhop/aws-lc that referenced this issue Oct 16, 2023
@davidben @andrewhop
Prefix the private stack functions
28b9ec9 
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
@davidben @andrewhop
Prefix the private stack functions
7fcd9a6 
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
@davidben @andrewhop
Prefix the private stack functions
bb0fca9 
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
@davidben @andrewhop
Prefix the private stack functions
49c4f36 
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
@davidben @andrewhop
Prefix the private stack functions
edff359 
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 davidben mentioned this issue Nov 19, 2023
Closed
@davidben davidben mentioned this issue Jan 19, 2024
Merged
davidben added a commit to davidben/rust-openssl that referenced this issue May 1, 2024
@davidben
Use the newer names for STACK_OF(T) functions with BoringSSL
ef65d8d 
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.
@davidben davidben mentioned this issue May 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@alex @davidben