Support new -invalidcert flag #278
Conversation
Run amppackager in production mode with known invalid SSL certificates. Useful to emulate an otherwise fully production environment without having to pay the sole provider of SSL certificates with CanSignHttpExchanges extension.
|
@Gregable I'm not normally in the camp of we need more flags!, but this feature would have been very useful to me while I was initially testing amppackager. Not a problem if you would rather implement this differently; I just hope it can inspire thoughts about making first-time use of amppackager easier. |
|
Thanks! I'm generally in the same camp as you, but I'm okay with flags for things that signal this isn't a production instance. (It's easier to accidentally leave something in a config file than to accidentally paste it into your init script.) FYI there's a similar FR at #188. That one addresses a different stage: I've got a production SXG cert from a CA, but I want to test it before intercepting production requests. I think your PR can coexist with that one—they probably should be separate flags—but I wanted to point it out in case you had ideas. |
Use form !(A || B) rather than !A && !B for invalidcert and development flags.
cmd/amppkg/main.go
Outdated
| @@ -91,7 +91,7 @@ func main() { | |||
| if certs == nil || len(certs) == 0 { | |||
| die(fmt.Sprintf("no cert found in %s", config.CertFile)) | |||
| } | |||
| if (!*flagDevelopment && !*flagInvalidCert) && !util.CanSignHttpExchanges(certs[0]) { | |||
| if !(*flagDevelopment || *flagInvalidCert) && !util.CanSignHttpExchanges(certs[0]) { | |||
There was a problem hiding this comment.
Oops, actually it reads even better if you De Morgan again:
!(*flagDevelopment || *flagInvalidCert || util.CanSignHttpExchanges(certs[0]))
But that's outside the scope of your change, so feel free to ignore.
There was a problem hiding this comment.
Resolved in commit 79453d9
cmd/amppkg/main.go
Outdated
| @@ -127,7 +127,7 @@ func main() { | |||
| } | |||
|
|
|||
| packager, err := signer.New(certs[0], key, config.URLSet, rtvCache, certCache.IsHealthy, | |||
| overrideBaseURL, /*requireHeaders=*/!*flagDevelopment && !*flagInvalidCert) | |||
| overrideBaseURL, /*requireHeaders=*/!(*flagDevelopment || *flagInvalidCert)) | |||
There was a problem hiding this comment.
Actually, on further consideration, are you sure flagInvalidCert belongs here? requireHeaders seems unrelated to the invalid cert:
amppackager/packager/signer/signer.go
Lines 352 to 362 in e4bf043
For testing one can use curl, amppkg_dl_sxg, or something like ModHeader or Requestly.
There was a problem hiding this comment.
Thanks for looking at this closer. You're correct. The -invalidcert flag is meant to be "otherwise production", so yes, we should expect a valid AMP-Cache-Transform header.
There was a problem hiding this comment.
I verified commit 97f8a38 works as intended when sending values nothing and any for header AMP-Cache-Transform.
Use form !(A || B) rather than !A && !B
Restore the requirement that the AMP-Cache-Transform header should be present and valid when determining whether to respond with an SXG.
|
Done. Note this is |
Run amppackager in production mode with known invalid SSL certificates.
Useful to emulate an otherwise fully production environment without
having to pay the sole provider of SSL certificates with
CanSignHttpExchanges extension.