tls_cipher is not logged correctly with TLSv1.3 #2505

vcsjones · 2019-03-05T14:58:08Z

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 · 2019-03-05T15:09:27Z

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 · 2019-03-05T15:24:27Z

Similar to #2485

moorereason · 2020-01-10T23:43:35Z

Commit golang/go@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 · 2020-01-11T07:39:22Z

@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.

ali-alsabbah · 2020-01-11T20:44:22Z

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

mholt · 2020-01-11T22:20:34Z

@ali-alsabbah That would be greatly appreciated!

ali-alsabbah · 2020-01-12T00:00:16Z

@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 · 2020-01-12T17:37:26Z

@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 · 2020-01-12T21:03:45Z

Also, I should mention that this should be branched from the v2 branch, see here (and see the highlighted comment):

caddy/modules/caddytls/values.go

Lines 25 to 55 in 64f0173

    
           // SupportedCipherSuites is the unordered map of cipher suite 
        
           // string names to their definition in crypto/tls. All values 
        
           // should be IANA-reserved names. See 
        
           // https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml 
        
           // Two of the cipher suite constants in the standard lib do not use the 
        
           // full IANA name, but we do; see: 
        
           // https://github.com/golang/go/issues/32061 and 
        
           // https://github.com/golang/go/issues/30325#issuecomment-512862374. 
        
           // TODO: might not be needed much longer: https://github.com/golang/go/issues/30325 
        
           var SupportedCipherSuites = map[string]uint16{ 
        
           	"TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384":       tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, 
        
           	"TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384":         tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, 
        
           	"TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256":       tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, 
        
           	"TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256":         tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, 
        
           	"TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256": tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, 
        
           	"TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256":   tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305, 
        
           	"TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA":            tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, 
        
           	"TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256":         tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256, 
        
           	"TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA":            tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, 
        
           	"TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA":          tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, 
        
           	"TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256":       tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256, 
        
           	"TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA":          tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, 
        
           	"TLS_RSA_WITH_AES_128_GCM_SHA256":               tls.TLS_RSA_WITH_AES_128_GCM_SHA256, 
        
           	"TLS_RSA_WITH_AES_256_GCM_SHA384":               tls.TLS_RSA_WITH_AES_256_GCM_SHA384, 
        
           	"TLS_RSA_WITH_AES_256_CBC_SHA":                  tls.TLS_RSA_WITH_AES_256_CBC_SHA, 
        
           	"TLS_RSA_WITH_AES_128_CBC_SHA256":               tls.TLS_RSA_WITH_AES_128_CBC_SHA256, 
        
           	"TLS_RSA_WITH_AES_128_CBC_SHA":                  tls.TLS_RSA_WITH_AES_128_CBC_SHA, 
        
           	"TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA":           tls.TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA, 
        
           	"TLS_RSA_WITH_3DES_EDE_CBC_SHA":                 tls.TLS_RSA_WITH_3DES_EDE_CBC_SHA, 
        
           }

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

moorereason · 2020-01-13T22:17:47Z

@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 · 2020-01-14T15:18:38Z

@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.

ali-alsabbah · 2020-01-16T02:49:08Z

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

moorereason · 2020-01-17T04:04:42Z

@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 · 2020-01-17T04:57:14Z

@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:

caddy/modules/caddyhttp/server.go

Lines 175 to 181 in e51e56a

    
           log("handled request", 
        
           	zap.String("common_log", repl.ReplaceAll(commonLogFormat, "-")), 
        
           	zap.Duration("latency", latency), 
        
           	zap.Int("size", wrec.Size()), 
        
           	zap.Int("status", wrec.Status()), 
        
           	zap.Object("resp_headers", LoggableHTTPHeader(wrec.Header())), 
        
           )

moorereason · 2020-01-17T13:39:21Z

@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]?

ali-alsabbah · 2020-01-18T19:49:50Z

@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 · 2020-01-18T21:00:06Z

@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 · 2020-01-19T04:18:52Z

@ali-alsabbah: Correct.

ali-alsabbah · 2020-01-26T23:37:36Z

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

francislavoie · 2020-01-26T23:52:16Z

@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.

mholt added bug help wanted good first issue labels Mar 5, 2019

vcsjones mentioned this issue Mar 5, 2019

Add TLSv1.3 to list of known ciphers. #2506

Closed

mholt added this to the 2.0 milestone May 9, 2019

caddyserver deleted a comment Nov 12, 2019

moorereason mentioned this issue Jan 14, 2020

v2: Expose TLS placeholders #2977

Open

moorereason mentioned this issue Jan 17, 2020

v2: Expose TLS placeholders #2982

Open

caddyserver / caddy

Sponsor caddyserver/caddy

Track tasks and feature requests

tls_cipher is not logged correctly with TLSv1.3 #2505

tls_cipher is not logged correctly with TLSv1.3 #2505

Comments

vcsjones commented Mar 5, 2019 • edited

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

2. What are you trying to do?

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

6. What did you expect to see?

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

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

This comment has been minimized.

mholt commented Mar 5, 2019

This comment has been minimized.

francislavoie commented Mar 5, 2019

This comment has been minimized.

moorereason commented Jan 10, 2020

This comment has been minimized.

mholt commented Jan 11, 2020

This comment has been minimized.

ali-alsabbah commented Jan 11, 2020

This comment has been minimized.

mholt commented Jan 11, 2020

This comment has been minimized.

ali-alsabbah commented Jan 12, 2020

This comment has been minimized.

vcsjones commented Jan 12, 2020

This comment has been minimized.

mholt commented Jan 12, 2020 • edited

This comment has been minimized.

moorereason commented Jan 13, 2020

This comment has been minimized.

mholt commented Jan 14, 2020

This comment has been minimized.

ali-alsabbah commented Jan 16, 2020

This comment has been minimized.

moorereason commented Jan 17, 2020

This comment has been minimized.

mholt commented Jan 17, 2020

This comment has been minimized.

moorereason commented Jan 17, 2020

This comment has been minimized.

ali-alsabbah commented Jan 18, 2020

This comment was marked as off-topic.

francislavoie commented Jan 18, 2020 • edited

This comment has been minimized.

moorereason commented Jan 19, 2020

This comment was marked as off-topic.

ali-alsabbah commented Jan 26, 2020

This comment was marked as off-topic.

francislavoie commented Jan 26, 2020

vcsjones commented Mar 5, 2019 •

edited

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

mholt commented Jan 12, 2020 •

edited

francislavoie commented Jan 18, 2020 •

edited