Skip to content

Add wildcard exact blacklist to PA.#3318

Merged
rolandshoemaker merged 6 commits into
masterfrom
cpu-wildcard-exactblocklist
Jan 4, 2018
Merged

Add wildcard exact blacklist to PA.#3318
rolandshoemaker merged 6 commits into
masterfrom
cpu-wildcard-exactblocklist

Conversation

@cpu

@cpu cpu commented Jan 3, 2018

Copy link
Copy Markdown
Contributor

This commit adds a new wildcardExactBlacklist map to the PA's
AuthorityImpl that is used by WillingToIssueWildcard to decide if
a wildcard issuance would cover a high value domain.

This prevents getting a wildcard for "*.example.com" if
"highvalue.example.com" is on the exact blacklist since it would
circumvent the intention of the exact blacklist by minting a certificate
that could be used for "highvalue.example.com".

Resolves #3239

This commit adds a new "wildcardExactBlacklist" map to the PA's
`AuthorityImpl` that is used by `WillingToIssueWildcard` to decide if
a wildcard issuance would cover a high value domain.

This prevents getting a wildcard for "*.example.com" if
"highvalue.example.com" were on the exact blacklist since it would
circumvent the intention of the exact blacklist by minting a certificate
that could be used for "highvalue.example.com".
@cpu

cpu commented Jan 3, 2018

Copy link
Copy Markdown
Contributor Author

I filed #3319 for a follow-up to add an integration test. This is blocked presently.

I was wrong! Only multi-domain DNS-01 is blocked. d10955d adds an integration test to this PR.

@jsha jsha left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a question of whether an ExactBlacklist entry for highvalue.example.org should block *.highvalue.example.org in addition to *.example.org. This is mainly based on an implementation difficulty: Since WillingToIssueWildcard calls WillingToIssue on the base domain, an issuance attempt for *.highvalue.example.org winds up calling WillingToIssue(highvalue.example.org), which hits the exact blacklist.

I think we can remedy this by calling prepending an example label, like "x", so we wind up calling WillingToIssue(x.highvalue.example.org) (which would pass). This is a little silly, but I think it's better than creating surprising semantics for ExactBlacklist. Note that I propose a single-letter prefix ("x") instead of "example" to avoid edge cases around super long domain names.

Comment thread policy/pa.go Outdated
//
// First, split the domain into two parts: the first label (including the
// ".") and the rest of the domain.
parts := strings.SplitAfterN(v, ".", 2)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think SplitN works just as well here and is slightly clearer (you can remove the "including the ." comment above).

Comment thread policy/pa.go Outdated
return errICANNTLDWildcard
}
// The base domain can't be in the wildcard exact blacklist
if pa.wildcardExactBlacklist[baseDomain] {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to lock the mutex before this access.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch - you're right 👍

@cpu

cpu commented Jan 3, 2018

Copy link
Copy Markdown
Contributor Author

Ready for another 🔍 round

Comment thread policy/pa.go
// Since the base domain without the "*." may trip the exact hostname policy
// blacklist when the "*." is removed we replace it with a single "x"
// character to differentiate "*.example.com" from "example.com" for the
// exact hostname check.

@rolandshoemaker rolandshoemaker Jan 3, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to immortalize an OOB conversation: this is a bit hacky, and should probably be done in a different way, but doing so would require separating the valid domain structure logic from the policy/blacklist logic. If we want to push that work to another sprint can we add a TODO with the relevant ticket.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed #3323 for this and added a TODO in the code with a reference as part of e1f7aa2

@rolandshoemaker

Copy link
Copy Markdown
Contributor

Generally LGTM, conflicts + TODO for the followup work.

@rolandshoemaker rolandshoemaker merged commit 292b2a2 into master Jan 4, 2018
@cpu cpu deleted the cpu-wildcard-exactblocklist branch January 4, 2018 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants