incorporating aasp code into this repo #26

stevendongatmsft · 2023-04-11T18:40:12Z

This PR is for incorporating aasp repo into the SKR repo

cmd/aasp/main.go

BryceDFisher · 2023-04-12T13:58:20Z

cmd/aasp/main.go

+
+	// bearerToken := ""
+
+	// // Note: obtaining access token through federated token file is AKS specific


And only if workload identities are implemented. If the customer configures cluster identity or pod identity (deprecated) the below doesn't apply.

This comment is not correct. Any environment can login via federated tokens if you provide the values below.

cmd/aasp/main.go

BryceDFisher · 2023-04-12T14:32:33Z

pkg/common/crypto.go

@@ -75,3 +79,60 @@ func PrivateKeyFromPEM(privatePEMString string) (*rsa.PrivateKey, error) {
 		return privateWrappingKey, nil
 	}
 }
+
+func RSAPrivateKeyFromJWK(jwKey *jwk.Key) (*rsa.PrivateKey, error) {


Does it make sense to support other key types than RSA? You could return a https://pkg.go.dev/crypto#PrivateKey to more generically handle key types in other functions. JWK even includes a key type parameter. The JWT library you're importing has a JWKS extension that could help you do this as well: https://github.com/MicahParks/keyfunc.

stevendongatmsft · 2023-05-01T14:20:00Z

Talked with Stavros regarding extending support for additional types of keys. Since there has not been a case requiring them, it should not be difficult to extend the current akv/skr package to support these keys. But Imo this should be a separate issue from a different PR.

examples/aasp/README.md

BryceDFisher · 2023-05-01T18:56:40Z

examples/aasp/README.md

+The `setup-key-mhsm.sh` script creates a private/public key pair in the keyvault and a key release policy with the file name `keyname-release-policy.json`. 
+The public key is then downloaded as `keyname-pub.pem` to be used for wrapping the secret. 
+The script also assigns necessary access role(Managed HSM Crypto User) to the Managed Service Idenitty and it also generates a key info file that is later used for unwrapping the secret. 
+Depending on the operating system, make sure the end of line sequence(LF vs CRLF) is set correctly before running the script. 


Are one of these required, or is it system dependent?

I'm not sure what you mean. These are the things users need to make sure before they move onto the secret provisioning.

I would just specify the use of LF is set before running the script, since it can only be run in WSL in Windows or just make a powershell script that does the same stuff for the Windows users.

examples/aasp/README.md

examples/aasp/testkey000-pub.pem

examples/aasp/testkey000-info.json

docker/aasp/Dockerfile.build

examples/aasp/README.md

takuro-sato · 2023-05-25T11:42:51Z

examples/aasp/README.md

+}
+
+```
+Base64 decode the result to see the encrypted secret. 


Can we add a script to deploy and test AASP for future PRs?

$ ./deploy_and_test.sh <resource_group> <and any other parameters> .... OK

deploy_and_test.sh would do everything that this README.md tells (like creating mhsm instance, building docker image, and check the result of unwrap key command).

I am aiming to add an equivalent for attestation-container.

https://github.com/microsoft/confidential-sidecar-containers/blob/2fe9a2d62a7f5e61e57f5101562503580bb42966/docker/attestation-container/deploy_and_test.sh

I would be time consuming to write but it should help us for future PRs (Also it should help to add CI).

In order to automate everything, the following has to be done by the scripts:

The script will make MAA and mHSM endpoint required and reports error if not.

create and import keys, which can be easily done by the setup-key-mhsm.sh script.

wrapping a secret using the aasp binary as a command line tool

build and push docker images with wrapped secret and a script that does the unwrapping copied into the container

setup a nested k8s cluster with /dev/sev support

create k8s secret for container registry access.

deploy the example and make the unwrap scripts automatically run to unwrap the secrets.

Step 5 is currently under development and the way we are testing aasp container is through using peer pod approach. So this won't be 100% complete until we have a full nested Kata K8s environment available. (Currently a dependency outside of our team) I suggest that I can do this on a separate PR and use an issue to track it. But yeah, eventually this script would be beneficial to customers.

examples/aasp/README.md

pkg/aasp/keyprovider/keyprovider.proto

takuro-sato · 2023-05-26T08:25:27Z

pkg/attest/certcache.go

@@ -166,7 +166,7 @@ func (certFetcher CertFetcher) retrieveCertChain(chipID string, reportedTCB uint
 		case "LocalTHIM":
 			uri = fmt.Sprintf(LocalTHIMUriTemplate, certFetcher.Endpoint)
 			// local THIM cert cache endpoint returns THIM Certs object
-			httpResponse, err := common.HTTPGetRequest(uri, false)
+			httpResponse, err := common.HTTPGetRequest(uri, true)


Just question, but could you give me the context of this change?
(Why it's necessary, etc)

So this is for getting the certificate from the THIM endpoint. As far as I know, the THIM endpoint is local so we use http instead of https. But I think I'm going to reverse this change because there is no environment that allows us to use the THIM endpoint right now.

I suspect this ought to go in a separate PR, along with the https->http in the template.

Yeah this is removed for now

cmd/aasp/main.go

takuro-sato · 2023-05-26T08:38:52Z

pkg/msi/msi.go

+// Borrowed from https://github.com/Azure/azure-workload-identity/blob/c155ecee0d9fa681c15ead4bbdce729fd8c99da1/pkg/proxy/proxy.go#L195
+// See tutorial at: https://learn.microsoft.com/en-us/azure/aks/learn/tutorial-kubernetes-workload-identity
+
+func GetAccessTokenFromFederatedToken(ctx context.Context, federatedTokenFile, clientID, tenantID, resource string) (string, error) {


Can we add test for it?

This relies on k8s cluster mount a volume for federated token file. I don't think it's feasible to add a test for this piece of code.

pkg/common/crypto.go

docker/aasp/Dockerfile.build

hgarvison · 2023-06-05T13:10:40Z

pkg/aasp/keyprovider/keyprovider.proto

+}
+
+message HelloRequest {
+  string name = 1;


What's the point of making separate messages just for testing? I would remove the "Hello" req/rep stuff and just use the key/report messages for the grpc test since test code shouldn't be in production code.

hgarvison · 2023-06-05T13:18:07Z

pkg/attest/maa.go

+				SNPReport: base64.URLEncoding.EncodeToString(snpAttestationReport),
+				CertChain: base64.URLEncoding.EncodeToString(vcekCertChain),
+			},
+			Endorsements: base64urlEncodedmaaEndorsement,


Instead of this if-else, I would just set base64urlEncodedmaaEndorsement to the base64 encoding of the byte string (line82), since I believe this will be "" if the uvmInfo == "". Then you can create the maaReport object, setting Endorsements equal to the variable regardless of if you update the base64url endorsement var. Otherwise you have a lot of duplicated code in this if-else.

hgarvison · 2023-06-05T13:22:36Z

pkg/common/crypto.go

+	}
+	n, err := base64.RawURLEncoding.DecodeString(jwkData.N)
+	if err != nil {
+		return nil, errors.Wrapf(err, "Interpretting jwk key element failed")


I would make the error more specific about which key element failed (N in this case) for all these url encodings.

hgarvison · 2023-06-05T13:28:26Z

examples/aasp/README.md

+The `setup-key-mhsm.sh` script creates a private/public key pair in the keyvault and a key release policy with the file name `keyname-release-policy.json`. 
+The public key is then downloaded as `keyname-pub.pem` to be used for wrapping the secret. 
+The script also assigns necessary access role(Managed HSM Crypto User) to the Managed Service Idenitty and it also generates a key info file that is later used for unwrapping the secret. 
+Depending on the operating system, make sure the end of line sequence(LF vs CRLF) is set correctly before running the script. 


I would just specify the use of LF is set before running the script, since it can only be run in WSL in Windows or just make a powershell script that does the same stuff for the Windows users.

hgarvison · 2023-06-05T13:29:41Z

docker/aasp/Dockerfile.sample

@@ -0,0 +1,17 @@
+FROM alpine:3.17.1


I would like a short README here on how these two dockerfiles relate/can be used,