letsencrypt
/
boulder
mirror of https://github.com/letsencrypt/boulder.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Make CAA checking more compliant with the RFC; CAA refactoring 

The CAA response checking method has been refactored to have a
easier to follow straight-line control flow. Several bugs in it have
been fixed:

  - Firstly, parameters for issue and issuewild directives were not
    parsed, so any attempt to specify parameters would result in
    a string mismatch with the CA CAA identity (e.g. "letsencrypt.org").
    Moreover, the syntax as specified permits leading and trailing
    whitespace, so a parameter-free record such as
    "  letsencrypt.org ;  " would not be considered a match.

    This has been fixed by stripping whitespace and parameters. The RFC
    does not specify the criticality of parameters, so unknown
    parameters (currently all parameters) are considered noncritical.
    I justify this as follows:

    If someone decides to nominate a CA in a CAA record, they can,
    with trivial research, determine what parameters, if any, that
    CA supports, and presumably in trusting them in the first place
    is able to adequately trust that the CA will continue to support
    those parameters. The risk from other CAs is zero because other CAs
    do not process the parameters because the records in which they
    appear they do not relate to them.

  - Previously, all of the flag bits were considered to effectively mean
    'critical'. However, the RFC specifies that all bits except for the
    actual critical bit (decimal 128) should be ignored. In practice,
    many people have misunderstood the RFC to mean that the critical bit
    is decimal 1, so both bits are interpreted to mean 'critical', but
    this change ignores all of the other bits. This ensures that the
    remaining six bits are reasonably usable for future standards action
    if any need should arise.

  - Previously, existence of an "issue" directive but no "issuewild"
    directive was essentially equivalent to an unsatisfiable "issuewild"
    directive, meaning that no wildcard identifiers could pass the CAA
    check. This is contrary to the RFC, which states that issuewild
    should default to what is specified for "issue" if no issuewild
    directives are specified. (This is somewhat moot since boulder
    doesn't currently support wildcard issuance.)

  - Conversely, existence of an "issuewild" directive but no "issue"
    directive would cause CAA validation for a non-wildcard identifier
    to fail, which was contrary to the RFC. This has been fixed.

  - More generally, existence of any unknown non-critical directive, say
    "foobar", would cause the CAA checking code to act as though an
    unsatisfiable "issue" directive existed, preventing any issuance.
    This has been fixed.

Test coverage for corner cases is enhanced and provides regression
testing for these bugs.

statsd statistics have been added for tracking the relative frequency
of occurrence of different CAA features and outcomes. I added these on
a whim suspecting that they may be of interest.

Fixes #1436.
This commit is contained in:
Hugo Landau 2016-01-28 05:07:31 +00:00
parent e0bfb710b9
commit 4f27c24cf3
3 changed files with 128 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

31
bdns/mocks.go
View File

@ -107,6 +107,37 @@ func (mock *MockDNSResolver) LookupCAA(_ context.Context, domain string) ([]*dns
secondRecord := record
secondRecord.Value = "letsencrypt.org"
results = append(results, &secondRecord)
case "unknown-critical.com":
record.Flag = 128
record.Tag = "foo"
record.Value = "bar"
results = append(results, &record)
case "unknown-critical2.com":
record.Flag = 1
record.Tag = "foo"
record.Value = "bar"
results = append(results, &record)
case "unknown-noncritical.com":
record.Flag = 0x7E // all bits we don't treat as meaning "critical"
record.Tag = "foo"
record.Value = "bar"
results = append(results, &record)
case "present-with-parameter.com":
record.Tag = "issue"
record.Value = " letsencrypt.org ;foo=bar;baz=bar"
results = append(results, &record)
case "unsatisfiable.com":
record.Tag = "issue"
record.Value = ";"
results = append(results, &record)
case "issuewild.com":
record.Tag = "issuewild"
record.Value = "symantec.com"
results = append(results, &record)
case "issuewild2.com":
record.Tag = "issuewild"
record.Value = "letsencrypt.org"
results = append(results, &record)
}
return results, nil
}

98
va/validation-authority.go
View File

@ -568,9 +568,13 @@ type CAASet struct {
func (caaSet CAASet) criticalUnknown() bool {
if len(caaSet.Unknown) > 0 {
for _, caaRecord := range caaSet.Unknown {
// Critical flag is 1, but according to RFC 6844 any flag other than
// 0 should currently be interpreted as critical.
if caaRecord.Flag > 0 {
// The critical flag is the bit with significance 128. However, many CAA
// record users have misinterpreted the RFC and concluded that the bit
// with significance 1 is the critical bit. This is sufficiently
// widespread that that bit must reasonably be considered an alias for
// the critical bit. The remaining bits are 0/ignore as proscribed by the
// RFC.
if (caaRecord.Flag & (128 | 1)) != 0 {
return true
}
}
@ -637,33 +641,85 @@ func (va *ValidationAuthorityImpl) checkCAARecords(ctx context.Context, identifi
if err != nil {
return
}
if caaSet == nil {
// No CAA records found, can issue
va.stats.Inc("VA.CAA.None", 1, 1.0)
present = false
valid = true
return
} else if caaSet.criticalUnknown() {
present = true
valid = false
return
} else if len(caaSet.Issue) > 0 || len(caaSet.Issuewild) > 0 {
present = true
var checkSet []*dns.CAA
if strings.SplitN(hostname, ".", 2)[0] == "*" {
checkSet = caaSet.Issuewild
} else {
checkSet = caaSet.Issue
}
for _, caa := range checkSet {
if caa.Value == va.IssuerDomain {
valid = true
return
}
}
}
present = true
// Record stats on directives not currently processed.
if len(caaSet.Iodef) > 0 {
va.stats.Inc("VA.CAA.WithIodef", 1, 1.0)
}
if caaSet.criticalUnknown() {
// Contains unknown critical directives.
va.stats.Inc("VA.CAA.UnknownCritical", 1, 1.0)
valid = false
return
}
if len(caaSet.Unknown) > 0 {
va.stats.Inc("VA.CAA.WithUnknownNoncritical", 1, 1.0)
}
checkSet := caaSet.Issue
if strings.SplitN(hostname, ".", 2)[0] == "*" {
// 'issuewild' policy defaults to 'issue' policy, so check against
// 'issue' unless one or more 'issuewild' directives were actually
// specified.
if len(caaSet.Issuewild) > 0 {
checkSet = caaSet.Issuewild // TODO check this
}
}
if len(checkSet) == 0 {
// Although CAA records exist, none of them pertain to issuance in this case.
// (e.g. there is only an issuewild directive, but we are checking for a
// non-wildcard identifier, or there is only an iodef or non-critical unknown
// directive.)
va.stats.Inc("VA.CAA.NoneRelevant", 1, 1.0)
valid = true
return
}
// There are CAA records pertaining to issuance in our case. Note that this
// includes the case of the unsatisfiable CAA record value ";", used to
// prevent issuance by any CA under any circumstance.
//
// Our CAA identity must be found in the chosen checkSet.
for _, caa := range checkSet {
if extractIssuerDomain(caa) == va.IssuerDomain {
va.stats.Inc("VA.CAA.Authorized", 1, 1.0)
valid = true
return
}
}
// The list of authorized issuers is non-empty, but we are not in it. Fail.
va.stats.Inc("VA.CAA.Unauthorized", 1, 1.0)
valid = false
return
}
// Given a CAA record, assume that the Value is in the issue/issuewild format,
// that is, a domain name with zero or more additional key-value parameters.
// Returns the domain name, which may be "" (unsatisfiable).
func extractIssuerDomain(caa *dns.CAA) string {
v := caa.Value
v = strings.Trim(v, " \t") // Value can start and end with whitespace.
idx := strings.IndexByte(v, ';')
if idx < 0 {
return v // no parameters; domain only
}
// Currently, ignore parameters. Unfortunately, the RFC makes no statement on
// whether any parameters are critical. Treat unknown parameters as
// non-critical.
return strings.Trim(v[0:idx], " \t")
}

20
va/validation-authority_test.go
View File

@ -681,6 +681,26 @@ func TestCAAChecking(t *testing.T) {
CAATest{"present.com", true, true},
// Good (multiple critical, one matching)
CAATest{"multi-crit-present.com", true, true},
// Bad (unknown critical)
CAATest{"unknown-critical.com", true, false},
CAATest{"unknown-critical2.com", true, false},
// Good (unknown noncritical, no issue/issuewild records)
CAATest{"unknown-noncritical.com", true, true},
// Good (issue record with unknown parameters)
CAATest{"present-with-parameter.com", true, true},
// Bad (unsatisfiable issue record)
CAATest{"unsatisfiable.com", true, false},
// Bad (issuewild prevents)
CAATest{"*.issuewild.com", true, false},
// Good (issuewild allows)
CAATest{"*.issuewild2.com", true, true},
// CAA records pertain to more than just direct descendents
CAATest{"*.foo.bar.issuewild.com", true, false},
CAATest{"*.foo.bar.issuewild2.com", true, true},
// The presence of an issuewild directive does not affect non-wildcard issuance
CAATest{"foo.bar.issuewild2.com", true, true},
// Good (issuewild defaults to issue, which allows)
CAATest{"*.present.com", true, true},
}
stats, _ := statsd.NewNoopClient()