Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Route53 doesn't validate TXT record lengths #14941

Open
wkoszek opened this issue Sep 1, 2020 · 9 comments
Open

Route53 doesn't validate TXT record lengths #14941

wkoszek opened this issue Sep 1, 2020 · 9 comments
Labels
bug Addresses a defect in current functionality. good first issue Call to action for new contributors looking for a place to start. Smaller or straightforward issues. service/route53 Issues and PRs that pertain to the route53 service.

Comments

@wkoszek
Copy link

wkoszek commented Sep 1, 2020

Community Note

  • Please vote on this issue by adding a 馃憤 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Terraform CLI and Terraform AWS Provider Version

Terraform v0.13.1

Affected Resource(s)

  • aws_route53_record

Terraform Configuration Files

resource "aws_route53_record" "google_mail_dkim2" {
  zone_id = aws_route53_zone.segmed_ai.zone_id
  name = "google._domainkey"
  type = "TXT"
  ttl = "1799"
  records = [
    "v=DKIM1; k=rsa; p=verylongthinggoes........................................................................................................................................................................................................................................................................here"
  ]
}

Debug Output

Panic Output

[ERR]: Error building changeset: InvalidChangeBatch: [Invalid Resource Record: FATAL problem: CharacterStringTooLong (Value is too long) encountered with '"v=DKIM1; k=rsa; p=MIIBIjANBgkqhRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRrRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRRR_very_long_string"']
status code: 400, request id: 1f3fcfb0-344a-4b66-adbd-a40788df8990

Expected Behavior

Since 255 length limit is known, I'd expect terraform plan to warn me about a domain name that is too long, so that my GitOps "prep" stage could fail.

Actual Behavior

terraform plan is OK with very long domains. terraform apply crashes.

Steps to Reproduce

  1. Take a DNS block from above. The idea is that you should attempt to make a very long (400characters+) long TXT record, which is popular in DKIM settings.
  2. Run terraform plan and see it succeed.
  3. Run terraform apply and see it crash with CharacterStringTooLong error

Important Factoids

References

  • #0000
@ghost ghost added the service/route53 Issues and PRs that pertain to the route53 service. label Sep 1, 2020
@github-actions github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label Sep 1, 2020
@gdavison gdavison added bug Addresses a defect in current functionality. good first issue Call to action for new contributors looking for a place to start. Smaller or straightforward issues. and removed needs-triage Waiting for first response or review from a maintainer. labels Sep 1, 2020
@wkoszek
Copy link
Author

wkoszek commented Sep 4, 2020

BTW, the solution to "fix" is to split the record to 255-byte chunks. So you end up with:

"1stpartof256byte\"\"other255bytepart\"

It's quite error prone. Great addition would be allow me to re-paste DKIM / TXT records the way GSuite / Hubspot etc software tell me too, without a need of manual tinkering.

@gmtreacy
Copy link

gmtreacy commented Sep 6, 2020

The records are in a schema.TypeSet and there is no helper validation support for sets.

something like this would be nice but is not possible at this time:

resource_aws_route53_record.go

diff --git a/aws/resource_aws_route53_record.go b/aws/resource_aws_route53_record.go
index 63e1e26ee..daae949d3 100644
--- a/aws/resource_aws_route53_record.go
+++ b/aws/resource_aws_route53_record.go
@@ -235,6 +235,7 @@ func resourceAwsRoute53Record() *schema.Resource {
                                Elem:          &schema.Schema{Type: schema.TypeString},
                                Optional:      true,
                                Set:           schema.HashString,
+                               ValidateFunc:  validation.StringLenBetween(1, 255),
                        },
 
                        "allow_overwrite": {

zordak@sony ~/repos/issues/14941 $ terraform plan

Error: InternalValidate

Internal validation of the provider failed! This is always a bug
with the provider itself, and not a user issue. Please report
this bug:

1 error occurred:
        * resource aws_route53_record: records: ValidateFunc and ValidateDiagFunc are
not yet supported on lists or sets.

hashicorp/terraform#6508
hashicorp/terraform-plugin-sdk#156

@angelabad
Copy link
Contributor

His, this is documented in resource official docs:

records - (Required for non-alias records) A string list of records. To specify a single record value longer than 255 characters such as a TXT record for DKIM, add \"\" inside the Terraform configuration string (e.g. "first255characters\"\"morecharacters").

@philnichol
Copy link
Contributor

Hope you all don't mind but I've raised #16317 that hopefully addresses this, let me know if I've missed anything 馃槃

@wkoszek
Copy link
Author

wkoszek commented Nov 19, 2020

@philnichol The PR looks good, but I wonder if we're bound to the current way of doing it, just because we already have this limit in place, and for backward compatibility, or could we do something better?

If I give domain record > 255 chars, why can't AWS provider split it up for me? What's the advantage of me putting quotes in place?

@philnichol
Copy link
Contributor

@wkoszek I agree with you personally. Main thought behind just adding validation for me is backward compatibility and to avoid introducing a breaking change to route53 (which is pretty
terrifying).
Definitely one for the maintainers to advise on rather than me 馃槃, I'm quite new to this repo and have only contributed tiny amounts.

@zimbatm
Copy link

zimbatm commented Dec 14, 2020

Hit this again today. Here is the nicest workaround I could come up with:

locals {
  dkim_record = "v=DKIM1; k=rsa; p=MIIBIjAN..."
  zone_id = "..."
  domain = "mydomain.com"
}

resource "aws_route53_record" "scarf-sh-google-domainkey" {
  zone_id = local.zone_id
  name    = "google._domainkey.${local.domain}"
  type    = "TXT"
  # AWS only accepts records up to 255 chars. We know that this one is ~410.
  # The provider doesn't know about it. So this is our workaround.
  # See https://github.com/hashicorp/terraform-provider-aws/issues/14941
  records = [
    join("\"\"", [
      substr(local.dkim_record, 0, 255),
      substr(local.dkim_record, 255, 255),
    ])
  ]
  ttl = "300"
}

ruohola added a commit to skoleapp/skole-infra that referenced this issue Jan 25, 2021
> `records` - (Required for non-alias records) A string list of records.
> To specify a single record value longer than 255 characters such as a
> TXT record for DKIM, add `\"\"` inside the Terraform configuration
> string (e.g. `"first255characters\"\"morecharacters"`).
>
> https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/route53_record#records

More info:
hashicorp/terraform-provider-aws#14941 (comment)
@kevinmartin
Copy link
Contributor

kevinmartin commented Oct 5, 2021

Another workaround, in case your record would need to be split up more than once potentially (or even 0 times, ie. within a module that creates these records). This will just insert \"\" after every 255th character. Works for any length of string.

resource "aws_route53_record" "example" {
  ...
  records = [
    replace(local.dkim_record, "/(.{255})/", "$1\"\"")
  ]
}

@ei-grad
Copy link
Contributor

ei-grad commented May 10, 2023

Splitting a single DNS record to several DNS records automatically in aws_route53_record resource code is very arguable behavior. While it could be valid for DKIM records, it would lead to unexpected failures with other records. Mentioned workaround looks like the proper way to configure DKIM records (or it could be split by hand otherwise, which also looks absolutely normal). This issue only states that the validation should be in place, terraform plan should fail if it is clear that the record resource definition is invalid. And a TXT DNS record with length more than 255 is clearly invalid.

// got the difference between "different TXT DNS records" and "TXT DNS record with several 255-or-less strings", RTFD :(

Was something wrong with #16317 ? Any chances such PR get merged after proper rebase and updating?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Addresses a defect in current functionality. good first issue Call to action for new contributors looking for a place to start. Smaller or straightforward issues. service/route53 Issues and PRs that pertain to the route53 service.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants