[RFC] re-structure named.toml syntax to reject invalid configurations

[japaric]

According to this comment

enable_dnssec - enable signing of the zone for DNSSEC

The enable_dnssec setting is used to enable (runtime) signing of DNS records.

Currently, this setting sits at the zones level, see below

[[zones]]
zone = "."
zone_type = "Hint"
enabled_dnssec = true
stores = { type = "recursor", roots = "/tmp/root.hints" }

However, in a recursive resolver (a recursor store) we don't want to (DNSSEC) sign records; instead we want to DNSSEC validate them. In RFC #2194, the idea is to put the setting inside the stores field (which maps to the RecursiveConfig type). Once you pick a dnssec_validation option, the enabled_dnssec setting one level above becomes irrelevant.

Only in the case of authoritative name servers -- the file and sqlite store types -- we want to be able to (DNSSEC) sign records. The forward resolver mode (the forward store type) should neither DNSSEC sign records nor DNSSEC validate the answers to the queries it forwards. To me that means, that enable_dnssec should be inside FileConfig and SqliteConfig, that is beneath zones.stores

Overall, it seems that ZoneConfig allows representing impossible configurations and storing settings unrelated to the operating mode of the zone (resolver vs authoritative name server). For example,

• zones.allow_axfr only applies to (secondary?) name servers that serve a zone file
• what happens if you set the zones.zone_type to Primary (authoritative name server) but the store to the recursor type (recursive resolver) -- those are different roles
• zones.file is "a short-hand for StoreConfig::FileConfig"; what happens if you set both zones.file and zones.stores.type = "file"? which one takes precedence? what happens if you set zones.stores.type to something other than file.
• zones.keys are only going to be used if enable_dnssec is true but they can be specified when enable_dnssec is false and you don't get a warning that they are not used (+)

I think it would make sense to:

(1) rework the ZoneConfig struct so that in memory one cannot represent impossible configurations nor store settings that won't be used. I think that would mean using an enum-struct to represent the ZoneType. Something like: (NOTE: very rough approximation as per my understanding of hickory's features)

struct ZoneConfig {
    zone: String,
    zone_type: ZoneType,
}

enum ZoneType {
    Authoritative(AuthoritativeNameServerConfig),
    Hint(RecursiveResolverConfig),
    Forward(ForwardResolverConfig),
}

struct AuthoritativeNameServerConfig {
    dnssec_sign: Option<DnssecSign>
    storage: NameServerStorage,
    // ..
}

struct DnssecSign {
    keys: Vec<Key>
    // ..
}

enum NameServerStorage {
    // zone is stored in a plain `.zone` text file
    File(FileConfig),
    // zone is stored is a sqlite database
    Sqlite(SqliteConfig),
}

struct RecursiveResolverConfig {
    // see hickory-dns RFC #2194
    dnssec_validation: DnssecValidation,
    // ..
}

(2) lint the named.toml file and emit warnings when settings that are not part of ZoneConfig are used (+)

(+) the toml crate is lax / permissive in this regard. it does not emit an error when the input includes a field that's not part of the struct is being deserialized into ( see example below ). performing the linting might require quite a bit of extra code -- unless serde has some "strict" mode that we could use here

use serde::Deserialize;

#[derive(Debug, Deserialize)]
struct Config {
    foo: bool,
}

fn main() {
    let input = "bar = true
foo = false";

    println!("{:?}", toml::from_str::<Config>(&input));
}

prints

Ok(Config { foo: false })

[djc]

Reworking this sounds sensible. I found #[serde(deny_unknown_fields)] which sounds like it'll do what we need?

[japaric]

I found #[serde(deny_unknown_fields)] which sounds like it'll do what we need?

nice finding. that indeed does the trick.

changing ZoneConfig would be a breaking change. are breaking changes currently being merged into main?

[djc]

Yes, we're currently waiting on a new Quinn release which will let us bump Quinn and rustls and related dependencies and will plan to do a new (semver-incompatible) release soon after.