Conversation
tertsdiepraam
force-pushed
the
key2ds
branch
from
4fd47ff to
c0d8ffa
Compare
src/commands/key2ds.rs
Outdated
| | SecAlg::ECDSAP256SHA256 => DigestAlg::SHA256, | ||
| SecAlg::ECDSAP384SHA384 => DigestAlg::SHA384, | ||
| SecAlg::ECC_GOST => DigestAlg::GOST, | ||
| _ => DigestAlg::SHA1, |
There was a problem hiding this comment.
Is using SHA-1 as the fallback a good idea? If new algorithms were to get introduced and we forget to update the above list, they would fallback to using SHA-1, right?
There was a problem hiding this comment.
It's the current ldns behaviour: https://github.com/NLnetLabs/ldns/blob/b87b8e1d212b43f225891570ebc5045c6331dd8a/examples/ldns-key2ds.c#L48C1-L48C14
If we want to change it, let's take it to a separate PR, where we can discuss this in depth.
src/commands/key2ds.rs
Outdated
| let sec_alg = sec_alg.to_int(); | ||
| let filename = | ||
| format!("K{owner}+{sec_alg:03}+{key_tag:05}.ds"); | ||
| let mut out_file = File::create(&filename).map_err(|e| { |
There was a problem hiding this comment.
Would it make sense to use File::create_new() here?
There was a problem hiding this comment.
Possibly, but ldns-key2ds doesn't care I think.
There was a problem hiding this comment.
That is, if we have consensus that we want to break compat here, I'd be happy to change it.
There was a problem hiding this comment.
Now that we have different argument parsing for ldns and dnst, and we can use different defaults, we can probably also introduce differences here. Maybe a simple running_as_ldns: bool (clap-hidden) member is enough or should it be an enum like MultiCallMode::{LDNS,DNST}/CompatMode::{None/DNST,LDNS} (or similar)?
|
Little update: I changed the arguments for the dnst version so that we can use |
mozzieongit
left a comment
mozzieongit
left a comment
There was a problem hiding this comment.
Apart from the minor comments, it looks good to me. Also, very thorough testing.
tertsdiepraam
force-pushed
the
key2ds
branch
from
6f570da to
0d8cc5d
Compare
4 participants
No description provided.