starred/hickory-dns

Fork 0

mirror of https://github.com/hickory-dns/hickory-dns.git synced 2026-04-25 19:25:56 +03:00

[GH-ISSUE #127] Combine `KeyPair::generate()` and `KeyPair::encode_key()` into one operation #61

New issue

Closed

opened 2026-03-07 22:18:28 +03:00 by kerem · 6 comments

kerem commented

2026-03-07 22:18:28 +03:00

Owner

Originally created by @briansmith on GitHub (May 8, 2017).
Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/127

Originally assigned to: @bluejekyll on GitHub.

In the code there is this:

        let key = try!(KeyPair::generate(algorithm)
            .map_err(|e| format!("could not generate key: {}", e)));
        let key_bytes: Vec<u8> =
            try!(format
                     .encode_key(&key, key_config.password())
                     .map_err(|e| format!("could not get key bytes: {}", e)));

I was looking at the code to see how to update it for the new version of ring. ring is designed intentionally to make an encode_key() operation impossible without subverting memory safety. The idea is that once you have a private key object, e.g. Ed25519KeyPair, you can use the key, but you can't extract the private key from it. In the current version of Trust-DNS this is worked around by using ``Ed25519KeyPairBytes`, which was intended to help with serialization. But that's been replaced in ring by its PKCS#8 support. And that wouldn't work with the upcoming ECDSA support in ring either, nor with its RSA support.

Instead, ring supports a generate_pkcs8() function that returns the encoded bytes: i.e. it combines generation and serialization together. I think this is what Trust-DNS's KeyPair should do too, because other than generating a key, there's no reason (AFAICT) to need to encode a private key in Trust-DNS.

WDYT?

Originally created by @briansmith on GitHub (May 8, 2017). Original GitHub issue: https://github.com/hickory-dns/hickory-dns/issues/127 Originally assigned to: @bluejekyll on GitHub. In the code there is this: ```rust let key = try!(KeyPair::generate(algorithm) .map_err(|e| format!("could not generate key: {}", e))); let key_bytes: Vec<u8> = try!(format .encode_key(&key, key_config.password()) .map_err(|e| format!("could not get key bytes: {}", e))); ``` I was looking at the code to see how to update it for the new version of *ring*. *ring* is designed intentionally to make an `encode_key()` operation impossible without subverting memory safety. The idea is that once you have a private key object, e.g. `Ed25519KeyPair`, you can *use* the key, but you can't extract the private key from it. In the current version of Trust-DNS this is worked around by using ``Ed25519KeyPairBytes`, which was intended to help with serialization. But that's been replaced in *ring* by its PKCS#8 support. And that wouldn't work with the upcoming ECDSA support in *ring* either, nor with its RSA support. Instead, ring supports a `generate_pkcs8()` function that returns the encoded bytes: i.e. it combines generation and serialization together. I think this is what Trust-DNS's `KeyPair` should do too, because other than generating a key, there's no reason (AFAICT) to need to encode a private key in Trust-DNS. WDYT?

kerem closed this issue

2026-03-07 22:18:28 +03:00

kerem commented

2026-03-07 22:18:29 +03:00

Author

Owner

@bluejekyll commented on GitHub (May 9, 2017):

That sounds like a great plan. I'd love to have a better story around this. I really like the idea of moving to a common pkcs8 storage.

@bluejekyll commented on GitHub (May 9, 2017): That sounds like a great plan. I'd love to have a better story around this. I really like the idea of moving to a common pkcs8 storage.

kerem commented

2026-03-07 22:18:29 +03:00

Author

Owner

@bluejekyll commented on GitHub (May 11, 2017):

I'll have a patch for this as soon as possible. Did I understand that you want to pull previous versions, is this accurate?

@bluejekyll commented on GitHub (May 11, 2017): I'll have a patch for this as soon as possible. Did I understand that you want to pull previous versions, is this accurate?

kerem commented

2026-03-07 22:18:29 +03:00

Author

Owner

@briansmith commented on GitHub (May 11, 2017):

Yes, that's true, but IIUC your cargo.lock should make it so you're not affected by that if/when I do it. Also I know it's an inconvenience so I won't do it this week.

@briansmith commented on GitHub (May 11, 2017): Yes, that's true, but IIUC your cargo.lock should make it so you're not affected by that if/when I do it. Also I know it's an inconvenience so I won't do it this week.

kerem commented

2026-03-07 22:18:29 +03:00

Author

Owner

@bluejekyll commented on GitHub (May 11, 2017):

It's not a huge inconvenience, I think these are minor changes. But I believe if you yank previous version off crates.io, it will break any software that builds against those versions. Cargo.lock only binds the build to a particular version as I understand it, meaning it still needs to be available in crates.io for any builds at that version. I don't know of anyone depending on ring with TRust-DNS, and it's currently not a default dependency so I'm not worried about it.

But, for other users of ring and their downstream libraries, won't it be a cascading effect on every downstream dependency?

@bluejekyll commented on GitHub (May 11, 2017): It's not a huge inconvenience, I think these are minor changes. But I believe if you yank previous version off crates.io, it will break any software that builds against those versions. Cargo.lock only binds the build to a particular version as I understand it, meaning it still needs to be available in crates.io for any builds at that version. I don't know of anyone depending on *ring* with TRust-DNS, and it's currently not a default dependency so I'm not worried about it. But, for other users of *ring* and their downstream libraries, won't it be a cascading effect on every downstream dependency?

kerem commented

2026-03-07 22:18:29 +03:00

Author

Owner

@briansmith commented on GitHub (May 11, 2017):

I don't know of anyone depending on ring with TRust-DNS, and it's currently not a default dependency so I'm not worried about it.

OK, that's good. I think moving the Ed25519 stuff to PKCS#8 is better done as a clean break without ugly migration logic anyway.

But, for other users of ring and their downstream libraries, won't it be a cascading effect on every downstream dependency?

The important thing here is how the change affects TRust-DNS.

@briansmith commented on GitHub (May 11, 2017): > I don't know of anyone depending on ring with TRust-DNS, and it's currently not a default dependency so I'm not worried about it. OK, that's good. I think moving the Ed25519 stuff to PKCS#8 is better done as a clean break without ugly migration logic anyway. > But, for other users of ring and their downstream libraries, won't it be a cascading effect on every downstream dependency? The important thing here is how the change affects TRust-DNS.

kerem commented

2026-03-07 22:18:29 +03:00

Author

Owner

@bluejekyll commented on GitHub (May 17, 2017):

fixed in #133

@bluejekyll commented on GitHub (May 17, 2017): fixed in #133

kerem referenced this issue

2026-03-15 22:03:15 +03:00

[GH-ISSUE #61] Switch out direct usage of MIO for futures.rs (server) #338