| Message ID | 20160505211603.GD30363@w1.fi |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, May 5, 2016 at 5:16 PM, Jouni Malinen <j@w1.fi> wrote: > This is problematic with existing versions of BoringSSL. As an example, > if I apply this and try to build against my previous BoringSSL build, > the compilation fails with: > > CC ../src/eap_peer/eap_tls_common.c > ../src/crypto/tls_openssl.c: In function ‘tls_connection_get_random’: > ../src/crypto/tls_openssl.c:3079:2: error: implicit declaration of function ‘SSL_get_client_random’ [-Werror=implicit-function-declaration] > keys->client_random_len = SSL_get_client_random( > ^ > ../src/crypto/tls_openssl.c:3082:2: error: implicit declaration of function ‘SSL_get_server_random’ [-Werror=implicit-function-declaration] > keys->server_random_len = SSL_get_server_random( > ^ > ../src/crypto/tls_openssl.c: In function ‘openssl_tls_prf’: > ../src/crypto/tls_openssl.c:3203:2: error: implicit declaration of function ‘SSL_SESSION_get_master_key’ > [-Werror=implicit-function-declaration] > master_key_len = SSL_SESSION_get_master_key(sess, master_key, > ^ > > > IMHO, it is quite unfortunate that BoringSSL is maintained in a manner > that prevents clean backwards compatibility with at least the versions > used in the recent past. Applying this patch would break various Android > cases where the BoringSSL version in the branch is not sufficiently > recent to have the macro defined. > > Would there be any other way of using the pre-processor to automatically > determine whether BoringSSL is recent enough to include the new > commands? The "Add SSL_get_client_random and SSL_get_server_random" > doesn't seem to add anything to the header files to help the > pre-processor for this.. So, previously, our approach was not to try to support old versions of BoringSSL much. It's annoying having our own code (much less yours!) saddled with #ifdefs for our past mistakes. (You all have enough of a mess on your hands with multiple versions of OpenSSL. I don't want to make things worse!) Instead, we only use it in environments with controlled versions that move forwards, so we can coordinate caller and callee updates, sometimes even atomically. If atomic changes aren't possible, we'd add very temporary scaffolding #defines and #ifdefs locally, get to the end state (caller and callee both assumed new enough) as fast as possible, and dismantle it. Though talking with Dmitry some more, it sounds like it's actually desirable to be able to build newer wpa_supplicant against a release or two behind of AOSP? Is that right? This isn't how we had previous done things on the BoringSSL end, but we can certainly accommodate it. I can start a BORINGSSL_API_VERSION counter and roll that into AOSP now. This will be a random meaningless number except we promise it will only increase and we'll probably increment it at points vaguely corresponding with additions or changes in the API, wherever it ends up convenient to do so. :-) Then this patch will be updated to be defined(BORINGSSL_API_VERSION). In future we'd do BORINGSSL_API_VERSION > whatever. And then you all can figure out how far back it should go. (For my part, I want to minimize your burden, so I would encourage you need to retain support for versions older than you need, but it sounds like your master branch cares about more Android releases than I thought.) Does that sound reasonable? >> BoringSSL added 1.1.0's SSL_get_client_random and friends in working towards >> opaquifying the SSL struct. But it, for the moment, still looks more like 1.0.2 >> than 1.1.0 and advertises OPENSSL_VERSION_NUMBER as such. This means that there >> is no need to define those in BoringSSL and defining them causes conflicts. (C >> does not like having static and non-static functions with the same name.) > > I guess the wrapper functions in src/crypto/tls_openssl.c could be > defined to us an alternative name and then use #define to override the > functions. Not that this is exactly nice, but at least it seems to build > with the current boringssl.git snapshot: > > diff --git a/src/crypto/tls_openssl.c b/src/crypto/tls_openssl.c > index ebcc545..0f67290 100644 > --- a/src/crypto/tls_openssl.c > +++ b/src/crypto/tls_openssl.c > [snip] Yeah, that's horrific. :-) I would definitely prefer changing our strategy over forcing you to do that! If we start the BORINGSSL_API_VERSION counter, the only versions subject to the naming conflict would be those between when I added SSL_get_client_random and when I add BORINGSSL_API_VERSION. I'll update AOSP as soon that happens, so no released version of AOSP will check a BoringSSL revision in that window. That should avoid this. David
On Thu, May 05, 2016 at 06:41:19PM -0400, David Benjamin wrote: > So, previously, our approach was not to try to support old versions of > BoringSSL much. It's annoying having our own code (much less yours!) > saddled with #ifdefs for our past mistakes. (You all have enough of a > mess on your hands with multiple versions of OpenSSL. I don't want to > make things worse!) Instead, we only use it in environments with > controlled versions that move forwards, so we can coordinate caller > and callee updates, sometimes even atomically. If atomic changes > aren't possible, we'd add very temporary scaffolding #defines and > #ifdefs locally, get to the end state (caller and callee both assumed > new enough) as fast as possible, and dismantle it. In theory, that would be nice in case all components using BoringSSL are only used in the same controlled environment.. > Though talking with Dmitry some more, it sounds like it's actually > desirable to be able to build newer wpa_supplicant against a release > or two behind of AOSP? Is that right? This isn't how we had previous > done things on the BoringSSL end, but we can certainly accommodate it. Yes, my goal is to be able to take a master branch snapshot from hostap.git and be able to build wpa_supplicant from that with at least the latest releases AOSP version or ideally two if the previous one is still being used in significant new development. Quite a bit of Wi-Fi related work happens with and on Android versions that are a bit older than what is in the latest development trees (especially if those trees are not always publicly available). If this can be maintained with couple of well placed #ifdefs, then so be it. I'm willing to take that temporary extra complexity to get the benefits of having a new version that can be built on top of Android versions that are used as the baseline for ongoing development and product releases. > I can start a BORINGSSL_API_VERSION counter and roll that into AOSP > now. This will be a random meaningless number except we promise it > will only increase and we'll probably increment it at points vaguely > corresponding with additions or changes in the API, wherever it ends > up convenient to do so. :-) Then this patch will be updated to be > defined(BORINGSSL_API_VERSION). In future we'd do > BORINGSSL_API_VERSION > whatever. And then you all can figure out how > far back it should go. (For my part, I want to minimize your burden, > so I would encourage you need to retain support for versions older > than you need, but it sounds like your master branch cares about more > Android releases than I thought.) > > Does that sound reasonable? Yes, this would work fine for me and the updated patch you sent looked good as well. And as a side note, most of my automated testing is on non-Android platforms with Linux, so it is a significant benefit to be able to build the latest wpa_supplicant with number of snapshots of BoringSSL on x86_64 Linux. This allows both the current master branch of BoringSSL (i.e., what to expect to see in the products in the future) and various releases version (e.g., based on Android release tags) being used to run through the mac80211_hwsim test runs for regression testing and to come up with new test cases to verify issues that may show up in this area.
On Tue, May 10, 2016 at 12:48 PM, Jouni Malinen <j@w1.fi> wrote: >> I can start a BORINGSSL_API_VERSION counter and roll that into AOSP >> now. This will be a random meaningless number except we promise it >> will only increase and we'll probably increment it at points vaguely >> corresponding with additions or changes in the API, wherever it ends >> up convenient to do so. :-) Then this patch will be updated to be >> defined(BORINGSSL_API_VERSION). In future we'd do >> BORINGSSL_API_VERSION > whatever. And then you all can figure out how >> far back it should go. (For my part, I want to minimize your burden, >> so I would encourage you need to retain support for versions older >> than you need, but it sounds like your master branch cares about more >> Android releases than I thought.) >> >> Does that sound reasonable? > > Yes, this would work fine for me and the updated patch you sent looked > good as well. > > And as a side note, most of my automated testing is on non-Android > platforms with Linux, so it is a significant benefit to be able to build > the latest wpa_supplicant with number of snapshots of BoringSSL on > x86_64 Linux. This allows both the current master branch of BoringSSL > (i.e., what to expect to see in the products in the future) and various > releases version (e.g., based on Android release tags) being used to run > through the mac80211_hwsim test runs for regression testing and to come > up with new test cases to verify issues that may show up in this area. Sounds good. I'll bump BORINGSSL_API_VERSION where relevant for you all in future then. I'll leave cleaning up any old codepaths to you, since I don't know how far back you want to go. Is there anything else you need from me w.r.t. the updated patch, or is it good to go? external/boringssl on AOSP master is now updated past the point where BORINGSSL_API_VERSION is defined, so that should be all good. David
diff --git a/src/crypto/tls_openssl.c b/src/crypto/tls_openssl.c index ebcc545..0f67290 100644 --- a/src/crypto/tls_openssl.c +++ b/src/crypto/tls_openssl.c @@ -57,8 +57,8 @@ typedef int stack_index_t; * 1.1.0. Provide compatibility wrappers for older versions. */ -static size_t SSL_get_client_random(const SSL *ssl, unsigned char *out, - size_t outlen) +static size_t _SSL_get_client_random(const SSL *ssl, unsigned char *out, + size_t outlen) { if (!ssl->s3 || outlen < SSL3_RANDOM_SIZE) return 0; @@ -66,9 +66,11 @@ static size_t SSL_get_client_random(const SSL *ssl, unsigned char *out, return SSL3_RANDOM_SIZE; } +#define SSL_get_client_random _SSL_get_client_random -static size_t SSL_get_server_random(const SSL *ssl, unsigned char *out, - size_t outlen) + +static size_t _SSL_get_server_random(const SSL *ssl, unsigned char *out, + size_t outlen) { if (!ssl->s3 || outlen < SSL3_RANDOM_SIZE) return 0; @@ -76,9 +78,10 @@ static size_t SSL_get_server_random(const SSL *ssl, unsigned char *out, return SSL3_RANDOM_SIZE; } +#define SSL_get_server_random _SSL_get_server_random -static size_t SSL_SESSION_get_master_key(const SSL_SESSION *session, - unsigned char *out, size_t outlen) +static size_t _SSL_SESSION_get_master_key(const SSL_SESSION *session, + unsigned char *out, size_t outlen) { if (!session || session->master_key_length < 0 || (size_t) session->master_key_length > outlen) @@ -89,6 +92,8 @@ static size_t SSL_SESSION_get_master_key(const SSL_SESSION *session, return outlen; } +#define SSL_SESSION_get_master_key _SSL_SESSION_get_master_key + #endif #ifdef ANDROID