OCPBUGS-37587: Sign blob container using user delegated creds#9851
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@rna-afk: This pull request references Jira Issue OCPBUGS-37587, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
rna-afk
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
rna-afk
force-pushed
the
azure_user_delegated_sas
branch
from
299c68e to
0aa8ea7
Compare
patrickdillon
left a comment
patrickdillon
left a comment
There was a problem hiding this comment.
You can reduce the duplication inside the two create blob functions, by inserting the user delegated sas link creation after the creation of the blob but before the ignition shim is populated, right before this:
installer/pkg/infrastructure/azure/azure.go
Line 787 in 72c6cb7
rna-afk
force-pushed
the
azure_user_delegated_sas
branch
2 times, most recently
from
871ec99 to
a6de9b6
Compare
rna-afk
changed the title
rna-afk
force-pushed
the
azure_user_delegated_sas
branch
from
a6de9b6 to
2bfd640
Compare
patrickdillon
left a comment
patrickdillon
left a comment
There was a problem hiding this comment.
I think we can still remove the auth type variable? We don't need to do any switching on auth type AFAIK.
The azurestack point is a good one, and as always, will complicate this a little. I left some comments to help with this, but I think we want to keep with shared key access on azure stack (and we can use the shared-key sas in that case).
On that note, the azure stack ci is failing and I wasted quite a bit of time trying to troubleshoot it, but I cannot reproduce the error! Will follow up more.
|
/test e2e-azurestack |
rna-afk
force-pushed
the
azure_user_delegated_sas
branch
from
2bfd640 to
e7cd9e2
Compare
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
rna-afk
changed the title
openshift-ci-robot
removed
the
jira/invalid-bug
|
@rna-afk: This pull request references Jira Issue OCPBUGS-37587, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
rna-afk
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
rna-afk
force-pushed
the
azure_user_delegated_sas
branch
from
e7cd9e2 to
fde21c0
Compare
|
/test e2e-azure-ovn |
|
The fact that this requires a new permission complicates how we will handle this, especially if we need to make a backport. If we need a backport, we cannot require a new permission, so we would need to make this opt-in/configurable. And I do believe we need this backported for ARO, right? |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds an optional Azure platform field AllowSharedKeyAccess, updates schema, deepcopy and validation, and refactors Azure provider and storage code to support shared-key or user-delegation (token) blob operations driven by this flag. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.3)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
pkg/infrastructure/azure/storage.go (1)
255-305:⚠️ Potential issue | 🔴 Critical
CreatePageBlobdrops shared-key SAS generation and can leave ignition URL empty.Line 255 always uses token auth, and Lines 297-305 return
""for non-Stack clouds. Inpkg/infrastructure/azure/azure.go, Line 841 only falls back to user-delegation signing when!sharedKey, so CMK + default shared-key flow can produce an empty bootstrap ignition URL.Proposed fix
func CreatePageBlob(ctx context.Context, in *CreatePageBlobInput) (string, error) { logrus.Debugf("Getting page blob client") - - pageBlobClient, err := pageblob.NewClient( - in.BlobURL, - in.TokenCredential, - &pageblob.ClientOptions{ - ClientOptions: azcore.ClientOptions{ - Cloud: in.ClientOpts.Cloud, - }, - }, - ) + var ( + pageBlobClient *pageblob.Client + err error + ) + if in.AllowSharedKeyAccess { + if len(in.StorageAccountKeys) == 0 || in.StorageAccountKeys[0].Value == nil { + return "", fmt.Errorf("missing storage account key for shared-key page blob client") + } + sharedKeyCredential, err := azblob.NewSharedKeyCredential(in.StorageAccountName, *in.StorageAccountKeys[0].Value) + if err != nil { + return "", fmt.Errorf("failed to get shared credentials for storage account: %w", err) + } + pageBlobClient, err = pageblob.NewClientWithSharedKeyCredential( + in.BlobURL, + sharedKeyCredential, + &pageblob.ClientOptions{ + ClientOptions: azcore.ClientOptions{ + Cloud: in.ClientOpts.Cloud, + }, + }, + ) + } else { + pageBlobClient, err = pageblob.NewClient( + in.BlobURL, + in.TokenCredential, + &pageblob.ClientOptions{ + ClientOptions: azcore.ClientOptions{ + Cloud: in.ClientOpts.Cloud, + }, + }, + ) + } if err != nil { return "", fmt.Errorf("failed to get page blob client: %w", err) } @@ - if in.CloudEnvironment == aztypes.StackCloud { - // Is this addition OK for when CreatePageBlob() is called from InfraReady() + if in.AllowSharedKeyAccess { sasURL, err := pageBlobClient.GetSASURL(sas.BlobPermissions{Read: true}, time.Now().Add(time.Minute*60), &blob.GetSASURLOptions{}) if err != nil { return "", fmt.Errorf("failed to get Page Blob SAS URL: %w", err) } return sasURL, nil } return "", nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/infrastructure/azure/storage.go` around lines 255 - 305, CreatePageBlob currently always builds pageblob.NewClient with token auth and only returns a SAS URL for aztypes.StackCloud, which causes an empty bootstrap ignition URL when shared-key signing should be used; modify the flow to (1) choose the correct credential when creating the page blob client: if shared-key credentials are available (the same flag used in azure.go's fallback logic), construct the page blob client with the shared-key credential so shared-key SAS generation is possible, otherwise use the token credential as now; and (2) always produce and return a usable SAS URL for the created blob (not only when CloudEnvironment == aztypes.StackCloud) by calling the appropriate GetSASURL or generating a shared-key SAS (using the shared-key signer) after creating/uploading pages (symbols to update: pageblob.NewClient, pageBlobClient.Create, doUploadPages, doUploadPagesFromURL, pageBlobClient.GetSASURL, and the CloudEnvironment/shared-key flag used in azure.go), ensuring the function never returns an empty bootstrap URL for valid auth configurations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data/data/install.openshift.io_installconfigs.yaml`:
- Around line 5229-5234: The description for allowSharedKeyAccess is inverted;
update the text for the allowSharedKeyAccess field so it correctly states the
semantics: it specifies whether shared key access is allowed (default true), and
setting it to false disables shared key access and requires the "Storage Blob
Data Contributor" permission in Azure; modify the description string near the
allowSharedKeyAccess declaration to reflect this exact wording so docs and
behavior match.
In `@pkg/explain/printer_test.go`:
- Around line 344-348: Update the test expectations in
pkg/explain/printer_test.go to match the actual schema: replace the expected
field name "disallowSharedKeyAccess" with "allowSharedKeyAccess" and change its
expected default value from false to true in the test assertions (search for the
string literal or assertion referencing disallowSharedKeyAccess/default false).
Ensure any related assertion messages or snapshot lines that reference the old
name/value are updated accordingly so the test compares against
allowSharedKeyAccess with default true.
In `@pkg/types/azure/validation/platform.go`:
- Around line 160-162: The validation error uses the wrong JSON field name in
the error path; change fldPath.Child("disallowSharedAccessKey") to
fldPath.Child("allowSharedKeyAccess") so the validation message points to the
actual field; update the append call inside the conditional that checks
p.CloudName == azure.StackCloud && p.AllowSharedKeyAccess != nil &&
!*p.AllowSharedKeyAccess to use fldPath.Child("allowSharedKeyAccess") when
adding the field.Invalid error for p.AllowSharedKeyAccess.
---
Duplicate comments:
In `@pkg/infrastructure/azure/storage.go`:
- Around line 255-305: CreatePageBlob currently always builds pageblob.NewClient
with token auth and only returns a SAS URL for aztypes.StackCloud, which causes
an empty bootstrap ignition URL when shared-key signing should be used; modify
the flow to (1) choose the correct credential when creating the page blob
client: if shared-key credentials are available (the same flag used in
azure.go's fallback logic), construct the page blob client with the shared-key
credential so shared-key SAS generation is possible, otherwise use the token
credential as now; and (2) always produce and return a usable SAS URL for the
created blob (not only when CloudEnvironment == aztypes.StackCloud) by calling
the appropriate GetSASURL or generating a shared-key SAS (using the shared-key
signer) after creating/uploading pages (symbols to update: pageblob.NewClient,
pageBlobClient.Create, doUploadPages, doUploadPagesFromURL,
pageBlobClient.GetSASURL, and the CloudEnvironment/shared-key flag used in
azure.go), ensuring the function never returns an empty bootstrap URL for valid
auth configurations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f9ab0f1-a760-42e0-9fd2-73c03d10367e
📒 Files selected for processing (7)
data/data/install.openshift.io_installconfigs.yamlpkg/explain/printer_test.gopkg/infrastructure/azure/azure.gopkg/infrastructure/azure/storage.gopkg/types/azure/platform.gopkg/types/azure/validation/platform.gopkg/types/azure/zz_generated.deepcopy.go
rna-afk
force-pushed
the
azure_user_delegated_sas
branch
2 times, most recently
from
0fc1b28 to
b58c034
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/infrastructure/azure/storage.go (1)
255-306:⚠️ Potential issue | 🔴 Critical
CreatePageBlobignoresAllowSharedKeyAccessand may return no URL for non-Stack clouds.The function always uses token auth (Line 255 onward) and returns SAS only for StackCloud (Lines 297-305). Downstream impact: the ignition CMK flow in
pkg/infrastructure/azure/azure.go(Lines 825-859) can end up with an empty blob URL.Suggested direction
func CreatePageBlob(ctx context.Context, in *CreatePageBlobInput) (string, error) { - pageBlobClient, err := pageblob.NewClient( - in.BlobURL, - in.TokenCredential, - &pageblob.ClientOptions{ ... }, - ) + var pageBlobClient *pageblob.Client + var err error + if in.AllowSharedKeyAccess { + if len(in.StorageAccountKeys) == 0 || in.StorageAccountKeys[0].Value == nil { + return "", fmt.Errorf("missing storage account key for shared-key page blob upload") + } + sharedKeyCredential, err := azblob.NewSharedKeyCredential(in.StorageAccountName, *in.StorageAccountKeys[0].Value) + if err != nil { + return "", fmt.Errorf("failed to build shared key credential: %w", err) + } + pageBlobClient, err = pageblob.NewClientWithSharedKeyCredential(in.BlobURL, sharedKeyCredential, &pageblob.ClientOptions{ ... }) + } else { + pageBlobClient, err = pageblob.NewClient(in.BlobURL, in.TokenCredential, &pageblob.ClientOptions{ ... }) + } + if err != nil { + return "", fmt.Errorf("failed to get page blob client: %w", err) + } @@ - if in.CloudEnvironment == aztypes.StackCloud { + if in.CloudEnvironment == aztypes.StackCloud || in.AllowSharedKeyAccess { sasURL, err := pageBlobClient.GetSASURL(...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/infrastructure/azure/storage.go` around lines 255 - 306, CreatePageBlob currently always constructs pageblob.NewClient with TokenCredential and only returns a SAS URL for aztypes.StackCloud, causing empty blob URL for other clouds and ignoring AllowSharedKeyAccess; update CreatePageBlob to check in.AllowSharedKeyAccess and if true create the page blob client using a SharedKeyCredential (or construct the client via the account key flow) instead of TokenCredential (identify where pageblob.NewClient is called), and ensure that when shared-key auth is used you return an accessible URL (either the blob URL signed with the shared key or by returning the client URL) so downstream callers (see CreatePageBlob, pageblob.NewClient, AllowSharedKeyAccess, GetSASURL) always receive a usable blob URL even when in.CloudEnvironment != aztypes.StackCloud.
♻️ Duplicate comments (2)
pkg/types/azure/validation/platform.go (1)
160-162:⚠️ Potential issue | 🟡 MinorUse the actual API field name in the validation error path.
Line 161 reports
disallowSharedAccessKey, but the config field isallowSharedKeyAccess. This makes the error output misleading.Suggested fix
- allErrs = append(allErrs, field.Invalid(fldPath.Child("disallowSharedAccessKey"), p.AllowSharedKeyAccess, "disabling shared access key creation is unsupported in Azure stack hub")) + allErrs = append(allErrs, field.Invalid(fldPath.Child("allowSharedKeyAccess"), p.AllowSharedKeyAccess, "disabling shared access key creation is unsupported in Azure Stack Hub"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/types/azure/validation/platform.go` around lines 160 - 162, The validation error uses the wrong API field name in the error path; update the fldPath.Child call in the validation logic that checks p.CloudName == azure.StackCloud and p.AllowSharedKeyAccess to use "allowSharedKeyAccess" (the actual config field) instead of "disallowSharedAccessKey" so the field.Invalid report references the correct field name (look for p.AllowSharedKeyAccess, fldPath.Child("disallowSharedAccessKey") and change the child name to "allowSharedKeyAccess").pkg/infrastructure/azure/azure.go (1)
769-774:⚠️ Potential issue | 🟠 MajorUser-delegation key expires before the SAS token.
Line 770 sets delegation key expiry to 1 hour, while Line 849 creates a SAS valid for 2 hours. The SAS should not outlive the delegation key.
Suggested fix
now := time.Now().UTC().Add(-10 * time.Second) - expiry := now.Add(1 * time.Hour) + expiry := now.Add(1 * time.Hour) @@ sasQueryParams, err := sas.BlobSignatureValues{ Protocol: sas.ProtocolHTTPS, - StartTime: time.Now().UTC().Add(time.Second * -10), - ExpiryTime: time.Now().UTC().Add(2 * time.Hour), + StartTime: now, + ExpiryTime: expiry, Permissions: to.Ptr(sas.ContainerPermissions{Read: true}).String(),Also applies to: 846-850
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/infrastructure/azure/azure.go` around lines 769 - 774, The delegation key expiry (variable expiry and service.KeyInfo Start/Expiry) is set to 1 hour while the SAS token generated later is valid for 2 hours, so update the delegation key expiry to be at least as long as the SAS expiry; compute the delegation key expiry from the same SAS duration constant/variable used when creating the SAS (or set expiry := now.Add(2 * time.Hour)) and populate service.KeyInfo with that value so the user-delegation key does not expire before the SAS.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/infrastructure/azure/storage.go`:
- Around line 488-490: Guard against empty or nil storage keys before
dereferencing in the AllowSharedKeyAccess branch: check that
in.StorageAccountKeys is non-nil and has length > 0 and that
in.StorageAccountKeys[0].Value is non-nil before calling
azblob.NewSharedKeyCredential with in.StorageAccountName and
*in.StorageAccountKeys[0].Value; if any check fails, return or propagate a clear
error (e.g., "missing storage account key") instead of proceeding to
NewSharedKeyCredential.
In `@pkg/types/azure/platform.go`:
- Around line 54-57: The comment for the AllowSharedKeyAccess field is
incorrect; update the doc on the AllowSharedKeyAccess field in
pkg/types/azure/platform.go to state that the field controls whether shared key
access is allowed (true = allowed, default true) and that setting it to false
disables shared key access and will require assigning the "Storage Blob Data
Contributor" permission; keep the wording concise and reference
AllowSharedKeyAccess so readers know the default behavior and consequences of
setting it to false.
---
Outside diff comments:
In `@pkg/infrastructure/azure/storage.go`:
- Around line 255-306: CreatePageBlob currently always constructs
pageblob.NewClient with TokenCredential and only returns a SAS URL for
aztypes.StackCloud, causing empty blob URL for other clouds and ignoring
AllowSharedKeyAccess; update CreatePageBlob to check in.AllowSharedKeyAccess and
if true create the page blob client using a SharedKeyCredential (or construct
the client via the account key flow) instead of TokenCredential (identify where
pageblob.NewClient is called), and ensure that when shared-key auth is used you
return an accessible URL (either the blob URL signed with the shared key or by
returning the client URL) so downstream callers (see CreatePageBlob,
pageblob.NewClient, AllowSharedKeyAccess, GetSASURL) always receive a usable
blob URL even when in.CloudEnvironment != aztypes.StackCloud.
---
Duplicate comments:
In `@pkg/infrastructure/azure/azure.go`:
- Around line 769-774: The delegation key expiry (variable expiry and
service.KeyInfo Start/Expiry) is set to 1 hour while the SAS token generated
later is valid for 2 hours, so update the delegation key expiry to be at least
as long as the SAS expiry; compute the delegation key expiry from the same SAS
duration constant/variable used when creating the SAS (or set expiry :=
now.Add(2 * time.Hour)) and populate service.KeyInfo with that value so the
user-delegation key does not expire before the SAS.
In `@pkg/types/azure/validation/platform.go`:
- Around line 160-162: The validation error uses the wrong API field name in the
error path; update the fldPath.Child call in the validation logic that checks
p.CloudName == azure.StackCloud and p.AllowSharedKeyAccess to use
"allowSharedKeyAccess" (the actual config field) instead of
"disallowSharedAccessKey" so the field.Invalid report references the correct
field name (look for p.AllowSharedKeyAccess,
fldPath.Child("disallowSharedAccessKey") and change the child name to
"allowSharedKeyAccess").
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e868f72-ef32-4f52-8c7c-69528eaacd5f
📒 Files selected for processing (7)
data/data/install.openshift.io_installconfigs.yamlpkg/explain/printer_test.gopkg/infrastructure/azure/azure.gopkg/infrastructure/azure/storage.gopkg/types/azure/platform.gopkg/types/azure/validation/platform.gopkg/types/azure/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/explain/printer_test.go
- pkg/types/azure/zz_generated.deepcopy.go
- data/data/install.openshift.io_installconfigs.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
pkg/types/azure/validation/platform.go (1)
160-162:⚠️ Potential issue | 🟡 MinorPoint the validation error at
allowSharedKeyAccess.
allowSharedAccessKeyis not the install-config field name, so the current error tells users to fix a key they cannot actually set.Suggested fix
- allErrs = append(allErrs, field.Invalid(fldPath.Child("allowSharedAccessKey"), p.AllowSharedKeyAccess, "disabling shared access key creation is unsupported in Azure stack hub")) + allErrs = append(allErrs, field.Invalid(fldPath.Child("allowSharedKeyAccess"), p.AllowSharedKeyAccess, "disabling shared access key creation is unsupported in Azure stack hub"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/types/azure/validation/platform.go` around lines 160 - 162, The validation currently reports field.Invalid at fldPath.Child("allowSharedAccessKey") which is the wrong install-config key; update the field path to fldPath.Child("allowSharedKeyAccess") so the error points at the actual field, leaving the surrounding logic for p.CloudName == azure.StackCloud and the check on p.AllowSharedKeyAccess intact (i.e., change only the string passed to fldPath.Child in the field.Invalid call).pkg/infrastructure/azure/azure.go (1)
769-774:⚠️ Potential issue | 🟠 MajorKeep the user-delegation SAS inside the delegation-key window.
The key requested in
infoexpires after 1 hour, but the blob SAS is signed for 2 hours. That makes the ignition URL expire earlier than advertised during slow provisioning; reusenow/expiryfor the SAS values and passctxthrough the key request.Suggested fix
- udc, err := serviceClient.GetUserDelegationCredential(context.Background(), info, nil) + udc, err := serviceClient.GetUserDelegationCredential(ctx, info, nil) if err != nil { return nil, fmt.Errorf("failed to create user delegation credentials: %w", err) } sasQueryParams, err := sas.BlobSignatureValues{ Protocol: sas.ProtocolHTTPS, - StartTime: time.Now().UTC().Add(time.Second * -10), - ExpiryTime: time.Now().UTC().Add(2 * time.Hour), + StartTime: now, + ExpiryTime: expiry, Permissions: to.Ptr(sas.ContainerPermissions{Read: true}).String(), ContainerName: "ignition", BlobName: blobName, }.SignWithUserDelegation(udc)Run this to confirm the mismatch against the current code and the Azure docs. Expected: the code excerpt shows a 1-hour delegation key and a 2-hour SAS, and the doc excerpt states that the SAS start/expiry must stay within the user delegation key window.
#!/bin/bash set -euo pipefail echo "Relevant code:" sed -n '768,858p' pkg/infrastructure/azure/azure.go echo python - <<'PY' import html import re import urllib.request url = "https://learn.microsoft.com/en-us/rest/api/storageservices/create-user-delegation-sas" doc = urllib.request.urlopen(url, timeout=20).read().decode("utf-8", errors="ignore") text = html.unescape(re.sub(r"<[^>]+>", " ", doc)) text = re.sub(r"\s+", " ", text) for label, pattern in [ ("expiry", r"signed expiry time.*?user delegation key"), ("start", r"signed start time.*?user delegation key"), ]: m = re.search(pattern, text, flags=re.I) print(f"{label}: {m.group(0) if m else 'NOT FOUND'}") PYAlso applies to: 841-858
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/infrastructure/azure/azure.go` around lines 769 - 774, The user-delegation key window currently uses now/expiry with a 1-hour expiry while the generated blob SAS is signed for 2 hours, causing the SAS to fall outside the delegation key window; fix this by requesting the user delegation key with the same now and expiry values used for the SAS (reuse the now and expiry variables when building service.KeyInfo and when formatting the SAS start/expiry) and pass the request context through to GetUserDelegationKey (ensure ctx is forwarded into the call that requests the delegation key); update the references to service.KeyInfo, now, expiry and the GetUserDelegationKey invocation so the SAS start/expiry are within the delegation key window.pkg/infrastructure/azure/storage.go (1)
488-490:⚠️ Potential issue | 🟠 MajorGuard the shared-key branch before indexing the first key.
An empty
StorageAccountKeysslice or a nil first value still panics here instead of returning a normal error.Suggested guard
if in.AllowSharedKeyAccess { + if len(in.StorageAccountKeys) == 0 || in.StorageAccountKeys[0].Value == nil { + return "", fmt.Errorf("missing storage account key for shared-key block blob upload") + } sharedKeyCredential, err := azblob.NewSharedKeyCredential(in.StorageAccountName, *in.StorageAccountKeys[0].Value) if err != nil { return "", fmt.Errorf("failed to get shared crdentials for storage account: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/infrastructure/azure/storage.go` around lines 488 - 490, The AllowSharedKeyAccess branch indexes in.StorageAccountKeys[0].Value without validating the slice or pointer, which can panic; before calling azblob.NewSharedKeyCredential update the guard in the block that checks in.AllowSharedKeyAccess to ensure len(in.StorageAccountKeys) > 0 and in.StorageAccountKeys[0] != nil and in.StorageAccountKeys[0].Value != nil, and if any of those conditions fail return a descriptive error (or handle it consistently with surrounding error returns) instead of proceeding to dereference and call azblob.NewSharedKeyCredential.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/infrastructure/azure/storage.go`:
- Around line 231-243: CreatePageBlob currently always builds a
token-authenticated client and returns a URL for StackCloud, ignoring
AllowSharedKeyAccess; mirror the credential split used in createBlockBlob so
shared-key flows work. Update CreatePageBlob to choose credentials based on
AllowSharedKeyAccess/UserDelegatedSAS/TokenCredential/StorageAccountKeys (same
logic as createBlockBlob), construct the appropriate service/client (shared-key
vs token) and produce the correct URL for non-StackCloud paths, and ensure
CreatePageBlobInput in pkg/infrastructure/azure/azure.go is wired to pass
AllowSharedKeyAccess through so the CMK ignition flow can use the shared-key
path. Ensure the same fix is applied to the other CreatePageBlob usages around
the indicated ranges.
---
Duplicate comments:
In `@pkg/infrastructure/azure/azure.go`:
- Around line 769-774: The user-delegation key window currently uses now/expiry
with a 1-hour expiry while the generated blob SAS is signed for 2 hours, causing
the SAS to fall outside the delegation key window; fix this by requesting the
user delegation key with the same now and expiry values used for the SAS (reuse
the now and expiry variables when building service.KeyInfo and when formatting
the SAS start/expiry) and pass the request context through to
GetUserDelegationKey (ensure ctx is forwarded into the call that requests the
delegation key); update the references to service.KeyInfo, now, expiry and the
GetUserDelegationKey invocation so the SAS start/expiry are within the
delegation key window.
In `@pkg/infrastructure/azure/storage.go`:
- Around line 488-490: The AllowSharedKeyAccess branch indexes
in.StorageAccountKeys[0].Value without validating the slice or pointer, which
can panic; before calling azblob.NewSharedKeyCredential update the guard in the
block that checks in.AllowSharedKeyAccess to ensure len(in.StorageAccountKeys) >
0 and in.StorageAccountKeys[0] != nil and in.StorageAccountKeys[0].Value != nil,
and if any of those conditions fail return a descriptive error (or handle it
consistently with surrounding error returns) instead of proceeding to
dereference and call azblob.NewSharedKeyCredential.
In `@pkg/types/azure/validation/platform.go`:
- Around line 160-162: The validation currently reports field.Invalid at
fldPath.Child("allowSharedAccessKey") which is the wrong install-config key;
update the field path to fldPath.Child("allowSharedKeyAccess") so the error
points at the actual field, leaving the surrounding logic for p.CloudName ==
azure.StackCloud and the check on p.AllowSharedKeyAccess intact (i.e., change
only the string passed to fldPath.Child in the field.Invalid call).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 170cd4c7-174d-46c8-a921-a6459fa88084
📒 Files selected for processing (7)
data/data/install.openshift.io_installconfigs.yamlpkg/explain/printer_test.gopkg/infrastructure/azure/azure.gopkg/infrastructure/azure/storage.gopkg/types/azure/platform.gopkg/types/azure/validation/platform.gopkg/types/azure/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/types/azure/platform.go
- pkg/types/azure/zz_generated.deepcopy.go
- pkg/explain/printer_test.go
rna-afk
force-pushed
the
azure_user_delegated_sas
branch
from
b58c034 to
8a7afa5
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pkg/infrastructure/azure/azure.go (1)
769-774:⚠️ Potential issue | 🟠 MajorAlign user-delegation key expiry with SAS expiry.
Line 770 limits the user-delegation key to 1 hour, but Line 850 issues a 2-hour SAS. That can invalidate late reads when the key expires before the SAS.
Proposed fix
- now := time.Now().UTC().Add(-10 * time.Second) - expiry := now.Add(1 * time.Hour) + now := time.Now().UTC().Add(-10 * time.Second) + expiry := now.Add(1 * time.Hour) @@ sasQueryParams, err := sas.BlobSignatureValues{ Protocol: sas.ProtocolHTTPS, - StartTime: time.Now().UTC().Add(time.Second * -10), - ExpiryTime: time.Now().UTC().Add(2 * time.Hour), + StartTime: now, + ExpiryTime: expiry, Permissions: to.Ptr(sas.ContainerPermissions{Read: true}).String(), ContainerName: "ignition", BlobName: blobName, }.SignWithUserDelegation(udc)Use this to verify the mismatch and confirm alignment after patching:
#!/bin/bash set -euo pipefail rg -n -C3 'KeyInfo|GetUserDelegationCredential|StartTime|ExpiryTime' pkg/infrastructure/azure/azure.goExpected result after fix: both delegation key and SAS use the same
now/expirywindow.Also applies to: 849-850
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/infrastructure/azure/azure.go` around lines 769 - 774, The user-delegation KeyInfo currently sets Expiry to now+1h while the SAS is issued for 2h, causing the delegation key to expire before the SAS; update the KeyInfo creation in the GetUserDelegationCredential flow (look for service.KeyInfo and the variables now and expiry) so that the Start and Expiry use the same window as the SAS (e.g., set expiry to now.Add(2 * time.Hour) or reuse the same expiry variable used when constructing the SAS) and ensure both Start and Expiry are formatted with sas.TimeFormat.pkg/infrastructure/azure/storage.go (1)
257-259:⚠️ Potential issue | 🟠 MajorGuard storage keys before dereferencing index 0.
Both shared-key branches dereference
in.StorageAccountKeys[0].Valuewithout checking for empty keys or nil value, which can panic.Proposed fix
if in.AllowSharedKeyAccess { + if len(in.StorageAccountKeys) == 0 || in.StorageAccountKeys[0].Value == nil { + return "", fmt.Errorf("missing storage account key for shared-key page blob upload") + } sharedKeyCredential, err := azblob.NewSharedKeyCredential(in.StorageAccountName, *in.StorageAccountKeys[0].Value) if err != nil { return "", fmt.Errorf("failed to get shared credentials for storage account: %w", err) } @@ if in.AllowSharedKeyAccess { + if len(in.StorageAccountKeys) == 0 || in.StorageAccountKeys[0].Value == nil { + return "", fmt.Errorf("missing storage account key for shared-key block blob upload") + } sharedKeyCredential, err := azblob.NewSharedKeyCredential(in.StorageAccountName, *in.StorageAccountKeys[0].Value) if err != nil { return "", fmt.Errorf("failed to get shared crdentials for storage account: %w", err) }Also applies to: 508-510
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/infrastructure/azure/storage.go` around lines 257 - 259, The code dereferences in.StorageAccountKeys[0].Value when creating sharedKeyCredential (azblob.NewSharedKeyCredential) without validating that in.StorageAccountKeys has at least one element and that .Value is non-nil; update the branches that check in.AllowSharedKeyAccess to first verify len(in.StorageAccountKeys) > 0 and in.StorageAccountKeys[0].Value != nil (or iterate to find the first non-nil key), and return a clear error if no usable key exists before calling azblob.NewSharedKeyCredential; apply the same guard/fix to the other shared-key branch present around the second occurrence (the code at the other reported location).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/infrastructure/azure/azure.go`:
- Around line 769-774: The user-delegation KeyInfo currently sets Expiry to
now+1h while the SAS is issued for 2h, causing the delegation key to expire
before the SAS; update the KeyInfo creation in the GetUserDelegationCredential
flow (look for service.KeyInfo and the variables now and expiry) so that the
Start and Expiry use the same window as the SAS (e.g., set expiry to now.Add(2 *
time.Hour) or reuse the same expiry variable used when constructing the SAS) and
ensure both Start and Expiry are formatted with sas.TimeFormat.
In `@pkg/infrastructure/azure/storage.go`:
- Around line 257-259: The code dereferences in.StorageAccountKeys[0].Value when
creating sharedKeyCredential (azblob.NewSharedKeyCredential) without validating
that in.StorageAccountKeys has at least one element and that .Value is non-nil;
update the branches that check in.AllowSharedKeyAccess to first verify
len(in.StorageAccountKeys) > 0 and in.StorageAccountKeys[0].Value != nil (or
iterate to find the first non-nil key), and return a clear error if no usable
key exists before calling azblob.NewSharedKeyCredential; apply the same
guard/fix to the other shared-key branch present around the second occurrence
(the code at the other reported location).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2f5a469f-ce67-44f2-bb2d-6b85452c0372
📒 Files selected for processing (7)
data/data/install.openshift.io_installconfigs.yamlpkg/explain/printer_test.gopkg/infrastructure/azure/azure.gopkg/infrastructure/azure/storage.gopkg/types/azure/platform.gopkg/types/azure/validation/platform.gopkg/types/azure/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (4)
- data/data/install.openshift.io_installconfigs.yaml
- pkg/types/azure/validation/platform.go
- pkg/types/azure/zz_generated.deepcopy.go
- pkg/types/azure/platform.go
rna-afk
force-pushed
the
azure_user_delegated_sas
branch
from
8a7afa5 to
0d2ac71
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/infrastructure/azure/azure.go`:
- Around line 847-854: The SAS generation uses sas.BlobSignatureValues with
ExpiryTime set to 2 hours while the user delegation key (udc) created earlier
has a 1-hour expiry, causing an invalid SAS; update the ExpiryTime (and
optionally StartTime) in the sas.BlobSignatureValues used with
SignWithUserDelegation to be within udc's validity window—e.g., derive
ExpiryTime from the user delegation key (udc.SignedExpiry or compute
time.Until(udc.SignedExpiry)) or set ExpiryTime to
time.Now().UTC().Add(1*time.Hour) so it does not exceed udc, ensuring the code
path that calls SignWithUserDelegation(udc) uses the adjusted expiry.
In `@pkg/infrastructure/azure/storage.go`:
- Around line 512-517: The error messages in the shared key credential block are
misleading and contain a typo: update the missing-key error to reference "block
blob upload" (instead of "page blob") and fix the typo in the
NewSharedKeyCredential error message ("crdentials" → "credentials"); locate the
block that checks in.StorageAccountKeys and the creation of sharedKeyCredential
via azblob.NewSharedKeyCredential and change the two returned fmt.Errorf strings
accordingly so they accurately describe a block blob upload failure and spell
"credentials" correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a345cef2-8213-4c22-9116-8f9fc1715d6b
📒 Files selected for processing (7)
data/data/install.openshift.io_installconfigs.yamlpkg/explain/printer_test.gopkg/infrastructure/azure/azure.gopkg/infrastructure/azure/storage.gopkg/types/azure/platform.gopkg/types/azure/validation/platform.gopkg/types/azure/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/types/azure/validation/platform.go
- pkg/explain/printer_test.go
- pkg/types/azure/platform.go
rna-afk
force-pushed
the
azure_user_delegated_sas
branch
from
0d2ac71 to
9cf0d3d
Compare
Since this needs to be backported to earlier versions, adding a field to disallow shared access key if necessary. Made the field negative type field to have the default be enabled for shared key access for earlier versions.
rna-afk
force-pushed
the
azure_user_delegated_sas
branch
from
9cf0d3d to
8b57f2c
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
|
@rna-afk: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Retested with new commit, installation with/without /verified by jima |
openshift-ci-robot
added
the
verified
|
@jinyunma: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/lgtm |
|
Seems like ci/prow/e2e-azurestack has no green runs in the recent past. |
openshift-merge-bot
Bot
merged commit 3fa9ca0
into
openshift:main
|
@rna-afk: Jira Issue OCPBUGS-37587: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-37587 has been moved to the MODIFIED state. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/cherry-pick release-4.21 |
|
@rna-afk: #9851 failed to apply on top of branch "release-4.21": DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
7 participants
Signing the blob container using user delegated creds while
creating the container to upload the ignition assets.