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 support for OpenSSL 1.1.1's ed25519 and ed448 for signing and verifying #6910

Merged
merged 10 commits into from Oct 4, 2018

Conversation

Projects
None yet
3 participants
@pieterlexis
Copy link
Member

pieterlexis commented Aug 31, 2018

Short description

This implements algorithm 15 and 16 support in the OpenSSL signer. It will only be used if the (as of yet unreleased) OpenSSL 1.1.1 is used.

Please review carefully.

CC: @mind04

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

pieterlexis added some commits Apr 10, 2018

@pieterlexis pieterlexis added this to the auth-4.2.0 milestone Aug 31, 2018

@pieterlexis pieterlexis requested a review from rgacogne Aug 31, 2018


if(d_algorithm == 15) {
d_len = 32;
d_id = NID_ED25519;

This comment has been minimized.

@rgacogne

rgacogne Aug 31, 2018

Member

Shouldn't we only use NID_ED25519 if HAVE_LIBCRYPTO_ED25519 is set?

This comment has been minimized.

@pieterlexis

pieterlexis Sep 3, 2018

Member

I think HAVE_LIBCRYPTO_ED25519 and HAVE_LIBCRYPTO_ED448 can go, ed25519 and ed448 support were added in the same commit in OpenSSL

This comment has been minimized.

@rgacogne

rgacogne Sep 3, 2018

Member

Is it possible to compile with only one of them available?

This comment has been minimized.

@rgacogne

rgacogne Sep 3, 2018

Member

(openssl, I mean)

This comment has been minimized.

@pieterlexis

pieterlexis Sep 3, 2018

Member

It does not looks like it from the perl configdata.pm --dump

d_id = NID_ED25519;
} else if (d_algorithm == 16) {
d_len = 57;
d_id = NID_ED448;

This comment has been minimized.

@rgacogne

rgacogne Aug 31, 2018

Member

Same thing with NID_ED448 and HAVE_LIBCRYPTO_ED448.

Show resolved Hide resolved pdns/opensslsigners.cc Outdated

d_edctx = EVP_PKEY_CTX_new_id(d_id, NULL);
if (d_edctx == NULL) {
throw runtime_error(getName()+" unable to allocate context");

This comment has been minimized.

@rgacogne

rgacogne Aug 31, 2018

Member

Leaking d_edkey as well.

Show resolved Hide resolved pdns/opensslsigners.cc Outdated
bool OpenSSLEDDSADNSCryptoKeyEngine::verify(const std::string& msg, const std::string& signature) const
{
auto ctx = EVP_MD_CTX_new();
auto pctx = d_edctx;

This comment has been minimized.

@rgacogne

rgacogne Aug 31, 2018

Member

I don't think we need pctx, it seems that it's only written to by EVP_DigestVerifyInit() and we don't use it afterwards.


void OpenSSLEDDSADNSCryptoKeyEngine::fromPublicKeyString(const std::string& content)
{
const unsigned char* raw = (const unsigned char*)content.c_str();

This comment has been minimized.

@rgacogne

rgacogne Aug 31, 2018

Member

reinterpret_cast<const unsigned char*>(content.c_str()); ?

const size_t inputLen = content.length();

if (inputLen < 1) {
throw runtime_error(getName()+" invalid input size for the public key");

This comment has been minimized.

@rgacogne

rgacogne Aug 31, 2018

Member

Unsure what the point of this check is since we check it again below?

int type{0};

if (inputLen == 32) {
type = NID_ED25519;

This comment has been minimized.

@rgacogne

rgacogne Aug 31, 2018

Member

Same remark regarding the HAVE_ than above.

throw runtime_error(getName()+" unknown algorithm "+std::to_string(d_algorithm));
}

d_edctx = EVP_PKEY_CTX_new_id(d_id, NULL);

This comment has been minimized.

@rgacogne

rgacogne Aug 31, 2018

Member

From what I see, it doesn't look like we need a EVP_PKEY_CTX object except for generating a new key so perhaps we could just deal with it in ::create() and be done with it afterwards?

[AC_INCLUDES_DEFAULT
#include <$ssldir/include/openssl/evp.h>])

AS_IF([test "$libcrypto_ed448" = "yes" -o "$libcrypto_ed448" = "yes"], [

This comment has been minimized.

@mnordhoff

mnordhoff Sep 1, 2018

Contributor

That checks ed448 twice. Should one of them be ed25519?

pieterlexis added some commits Sep 3, 2018

Stop leaking EVP_PKEY_CTX's
* remove d_edctx and create a context when needed.
* Use nullptr instead of NULL

void OpenSSLEDDSADNSCryptoKeyEngine::create(unsigned int bits)
{
auto pctx = EVP_PKEY_CTX_new(d_edkey, nullptr);

This comment has been minimized.

@rgacogne

rgacogne Sep 4, 2018

Member

I think we should generate the context from the algorithm id:

auto pctx = EVP_PKEY_CTX_new_id(d_id, nullptr);

Otherwise I don't see how the correct type of key can be generated.

throw runtime_error(getName()+" context initialization failed");
}
if (EVP_PKEY_keygen_init(pctx) < 1) {
throw runtime_error(getName()+" keygen initialization failed");

This comment has been minimized.

@rgacogne

rgacogne Sep 4, 2018

Member

I think we are leaking pctx here.

throw runtime_error(getName()+" keygen initialization failed");
}
if (EVP_PKEY_keygen(pctx, &d_edkey) < 1) {
throw runtime_error(getName()+" key generation failed");

This comment has been minimized.

@rgacogne

rgacogne Sep 4, 2018

Member

Also here.

}
if (EVP_PKEY_keygen(pctx, &d_edkey) < 1) {
throw runtime_error(getName()+" key generation failed");
}

This comment has been minimized.

@rgacogne

rgacogne Sep 4, 2018

Member

We need to release pctx here.


std::string OpenSSLEDDSADNSCryptoKeyEngine::sign(const std::string& msg) const
{
auto pctx = EVP_PKEY_CTX_new(d_edkey, nullptr);

This comment has been minimized.

@rgacogne

rgacogne Sep 4, 2018

Member

I don't think we need pctx at all here, EVP_DigestSignInit() will accept a nullptr instead.


bool OpenSSLEDDSADNSCryptoKeyEngine::verify(const std::string& msg, const std::string& signature) const
{
auto pctx = EVP_PKEY_CTX_new(d_edkey, nullptr);

This comment has been minimized.

@rgacogne

rgacogne Sep 4, 2018

Member

Same here, I don't think pctx is needed.

pieterlexis added some commits Sep 5, 2018

@pieterlexis

This comment has been minimized.

Copy link
Member

pieterlexis commented Sep 28, 2018

Could use a new review

@pieterlexis pieterlexis merged commit 76849a0 into PowerDNS:master Oct 4, 2018

4 checks passed

LGTM analysis: C/C++ No alert changes
Details
LGTM analysis: JavaScript No alert changes
Details
LGTM analysis: Python No alert changes
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pieterlexis pieterlexis deleted the pieterlexis:openssl-eddsa branch Oct 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment