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

To handle nested claims. #10

Open
wants to merge 6 commits into
base: dev
Choose a base branch
from
Open

Conversation

tle15
Copy link

@tle15 tle15 commented Nov 19, 2024

Claims are in nested and in the following format:

"x-ms-azurevm-os-provisioning": {
"node-policy-identity": {
"eventVersion": "1",
"policyId": "openai-whisper",
"signer": "8fe6e7a314b8695b21710cebf0265e8d7bbaabde26f431c407faf16fcbd6b924",
"svn": "1"
},
"os-image-identity": {
"diskId": "singularity.ubuntu-22.04",
"eventVersion": "1",
"signer": "f9cce5b7bdc2aaacfc4c78cb2b7515459aded8149287b74667bb2f178b0cf7b9",
"svn": "1"
}

@beejones
Copy link
Collaborator

This is the current test key release policy caught from the debug endpoint:
{
"type": "add",
"claims": {
"x-ms-ver": [
"1.0"
],
"x-ms-azurevm-debuggersdisabled": [
true
],
"x-ms-azurevm-osversion-major": [
22,
23
],
"x-ms-azurevm-os-provisioning": [
{
"node-policy-identity": {
"eventVersion": "1",
"policyId": "openai-whisper",
"signer": "8fe6e7a314b8695b21710cebf0265e8d7bbaabde26f431c407faf16fcbd6b924"
}
},
{
"node-policy-identity": {
"eventVersion": "2",
"policyId": "openai-whisper",
"signer": "8fe6e7a314b8695b21710cebf0265e8d7bbaabde26f431c407faf16fcbd6b924"
}
}
]
},
"gte": {
"x-ms-azurevm-osversion-major": "22"
},
"gt": {
"x-ms-azurevm-osversion-major": 21
}
}

There are two node-policy-identity claims with different values. This policy will not release a key because this claim is not present in your test token:
"node-policy-identity": {
"eventVersion": "2",

"x-ms-azurevm-os-provisioning": {
"node-policy-identity": {
"eventVersion": "number",
"policyId": "string",
Copy link
Collaborator

Choose a reason for hiding this comment

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

where are these claims tested when they appear in a KRP?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this will eventually become very confusing. E.g. policyId is a claim that we want to compare on value while eventVersion is typically a gte check.
If we use flattened claims it will be clearer and we can easily check if these claim is allowed in CLAIMS:
"x-ms-azurevm-os-provisioning.node-policy-identity.eventVersion"
"x-ms-azurevm-os-provisioning.node-policy-identity.policyId"

Copy link
Collaborator

@beejones beejones left a comment

Choose a reason for hiding this comment

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

We will need unit tests to test the new functionality. See npm run test.
They tests added by Yu are failing. I fixed them but I need to sync.

"x-ms-azurevm-os-provisioning": {
"node-policy-identity": {
"eventVersion": "number",
"policyId": "string",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this will eventually become very confusing. E.g. policyId is a claim that we want to compare on value while eventVersion is typically a gte check.
If we use flattened claims it will be clearer and we can easily check if these claim is allowed in CLAIMS:
"x-ms-azurevm-os-provisioning.node-policy-identity.eventVersion"
"x-ms-azurevm-os-provisioning.node-policy-identity.policyId"

@@ -173,7 +197,7 @@ actions.set(
item.forEach((i) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

in the non-flattened model we should test if the key represents an object and than drill down in the object to check presence in CLAIMS. Way harder than using flattened claims.

@@ -11,4 +11,5 @@ export enum JwtIdentityProviderEnum {
MAA_NoSecureBootTyFu = "https://maanosecureboottestyfu.eus.attest.azure.net",
MAA_NoSecureBootWeu = "https://accnosecurebootmaawesteu.weu.attest.azure.net",
MAA_NoSecureBootEus = "https://accnosecurebootmaa.eus2.attest.azure.net",
MAA_NoSecureBootInteTest = "https://confinfermaaeus2test.eus2.test.attest.azure.net",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have removed the necessity to add an issuer in code. This will not scale. I will sync my changes so we do no longer need this

this.identityProviders.set(
JwtIdentityProviderEnum.MAA_NoSecureBootIntgTest,
new MsJwtProvider("JwtMaaProvider", this.logContext),
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

@beejones
Copy link
Collaborator

I am having build errors doing make build

@tle15
Copy link
Author

tle15 commented Nov 19, 2024

This is the current test key release policy caught from the debug endpoint: { "type": "add", "claims": { "x-ms-ver": [ "1.0" ], "x-ms-azurevm-debuggersdisabled": [ true ], "x-ms-azurevm-osversion-major": [ 22, 23 ], "x-ms-azurevm-os-provisioning": [ { "node-policy-identity": { "eventVersion": "1", "policyId": "openai-whisper", "signer": "8fe6e7a314b8695b21710cebf0265e8d7bbaabde26f431c407faf16fcbd6b924" } }, { "node-policy-identity": { "eventVersion": "2", "policyId": "openai-whisper", "signer": "8fe6e7a314b8695b21710cebf0265e8d7bbaabde26f431c407faf16fcbd6b924" } } ] }, "gte": { "x-ms-azurevm-osversion-major": "22" }, "gt": { "x-ms-azurevm-osversion-major": 21 } }

There are two node-policy-identity claims with different values. This policy will not release a key because this claim is not present in your test token: "node-policy-identity": { "eventVersion": "2",

It should be OR instead of AND, similar to this case: "x-ms-azurevm-osversion-major": [22,23]. So, the token should be valid.

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.

2 participants