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

add optional 'ca' tls directive, closes #1689 #1699

Merged
merged 2 commits into from Jun 24, 2017

Conversation

4 participants
@zikes
Contributor

zikes commented Jun 1, 2017

1. What does this change do, exactly?

This change adds an optional ca option to the tls directive, which lets you specify an ACME endpoint within the Caddyfile.

2. Please link to the relevant issues.

#1689

3. Which documentation changes (if any) need to be made because of this PR?

The new option should be added at https://caddyserver.com/docs/tls

4. Checklist

  • I have written tests and verified that they fail without my change
  • I have squashed any insignificant commits
  • This change has comments for package types, values, functions, and non-obvious lines of code
  • I am willing to help maintain this change if there are issues with it later
@CLAassistant

This comment has been minimized.

CLAassistant commented Jun 1, 2017

CLA assistant check
All committers have signed the CLA.

@elcore

Well done 👍

@@ -277,6 +277,25 @@ func TestSetupParseWithClientAuth(t *testing.T) {
}
}

func TestSetupParseWithCAUrl(t *testing.T) {
testUrl := "https://acme-staging.api.letsencrypt.org/directory"

This comment has been minimized.

@elcore

elcore Jun 1, 2017

Collaborator

Rename testUrl to testURL

This comment has been minimized.

@zikes

zikes Jun 1, 2017

Contributor

@elcore Updated and squashed

@zikes zikes force-pushed the zikes:master branch from 8f6ab9d to de6008a Jun 1, 2017

@elcore

elcore approved these changes Jun 1, 2017

@mholt

This comment has been minimized.

Owner

mholt commented Jun 1, 2017

Welcome @zikes! Thanks for your contribution. I'll give this a second review before long. :)

@mholt

mholt requested changes Jun 3, 2017 edited

Just one little change needed, that I can see!

@@ -66,6 +66,12 @@ func setupTLS(c *caddy.Controller) error {
for c.NextBlock() {
hadBlock = true
switch c.Val() {
case "ca":
arg := c.RemainingArgs()

This comment has been minimized.

@mholt

mholt Jun 3, 2017

Owner

We should use only the first argument; if there are more or less than 1 argument, it should be an error.

This comment has been minimized.

@elcore

elcore Jun 3, 2017

Collaborator

Sounds good to me :)

This comment has been minimized.

@zikes

zikes Jun 6, 2017

Contributor

I've replaced the check with len(arg) != 1 similar to how dns and storage check their args, and added test cases for too many/too few args.

@zikes zikes force-pushed the zikes:master branch from de6008a to 5d2089b Jun 6, 2017

@zikes zikes force-pushed the zikes:master branch from a815bf1 to 5d2089b Jun 17, 2017

@mholt mholt modified the milestone: 0.10.4 Jun 24, 2017

@mholt

mholt approved these changes Jun 24, 2017

@mholt mholt merged commit f3721c1 into mholt:master Jun 24, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@mholt

This comment has been minimized.

Owner

mholt commented Jun 24, 2017

Thank you @zikes! If you have a chance, submit a PR to https://github.com/caddyserver/website to add the docs for the new ca option. :)

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