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

rfc: add dns names rfc #103

Closed
wants to merge 1 commit into from
Closed

rfc: add dns names rfc #103

wants to merge 1 commit into from

Conversation

3u13r
Copy link
Member

@3u13r 3u13r commented Jan 30, 2024

After yesterdays discussion I found a flaw in my proposed architecture. Namely, we'd need to move the attestation and certificate creation to the application layer. For a more detailed explanation see below.
I also described an alternative approach (that is essentially the status quo) with another suggestion on how to improve things.

Let me know what you think. I hope I described the problem detailed enough that everyone can have an informed opinion about this.

@3u13r 3u13r requested a review from katexochen as a code owner January 30, 2024 00:10
@3u13r 3u13r requested review from malt3 and burgerdev January 30, 2024 00:11
Comment on lines +34 to +35
The environment value is read by the initializer init container and used as
argument for the `NewMeshCert()` call.
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not solve the lateral movement problem: with a foothold in the mesh, I can obtain my own certificate with SAN of my choice.

Copy link
Contributor

@malt3 malt3 Jan 30, 2024

Choose a reason for hiding this comment

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

In theory, the coordinator could still validate requests using the EDG_DNS_NAMES variable since it is part of the policy and only return certs that were specified there. Not sure if I like this design.

Comment on lines +37 to +41
Note that `NewMeshCert()` does not create the cert on the application layer but
only retrieves the cert created on layer 4.
In order to pass additional arguments from the initializer side, we'd need to
shift the certificate creation and therefore the attestation to the application
layer.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: if we need an OSI layer here, shouldn't it be 6?

Why is this a bad thing? Wouldn't it be better anyway to attest on the aTLS layer and then use the attestation result in the RPC handler to generate a fresh certificate?

Comment on lines +48 to +50
Let the manifest set by the user inside the coordinator contain a mapping from
workload policy hash to DNS names. When creating the manifest we map each
workload to `*`. This can then be changed later by the user.
Copy link
Contributor

Choose a reason for hiding this comment

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

Imho this is the way to go.

Copy link
Contributor

Choose a reason for hiding this comment

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

@3u13r does this have obvious downsides that we are not seeing? I also believe that this is a usable solution.

Comment on lines +52 to +54
As an improvement we can add a annotation similar as described above containing
a comma separated list of DNS names. This is then used as an override for the
DNS names when generating the manifest.
Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to make life as easy as possible for users, we can also think about doing a best-effort analysis of the selectors used by services and try to match them to policies.

Confidential pods should contain an environment value that contains a list
of additional DNS names the pod should receive.

## The Problem
Copy link
Member

Choose a reason for hiding this comment

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

This section is fundamentally wrong. The coordinator does bind the certificate to an identity, in the form of the attestation report (currently, we embed the full report as bytes, in the future these should be parsed claims). And IMO this is the only valid identity in a confidential service mesh.

DNS names in the service mesh shouldn't, and IMO cannot, be used as workload identity, nor as a security feature/aspect im general. This would require a trusted DNS resolver at least. DNS names in our service mesh are rather a convenience feature that enables interaction with tools that are picky and don't like wildcard certs.

## The Problem

We currently make use of certificates to distinguish between attested pods that
are part of one mesh and unattested pods. The coordinator who singes the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
are part of one mesh and unattested pods. The coordinator who singes the
are part of one mesh and unattested pods. The coordinator who signs the

Comment on lines +34 to +35
The environment value is read by the initializer init container and used as
argument for the `NewMeshCert()` call.
Copy link
Contributor

@malt3 malt3 Jan 30, 2024

Choose a reason for hiding this comment

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

In theory, the coordinator could still validate requests using the EDG_DNS_NAMES variable since it is part of the policy and only return certs that were specified there. Not sure if I like this design.

Comment on lines +48 to +50
Let the manifest set by the user inside the coordinator contain a mapping from
workload policy hash to DNS names. When creating the manifest we map each
workload to `*`. This can then be changed later by the user.
Copy link
Contributor

Choose a reason for hiding this comment

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

@3u13r does this have obvious downsides that we are not seeing? I also believe that this is a usable solution.

@3u13r
Copy link
Member Author

3u13r commented Jan 30, 2024

Summary from in person discussion:

  • Issue wildcard certificates by default
  • Let the user override dnsnames in the manifest

This enables users to implement custom authentication on top of what we provide.
Since this is closely aligned with what we already have, close this PR and implement the missing steps.

@3u13r 3u13r closed this Jan 30, 2024
@katexochen katexochen deleted the rfc/002-dns-names branch July 1, 2024 14:27
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