Skip to content

Add configuration for TLS session ticket encryption key, to allow#178

Merged
htuch merged 6 commits intoenvoyproxy:masterfrom
ggreenway:tls-ticket-key
Oct 3, 2017
Merged

Add configuration for TLS session ticket encryption key, to allow#178
htuch merged 6 commits intoenvoyproxy:masterfrom
ggreenway:tls-ticket-key

Conversation

@ggreenway
Copy link
Copy Markdown
Member

session resumption across hot-restart or between multiple envoy
instances.

Signed-off-by: Greg Greenway ggreenway@apple.com

session resumption across hot-restart or between multiple envoy
instances.

Signed-off-by: Greg Greenway <ggreenway@apple.com>
@htuch
Copy link
Copy Markdown
Member

htuch commented Sep 26, 2017

@PiotrSikora

@htuch htuch requested a review from PiotrSikora September 26, 2017 18:42
@ggreenway
Copy link
Copy Markdown
Member Author

This commit is paired with envoyproxy/envoy#1747

Comment thread api/tls_context.proto Outdated
// Keys to encrypt/decrypt TLS session tickets for session resumption. The first
// key is used to encrypt new tickets that are created. All keys are candidates
// for decrypting received tickets.
repeated DataSource tls_session_ticket_key = 4;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be separate message with a name, so that keying material can be added to the Secret and pushed using KDS (see: #180). cc @htuch

Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Sep 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 I was going to make the same comment. We will definitely want session ticket keys to be able to be provided via KDS.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PiotrSikora it would be kind of nice if there was a common message that people can use as opposed to everyone supplying their own name, etc.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to have a message like "KdsReference", and then make this oneof KdsReference or repeated Datasource?

Signed-off-by: Greg Greenway <ggreenway@apple.com>
Comment thread api/tls_context.proto Outdated

message TlsSessionTicketKeys {
// Optional identifier for this set of keys
string name = 1;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there also be a ConfigSource for kds here, similar to #180

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be KdsSecretConfig, but you might want to wait a day or two, before #180 is merged.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, let's wait for #180 to get merged and then I'll fix this up.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now merged, do you want to rebase/merge?

Comment thread api/tls_context.proto

// Keys to encrypt/decrypt TLS session tickets for session resumption. The first
// key is used to encrypt new tickets that are created. All keys are candidates
// for decrypting received tickets.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I document the expected format here? The only reason it seems weird is because if the format ever changes (due to different crypto being used), it seems weird to modify an existing protobuf field (even though the modification is only documentation, and doesn't break the protobuf wire format).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the format should be documented here, even though it doesn't affect the wire format.

Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Comment thread api/sds.proto Outdated
// If specified, Envoy will reject connections without a valid and matching SNI.
google.protobuf.BoolValue require_sni = 3;

TlsSessionTicketKeys tls_session_ticket_keys = 4;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I have a SdsSecretConfig here paired with this? I don't grok the relationship between the inline-secrets vs the pointer-to-SDS secrets.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can add a oneof for new users @PiotrSikora ?

Copy link
Copy Markdown
Member Author

@ggreenway ggreenway Oct 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was looking at this to figure out what to do:

repeated TlsCertificate tls_certificates = 2;
repeated SdsSecretConfig sds_secret_configs = 6;

Is that two separate fields instead of oneof for backwards compatibility reasons? If so, should I add a comment to that effect?

Signed-off-by: Greg Greenway <ggreenway@apple.com>
@htuch htuch merged commit e14661a into envoyproxy:master Oct 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants