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

remove admin_enabled #131

Merged
merged 8 commits into from
Aug 26, 2024
Merged

remove admin_enabled #131

merged 8 commits into from
Aug 26, 2024

Conversation

somesylvie
Copy link
Contributor

@somesylvie somesylvie commented Aug 21, 2024

Description

Fortify wants us to not have admin_enabled set to true on the container registry. This PR changes that setting and turns on container_registry_use_managed_identity instead. To test this, we deployed to both a Flexion environment (this PR env) and a CDC one (dev) and confirmed the app deployed successfully and was running

Issue

CDCgov/trusted-intermediary#1209

Co-Authored-By: jcrichlake <145698165+jcrichlake@users.noreply.github.com>
Co-Authored-By: jherrflexion <118225331+jherrflexion@users.noreply.github.com>
Co-Authored-By: halprin <halprin@users.noreply.github.com>
Co-Authored-By: Samuel Aquino <saquino@flexion.us>
Co-Authored-By: Bella L. Quintero <96704946+pluckyswan@users.noreply.github.com>
Co-Authored-By: jcrichlake <145698165+jcrichlake@users.noreply.github.com>
Co-Authored-By: Bella L. Quintero <96704946+pluckyswan@users.noreply.github.com>
Copy link
Contributor

@pluckyswan pluckyswan left a comment

Choose a reason for hiding this comment

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

Would it be beneficial to include why Fortify wants this?

Copy link
Member

@halprin halprin left a comment

Choose a reason for hiding this comment

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

Just a question on whether we can remove a small bit of code.

@@ -53,6 +52,8 @@ resource "azurerm_linux_web_app" "sftp" {
health_check_path = "/health"
health_check_eviction_time_in_min = 5

container_registry_use_managed_identity = true
Copy link
Member

Choose a reason for hiding this comment

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

Down below on what is currently line 84 and 85, we have...

DOCKER_REGISTRY_SERVER_USERNAME = azurerm_container_registry.registry.admin_username
DOCKER_REGISTRY_SERVER_PASSWORD = azurerm_container_registry.registry.admin_password

Can we remove those lines and this continues to work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can try it and find out 😅

Co-Authored-By: jcrichlake <145698165+jcrichlake@users.noreply.github.com>
Co-Authored-By: Bella L. Quintero <96704946+pluckyswan@users.noreply.github.com>
Co-Authored-By: halprin <halprin@users.noreply.github.com>
Co-Authored-By: Samuel Aquino <saquino@flexion.us>
Co-Authored-By: jcrichlake <145698165+jcrichlake@users.noreply.github.com>
Co-Authored-By: Bella L. Quintero <96704946+pluckyswan@users.noreply.github.com>
Co-Authored-By: halprin <halprin@users.noreply.github.com>
Co-Authored-By: Samuel Aquino <saquino@flexion.us>
Co-Authored-By: jcrichlake <145698165+jcrichlake@users.noreply.github.com>
Co-Authored-By: Bella L. Quintero <96704946+pluckyswan@users.noreply.github.com>
Co-Authored-By: halprin <halprin@users.noreply.github.com>
Co-Authored-By: Samuel Aquino <saquino@flexion.us>
Co-Authored-By: jcrichlake <145698165+jcrichlake@users.noreply.github.com>
Co-Authored-By: Bella L. Quintero <96704946+pluckyswan@users.noreply.github.com>
Co-Authored-By: halprin <halprin@users.noreply.github.com>
Co-Authored-By: Samuel Aquino <saquino@flexion.us>
Co-authored-by: saquino0827 <saquino@flexion.us>
@somesylvie
Copy link
Contributor Author

Would it be beneficial to include why Fortify wants this?

The Fortify rec is actually pretty vague - this is all it says
image

Copy link

sonarcloud bot commented Aug 26, 2024

@somesylvie somesylvie merged commit a25f645 into main Aug 26, 2024
14 checks passed
@somesylvie somesylvie deleted the turn-off-admin_enabled branch August 26, 2024 16:30
somesylvie added a commit that referenced this pull request Aug 26, 2024
somesylvie added a commit that referenced this pull request Aug 26, 2024
This reverts commit a25f645 - we need add'l permissions from CDC for this to work in staging
somesylvie added a commit that referenced this pull request Aug 26, 2024
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.

3 participants