feat(azure): migrate to modern azblob SDK and add SAS-token auth - #37
Merged
Merged
Conversation
Replace the deprecated github.com/Azure/azure-storage-blob-go (v0.15.0, EOL) with github.com/Azure/azure-sdk-for-go/sdk/storage/azblob v1.8.0 and add SAS-token authentication as the recommended, least-privilege method. Auth precedence in NewAzureStorage (first match wins): 1. SAS token (RECORDING_STORAGE_AZURE_SAS_TOKEN) - recommended 2. account key (RECORDING_STORAGE_AZURE_ACCESS_KEY) - logs a warning Details: - pkg/backup/storage.go: rewrite Azure backend on the client-based API (UploadFile/DownloadFile/NewListBlobsFlatPager/DeleteBlob). Location URLs are built from account/container/blob names only, never from client.URL(), so a SAS token can never leak into logs or stored locations. Metadata key stays "backup_id" (Azure rejects hyphens with HTTP 400). - pkg/config: new SASToken field + env loader; validateAzureStorageConfig enforces exactly one auth method when Azure is enabled and warns on account-key use. - cmd/siprec/main.go: pass SASToken through to the backup config. - pkg/http/config_redact.go: add "sas" to the redaction patterns. - Tests: both auth paths, SAS-token leak guard, blob-name round-trip, validation cases, and config redaction of the new fields. - Docs: README storage table, configuration.md "Azure Blob Storage Authentication" section, CHANGELOG 1.3.0. Backwards compatible: existing RECORDING_STORAGE_AZURE_ACCESS_KEY deployments keep working unchanged (with an added least-privilege warning at startup). Managed Identity is intentionally out of scope. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR migrates Azure Blob recording storage from the deprecated azure-storage-blob-go SDK to azure-sdk-for-go's azblob package, adds container-scoped SAS token authentication as an alternative to account-key auth, enforces exactly-one-auth-method validation at startup, extends secret redaction to SAS fields, and updates dependencies and documentation accordingly. ChangesAzure Blob Storage SAS Authentication and SDK Migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Main as createRecordingStorage
participant Storage as AzureStorage
participant Client as azblob.Client
participant Blob as Azure Blob Storage
Main->>Storage: NewAzureStorage(AzureConfig with SASToken/AccessKey)
alt SAS token configured
Storage->>Client: NewClientWithNoCredential(SAS URL)
else Access key configured
Storage->>Client: NewClientWithSharedKeyCredential(account, key)
end
Storage-->>Main: AzureStorage instance
Main->>Storage: Upload(backup)
Storage->>Client: UploadFile(blobName, metadata)
Client->>Blob: PUT blob data
Storage->>Storage: blobLocation() (no SAS embedded)
Storage-->>Main: azure://account/container/blob
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/backup/storage.go (1)
738-767: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNo timeout on Azure network calls; also applies to Download, List, and Delete.
Upload(and identicallyDownloadat line 782,List's pager at lines 790/804, andDeleteat line 848) usecontext.Background()with no deadline. A stalled connection or hung Azure request will block the calling goroutine indefinitely, since none of these calls have a cancellation/timeout path.🐛 Proposed fix (repeat for Download/List/Delete)
- uploaded := time.Now().Format(time.RFC3339) - ctx := context.Background() + uploaded := time.Now().Format(time.RFC3339) + ctx, cancel := context.WithTimeout(context.Background(), uploadTimeout) + defer cancel() _, err = a.client.UploadFile(ctx, a.container, blobName, file, &azblob.UploadFileOptions{🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/backup/storage.go` around lines 738 - 767, The Azure storage operations are using uncancelable contexts, so stalled network calls can hang indefinitely. Update AzureStorage.Upload and the matching Download, List pager, and Delete paths to use a context with deadline or cancellation instead of context.Background, and propagate that context through the Azure SDK calls. Keep the fix consistent across the Upload, Download, List, and Delete methods so all Azure requests can time out or be canceled.
🧹 Nitpick comments (1)
pkg/backup/storage.go (1)
769-787: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPartial file left on disk when Download fails.
If
DownloadFilefails partway through,outFilemay contain a truncated/corrupt file atlocalPaththat isn't cleaned up, and callers may not realize the file is invalid.♻️ Suggested cleanup on failure
ctx := context.Background() if _, err := a.client.DownloadFile(ctx, a.container, blobName, outFile, nil); err != nil { + outFile.Close() + os.Remove(localPath) return fmt.Errorf("failed to download from Azure: %w", err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/backup/storage.go` around lines 769 - 787, The AzureStorage.Download method leaves a partial file behind if a failure occurs after os.Create succeeds but before DownloadFile completes. Update Download to clean up localPath on any download error by removing the created file when aDownloadFile returns an error, while keeping the existing valid-file behavior on success; use the AzureStorage.Download and blobNameFromLocation flow to place the cleanup in the failure path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/configuration.md`:
- Around line 428-438: The shell example has a broken line continuation because
the inline comment after the --permissions option interrupts the backslash, so
the command will not copy/paste as one statement. Update the generate-sas
snippet so the continuation is preserved in the az storage container
generate-sas example by moving the explanation for --permissions cw off the same
line (or otherwise removing the inline comment) and keeping the command lines
chained correctly.
In `@pkg/backup/storage.go`:
- Around line 694-698: The warning in the Azure Blob Storage access-key path is
written in German and should be converted to English. Update the logger.Warn
message in the AccessKey branch of storage.go to an English least-privilege
warning, keeping the same behavior and context around
azblob.NewSharedKeyCredential and the config.AccessKey handling.
---
Outside diff comments:
In `@pkg/backup/storage.go`:
- Around line 738-767: The Azure storage operations are using uncancelable
contexts, so stalled network calls can hang indefinitely. Update
AzureStorage.Upload and the matching Download, List pager, and Delete paths to
use a context with deadline or cancellation instead of context.Background, and
propagate that context through the Azure SDK calls. Keep the fix consistent
across the Upload, Download, List, and Delete methods so all Azure requests can
time out or be canceled.
---
Nitpick comments:
In `@pkg/backup/storage.go`:
- Around line 769-787: The AzureStorage.Download method leaves a partial file
behind if a failure occurs after os.Create succeeds but before DownloadFile
completes. Update Download to clean up localPath on any download error by
removing the created file when aDownloadFile returns an error, while keeping the
existing valid-file behavior on success; use the AzureStorage.Download and
blobNameFromLocation flow to place the cleanup in the failure path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e05a820-7f01-49ea-b3a3-a4ae064491bd
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
CHANGELOG.mdREADME.mdcmd/siprec/main.godocs/configuration.mdgo.modpkg/backup/storage.gopkg/backup/storage_test.gopkg/config/config.gopkg/config/validation.gopkg/config/validation_test.gopkg/http/config_handlers_test.gopkg/http/config_redact.go
Comment on lines
+428
to
+438
| ```bash | ||
| az storage container generate-sas \ | ||
| --account-name mystorageaccount \ | ||
| --name recordings \ | ||
| --permissions cw \ # c = create, w = write (no read/delete) | ||
| --expiry 2027-01-01T00:00:00Z \ | ||
| --https-only \ | ||
| --auth-mode key \ | ||
| --account-key "<account-key-used-only-to-mint-the-sas>" \ | ||
| --output tsv | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the shell example's line continuation.
The inline comment after --permissions cw \ breaks the continuation, so copy/paste will split the command into separate shell commands.
Proposed fix
- --permissions cw \ # c = create, w = write (no read/delete)
+ --permissions cw \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ```bash | |
| az storage container generate-sas \ | |
| --account-name mystorageaccount \ | |
| --name recordings \ | |
| --permissions cw \ # c = create, w = write (no read/delete) | |
| --expiry 2027-01-01T00:00:00Z \ | |
| --https-only \ | |
| --auth-mode key \ | |
| --account-key "<account-key-used-only-to-mint-the-sas>" \ | |
| --output tsv | |
| ``` |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/configuration.md` around lines 428 - 438, The shell example has a broken
line continuation because the inline comment after the --permissions option
interrupts the backslash, so the command will not copy/paste as one statement.
Update the generate-sas snippet so the continuation is preserved in the az
storage container generate-sas example by moving the explanation for
--permissions cw off the same line (or otherwise removing the inline comment)
and keeping the command lines chained correctly.
Comment on lines
+694
to
+698
| case config.AccessKey != "": | ||
| if logger != nil { | ||
| logger.Warn("Azure Blob Storage nutzt Account Key Auth – SAS-Token wird empfohlen (least privilege).") | ||
| } | ||
| cred, credErr := azblob.NewSharedKeyCredential(config.Account, config.AccessKey) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Warning log message is written in German, not English.
Line 696 mixes German text into an otherwise all-English codebase: "Azure Blob Storage nutzt Account Key Auth – SAS-Token wird empfohlen (least privilege)." This is an important least-privilege security warning that most operators/log consumers won't understand if they don't read German.
🐛 Proposed fix
case config.AccessKey != "":
if logger != nil {
- logger.Warn("Azure Blob Storage nutzt Account Key Auth – SAS-Token wird empfohlen (least privilege).")
+ logger.Warn("Azure Blob Storage is using account key auth; a SAS token is recommended (least privilege).")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case config.AccessKey != "": | |
| if logger != nil { | |
| logger.Warn("Azure Blob Storage nutzt Account Key Auth – SAS-Token wird empfohlen (least privilege).") | |
| } | |
| cred, credErr := azblob.NewSharedKeyCredential(config.Account, config.AccessKey) | |
| case config.AccessKey != "": | |
| if logger != nil { | |
| logger.Warn("Azure Blob Storage is using account key auth; a SAS token is recommended (least privilege).") | |
| } | |
| cred, credErr := azblob.NewSharedKeyCredential(config.Account, config.AccessKey) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/backup/storage.go` around lines 694 - 698, The warning in the Azure Blob
Storage access-key path is written in German and should be converted to English.
Update the logger.Warn message in the AccessKey branch of storage.go to an
English least-privilege warning, keeping the same behavior and context around
azblob.NewSharedKeyCredential and the config.AccessKey handling.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Migrates the Azure Blob Storage backend from the deprecated
github.com/Azure/azure-storage-blob-go(v0.15.0, end-of-life, last release 2021) to the moderngithub.com/Azure/azure-sdk-for-go/sdk/storage/azblob(v1.8.0), and adds SAS-token authentication as a recommended, least-privilege alternative to the storage account key.Motivation
What changed
Authentication precedence in
NewAzureStorage(first match wins):RECORDING_STORAGE_AZURE_SAS_TOKEN) — recommended; can be scoped to a single container with just create/write permissions and an expiry.RECORDING_STORAGE_AZURE_ACCESS_KEY) — still supported; logs a least-privilege warning at startup.Code:
pkg/backup/storage.go— Azure backend rewritten on the client-based API (UploadFile/DownloadFile/NewListBlobsFlatPager/DeleteBlob). Location URLs are built from the account/container/blob names only, never fromclient.URL(), so a SAS token can never leak into logs or stored.locationsfiles. Metadata key staysbackup_id(Azure rejects hyphens with HTTP 400).pkg/config— newSASTokenfield + env loader;validateAzureStorageConfigenforces exactly one auth method when Azure is enabled and warns when the account key is used.cmd/siprec/main.go— pass the SAS token through to the backup config.pkg/http/config_redact.go— addsasto the secret-redaction patterns so tokens never appear in/api/config.Tests: both auth paths, a SAS-token leak guard on location strings, blob-name round-trip, Azure validation cases, and redaction of the new fields.
Docs: README storage table, a new "Azure Blob Storage Authentication" section in
docs/configuration.md(incl. anaz storage container generate-sasexample), and aCHANGELOG1.3.0 entry.Backwards compatibility
Existing deployments that use
RECORDING_STORAGE_AZURE_ACCESS_KEYkeep working unchanged — the only visible difference is a startup warning recommending a SAS token.Out of scope
Managed Identity /
azidentitywas intentionally left out to keep the dependency footprint small; it can be added later.Verification
go build ./...andgo vet ./...pass.go test ./pkg/backup/... ./pkg/config/... ./pkg/http/...pass.create+write): recordings upload as.mp3blobs withbackup_idmetadata; no credentials appear in logs or location URLs.Summary by CodeRabbit
New Features
Bug Fixes
Documentation