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

resolve: add support for RFC 8080 #7600

Merged
merged 1 commit into from Dec 12, 2017

Conversation

4 participants
@ott
Contributor

ott commented Dec 10, 2017

RFC 8080 describes how to use EdDSA keys and signatures in DNSSEC. It
uses the curves Ed25519 and Ed448. Libgcrypt 1.8.1 does not support
Ed448, so only the Ed25519 is supported at the moment. Once Libgcrypt
supports Ed448, support for it can be trivially added to resolve.

} else
fclose(f);
switch (rrsig->rrsig.algorithm) {

This comment has been minimized.

@ott

ott Dec 10, 2017

Contributor

The control flow within dnssec_verify_rrset is somewhat complicated. I would appreciate if someone verify that it's correct.

@yuwata yuwata added the resolve label Dec 10, 2017

@poettering

looks really good. Just some minor fixups

return -EIO;
f = open_memstream(&sig_data, &sig_size);
if (!f)
return -ENOMEM;

This comment has been minimized.

@poettering

poettering Dec 11, 2017

Member

hmm, given that this is somewhat performance-sensitive I figure it would be low-hanging but worthy to call __fsetlocking(f, FSETLOCKING_BYCALLER) on the memstream, to turn off all locking on it. For just concatenating a few bytes we really shouldn't pay the hefty locking price stdio makes us pay normally

if (ferror(f)) {
fclose(f);
free(sig_data);
return -ENOMEM;

This comment has been minimized.

@poettering

poettering Dec 11, 2017

Member

please use fflush_and_check() like everywhere else for this

@@ -613,6 +707,9 @@ int dnssec_verify_rrset(
gcry_md_hd_t md = NULL;
int r, md_algorithm;
size_t k, n = 0;
FILE *f = NULL;
size_t sig_size = 0;

This comment has been minimized.

@poettering

poettering Dec 11, 2017

Member

Please use cleanup_free and cleanup_fclose on these and avoid manual freeing/closing

@@ -445,9 +530,18 @@ static void md_add_uint16(gcry_md_hd_t md, uint16_t v) {
gcry_md_write(md, &v, sizeof(v));
}
static void md_add_uint32(gcry_md_hd_t md, uint32_t v) {
static size_t fwrite_uint8(uint8_t v, FILE *fp) {

This comment has been minimized.

@poettering

poettering Dec 11, 2017

Member

could you swap the two args please? libc is weird that fputc() take the argument first, and the object to operate on second, but we really shouldn't replicate that here...

return fwrite(&v, sizeof(v), 1, fp);
}
static size_t fwrite_uint16(uint16_t v, FILE *fp) {

This comment has been minimized.

@poettering

poettering Dec 11, 2017

Member

as above

return fwrite(&v, sizeof(v), 1, fp);
}
static size_t fwrite_uint32(uint32_t v, FILE *fp) {

This comment has been minimized.

@poettering

poettering Dec 11, 2017

Member

as above

@@ -57,6 +57,8 @@ enum {
DNSSEC_ALGORITHM_ECC_GOST = 12, /* RFC 5933 */
DNSSEC_ALGORITHM_ECDSAP256SHA256 = 13, /* RFC 6605 */
DNSSEC_ALGORITHM_ECDSAP384SHA384 = 14, /* RFC 6605 */
DNSSEC_ALGORITHM_ED25519 = 15,

This comment has been minimized.

@poettering

poettering Dec 11, 2017

Member

please add the RFC number as comment for this too!

@ott

This comment has been minimized.

Contributor

ott commented Dec 11, 2017

I did incorporate the comments.

@@ -613,6 +711,9 @@ int dnssec_verify_rrset(
gcry_md_hd_t md = NULL;
int r, md_algorithm;
size_t k, n = 0;
size_t sig_size = 0;
_cleanup_free_ char *sig_data = NULL;

This comment has been minimized.

@ott

ott Dec 11, 2017

Contributor

free has to be called after fclose. The GCC documentation for the cleanup attribute does not state in which order the functions are called. I only found a Google+ post by Lennart Poettering that claims that they are called “in order”, presumably in the reverse order of declaration. I would like to avoid that we depend on undefined behaviour as the wrong order caused a memory corruption when I tested it.

This comment has been minimized.

@poettering

poettering Dec 11, 2017

Member

well, we do that all over the place already by ordering the declarations properly, so by doing this you are not going to break anything new...

Moreover the gcc C cleanup stuff is pretty much identical to the C++ destructor logic, and exposes the same semantics. And for C++ the standard is very explicit about the destruction order, see https://stackoverflow.com/a/2254306

@@ -445,9 +534,18 @@ static void md_add_uint16(gcry_md_hd_t md, uint16_t v) {
gcry_md_write(md, &v, sizeof(v));
}
static void md_add_uint32(gcry_md_hd_t md, uint32_t v) {
static size_t fwrite_uint8(FILE *fp, uint8_t v) {
return fwrite(&v, sizeof(v), 1, fp);

This comment has been minimized.

@poettering

poettering Dec 11, 2017

Member

hmm, thinking about this, these function prototypes are actually weird since they are declared size_t, but will only return 0 or 1... (that's because fwrite() returns the "nmemb" paramater on success, not the first. It's kinda strange to return a "size_t" if we never actually return anything non-binary there... dunno.

Given that we don't care about the return value, and these functions are static anyway, I am pretty sure it would make more senseto just return void here...

hash = gcry_md_read(md, 0);
if (!hash) {
r = -EIO;
if (fflush_and_check(f))

This comment has been minimized.

@poettering

poettering Dec 11, 2017

Member
r = fflush_and_check(f);
if (r < 0)
        return r;
@ott

This comment has been minimized.

Contributor

ott commented Dec 11, 2017

I incorporated the comments.

r = -EIO;
r = fflush_and_check(f);
if (r < 0)
return -ENOMEM;

This comment has been minimized.

@poettering

poettering Dec 11, 2017

Member

why not return r?

@poettering

This comment has been minimized.

Member

poettering commented Dec 11, 2017

Patch looks good to me, but the semaphore CI does not: does that work correctly locally for you?

(maybe semaphore's gcrypt is too old for this. Might be worth skipping the test if gcrypt doesn't know the EC algs yet)

DS: example.com. IN DS 3613 15 2 dda6b969bdfb79f71ee7b7fbdfb7dcd7adbbd35ddf79ed3b6dd7f6e356ddd747f76f5f7ae1a6f9e5cefc7bbf5adf4e1b
MX: example.com. IN MX 10 mail.example.com.
RRSIG: example.com. IN RRSIG MX ED25519 2 3600 20150819220000 20150729220000 3613 example.com. 
        oL9krJun7xfBOIWcGHi7mag5/hdZrKWw15jPGrHpjQeRAvTdszaPD+QLs3fx8A4M3e23mRZ
        9VrbpMngwcrqNAg==
Assertion 'dnssec_verify_rrset(answer, mx->key, rrsig, dnskey, rrsig->rrsig.inception * USEC_PER_SEC, &result) >= 0' failed at ../src/resolve/test-dnssec.c:211, function test_dnssec_verify_rfc8080_ed25519_example1(). Aborting.
-------

Full log written to /home/runner/systemd/build/meson-logs/testlog.txt
FAILED: meson-test 
/usr/bin/python3 -u /home/runner/.local/bin/meson test --no-rebuild --print-errorlogs
ninja: build stopped: subcommand failed.
@ott

This comment has been minimized.

Contributor

ott commented Dec 11, 2017

I successfully built systemd on Debian Stretch. It includes Libgcrypt 1.7.6 and test-dnssec runs without problems. I think Semaphore CI uses 1.7.8. Can you run libgcrypt-config --version and libgcrypt-config --algorithms as part of the build on Semaphore CI?

@poettering

This comment has been minimized.

Member

poettering commented Dec 11, 2017

hmm, best would be to just add some versioning printing code to the test case, and then push it to see what version it is. That's how I'd do this myself at least...

@ott

This comment has been minimized.

Contributor

ott commented Dec 11, 2017

Semaphore CI uses Libgcrypt 1.5.3. Ed25519 support was introduced 1.5.4. If you install the libgcrypt20-dev package, version 1.7.6 should be used on Ubuntu 17.04.

@poettering

This comment has been minimized.

Member

poettering commented Dec 11, 2017

@martinpitt any chance you can update the libgrcypt package for the semaphore CI?

@ott I think it would make sense to change the test case for now to simply skip testing this bit if gcrypt doesn't support the algorithm. That way, we can merge your patch now, and as soon as @martinpitt does the CI update we'll test it on semaphore too. Note that semaphore isn't out only CI, we also test on current development ubuntu (which should hvae the new gcrypt) , hence it's entirely OK if one CI only tests a subset of what there is to test..

if you change the test as suggested, please change it so that the availability of the algorithm is tested itself, rather than a version check, to cover for backporting distros...

@poettering

This comment has been minimized.

Member

poettering commented Dec 11, 2017

hmm, i see you just updated the patch already. test version check doesn't look too bad, hence i figure it's ok to leave it as is.

resolve: add support for RFC 8080
RFC 8080 describes how to use EdDSA keys and signatures in DNSSEC. It
uses the curves Ed25519 and Ed448. Libgcrypt 1.8.1 does not support
Ed448, so only the Ed25519 is supported at the moment. Once Libgcrypt
supports Ed448, support for it can be trivially added to resolve.
@ott

This comment has been minimized.

Contributor

ott commented Dec 11, 2017

I included a check that checks that Libgcrypt is at least version 1.6.0 before it enables EdDSA support.

@martinpitt

This comment has been minimized.

Contributor

martinpitt commented Dec 12, 2017

Semaphore CI uses Libgcrypt 1.5.3. Ed25519 support was introduced 1.5.4.

No, semaphore uses version 1.6.1 (in Ubuntu 14.04), so in theory that should satisfy the check #if GCRYPT_VERSION_NUMBER >= 0x010600 that you added here. If 1.6.1 is not enough, can you please bump the version in that check? I can then backport a newer one.

@ott

This comment has been minimized.

Contributor

ott commented Dec 12, 2017

You can see in build 6 of Semaphore CI of this pull request that it uses Libgcrypt 1.5.6.

I successfully built and tested systemd with Libgcrypt 1.6.0. The configuration was as follows:

version:1.6.0:
ciphers:arcfour🐡cast5:des:aes:twofish:serpent:rfc2268:seed:camellia:idea:salsa20:gost28147:
pubkeys:dsa:elgamal:rsa:ecc:
digests:crc:gostr3411-94:md4:md5:rmd160:sha1:sha256:sha512🐯whirlpool:stribog:
rnd-mod:linux:
cpu-arch:x86:
mpi-asm:amd64/mpih-add1.S:amd64/mpih-sub1.S:amd64/mpih-mul1.S:amd64/mpih-mul2.S:amd64/mpih-mul3.S:amd64/mpih-lshift.S:amd64/mpih-rshift.S:
threads:undefined:
hwflist:
fips-mode:y:n:
rng-type:fips:2:

The build on Semaphore CI is also successful now. test-dnssec also seems to have passed on artful-s390x, xenial-amd64 and artful-i386. I'm unsure what the problem with Fedora Rawhide CI x86_64 is.

@poettering

This comment has been minimized.

Member

poettering commented Dec 12, 2017

CI failures are unrelated (KVM and dpkg locking issues)

@poettering

This comment has been minimized.

Member

poettering commented Dec 12, 2017

looks good. merging

@poettering poettering merged commit cb9eeb0 into systemd:master Dec 12, 2017

2 of 6 checks passed

Fedora Rawhide CI x86_64 rpm build [internal error]
Details
artful-s390x autopkgtest finished (failure)
Details
xenial-amd64 autopkgtest finished (failure)
Details
default Build triggered. sha1 is merged.
Details
artful-i386 autopkgtest finished (success)
Details
semaphoreci The build passed on Semaphore.
Details
@ott

This comment has been minimized.

Contributor

ott commented Dec 12, 2017

It is possible that Libgcrypt was compiled without EdDSA support. But it can be also compiled without RSA, ECDSA or SHA support and we don't check for that. We could test that via gcry_pk_test_algo and so on. I can make this a separate pull request though.

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