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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CAA support #132

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@whs
Contributor

whs commented Jun 6, 2017

This PR add CAA support.

CAA is supported by Google Cloud DNS. (I'm not sure whether you would have to get in DNSSEC beta to gain access or not, but it was just a simple Google Forms before I could get in) Also, BIND obviously supported this.

I almost never wrote Go before (and this is my first Go PR), so if you could be more specific when suggesting changes I'd be appreciated.

Design decision that I think worth discussing:

  • I implement CAA with a metadata field caa_tag plus a new struct field CAAFlag, as the flag needs to be int. Should I move the tag into the struct as well?
    • edit: After reviewing the SRV changes I moved the tag into the struct
  • Or, should CAA be implemented without special parsing like TXT records? (i.e. CAA("@", "0 issue \"letsencrypt.org\""))
  • Should CAA tag be validated? I think that it would be more future-proof without validation.
  • Flag is implemented by modifier because most of the time people would probably use no flag (0 in record). However, this cause mismatch in function definition to record ordering (CAA("@", "iodef", "mailto:test@example.com", CAA_CRITICAL) vs @ CAA 1 iodef "mailto:test@example.com"). Should I move flag into the function definition?
  • The target field are quoted automatically. Is this too much magic?
@whs

This comment has been minimized.

Show comment
Hide comment
@whs

whs Jun 7, 2017

Contributor

I'm getting a crash with bind. I'll try to fix this later:

github.com/StackExchange/dnscontrol/providers/bind.rrToRecord(0xe9d3a0, 0xc42013c230, 0xc4203baff0, 0x9, 0x1, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/home/whs/build/go/src/github.com/StackExchange/dnscontrol/providers/bind/bindProvider.go:78 +0x53e
github.com/StackExchange/dnscontrol/providers/bind.(*Bind).GetDomainCorrections(0xc420057ab0, 0xc4203545b0, 0xc420272174, 0x4, 0xc4202fb298, 0x1, 0x0)
	/home/whs/build/go/src/github.com/StackExchange/dnscontrol/providers/bind/bindProvider.go:179 +0x4a8
main.main()
	/home/whs/build/go/src/github.com/StackExchange/dnscontrol/main.go:187 +0xcdc
Contributor

whs commented Jun 7, 2017

I'm getting a crash with bind. I'll try to fix this later:

github.com/StackExchange/dnscontrol/providers/bind.rrToRecord(0xe9d3a0, 0xc42013c230, 0xc4203baff0, 0x9, 0x1, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/home/whs/build/go/src/github.com/StackExchange/dnscontrol/providers/bind/bindProvider.go:78 +0x53e
github.com/StackExchange/dnscontrol/providers/bind.(*Bind).GetDomainCorrections(0xc420057ab0, 0xc4203545b0, 0xc420272174, 0x4, 0xc4202fb298, 0x1, 0x0)
	/home/whs/build/go/src/github.com/StackExchange/dnscontrol/providers/bind/bindProvider.go:179 +0x4a8
main.main()
	/home/whs/build/go/src/github.com/StackExchange/dnscontrol/main.go:187 +0xcdc
@whs

This comment has been minimized.

Show comment
Hide comment
@whs

whs Jun 7, 2017

Contributor

I've fixed the issue with parsing CAA records in BIND

Contributor

whs commented Jun 7, 2017

I've fixed the issue with parsing CAA records in BIND

@tlimoncelli

This comment has been minimized.

Show comment
Hide comment
@tlimoncelli

tlimoncelli Jul 14, 2017

Contributor

Adding new rtypes will become a lot easier once https://github.com/StackExchange/dnscontrol/tree/tlim_srv is merged. At that point, we should rebase. That particular panic might go away :-)

Contributor

tlimoncelli commented Jul 14, 2017

Adding new rtypes will become a lot easier once https://github.com/StackExchange/dnscontrol/tree/tlim_srv is merged. At that point, we should rebase. That particular panic might go away :-)

@tlimoncelli

This comment has been minimized.

Show comment
Hide comment
@tlimoncelli

tlimoncelli Jul 19, 2017

Contributor

https://github.com/StackExchange/dnscontrol/tree/tlim_srv is merged. Please rebase. It should be a lot easier to add CAA now.

Contributor

tlimoncelli commented Jul 19, 2017

https://github.com/StackExchange/dnscontrol/tree/tlim_srv is merged. Please rebase. It should be a lot easier to add CAA now.

@whs

This comment has been minimized.

Show comment
Hide comment
@whs

whs Jul 21, 2017

Contributor

I'll try to get this done tomorrow.

Contributor

whs commented Jul 21, 2017

I'll try to get this done tomorrow.

@whs

This comment has been minimized.

Show comment
Hide comment
@whs

whs Jul 23, 2017

Contributor

Rebased. It seems that master is currently broken though.

Contributor

whs commented Jul 23, 2017

Rebased. It seems that master is currently broken though.

@tlimoncelli

This looks great so far! I have a couple tiny suggestions and there are a few tests to add.

Please add:
pkg/js/parse_tests/014-caa.js
pkg/js/parse_tests/014-caa.json

014-caa.js should probably be something like:

D("foo.com","none",
    // Allow letsencrypt to issue certificate for this domain
    CAA("@", "issue", "letsencrypt.org"),
    // Allow no CA to issue wildcard certificate for this domain
    CAA("@", "issuewild", ";"),
    // Report all violation to test@example.com. If CA does not support
    // this record then refuse to issue any certificate
    CAA("@", "iodef", "mailto:test@example.com", CAA_CRITICAL)
);

(I'll let you work out what 014-caa.json should contain).

I'd also add an integration test. If you don't have an environment that can run them, please let me know and I can do that for you or help you get set up. My suggested test is something like:

diff --git a/integrationTest/integration_test.go b/integrationTest/integration_test.go
index 04f8d2a..7913497 100644
--- a/integrationTest/integration_test.go
+++ b/integrationTest/integration_test.go
@@ -327,6 +327,15 @@ var tests = []*TestCase{
        tc("Change Weight", srv("@", 52, 62, 7, "foo.com."), srv("@", 15, 65, 75, "foo4.com.")).IfHasCapability(providers.CanUseSRV),
        tc("Change Port", srv("@", 52, 62, 72, "foo.com."), srv("@", 15, 65, 75, "foo4.com.")).IfHasCapability(providers.CanUseSRV),
 
+       //CAA
+       tc("Empty").IfHasCapability(providers.CanUseSRV),
+       tc("CAA record", caa("@", "issue", "letsencrypt.org")).IfHasCapability(providers.CanUseCAA),
+       tc("CAA change tag", caa("@", "issuewild", "letsencrypt.org"), 0).fHasCapability(providers.CanUseCAA),
+       tc("CAA change target", caa("@", "issuewild", "example.com"), 0).fHasCapability(providers.CanUseCAA),
+       tc("CAA change flag", caa("@", "issuewild", "example.com", 1)).fHasCapability(providers.CanUseCAA),
+       tc("CAA many records", caa("@", "issue", "letsencrypt.org"), caa("@", "issuewild", ";"), caa("@", "iodef", "mailto:test@example.com", CAA_CRITICAL)).IfHasCapability(providers.CanUseCAA),
+       tc("CAA delete", caa("@", "issue", "letsencrypt.org")).IfHasCapability(providers.CanUseCAA),
+
        //TODO: in validation, check that everything is given in unicode. This case hurts too much.
        //tc("IDN pre-punycoded", cname("xn--o-0gab", "xn--o-0gab.xn--o-0gab.")),
 }
Show outdated Hide outdated models/dns.go
Show outdated Hide outdated docs/_functions/domain/CAA.md
Show outdated Hide outdated docs/_functions/domain/CAA.md
Show outdated Hide outdated pkg/js/helpers.js
Show outdated Hide outdated docs/_functions/domain/CAA.md
@whs

This comment has been minimized.

Show comment
Hide comment
@whs

whs Jul 24, 2017

Contributor

Thanks for your suggestions, I'll try to get this done by tomorrow.

Contributor

whs commented Jul 24, 2017

Thanks for your suggestions, I'll try to get this done by tomorrow.

@whs

This comment has been minimized.

Show comment
Hide comment
@whs

whs Jul 25, 2017

Contributor

That should be everything. I've ran the integration test against BIND and GCLOUD.

Contributor

whs commented Jul 25, 2017

That should be everything. I've ran the integration test against BIND and GCLOUD.

@tlimoncelli

This comment has been minimized.

Show comment
Hide comment
@tlimoncelli

tlimoncelli Jul 25, 2017

Contributor

I modified the PR and merged it in #161
(I think that if I knew git better I could have done this in one branch but I have a lot to learn about git).

Contributor

tlimoncelli commented Jul 25, 2017

I modified the PR and merged it in #161
(I think that if I knew git better I could have done this in one branch but I have a lot to learn about git).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment