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

[TestResources] Doc Updates and Revisions for External Use #1983

Merged
3 commits merged into from
Sep 13, 2021

Conversation

jsquire
Copy link
Member

@jsquire jsquire commented Sep 6, 2021

Summary

The focus of these changes is to revise the script to better support use by external contributors and others outside of the Azure SDK ecosystem and without access to the Microsoft AAD Tenant.

Details

  • Creation of a new Test Application service principal is now possible from a non-Microsoft AAD tenant.

  • When a new Test Application principal is created, the principle of least privilege is now applied; the new Test Application is granted ownership of the resource group associated with the test resources and no longer has access to any other resources in the subscription.

  • If an existing Test Application principal is specified, it will be assigned ownership of the resource group created. This supports using a Test Application principal without privileges at the subscription-level.

  • When no provisioner is specified, the script is now executed in the context of the caller rather than the Test Application principal. This supports using a Test Application principal that has restricted privileges and better aligns to the purpose of the Test Application principal.

  • The $TestApplicationOid is now explicitly bound at the time a new Test Application principal is created rather than having to query for it later.

  • The Object Id of the provisioner, either the specified $ProvisionerApplicationId or the interactive user, is now passed to the template as part of the deployment parameters.

  • Common error scenarios resulting from lack of permissions now provide messaging with more context of why the failure occurred and suggest remediation.

  • Added new examples to illustrate the common call patterns needed by external contributors running the script outside of the Microsoft tenant and Azure SDK ecosystem.

  • Documentation has been enhanced with additional context to detail the permissions and roles assigned by the script.

  • Added documentation details for Bicep template use.

@jsquire jsquire added EngSys This issue is impacting the engineering system. Client This issue points to a problem in the data-plane of the library. labels Sep 6, 2021
@jsquire jsquire self-assigned this Sep 6, 2021
@azure-sdk
Copy link
Collaborator

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

@azure-sdk
Copy link
Collaborator

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

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

Discussed offline. Let's proceed through the pipeline and create the per-language repo PRs. After that - maybe across a sampling of language/service combinations - run live pipelines across the following services at least:

  • Storage
  • Event Hubs
  • Key Vault

I'll do a manual run with Key Vault as well since I was about to anyway to record some new test cases.

@heaths
Copy link
Member

heaths commented Sep 9, 2021

Key Vault's test-resources-post.ps1 is currently broken because we set the TestApplicationOid as the initial Managed HSM owner, which is a hardened security feature. This needs to include the provisioner as well because the post script needs to activate the domain. We either log in as the test application, or we add the Provisioner* properties to the $templateParameters passed to New-AzResourceGroupDeployment.

I might also be able to work around this with a pre-script that adds values to $templateFileParameters (should be in scope, I believe), but I think it better to just pass the provisioner properties through to the deployment via $templateParameters.

@jsquire
Copy link
Member Author

jsquire commented Sep 9, 2021

Key Vault's test-resources-post.ps1 is currently broken because we set the TestApplicationOid as the initial Managed HSM owner, which is a hardened security feature. This needs to include the provisioner as well because the post script needs to activate the domain. We either log in as the test application, or we add the Provisioner* properties to the $templateParameters passed to New-AzResourceGroupDeployment.

What's involved with activating the domain? If we assign the provisioner (either SP or the interactive user) as the initial owner and the post- script is running in the context of the same provisioner, would that work or is there something that needs to be done with the provisioner's identifying information?

If I'm reading the docs correctly, if we execute the ExportAzKeyVaultSecurityDomain cmdlet in the context of the provisioner (SP or interactive user), since it was tagged as the admin, that should work?

My assumption here is that we're running the pre- and post- scripts in the same context as New-TestResources.ps1 - which I'm not fully convinced is what we're actually doing.

@jsquire
Copy link
Member Author

jsquire commented Sep 9, 2021

Key Vault's test-resources-post.ps1 is currently broken because we set the TestApplicationOid as the initial Managed HSM owner, which is a hardened security feature. This needs to include the provisioner as well because the post script needs to activate the domain. We either log in as the test application, or we add the Provisioner* properties to the $templateParameters passed to New-AzResourceGroupDeployment.

An alternate thought: If we continue to assign the test application as the administrator of the HSM, couldn't we have the post- script log into the test app, authorize the HSM, and then restore the calling context? The test application will have Owner rights on the RG as well and I can't think of a reason why that wouldn't work.

@jsquire
Copy link
Member Author

jsquire commented Sep 9, 2021

GitHub isn't showing the changes to the markdown file as a diff. What did you change?

L592, changed http to https because Analyze was complaining.

@heaths
Copy link
Member

heaths commented Sep 9, 2021

Right - everything during this script is now run as the provisioner (explicit or current context), which is fine. The problem was that the Key Vault test-resources.json and test-resources-post.ps1 assumed the test application likely was that provisioner. Not sure, now that I've run into this problem after your (perfectly fine) changes, how it was working during live test runs by the CI which presumably passed different contexts. Or maybe it didn't. @benbp?

@benbp
Copy link
Member

benbp commented Sep 9, 2021

@heaths not quite sure what you are asking, but CI uses the same SP values for test application and provisioner.

@heaths
Copy link
Member

heaths commented Sep 9, 2021

@benbp thanks, that explains it. Test app and provisioner were assumed to be the same, but that's no longer true (which is a good change, I agree).

heaths added a commit to heaths/azure-sdk-for-net that referenced this pull request Sep 9, 2021
@azure-sdk
Copy link
Collaborator

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

jsquire and others added 2 commits September 10, 2021 09:52
The focus of these changes is to revise the script to better support use
by external contributors and others outside of the Azure SDK ecosystem and
without access to the Microsoft AAD Tenant.

Changes include:

- Creation of a new Test Application service principal is now possible
  from a non-Microsoft AAD tenant.

- When a new Test Application principal is created, the principle of least
  privilege is now applied; the new Test Application is granted ownership
  of the resource group associated with the test resources and no longer
  has access to any other resources in the subscription.

- If an existing Test Application principal is specified, it will be
  assigned ownership of the resource group created.  This supports using
  a Test Application principal without privileges at the subscription-level.

- When no provisioner is specified, the script is now executed in the
  context of the caller rather than the Test Application principal.
  This supports using a Test Application principal that has restricted
  privileges and better aligns to the purpose of the Test Application
  principal.

- The `$TestApplicationOid` is now explicitly bound at the time a new Test
  Application principal is created rather than having to query for it later.

- Common error scenarios resulting from lack of permissions now provide
  messaging with more context of why the failure occurred and suggest
  remediation.

- Added new examples to illustrate the common call patterns needed by
  external contributors running the script, outside of the Microsoft tenant
  and Azure SDK ecosystem.

- Documentation has been enhanced with additional context to detail the
  permissions and roles assigned by the script.

- Added documentation details for Bicep template use.
Key Vault needs this to deploy Managed HSMs. There's a corresponding change necessary in test-resources.json I'll roll out across languages.
@azure-sdk
Copy link
Collaborator

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

Copy link
Member

@benbp benbp left a comment

Choose a reason for hiding this comment

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

One property question and some spelling fixes, but otherwise LGTM.

@jsquire
Copy link
Member Author

jsquire commented Sep 10, 2021

One property question and some spelling fixes, but otherwise LGTM.

If there weren't typos, nobody would believe that I wrote it. 😄

@azure-sdk
Copy link
Collaborator

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

@heaths
Copy link
Member

heaths commented Sep 10, 2021

Fixing typos and spelling mistakes

Would've been funnier if you misspelled your commit title.

@ghost
Copy link

ghost commented Sep 13, 2021

Hello @azure-sdk!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@jsquire
Copy link
Member Author

jsquire commented Sep 13, 2021

/check-enforcer evaluate

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. EngSys This issue is impacting the engineering system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants