Updating the toml to include ACME related entries. #49

banaag · 2020-07-07T01:09:05Z

No description provided.

banaag · 2020-07-07T01:10:00Z

yuizumi · 2020-07-08T03:36:48Z

server/tomlconfig/config.go

@@ -29,6 +29,7 @@ type Config struct {
 	Listen    ListenConfig
 	Server    ServerConfig
 	SXG       SXGConfig
+	ACME      SXGACMEConfig


If you want it to be [SXG.ACME], you should define it in SXGConfig.

yuizumi · 2020-07-08T03:39:36Z

cmd/webpkgserver/webpkgserver.example.toml

+# ACME is a protocol that allows for automatic renewal of certificates.
+# Web Packager HTTP server uses an ACME library https://github.com/go-acme/lego
+# to handle certificate renewal. Automatic certificate renewal is enabled
+# in Web Packager HTTP server via the 'autorenewcert' flag. Turning the flag on


I'm against having a command line flag for this purpose. That way the config gets scattered into two places, namely the toml config and the startup command.

Consider, for example, adding an Enable parameter. Another approach is to enable ACME when one (or more) of the challenges are set correctly.

Added an explicit field in the TOML to indicate enabling cert auto renewal. I don't like the implicit setting of a parameter based on other parameters, I think that is not the right path.

Please update this paragraph too.

yuizumi · 2020-07-08T03:42:49Z

cmd/webpkgserver/webpkgserver.example.toml

+# Web Packager HTTP server uses an ACME library https://github.com/go-acme/lego
+# to handle certificate renewal. Automatic certificate renewal is enabled
+# in Web Packager HTTP server via the 'autorenewcert' flag. Turning the flag on
+# will enable AMP Packager to automatically request certificate renewals


This is not AMP Packager.

Updated all instances to Web Packager.

yuizumi · 2020-07-08T03:44:03Z

cmd/webpkgserver/webpkgserver.example.toml

+# whenever it has determined that the current certificate is expired or about to
+# expire.
+#
+# SXG.ACME only needs to be present in the toml file if 'autorenewcert' command


Will it be true, especially given you're working to add support for multi-cert cache (#48)?

Yes, this will still be true. I think we can set the default to SingleCertDiskCache is they don't want to autorenew certs.

Since we are going to add support for multi-cert cache either way, I still don't think multi-instance setup is the "best practice."

So multi-cert cache and multi-instance of webpackager solve different problems. The first solves keeping unexpired certs for the webpackager instance. The second assigns only one instance for certificate renewal (and other webpackager instances will be consumers). This way, if there are multiple instances of the webpackager running, not all of them will be requesting certs from ACME server.

yuizumi · 2020-07-08T03:49:46Z

server/tomlconfig/verify.go

@@ -132,6 +132,28 @@ func (c *SXGCertConfig) verify() error {
 	return errs.ErrorOrNil()
 }

+func (c *SXGACMEConfig) verify() error {


This method doesn't look to be called.

yuizumi · 2020-07-08T03:50:36Z

server/tomlconfig/config.go

@@ -72,6 +73,42 @@ type SXGCertConfig struct {
 	AllowTestCert bool
 }

+// SXGACMEConfig represents the [SXG.ACME] section.
+type SXGACMEConfig struct {
+	// DiscoveryURL is the Discovery Resource URL provided by the Certificate


Consider removing GoDoc for these fields. In general we should have a GoDoc for each exposed field, but in this particular case we want to avoid duplicate with the example toml. It's also confusing these comments state "See Port Usage above" but this go module doesn't have the "Port Usage" section.

The GoDoc for Config has a mention to the example toml.

yuizumi · 2020-07-08T03:54:25Z

server/tomlconfig/verify.go

+	if c.Email == "" {
+		errs = multierror.Append(errs, wrapError("Email", errEmpty))
+	}
+	if c.HTTPWebRootDir == "" {


Does the user need to specify all these three parameters?

No, good catch. Fixed the logic.

yuizumi · 2020-07-08T03:57:46Z

cmd/webpkgserver/webpkgserver.example.toml

+
+  # This is the email address you used to create an account with the Certificate
+  # Authority that is registered to request signed exchange certificates.
+  Email = "user@company.com"


@example.com is a much safer choice here. company.com is a domain in real use.

yuizumi · 2020-07-08T03:58:23Z

cmd/webpkgserver/webpkgserver.example.toml

+  # Certificate Authority that 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/
+  DiscoveryURL = "https://production-acme.discovery.url/"


Consistency: Use single quotes for plain strings.

yuizumi · 2020-07-08T04:02:16Z

cmd/webpkgserver/webpkgserver.example.toml

+  # fulfill the DNS challenge: https://go-acme.github.io/lego/dns/
+  # To use this, build amppkg with `go build -tags dns01`; it is disabled by
+  # default because it bloats the binary.
+  #DnsProvider = "gcloud"


This config file states at the beginning: "Commented out lines show the default value for each parameter." It doesn't seem you've set the default value in the code though.

Fixed, DNSProvider should default to empty string.

yuizumi · 2020-08-04T14:28:44Z

cmd/webpkgserver/webpkgserver.example.toml

@@ -211,7 +290,7 @@

  # The domain to limit signed URLs to, case-insensitive. The certificate is
  # supposed to cover this domain. Required.
-  Domain = 'example.org'
+  Domain = 'example.com'


Any reason to change the domain from org to com?

Reverted back to org

yuizumi · 2020-08-04T14:32:18Z

server/tomlconfig/verify.go

+
+	if c.HTTPWebRootDir == "" && c.HTTPChallengePort == 0 &&
+		c.TLSChallengePort == 0 && c.DNSProvider == "" {
+		errs = multierror.Append(errs, wrapError("at least one of HTTPWebRootDir, HTTPChallengePort, TLSChallengePort or DNSProvider has to be non-empty", errEmpty))


Should there be exactly one of these config parameters, or is it meaningful to set more than one parameter?

The first argument of wrapError is meant to be the parameter names, so it should be something like one of {HTTP..., HTTP..., TLS..., DNS...}.

It isn't required to have exactly one parameter. They can all be set and it's up to the library to decide which method to use and to use the other settings as backup.

The first argument of wrapError is meant to be the parameter names, so it should be something like one of {HTTP..., HTTP..., TLS..., DNS...}.

So I'm not sure how this would work when we are generating the error because none of these parameters have values. Do I pass in all the parameter names with empty values to wrapError, is that what you mean?

I think you get it mostly right, I meant the first argument of wrapError should look like a parameter name (a list of parameter names in this case) rather than a sentence. With another thought, I suggest:

newError( "HTTPWebRootDir, HTTPChallengePort, TLSChallengePort, DNSProvider", "at least one of these parameters must be non-empty", )

Technically the parameter name(s) are just printed as part of error messages (Code).

It isn't required to have exactly one parameter. They can all be set and it's up to the library to decide which method to use and to use the other settings as backup.

This makes sense, but it's worth mentioned in the example toml.

yuizumi · 2020-08-04T14:37:50Z

cmd/webpkgserver/webpkgserver.example.toml

+# IMPORTANT NOTE: the support of the ACME protocol and automatic renewal of
+# certificates is currently in the EXPERIMENTAL stage.  Once we have more
+# experience with people using it out in the wild, we will gradually move it to
+# PRODUCTION mode.


"... to the PRODUCTION stage"?

s/to/to the/, s/mode/stage/.

yuizumi · 2020-08-04T14:40:26Z

cmd/webpkgserver/webpkgserver.example.toml

+# experience with people using it out in the wild, we will gradually move it to
+# PRODUCTION mode.
+#
+# ACME is a protocol that allows for automatic renewal of certificates.


s/allows for/allows/ ― "for" is not needed when "allow" is used to mean "enable."

yuizumi · 2020-08-04T14:41:26Z

cmd/webpkgserver/webpkgserver.example.toml

+# PRODUCTION mode.
+#
+# ACME is a protocol that allows for automatic renewal of certificates.
+# Web Packager HTTP server uses an ACME library https://github.com/go-acme/lego


"... uses the ACME library from github.com/go-acme/lego"

s/a/the/, add "from", (optionally) remove "https://".

yuizumi · 2020-08-04T17:43:36Z

cmd/webpkgserver/webpkgserver.example.toml

+  # https://medium.com/@dipeshwagle/add-https-using-lets-encrypt-to-nginx-configured-as-a-reverse-proxy-on-ubuntu-b4455a729176
+  HTTPChallengePort = 5002
+
+  # This is the port used by Web packager to respond to the TLS challenge issued


s/packager/Packager/

yuizumi · 2020-08-04T17:44:44Z

cmd/webpkgserver/webpkgserver.example.toml

+  # 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: https://go-acme.github.io/lego/dns/
+  # To use this, build amppkg with `go build -tags dns01`; it is disabled by


This config is not for amppkg.

yuizumi · 2020-08-04T17:46:32Z

cmd/webpkgserver/webpkgserver.example.toml

+  # This is the ACME discovery URL that is used for ACME http requests to the
+  # Certificate Authority that 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/


Maybe mention "Required when ACME is enabled." (see: Cert.PEMFile)

I don't see "Required when ACME is enabled." or anything similar, even in the latest commit. I guess you pushed a wrong thing?

That's weird, not sure what happened. Fixed now.

Sorry but I can't tell what is updated...

I think what happened is that you suggested to add "Required when ACME is enabled" then you said I should not add it to the "Enable = false" field, so I removed it. It was added to the other fields below though.

Oops, sorry, I made this comment at a wrong place.

yuizumi · 2020-08-04T17:52:53Z

cmd/webpkgserver/webpkgserver.example.toml

+  # Certificate Authority that 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/
+  DiscoveryURL = 'https://production-acme.discovery.url/'


Use some invalid value? So that verify would raise an error when the user forgets to set this parameter properly (rather than waiting for the server failing the first attempt of ACME request).

e.g. DiscoveryURL = '<Your Discovery URL>'

production-acme is an invalid URL but I get your point. Replaced.

Same: I don't see the url replaced.

yuizumi · 2020-08-04T17:56:16Z

cmd/webpkgserver/webpkgserver.example.toml

+  # different challenges employed as part of the ACME protocol:
+  # https://ietf-wg-acme.github.io/acme/draft-ietf-acme-acme.html#identifier-validation-challenges
+  # https://letsencrypt.org/docs/challenge-types/
+  # https://certbot.eff.org/docs/challenges.html?highlight=http


Remove ?hightlight=http? I don't see good reason to highlight "http" in the document.

yuizumi

I was about to send some comments but made a mistake to get them lost. I'll do the review again next week. Sorry for inconvenience.

yuizumi · 2020-08-17T13:47:48Z

cmd/webpkgserver/webpkgserver.example.toml

+[SXG.ACME]
+  # This field enables Web Packager to attempt to auto renew certificates using
+  # ACME. Required when ACME is enabled.
+  #Enable=false


Spaces around the equal sign.

Done. Also set the value to be true because it's already false by default.

yuizumi · 2020-08-17T13:50:34Z

cmd/webpkgserver/webpkgserver.example.toml

+# For the full ACME spec, see:
+#   https://tools.ietf.org/html/rfc8555 
+[SXG.ACME]
+  # This field enables Web Packager to attempt to auto renew certificates using


s/This field enables/Enable/ ― docs for bool params start with a verb phrase in this config.

yuizumi · 2020-08-17T13:51:35Z

cmd/webpkgserver/webpkgserver.example.toml

+  # DNSProvider is a supported method of validation, the others cannot be used. 
+  # See below.
+
+  # This is the http server root directory where the ACME http challenge token


Remove "This is" ― docs for non-bool params start with a noun phrase. Same below.

yuizumi · 2020-08-17T14:40:21Z

cmd/webpkgserver/webpkgserver.example.toml

+# expire.
+#
+# Note that a recommended best practice for setting up the cert renewal that
+# minimizes both cost and bombarding your Certificate Authority with requests is


Which "cost" does this approach minimize?

Added explanation.

yuizumi · 2020-08-17T15:59:28Z

cmd/webpkgserver/webpkgserver.example.toml

+# whenever it has determined that the current certificate is expired or about to
+# expire.
+#
+# Note that a recommended best practice for setting up the cert renewal that


Now I understand your standpoint. I found this paragraph hard for me to read correctly, to be honest, though. It's probably because of my limited English ability, but I'd like documentation to be easy to understand even for those less fluent in English than me.

The paragraph would become easier to read ("scan" or "skim" in particular) if it'd be written in the order of importance, starting from the most important part. In this case:

The most important part is assumption ("setting up multiple instances"), because the rest of this paragraph is irrelevant if the user is setting up just a single instance.

The recommended practice ("only one instance does cert renewals") comes next.

The reason ("minimizes both cost and bombarding CA") is still very important, as it helps with deeper understanding, but what comes before why.

The implication ("the certificate on a shared filesystem") can be a subtle trap thus should be mentioned clearly, but it is relatively obvious to experienced admins so can come to the last.

Here's my attempt: "If you set up more than one instance of webpkgserver, the recommended practice is to have only one instance handle automatic certificate renewals and let other instances reload the fresh certificate from the disk. It minimizes the cost [of what?] and prevents the webpkgserver instances from bombarding your Certificate Authority with certificate requests. Note this setup requires you to locate the certificate file on a shared filesystem accessible by all webpkgserver instances."

Merged your description with my changes above.

yuizumi · 2020-08-17T16:13:58Z

cmd/webpkgserver/webpkgserver.example.toml

+# experience with people using it out in the wild, we will gradually move it to
+# the PRODUCTION stage.
+#
+# ACME is a protocol that allows automatic renewal of certificates.


The paragraph should start with a sentence to indicate "this section is meant for ACME config," then explain ACME protocol. Otherwise the reader might get confused as they see the ACME protocol explained suddenly.

yuizumi · 2020-08-17T16:21:53Z

cmd/webpkgserver/webpkgserver.example.toml

+  # 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: https://go-acme.github.io/lego/dns/
+  # To use this, build webpkgserver with `go build -tags dns01`; it is disabled


This is not obvious to anyone new to webpkgserver, so I suggest putting it right after the lead sentence. Then "Note that you only need the DNS challenge setup" makes more sense to readers.

Rearranged.

yuizumi · 2020-08-17T16:24:07Z

server/tomlconfig/verify.go

+		return errs.ErrorOrNil()
+	}
+
+	if c.DiscoveryURL == "" {


Is it okay to accept a relative url (an absolute path) though?

yuizumi · 2020-08-17T16:28:43Z

server/tomlconfig/verify.go

+
+	if c.HTTPWebRootDir == "" && c.HTTPChallengePort == 0 &&
+		c.TLSChallengePort == 0 && c.DNSProvider == "" {
+		errs = multierror.Append(errs, wrapError("at least one of HTTPWebRootDir, HTTPChallengePort, TLSChallengePort or DNSProvider has to be non-empty", errEmpty))


I think you get it mostly right, I meant the first argument of wrapError should look like a parameter name (a list of parameter names in this case) rather than a sentence. With another thought, I suggest:

newError( "HTTPWebRootDir, HTTPChallengePort, TLSChallengePort, DNSProvider", "at least one of these parameters must be non-empty", )

Technically the parameter name(s) are just printed as part of error messages (Code).

It isn't required to have exactly one parameter. They can all be set and it's up to the library to decide which method to use and to use the other settings as backup.

This makes sense, but it's worth mentioned in the example toml.

yuizumi · 2020-08-19T13:45:07Z

server/tomlconfig/verify.go

+		return errs.ErrorOrNil()
+	}
+
+	if c.DiscoveryURL == "" {


Let me rephrase: I think we should reject relative urls thus verifyURL isn't good enough.

yuizumi · 2020-08-19T14:19:45Z

cmd/webpkgserver/webpkgserver.example.toml

+# If you set up more than one instance of webpkgserver, the recommended practice
+# is to have only one instance handle automatic certificate renewals and let
+# other instances reload the fresh certificate from the disk. It minimizes the
+# cost (SXG certificates cost money and we don't need each webpackager instance


Thanks for adding the explanation. It needs extra caution though. I presume it's not the case with DigiCert, but some providers allow purchased certificates to be installed only on a single (physical) machine.

The purchased certificates will still be in a single physical drive, it's just mounted on the machines that needs the certs. I can mention that.

I should have been a bit more clear, but "Some CAs allow certificates to be installed only on one machine" is just one example [*1]. Some other CAs may, for example, disallow certificates to be served from more than one machine.

Giving second thoughts, "minimize cost" is not really the point here. It depends on how CAs charge their clients. I guess that maybe DigiCert is charging for each certificate issued, but it's also possible to charge on a per-domain-per-year basis instead.

The real point is that this setup allows certificate sharing. "It allows certificates to be shared on all webpkgserver instances (which may help minimize the monetary cost, subject to Terms of Service set by your Certificate Authority) and prevents webpkgserver from bombarding your Certificate Authority with certificate requests."

[*1]: It was actually the case when I purchased a relatively cheap certificate ~15 years ago.

yuizumi · 2020-08-19T14:29:10Z

cmd/webpkgserver/webpkgserver.example.toml

+# For the full ACME spec, see:
+#   https://tools.ietf.org/html/rfc8555 
+[SXG.ACME]
+  # Enable enables Web Packager to attempt to auto renew certificates using


s/Enable enables/Enable/

yuizumi · 2020-08-19T14:34:15Z

cmd/webpkgserver/webpkgserver.example.toml

+[SXG.ACME]
+  # Enable enables Web Packager to attempt to auto renew certificates using
+  # ACME. Required when ACME is enabled.
+  #Enable = true


Again, recall "Commented out lines show the default value for each parameter." at the top of this config file; the value needs to be false.

Ah yes, sorry forgot about that rule.

yuizumi · 2020-08-19T16:16:44Z

cmd/webpkgserver/webpkgserver.example.toml

+#   https://tools.ietf.org/html/rfc8555 
+[SXG.ACME]
+  # Enable enables Web Packager to attempt to auto renew certificates using
+  # ACME. Required when ACME is enabled.


I meant adding "Required when ACME is enabled" to DiscoveryURL (and maybe other parameters, such as Email).

Missed this, done.

This "Required when ACME is enabled." is not needed, possibly confusing to users. "ACME is enabled" means setting true to this param, essentially.

yuizumi · 2020-08-19T17:12:41Z

cmd/webpkgserver/webpkgserver.example.toml

+  # of the ACME protocol.
+  TLSChallengePort = 5003
+
+  # The DnsProvider to be used in fulfilling the ACME DNS challenge.


s/DnsProvider/DNS provider/

s/in/for/

yuizumi · 2020-08-19T17:14:53Z

cmd/webpkgserver/webpkgserver.example.toml

@@ -202,6 +202,89 @@
  # bytes for the OCSP response.
  #AllowTestCert = false

+# IMPORTANT NOTE: the support of the ACME protocol and automatic renewal of


Don't forget to remove TODO at Line 4.

yuizumi · 2020-08-19T17:16:20Z

cmd/webpkgserver/webpkgserver.example.toml

+# experience with people using it out in the wild, we will gradually move it to
+# the PRODUCTION stage.
+#
+# This section is meant for ACME config, which allows the Web Packager to use


s/This section is meant for ACME config/Configure ACME/

yuizumi · 2020-08-19T17:22:58Z

cmd/webpkgserver/webpkgserver.example.toml

+# experience with people using it out in the wild, we will gradually move it to
+# the PRODUCTION stage.
+#
+# This section is meant for ACME config, which allows the Web Packager to use


We have used "webpkgserver" to reference the application elsewhere in this config file, so I prefer "webpkgserver" over "the Web Packager" or "Web Packager HTTP Server". I'm not saying "webpkgserver" is better or more correct; I just believe it's less confusing to readers if we use the same term consistently.

yuizumi · 2020-08-19T17:24:53Z

cmd/webpkgserver/webpkgserver.example.toml

+# the PRODUCTION stage.
+#
+# This section is meant for ACME config, which allows the Web Packager to use
+# ACME to automatically renew Signed Exchange certificates.


"to use ACME" doesn't look to give much information (because it's implied by "This section is meant for ACME config" or "Configure ACME").

yuizumi · 2020-08-25T09:13:54Z

cmd/webpkgserver/webpkgserver.example.toml

+  # Required when ACME is enabled.
+  Email = 'user@example.com'
+
+  # For the remaining configuration items, it's important to understand the


Nit: s/it's/it is/ since we (maybe "I") removed apostrophes (replaced short forms). I thinK I've used long forms in this config file.

yuizumi · 2020-08-25T11:09:00Z

cmd/webpkgserver/webpkgserver.example.toml

+  # others cannot be used. See DNSProvider for more detail.
+
+  # The http server root directory where the ACME http challenge token should
+  # be deposited. Note that you may need to do some configuration work to


What kind of "configuration work" does the user need to do?

Configuration = setting one or more of the values in the toml.

Can you update the documentation here please?

Sorry but I can't tell what is updated...

" Note that you do not need to set the fields for all of these challenges. It

is typically sufficient to have a setting for just one of the challenges."

I used "set the fields" instead of "configure".

Thanks for your answer, but I was asking about "some configuration work" on this line, as in "... be deposited. Note that you may need to do some configuration work to get this setup to work where ..."

yuizumi · 2020-08-25T11:37:54Z

cmd/webpkgserver/webpkgserver.example.toml

+# If you set up more than one instance of webpkgserver, the recommended practice
+# is to have only one instance handle automatic certificate renewals and let
+# other instances reload the fresh certificate from the disk. It minimizes the
+# cost (SXG certificates cost money and we don't need each webpackager instance


I should have been a bit more clear, but "Some CAs allow certificates to be installed only on one machine" is just one example [*1]. Some other CAs may, for example, disallow certificates to be served from more than one machine.

Giving second thoughts, "minimize cost" is not really the point here. It depends on how CAs charge their clients. I guess that maybe DigiCert is charging for each certificate issued, but it's also possible to charge on a per-domain-per-year basis instead.

The real point is that this setup allows certificate sharing. "It allows certificates to be shared on all webpkgserver instances (which may help minimize the monetary cost, subject to Terms of Service set by your Certificate Authority) and prevents webpkgserver from bombarding your Certificate Authority with certificate requests."

[*1]: It was actually the case when I purchased a relatively cheap certificate ~15 years ago.

yuizumi · 2020-08-25T11:42:36Z

cmd/webpkgserver/webpkgserver.example.toml

+# to have a separate SXG certificate) and prevents the webpkgserver instances
+# from bombarding your Certificate Authority with certificate requests. Note
+# this setup requires you to locate the certificate file on a shared filesystem
+# accessible by all webpkgserver instances. Also note that some Certificate


"Also note that ..." is possibly inaccurate and confusing (see my comment above), so I suggest a more general warning statement. My version above implies it by "subject to Terms of Service."

It's eventually the users' responsibility to use their purchased certificates (only) in a way allowed by their issuers. I just wanted to reduce the chance that users assume their certificates can be used on multiple machines just by reading our document, not consulting Terms of Service.

Replaced the verbage with your suggestion.

yuizumi · 2020-08-25T12:00:03Z

cmd/webpkgserver/webpkgserver.example.toml

+
+  # The port used by the webpkgserver 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


Same here: what "configuration change" does webpkgserver require?

Changed it to refer to reverse proxy server configuration changes.

yuizumi · 2020-08-25T12:16:36Z

cmd/webpkgserver/webpkgserver.example.toml

+  # The port used by the webpkgserver 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:


proxy_pass is Nginx-specific. I prefer a more general description as webpkgserver can work with other http servers (e.g. Apache).

Clarified the description.

yuizumi · 2020-08-25T16:17:08Z

cmd/webpkgserver/webpkgserver.example.toml

+# Exchange certificates.
+#
+# ACME is a protocol that allows automatic renewal of certificates.
+# webpkgserver uses the ACME library from github.com/go-acme/lego


"webpkgserver uses the ACME library ..." is probably not very important for those who just use webpkgserver, especially those who don't set up DNSProvider.

"ACME is a protocol that allows automatic certificate renewal. Setting the Enable parameter to true will make webpkgserver automatically request a renewed certificate whenever it has determined that the current certificate is expired or about to expire.

If you set up more than one instance of webpkgserver, ...

webpkgserver uses the ACME library from github.com/go-acme/lego.

For the full ACME spec, see https://tools.ietf.org/html/rfc8555."

yuizumi · 2020-08-25T16:17:33Z

cmd/webpkgserver/webpkgserver.example.toml

+#   https://tools.ietf.org/html/rfc8555 
+[SXG.ACME]
+  # Enable webpkgserver to attempt to auto renew certificates using
+  # ACME.


Move ACME to the previous line?

yuizumi · 2020-08-25T16:21:52Z

cmd/webpkgserver/webpkgserver.example.toml

+  # others cannot be used. See DNSProvider for more detail.
+
+  # The http server root directory where the ACME http challenge token should
+  # be deposited. Note that you may need to do some configuration work to


Can you update the documentation here please?

yuizumi · 2020-08-25T16:25:39Z

cmd/webpkgserver/webpkgserver.example.toml

+  HTTPWebRootDir = '/path/to/www_root_dir'
+
+  # The port used by the webpkgserver to respond to the HTTP challenge
+  # issued as part of ACME protocol. Note that if your setup only opens up


Thanks for the update, but I found I still hadn't understood "if your setup only opens up certain ports."

yuizumi

Sorry for slow iterations. I think it's very close and hope this will be the last iteration...

yuizumi · 2020-08-26T12:09:26Z

server/tomlconfig/verify.go

+	if err != nil {
+		return err
+	}
+	switch u.Scheme {


Optional: switch is maybe "too much" here.

if u.Scheme != "https" { return errors.New("must be https://") } return verifyServePath(u.Path)

yuizumi · 2020-08-27T17:24:18Z

cmd/webpkgserver/webpkgserver.example.toml

+  # This is the ACME discovery URL that is used for ACME http requests to the
+  # Certificate Authority that 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/


Sorry but I can't tell what is updated...

yuizumi · 2020-08-27T17:32:59Z

cmd/webpkgserver/webpkgserver.example.toml

+  HTTPWebRootDir = '/path/to/www_root_dir'
+
+  # The port used by the webpkgserver to respond to the HTTP challenge
+  # issued as part of ACME protocol. Note that if your network firewall setup


I've been confusing with this sentence, and I've now found why. If I understand correctly, even if the firewall accepted any incoming ports, the user would anyway need to set up a reverse proxy listening at 80/tcp (http) or 443/tcp (https) and route the challenge requests to this port. This sentence reads that the user needs to configure reverse proxy only when there's a firewall, which doesn't match my understanding.

Removed the note about the firewall and changed the paragraph structure.

yuizumi · 2020-08-27T17:33:55Z

cmd/webpkgserver/webpkgserver.example.toml

+  # The port used by the webpkgserver to respond to the HTTP challenge
+  # issued as part of ACME protocol. Note that if your network firewall setup
+  # only opens up certain ports, you may need to do a configuration change to
+  # your reverse-proxy server  where you forward requests to this port using a


Remove one extra space between "server" and "where"

yuizumi · 2020-08-27T17:35:08Z

cmd/webpkgserver/webpkgserver.example.toml

+
+  # The port used by the webpkgserver to respond to the HTTP challenge
+  # issued as part of ACME protocol. Note that if your network firewall setup
+  # only opens up certain ports, you may need to do a configuration change to


s/do a configuration change to/change configuration of/

I changed the sentence to "You will need to configure your ..."

yuizumi · 2020-08-27T17:35:23Z

cmd/webpkgserver/webpkgserver.example.toml

+  # only opens up certain ports, you may need to do a configuration change to
+  # your reverse-proxy server  where you forward requests to this port using a
+  # mechamism similar to NGINX's proxy_pass (your choice of server will change
+  # how you forward requests). An example specific to NGINX:


Nit: s/specific to/for/

Sorry, I think "specific to" is correct.

yuizumi · 2020-08-27T17:38:59Z

cmd/webpkgserver/webpkgserver.example.toml

+  # others cannot be used. See DNSProvider for more detail.
+
+  # The http server root directory where the ACME http challenge token should
+  # be deposited. Note that you may need to do some configuration work to


Sorry but I can't tell what is updated...

yuizumi · 2020-08-28T11:17:17Z

cmd/webpkgserver/webpkgserver.example.toml

+  # This is the ACME discovery URL that is used for ACME http requests to the
+  # Certificate Authority that 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/


Oops, sorry, I made this comment at a wrong place.

yuizumi · 2020-08-28T11:26:50Z

cmd/webpkgserver/webpkgserver.example.toml

+  # The port used by the webpkgserver to respond to the HTTP challenge
+  # issued as part of ACME protocol. You will need to configure your
+  # reverse-proxy server where you route the challenge requests to this port
+  # using a mechamism similar to NGINX's proxy_pass (your choice of server will


Optional: "... using proxy_pass on NGINX or a similar mechanism on the server of your choice."

yuizumi · 2020-08-28T11:28:20Z

cmd/webpkgserver/webpkgserver.example.toml

+  # others cannot be used. See DNSProvider for more detail.
+
+  # The http server root directory where the ACME http challenge token should
+  # be deposited. Note that you may need to do some configuration work to


Thanks for your answer, but I was asking about "some configuration work" on this line, as in "... be deposited. Note that you may need to do some configuration work to get this setup to work where ..."

yuizumi · 2020-08-31T16:51:46Z

cmd/webpkgserver/webpkgserver.example.toml

+  # detail.
+
+  # The http server root directory where the ACME http challenge token should
+  # be deposited. Note that you may need configure your reverse-proxy server to 


Thanks, it's become a bit clearer, but I still don't understand why and how the user needs to configure their reverse-proxy server when running multiple webpkgserver instances.

Ok changed it some more, let me know if that's ok.

yuizumi · 2020-09-03T17:53:30Z

cmd/webpkgserver/webpkgserver.example.toml

+
+  # The http server root directory where the ACME http challenge token should
+  # be deposited. Note that if you are using a reverse-proxy server with
+  # multiple instances of webpkgserver instances, you may need to configure it  


Sorry, I'm trying hard to understand this sentence and reading the linked page, but I'm still failing to understand. A couple of questions include:

What does "it" refer to, as in "... to configure it in order to ..."?

What does "this" refer to, as in "...to get this work."?

Why does the user need to take extra care when they run multiple webpkgserver instances using the HTTPWebRootDir method?

Actually, this whole note about multiple webpkgserver is confusing. Best to remove it and let the reader research what's best for their particular setup.

banaag requested review from yuizumi and twifkak July 7, 2020 01:09

banaag removed the request for review from twifkak July 7, 2020 01:10

yuizumi reviewed Jul 8, 2020

View reviewed changes

banaag force-pushed the toml branch from 56fade6 to 4cbef2c Compare July 9, 2020 22:22

banaag requested a review from yuizumi July 9, 2020 22:22

yuizumi reviewed Aug 4, 2020

View reviewed changes

banaag force-pushed the toml branch from 4cbef2c to b1e14c0 Compare August 7, 2020 22:54

banaag requested a review from yuizumi August 7, 2020 22:54

banaag force-pushed the toml branch from b1e14c0 to 0097c32 Compare August 12, 2020 18:26

yuizumi reviewed Aug 14, 2020

View reviewed changes

yuizumi reviewed Aug 17, 2020

View reviewed changes

banaag force-pushed the toml branch from 0097c32 to 3c86b94 Compare August 17, 2020 22:33

banaag requested a review from yuizumi August 17, 2020 22:33

yuizumi reviewed Aug 19, 2020

View reviewed changes

banaag force-pushed the toml branch from 3c86b94 to 257cc6f Compare August 19, 2020 18:23

yuizumi requested a review from twifkak August 20, 2020 14:20

banaag requested a review from yuizumi August 20, 2020 15:39

banaag force-pushed the toml branch 2 times, most recently from f89a8b0 to 0e63a50 Compare August 20, 2020 18:19

yuizumi reviewed Aug 25, 2020

View reviewed changes

banaag force-pushed the toml branch from 0e63a50 to 391da06 Compare August 25, 2020 15:42

banaag requested a review from yuizumi August 25, 2020 15:42

banaag force-pushed the toml branch from 391da06 to b0904a1 Compare August 25, 2020 15:44

yuizumi reviewed Aug 25, 2020

View reviewed changes

banaag force-pushed the toml branch from b0904a1 to 90e391c Compare August 25, 2020 17:38

yuizumi reviewed Aug 27, 2020

View reviewed changes

banaag force-pushed the toml branch from 90e391c to a5d48db Compare August 27, 2020 21:07

banaag requested a review from yuizumi August 28, 2020 00:45

yuizumi reviewed Aug 28, 2020

View reviewed changes

banaag force-pushed the toml branch from a5d48db to a55d00c Compare August 28, 2020 16:09

banaag requested a review from yuizumi August 28, 2020 16:10

yuizumi reviewed Aug 31, 2020

View reviewed changes

banaag force-pushed the toml branch from a55d00c to c234d1d Compare August 31, 2020 18:20

banaag requested a review from yuizumi August 31, 2020 18:24

yuizumi reviewed Sep 3, 2020

View reviewed changes

Updating the toml to include ACME related entries.

a291ec8

banaag force-pushed the toml branch from c234d1d to a291ec8 Compare September 3, 2020 19:32

banaag requested a review from yuizumi September 3, 2020 19:37

yuizumi approved these changes Sep 4, 2020

View reviewed changes

banaag merged commit 5520f52 into master Sep 4, 2020

banaag deleted the toml branch September 4, 2020 16:19

Updating the toml to include ACME related entries. #49

Updating the toml to include ACME related entries. #49

Conversation

banaag commented Jul 7, 2020

banaag commented Jul 7, 2020

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

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

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

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

Choose a reason for hiding this comment

yuizumi 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

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