Add implementation of CertFetcher abstraction on top of Lego/ACME library. #345

banaag · 2019-09-11T19:31:56Z

No description provided.

… modify the current flow to inject an instance of CertFetcher into CertCache to take care of the scheduled cert renewals.

googlebot · 2019-09-11T19:32:02Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

banaag · 2019-09-11T19:36:48Z

@googlebot

googlebot · 2019-09-11T19:36:51Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

twifkak

Nice progress! Haven't reviewed tests yet, but let me know if there's anything in particular there you want me to notice. :)

twifkak · 2019-09-11T19:42:43Z

packager/certfetcher/certfetcher.go

+	"strconv"
+
+	"github.com/WICG/webpackage/go/signedexchange"
+	"github.com/go-acme/lego/v3/certcrypto"


Please run dep ensure and add the vendor/ files to this PR. (https://golang.github.io/dep/)

Will do this in next commit.

packager/certfetcher/certfetcher.go

twifkak · 2019-09-11T20:36:53Z

packager/certfetcher/certfetcher.go

+		return nil, err
+	}
+
+	// We specify an http port of `acmeChallengePort`


OK for this PR. I think for some future PR it'd be preferable if we can host the challenge URLs on the same port as the rest of amppkg. (Agree/disagree?)

Naive ideas:

https://golang.org/pkg/net/http/httputil/#ReverseProxy

implement a custom challenge.Provider (https://github.com/go-acme/lego/blob/a5a29187fea9f555ea43fae16ca65066c452d330/challenge/http01/http_challenge_server.go#L25)

We'd also talked about, instead of the reverse-proxy approach, adding a new config field with a path to a directory where we'd dump the challenge responses, to be served by a normal web server (e.g. /var/www/html/.well-known/acme-challenge). Especially useful if having to coexist with certbot. If that's still your thinking, then I guess the custom ProviderServer seems like the way to go?

My 2c based on what I know so far is that we should lean towards the reverse proxy if we're picking exactly one.

The idea here is that this may be running on some remote server in a Google Cloud, Amazon, etc installation where shared filesystem may be difficult. The publisher already needs to set up reverse proxy rules for the Packager anyway, so adding one new rule should be fairly easy in any configuration.

Good point, and I think agreed. If most people are using some CDN for their TLS termination, then they're probably using ACME DNS-01 challenge type, which won't interfere. This is probably the more common use-case.

If people are going full DIY, running their own Apache/nginx with certbot to manage their own TLS certs, then they're using HTTP-01 challenge type (certbot doc), so it would have to share the /.well-known/acme-challenge/ URL path prefix. I'm not sure if there's a way to tell Apache/nginx to say "rev-proxy only if there isn't a file on disk".

Sorry, twifkak@, forgot to give you a heads up about this and my decision to do the proxy method after discussions with gregable@. At this point, let's go the direction of the reverse proxy method and address the co-mingling of certbot/DIY method with the reverse proxy at a later time. It's possible that DevRel type folks may have encountered this in the field and have suitable suggestions.

Sounds good. Still would lean towards it running on the same port (future PR OK), unless you see a problem with that.

twifkak · 2019-09-11T22:44:03Z

packager/certfetcher/certfetcher.go

+	if !shouldRegister {
+		myUser.Registration = new(registration.Resource)
+	} else {
+		reg, err := client.Registration.Register(registration.RegisterOptions{TermsOfServiceAgreed: true})


Add a TODO to make sure we present the TOS URL to the user and prompt for confirmation. (The plan is to move this to some separate setup command outside the server?)

Gregable · 2019-09-11T23:11:01Z

packager/certfetcher/certfetcher.go

+		return nil, err
+	}
+
+	// We specify an http port of `acmeChallengePort`


My 2c based on what I know so far is that we should lean towards the reverse proxy if we're picking exactly one.

The idea here is that this may be running on some remote server in a Google Cloud, Amazon, etc installation where shared filesystem may be difficult. The publisher already needs to set up reverse proxy rules for the Packager anyway, so adding one new rule should be fairly easy in any configuration.

Gregable · 2019-09-11T23:12:44Z

packager/certfetcher/certfetcher_test.go

+	jose "gopkg.in/square/go-jose.v2"
+)
+
+const CertResponseMock = `-----BEGIN CERTIFICATE-----


Maybe document what you did to generate these.

twifkak

Thanks for the nit fixes. LGTM once Travis is happy.

twifkak · 2019-09-12T20:45:03Z

packager/certfetcher/certfetcher.go

+		return nil, err
+	}
+
+	// We specify an http port of `acmeChallengePort`


Sounds good. Still would lean towards it running on the same port (future PR OK), unless you see a problem with that.

twifkak · 2019-09-12T21:11:10Z

packager/certfetcher/certfetcher_test.go

+	_, apiURL, tearDown := tester.SetupFakeAPI()
+	defer tearDown()
+
+	privateKey, err := rsa.GenerateKey(rand.Reader, 2048)


Should this be ec.GenerateKey or something?

twifkak · 2019-09-12T21:12:50Z

packager/certfetcher/certfetcher_test.go

+`
+
+func TestNewFetcher(t *testing.T) {
+	_, apiURL, tearDown := tester.SetupFakeAPI()


Instead of doing this, you may be interested in https://godoc.org/github.com/stretchr/testify/suite which supports SetupTest() and TearDownTest() methods.

(Also, assert.X(t, ...) becomes this.Assert().X(...) which is neither better nor worse.)

twifkak · 2019-09-12T21:16:28Z

packager/certfetcher/certfetcher_test.go

+
+	cert, err := fetcher.FetchNewCert()
+	assert.NotNil(t, err)
+	assert.Nil(t, cert)


Maybe also assert the cert subject or something just to make sure it's been populated.

twifkak · 2019-09-12T21:18:15Z

packager/certfetcher/certfetcher_test.go

+	assert.Nil(t, cert)
+}
+
+func setupMux(mux *http.ServeMux, apiURL string, privateKey *rsa.PrivateKey) {


Add a quick comment to say what this does, e.g. "Builds a fake ACME server that supports /newOrder and /finalize; tests should add a custom implementation of /certificate."

banaag · 2019-09-13T15:23:13Z

Thanks for the approvals. Per an offline conversation with twifkak@, we will need to move the amppackager project from dep to go modules (issue #270) in order to resolve the travis failures for this PR.

twifkak

LICENSE files LGTM for new vendored deps, and vendored deps outside golang.org/x seem fairly minimal, and changes for existing vendored deps seem harmless.

On of these days, I would love to understand the difference between the lists of deps in go.mod, go.sum, vendor, and vendor/modules.txt. All 4 are different... for good reasons I hope.

CertFetcher abstraction on top of LEGO/ACME library. The intent is to…

3d0a93a

… modify the current flow to inject an instance of CertFetcher into CertCache to take care of the scheduled cert renewals.

banaag requested review from twifkak and Gregable September 11, 2019 19:31

googlebot added the cla: no label Sep 11, 2019

banaag changed the base branch from releases to master September 11, 2019 19:34

banaag removed the cla: no label Sep 11, 2019

googlebot added the cla: yes label Sep 11, 2019

twifkak requested changes Sep 11, 2019

View reviewed changes

Gregable reviewed Sep 11, 2019

View reviewed changes

Fixed twifkak/gregable commentsi except for dep ensure

566f764

banaag requested review from twifkak and Gregable September 12, 2019 20:14

Switched shortn to full URL

fbabbbc

twifkak approved these changes Sep 12, 2019

View reviewed changes

Gregable approved these changes Sep 13, 2019

View reviewed changes

banaag added 2 commits September 16, 2019 15:07

Merge remote-tracking branch 'upstream/master' into packager-certcache

5a07fdc

Update go modules deps, go mod tidy and go mod vendor.

ac103fc

banaag requested a review from twifkak September 16, 2019 22:14

twifkak approved these changes Sep 16, 2019

View reviewed changes

banaag merged commit f28e7ed into ampproject:master Sep 16, 2019

banaag added the acme All ACME related PRs label Feb 24, 2020

banaag deleted the packager-certcache branch March 5, 2020 22:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add implementation of CertFetcher abstraction on top of Lego/ACME library. #345

Add implementation of CertFetcher abstraction on top of Lego/ACME library. #345

banaag commented Sep 11, 2019

googlebot commented Sep 11, 2019

banaag commented Sep 11, 2019

googlebot commented Sep 11, 2019

twifkak left a comment

twifkak Sep 11, 2019

banaag Sep 12, 2019

twifkak Sep 11, 2019

Gregable Sep 11, 2019

twifkak Sep 12, 2019

banaag Sep 12, 2019

twifkak Sep 12, 2019

twifkak Sep 11, 2019

banaag Sep 12, 2019

Gregable Sep 11, 2019

Gregable Sep 11, 2019

banaag Sep 12, 2019

twifkak left a comment

twifkak Sep 12, 2019

twifkak Sep 12, 2019

twifkak Sep 12, 2019

twifkak Sep 12, 2019

twifkak Sep 12, 2019

banaag commented Sep 13, 2019

twifkak left a comment

Add implementation of CertFetcher abstraction on top of Lego/ACME library. #345

Add implementation of CertFetcher abstraction on top of Lego/ACME library. #345

Conversation

banaag commented Sep 11, 2019

googlebot commented Sep 11, 2019

banaag commented Sep 11, 2019

googlebot commented Sep 11, 2019

twifkak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twifkak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banaag commented Sep 13, 2019

twifkak left a comment

Choose a reason for hiding this comment