Opened 7 years ago

Closed 6 years ago

#2406 closed enhancement (fixed)

alternative crypto provider (use OpenSSL rather than Botan)

Reported by: fdupont Owned by: fdupont
Priority: low Milestone: Kea0.9
Component: crypto Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 7 Add Hours to Ticket: 2
Total Hours: 28 Internal?: no

Description

A ticket for my own R&D/learning.
The idea is to replace Botan by another crypto provider. Of course it is easier today with only HMAC then it will be with RSA & co.

Subtickets

Change History (46)

comment:1 Changed 7 years ago by fdupont

Done...
BTW I found (or found again) an issue about TSIG vs truncated HMAC. Botan/current code doesn't support truncated HMAC so can't get the problem... I modified the OpenSSL code to accept truncated HMAC down to the half size.

comment:2 follow-up: Changed 7 years ago by shane

Is there a git branch for this?

comment:3 in reply to: ↑ 2 Changed 7 years ago by fdupont

Replying to shane:

Is there a git branch for this?

trac2406 but it is more than a private experiment than anything else, i.e., what you should conclude is to replace Botan by something else (OpenSSL here but PKCS#11 is of course more interesting :-) is possible and what amount of work it needs, not than Botan should be replaced...

comment:4 Changed 7 years ago by vorner

Well, if it was working, we could switch at compile time which one is used. Maybe even auto-detect which one is available.

comment:5 Changed 7 years ago by shane

  • Milestone New Tasks deleted

comment:6 Changed 6 years ago by tomek

  • Milestone set to DHCP-Kea-proposed

comment:7 Changed 6 years ago by tomek

  • Summary changed from alternative crypto provider to alternative crypto provider (use OpenSSL rather than Botan)

comment:8 Changed 6 years ago by tomek

  • Priority changed from low to medium

comment:9 Changed 6 years ago by tomek

  • Milestone changed from Kea-proposed to Kea0.9
  • Priority changed from medium to low

comment:10 Changed 6 years ago by fdupont

Ported old code to Kea. Still TODO:

  • support both Botan and OpenSSL (vs. replace Botan by OpenSSL) in the code, making the choice at configure time.
  • remove direct use of Botan in DDNS code (highest priority as it is a blocking item).
  • tests and (if needed) fixes.

comment:11 Changed 6 years ago by fdupont

Did the second item (removed direct use of Botan (vs. crypto link with hash support) in DDNS code) in _cl branch.
Todo:

  • support both Botan and OpenSSL
  • merge with _cl stuff (including the hash support)

For the first item we have to decide the default (for instance by default try Botan then OpenSSL, and refuse configure with both?)

comment:12 Changed 6 years ago by fdupont

Done at the exception of src/bin/cmdctl/openssl-certgen.c which is not finished. I suggest to postpone the remaining work.
The branch to review and merge is trac2406km for kea.

comment:13 Changed 6 years ago by fdupont

Finished src/bin/cmdctl/openssl-certgen.cc code (extensions, validate).
Cloned *-certgen_test.py for validation error codes so 'make check' passes.
Updated build guide to explain the new --with-openssl configure option.

Ready for final review and merge (still Kea trac2406km branch).

comment:14 Changed 6 years ago by tomek

  • Owner changed from fdupont to tomek
  • Status changed from new to reviewing

comment:15 follow-ups: Changed 6 years ago by tomek

  • Add Hours to Ticket changed from 0 to 16
  • Total Hours changed from 0 to 16

I did review the changes made on branch trac2406km up to and including commit cf42969f5d0996cd525bb3010fa7be88b39f8c4b. I did push one fix (a17ae64b57dbce5f8a6d00cf75e756503663c2a4) for User's Guide.

configure.ac
Comment for --with-openssl is confusing. I don't know what path
should I specify on Ubuntu 13.10. Path to openssl tool
(/usr/bin/openssl)? The comment for this parameter should also
explain what path is expected and that "yes", "no" and "auto"
are valid parameters.

I managed to compile it using --with-openssl=yes. It took me a while
to discover that yes is viable selection. I did expect that it would
only accept paths.

When compiling with OpenSSL, the config.report file should have
a line "Botan: no" and vice versa.

In line 896 you assume that openssl 1.0.0 is available.
That may not be true in a year or so.

Please remove line 953 which is commented out and doesn't do anything.

doc/guide/bind10-guide.xml
The guide failed to generate as you included double hyphen in
a comment, which is not allowed. I fixed that and pushed my changes.

src/bin/cmdctl

I chose to do only very casual reviews of all changes in src/bin/cmdctl as this
is part of the Bundy framework that is going away really soon (next
couple weeks). So I'm afraid the changes you did in this particular area
are going away with the whole framework.

The only comment I have is that you should remove first commented out
line in main() in openssl-certgen.cc.

I compared openssl-certgen.cc with botan-certgen.cc. Aren't those
two supposed to be exactly the same and just use cryptolink? That's
just a question out of curiosity. Please do not update the code.
As I said above, the whole dir is going away as soon as we convert
all Kea components to be able to read configs from files.

Why do you need 2 separate test files (botan-certgen_test.py and
openssl-certgen_test.py) when they differ with only a single number?

src/lib/cryptolink/botan_hash.cc

getBotanHashAlgorithmName(): why are there breaks after return?
Some compilers will complain about unreachable code.

Also, the last label:

    case isc::cryptolink::UNKNOWN_HASH:

Could be extended with default: and then there could be just
a single return ("Unknown"); statement.

Please add Doxygen comments to HashImpl? methods and fields.

src/lib/cryptolink/openssl_hash.cc
getOpenSSLHashAlgorithm() see comment for getBotanHashAlgorithmName()
above. Also, since this function returns pointers, it should
use return (NULL); rather than return (0);

Please add Doxygen comments to HashImpl? methods and fields.

src/lib/cryptolink/botan_hmac.cc
getBotanHashAlgorithmName() is duplicated here and in botan_hash.cc.

verify(): please add / @todo someware around comments
regarding temporary change.

Please add Doxygen comments to HMACImpl methods and fields.

src/lib/cryptolink/openssl_hmac.cc
getOpenSSLHashAlgorithm() is duplicated here and in openssl_hash.cc.

Please add doxygen comments explaining what the SecBuf? template
is for.

Please add Doxygen comments to HMACImpl methods and fields.

src/lib/cryptolink/openssl_link.cc
initialize(): Can you catch std::exception instead of ...?
Then you could print ex.what(). It's better than reporting general
InitializationError? without any clues what went wrong.

src/lib/cryptolink/tests/hash_unittests.cc
Please add brief Doxygen comments for all functions.

doHashTestArray(): unsafe result allocation. You can keep its
pointer with boost::scoped_array. It will release the array
in a safe manner, even if there's exception.

src/lib/cryptolink/tests/hmac_unittests.cc
Please add brief Doxygen comments for all functions.

Around line 255: There is special case for Botan, but there is none
for OpenSSL. If it's easy, can you add one for OpenSSL? If it's
tricky, feel free to add @todo in the code and create new ticket
for it. If no such case is needed for OpenSSL, please add a comment
that explains why.

#ifndef WITH_BOTAN around line 541:
I would very much prefer to have the following construct:

#ifndef WITH_BOTAN
TEST(HMACTest, HMAC_SHA256_RFC2202_SIGN_TRUNCATED) {
#else
TEST(HMACTest, DISABLED_HMAC_SHA256_RFC2202_SIGN_TRUNCATED) {
#endif

This way, we will know that we are missing some tests for OpenSSL.
Also, if it is feasible to implement those tests for OpenSSL,
please add / @todo in the code.

Tests should use lowerCamelCase notation for their names.


General comment. Most of the implementation methods in
{botan,openssl}_{hmac,hash}.cc have the descriptions already
available in crypto_{hash,hmac}.h. Adding simple doxygen comment
with something like "see @ref Hash::update()" will do the trick.


I checked that the source code builds on Ubuntu 13.10 x64 in both
configurations: Botan and OpenSSL. There will be a follow-up comment
with results of build/make check on Mac.

comment:16 Changed 6 years ago by tomek

  • Component changed from Unclassified to crypto

Added component crypto.

comment:17 follow-up: Changed 6 years ago by tomek

I'm afraid this ticket is branched from too old master to compile. I had to cherry-pick changes from trac3449 (they are on master already)
to just complete configure process. I pushed those commits to trac2406km.

comment:18 Changed 6 years ago by tomek

  • Add Hours to Ticket changed from 16 to 2
  • Owner changed from tomek to fdupont
  • Total Hours changed from 16 to 18

After those changes, I did the following on Mac OS X 10.9.2

$g++ --version
Configured with: --prefix=/Library/Developer/CommandLineTools/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 5.1 (clang-503.0.40) (based on LLVM 3.4svn)
Target: x86_64-apple-darwin13.1.0
Thread model: posix

I did the following:

autoreconf -i && ./configure --with-gtest-source=/Users/thomson/devel/gtest-1.7.0 --with-openssl=yes && make -j9

It detected:

OpenSSL:
  CRYPTO_VERSION:  OpenSSL 0.9.8y 5 Feb 2013
  CRYPTO_INCLUDES: 
  CRYPTO_LDFLAGS:  
  CRYPTO_LIBS:     -lcrypto

Compilation failed with the following error:

Making all in cryptolink
Making all in .
  CXX      openssl_link.lo
  CXX      openssl_hash.lo
  CXX      openssl_hmac.lo
openssl_hash.ccopenssl_hmac.cc:29::2917::17 : error: error: 'EVP_md5''EVP_md5'  isis  deprecated:deprecated:  firstfirst  deprecateddeprecated  inin  OSOS  XX  10.710.7  [-Werror,-Wdeprecated-declarations][-Werror,-Wdeprecated-declarations]

        return (EVP_md5());        return (EVP_md5());

                ^                ^

/usr/include/openssl/evp.h:662:15: note: /usr/include/openssl/evp.h:'EVP_md5'662 :declared15 :here 
note: 'EVP_md5' declared here
const EVP_MD *EVP_md5(void) DEPRECATED_IN_MAC_OS_X_VERSION_10_7_AND_LATER;
              ^
const EVP_MD *EVP_md5(void) DEPRECATED_IN_MAC_OS_X_VERSION_10_7_AND_LATER;
              ^
openssl_hash.cc:openssl_hmac.cc32::3217::17 : error: error: 'EVP_sha1' is'EVP_sha1'  deprecated:is  firstdeprecated:  firstdeprecated  deprecatedin  inOS  OSX  X10.7  10.7[-Werror,-Wdeprecated-declarations] [-Werror,-Wdeprecated-declarations]

        return (EVP_sha1());
        return (EVP_sha1());
                ^
                ^
/usr/include/openssl/evp.h:/usr/include/openssl/evp.h666::66615::15 : notenote: : 'EVP_sha1''EVP_sha1' declared  declaredhere here

const EVP_MD *EVP_sha1(void) DEPRECATED_IN_MAC_OS_X_VERSION_10_7_AND_LATER;
              ^
const EVP_MD *EVP_sha1(void) DEPRECATED_IN_MAC_OS_X_VERSION_10_7_AND_LATER;
              ^
openssl_hmac.cc:35:17: error: 'EVP_sha256' is deprecated: first deprecated in OS X 10.7 [-Werror,-Wdeprecated-declarations]
        return (EVP_sha256());
openssl_hash.cc:                ^35
:17: /usr/include/openssl/evp.h:error673: :15: 'EVP_sha256' isnote : deprecated: 'EVP_sha256'first  declareddeprecated  herein 
OS X 10.7 [-Werror,-Wdeprecated-declarations]
        return (EVP_sha256());
const EVP_MD *EVP_sha256(void) DEPRECATED_IN_MAC_OS_X_VERSION_10_7_AND_LATER;
                ^
              ^
/usr/include/openssl/evp.h:673:15: note: 'EVP_sha256' declared hereopenssl_hmac.cc

Compilation and make check with botan went fine.

comment:19 follow-ups: Changed 6 years ago by tomek

I found one other issue: make distcheck failed:

make[5]: Entering directory `/home/thomson/devel/kea2/bind10-20140313/_build/src/bin/cmdctl'
make[5]: Nothing to be done for `check-am'.
make[5]: Leaving directory `/home/thomson/devel/kea2/bind10-20140313/_build/src/bin/cmdctl'
Making check in tests
make[5]: Entering directory `/home/thomson/devel/kea2/bind10-20140313/_build/src/bin/cmdctl/tests'
make  check-local
make[6]: Entering directory `/home/thomson/devel/kea2/bind10-20140313/_build/src/bin/cmdctl/tests'
for pytest in cmdctl_test.py botan-certgen_test.py  ; do \
	echo Running test: $pytest ; \
	 \
	PYTHONPATH=/home/thomson/devel/kea2/bind10-20140313/_build/src/lib/python/isc/log_messages:/home/thomson/devel/kea2/bind10-20140313/_build/src/lib/python/isc/cc:/home/thomson/devel/kea2/bind10-20140313/_build/../src/lib/python:/home/thomson/devel/kea2/bind10-20140313/_build/src/lib/python:/home/thomson/devel/kea2/bind10-20140313/_build/src/lib/dns/python/.libs:/home/thomson/devel/kea2/bind10-20140313/_build/src/bin/cmdctl \
	CMDCTL_SRC_PATH=/home/thomson/devel/kea2/bind10-20140313/_build/../src/bin/cmdctl \
	CMDCTL_BUILD_PATH=/home/thomson/devel/kea2/bind10-20140313/_build/src/bin/cmdctl \
	B10_LOCKFILE_DIR_FROM_BUILD=/home/thomson/devel/kea2/bind10-20140313/_build \
	/usr/bin/python3.3 /home/thomson/devel/kea2/bind10-20140313/_build/../src/bin/cmdctl/tests/$pytest || exit ; \
	done
Running test: cmdctl_test.py
.......................................
----------------------------------------------------------------------
Ran 39 tests in 0.015s

OK
Running test: botan-certgen_test.py
/usr/bin/python3.3: can't open file '/home/thomson/devel/kea2/bind10-20140313/_build/../src/bin/cmdctl/tests/botan-certgen_test.py': [Errno 2] No such file or directory
make[6]: *** [check-local] Error 2
make[6]: Leaving directory `/home/thomson/devel/kea2/bind10-20140313/_build/src/bin/cmdctl/tests'
make[5]: *** [check-am] Error 2
make[5]: Leaving directory `/home/thomson/devel/kea2/bind10-20140313/_build/src/bin/cmdctl/tests'
make[4]: *** [check-recursive] Error 1
make[4]: Leaving directory `/home/thomson/devel/kea2/bind10-20140313/_build/src/bin/cmdctl'
make[3]: *** [check-recursive] Error 1
make[3]: Leaving directory `/home/thomson/devel/kea2/bind10-20140313/_build/src/bin'
make[2]: *** [check-recursive] Error 1
make[2]: Leaving directory `/home/thomson/devel/kea2/bind10-20140313/_build/src'
make[1]: *** [check-recursive] Error 1
make[1]: Leaving directory `/home/thomson/devel/kea2/bind10-20140313/_build'
make: *** [distcheck] Error 1

comment:20 in reply to: ↑ 15 Changed 6 years ago by fdupont

Replying to tomek:

configure.ac
Comment for --with-openssl is confusing. I don't know what path
should I specify on Ubuntu 13.10.

=> IMHO --with-openssl[=PATH] is better.

Path to openssl tool
(/usr/bin/openssl)? The comment for this parameter should also
explain what path is expected and that "yes", "no" and "auto"
are valid parameters.

=> the expected common case is --with-openssl. And "auto" is an internal value.

I managed to compile it using --with-openssl=yes. It took me a while
to discover that yes is viable selection. I did expect that it would
only accept paths.

When compiling with OpenSSL, the config.report file should have
a line "Botan: no" and vice versa.

=> the report says what is the crypto backend, isn't it enough?

In line 896 you assume that openssl 1.0.0 is available.
That may not be true in a year or so.

=> I copied the Botan stuff which has the same issue.

Please remove line 953 which is commented out and doesn't do anything.

=> I prefer to keep it until we are sure there is no platform where -ldl is required.

Commits d9b41c4..1d04245

comment:21 in reply to: ↑ 15 Changed 6 years ago by fdupont

Replying to tomek:

src/bin/cmdctl

I chose to do only very casual reviews of all changes in src/bin/cmdctl as this
is part of the Bundy framework that is going away really soon (next
couple weeks). So I'm afraid the changes you did in this particular area
are going away with the whole framework.

=> this is why I invested the minimum into it.

The only comment I have is that you should remove first commented out
line in main() in openssl-certgen.cc.

=> do you mean ERR_load_crypto_strings();? It doesn't change things as no ERR_* function is called now (and the code will be dropped...).

I compared openssl-certgen.cc with botan-certgen.cc. Aren't those
two supposed to be exactly the same and just use crypto link?

=> crypto link doesn't provide the needed API.

That's
just a question out of curiosity. Please do not update the code.
As I said above, the whole dir is going away as soon as we convert
all Kea components to be able to read configs from files.

Why do you need 2 separate test files (botan-certgen_test.py and
openssl-certgen_test.py) when they differ with only a single number?

=> because Botan and OpenSSL differ only in one case... Did you say silly?

comment:22 in reply to: ↑ 15 ; follow-up: Changed 6 years ago by fdupont

Replying to tomek:

src/lib/cryptolink/botan_hash.cc

getBotanHashAlgorithmName(): why are there breaks after return?
Some compilers will complain about unreachable code.

=> this was already in the old cryptolink code so if you believe it should be changed please open a new ticket.

Also, the last label:

    case isc::cryptolink::UNKNOWN_HASH:

Could be extended with default: and then there could be just
a single return ("Unknown"); statement.

=> same.

Please add Doxygen comments to HashImpl? methods and fields.

=> same.

src/lib/cryptolink/openssl_hash.cc
getOpenSSLHashAlgorithm() see comment for getBotanHashAlgorithmName()
above.

=> same

Also, since this function returns pointers, it should
use return (NULL); rather than return (0);

=> NULL is C, not C++ (and null_ptr is newer C++)

Please add Doxygen comments to HashImpl? methods and fields.

=> new ticket again?

src/lib/cryptolink/botan_hmac.cc
getBotanHashAlgorithmName() is duplicated here and in botan_hash.cc.

=> yes, in a better API there should be a relationship between Hash and HMAC so no duplicate.

verify(): please add / @todo someware around comments
regarding temporary change.

=> inherited... new ticket?

Please add Doxygen comments to HMACImpl methods and fields.

=> same

src/lib/cryptolink/openssl_hmac.cc
getOpenSSLHashAlgorithm() is duplicated here and in openssl_hash.cc.

=> same than for OpenSSL.

Please add doxygen comments explaining what the SecBuf? template
is for.

=> I added a comment which should be doxygen'ed.

Please add Doxygen comments to HMACImpl methods and fields.

=> new ticket?

src/lib/cryptolink/openssl_link.cc
initialize(): Can you catch std::exception instead of ...?

=> I have no idea of what exception can be raised so I prefer to keep a catch all.

Then you could print ex.what(). It's better than reporting general
InitializationError? without any clues what went wrong.

=> it is a nearly impossible condition so this won't help: it just makes the code more complex (and BTW impossible to test).

src/lib/cryptolink/tests/hash_unittests.cc
Please add brief Doxygen comments for all functions.

=> new ticket

doHashTestArray(): unsafe result allocation. You can keep its
pointer with boost::scoped_array. It will release the array
in a safe manner, even if there's exception.

=> done

src/lib/cryptolink/tests/hmac_unittests.cc
Please add brief Doxygen comments for all functions.

=> new ticket

Around line 255: There is special case for Botan, but there is none
for OpenSSL. If it's easy, can you add one for OpenSSL? If it's
tricky, feel free to add @todo in the code and create new ticket
for it. If no such case is needed for OpenSSL, please add a comment
that explains why.

=> IMHO the whole truncation stuff should be removed but I don't know if it is needed. BTW truncation makes a change from the crypto point of view for the verify() operation.

#ifndef WITH_BOTAN around line 541:
I would very much prefer to have the following construct:

#ifndef WITH_BOTAN
TEST(HMACTest, HMAC_SHA256_RFC2202_SIGN_TRUNCATED) {
#else
TEST(HMACTest, DISABLED_HMAC_SHA256_RFC2202_SIGN_TRUNCATED) {
#endif

This way, we will know that we are missing some tests for OpenSSL.

=> done (and BTW it is missing (ifndef) for Botan.

Also, if it is feasible to implement those tests for OpenSSL,
please add / @todo in the code.

Tests should use lowerCamelCase notation for their names.


General comment. Most of the implementation methods in
{botan,openssl}_{hmac,hash}.cc have the descriptions already
available in crypto_{hash,hmac}.h. Adding simple doxygen comment
with something like "see @ref Hash::update()" will do the trick.

=> I am creating a new ticket for Doxygen for cryptolink.

commits 1d04245..c0e5a18

comment:23 in reply to: ↑ 17 Changed 6 years ago by fdupont

Replying to tomek:

I'm afraid this ticket is branched from too old master to compile. I had to cherry-pick changes from trac3449 (they are on master already)
to just complete configure process. I pushed those commits to trac2406km.

=> I looked at the changes: they don't seem to badly interfere.

comment:24 in reply to: ↑ 19 Changed 6 years ago by fdupont

Replying to tomek:

I found one other issue: make distcheck failed:

make[5]: Entering directory `/home/thomson/devel/kea2/bind10-20140313/_build/src/bin/cmdctl'
make[5]: Nothing to be done for `check-am'.
make[5]: Leaving directory `/home/thomson/devel/kea2/bind10-20140313/_build/src/bin/cmdctl'
Making check in tests
make[5]: Entering directory `/home/thomson/devel/kea2/bind10-20140313/_build/src/bin/cmdctl/tests'
make  check-local
make[6]: Entering directory `/home/thomson/devel/kea2/bind10-20140313/_build/src/bin/cmdctl/tests'
for pytest in cmdctl_test.py botan-certgen_test.py  ; do \
	echo Running test: $pytest ; \
	 \
	PYTHONPATH=/home/thomson/devel/kea2/bind10-20140313/_build/src/lib/python/isc/log_messages:/home/thomson/devel/kea2/bind10-20140313/_build/src/lib/python/isc/cc:/home/thomson/devel/kea2/bind10-20140313/_build/../src/lib/python:/home/thomson/devel/kea2/bind10-20140313/_build/src/lib/python:/home/thomson/devel/kea2/bind10-20140313/_build/src/lib/dns/python/.libs:/home/thomson/devel/kea2/bind10-20140313/_build/src/bin/cmdctl \
	CMDCTL_SRC_PATH=/home/thomson/devel/kea2/bind10-20140313/_build/../src/bin/cmdctl \
	CMDCTL_BUILD_PATH=/home/thomson/devel/kea2/bind10-20140313/_build/src/bin/cmdctl \
	B10_LOCKFILE_DIR_FROM_BUILD=/home/thomson/devel/kea2/bind10-20140313/_build \
	/usr/bin/python3.3 /home/thomson/devel/kea2/bind10-20140313/_build/../src/bin/cmdctl/tests/$pytest || exit ; \
	done
Running test: cmdctl_test.py
.......................................
----------------------------------------------------------------------
Ran 39 tests in 0.015s

OK
Running test: botan-certgen_test.py
/usr/bin/python3.3: can't open file '/home/thomson/devel/kea2/bind10-20140313/_build/../src/bin/cmdctl/tests/botan-certgen_test.py': [Errno 2] No such file or directory
make[6]: *** [check-local] Error 2
make[6]: Leaving directory `/home/thomson/devel/kea2/bind10-20140313/_build/src/bin/cmdctl/tests'
make[5]: *** [check-am] Error 2
make[5]: Leaving directory `/home/thomson/devel/kea2/bind10-20140313/_build/src/bin/cmdctl/tests'
make[4]: *** [check-recursive] Error 1
make[4]: Leaving directory `/home/thomson/devel/kea2/bind10-20140313/_build/src/bin/cmdctl'
make[3]: *** [check-recursive] Error 1
make[3]: Leaving directory `/home/thomson/devel/kea2/bind10-20140313/_build/src/bin'
make[2]: *** [check-recursive] Error 1
make[2]: Leaving directory `/home/thomson/devel/kea2/bind10-20140313/_build/src'
make[1]: *** [check-recursive] Error 1
make[1]: Leaving directory `/home/thomson/devel/kea2/bind10-20140313/_build'
make: *** [distcheck] Error 1

I need more information to reproduce it (in particular the config arguments).

comment:25 follow-up: Changed 6 years ago by fdupont

How do we address OpenSSL vs OSX issue:

  • add a note?
  • fix it in the configure file (forcing a CRYPTO_CFLAGS)
  • fix it in openssl files with a pragma

As it is a known issue, we have to choose only before the release (i.e., it should not block the merge).

comment:26 in reply to: ↑ 19 ; follow-up: Changed 6 years ago by fdupont

Replying to tomek:

I found one other issue: make distcheck failed:

=> I have to finish analysis but it seems the Botan vs. OpenSSL choice is forgotten somewhere in distcheck, so I have to understand how distcheck is supposed to work...

comment:27 in reply to: ↑ 26 Changed 6 years ago by fdupont

Replying to fdupont:

Replying to tomek:

I found one other issue: make distcheck failed:

=> I have to finish analysis but it seems the Botan vs. OpenSSL choice is forgotten somewhere in distcheck, so I have to understand how distcheck is supposed to work...

Should be fixed now (commits c0e5a18..d7e7459).
BTW it was the only blocking item.

comment:28 Changed 6 years ago by tomek

  • Owner changed from fdupont to tomek

comment:29 in reply to: ↑ 22 ; follow-up: Changed 6 years ago by tomek

Replying to fdupont:

Replying to tomek:

src/lib/cryptolink/botan_hash.cc

getBotanHashAlgorithmName(): why are there breaks after return?
Some compilers will complain about unreachable code.

=> this was already in the old cryptolink code so if you believe it should be changed please open a new ticket.

That's just aesthetics, so it's not very productive to spend time on it.

Also, the last label:

    case isc::cryptolink::UNKNOWN_HASH:

Could be extended with default: and then there could be just
a single return ("Unknown"); statement.

=> same.

Fair enough.

Please add Doxygen comments to HashImpl? methods and fields.

=> same.

No. Since this is a new file being added, it has to come with Doxygen comments.
Come on, it's not a big deal. In most cases it's a matter of adding references
to Hash class.

I don't want to let Doxygen comments to slide to separate ticket, for several reasons:

1) We do not accept new code without comments. No exceptions. Sorry. See coding guidelines
here http://kea.isc.org/wiki/CodingGuidelines.
2) If you create new ticket, it will be a ticket about adding comments. There are tons
of other, more important tickets, so we'll likely never get to that ticket.
3) I don't want to make a precedent here. Otherwise other patches will come without comments, just with extra ticket that says they need comments. That's not acceptable.

So I'm sorry, but this code must be extended with properly formed Doxygen comments
before it is merged.

src/lib/cryptolink/openssl_hash.cc
getOpenSSLHashAlgorithm() see comment for getBotanHashAlgorithmName()
above.

=> same

This is easy to solve. You can move both functions to a separate .cc/.h files,
e.g. openssl_util.{cc|h} and botan_util.{cc|h}. This shouldn't be much work
and it eliminates code duplication that is easy to do.

Please add Doxygen comments to HashImpl? methods and fields.

=> new ticket again?

No, see above. I can't allow code that does not have comments to be merged.
I know that it's boring thing to do, but everyone has to do it.

src/lib/cryptolink/botan_hmac.cc
getBotanHashAlgorithmName() is duplicated here and in botan_hash.cc.

=> yes, in a better API there should be a relationship between Hash and HMAC so no duplicate.

See my proposed solution above.

verify(): please add / @todo someware around comments
regarding temporary change.

=> inherited... new ticket?

I just wanted to add / @todo in the text, so it will start showing up on
Doxygen TODO list. Done that. Please pull.

Please add Doxygen comments to HMACImpl methods and fields.

=> same

See my response above.

src/lib/cryptolink/openssl_hmac.cc
getOpenSSLHashAlgorithm() is duplicated here and in openssl_hash.cc.

=> same than for OpenSSL.

See my response above.

Please add doxygen comments explaining what the SecBuf? template
is for.

=> I added a comment which should be doxygen'ed.

One line comment is not sufficient. There should be a Doxygen comment for
the whole class and all its members and methods. Each method had to have
all of its parameters and return value documented.

If you're not used to writing comments, this is awkward at the beginning.
Personally, I think about Doxygen comments as a way to lecture a beginner
developer who I need to explain they code. With that mental image in mind,
I try to write the comments in explanatory way: explaining why I chose
specific solution and not the alternative, how to use this class/method,
what are the possible pitfalls etc.

Please add Doxygen comments to HMACImpl methods and fields.

=> new ticket?

No. See my response above.

src/lib/cryptolink/openssl_link.cc
initialize(): Can you catch std::exception instead of ...?

=> I have no idea of what exception can be raised so I prefer to keep a catch all.

Ok, but users won't know what breaks down. Even if they report the problem,
we won't be able to help them. Have you ever experienced a code that throws
exceptions different than derived from std::exception?

Then you could print ex.what(). It's better than reporting general
InitializationError? without any clues what went wrong.

=> it is a nearly impossible condition so this won't help: it just makes the code more complex (and BTW impossible to test).

I disagree. Just change ... to const std::exception& ex and then print out ex.what().
I would increase the code length by 1 line at most. If you are really concerned that some other type of exception is thrown that is not derived from std::exception, please put two catch clauses there. It's just couple lines of code, it doesn't add much complexity.

src/lib/cryptolink/tests/hash_unittests.cc
Please add brief Doxygen comments for all functions.

=> new ticket

No, see above. Every code merged to master must be commented.

doHashTestArray(): unsafe result allocation. You can keep its
pointer with boost::scoped_array. It will release the array
in a safe manner, even if there's exception.

=> done

Thanks.

src/lib/cryptolink/tests/hmac_unittests.cc
Please add brief Doxygen comments for all functions.

=> new ticket

See above.

Around line 255: There is special case for Botan, but there is none
for OpenSSL. If it's easy, can you add one for OpenSSL? If it's
tricky, feel free to add @todo in the code and create new ticket
for it. If no such case is needed for OpenSSL, please add a comment
that explains why.

=> IMHO the whole truncation stuff should be removed but I don't know if it is needed. BTW truncation makes a change from the crypto point of view for the verify() operation.

I do not know enough about security to make any suggestion here.
Perhaps adding @todo in the code that says that the truncation matter should be
investigated further is the best way forward here?

#ifndef WITH_BOTAN around line 541:
I would very much prefer to have the following construct:

#ifndef WITH_BOTAN
TEST(HMACTest, HMAC_SHA256_RFC2202_SIGN_TRUNCATED) {
#else
TEST(HMACTest, DISABLED_HMAC_SHA256_RFC2202_SIGN_TRUNCATED) {
#endif

This way, we will know that we are missing some tests for OpenSSL.

=> done (and BTW it is missing (ifndef) for Botan.

I had slightly different ifdef layout in mind. See commit 8196fd6d1cc7d244ef92586728ac9fae7f9b8cf1.

Tests should use lowerCamelCase notation for their names.

You haven't responded to this one. But after giving second thought, the test
names are just mostly acronyms, so it's ok to have them capitalized.


General comment. Most of the implementation methods in
{botan,openssl}_{hmac,hash}.cc have the descriptions already
available in crypto_{hash,hmac}.h. Adding simple doxygen comment
with something like "see @ref Hash::update()" will do the trick.

=> I am creating a new ticket for Doxygen for cryptolink.

That's ok for existing code. Any new code you have added must be
commented in this ticket.

I'll respond to the remaining comments in separate response.

comment:30 in reply to: ↑ 25 ; follow-up: Changed 6 years ago by tomek

  • Add Hours to Ticket changed from 2 to 4
  • Owner changed from tomek to fdupont
  • Total Hours changed from 18 to 22

Replying to fdupont:

How do we address OpenSSL vs OSX issue:

  • add a note?
  • fix it in the configure file (forcing a CRYPTO_CFLAGS)
  • fix it in openssl files with a pragma

As it is a known issue, we have to choose only before the release (i.e., it should not block the merge).

I am afraid not. We do have a build farm that builds on many platforms, including Mac. The automated builds will start failing as soon as we enable OpenSSL builds there. This has
to be solve before we merge this ticket.

I think we should add a check if this is Mac and OpenSSL is enabled. If the answer to both is yes, we should add -Wno-deprecated-declarations to AM_CPPFLAGS in src/lib/cryptolink/Makefile.am (or possibly to CRYPTO_INCLUDES).

Saying that something is a known issue does not make the issue any smaller.

comment:31 Changed 6 years ago by tomek

Regarding Mac OS detection, you may want to take a look at configure.ac:235.

comment:32 in reply to: ↑ 30 ; follow-up: Changed 6 years ago by fdupont

Replying to tomek:

Replying to fdupont:

How do we address OpenSSL vs OSX issue:

  • add a note?
  • fix it in the configure file (forcing a CRYPTO_CFLAGS)
  • fix it in openssl files with a pragma

As it is a known issue, we have to choose only before the release (i.e., it should not block the merge).

I am afraid not. We do have a build farm that builds on many platforms, including Mac. The automated builds will start failing as soon as we enable OpenSSL builds there. This has
to be solve before we merge this ticket.

I think we should add a check if this is Mac and OpenSSL is enabled. If the answer to both is yes, we should add -Wno-deprecated-declarations to AM_CPPFLAGS in src/lib/cryptolink/Makefile.am (or possibly to CRYPTO_INCLUDES).

=> it seems you want the second solution (aka CRYPTO_CFLAGS).

comment:33 in reply to: ↑ 32 Changed 6 years ago by tomek

Replying to fdupont:

=> it seems you want the second solution (aka CRYPTO_CFLAGS).

Yup.

comment:34 Changed 6 years ago by fdupont

Added some comments in cryptolink code (both inherited and new).
Diff 8fc6db1..c955d65
Added CRYPTO_CFLAGS for OSX deprecation of OpenSSL
Diff c955d65..50606ea

Ready for another review round!

comment:35 Changed 6 years ago by tomek

  • Owner changed from fdupont to tomek

comment:36 in reply to: ↑ 29 ; follow-ups: Changed 6 years ago by tomek

  • Add Hours to Ticket changed from 4 to 2
  • Owner changed from tomek to fdupont
  • Total Hours changed from 22 to 26

Ok, the list of issues to address is getting very short. That's a good progress.

I've added Doxygen comments for several methods. See commit 4c09ece85df47ee1f2c6233caa43d6d2ea2eb099. They are not complete, but
I think that's the reasonable compromise. It covers also several cosmetic
changes (Coding guidelines say that having if { code } in one line is not
allowed, also every if clause has to open a new scope, even if there's only
one instruction following. Please review them.

Replying to tomek:

Replying to fdupont:

Replying to tomek:

src/lib/cryptolink/openssl_hash.cc
getOpenSSLHashAlgorithm() see comment for getBotanHashAlgorithmName()
above.

=> same

This is easy to solve. You can move both functions to a separate .cc/.h files,
e.g. openssl_util.{cc|h} and botan_util.{cc|h}. This shouldn't be much work
and it eliminates code duplication that is easy to do.

This comment is still not addressed, but it is small enough, so you
can push it to separate ticket if you like.

src/lib/cryptolink/botan_hmac.cc
getBotanHashAlgorithmName() is duplicated here and in botan_hash.cc.

=> yes, in a better API there should be a relationship between Hash and HMAC so no duplicate.

See my proposed solution above.

That is true for this one as well.

src/lib/cryptolink/openssl_link.cc
initialize(): Can you catch std::exception instead of ...?

=> I have no idea of what exception can be raised so I prefer to keep a catch all.

Ok, but users won't know what breaks down. Even if they report the problem,
we won't be able to help them. Have you ever experienced a code that throws
exceptions different than derived from std::exception?

This one is not addressed either. The easiest way to address this is to
do the following:

        try {
            c.impl_ = new CryptoLinkImpl();
        } catch (const std::exception &ex) {
            isc_throw(InitializationError, "Error during OpenSSL initialization:" + ex.what());
        } catch (...) {
            // Should never happen
            isc_throw(InitializationError, "");
        }

src/lib/cryptolink/tests/hash_unittests.cc

Please document parameters for checkBuffer, doHashTestConv, doHashTestDirect,
doHashTestVector, doHashTestArray.

src/lib/cryptolink/tests/hmac_unittests.cc

Please document parameters for checkBuffer, doHMACTestConv, doHMACTestDirect,
doHMACTestVector, doHMACTestArray.

I confirm that the compilation fix for OS X is working. The tests failed (ConvVarTest?.destroyWhileWait), but that's unrelated issue.

This change requires ChangeLog? entry. Please write it in a way that makes
it clear that OpenSSL is a new alternative and Botan is still supported.
It's easy to give the impression that we're replacing Botan with OpenSSL.

Finally, a comment about Kea/bind10 review procedure. Once you finish a round
of review, please reassign the ticket back to the reviewer (to me in this particular
ticket). If I didn't notice your comment, the ticket would be ignored for a long time.

comment:37 in reply to: ↑ 36 Changed 6 years ago by fdupont

Replying to tomek:

Finally, a comment about Kea/bind10 review procedure. Once you finish a round
of review, please reassign the ticket back to the reviewer (to me in this particular
ticket). If I didn't notice your comment, the ticket would be ignored for a long time.

=> this didn't work for me with bind10 so I used to not try... I apologise.

comment:38 in reply to: ↑ 36 ; follow-up: Changed 6 years ago by fdupont

Replying to tomek:

I've added Doxygen comments for several methods.

=> got and reviewed/fixed them.

It covers also several cosmetic
changes (Coding guidelines say that having if { code } in one line is not
allowed, also every if clause has to open a new scope, even if there's only
one instruction following.

=> I have to say I prefer multiple lines too. BTW is there a policy for
empty destructors?

This is easy to solve. You can move both functions to a separate .cc/.h files,
e.g. openssl_util.{cc|h} and botan_util.{cc|h}. This shouldn't be much work
and it eliminates code duplication that is easy to do.

This comment is still not addressed, but it is small enough, so you
can push it to separate ticket if you like.

=> as I explained the problem is deeper: the current API doesn't allow a better relationship between hash and hmac. I can open a generic ticket for brainstorming about a better cryptolink API.

src/lib/cryptolink/openssl_link.cc
initialize(): Can you catch std::exception instead of ...?

=> I adopted and fixed your proposal. Unfortunately there is no way in C++ to ask for object with a what() method...

src/lib/cryptolink/tests/hash_unittests.cc

Please document parameters for checkBuffer, doHashTestConv, doHashTestDirect,
doHashTestVector, doHashTestArray.

src/lib/cryptolink/tests/hmac_unittests.cc

Please document parameters for checkBuffer, doHMACTestConv, doHMACTestDirect,
doHMACTestVector, doHMACTestArray.

=> is it possible to add a @ref pointing to the following doHXXXTest() as they all have the same parameters? (done using this)

I confirm that the compilation fix for OS X is working. The tests failed (ConvVarTest?.destroyWhileWait), but that's unrelated issue.

=> if you have a simple fix for the unrelated test failure I can merge it (there are already merged fixed in configure.ac)

This change requires ChangeLog? entry. Please write it in a way that makes
it clear that OpenSSL is a new alternative and Botan is still supported.
It's easy to give the impression that we're replacing Botan with OpenSSL.

=> I tried but it will need some work before and during integration...

git diff 4c09ece..9aec061

comment:39 Changed 6 years ago by fdupont

  • Owner changed from fdupont to tomek

comment:40 in reply to: ↑ 38 ; follow-up: Changed 6 years ago by tomek

  • Owner changed from tomek to fdupont
  • Total Hours changed from 26 to 28

Replying to fdupont:

Replying to tomek:

I've added Doxygen comments for several methods.

=> got and reviewed/fixed them.

Thanks.

It covers also several cosmetic
changes (Coding guidelines say that having if { code } in one line is not
allowed, also every if clause has to open a new scope, even if there's only
one instruction following.

=> I have to say I prefer multiple lines too. BTW is there a policy for
empty destructors?

No. Just the generic C++ recommendation applies. If you have a virtual class,
add a virtual destructor to ensure that all derived classes also have virtual
destructors. On a related note, we'll need to update our CodingGuidelines.
They have some useless cruft and are missing some useful things.

This is easy to solve. You can move both functions to a separate .cc/.h files,
e.g. openssl_util.{cc|h} and botan_util.{cc|h}. This shouldn't be much work
and it eliminates code duplication that is easy to do.

This comment is still not addressed, but it is small enough, so you
can push it to separate ticket if you like.

=> as I explained the problem is deeper: the current API doesn't allow a better relationship between hash and hmac. I can open a generic ticket for brainstorming about a better cryptolink API.

That may be true, but no large API change is required. Anyway, I've created
ticket #3471 for that.

src/lib/cryptolink/openssl_link.cc
initialize(): Can you catch std::exception instead of ...?

=> I adopted and fixed your proposal. Unfortunately there is no way in C++ to ask for object with a what() method...

Your changes are fine.


src/lib/cryptolink/tests/hash_unittests.cc

Please document parameters for checkBuffer, doHashTestConv, doHashTestDirect,
doHashTestVector, doHashTestArray.

src/lib/cryptolink/tests/hmac_unittests.cc

Please document parameters for checkBuffer, doHMACTestConv, doHMACTestDirect,
doHMACTestVector, doHMACTestArray.

=> is it possible to add a @ref pointing to the following doHXXXTest() as they all have the same parameters? (done using this)

Sure. That does the trick. Actually, I checked that there's a couple of new Doxygen
warnings. If you do:

make -C doc devel

and then look at doc/html/doxygen-error.log, you'll notice a couple of new errors
in cryptolink lib. If you decide not to fix them, please add a note to one of
existing Doxygen tickets (e.g. #3257) that is should be dealt with later.

I confirm that the compilation fix for OS X is working. The tests failed (ConvVarTest?.destroyWhileWait), but that's unrelated issue.

=> if you have a simple fix for the unrelated test failure I can merge it (there are already merged fixed in configure.ac)

I think it's fixed on master a long time ago.

This change requires ChangeLog? entry. Please write it in a way that makes
it clear that OpenSSL is a new alternative and Botan is still supported.
It's easy to give the impression that we're replacing Botan with OpenSSL.

=> I tried but it will need some work before and during integration...

It looks fine.

Ok, that addresses all my issues. The changes are ready to go. Before you merge it,
can you ask Jeremy or Wlodek to run it on the build farm? I tested it with both
Botan and OpenSSL on Ubuntu 13.10 x64 and Mac OS 10.9.3. Let's hope the merge
won't be too painful. There shouldn't be any other changes in crypto that could
cause a conflict, but I'm sure a thing or two have changed in configure.ac...

Phew. That was a long ticket, but it's a very useful addition, so it was well
worth the effort. Thanks a lot for doing this work.

Last edited 6 years ago by tomek (previous) (diff)

comment:41 in reply to: ↑ 40 Changed 6 years ago by fdupont

Replying to tomek:

Sure. That does the trick. Actually, I checked that there's a couple of new Doxygen
warnings. If you do:

make -C doc devel

and then look at doc/html/doxygen-error.log, you'll notice a couple of new errors
in cryptolink lib. If you decide not to fix them, please add a note to one of
existing Doxygen tickets (e.g. #3257) that is should be dealt with later.

=> I am installing oxygen on my OS X to look at them. If the fix is easy I'll add it.

=> if you have a simple fix for the unrelated test failure I can merge it (there are already merged fixed in configure.ac)

I think it's fixed on master a long time ago.

=> it is so I'll try to get and merge it.

Ok, that addresses all my issues. The changes are ready to go. Before you merge it,
can you ask Jeremy or Wlodek to run it on the build farm?

=> I believe this means I have to fix the unrelated test issue first. Jemery or Wlodek, can you comment?

BTW I'll need to know what hash to put in the ChangeLog? (I expect it is the merge commit).

comment:42 Changed 6 years ago by fdupont

Fixed oxygen errors (git diff 9aec061..fc821c1).

comment:43 Changed 6 years ago by fdupont

Fixed the "make check" failure. BTW the configure.ac is IMHO far too fragile about this.
git commit fc821c1..b24b6c9

With this fix the trac2406km should be ready for the build farm. Note it introduces a new variant for the crypto backend:

  • as current (should default to Botan)
  • add --with-openssl in configure

comment:44 Changed 6 years ago by tomek

I reviewed your changes in fc821c1..b24b6c9. They are fine.

comment:45 Changed 6 years ago by fdupont

Committed to master. Closing.

comment:46 Changed 6 years ago by fdupont

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.