On 7/30/20 6:28 AM, Amos Jeffries wrote: >> On 7/15/20 3:14 PM, Alex Rousskov wrote: >>> I propose to add a new tls_key_log directive to record TLS >>> pre-master secret (and related encryption details) for to- and >>> from-Squid TLS connections. This very useful triage feature is common >>> for browsers and some networking tools. Wireshark supports it[1]. You >>> might know it as SSLKEYLOGFILE. It has been requested by several Squid >>> admins. A draft documentation of the proposed directive is at the end of >>> this email. >>> >>> [1] https://wiki.wireshark.org/TLS#Using_the_.28Pre.29-Master-Secret >>> >>> If you have any feature scope adjustments, implementation wishes, or >>> objections to this feature going in, please let me know!
> Two design points: > > 1) It seems to me these bits are part of the handshake. So would come in > either as members/args of the %handshake logformat macros (some not yet > implemented) or as secondary %handshake_foo macros in the style %cert_* > macros use. Sure, if somebody wants to add a new logformat %code to log secrets, they should do that. It would not be a good solution for the problems tls_key_log is solving, but it could be useful for other reasons, of course. That addition will probably be able to reuse some of the tls_key_log code as well. As for reusing %handshake for such logging, I am not sure: Connection secrets cannot be a part of the plain text handshake (for obvious reasons). Logging encrypted secrets does not help. If somebody wants to add plain text secrets logging via a new %handshake parameter, they should double check whether all the secrets are truly a part of the encrypted handshake -- I suspect that the handshake actually ends sooner and/or contains secret _derivatives_. At any rate, these details are all outside the tls_key_log scope. We do not need to investigate or agree on them right now AFAICT. If I missed your point, please clarify. > 2) Please do use the logging logic implemented for access_log, just with > the next directive as list of log outputs to write at the appropriate > logging trigger time. Sorry, I do not understand what the above paragraph means. The term "logging logic implemented for access_log" is so broad that I cannot tell what you are trying to subtract or add to the proposed tls_key_log directive interface and/or its implementation. If this is already covered by the specific discussion below, then just skip to that! > I accept the reasoning for not using access_log directive. This will > need a new log directive with different times when it triggers output > written there. Yes. > However (most of) the implementation logic of access_log > should be usable for this new output. I see no reason to repeat access_log interface mistakes (e.g., the special "none" destination) and no need to support some of the powerful/complex access_log features in tls_key_log (e.g., logformat), but perhaps you are not asking for any of that. Please be more specific if this is not covered by the discussion below. >>> tls_key_log <filename> if <acl>... > Please allow extension points for options and modules: > > tls_key_log stdio:<filename> [options] if <acl>... We will requite the ugly "stdio:" suffix to avoid arguing about it. Future addition of optional parameters was already supported; there were just no such options until you requested rotate=N below. >>> At most one log file is supported at this time. Repeated tls_key_log >>> directives are treated as fatal configuration errors. By default, no >>> log is created or updated. > With ACL support it seems reasonable to support multiple logs. We should > be able to re-use (with minor change to pass the list of log outputs to > the function) the logic access_log has for writing to a list of outputs. I agree that supporting multiple tls_key_log is reasonable. I would leave that feature for a future seamless improvement -- that is why repeated tls_key_log directives are treated as a fatal configuration error for now. The existing multi-log logic for access_logs is confusing for admins and has serious implementation bugs. I do not think we should model the new feature on that interface or code. However, we do not need to agree on this aspect. It can and should be decided later, driven, in part, by real use cases for multilog support. >>> This log is rotated based on the logfile_rotate settings. > Please don't use solely that directive. The new directive should have a > rotate=N option of its own. Only using the global directive as a default > if that option is unset. Will add to avoid arguing about it. Thank you, Alex. _______________________________________________ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev