[Storage] Add structured message CRC64 content validation for azfile - #27261
[Storage] Add structured message CRC64 content validation for azfile#27261tanyasethi-msft wants to merge 7 commits into
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 5 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Adds XSM/1.0 CRC64 validation infrastructure and integrates it into Azure Files upload/download operations.
Changes:
- Adds streaming structured-message encoding, decoding, and tests.
- Exposes CRC64 validation options across azfile APIs.
- Integrates structured bodies with uploads, downloads, and retries.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
sdk/storage/azfile/internal/shared/structured_message.go |
Implements XSM encoding and decoding. |
sdk/storage/azfile/internal/shared/structured_message_test.go |
Tests azfile XSM infrastructure. |
sdk/storage/azfile/internal/generated/models.go |
Adds structured-body option setters. |
sdk/storage/azfile/internal/exported/transfer_validation_option.go |
Implements validation options. |
sdk/storage/azfile/fileerror/error_codes.go |
Adds unsupported-checksum error. |
sdk/storage/azfile/file/responses.go |
Propagates validation across retries. |
sdk/storage/azfile/file/models.go |
Adds validation configuration fields. |
sdk/storage/azfile/file/constants.go |
Exposes CRC64 validation APIs. |
sdk/storage/azfile/file/client.go |
Integrates encoding and decoding. |
sdk/storage/azdatalake/internal/shared/structured_message.go |
Adds XSM infrastructure for azdatalake. |
sdk/storage/azdatalake/internal/shared/structured_message_test.go |
Tests azdatalake XSM infrastructure. |
| totalDataLen := len(data) | ||
| numSegments := totalDataLen / segmentSize | ||
| if totalDataLen%segmentSize != 0 { | ||
| numSegments++ | ||
| } | ||
| if numSegments == 0 { | ||
| numSegments = 1 | ||
| } |
There was a problem hiding this comment.
Added overflow protection to SMEncode matching what NewSMEncoder already does — auto-increases segment size when numSegments exceeds uint16 max.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (2)
sdk/storage/azfile/internal/shared/structured_message.go:676
msgLenandbytesReadare recorded but never compared, so the streaming decoder accepts a header whose declared message length does not match the framing it consumed. This omits a structural integrity check already performed bySMDecode; validate the count before every transition todecStateDone.
d.msgLen = binary.LittleEndian.Uint64(buf[1:9])
sdk/storage/azdatalake/internal/shared/structured_message.go:676
msgLenandbytesReadare recorded but never compared, so the streaming decoder accepts a header whose declared message length does not match the framing it consumed. Validate the count before every transition todecStateDone.
d.msgLen = binary.LittleEndian.Uint64(buf[1:9])
| body, err = options.TransactionalValidation.Apply(body, uploadRangeOptions) | ||
| if err != nil { | ||
| return UploadRangeResponse{}, err | ||
| } | ||
| contentLength, err = shared.ValidateSeekableStreamAt0AndGetCount(body) | ||
| if err != nil { | ||
| return UploadRangeResponse{}, err | ||
| } |
There was a problem hiding this comment.
Fair point about the azfile upload path. The body assignment in UploadRange already uses the return from Apply, so the encoded body does get sent. But will double-check the Optionalbody vs body flow to make sure Content-Length is consistent.
| if resp.StructuredBodyType != nil && *resp.StructuredBodyType != "" { | ||
| resp.Body = shared.NewSMDecoder(resp.Body) | ||
| } |
There was a problem hiding this comment.
Noted — this is a valid concern for chunked downloads. Will investigate the RetryReader EOF behavior with the SM decoder trailer.
| func (c TransferValidationTypeCRC64) Apply(rsc io.ReadSeekCloser, cfg generated.TransactionalContentSetter) (io.ReadSeekCloser, error) { | ||
| buf := make([]byte, 8) | ||
| binary.LittleEndian.PutUint64(buf, uint64(c)) | ||
| cfg.SetCRC64(buf) | ||
| return rsc, nil |
There was a problem hiding this comment.
The CRC64 types for azfile are exported for consistency with azblob's public API surface. Azure Files does support transactional CRC via structured message (that's what this PR adds). The SetCRC64 no-op is for the header-based transactional CRC which Files doesn't support — we could add a guard there, but structured message CRC64 is the primary path.
| d.msgLen = binary.LittleEndian.Uint64(buf[1:9]) | ||
| d.flags = binary.LittleEndian.Uint16(buf[9:11]) | ||
| d.numSegments = binary.LittleEndian.Uint16(buf[11:13]) | ||
| d.hasCRC = d.flags&SMFlagCRC64 != 0 |
There was a problem hiding this comment.
We specifically want to accept the flags as-is from the service response. The server negotiation via x-ms-structured-body header already guarantees CRC64 will be present. Rejecting on missing flag would break forward compatibility if the service ever sends messages with additional properties.
| for i := uint16(1); i <= numSegments; i++ { | ||
| segment, err := decodeSMSegment(smData, offset, i, hasCRC) |
There was a problem hiding this comment.
Good catch on the uint16 overflow in the loop counter. Will use a wider counter.
| d.msgLen = binary.LittleEndian.Uint64(buf[1:9]) | ||
| d.flags = binary.LittleEndian.Uint16(buf[9:11]) | ||
| d.numSegments = binary.LittleEndian.Uint16(buf[11:13]) | ||
| d.hasCRC = d.flags&SMFlagCRC64 != 0 |
There was a problem hiding this comment.
Same rationale as above — we accept the flags from the server response as-is.
| if o.TransactionalValidation != nil { | ||
| if h := exported.GetStructuredBodyType(o.TransactionalValidation); h != "" { | ||
| opts.StructuredBodyType = &h | ||
| } | ||
| } |
There was a problem hiding this comment.
This follows the same pattern as azblob. The download options accept TransferValidationType for structured message CRC64 specifically. Other validation types are not applicable for downloads and are silently ignored, matching the blob behavior. We could add explicit rejection for non-SM types if preferred.
There was a problem hiding this comment.
I believe these two azdatalake files were added in error? I see them on the other PR.
There was a problem hiding this comment.
You're right — moved the azdatalake SM files to PR #27263 where they belong. This PR is now azfile-only.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (10)
sdk/storage/azfile/file/client.go:274
Applyreturns the SM-encoded reader, but the generated request readsuploadRangeOptions.Optionalbody, which still points to the original unencoded body set byformat. Structured uploads therefore send raw bytes while advertising the encoded content length and structured-body headers. Update the option to use the wrapped reader before sending the request.
body, err = options.TransactionalValidation.Apply(body, uploadRangeOptions)
if err != nil {
return UploadRangeResponse{}, err
}
contentLength, err = shared.ValidateSeekableStreamAt0AndGetCount(body)
if err != nil {
return UploadRangeResponse{}, err
}
sdk/storage/azfile/file/models.go:600
- Non-structured validation values are silently ignored here. For example, passing
TransferValidationTypeMD5orTransferValidationTypeCRC64to the new download option sends no validation header and returns an unvalidated body even though the caller requested validation. Reject unsupported types (or use a download-specific option type) instead of treating them as no validation.
if o.TransactionalValidation != nil {
if h := exported.GetStructuredBodyType(o.TransactionalValidation); h != "" {
opts.StructuredBodyType = &h
}
}
sdk/storage/azfile/internal/shared/structured_message.go:212
- When
numSegmentsis 65535, thisuint16loop wraps from 65535 to 0 and executes again, soSMDecoderejects a valid message produced at the supported segment-count limit. Iterate with a wider integer and convert only the expected segment number.
for i := uint16(1); i <= numSegments; i++ {
segment, err := decodeSMSegment(smData, offset, i, hasCRC)
sdk/storage/azdatalake/internal/shared/structured_message.go:212
- When
numSegmentsis 65535, thisuint16loop wraps from 65535 to 0 and executes again, soSMDecoderejects a valid message produced at the supported segment-count limit. Iterate with a wider integer and convert only the expected segment number.
for i := uint16(1); i <= numSegments; i++ {
segment, err := decodeSMSegment(smData, offset, i, hasCRC)
sdk/storage/azfile/internal/shared/structured_message.go:754
- A zero message length disables validation because of the
d.msgLen > 0guard. Changing only the length field to zero leaves all CRCs valid, and the decoder then accepts the corrupted header. Compare the consumed length unconditionally.
if d.msgLen > 0 && d.bytesRead != int64(d.msgLen) {
return fmt.Errorf("structured message length mismatch: header says %d, consumed %d bytes", d.msgLen, d.bytesRead)
}
sdk/storage/azdatalake/internal/shared/structured_message.go:754
- A zero message length disables validation because of the
d.msgLen > 0guard. Changing only the length field to zero leaves all CRCs valid, and the decoder then accepts the corrupted header. Compare the consumed length unconditionally.
if d.msgLen > 0 && d.bytesRead != int64(d.msgLen) {
return fmt.Errorf("structured message length mismatch: header says %d, consumed %d bytes", d.msgLen, d.bytesRead)
}
sdk/storage/azfile/internal/shared/structured_message.go:690
- If the CRC64 flag is missing or corrupted, the streaming decoder switches to a no-checksum path and returns the payload successfully, even though this decoder is installed to provide CRC64 validation. Require
SMFlagCRC64before reading segments so a flipped flag cannot disable integrity checking.
d.msgLen = binary.LittleEndian.Uint64(buf[1:9])
d.flags = binary.LittleEndian.Uint16(buf[9:11])
d.numSegments = binary.LittleEndian.Uint16(buf[11:13])
d.hasCRC = d.flags&SMFlagCRC64 != 0
if d.hasCRC {
d.msgCRC = crc64.New(CRC64Table)
sdk/storage/azdatalake/internal/shared/structured_message.go:690
- If the CRC64 flag is missing or corrupted, the streaming decoder switches to a no-checksum path and returns the payload successfully, even though this decoder is installed to provide CRC64 validation. Require
SMFlagCRC64before reading segments so a flipped flag cannot disable integrity checking.
d.msgLen = binary.LittleEndian.Uint64(buf[1:9])
d.flags = binary.LittleEndian.Uint16(buf[9:11])
d.numSegments = binary.LittleEndian.Uint16(buf[11:13])
d.hasCRC = d.flags&SMFlagCRC64 != 0
if d.hasCRC {
d.msgCRC = crc64.New(CRC64Table)
sdk/storage/azfile/file/client.go:267
- The new upload integration is not exercised by the structured-message codec tests, so they cannot detect whether
UploadRangeactually sends the wrapped body and the required headers/content lengths. Add a client-level test that captures the outgoing request for direct and chunked structured uploads.
if options != nil && options.TransactionalValidation != nil {
if _, ok := options.TransactionalValidation.(TransferValidationTypeMD5); !ok {
body, err = options.TransactionalValidation.Apply(body, uploadRangeOptions)
sdk/storage/azfile/file/client.go:623
- A bounded
RetryReaderstops as soon as its decoded range count reaches zero, before calling the wrapped decoder again. If the final raw bytes exactly fill the caller's buffer (common for 4 MiB chunks copied with a 32 KiB buffer),SMDecoderis left indecStateSegFooter, so neither the final segment CRC nor message CRC is ever read or validated. Ensure structured decoding reaches its trailer before the retry reader reports EOF.
if resp.StructuredBodyType != nil && *resp.StructuredBodyType != "" {
resp.Body = shared.NewSMDecoder(resp.Body)
}
| flags := binary.LittleEndian.Uint16(smData[9:11]) | ||
| numSegments := binary.LittleEndian.Uint16(smData[11:13]) | ||
| hasCRC := flags&SMFlagCRC64 != 0 |
There was a problem hiding this comment.
The SMDecode function is a general-purpose structured message parser. CRC64 validation is enforced at the transport layer — we only install the decoder when the response header says properties=crc64. A corrupted flags field would also cause the msgLen check to fail since the byte counts wouldn't add up without footer/trailer bytes. Hardcoding a CRC64 requirement here would break forward compatibility if the format gains new property types.
There was a problem hiding this comment.
Same reasoning as above — the azdatalake decoder is a general-purpose SM parser. CRC64 enforcement happens at the transport level via header negotiation.
| } | ||
| } | ||
| if err != nil && d.segRemain > 0 { | ||
| d.setError(fmt.Errorf("segment %d: %w", d.segIndex, err)) |
There was a problem hiding this comment.
fmt.Errorf("segment %d: %s", d.segIndex, err.Error())There was a problem hiding this comment.
We actually changed this from %s to %w intentionally — using %w preserves the original error type through errors.Is/errors.As, which is important here because RetryReader checks for net.Error to decide if a failure is retryable. With %s or .Error(), the concrete type gets erased and transient network errors during segment reads would stop being retried. That was actually one of the Copilot findings we fixed in this round.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
sdk/storage/azfile/file/client.go:267
- The wrapped encoder is assigned only to the local
body;uploadRangeOptions.Optionalbodystill points to the original stream created byformat. The generated request readsOptionalbody(zz_file_client.go:2611-2614), so this sends unframed data while advertising structured-message headers and the encoded content length. UpdateOptionalbodyafter applying the validation wrapper.
body, err = options.TransactionalValidation.Apply(body, uploadRangeOptions)
sdk/storage/azfile/internal/shared/structured_message.go:633
- Wrapping with
%wstill makes transient read failures non-retryable here.RetryReaderuses a directerr.(net.Error)assertion and exacterr == io.ErrUnexpectedEOFcomparison (retry_reader.go:137-139), neither of which traverses this wrapper; a prematureio.EOFis also wrapped instead of converted toio.ErrUnexpectedEOF. Preserve retry classification by updating those checks toerrors.As/errors.Isand mapping premature EOF appropriately, or return the classified errors directly.
if err != nil && d.segRemain > 0 {
d.setError(fmt.Errorf("segment %d: %w", d.segIndex, err))
return totalOut, d.err
sdk/storage/azdatalake/internal/shared/structured_message.go:212
- This copy still uses a
uint16loop counter. For a valid message with 65,535 segments,i++wraps to zero after the last segment and the decoder attempts to parse another segment, so the maximum representable message cannot be decoded. Match the azfile copy by iterating with a wider type.
for i := uint16(1); i <= numSegments; i++ {
segment, err := decodeSMSegment(smData, offset, i, hasCRC)
sdk/storage/azfile/internal/shared/structured_message.go:752
- The
msgLen > 0guard lets a corrupted zero message-length header bypass validation entirely, even after the decoder consumes a complete nonempty frame.SMDecoderejects the same input, so the streaming decoder should compare every declared length, including zero.
if d.msgLen > 0 && d.bytesRead != int64(d.msgLen) {
sdk/storage/azdatalake/internal/shared/structured_message.go:752
- The
msgLen > 0guard lets a corrupted zero message-length header bypass validation entirely, even after the decoder consumes a complete nonempty frame.SMDecoderejects the same input, so the streaming decoder should compare every declared length, including zero.
if d.msgLen > 0 && d.bytesRead != int64(d.msgLen) {
… length validation
c186bd0 to
8b1fc33
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
sdk/storage/azfile/internal/shared/structured_message.go:633
%wstill produces a wrapper that does not satisfy RetryReader's directerr.(net.Error)check (file/retry_reader.go:137), so transient read failures remain non-retryable despite the attempted fix. Return the underlying error unchanged here, or update RetryReader to useerrors.As.
if err != nil && d.segRemain > 0 {
d.setError(fmt.Errorf("segment %d: %w", d.segIndex, err))
return totalOut, d.err
sdk/storage/azfile/internal/shared/structured_message.go:754
- The
msgLen > 0guard lets a message whose required length field is zero pass validation as long as its CRCs are valid.SMDecoderejects the same payload because the declared length never matches the bytes consumed; the streaming decoder should compare unconditionally too.
if d.msgLen > 0 && d.bytesRead != int64(d.msgLen) {
return fmt.Errorf("structured message length mismatch: header says %d, consumed %d bytes", d.msgLen, d.bytesRead)
}
sdk/storage/azfile/internal/shared/structured_message.go:243
- After validating the trailer, the decoder never advances
offsetor verifies that the parsed framing consumed the declared message. Appending bytes to a valid payload and updatingmsgLentherefore makesSMDecodeaccept trailing garbage. Require the final parsed offset to equal the input length.
expectedMsgCRC := binary.LittleEndian.Uint64(smData[offset : offset+8])
actualMsgCRC := msgHasher.Sum64()
if expectedMsgCRC != actualMsgCRC {
return SMDecodeResult{}, fmt.Errorf("message trailer CRC64 mismatch (expected 0x%016x, got 0x%016x)", expectedMsgCRC, actualMsgCRC)
}
}
Summary
TransferValidationTypeComputeStructuredMessageCRC64option for azfile clientsx-ms-structured-bodyheaderTest plan