search

Azure / simdem

Tool for Simulating Demo's, delivering Tutorials and using documentation as tests.
MIT License
34 stars 17 forks source link

Moved notice to line 3. Removed the code 'may break unexpectedly' com… #131

Closed colinmixonn closed 2 years ago

colinmixonn commented 2 years ago

…ment

colinmixonn commented 2 years ago

@rgardler-msft, I did all the checks I know to confirm I only changed 1 file. So, unsure why this README change did not pass CI check?

rgardler-msft commented 2 years ago

@jasonmesser7 can you take a look at this. CircleCI is reporting:

#!/bin/bash -eo pipefail
az login \
  --service-principal \
  --tenant $AZURE_SP_TENANT \
  -u $AZURE_SP \
  -p $AZURE_SP_PASSWORD

argument --tenant/-t: expected one argument

Looks like $AZURE_SP_TENANT is not set. Probably a glitch in the request but we should be aware of what happened in case we see it multiple times.

rgardler-msft commented 2 years ago

I'm going to go ahead and merge anyway. The file looks great and its a trivial change.

jasonmesser7 commented 2 years ago

@rgardler-msft I suspect this is something CircleCI does within a PR to protect the project and the credentials/service principles from malicious attackers. When the PR was merged it into main the test ran correctly and passed on CircleCI.

What I suspect is that CircleCI doesn't give permission in the PR to access the core projects environment variables. For example, what if in the PR someone decided to try and attack Azure by deploying 1 million VMs in the test file. If a PR had access to our environment variables and thus service principle, then a malicious attack could simply create a PR and spend on Azure through our SP.

We may need to update the Readme to specify that in order for a PR to pass the tests a user will have to set up CircleCI in their fork with a service principle from their own Azure subscription.

I suspect why we didn't have this issue before is because all of the other PR's were from branches on the main project we were working on as a contributor and Colins was the first outside PR since CircleCI was set up.

rgardler-msft commented 2 years ago

Lets not worry about it for now. We will move to GitHub actions in the future and the solution may be different at that point.

In the interim we can run any code change merges through local tests before merging.

Ross

From: jasonmesser7 @.> Sent: Friday, August 12, 2022 10:49 To: Azure/simdem @.> Cc: Ross Gardler (HE/THEY) @.>; Mention @.> Subject: Re: [Azure/simdem] Moved notice to line 3. Removed the code 'may break unexpectedly' com… (PR #131)

@rgardler-msfthttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Frgardler-msft&data=05%7C01%7Cross.gardler%40microsoft.com%7C66bc5e05c8284ea0364d08da7c8afb48%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637959233606390809%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=OlA%2F5uwkO76l%2BSJCe6MooH20YElmQq5lZLafPUcTAjw%3D&reserved=0 I suspect this is something CircleCI does within a PR to protect the project and the credentials/service principles from malicious attackers. When the PR was merged it into main the test ran correctly and passed on CircleCI.

What I suspect is that CircleCI doesn't give permission in the PR to access the core projects environment variables. For example, what if in the PR someone decided to try and attack Azure by deploying 1 million VMs in the test file. If a PR had access to our environment variables and thus service principle, then a malicious attack could simply create a PR and spend on Azure through our SP.

We may need to update the Readme to specify that in order for a PR to pass the tests a user will have to set up CircleCI in their fork with a service principle from their own Azure subscription.

I suspect why we didn't have this issue before is because all of the other PR's were from branches on the main project we were working on as a contributor and Colins was the first outside PR since CircleCI was set up.

— Reply to this email directly, view it on GitHubhttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAzure%2Fsimdem%2Fpull%2F131%23issuecomment-1213365407&data=05%7C01%7Cross.gardler%40microsoft.com%7C66bc5e05c8284ea0364d08da7c8afb48%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637959233606390809%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=B1k4wH6%2BbRqFT1WTurdYb9jDWBXEjHgINS77yXxNW%2F0%3D&reserved=0, or unsubscribehttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAZ5Z7CSFP756YXUG73OXGHLVY2FB5ANCNFSM56JW5HDQ&data=05%7C01%7Cross.gardler%40microsoft.com%7C66bc5e05c8284ea0364d08da7c8afb48%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637959233606390809%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=7UYc7T4Osfq6k2JfShRhR4WTOa5XZxghqk1bQedPDzQ%3D&reserved=0. You are receiving this because you were mentioned.Message ID: @.***>