Skip to content
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

Updating amppackage.example.toml documentation. #361

Merged
merged 4 commits into from
Dec 5, 2019
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 90 additions & 0 deletions amppkg.example.toml
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,24 @@
# SHA-256).
CertFile = './pems/cert.pem'

# The path to save a new cert retrieved from the CA if the current cert in
# 'CertFile' above is still valid.
# This is optional and is needed only if you have 'autorenewcert' # turned on.
Copy link
Member

Choose a reason for hiding this comment

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

Stray '#'?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this optional? My build complains if it's missing, and it looks like the code checks for it too:

if config.CertFile == "" || config.NewCertFile == "" {
.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in #372

Copy link
Member

Choose a reason for hiding this comment

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

Clarify in the docs: For multi-replica setups, does this need to be set on the non-autorenewcert replicas?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

# NewCertFile = './pems/newcert.pem'

# The path to the Certificate Signing Request (CSR) that is needed to request
# new certificates from the Cert Authority using ACME.
# CSRs are typically created using the openssl command:
# openssl req -new -key /path/to/privkey -out /path/to/cert.csr
# To verify:
# openssl req -text -noout -verify -in cert.csr
# The following docs list examples on how to go about generating CSRs:
# https://www.digicert.com/csr-creation.htm?rid=011592
# https://www.ssl.com/how-to/manually-generate-a-certificate-signing-request-csr-using-openssl/
# https://geekflare.com/san-ssl-certificate/
# This is optional and is needed only if you have 'autorenewcert' # turned on.
Copy link
Member

Choose a reason for hiding this comment

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

Stray '#'?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed.

# CSRFile = './pems/cert.csr'

# The path to the PEM file containing the private key that corresponds to the
# leaf certificate in CertFile.
KeyFile = './pems/privkey.pem'
Expand Down Expand Up @@ -170,3 +188,75 @@ ForwardedRequestHeaders = []
# Domain = "www.corp.amppackageexample.com"
# PathRE = "/world/.*"
# QueryRE = ""

# ACME is a protocol that allows for automatic renewal of certificates. Amp packager uses an ACME library
Copy link
Member

Choose a reason for hiding this comment

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

s/Amp/AMP/ (ditto lines 229 and 237) and capitalize packager as Packager whenever it follows "AMP" (ditto below)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

For now, please open with some sort of "experimental" disclaimer until we've had more experience with it in the wild. amppkg README.md had such a thing at the top for a while.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

# https://github.com/go-acme/lego to handle certificate renewal.
Copy link
Member

Choose a reason for hiding this comment

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

I would move details about the spec and the Go library to the bottom of this prologue comment. Focus on what it does, how to enable it, and any gotchas.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed, let me know if the changes are satisfactory.

#
# For the full ACME spec, see:
# https://tools.ietf.org/html/draft-ietf-acme-acme-02
# https://ietf-wg-acme.github.io/acme/draft-ietf-acme-acme.html
#
# ACMEConfig only needs to be present in the toml file if 'autorenewcert' command line flag was turned on.
# If the flag is on, at least one of ACMEConfig.Production or ACMEConfig.Development should be present.
# Note that a recommended best practice for setting up the cert renewal that minimizes both cost and bombarding
# your Certificate Authority with requests is that for a multi-instance setup of AMP packager, only one instance is
# setup to do automatic cert renewals and the rest of the instances will just be configured to reload the fresh
# certificate from disk when their in-memory copies expire. This also implies that the cert paths configured above
# in 'CertFile' and 'NewCertFile' are located on a shared filesystem accessible by all AMP packager instances.
# [ACMEConfig]
Copy link
Member

Choose a reason for hiding this comment

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

Smallest of nits, but I'm wondering if we should just rename this (in a future PR) from [ACMEConfig] to [ACME]. "Config" seems redundant inside of a config file. WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added TODO

# [ACMEConfig.Production]
# This config will be used if 'autorenewcert' is turned on and 'development' is turned off.
Copy link
Member

Choose a reason for hiding this comment

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

Move this comment above [ACMEConfig.Production] to match the style in this file.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

# This is the ACME discovery URL that is used for ACME http requests to the Certificate authority that
Copy link
Member

Choose a reason for hiding this comment

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

Pick a consistent capitalization throughout, Certificate Authority or certificate authority.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

# doles out the certificates.
# Currently, the only CA that supports automatic signed exchange cert renewals is Digicert:
# https://docs.digicert.com/certificate-tools/acme-user-guide/acme-directory-urls-signed-http-exchange-certificates/
#
# DiscoURL = "production-acme.discovery.url"
Copy link
Member

Choose a reason for hiding this comment

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

Make the example look like a URL, e.g. https://production-acme.discovery.url/

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

#
# This is the email address you used to create an account with the Certificate Authority that is registered to
# request signed exchange certificates.
#
# EmailAddress = "[email protected]"
#
Copy link
Member

Choose a reason for hiding this comment

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

For the blank lines between fields, I'd omit the '#' so there's a clear visual separation. Otherwise, it's unclear whether the text is talking about the preceding or following field. (Ditto below.)

(Looks like you may have been following the example of URLSet.Fetch, which "solves" this with a double-comment prefix.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

# For the remaining configuration items, it's important to understand the different challenges employed as
# part of the ACME protocol. See:
# https://ietf-wg-acme.github.io/acme/draft-ietf-acme-acme.html#identifier-validation-challenges
# https://letsencrypt.org/docs/challenge-types/
Copy link
Member

Choose a reason for hiding this comment

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

Fix indent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

# https://certbot.eff.org/docs/challenges.html?highlight=http
# Note that you don't need to have all the challenges configured, it's typically sufficient to have one configured.
# The exception arises when you have to deal with wildcard certificates, see below.
#
# This is the port used by the Amp packager to respond to the HTTP challenge issued as part of ACME protocol.
# Note that if your setup only opens up certain ports, you may need to do a configuration change where you forward
# requests to this port using proxy_pass, for example:
# https://medium.com/@dipeshwagle/add-https-using-lets-encrypt-to-nginx-configured-as-a-reverse-proxy-on-ubuntu-b4455a729176
#
# HttpChallengePort = 5002
#
# This is the http server root directory where the ACME http challenge token could be deposited. Note that you may
# need to do some configuration work to get this setup to work where multiple instances of Amp packager is running.
# For example:
# https://community.letsencrypt.org/t/how-to-nginx-configuration-to-enable-acme-challenge-support-on-all-http-virtual-hosts/5622/3
#
# HttpWebRootDir = '/path/to/www_root_dir'
Copy link
Member

Choose a reason for hiding this comment

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

I have a slight preference to list this one before the HttpChallengePort, because it's less risky since it doesn't involve a direct path to the amppkg binary. But I likely don't have the full picture... WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reordered.

#
# This is the port used by AMP packager to respond to the TLS challenge issued as part of the ACME protocol.
#
# TlsChallengePort = 5003
#
# This is the DnsProvider to be used in fulfilling the ACME DNS challenge. Note that you only need the DNS challenge
# setup if you have wildcard certificates. See: https://searchsecurity.techtarget.com/definition/wildcard-certificate
# For the DNS challenge, go-acme/lego, there are certain environment variables that need to be set up which depends on
# the DNS provider that you use to fulfill the DNS challenge. See:
# https://go-acme.github.io/lego/dns/
# DnsProvider = "gcloud"
# [ACMEConfig.Development]
# This config will ne used if 'autorenewcert' is turned on and 'development' is turned on.
Copy link
Member

Choose a reason for hiding this comment

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

What happens if -autorenewcert -development is turned on, but no [ACMEConfig.Development] is specified here? The above text says this is okay, but ISTR amppkg will refuse to start in this case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The text just says this config will be used if those flags are on. I added more verbage that says if the flags are on and the config is missing, AMP Packager will fail to start.

# All the other fields below have the same semantics as the one in ACMEConfig.Production above.
Copy link
Member

Choose a reason for hiding this comment

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

Ditto, move this comment above [ACMEConfig.Development].

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

I would add a suggestion of Let's Encrypt in development, since it's free and development-mode doesn't require the SXG extension. (This lines up with how Matt suggested use of development-mode in the talk.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

# DiscoURL = "development-acme.discovery.url"
Copy link
Member

Choose a reason for hiding this comment

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

Ditto make it look like a URL.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

# EmailAddress = "[email protected]"
# HttpChallengePort = 5002
# HttpWebRootDir = '/path/to/www_root_dir'
# TlsChallengePort = 5003
# DnsProvider = "gcloud"