Disable and remove use of local/shared key auth for SAs #4122
Conversation
fahlmant
requested review from
jewzaam,
bennerv,
hawkowl,
rogbas,
petrkotas,
jharrington22,
cblecker,
cadenmarchese,
UlrichSchlueter,
SudoBrendan,
yjst2012,
jaitaiwan,
anshulvermapatel,
hlipsig,
tiguelu,
mociarain,
kimorris27,
tsatam,
bitoku and
sankur-codes
as code owners
There was a problem hiding this comment.
Do you plan to test this with a full service dev RP before merge? I'm not sure if it's worth the effort.
fahlmant
force-pushed
the
ARO-14659
branch
3 times, most recently
from
5261d3b to
d01e9bb
Compare
|
@kimorris27 No, I ran a INT Release pipeline against the PR to ensure that the version upload works as expected, so I agree a full deployment test is overkill. |
|
Please rebase pull request. |
| d.log.Infof("instantiating blobs client using SAS token for ensure static web content is enabled") | ||
| serviceUrl := fmt.Sprintf("https://%s.blob.%s", *d.config.Configuration.RPVersionStorageAccountName, d.env.Environment().StorageEndpointSuffix) | ||
| blobsClient, err := azblob.NewBlobsClientUsingEntra(serviceUrl, tokenCredential, d.env.Environment().ArmClientOptions()) |
There was a problem hiding this comment.
Maybe instead of constructing the blobsClient in the SaveVersion function, we should construct it in the deployer similar to what we do for clusterKeyvault, from a consistency perspective. Then we don't have to pass a token everywhere.
Thoughts? Non-blocking, just a comment.
There was a problem hiding this comment.
Same would apply for uploadBlobsClient below as well.
pkg/deploy/saveversion.go
Outdated
| d.log.Infof("instantiating blobs client using SAS token to upload content") | ||
| containerUrl := fmt.Sprintf("https://%s.blob.%s/%s", *d.config.Configuration.RPVersionStorageAccountName, d.env.Environment().StorageEndpointSuffix, "$web") | ||
| uploadBlobsClient, err := azblob.NewBlobsClientUsingEntra(containerUrl, tokenCredential, d.env.Environment().ArmClientOptions()) | ||
| if err != nil { | ||
| d.log.Errorf("failure to instantiate blobs client using SAS: %v", err) | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
why is there a separate blobs client. Can we not use the previous one? Is there something special about the new service URL containing the $web container, even though we reference it in upload buffer
pkg/deploy/generator/resources_rp.go
Outdated
| rbac.RoleStorageBlobDataContributor, | ||
| "parameters('globalDevopsServicePrincipalId')", | ||
| resourceTypeStorageAccount, | ||
| storageAccountName, |
There was a problem hiding this comment.
Do we want to set the role at the scope of the $web container instead of the whole storage account?
az role assignment create \
--role "Storage Blob Data Contributor" \
--assignee <email> \
--scope "/subscriptions/<subscription-id>/resourceGroups/<resource-group-name>/providers/Microsoft.Storage/storageAccounts/<storage-account-name>/blobServices/default/containers/<container-name>"
2d78714 to
de10843
Compare
fahlmant
force-pushed
the
ARO-14659
branch
9 times, most recently
from
63ec987 to
b9d3e6c
Compare
Change access to SA in saveversion to use entra credentials Appending to the blob url based on what's done in oidcbuilder.go Add RoleStorageBlobDataContributor
Which issue this PR addresses:
https://issues.redhat.com/browse/ARO-14659
What this PR does / why we need it:
As part of security wave, all SAs need to disable local/shared key auth.