tls_cipher is not logged correctly with TLSv1.3

vcsjones commented 5 years ago

1. Which version of Caddy are you using (`caddy -version`)?

0.11.5.

2. What are you trying to do?

Have caddy log the tls_cipher that the client and server negotiated with TLSv1.3

4. How did you run Caddy (give the full command and describe the execution environment)?

Can re reproduced simply by running caddy.

6. What did you expect to see?

I expect the logs to contain the TLS cipher like "TLS_AES_128_GCM_SHA256".

7. What did you see instead (give full error messages and/or log)?

"UNKNOWN" is seen in the logs.

8. Why is this a bug, and how do you think this should be fixed?

It looks like config.go needs the 1.3 cipher suites added to SupportedCiphersMap which is used by GetSupportedCipherName

https://github.com/mholt/caddy/blob/72d0debde6bf01b5fdce0a4f3dc2b35cba28241a/caddytls/config.go#L457

The lack of the TLSv1.3 cipher suite IDs means that the logged suite will be "UNKNOWN", and that the tls_cipher environment variable that the fastcgi module sets here:

https://github.com/mholt/caddy/blob/053373a38519d8cdf4ee7582ed9dc6ce239597cc/caddyhttp/fastcgi/fastcgi.go#L341

The go docs seem to indicate that the TLSv1.3 constants are there, so I think this is a matter of adding a few more IDs to the map, something like:

...
"TLS_AES_128_GCM_SHA256": tls.TLS_AES_128_GCM_SHA256,
"TLS_AES_256_GCM_SHA384": tls.TLS_AES_256_GCM_SHA384,
"TLS_CHACHA20_POLY1305_SHA256": tls.TLS_CHACHA20_POLY1305_SHA256

Looking a bit hard through, it looks like this map is also used for configuring caddy. Since Go's ciphers are not configurable for TLSv1.3, adding them to the existing map is likely not correct.

https://github.com/mholt/caddy/blob/72d0debde6bf01b5fdce0a4f3dc2b35cba28241a/caddytls/setup.go#L200

mholt commented 5 years ago

Oops, good point. The new ciphers were originally omitted from the map intentionally because they can't be configured.

Would totally accept a pull request that adds those to the map, if it has a test that shows that setting up the TLS directive doesn't break if those ciphers are specified.

francislavoie commented 5 years ago

moorereason commented 4 years ago

Commit https://github.com/golang/go/commit/0ee22d9 should be included in the upcoming Go 1.14 release. Can that replace some of Caddy's existing code? Would Caddy's API need to change for v2.0 to prepare for these pending changes in Go 1.14? If so, is it too late for that?

mholt commented 4 years ago

@moorereason It's not too late; if Go 1.14 is released on time, it should be before our release candidates. We should try to get this into v2.

chimeworld commented 4 years ago

I'd like to work on this if help is still needed.

mholt commented 4 years ago

@ali-alsabbah That would be greatly appreciated!

chimeworld commented 4 years ago

@mholt just curious, do I just need to add the ciphers mentioned by @vcsjones to the map SupportedCiphersMap?

(apologies in advance, I'm fairly new to Go and this project)

vcsjones commented 4 years ago

@ali-alsabbah further up in this issue's history, you should see a closed pull request where I attempted to fix it.

There is a little more to it than adding it to the map - last I looked, Caddy used the same map to parse caddy's config - and TLS 1.3 suites are not configurable in Go. The linked pull request noted that. I think the pull request was fairly close - I just struggled to find the time to finish it out.

If you want to take what I did in the pull request, please do.

mholt commented 4 years ago

Also, I should mention that this should be branched from the v2 branch, see here (and see the highlighted comment): https://github.com/caddyserver/caddy/blob/64f0173948e9b87a8f898d451ef35daefedf2c50/modules/caddytls/values.go#L25-L55

Note that there are some divergences in the naming... would need to see if it is still worth using.

moorereason commented 4 years ago

@mholt, I was going to add this to v2 a few days ago, but I've been unable to replicate the original issue in v2 since the log format doesn't seem to be customizable from the config. Additionally, the tls_* fields don't appear to be in the caddyhttp replacer.

This is the first time I've looked at v2, so maybe I'm missing something. Any pointers?

mholt commented 4 years ago

@moorereason Good point, I haven't yet exposed TLS-related placeholders in v2. In v2, we use structured logging, so the only customization would be filtering/removal of fields, rather than crafting a template.

chimeworld commented 4 years ago

Alright, I’ll work on this over the next couple of weeks.

moorereason commented 4 years ago

@mholt said:

In v2, we use structured logging, so the only customization would be filtering/removal of fields,

How does this work? I have http.log.access log working (using this), but I'm not sure how to customize the structured access logging.

@ali-alsabbah, I've submitted #2982 that should (eventually) fix this issue.

mholt commented 4 years ago

@moorereason Great question. What happens is that log entries are emitted with structure, i.e. a mapping of field to value. What fields any particular log emits is arbitrary depending on what the developer writes.

The encoder is what determines how to write the output (i.e. JSON, console, etc.). We have a special encoder called a filter encoder which has access to the individual fields, so it can change or remove them before passing them to a wrapped/underlying encoder that does the actual encoding work.

To add TLS data to log emissions, it should be as simple as just adding the relevant fields to a log like this one: https://github.com/caddyserver/caddy/blob/e51e56a4944622c0a2c7d19da4bb6b9bb07c1973/modules/caddyhttp/server.go#L175-L181

moorereason commented 4 years ago

@mholt, Thanks for the explanation. I understand the structured logging aspect, but how does a Caddy2 user (as opposed to a developer) customize the the access logs from the config, similar to how Caddy1 allows you to customize the format with log path file [format]?

chimeworld commented 4 years ago

@moorereason does this mean no work is needed from me here?

@mholt if that’s the case, do you have any other issues you need help with that are good for newcomers to Caddy?

francislavoie commented 4 years ago

@ali-alsabbah this is getting off-topic from this thread, but one thing I could see being very valuable is re-implementing some of the Caddy v1 plugins as v2 modules.

See the bottom of the sidebar on https://caddyserver.com/v1/docs (Directives/Middleware) for a list of v1 plugins.

See https://caddyserver.com/docs/extending-caddy for docs on setting up v2 modules.

Feel free to open up new issues if you need any help!

moorereason commented 4 years ago

@ali-alsabbah: Correct.

chimeworld commented 4 years ago

@francislavoie is there an issue open for this? Would like to discuss further.

francislavoie commented 4 years ago

@ali-alsabbah Nope. Like I said, feel free to open up new issues for any plugins you want to work on if you need any help.

moorereason commented 4 years ago

Resolved in b0a491aec808652dfd65910ee192ab88c07ac99d

caddyserver / caddy