fix(acm): correct wildcard validation record names - #2575
Conversation
|
🎉 Thanks for your first pull request to Floci! Your CI checks need a maintainer to approve them before they run. That is GitHub's standard gate on first-time contributors, not a problem with your PR — so if the checks look like they are doing nothing, that is why. Once a maintainer approves, CI and the compatibility suite start automatically. Nothing is needed from you in the meantime. While you wait, a couple of things that make review faster:
Come join us in Slack — it is the fastest way to reach maintainers if you get stuck, or want feedback on an approach before investing more time in it. |
|
| Filename | Overview |
|---|---|
| src/main/java/io/github/hectorvent/floci/services/acm/AcmService.java | Normalizes wildcard domains and generates deterministic ACM validation record names and values. |
| src/test/java/io/github/hectorvent/floci/services/acm/AcmEdgeCaseTest.java | Adds integration assertions for wildcard record shapes, shared apex/wildcard records, case normalization, and deterministic records. |
Reviews (3): Last reviewed commit: "fix(acm): correct wildcard validation re..." | Re-trigger Greptile
There was a problem hiding this comment.
Thanks Aniket — the core fix is right, and it's right for the right reason. I checked the
wildcard stripping against ACM's own CNAME table rather than taking the issue's word for
it, and the documented shape is exactly what you produce.
local build came back PASS — merges cleanly onto current main (383947c9) and
./mvnw test succeeded, so the unticked checklist box is covered; no need to chase the
full suite on your machine.
What I verified
The wildcard label really is stripped, and only from the record name. From ACM's DNS
validation guide:
| Domain name | Record Name | Record Value | Comment |
|*.example.com|_{x1}.example.com.|_{x2}.acm-validations.aws.| Identical |
So _hash.example.com. for *.example.com is correct, and the literal * label the issue
reported is genuinely wrong.
Keeping the * in DomainName is also correct. DomainValidation.DomainName is
Required: Yes with pattern (\*\.)?(((?!-)[A-Za-z0-9-]{0,62}[A-Za-z0-9])\.)+… — the
wildcard belongs there and is stripped only from the record. Your test asserts both halves
(DomainName equals *.example.com, record name matches
^_[0-9a-f]{32}\.example\.com\.$), which is the pair that matters.
One code path, so the fix covers primary and SAN alike. generateDomainValidation is
called from a single loop over allSans (AcmService.java:195), and acm-validations
appears exactly once in the whole service — there's no second builder that would still emit
the *. Worth saying because a one-line fix in a shared helper is only safe if it really
is the only one.
Nested wildcards land correctly too. startsWith("*.") on *.api.example.com yields
_hash.api.example.com., which matches ACM's rule that the wildcard is only ever the
leftmost label.
The regex assertion is the right strictness — 32 lowercase hex, literal trailing dot,
anchored — so it would fail on a stray *, a missing dot, or a truncated token, rather
than just on the one symptom.
(should fix) The wildcard and its base domain must produce the identical record — they still don't
The same doc table says this explicitly:
In the table, note that the first two Record Name-Record Value pairs are the same.
This illustrates that for a wildcard domain, such as*.example.com, the strings created
by ACM are the same as those created for its base domain,example.com. Otherwise, the
paired Record Name and Record Value differ for each domain name.
generateValidationToken (AcmService.java:643-647) returns 32 random bytes, so
*.example.com and example.com get different tokens. After this PR the wildcard's suffix
is fixed, but a certificate covering both still comes back as:
*.example.com _A.example.com. → _A2.acm-validations.aws.
example.com _B.example.com. → _B2.acm-validations.aws.
where AWS returns the same pair twice.
That matters here specifically because it's the reporter's own configuration. #2541's debug
section says: "Observed on: cert with domain_name = "*.nonprod.janus.example.com",
subject_alternative_names = ["nonprod.janus.example.com"]." So the issue is only half
closed by this change.
And it can still fail the provider. The for_each keyed on dvo.domain_name happens to
create both records and pass — but the equally common dedupe idiom, keying by
resource_record_name or wrapping in distinct(...) precisely because on real AWS the two
entries collide, creates one record. Then the other DVO's record is genuinely absent, and
you get the same missing … DNS validation record failure the issue opened with, just from
a different direction.
(minor) generateValidationToken(String domain) never reads domain
private String generateValidationToken(String domain) {
byte[] randomBytes = new byte[32];
SECURE_RANDOM.nextBytes(randomBytes);
return HexFormat.of().formatHex(randomBytes);
}The unused parameter is what makes the finding above easy to miss — generateValidationToken(domain)
at line 622 reads as though the token were derived from the domain, which is the one thing
that would make wildcard/base sharing fall out for free.
Deriving it from a stable hash of the account id plus the wildcard-stripped domain fixes
that and a second documented behaviour at the same time:
Without the need to repeat validation, you can request additional ACM certificates for
your fully qualified domain name (FQDN) for as long as the CNAME record remains in place.
… Since the CNAME validation token works for any AWS Region, you can re-create the same
certificate in multiple Regions.
Today, re-requesting a certificate for the same FQDN hands back a brand-new CNAME every
time, so a user who already placed the record has to place another one. Either derive the
token or drop the parameter — right now the signature and the body disagree.
(minor) The test that would pin this already exists and wasn't touched
wildcardDomainAsSan (AcmEdgeCaseTest.java:65-80) already requests example.com with
SANs ["*.example.com", "www.example.com"] — which is exactly the doc's Identical/Unique
table — and still asserts only that the ARN starts with arn:aws:acm:. Two assertions
there would cover both the fix and the finding above:
- the
*.example.comandexample.comentries carry equalResourceRecord.Nameand
equal.Value - the
www.example.comentry differs from both
nestedWildcardDomain is in the same shape: *.api.example.com is handled correctly today
but nothing asserts it, so the leftmost-label case is unprotected.
Notes
ValidationDomainis populated with the raw domain including the*
(new DomainValidation(domain, domain, …),AcmService.java:633-635). The reference
describes it as "The domain name that ACM used to send domain validation emails" —
an EMAIL-validation field,Required: No, which real ACM omits on DNS-validated
certificates. Its pattern does allow a leading(\*\.)?, so the*isn't malformed
there, and I can't confirm the omission from the reference either way. Pre-existing and
adjacent — not asking you to touch it in this PR, just noting it's the other member
carrying a wildcard.- The description says the new coverage "preserves the existing behavior for concrete
domains", but nothing in this diff asserts a concrete domain's record name, and the
pre-existing ACM tests don't checkResourceRecord.Nameeither. The behaviour is
preserved — the claim just isn't backed by an assertion yet.
The one-line change is correct and well-targeted. What I'd want before this closes #2541 is
the wildcard/base collision, since that's the configuration the issue actually reports.
de575f1 to
167d412
Compare
|
Thanks for the detailed review — fixed in 167d412. Validation tokens are now deterministic per account and wildcard-stripped domain, so |
pgermosen
left a comment
There was a problem hiding this comment.
Thanks Aniket — both round-1 findings are closed, and the fix for the main one is the right shape.
generateValidationToken now derives from the account id plus the wildcard-stripped domain, so
*.example.com and example.com share a token and produce the same record on both halves. That's
exactly the doc table:
| *.example.com | _x1.example.com. | _x2.acm-validations.aws. | Identical |
| example.com | _x1.example.com. | _x2.acm-validations.aws. | Identical |
| www.example.com | _x3.www.example.com. | _x4.acm-validations.aws. | Unique |
And wildcardDomainAsSan now pins all three rows of it — equal Name and equal Value for the
pair, unequal for www. That's the assertion set that makes the behaviour a contract rather than a
coincidence: revert the token to SecureRandom and the two entries get separate tokens, so the
test fails rather than passing quietly.
The unused-parameter finding is closed as a side effect — the signature and the body agree now.
Worth calling out that this also picks up the second documented behaviour I quoted, for free: the
token input carries no certificate id and no region, so re-requesting the same FQDN returns the
same CNAME.
Without the need to repeat validation, you can request additional ACM certificates for your fully
qualified domain name (FQDN) for as long as the CNAME record remains in place. … Since the CNAME
validation token works for any AWS Region, you can re-create the same certificate in multiple
Regions.
That's now true of Floci where it wasn't before. It's also the thing nothing tests — see the
question at the bottom.
This is a read of the diff; I don't have a build for this round. Happy to run one if you want it
before you finalise.
(should fix) The two wildcard-strips disagree on case, and the record name is the one that loses
The strip is now computed twice, from the same expression, in two places:
// generateDomainValidation, AcmService.java:623
String validationDomain = domain.startsWith("*.") ? domain.substring(2) : domain;
// generateValidationToken
String validationDomain = domain.startsWith("*.") ? domain.substring(2) : domain;
String tokenInput = regionResolver.getAccountId() + ":" + validationDomain.toLowerCase(Locale.ROOT);The token copy lowercases. The record-name copy at :625 doesn't — it interpolates
validationDomain as given. Nothing else in AcmService normalises domain case either; the only
toLowerCase in the file is the tag-key aws: check at :605. So a certificate for
*.EXAMPLE.com with SAN example.com comes back as:
*.EXAMPLE.com _x1.EXAMPLE.com. → _x2.acm-validations.aws.
example.com _x1.example.com. → _x2.acm-validations.aws.
Identical Value, and Names that differ only in case. A resolver wouldn't care — DNS names are
case-insensitive — but the consumer here isn't a resolver. A for_each or distinct(...) keyed on
resource_record_name compares strings, so it sees two records where AWS returns one, and you're
back to the class of failure #2541 opened with, just reached from a different direction.
Both problems have one fix: pull the strip into a single helper that also lowercases, and call it
from both sites.
private static String baseDomain(String domain) {
String stripped = domain.startsWith("*.") ? domain.substring(2) : domain;
return stripped.toLowerCase(Locale.ROOT);
}That matters beyond the case bug. The whole invariant this PR establishes — wildcard and base
produce the same record — now rests on two separately-maintained copies of the same expression
agreeing forever. They already don't. One helper makes the collision structural instead of a
coincidence that holds as long as nobody edits one copy.
(minor) nestedWildcardDomain is still unasserted
Carried over from round 1 and not mentioned in your reply, so flagging once more in case it was
missed rather than declined. AcmEdgeCaseTest.java:84-99 requests *.api.example.com and still
asserts only that the ARN starts with arn:aws:acm:. The leftmost-label rule is the other half of
the wildcard behaviour this PR is about, it works correctly today, and nothing stops a future
lastIndexOf("*.")-style rewrite from turning it into _hash.example.com.
The same one-liner you added to wildcardDomainAsPrimary covers it:
.body("Certificate.DomainValidationOptions[0].ResourceRecord.Name",
matchesPattern("^_[0-9a-f]{32}\\.api\\.example\\.com\\.$"));(question) Nothing pins the cross-certificate stability
The determinism is the headline change in this round, but every assertion for it lives inside
one certificate. Seed the hash with the certificate ARN — a plausible "make tokens unique per cert"
refactor — and wildcardDomainAsSan still passes, because both entries would still come from the
same certificate.
What isn't covered is the property the doc paragraph above describes: two separate
RequestCertificate calls for example.com should now hand back the same ResourceRecord. That's
the behaviour a user relies on when they've already placed the CNAME and re-request, and it's the
one this round newly delivers.
Two requests and one assertEquals on the record name would lock it. Deliberate omission, or just
where the round ended?
Notes
MessageDigest.getInstance("SHA-256")per call rather than a static field is correct —
MessageDigestisn't thread-safe, and hoisting it to a constant next to the oldSECURE_RANDOM
would have been the natural-looking mistake. Might be worth a short comment saying so, since the
removed field sat in exactly that spot.- SHA-256 hex is 64 characters, same width as the 32 random bytes it replaces, so the
substring(0, 32)/substring(32)split is untouched and the[0-9a-f]{32}assertions still
describe reality. Easy thing to break in a change like this and it didn't. Locale.ROOTon the lowercase is the right call rather than the default locale — worth keeping
when this moves into the shared helper.- Validation tokens are now predictable from account id and domain. That's the point, and it's
fine for an emulator — noting it only so it reads as a deliberate property rather than something
to "harden" later. ValidationDomainstill carries the raw*(AcmService.java:633-635). Pre-existing, raised as
a note last round, and still not something I'm asking you to change here — repeating it only so
it doesn't look like it was silently dropped.- The description's "preserves the existing behavior for concrete domains" is backed now:
www.example.comgets an explicit inequality assertion on both members, which is the concrete-domain
case the earlier wording was reaching for.
167d412 to
b65cd3c
Compare
|
Thanks for the second review. Fixed the remaining items in b65cd3c: wildcard stripping and case normalization now live in one baseDomain helper used by both the record name and token derivation; the uppercase wildcard/base-domain case now asserts identical records; nested wildcard coverage pins the expected _hash.api.example.com. name; and a new regression creates two separate certificates for the same FQDN and asserts their complete ResourceRecord values are identical. Focused ACM tests: 14 passed. |
pgermosen
left a comment
There was a problem hiding this comment.
Approving. This round closes everything from the last one — the strip and lowercase are unified into one baseDomain() helper, so the case-mismatch bug can't recur by accident, and the mixed-case test (*.EXAMPLE.com) pins it directly. nestedWildcardDomain finally asserts the record name instead of just the ARN prefix, and the new cross-certificate test locks in the determinism the AWS docs describe. Nice work chasing this all the way through.
Summary
Correct ACM DNS validation record names for wildcard domains by removing the literal wildcard label before adding the validation prefix. Regression coverage verifies wildcard names and preserves the existing behavior for concrete domains.
Fixes #2541
Type of change
fix:)feat:)feat!:orfix!:)AWS Compatibility
Wildcard certificates incorrectly produced validation record names containing a literal
*label. The generated name now matches ACM's wildcard DNS validation shape.Checklist
./mvnw testpasses locallyFocused verification:
AcmEdgeCaseTest(13 tests) passes locally.