Skip to content

Detect boringssl in expando.c? #1768

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

Closed
Tracked by #7164
alex opened this issue Dec 25, 2022 · 10 comments
Closed
Tracked by #7164

Detect boringssl in expando.c? #1768

alex opened this issue Dec 25, 2022 · 10 comments

Comments

@alex
Copy link
Collaborator

alex commented Dec 25, 2022

Right now if you compile against BoringSSL (i.e., point OPENSSL_DIR at it), but don't specify unstable_boringssl it will silently build, but many things won't work (e.g. pyca/cryptography#7933 (comment)).

At a minimum, would it make sense to detect this situation with expando.c and fail the build, rather than silently proceeding?

Even better would be if it could detect this and work -- perhaps by BoringSSL putting the generated bssl-sys crate somewhere that could be detected.

cc: @davidben, @maurer, @benbrittain, @durin42

@sfackler
Copy link
Owner

Yep, agree that we should detect boring and fail the build if it's not done explicitly in boring mode.

@alex
Copy link
Collaborator Author

alex commented Dec 25, 2022

@sfackler do you think the 2nd part makes sense as a follow-up -- provided BoringSSL is interested in it of course.

@sfackler
Copy link
Owner

Yeah I would definitely love to auto-detect bssl-sys - it was discussed on the initial BoringSSL support PR but seemed like it'd be tricky for some reason I've forgotten. For now we should just do the detection bit IMO.

@alex
Copy link
Collaborator Author

alex commented Dec 25, 2022

SGTM.

Shall I prep a PR for the first piece?

@sfackler
Copy link
Owner

👍

@alex
Copy link
Collaborator Author

alex commented Dec 25, 2022

The answer to why its hard seems to be: #1649 (comment)

I'm trying to think if there's a better way to make auto-detection Just Work than by requesting a new cargo feature...

sfackler added a commit that referenced this issue Dec 26, 2022
Refs #1768 -- reject boringssl if unstable_boringssl feature isn't specified
@maurer
Copy link
Contributor

maurer commented Dec 26, 2022

Thanks for fixing this so quickly, by the time I went to look at doing it y'all already had a PR up to fix it.

@alex
Copy link
Collaborator Author

alex commented Dec 26, 2022

No problem -- the first part was the easy part :-)

The hard part is figuring out if there's a way to make it work by default. To get started with this, I figured it'd make sense to get started by listing the design constraints on any potential solution:

BoringSSL constraints:

  • Bindings must be generated as a part of the BoringSSL build process.
  • The current bssl-sys has a build.rs, to specify how to link libcrypto and libssl and also to link some wrapper functions for macros.

pyca/cryptography constraints:

  • We want a unified build process, where any variation is detected from the OpenSSL (or fork thereof)
  • Invocations of cargo/rustc go through setuptools-rust, so we don't have unlimited flexibility.

rust-openssl constraints:

  • Not too much of a mess :-)
  • Package is publishable and usable from crates.io like normal.

So where does this leave us?

cargo needs to know all Rust crate dependencies statically up-front. This means that build.rs cannot be used to determine if bssl-sys will be linked. This means that OpenSSL's headers cannot determine which Rust crates we use.

This means either:

a) BoringSSL's bindings shouldn't be published as a crate, instead they should be a single Rust source file that can be include!().
b) bssl-sys needs to be in the dependency graph unconditionally.

The blockers to (a) are 1) the C wrapper functions, 2) the link directives in build.rs, 3) any potential future desires to add more things to the crate. (1) can be addressed by simply exporting these functions from BoringSSL's normal archive/object files, (2) can be addressed by letting openss-sys handle the linking (this already works fine), (3) I can't speak to :-)

A design to make (b) work would be something: A bssl-sys crate is published to crates.io. It is a skeleton, no substantive bindings in it. It has a build.rs which finds the real bindings generated by the BoringSSL build process, and which are pointed to by the environment variables used to configure rust-openssl's existing build process. If those environment variables point at a non-BoringSSL installation, the crate is a no-op. If they point at an OpenSSL, they copy those bindings into the bssl-sys crate and build them. Then they're exposed through openssl-sys.

I think either of these designs can work. (a) is a bit simpler, but ultimately less flexible.

I'd appreciate any thoughts from everyone on whether I missed any considerations, or if they have a preferred solution! Happy Holidays ❄️ ❄️ ❄️

@alex
Copy link
Collaborator Author

alex commented Jan 17, 2023

Hi all, hope folks had good holidays which were free from build system horrors.

I wanted to re-up this and see if folks had views: most importantly on if I have the constraints right? But also if there's a preference on what a solution might look like.

I'm enthusiastic to get to working on this. For myself, I think (a) will be easier, so all other things being equal I'd probably do that.

alex added a commit to alex/rust-openssl that referenced this issue Feb 2, 2023
…e naturally

This PR uses the in-development bindgen support for static inline functions (rust-lang/rust-bindgen#2335) + an in-development boringssl patch (https://boringssl-review.googlesource.com/c/boringssl/+/56505) to allow using boringssl with rust-openssl without needing a .cargo/config override
alex added a commit to alex/rust-openssl that referenced this issue Feb 3, 2023
…e naturally

This PR uses the in-development bindgen support for static inline functions (rust-lang/rust-bindgen#2335) + an in-development boringssl patch (https://boringssl-review.googlesource.com/c/boringssl/+/56505) to allow using boringssl with rust-openssl without needing a .cargo/config override
alex added a commit to alex/rust-openssl that referenced this issue Feb 3, 2023
…e naturally

This PR uses the in-development bindgen support for static inline functions (rust-lang/rust-bindgen#2335) + an in-development boringssl patch (https://boringssl-review.googlesource.com/c/boringssl/+/56505) to allow using boringssl with rust-openssl without needing a .cargo/config override
alex added a commit to alex/rust-openssl that referenced this issue Feb 7, 2023
…e naturally

This PR uses the in-development bindgen support for static inline functions (rust-lang/rust-bindgen#2335) + an in-development boringssl patch (https://boringssl-review.googlesource.com/c/boringssl/+/56505) to allow using boringssl with rust-openssl without needing a .cargo/config override
alex added a commit to alex/rust-openssl that referenced this issue Mar 4, 2023
This allows building it without the bssl-sys crate. This is an alternative approach to fixing sfackler#1768 (in contrast to sfackler#1806).

This maintains support for using the bssl-sys crate.
alex added a commit to alex/rust-openssl that referenced this issue Mar 4, 2023
This allows building it without the bssl-sys crate. This is an alternative approach to fixing sfackler#1768 (in contrast to sfackler#1806).

This maintains support for using the bssl-sys crate.
alex added a commit to alex/rust-openssl that referenced this issue Mar 4, 2023
This allows building it without the bssl-sys crate. This is an alternative approach to fixing sfackler#1768 (in contrast to sfackler#1806).

This maintains support for using the bssl-sys crate.
alex added a commit to alex/rust-openssl that referenced this issue Mar 4, 2023
This allows building it without the bssl-sys crate. This is an alternative approach to fixing sfackler#1768 (in contrast to sfackler#1806).

This maintains support for using the bssl-sys crate.
alex added a commit to alex/rust-openssl that referenced this issue Mar 4, 2023
This allows building it without the bssl-sys crate. This is an alternative approach to fixing sfackler#1768 (in contrast to sfackler#1806).

This maintains support for using the bssl-sys crate.
alex added a commit to alex/rust-openssl that referenced this issue Mar 4, 2023
This allows building it without the bssl-sys crate. This is an alternative approach to fixing sfackler#1768 (in contrast to sfackler#1806).

This maintains support for using the bssl-sys crate.
alex added a commit to alex/rust-openssl that referenced this issue Mar 4, 2023
This allows building it without the bssl-sys crate. This is an alternative approach to fixing sfackler#1768 (in contrast to sfackler#1806).

This maintains support for using the bssl-sys crate.
alex added a commit to alex/rust-openssl that referenced this issue Mar 10, 2023
This allows building it without the bssl-sys crate. This is an alternative approach to fixing sfackler#1768 (in contrast to sfackler#1806).

This maintains support for using the bssl-sys crate.
alex added a commit to alex/rust-openssl that referenced this issue Mar 10, 2023
This allows building it without the bssl-sys crate. This is an alternative approach to fixing sfackler#1768 (in contrast to sfackler#1806).

This maintains support for using the bssl-sys crate.
@alex
Copy link
Collaborator Author

alex commented Mar 15, 2023

resolved by #1831

@alex alex closed this as completed Mar 15, 2023
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

3 participants