feat: auto detect tee env #29

fnerdman · 2025-01-28T08:36:03Z

I think this patch needs some clarification:

This patch implements two types of automatism in terms of figuring out the right attestation-type to choose.

attestation type of the locally generated quote, which is only available within a TDX VM and which will be served as TLS certificated during the handshake to the other party
attestation type of the remote party. During the TLS handshake, well be provided with an aTLS certificate, how do we know it's attestation type such that we can find the right validator to verify this certificate?

What was the status quo before this patch?

The attestation type of the locally generated quote was set manually, and logic would direct to the correct methods that would setup the aTLS cert needed for the handshake. If "none" was specified, the logic would skip serving an aTLS cert during handshake.
The attestation type of the remote party was set manually. It was only possible to set one type of attestation, thus only one type of validator was possible (either azure or dcap)

What does this patch provide?

The attestation type of the locally generated quote is detected automatically by checking properties that are only available in these environments. If no such environment is detected the logic will default to not presenting an aTLS cert. This is the default behavior but we can "force" a specific attestation type via cmdline flag. I think auto is the right default flag for this type, as it fits "none" in all cases when no TEE is present, and selects the right TEE attestation type if a TEE is present.
The attestation type of the remote party can be detected via the OID parameter which is part of the remote party aTLS certificate. I don't think it makes sense to keep the attestation type flag since the remote party will advertise the type of attestation it is providing. We should now rather distinguish between these 3 use cases:

First, the remote attestation shall not be verified but be ignored. This is covered by leaving the measurements.json empty. No measurements provided will lead to attestation being ignored.

Second, only the non app specific parts of the remote attestations shall be verified (TCB level), the rest will be passed through as http headers. This is covered by specifying the validators that should be uses during the handshake attestation in the measurements file, but without describing app specific measurements (PCRs/MRTD/RTMRs).

Third, only the exact measurements provided in the measurements.json file should be used for verification. This is covered by specifying the exact measurements required in the json file. This patch will automatically handle different attestation types and use the correct validator based on the OID provided by the remote party.

cmd/proxy-client/main.go

cmd/proxy-server/main.go

metachris · 2025-01-28T09:12:24Z

cmd/proxy-server/main.go

+		} else {
+			log.With("attestation-type", cCtx.String("server-attestation-type")).Error("invalid server-attestation-type passed, see --help")
+			return err
+		}
 	}

 	clientAttestationType, err := proxy.ParseAttestationType(cCtx.String("client-attestation-type"))


can client-attestation-type also be detected? ideally we'd run a single server proxy instance that works for all client attestation types

Yes, I've added this feat. Client attestation type is now determined via the measurements.json. During TLS handshake attestation the attester will check the OID of the attestee and then select the measurements it has in store for this OID, and tries to verify if they are a match. This works vice versa for the proxy-client, it attests servers based on measurement attestation types as well.

proxy/atls_config.go

cmd/proxy-client/main.go

fnerdman · 2025-01-30T10:20:36Z

cmd/proxy-client/main.go

-		log.Error("invalid combination of --verify-tls and --server-attestation-type passed (only 'none' is allowed)")
-		return errors.New("invalid combination of --verify-tls and --server-attestation-type passed (only 'none' is allowed)")
+	if serverMeasurements != "" && verifyTLS {
+		log.Error("invalid combination of --verify-tls and --server-measurements passed (cannot add server measurements and verify default TLS at the same time)")


I can't check for server-attestation-type anymore since the type was removed as it is automatically selected based on OID during handshake.
Checking if serverMeasurements are set is an alternative, as serverMeasurements will include the measurements and attestationTypes we approve.

I am not sure about the differences and purpose of the checks before and now after. But would using the auto detect type here help instead and check if it is not "none"?
What is this check for? (just for my understanding)

so as a client, you are building up a connection to a server. That server will either provide a ca signed TLS cert, or a self signed aTLS cert. This check is just to make sure what you are doing, and prevent a configuration where you accidentally set verifyTLS but actually you wanted to verify measurements. Taking this example, if this check wouldn't be there, the client would verify a signed TLS cert and not the TEE attestation, thus undermining security. It's just a clear separation for security purposes.

Similar comment as elsewhere — no expected measurement is not the same as not expected attestation

fnerdman · 2025-01-30T10:31:53Z

I've verified this PR working on Azure, I've tested

auto detecting of TEE environment
selecting the right measurements for attestation verification
I've also verified TDX-DCAP auto detecting by mocking the /dev/tdx-guest device

MoeMahhouk · 2025-01-30T10:45:55Z

proxy/atls_config.go

+func ParseAttestationType(log *slog.Logger, attestationType string) (AttestationType, error) {
 	switch attestationType {
+	case string(AttestationAuto):
+		detectedType := DetectAttestationType()
+		log.With("detected_attestation", detectedType).Info("Auto-detected attestation type")
+		return detectedType, nil


nit: instead of changing the function signature to add logging for the extra new switch case, it could have been handled outside after calling the function and keep the signature as it was before for compatibility and avoid changing it everywhere it was called before.

I'd prefer to forward the log var than adding logic for extracting this log emit of a subcondition that might not be hit.

I don't think "autodetect" should be done here, since it doesn't really make sense to do in some cases (you can't detect someone else's attestation type, while you can parse it just fine from a string)
I think the issue is that autodetect is not really an attestation type, I would probably distinguish between parsing attestation type string to a type from "resolving" auto to a type of attestation.
Resolving the attestation type would make much more sense in CreateAttestationIssuer if you'd want to still fit it in the currently existing functions.

MoeMahhouk · 2025-01-30T10:50:04Z

cmd/proxy-client/main.go

-		log.Error("invalid combination of --verify-tls and --server-attestation-type passed (only 'none' is allowed)")
-		return errors.New("invalid combination of --verify-tls and --server-attestation-type passed (only 'none' is allowed)")
+	if serverMeasurements != "" && verifyTLS {
+		log.Error("invalid combination of --verify-tls and --server-measurements passed (cannot add server measurements and verify default TLS at the same time)")


I am not sure about the differences and purpose of the checks before and now after. But would using the auto detect type here help instead and check if it is not "none"?
What is this check for? (just for my understanding)

fnerdman · 2025-01-30T14:21:01Z

/dev/guest-tdx is going to be deprecated at some point. check for configfs-tsm as well or add it to the icebox for future

metachris · 2025-01-31T13:43:31Z

cmd/proxy-server/main.go

-	}
-
-	validators, err := proxy.CreateAttestationValidators(log, clientAttestationType, clientMeasurements)
+	validators, err := proxy.CreateAttestationValidatorsFromFile(log, clientMeasurements)


wait, this logic seems incorrect. doesn't this need attestation types to be present to actually verify the client attestation? this should not be loaded from a file, but instead from the client TCP connection!

the job of this proxy is NOT to gatekeep, it's to check the attestation and then pass it on for later services to reject or accept this measurement!

the issue is I think confusion of "none" attestation type with no expected measurements.
@fnerdman non-none attestation type with no expected measurements is a valid configuration with the meaning to authenticate the attestation without authorizing it, whereas now it's impossible (with a single measurements flag). What is impossible exactly is the difference between expecting no attestation (error if provided) and expecting an attestation, but not any particular one (error if not provided or if invalid) — although we could probably change "no measurements" to mean "error if provided but not valid". I think the products handle this case correctly — since we will simply not be forwarding any attestation report.

What I'd suggest is to re-introduce the client attestation type flag, and the configuration of "auto" client attestation type with no measurements retains the previous meaning — validate any submitted attestation without expecting any particular one.
The same applies to the client, as the error really is introduced in the first check of CreateAttestationValidatorsFromFile.
The alternative is to change the meaning of "none" attestation type and no measurements to "validate all provided attestations, no attestation — no problem"

Ruteri

It's probably a bit late, but the one alternative that would work very well would be negotiation of attestation type rather than detection — if the client and server exchange allowed measurements, the parties could check if they can provide any of the allowed ones, much like in hashing and encryption algo negotiations.

Regardless, we should verify this doesn't break products before merging (not only that the change works in isolation)

Ruteri · 2025-01-31T14:56:37Z

cmd/proxy-client/main.go

@@ -45,8 +40,8 @@ var flags []cli.Flag = []cli.Flag{
 	},
 	&cli.StringFlag{
 		Name:  "client-attestation-type",
-		Value: string(proxy.AttestationNone),
-		Usage: "type of attestation to present (" + proxy.AvailableAttestationTypes + ")",
+		Value: "auto",


I think the default should still be none — this is mostly still used to verify the server's attestation without providing client's own attestation. "auto" should be an option for the client to select if needed

Ruteri · 2025-01-31T15:08:28Z

cmd/proxy-server/main.go

-	}
-
-	validators, err := proxy.CreateAttestationValidators(log, clientAttestationType, clientMeasurements)
+	validators, err := proxy.CreateAttestationValidatorsFromFile(log, clientMeasurements)


the issue is I think confusion of "none" attestation type with no expected measurements.
@fnerdman non-none attestation type with no expected measurements is a valid configuration with the meaning to authenticate the attestation without authorizing it, whereas now it's impossible (with a single measurements flag). What is impossible exactly is the difference between expecting no attestation (error if provided) and expecting an attestation, but not any particular one (error if not provided or if invalid) — although we could probably change "no measurements" to mean "error if provided but not valid". I think the products handle this case correctly — since we will simply not be forwarding any attestation report.

What I'd suggest is to re-introduce the client attestation type flag, and the configuration of "auto" client attestation type with no measurements retains the previous meaning — validate any submitted attestation without expecting any particular one.
The same applies to the client, as the error really is introduced in the first check of CreateAttestationValidatorsFromFile.
The alternative is to change the meaning of "none" attestation type and no measurements to "validate all provided attestations, no attestation — no problem"

Ruteri · 2025-01-31T15:17:55Z

proxy/atls_config.go

+func ParseAttestationType(log *slog.Logger, attestationType string) (AttestationType, error) {
 	switch attestationType {
+	case string(AttestationAuto):
+		detectedType := DetectAttestationType()
+		log.With("detected_attestation", detectedType).Info("Auto-detected attestation type")
+		return detectedType, nil


I don't think "autodetect" should be done here, since it doesn't really make sense to do in some cases (you can't detect someone else's attestation type, while you can parse it just fine from a string)
I think the issue is that autodetect is not really an attestation type, I would probably distinguish between parsing attestation type string to a type from "resolving" auto to a type of attestation.
Resolving the attestation type would make much more sense in CreateAttestationIssuer if you'd want to still fit it in the currently existing functions.

Ruteri · 2025-01-31T15:21:06Z

cmd/proxy-client/main.go

-		log.Error("invalid combination of --verify-tls and --server-attestation-type passed (only 'none' is allowed)")
-		return errors.New("invalid combination of --verify-tls and --server-attestation-type passed (only 'none' is allowed)")
+	if serverMeasurements != "" && verifyTLS {
+		log.Error("invalid combination of --verify-tls and --server-measurements passed (cannot add server measurements and verify default TLS at the same time)")


Similar comment as elsewhere — no expected measurement is not the same as not expected attestation

Ruteri · 2025-02-03T11:02:25Z

cmd/proxy-client/main.go

@@ -25,11 +25,6 @@ var flags []cli.Flag = []cli.Flag{
 		Value: "https://localhost:80",
 		Usage: "address to proxy requests to",
 	},
-	&cli.StringFlag{


We could always simply deprecate the flag instead of removing it right away — the behavior should be the same and the flag could be ignored until we are confident we can safely remove it

fnerdman · 2025-02-03T16:50:30Z

closing in favour splitting auto detection of issuer (attestation type of the locally generated quote) and validator side (attestation type of the remote party)
issuer side: #32
validator side: #33

feat: auto detect tee env

e36bf5b

fnerdman force-pushed the auto_detect_tee_env branch from 49480b3 to e36bf5b Compare January 28, 2025 08:36

fnerdman requested a review from metachris January 28, 2025 08:37

metachris reviewed Jan 28, 2025

View reviewed changes

cmd/proxy-client/main.go Outdated Show resolved Hide resolved

metachris reviewed Jan 28, 2025

View reviewed changes

cmd/proxy-server/main.go Outdated Show resolved Hide resolved

metachris reviewed Jan 28, 2025

View reviewed changes

MoeMahhouk reviewed Jan 28, 2025

View reviewed changes

proxy/atls_config.go Show resolved Hide resolved

MoeMahhouk reviewed Jan 28, 2025

View reviewed changes

cmd/proxy-client/main.go Outdated Show resolved Hide resolved

fnerdman force-pushed the auto_detect_tee_env branch 2 times, most recently from d4c4207 to a1e15de Compare January 28, 2025 14:48

MoeMahhouk reviewed Jan 28, 2025

View reviewed changes

cmd/proxy-client/main.go Show resolved Hide resolved

feat: fetches validator attestation type from measurments, removes flag

80df1e4

fnerdman force-pushed the auto_detect_tee_env branch from a1e15de to 80df1e4 Compare January 30, 2025 10:12

fnerdman commented Jan 30, 2025

View reviewed changes

MoeMahhouk reviewed Jan 30, 2025

View reviewed changes

metachris reviewed Jan 31, 2025

View reviewed changes

Ruteri requested changes Jan 31, 2025

View reviewed changes

Ruteri reviewed Feb 3, 2025

View reviewed changes

fnerdman closed this Feb 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: auto detect tee env #29

feat: auto detect tee env #29

fnerdman commented Jan 28, 2025 •

edited

Loading

metachris Jan 28, 2025

fnerdman Jan 30, 2025

fnerdman Jan 30, 2025 •

edited

Loading

MoeMahhouk Jan 30, 2025

fnerdman Jan 30, 2025

Ruteri Jan 31, 2025

fnerdman commented Jan 30, 2025

MoeMahhouk Jan 30, 2025

fnerdman Jan 30, 2025

Ruteri Jan 31, 2025 •

edited

Loading

MoeMahhouk Jan 30, 2025

fnerdman commented Jan 30, 2025

metachris Jan 31, 2025

Ruteri Jan 31, 2025 •

edited

Loading

Ruteri left a comment •

edited

Loading

Ruteri Jan 31, 2025 •

edited

Loading

Ruteri Jan 31, 2025 •

edited

Loading

Ruteri Jan 31, 2025 •

edited

Loading

Ruteri Jan 31, 2025

Ruteri Feb 3, 2025

fnerdman commented Feb 3, 2025

feat: auto detect tee env #29

feat: auto detect tee env #29

Conversation

fnerdman commented Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fnerdman Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fnerdman commented Jan 30, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ruteri Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fnerdman commented Jan 30, 2025

Choose a reason for hiding this comment

Ruteri Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Ruteri left a comment • edited Loading

Choose a reason for hiding this comment

Ruteri Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Ruteri Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Ruteri Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fnerdman commented Feb 3, 2025

fnerdman commented Jan 28, 2025 •

edited

Loading

fnerdman Jan 30, 2025 •

edited

Loading

Ruteri Jan 31, 2025 •

edited

Loading

Ruteri Jan 31, 2025 •

edited

Loading

Ruteri left a comment •

edited

Loading

Ruteri Jan 31, 2025 •

edited

Loading

Ruteri Jan 31, 2025 •

edited

Loading

Ruteri Jan 31, 2025 •

edited

Loading