Separate tls::ServerName and identity::Id types by olix0r · Pull Request #2506 · linkerd/linkerd2-proxy

olix0r · 2023-11-07T03:54:01Z

commit 833de52
Author: Oliver Gould [email protected]
Date: Mon Nov 6 23:10:45 2023 +0000

Remove the Credentials::dns_name API

The Credentials trait needlessly exposes a dns::Name value. We plan to
decouple certificates from DNS names (though DNS names will remain in
use for SNI negotiation).

This change narrows the trait interface so we can begin to make those
changes.

commit 8c35fe7
Author: Oliver Gould [email protected]
Date: Tue Nov 7 01:56:15 2023 +0000

Separate tls::ServerName and identity::Id types

To prepare for the introduction of SPIFFE identities into Linkerd's
identity system, this change we need to begin to separate the
representation of an endpoint's SNI value and its "identity" for the
purposes of authentication.

This change distinguishes:

* Authenticated `Id` values (i.e. as described by a certificate)
* Server Name Indication (SNI) values used for TLS negotation

The `tls::ServerName` type to represent the SNI use case. The
`tls::ClientTls` type now has distinct `ServerName` and `ServerId`
configurations to support distinct behavior for sending an SNI value to
the server server

The `id::Name` type is genralized as `id::Id`. In the future, this type
will be used to encompass both DNS-like and SPIFFE identities. This type
is specifically used for peer authentication (and not for SNI
negotiation).

The `id::LocalId` type is removed. It was only being used as a means for
the local server name configuration.

Co-authored-by: <[email protected]>

The Credentials trait needlessly exposes a dns::Name value. We plan to decouple certificates from DNS names (though DNS names will remain in use for SNI negotiation). This change narrows the trait interface so we can begin to make those changes.

To prepare for the introduction of SPIFFE identities into Linkerd's identity system, this change we need to begin to separate the representation of an endpoint's SNI value and its "identity" for the purposes of authentication. This change distinguishes: * Authenticated `Id` values (i.e. as described by a certificate) * Server Name Indication (SNI) values used for TLS negotation The `tls::ServerName` type to represent the SNI use case. The `tls::ClientTls` type now has distinct `ServerName` and `ServerId` configurations to support distinct behavior for sending an SNI value to the server server The `id::Name` type is genralized as `id::Id`. In the future, this type will be used to encompass both DNS-like and SPIFFE identities. This type is specifically used for peer authentication (and not for SNI negotiation). The `id::LocalId` type is removed. It was only being used as a means for the local server name configuration. Co-authored-by: <[email protected]>

zaharidichev · 2023-11-07T07:11:05Z

linkerd/meshtls/rustls/src/client.rs

        };

-        let server_id = rustls::ServerName::try_from(client_tls.server_id.as_str())
+        let server_id = rustls::ServerName::try_from(&*client_tls.server_id.to_str())


Is that to minimize change area. In my mind, this should be client_tls.server_name,no ?

zaharidichev · 2023-11-07T07:12:40Z

linkerd/identity/src/lib.rs

+}
+
+impl Id {
+    pub fn to_str(&self) -> std::borrow::Cow<'_, str> {


What is the point of using a Cow here?

This is probably a bit overkill, but the intent was to allow for future changes (URIs) to be unconstrained with regard to returning an owned String. Having slept on it, I suspect we'll be able to revert this back to as_str() if we use the uri crate...

zaharidichev · 2023-11-07T07:20:07Z

linkerd/app/gateway/src/http.rs

-                self.inbound.identity().name().clone(),
-            )))
+            .push(NewHttpGateway::layer(
+                self.inbound.identity().server_name().clone().into(),


Should this take the Id type? If I remember, this logic ends up comparing against the Identity header in order to avoid loops. That being said, should this be an id::Id eventually

Per discussion: As we introduce URI IDs, we'll need to make a more explicit handling in the gateway. In the short term, we should at least comment this. It may be best to pass an Id in so that issue is forced in the Id type change.

zaharidichev

LGTM

hawkw

Overall, I'm onboard with the new name types, I think this is much easier to follow than the previous scheme.

IMO, it would be better if we still reference counted DNS name strings, since they get cloned kind of a lot. Since more names are now represented by the unwrapped dns::Name rather than the wrapped id::Id type, I agree with @olix0r that it makes sense to move the Arc from the identity type into the dns::Name type itself. It would be nice to put back the Arc, either in this branch or in a follow-up.

hawkw · 2023-11-07T23:26:54Z

linkerd/identity/src/lib.rs

+
+// === impl Id ===
+
+impl std::str::FromStr for Id {


it feels a bit sketchy to me that this newtype is intended to specifically represent an authenticated name, but can also freely be parsed from a random string anywhere? or am I missing something here?

Ah I can clarify the comment. It's not intended to be proof of authentication, but the kind of identity used for authentication

* Include server address in server error logs (linkerd/linkerd2-proxy#2500) * dev: v42 (linkerd/linkerd2-proxy#2501) * Separate tls::ServerName and identity::Id types (linkerd/linkerd2-proxy#2506) * Use reference-counted strings in dns::Name (linkerd/linkerd2-proxy#2509) * build(deps): bump tj-actions/changed-files from 39.2.0 to 40.1.1 (linkerd/linkerd2-proxy#2508) * build(deps): bump actions/checkout from 4.1.0 to 4.1.1 (linkerd/linkerd2-proxy#2485) * ci: Fix check-each workflow (linkerd/linkerd2-proxy#2511) * ci: Turn off debuginfo in ci test builds (linkerd/linkerd2-proxy#2512) * ci: Fix fuzzer listing (linkerd/linkerd2-proxy#2513) * Use heap indirection to manage type signatures (linkerd/linkerd2-proxy#2510) * dev: optimize image build (linkerd/linkerd2-proxy#2452) * dev: Disable nightly install for now (linkerd/linkerd2-proxy#2515) * meshtls: Extract TLS id verification out of TLS backends (linkerd/linkerd2-proxy#2507) * Update DEFAULT_OUTBOUND_TCP_QUEUE_CAPACITY to 10K (linkerd/linkerd2-proxy#2521) * admin: Add optional /debug/pprof/profile endpoint (linkerd/linkerd2-proxy#2516) Signed-off-by: Oliver Gould <[email protected]>

* Include server address in server error logs (linkerd/linkerd2-proxy#2500) * dev: v42 (linkerd/linkerd2-proxy#2501) * Separate tls::ServerName and identity::Id types (linkerd/linkerd2-proxy#2506) * Use reference-counted strings in dns::Name (linkerd/linkerd2-proxy#2509) * build(deps): bump tj-actions/changed-files from 39.2.0 to 40.1.1 (linkerd/linkerd2-proxy#2508) * build(deps): bump actions/checkout from 4.1.0 to 4.1.1 (linkerd/linkerd2-proxy#2485) * ci: Fix check-each workflow (linkerd/linkerd2-proxy#2511) * ci: Turn off debuginfo in ci test builds (linkerd/linkerd2-proxy#2512) * ci: Fix fuzzer listing (linkerd/linkerd2-proxy#2513) * Use heap indirection to manage type signatures (linkerd/linkerd2-proxy#2510) * dev: optimize image build (linkerd/linkerd2-proxy#2452) * dev: Disable nightly install for now (linkerd/linkerd2-proxy#2515) * meshtls: Extract TLS id verification out of TLS backends (linkerd/linkerd2-proxy#2507) * Update DEFAULT_OUTBOUND_TCP_QUEUE_CAPACITY to 10K (linkerd/linkerd2-proxy#2521) * admin: Add optional /debug/pprof/profile endpoint (linkerd/linkerd2-proxy#2516) * proxy: Use debian12 distroless base image Signed-off-by: Oliver Gould <[email protected]>

Changes the `TlsIdentity` type in the destination API such that: - we add an extra `UriLikeIdentity` identity type that should contain identities that are in URI format (e.g. SPIFFE) - we add a `server_name` to the `TlsIdentity` type. This allows us to differentiate between an SNI value and a TLS Id value. This is mainly needed because in certain identity systems (SPIFFE/SPIRE) the TLS SAN can be in URI form. A URI cannot be used as a SNI extension in a `ClientHello`, so an alternative SNI value needs to be provided. This brings the need to distinguish between these two concepts. For context: linkerd/linkerd2-proxy#2506 Signed-off-by: Zahari Dichev <[email protected]> Co-authored-by: Oliver Gould <[email protected]>

Remove the Credentials::dns_name API

833de52

The Credentials trait needlessly exposes a dns::Name value. We plan to decouple certificates from DNS names (though DNS names will remain in use for SNI negotiation). This change narrows the trait interface so we can begin to make those changes.

olix0r requested a review from zaharidichev November 7, 2023 03:54

olix0r requested a review from a team as a code owner November 7, 2023 03:54

olix0r force-pushed the ver/ids branch from 999044b to 8c35fe7 Compare November 7, 2023 04:01

zaharidichev reviewed Nov 7, 2023

View reviewed changes

zaharidichev approved these changes Nov 7, 2023

View reviewed changes

olix0r added 4 commits November 7, 2023 19:39

Update mTLS providers to have both a Name and an Id

897d53a

Clarify use of server_id to server_name in rustls client

127bb94

fixup boring test

eb0c17f

Appease no-tls compilation tests

c943b18

zaharidichev approved these changes Nov 7, 2023

View reviewed changes

hawkw self-requested a review November 7, 2023 21:20

hawkw approved these changes Nov 7, 2023

View reviewed changes

fixup comment

02cbae7

olix0r merged commit b4ff7d2 into main Nov 8, 2023

olix0r deleted the ver/ids branch November 8, 2023 00:04

olix0r mentioned this pull request Nov 8, 2023

split tls id and name #2503

Closed

zaharidichev mentioned this pull request Dec 20, 2023

destination: add UriLikeIdentity and server_name linkerd/linkerd2-proxy-api#285

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate tls::ServerName and identity::Id types#2506

Separate tls::ServerName and identity::Id types#2506
olix0r merged 7 commits intomainfrom
ver/ids

olix0r commented Nov 7, 2023 •

edited

Loading

Uh oh!

zaharidichev Nov 7, 2023

Uh oh!

olix0r Nov 7, 2023

Uh oh!

zaharidichev Nov 7, 2023

Uh oh!

olix0r Nov 7, 2023

Uh oh!

zaharidichev Nov 7, 2023

Uh oh!

olix0r Nov 7, 2023

Uh oh!

zaharidichev left a comment

Uh oh!

hawkw left a comment

Uh oh!

hawkw Nov 7, 2023

Uh oh!

olix0r Nov 7, 2023 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

olix0r commented Nov 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zaharidichev Nov 7, 2023

Choose a reason for hiding this comment

Uh oh!

olix0r Nov 7, 2023

Choose a reason for hiding this comment

Uh oh!

zaharidichev Nov 7, 2023

Choose a reason for hiding this comment

Uh oh!

olix0r Nov 7, 2023

Choose a reason for hiding this comment

Uh oh!

zaharidichev Nov 7, 2023

Choose a reason for hiding this comment

Uh oh!

olix0r Nov 7, 2023

Choose a reason for hiding this comment

Uh oh!

zaharidichev left a comment

Choose a reason for hiding this comment

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

hawkw Nov 7, 2023

Choose a reason for hiding this comment

Uh oh!

olix0r Nov 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

olix0r commented Nov 7, 2023 •

edited

Loading

olix0r Nov 7, 2023 •

edited

Loading