[squid-dev] RFC: tls_key_log: report TLS pre-master secrets, other key material

[Alex Rousskov]

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.