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

Add optional BoringSSL support via configure #343

Merged
merged 5 commits into from
Sep 30, 2015
Merged

Conversation

lminiero
Copy link
Member

This pull request adds optional BoringSSL support via a new flag in the configure script, --enable-boringssl. If enabled, it looks for BoringSSL in a specific folder and updates the Makefile accordingly to make use of it.

The README has also been updated with instructions on how to build BoringSSL, partly derived from info gathered on #136 and partly coming from what I needed to get it working.

Feedback welcome on whether this is indeed working for everybody.

git clone https://boringssl.googlesource.com/boringssl
cd boringssl
# We need a specific revision
git fetch origin 12fe1b25ead258858309d22ffa9e1f9a316358d7
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably better as git checkout 12fe1b25ead258858309d22ffa9e1f9a316358d7

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the heads-up, I'll change that.

@masahji
Copy link

masahji commented Sep 25, 2015

Thanks @lminiero! Everything built just fine for us; however, after checking links with ldd we could still see janus linked to the system libssl. So we build boringssl with shared libs instead (cmake -DBUILD_SHARED_LIBS=1 -DCMAKE_CXX_FLAGS="-lrt" ..) and then added /opt/boringssl/lib as an ldconfig.

It appears as though your instructions call for building boringssl in statically. But I think we'd have to modify autoconf to not link in libssl when building statically.

Thanks

@lminiero
Copy link
Member Author

@masahji actually all my attempts to use a shared version of the library failed somehow, or at least I couldn't find a way to reliably force that upon the stock OpenSSL, so I eventually resigned to using the static build (which is what BoringSSL tries to build by default, anyway). I also noticed ldd still listing the OpenSSL shared objects (as you pointed out, probably because we still have -lssl and -lcrypto), but the nm janus | grep DTLSv1_2 confirms BoringSSL is used apparently.

@ploxiln
Copy link
Contributor

ploxiln commented Sep 25, 2015

btw, if you do end up building shared libs and putting them somewhere besides /usr/lib/, the best way to make janus use them (at run time) is to add to the "rpath", the runtime library path.

As I mentioned in #325 (comment) I build with LDFLAGS="-L/usr/local/lib -Wl,-rpath=/usr/local/lib", and you can see the result of -Wl,-rpath=... in the output from "readelf", and "ldd" will confirm:

$ readelf -d /usr/local/bin/janus

Dynamic section at offset 0x5ad00 contains 38 entries:
  Tag        Type                         Name/Value
 0x0000000000000001 (NEEDED)             Shared library: [libnice.so.10]
 0x0000000000000001 (NEEDED)             Shared library: [libgobject-2.0.so.0]
 0x0000000000000001 (NEEDED)             Shared library: [libglib-2.0.so.0]
 0x0000000000000001 (NEEDED)             Shared library: [libmicrohttpd.so.10]
 0x0000000000000001 (NEEDED)             Shared library: [libjansson.so.4]
 0x0000000000000001 (NEEDED)             Shared library: [libssl.so.1.0.0]
 0x0000000000000001 (NEEDED)             Shared library: [libcrypto.so.1.0.0]
...
 0x0000000000000001 (NEEDED)             Shared library: [libc.so.6]
 0x000000000000000f (RPATH)              Library rpath: [/usr/local/lib]
 0x000000000000000c (INIT)               0x407fb0
 0x000000000000000d (FINI)               0x44b9c4
...
$ ldd /usr/local/bin/janus
    linux-vdso.so.1 =>  (0x00007ffc4d896000)
    libnice.so.10 => /usr/lib/x86_64-linux-gnu/libnice.so.10 (0x00007f464fdbd000)
    libgobject-2.0.so.0 => /usr/lib/x86_64-linux-gnu/libgobject-2.0.so.0 (0x00007f464fb6c000)
    libglib-2.0.so.0 => /lib/x86_64-linux-gnu/libglib-2.0.so.0 (0x00007f464f863000)
    libmicrohttpd.so.10 => /usr/lib/x86_64-linux-gnu/libmicrohttpd.so.10 (0x00007f464f64c000)
    libjansson.so.4 => /usr/lib/x86_64-linux-gnu/libjansson.so.4 (0x00007f464f440000)
    libssl.so.1.0.0 => /usr/local/lib/libssl.so.1.0.0 (0x00007f464f1c6000)
    libcrypto.so.1.0.0 => /usr/local/lib/libcrypto.so.1.0.0 (0x00007f464ed78000)
...

("..." is where I trimmed the output.) You could do the same with "/opt/boringssl/lib"; you can have multiple rpath additions.

@lminiero
Copy link
Member Author

I think I tried playing with rpath for a shared build of BoringSSL as well, using the info you provided in that other discussion, but I still couldn't get it to work, can't remember why. I'll give it another try next week.

@ploxiln
Copy link
Contributor

ploxiln commented Sep 25, 2015

Yeah this stuff can be fiddly and confusing. I wonder if it was due to the linker pulling in the system openssl library on behalf of another shared library before pulling in boringssl on behalf of janus proper... Shared libraries can also have an rpath of their own. So libsophia-sip / libsrtp / etc should also be built against the openssl lib you want to be used.

Now that I think about it, it's possible that janus was statically linking in boringssl stuff while libsophia-sip etc. were dynamically linking to the system openssl.

@lminiero
Copy link
Member Author

Merging for now as it does indeed work for those who are interested in this, we can worry about shared vs. static and other things later.

lminiero added a commit that referenced this pull request Sep 30, 2015
Add optional BoringSSL support via configure
@lminiero lminiero merged commit ed9e24d into master Sep 30, 2015
@lminiero lminiero deleted the boringssl-support branch September 30, 2015 15:25
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

Successfully merging this pull request may close these issues.

3 participants