Caddy: Incorrect documentation for {tls_version} placeholder

[vcsjones]

(Logging issue per a forum topic):

While setting up logging, I believe I found an issue with the documentation issue for the TLS version placeholder. The documentation for placeholders indicates that the placeholder is called {tls_version}. However, this placeholder always produces a value of "-".

Looking at the source, it appears that the placeholder is actually called {tls_protocol}:

https://github.com/mholt/caddy/blob/95514da91bbf134baaa6b18e3e9037a5dd165d1c/caddyhttp/httpserver/replacer.go#L414

[mholt]

Oops, you are right! Will fix that soon. Thanks.

[detroitenglish]

Fell victim to this today trying to rewrite clients using tls1.0 ... thought I was going bananas 🍌🙈

[mholt]

I wonder if we _should_ change it to tls_version. Wouldn't that make more sense?

[vcsjones]

Wouldn't that make more sense?

It would make more sense long term yes, at the cost of this being a breaking change that won't break in an obvious way (our logger would just start logging "-" until someone noticed it).

I'm not sure how Caddy handles deprecation, but perhaps:

case "{tls_protocol}":
    // Warn deprecated and will be removed in the future.
    fallthrough;
case "{tls_version}":
    //etc

[francislavoie]

+1 for deprecation and potentially removal in 0.12-ish, whenever that happens

[mholt]

Yeah, we can do that, although the docs always said tls_version so this is more of a bug fix (a patch release) than a breaking change.

+1 for deprecation and potentially removal in 0.12-ish, whenever that happens

Hoping not to have a 0.12 tree; next major release should be 1.0.

[francislavoie]

Okay :stuck_out_tongue: I was just going by the milestones listed in github

[vcsjones]

@mholt you mentioned previously you might prefer to make a breaking change prior to 1.0 to change the placeholder. If so, since 1.0 is near, just flagging this for you to consider if you think that approach should be done before 1.0.

[mholt]

Fixed, updating site shortly. Will be using {tls_protocol} because the returned string does include the protocol name ("tls") as a prefix. A bit leftover from the SSL days but whatever. We'll change this in future versions not subject to the compatibility guarantee. Thanks!