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

Updating Terraform Azure Service Principal PRIME-ReportStream-Terraform #17033

Merged
merged 6 commits into from
Jan 13, 2025

Conversation

emvaldes
Copy link
Collaborator

@emvaldes emvaldes commented Jan 9, 2025

This PR addresses the issue of having to implement a new Azure Service Principal to handle all Terraform operations within the PRIME-ReportStream domain. The new Azure Service Principal ID is: PRIME-ReportStream-Terraform.

A contributor light role was added to the following resource groups for the new SP:

prime-data-hub-demo, 
prime-data-hub-demo1, 
prime-data-hub-demo2, 
prime-data-hub-demo3, 
prime-data-hub-prod, 
prime-data-hub-staging, 
prime-data-hub-test

Setup access policies for the new Service Principal on the following key vaults was completed with the same permissions as with the original terraform-automation (original Azure Service Principal):

pdhdemo1-appconfigs5m, 
pdhdemo1-keyvaults5m, 
pdhdemo2-appconfig1wu, 
pdhdemo2-keyvault1wu, 
pdhdemo3-appconfigjeo, 
pdhdemo3-keyvaultjeo, 
pdhprod-appconfig, 
pdhprod-keyvault, 
pdhstaging-appconfig, 
pdhstaging-keyvault, 
pdhtest-app-config, 
pdhtest-keyvault

Although this PR does not fix any currently exploitable issue, it does address one concern with the setup for the legacy Azure Service Principal that was used until now (terraform-automation)

Warnings: After some investigation, the original Azure Service Principal (terraform-automation) had a contributor role, but it's at the Subscription level. The team cannot grant either the role nor the location for that without approval from higher up. They expressed their surprise that the original team was able to have those privs within the DMZ.

Objective: Because of this new role privileges that are granted to this new Azure Service Principal, it's possible that it might require more elevated privileges and we will determine that right now in this Pull Request.


Although this PR does not address these issues, it's important to mention that there are lots of Azure account related information that is hardcoded into the code-base. Something that should eventually be remediated.

We should not be supporting or enforcing such practices but instead do all we can to bring the code-base to an example of good standards to follow. Hardcoding anything in a Software Configuration Management is just not right regardless.

Instead of maintaining a bad practice, this is what we should be doing either via secrets or environment variables:
e.g.: In your Terraform modules, define variables without assigning default values.

variable "subscription_id" {
  description = "Azure Subscription ID"
  type        = string
}

e.g.: Enforcing the scope of these values as environment values that will be available to the Terraform engine during run-time. Making it a flexible and agnostic process.

jobs:
  deploy:
    runs-on: ubuntu-latest
    env:
      ARM_SUBSCRIPTION_ID: ${{ secrets.AZURE_SUBSCRIPTION_ID }}
      ARM_CLIENT_ID: ${{ secrets.AZURE_CLIENT_ID }}
      ARM_CLIENT_SECRET: ${{ secrets.AZURE_CLIENT_SECRET }}
      ARM_TENANT_ID: ${{ secrets.AZURE_TENANT_ID }}
    steps:
      - name: Checkout code
        uses: actions/checkout@v4

      - name: Set up Terraform
        uses: hashicorp/setup-terraform@v3

      - name: Initialize Terraform
        run: terraform init

      - name: Apply Terraform configuration
        run: terraform apply -auto-approve

By not specifying the these values directly in the provider block, Terraform will reference the ARM_SUBSCRIPTION_ID environment variable during execution.
This method keeps your configuration clean and free from hardcoded sensitive data.

Important Considerations:

  • Mandatory Subscription ID: As of AzureRM provider version 4.0.0, specifying the subscription_id is mandatory. If it's not provided in the provider block, Terraform will look for the ARM_SUBSCRIPTION_ID environment variable. Ensure this environment variable is set to avoid errors during plan or apply operations.

  • Environment Variable Precedence: The AzureRM provider gives precedence to environment variables for authentication details. If both the provider block and environment variables specify a subscription_id, the provider block value will take precedence. Therefore, ensure consistency between your environment variables and provider configurations.

  • Security Best Practices: Storing sensitive information in environment variables, especially within CI/CD pipelines, is a common practice. However, always use secure storage solutions like GitHub Secrets to manage these variables, preventing unauthorized access.

By following this approach, you maintain a secure and flexible Terraform configuration, free from hardcoded sensitive information, and adaptable across different environments.


In Azure, identifiers such as subscription_id, client_id, and tenant_id are generally not considered highly sensitive on their own. However, when combined with other information, they can potentially be misused.

Potential Risks:

Information Disclosure: While these IDs alone don't grant access, they can reveal details about your Azure environment. For instance, a subscription_id can indicate the existence of a subscription, and a tenant_id can disclose your Azure Active Directory (AAD) tenant.
MICROSOFT LEARN

Targeted Attacks: Malicious actors could use these IDs to craft targeted phishing attacks or social engineering schemes, aiming to extract more sensitive information or credentials from your organization.

Credential Combination: If an attacker obtains these IDs along with other credentials, such as a client_secret or certificate, they could potentially authenticate as a service principal in your Azure environment, leading to unauthorized access.
SECUREWORKS

Recommendations:

Limit Exposure: Avoid sharing these identifiers publicly or with untrusted parties. While they are not highly sensitive, minimizing their exposure reduces the risk of them being used in conjunction with other compromised information.

Secure Associated Credentials: Ensure that any credentials associated with these IDs, such as client_secrets or certificates, are stored securely using services like Azure Key Vault. This practice helps prevent unauthorized access even if the IDs are known.
MICROSOFT LEARN

Monitor and Audit: Regularly monitor access logs and audit trails for any suspicious activities related to these IDs. Implementing Azure's monitoring tools can help detect and respond to potential threats promptly.

In summary, while subscription_id, client_id, and tenant_id are not inherently sensitive, they should still be handled with care to prevent potential misuse, especially when combined with other confidential information.

Checklist

Process

  • Are there licensing issues with any new dependencies introduced? No
  • Includes a summary of what a code reviewer should test/verify? No
  • Updated the release notes? No
  • Database changes are submitted as a separate PR? No
  • DevOps team has been notified if PR requires ops support? We are the team performing this PR

Specific Security-related subjects a reviewer should pay specific attention to

  • Does this PR introduce new endpoints? No

  • Does this PR include changes in authentication and/or authorization of existing endpoints? No

  • Does this change introduce new dependencies that need vetting? No

  • Does this change require changes to our infrastructure? No

  • Does logging contain sensitive data? No

  • Does this PR include or remove any sensitive information itself? No

  • What are the potential security threats and mitigations? N/A

  • Have you ensured logging does not contain sensitive data? N/A

  • Have you received any additional approvals needed for this change? N/A

@emvaldes emvaldes added tech-debt Anything that is purely a technical issue and does not affect functionality github-actions Tracking GitHub Actions items documentation Tickets that add documentation on existing features and services reportstream engineering Work to be completed by an engineer infrastructure Updates and additions to application infrastructure DevSecOps Team Aq DevSecOps work label github-workflows Tracking GitHub Workflows items github-secrets Tracking GitHub Secrets items github-variables Tracking GitHub Variables items labels Jan 9, 2025
@emvaldes emvaldes requested a review from devopsmatt January 9, 2025 19:40
@emvaldes emvaldes self-assigned this Jan 9, 2025
@emvaldes emvaldes requested review from a team as code owners January 9, 2025 19:40
Copy link
Contributor

github-actions bot commented Jan 9, 2025

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

OpenSSF Scorecard

PackageVersionScoreDetails

Scanned Manifest Files

@emvaldes emvaldes added the terraform Pull requests that update Terraform code label Jan 9, 2025
@devopsmatt
Copy link
Collaborator

The object id strings should be static to conform to the previously established TF standard.

Copy link
Collaborator

@devopsmatt devopsmatt left a comment

Choose a reason for hiding this comment

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

This parametrization does not conform to the previously established standard.

@emvaldes
Copy link
Collaborator Author

emvaldes commented Jan 10, 2025

This parametrization does not conform to the previously established standard.

There is no previously established standard other than a terrible practice of hardcoding values that should always be parameterized as GitHub Environment variables to be passed via the GitHub Workflow.

Note: This crude strategy goes against any acceptable industry standard and best practices. Among other things because it makes it difficult for operations to modify parameters that are not related to any SDLC workflows.

@emvaldes emvaldes requested review from devopsmatt and removed request for devopsmatt January 13, 2025 22:17
Copy link
Collaborator

@arnejduranovic arnejduranovic left a comment

Choose a reason for hiding this comment

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

Please consider adding documentation in sharepoint on how this is all setup and designed.

@devopsmatt devopsmatt merged commit fbede22 into main Jan 13, 2025
26 checks passed
@devopsmatt devopsmatt deleted the devsecops/emvaldes/terraform-service-principal branch January 13, 2025 22:31
@emvaldes emvaldes added this to the done milestone Jan 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DevSecOps Team Aq DevSecOps work label documentation Tickets that add documentation on existing features and services engineering Work to be completed by an engineer github-actions Tracking GitHub Actions items github-secrets Tracking GitHub Secrets items github-variables Tracking GitHub Variables items github-workflows Tracking GitHub Workflows items infrastructure Updates and additions to application infrastructure reportstream tech-debt Anything that is purely a technical issue and does not affect functionality terraform Pull requests that update Terraform code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants