TSIG Authentication not used during queries

[jnahelou]

Hi there,

When using ACLs on resolvers for update and queries, Terraform doesn't provide tsig config during queries.

Terraform Version

Terraform v0.11.3

Affected Resource(s)

• All resourceDns*RecordSetRead

Terraform Configuration Files

provider "dns" {
  update {
    server = "127.0.0.1"

    key_secret    = "xxxx"
    key_name      = "yyyy."
    key_algorithm = "hmac-sha256"
  }
}

resource "dns_a_record_set" "www-prod" {
  zone     = "test.local."
  name     = "foo"

  addresses = [
    "192.168.0.1",
  ]
}

Debug Output

* dns_a_record_set.www-prod: 1 error(s) occurred:
* dns_a_record_set.www-prod: dns_a_record_set.www-prod: Error querying DNS record: 5

Expected Behavior

DNS Queries should use tsig name/secret

                msg := new(dns.Msg)
                msg.SetQuestion(rec_fqdn, dns.TypeA)
                // Add auth
                if keyname != "" {
                        msg.SetTsig(keyname, keyalgo, 300, time.Now().Unix())
                }
                r, _, err := c.Exchange(msg, srv_addr) 

Actual Behavior

Anonymous queries are sent

                msg := new(dns.Msg)
                msg.SetQuestion(rec_fqdn, dns.TypeA)
                r, _, err := c.Exchange(msg, srv_addr) 

[apparentlymart]

Hi @jnahelou!

Agreed that this doesn't seem quite right. I assume it works for many users because they have the nameserver configured to only require authentication for writes, but it is possible and valid to also require authentication for reads, if e.g. you handle updates on a separate server that doesn't respond to end-user queries and then synchronize the updates into a public-facing server.

The Terraform team at HashiCorp won't be able to work on this in the near future due to our focus being elsewhere, but we'd be happy to review a pull request if you or someone else has the time and motivation to implement it.

[jnahelou]

What do you think about adding a property auth_query on provider definition to enable authentication (or keeping it anonymous) to keep backward compatibility:

"auth_query": &schema.Schema{
    Type:        schema.TypeBool,
    Optional:    true,
    Default:     false,
    DefaultFunc: schema.EnvDefaultFunc("DNS_UPDATE_QUERYAUTH", nil),
},

and use it like :

provider "dns" {
  alias = "prod"

  update {
    server     = "xxx"
    auth_query = true

    key_name      = "xxxxx."
    key_secret    = "yyyyy"
    key_algorithm = "hmac-sha256"
  }
}

Where queries can be :

               authquery := meta.(*DNSClient).authquery
               if authquery {
                       keyname := meta.(*DNSClient).keyname
                       keyalgo := meta.(*DNSClient).keyalgo
                       if keyname != "" {
                               msg.SetTsig(keyname, keyalgo, 300, time.Now().Unix())
                       }
               }

I'm waiting for PR #28 to start new PR :)

[bodgit]

I would say the new attribute doesn't belong in the update block as this would also presumably affect the various dns_* data sources which don't perform any updates.

I'm struggling a bit with why you'd want to require TSIG on queries apart from because you can. I do something similar whereby I send updates to a non-public DNS server which is slaved by public DNS servers so I require TSIG for the updates but if someone wants to query the internal server then I don't really care as the data is publicly accessible anyway.

[jnahelou]

I use infoblox DNS cluster composed by one gridmaster cluster and resolvers. GM is the only one to allow update. To ensure performance, we denied queries on GM (that's resolvers's job).
This is a normal usage of TSIG to manage ACLs for update,transfer or query.
Ref rfc2845
I know that it's a major modification which need to update all dns resources, that's why before PR, I'm looking for advices.

[apparentlymart]

I would not expect any of the data sources in this provider to use authentication, and indeed they cannot as currently implemented because they are using the Go resolver directly.

Given that, it seems reasonable for the Read implementations in the resources to sign their requests and guarantee that they talk to the same endpoint they'd write updates to, and have that be the only behavior. Is there a situation where it would be desirable to allow authenticated writes and anonymous reads but not allow authenticated reads?

It's been a long time since I had a DNS server that supported updates so I'd love to hear your thoughts on what seems like reasonable behavior here. If possible I'd rather just do "the right thing" by default rather than adding another option, since each additional option adds both user-facing complexity and testing/maintenence complexity.

[bodgit]

Is there a situation where it would be desirable to allow authenticated writes and anonymous reads but not allow authenticated reads?

I think if you send a TSIG-signed query to a server that doesn't require it, it just gets ignored so signing all reads might work, at the expense of sending larger DNS packets. Handling EDNS0 and/or TCP for queries would become necessary.

I had a think about it, and an alternative could be to extend the provider configuration with an optional read block with the same options as the update block. That would then allow you the most flexibility of having different keys for reads vs. updates should anyone ever want that and also allow you to use different servers too, if someone ever had that as a design choice.

[jnahelou]

I like the idea of an optional read block in provider configuration which will use the update server if server is missing and anonymous queries if no keys are set.
I changed the configureProvider to return 2 DNSClient (with the 2 configurations) :

type DNSUpdater struct {
       Update *DNSClient
       Read   *DNSClient
}

Then it's easy to update existing code like :

• Read :

 c := meta.(*DNSUpdater).Read.c  
srv_addr := meta.(*DNSUpdater).Read.srv_addr
keyname := meta.(*DNSUpdater).Read.keyname 
keyalgo := meta.(*DNSUpdater).Read.keyalgo             

• Update :

c := meta.(*DNSUpdater).Update.c
srv_addr := meta.(*DNSUpdater).Update.srv_addr
keyname := meta.(*DNSUpdater).Update.keyname
keyalgo := meta.(*DNSUpdater).Update.keyalgo

I tested it locally on my own cluster (with ACL on read & write).
Here is my first draft following @bodgit suggestion : jnahelou/terraform-provider-dns

[apparentlymart]

Thanks for prototyping that, @jnahelou!

The separate read block here feels overly complex to me so I'd prefer to only go that route if we have a clear use-case in mind. A DNS server requiring different credentials to read back a value that was just written feels like an edge-case to me, but again I'm happy to be corrected on that.

My other worry about the separate read block is that it would seem to imply that our various data resources in this provider would use authentication too. We have no plans to support that since these data sources use the DNS functionality provided by the standard library rather than making DNS requests directly, and the resulting simplicity is desirable.

My preference (and again, I'm happy to change this if we can convince ourselves that there are common situations that would not be supported by this) is to have the credentials given in the update block be used for all requests made by the resources in this provider, accepting that this makes requests larger (which may require TCP) when we refresh an existing record. This feels like a reasonable tradeoff to me since reads from the resources here should be infrequent (compared to DNS lookups from normal clients) and thus should not create a burden on the network, and reading objects back using the same credentials that wrote them is the expected behavior from every other Terraform provider.

[katbyte]

Hi @jnahelou,

Just wanted to let you know that we have just released v2.0.0 of the dns provider where it now uses TSIG authentication 🙂

[TheEnbyperor]

I think if you send a TSIG-signed query to a server that doesn't require it, it just gets ignored

Just came across this and I can say for certain at least the knot DNS does not act this way. If it isn't expecting a TSIG record it responds with a REFUSED. I think a flag should be implemented.

[github-actions]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.