Skip to content

tls: support dual ECDSA/RSA certs via SDS#16605

Merged
htuch merged 8 commits intoenvoyproxy:mainfrom
ggreenway:sds-multiple-certs
May 25, 2021
Merged

tls: support dual ECDSA/RSA certs via SDS#16605
htuch merged 8 commits intoenvoyproxy:mainfrom
ggreenway:sds-multiple-certs

Conversation

@ggreenway
Copy link
Copy Markdown
Member

This was previously only supported via static file-based certificate configuration.

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

Commit Message:
Additional Description:
Risk Level: Low
Testing: Added integration test; possibly need more unit tests
Docs Changes: Documented in protos
Release Notes: Added
Platform Specific Features: none
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

This was previously only supported via static file-based certificate configuration.

Signed-off-by: Greg Greenway <ggreenway@apple.com>
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #16605 was opened by ggreenway.

see: more, trace.

ggreenway added 2 commits May 20, 2021 13:00
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Signed-off-by: Greg Greenway <ggreenway@apple.com>
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM modulo nit on version history comment.

* listener: added ability to change an existing listener's address.
* metric service: added support for sending metric tags as labels. This can be enabled by setting the :ref:`emit_tags_as_labels <envoy_v3_api_field_config.metrics.v3.MetricsServiceConfig.emit_tags_as_labels>` field to true.
* tcp: added support for :ref:`preconnecting <v1.18.0:envoy_v3_api_msg_config.cluster.v3.Cluster.PreconnectPolicy>`. Preconnecting is off by default, but recommended for clusters serving latency-sensitive traffic.
* tls: allow dual ECDSA/RSA certs via SDS. This was previously only supported via static file-based certificate configuration.
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.

That's not true; it was possible to do inline ECDS/RSA certs via TLS contexts included directly in Listener or Cluster transport socket configuration. Suggest rephrasing to reflect this.

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.

Ah, I see. That's what I intended to say, but the phrasing came out misleading. Will fix.

Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@htuch htuch merged commit 7d4b2ca into envoyproxy:main May 25, 2021
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
Risk Level: Low
Testing: Added integration test; possibly need more unit tests
Docs Changes: Documented in protos
Release Notes: Added

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

@ggreenway Sorry to tag you on old PR. This PR only enabled dual certificate in sds_secret_config but did not touch Validation Context. Is that assumption that CA will have single ca certificate for both RSA and ECDSA certs that it issues? Is that the normal case?

@ggreenway
Copy link
Copy Markdown
Member Author

@ramaraochavali the configured CA is a bundle and can have multiple CA certs, so you could bundle an RSA CA and an EC CA in the same bundle and configure it that way.

@ramaraochavali
Copy link
Copy Markdown
Contributor

Thank you. That is what I realized too.

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.

3 participants