Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Nov 8, 2023
Merged

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

merged 7 commits into from
Nov 8, 2023

Conversation

olix0r
Copy link
Member

@olix0r olix0r commented Nov 7, 2023

commit 833de52
Author: Oliver Gould ver@buoyant.io
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 ver@buoyant.io
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: <zahari@buoyant.io>

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 olix0r requested a review from zaharidichev November 7, 2023 03:54
@olix0r olix0r requested a review from a team as a code owner November 7, 2023 03:54
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: <zahari@buoyant.io>
@@ -68,7 +68,7 @@ impl Connect {
}
};

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())
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Good spot!

}

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

Choose a reason for hiding this comment

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

What is the point of using a Cow here?

Copy link
Member Author

Choose a reason for hiding this comment

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

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...

self.inbound.identity().name().clone(),
)))
.push(NewHttpGateway::layer(
self.inbound.identity().server_name().clone().into(),
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@zaharidichev zaharidichev left a comment

Choose a reason for hiding this comment

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

LGTM

@hawkw hawkw self-requested a review November 7, 2023 21:20
Copy link
Contributor

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

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.


// === impl Id ===

impl std::str::FromStr for Id {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@olix0r olix0r Nov 7, 2023

Choose a reason for hiding this comment

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

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

@olix0r olix0r merged commit b4ff7d2 into main Nov 8, 2023
15 checks passed
@olix0r olix0r deleted the ver/ids branch November 8, 2023 00:04
@olix0r olix0r mentioned this pull request Nov 8, 2023
olix0r added a commit to linkerd/linkerd2 that referenced this pull request Nov 17, 2023
* 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 <ver@buoyant.io>
olix0r added a commit to linkerd/linkerd2 that referenced this pull request Nov 17, 2023
* 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 <ver@buoyant.io>
zaharidichev added a commit to linkerd/linkerd2-proxy-api that referenced this pull request Jan 5, 2024
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 <zaharidichev@gmail.com>
Co-authored-by: Oliver Gould <ver@buoyant.io>
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