L83: TLS Session Key Export in gRPC C++

[Vignesh2208]

Relevant Github Issue: grpc/grpc#24944
Relevant PR: grpc/grpc#26812

CC @markdroth, @ctiller

[markdroth]

The number "L82" is already taken by #245. Please use "L83" for this.

[ctiller]

Thanks for writing this up!

[ctiller]

Let's omit this for now.

Instead, add a channel arg (string) for the path, and if we need to also specify the format we can add a secondary parameter in the future.

[Vignesh2208]

This is fine for the client side but there's an issue on the server side. Currently the ServerBuilder API does not allow adding unique channel args for each listening port - this is needed if we want to support ssl key logging on a per-port basis. We would need to modify the ServerBuilder API as well.

Currently the TLS Key logging config is associated with the TlsCredentials object. Not sure if we want to move it to channel args for a couple of reasons:

1. The above issue with configuring per-port ssl key logging on the server side - it should not require any ServerBuilder API changes if we don't want to support per-port ssl key logging though.
2. Compatibility with GRPC go implementation which also currently attaches TLS Key logging config to TLS credentials instead of channel args.
3. Adding such a config string to channel args can also be ambiguous if the corresponding credentials is not used. For e.g, a user might use InsecureCredentials for a channel but may still set tls key log file path in channel args. In this case it becomes a no-op. We need to document clearly which types of credentials would support it.

[yihuazhang]

One benefit I can think of using channel args is that it is agnostic to credential types and thus can support both SSL and TLS credentials. However, I do not think that benefit outweighs all three reasons you mentioned here, especially there is no motivation to support the feature in the old SSL stack.

[markdroth]

I think we definitely want this feature to be configured via TlsCreds, not via channel args. That's how we're doing all credential configuration, and I think it would be quite strange to do this one feature differently.

[ctiller]

I'm still an advocate for requiring both a channel arg and an environment variable... having two locks on such a potentially damaging feature seems useful.

[Vignesh2208]

You mean like an environment variable override i.e, log keys only when both the env variable and the above configs are set ?

[yihuazhang]

Craig, I agree it is a potentially damaging feature if misused. But in the current design, in order to enable it, a user needs to explicitly create a TlsKeyLoggerConfig instance and set it in TlsServerCredentialsOptions, which I think is sufficient to confirm user's intention to enable the feature. I am concerned that introducing extra knobs may result in usability problems.

[Vignesh2208]

There is a GRPC_TLS_KEY_LOGGING_ENABLED environment variable to override right now. I think we can remove this at a later point if there are usability issues.

[ctiller]

@yihuazhang @ZhenLian @markdroth I don't think I'm sufficiently up to date with the TLS creds model to be an approver here, can someone else take that role?

[yihuazhang]

@ZhenLian, since the new feature will be added to advanced TLS, do you mind taking a look? PLMK if you are busy, I can also take the role.

[ZhenLian]

@yihuazhang Yeah this is in my list, but feel free to take another look if possible.
Sorry about the delay - I was busy with perf&intern stuff, etc in the past few weeks.

[yihuazhang]

LGTM overall. Left some minor comments.

[yihuazhang]

One benefit I can think of using channel args is that it is agnostic to credential types and thus can support both SSL and TLS credentials. However, I do not think that benefit outweighs all three reasons you mentioned here, especially there is no motivation to support the feature in the old SSL stack.

[yihuazhang]

Craig, I agree it is a potentially damaging feature if misused. But in the current design, in order to enable it, a user needs to explicitly create a TlsKeyLoggerConfig instance and set it in TlsServerCredentialsOptions, which I think is sufficient to confirm user's intention to enable the feature. I am concerned that introducing extra knobs may result in usability problems.

[markdroth]

Thanks for writing this up!

Given that TlsCreds as a whole is still experimental and that @ZhenLian is still working on a gRFC for it, I think it would probably make sense to just proceed with the implementation as experimental and merge this content into that in-progress gRFC for TlsCreds as a whole.

Please let me know if you have any questions. Thanks!

[markdroth]

The fact that we're using experimental here gives me pause, since experimental APIs don't actually require gRFCs at all. TlsCreds as a whole is still experimental, and there will be a gRFC for it as a whole when we're ready to de-experimentalize it (see #205 for some work-in-progress).

I suspect this means that we should just add this API to the TlsCreds gRFC that @ZhenLian is working on, rather than having this separate gRFC. And not having the gRFC merged should not block the implementation, since the API is still currently experimental -- it just means it may be subject to change later.

[Vignesh2208]

Yes @ZhenLian also thought it would be a good idea to merge them. I'll do that after this is approved.

[markdroth]

I think we definitely want this feature to be configured via TlsCreds, not via channel args. That's how we're doing all credential configuration, and I think it would be quite strange to do this one feature differently.

[markdroth]

Please document both the C-core API and the C++ API for the TlsCreds options.

[Vignesh2208]

Done. I have added new sections to document them.

[ZhenLian]

nit: declare public earlier, then private:
https://google.github.io/styleguide/cppguide.html#Declaration_Order

[ZhenLian]

Since grpc_tls_key_log_config is an opaque type, how can the users specify different configurations through it?
I thought we were going to have something like this:

typedef struct tls_key_log_format {
  ...
}

typedef struct grpc_tls_key_log_config {
  const char* log_file_path;
  tls_key_log_format format;
}
void grpc_tls_credentials_options_set_tls_key_log_config(
    grpc_tls_credentials_options* options, grpc_tls_key_log_config* config);

Then we don't need to expose gprc_tls_key_logger at all.

[ZhenLian]

This is looking pretty good!

[ZhenLian]

nit: exporing/exporting

[ZhenLian]

@markdroth Mark, can I know in general, when do we expect a gRFC to be merged? Do we merge it when the proposed API is implemented, or we merge it when the proposed API become stable?
I know here we are waiting for #205 to get in, but given the recent work to unify the security API, the newly proposed change is unlikely to get implemented throughout the end of this year. That might further defer gRFC(s) like this.
There are still some rooms for improvements around Provider and Verifier, but the TlsCredentials and TlsCredentialsOptions themselves are pretty "stable" now. I wonder if we can come up with a way to merge the gRFCs step by step? Maybe I can write the gRFC for the TlsCredentials and TlsCredentialsOptions, and then we can gradually merge some small features, like TLS key exporting, or TLS version selection(which is a feature blocking for a really long time, since last year I think, @matthewstevenson88)? For gRFCs about Provider or Verifier, it's OK to put them in Open state, since they are still not finalized yet.
This also helps to keep each gRFC relatively short and staying on one single security topic, which might be easier for future readers to read. What do you think about this idea?
@yihuazhang FYI.

[markdroth]

My preference is still to have one single gRFC for the entire TlsCreds API, which we'd want to merge when we get to the point where we're ready to de-experimentalize the API. I think that will make it much easier to see the entire cohesive design in one place.

Note that the above can happen while some features are still experimental; those features would just not be covered in the gRFC and can be added later in a separate gRFC. But we would need to get to the point where the APIs for all of the core functionality of TlsCreds is stabilized and we're ready to commit to those APIs.

In particular, I don't think it makes sense to go forward with any gRFC for TlsCreds without stabilizing our design for the providers and verifiers. That's core functionality that we need to have nailed down, IMHO.