Skip to content

Add HKDF wrapper#5480

Merged
maskit merged 1 commit intoapache:masterfrom
maskit:hkdf
Jul 22, 2019
Merged

Add HKDF wrapper#5480
maskit merged 1 commit intoapache:masterfrom
maskit:hkdf

Conversation

@maskit
Copy link
Member

@maskit maskit commented May 13, 2019

This wraps HKDF API differences between OpenSSL and BoringSSL. The code come from QUIC branch.

@maskit maskit added the Core label May 13, 2019
@maskit maskit added this to the 9.0.0 milestone May 13, 2019
@maskit maskit self-assigned this May 13, 2019
@maskit
Copy link
Member Author

maskit commented May 14, 2019

@zwoop Does the FreeBSD box have OpenSSL? I suspect it's LibreSSL.

@maskit
Copy link
Member Author

maskit commented May 15, 2019

Hmm, I tried to build with LibreSSL on my local FreeBSD 12 box but the result of configure script doesn't match (check for HKDF_extract should be yes if it was LibreSSL).

I could strict the check so that FreeBSD box can skip HKDF module, but I don't think I should.

What are these? Bundled OpenSSL should be in /usr/lib and /usr/include.

OPENSSL_LDFLAGS: -L/usr/local/lib
OPENSSL_INCLUDES: -I/usr/local/include

@zwoop
Copy link
Contributor

zwoop commented May 17, 2019

I see this on the FreeBSD box:

openssl-1.0.2r,1               SSL and crypto library

root@github3:~ # ls -lrt /usr/local/lib/libssl.so
lrwxr-xr-x  1 root  wheel  11 Mar  2 01:22 /usr/local/lib/libssl.so -> libssl.so.9

and

root@github3:~ # ls -lrt /usr/lib/libssl.so
lrwxr-xr-x  1 root  wheel  13 May 10 19:49 /usr/lib/libssl.so -> libssl.so.111

FreeBSD is weird, in that very little goes in /usr (only "core" stuff, I think), and all things installed with "pkg" goes to /usr/local.

@zwoop
Copy link
Contributor

zwoop commented May 17, 2019

Seems /usr is openssl-1.1.1a and /usr/local is openssl-1.0.2r.

@maskit
Copy link
Member Author

maskit commented May 17, 2019

I find out the cause. It's because of having two versions on a system.

TL;DR, We need to uninstall 1.0.2 from the FreeBSD box.

If you have a source file below on a system that has the both 1.0.2 and 1.1.1, header files will be read from the both include directories (one from 1.0.2 and the other from 1.1.1).

Source file:

#include <openssl/evp.h>
#include <openssl/kdf.h>
int main()
{
#ifndef EVP_PKEY_CTX_hkdf_mode
#error no EVP_PKEY_CTX_hkdf_mode support
#endif
}

$ cpp -E a.cc -I/usr/local/include

(snip...)
# 3166 "/usr/local/include/openssl/ssl.h"
}
# 2 "a.cc" 2
# 1 "/usr/include/openssl/kdf.h" 1 3 4
# 13 "/usr/include/openssl/kdf.h" 3 4
# 1 "/usr/include/openssl/kdferr.h" 1 3 4
# 15 "/usr/include/openssl/kdferr.h" 3 4
extern "C"

int ERR_load_KDF_strings(void);
# 14 "/usr/include/openssl/kdf.h" 2 3 4

extern "C" {
# 95 "/usr/include/openssl/kdf.h" 3 4
}
# 3 "a.cc" 2
int main()
{
(snip...)

I thought it will be a compile error because "kdf.h" is not provided by 1.0.2. But "kdf.h" is actually available on the system and is read from "/usr/include".

So EVP_PKEY_CTX_hkdf_mode is always defined since a compiler can find it in a header file from 1.1.1.

@maskit
Copy link
Member Author

maskit commented May 22, 2019

Technically I could surround "kdf.h" with something like #if VERSION >= 1.1.0 to detect it correctly, but having multiple versions is still a trap (the same thing might happen on other files).

@maskit maskit mentioned this pull request May 29, 2019
12 tasks
@scw00
Copy link
Member

scw00 commented May 29, 2019

+1

@maskit
Copy link
Member Author

maskit commented May 30, 2019

@zwoop Please uninstall OpenSSL 1.0.2 from FreeBSD if you don't mind.

@zwoop
Copy link
Contributor

zwoop commented Jun 6, 2019

Removed OpenSSL v1.0.2

[approve ci]

@zwoop
Copy link
Contributor

zwoop commented Jun 15, 2019

I wonder if we should have a discussion at some point if we want to isolate OpenSSL vs BoringSSL features all into two single files? As such, maybe replace the HKDF prefix on the file names here, and make the two .cc files generic for all library specific details? Then we can have all specific SSL library implementations in each respective file, and also only check once in the Makefile. You’d then remove the check for HAS_HKDF in the Makefile, and move the ifdef there to each .cc file.

I can think of several other things that could go into these generic library specific .cc files, such as OCSP etc.

@bryancall @SolidWallOfCode wdyt?

@maskit
Copy link
Member Author

maskit commented Jun 17, 2019

fwiw, we have similar files for QUIC on the quic branch:

$ ll iocore/net/quic/*_openssl.cc
-rw-r--r--  1 mkitajo  staff   1900 Jun 17 09:49 iocore/net/quic/QUICKeyGenerator_openssl.cc
-rw-r--r--  1 mkitajo  staff   1723 Jun 17 09:49 iocore/net/quic/QUICPacketHeaderProtector_openssl.cc
-rw-r--r--  1 mkitajo  staff   4353 Jun 17 09:49 iocore/net/quic/QUICPacketPayloadProtector_openssl.cc
-rw-r--r--  1 mkitajo  staff  24778 Jun 17 09:49 iocore/net/quic/QUICTLS_openssl.cc

@zwoop
Copy link
Contributor

zwoop commented Jun 17, 2019

Hmmm, those are pretty annoying file names :) if we need to do this, maybe we should have subdirs for openssl/ and boringssl/ ?

@maskit
Copy link
Member Author

maskit commented Jun 17, 2019

I prefer to keep the current structure (no subdirs) because filename completion works well, but if you really don't want to see these files, having subdirs sounds much better than having "tscore_openssl.cc". I don't think people want to put everything under "tscore/" into "tscore.cc".

What is the annoying point exactly?

@maskit
Copy link
Member Author

maskit commented Jul 17, 2019

This is blocking merging QUIC branch, which will be a part of 9.0 release. Please review this as soon as possible..

@maskit maskit merged commit 7999f51 into apache:master Jul 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants