feat(remote_signer): Add job token signing endpoint for byoc job types - #3869
feat(remote_signer): Add job token signing endpoint for byoc job types#3869eliteprox wants to merge 46 commits into
Conversation
…o orchestrator and update gRPC spec - Introduced ExternalCapabilities method in the orchestrator to retrieve registered external capabilities. - Updated OrchestratorInfo message in protobuf to include external capabilities information. - Enhanced gRPC client and server implementations to utilize new method and handle external capabilities. - Added new ExternalCapabilityInfo message to define external capabilities with attributes like name, description, capacity, and pricing. - Updated remote signer to support BYOC job type and handle capability-specific logic in payment processing. - Adjusted tests to accommodate new external capabilities functionality.
…ethods - Removed PriceInfo field from ExternalCapabilityInfo in both protobuf and Go files to streamline capability information. - Updated orchestratorInfoWithCaps function to reflect the removal of PriceInfo, ensuring compatibility with existing external capabilities logic. - Adjusted related comments and documentation to maintain clarity and accuracy.
- Added a new route for retrieving capabilities at "/byoc/capabilities". - Introduced GetCapabilities method to handle HTTP requests and return a JSON response with external capabilities information. - Updated Orchestrator interface to include ExternalCapabilities method for fetching registered capabilities. - Removed ExternalCapabilityInfo from protobuf and related files to streamline capability management. - Adjusted orchestratorInfoWithCaps function to reflect changes in external capabilities handling.
- Downgraded protoc-gen-go-grpc version from v1.6.0 to v1.2.0. - Updated gRPC support package version requirement from v1.64.0 to v1.32.0. - Replaced constant method names with direct string references for RPC calls in the OrchestratorClient and OrchestratorServer interfaces. - Adjusted comments for clarity and consistency in the generated code.
github-actions
Bot
added
go
…g logic - Introduced Capability_BYOCExternal constant to represent the new BYOC external capability. - Updated CapabilityNameLookup to include a name for BYOC external capability. - Enhanced GetCapabilitiesPrices method in orchestrator to append pricing for BYOC external capabilities, ensuring seamless integration with existing pricing logic.
- Deleted the GetCapabilities method and its associated ExternalCapabilityInfo structure from the BYOC orchestrator. - Removed the "/byoc/capabilities" route from the HTTP mux. - Updated the Orchestrator interface to eliminate the ExternalCapabilities method, streamlining capability management.
- Added a test case for handling missing capability constraints in BYOC requests, ensuring proper error messaging for invalid configurations. - Updated the GenerateLivePayment function to validate the BYOC capability against the orchestrator's price info, improving robustness in payment processing.
…ricing logic - Removed redundant checks for orch.node.Recipient in PriceInfo and PriceInfoForCaps methods. - Updated PriceInfo method to include BYOC capability and constraint handling, allowing remote signers to identify BYOC prices. - Enhanced priceInfo method to differentiate pricing retrieval for BYOC capabilities, ensuring accurate price fetching for jobs. - Adjusted conditions for auto price adjustment based on node state.
j0sh
left a comment
j0sh
left a comment
There was a problem hiding this comment.
Partially reviewed; I'll hold off on the rest until the BYOC job signature is updated.
- Introduced a new V1 binary format for signing BYOC job requests, improving security and preventing signature replay. - Updated the `sign` method to use the new signing format and adjusted the `SignBYOCJobRequest` endpoint to support both V1 and legacy V0 formats. - Enhanced error handling in payment processing to provide more specific HTTP status codes based on the error type. - Refactored payment confirmation and processing methods to ensure consistent balance retrieval and error logging. - Added a new utility function to resolve pricing information for BYOC capabilities, ensuring accurate pricing retrieval during payment requests.
- Updated comments for clarity regarding the V1 structured binary format. - Removed fallback logic for the deprecated V0 legacy format, streamlining the job credential verification process. - Enhanced error logging for signature verification failures.
…ient check when auto-adjusting prices
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3869 +/- ##
===================================================
+ Coverage 35.53014% 35.53787% +0.00773%
===================================================
Files 174 174
Lines 45111 45225 +114
===================================================
+ Hits 16028 16072 +44
- Misses 27814 27875 +61
- Partials 1269 1278 +9
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
restore comment in byoc/utils.go
- Introduced a new function to validate BYOC capability constraints, ensuring that requests include the necessary capability name. - Updated the `resolvePriceInfo` function to accept the capability name as a parameter, improving error handling for missing pricing information. - Modified test cases to reflect changes in BYOC request handling, including new validation scenarios and error messages for missing capability constraints.
…ncapsulation of manifest ID and capabilities. - Streamlined the assignment of capabilities based on the presence of network capabilities, enhancing clarity and maintainability of the code. - Removed redundant code related to stream parameters to improve overall function readability.
- Renamed the `TestResolvePriceInfo_BYOCUsesCapabilitiesPrices` to `TestResolvePriceInfo_BYOCUsesEffectiveTopLevelPrice` for clarity. - Updated the `resolvePriceInfo` function to ensure the effective price is derived from the top-level `PriceInfo` while validating capability-specific pricing. - Enhanced test assertions to verify the expected price information in payment generation scenarios, ensuring accurate validation of BYOC capabilities.
…generation - Adjusted the calculation of `secondsToPrefund` to ensure a minimum of 1 second for sub-second wall-clock deltas. - Introduced a new variable `preloadSecs` to streamline the handling of timeout and minimum preload seconds. - Enhanced clarity in the logic for determining the pre-funding duration based on billable seconds and timeout settings.
- Grouped remote type constants into a single block for improved organization. - Removed redundant declaration of `RemoteType_BYOC`, ensuring a cleaner code structure.
- Introduced a new helper function `mustBYOCCaps` to streamline the creation of BYOC capabilities blobs for tests. - Updated the `resolvePriceInfo` function to accept capabilities directly, improving clarity and error handling. - Added a new test case `TestResolvePriceInfo_BYOC_AggregateWhenCapabilitiesPricesOmitted` to validate pricing resolution when capabilities prices are not provided. - Adjusted existing tests to utilize the new capabilities handling, ensuring consistent validation across scenarios.
…eneration - Moved the declaration of `manifestID` to a more appropriate location within the `GenerateLivePayment` function for improved clarity. - Ensured that `manifestID` is set based on the request type, enhancing the logic for handling different payment scenarios.
…ndling - Refactored the `resolvePriceInfo` function to enhance clarity and streamline the logic for handling BYOC capabilities. - Consolidated helper functions for validating BYOC capability constraints and checking price information, improving code organization. - Updated test cases to reflect changes in BYOC request handling, ensuring accurate validation and error reporting for missing capability constraints.
- Eliminated the `validateRemotePaymentType` function as it was no longer necessary for payment processing. - Updated the `GenerateLivePayment` function to remove the call to the now-removed validation, streamlining the payment generation logic.
- Removed the nil check for `orch.node.Balances` in the `priceInfo` function, streamlining the logic for fixed price lookup. - Enhanced clarity by directly checking for the presence of balances associated with the sender address.
- Eliminated the `resolvePriceInfo` function and associated helper methods, simplifying the pricing logic for payment requests. - Updated the `GenerateLivePayment` function to directly validate price information, enhancing clarity and reducing complexity. - Removed outdated test cases related to BYOC capability validation, streamlining the test suite.
|
@j0sh Any chance we can merge this week? |
Resolve CHANGELOG_PENDING.md conflict by keeping both breaking change entries from the PR branch and master. Co-authored-by: John | Elite Encoder <john@eliteencoder.net>
📝 WalkthroughWalkthroughThis PR adds structured BYOC job signing and verification, rejects invalid non-empty payment headers, adds remote BYOC signing, extends live-payment generation and pricing for BYOC, and adds validation and billing tests. ChangesBYOC Signing and Payment Flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant LivepeerServer as SignBYOCJobRequest
participant Gateway as gatewayJob.sign
participant Orchestrator as verifyJobCreds
participant Pricing as orch.node.GetPriceForJob
Client->>LivepeerServer: POST /sign-byoc-job
LivepeerServer->>LivepeerServer: Validate timeout and flatten BYOC job
LivepeerServer-->>Client: sender and signature
Gateway->>Gateway: Sign flattened BYOC payload
Gateway->>Orchestrator: Submit signed BYOC job
Orchestrator->>Orchestrator: Verify flattened signature
Orchestrator->>Pricing: GetPriceForJob(sender, modelID)
Orchestrator-->>Gateway: Accept job or return payment error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Add ValidateBYOCJobTimeoutSeconds and apply it at the remote signer endpoint, gateway signing path, and job request parsing so V1 signatures cannot silently truncate oversized timeouts. Co-authored-by: John | Elite Encoder <john@eliteencoder.net>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/orchestrator.go (1)
419-421: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
price == nilcheck is dead code for the BYOC path.
GetPriceForJobalways returns a non-nil*big.Rat(initialized tobig.NewRat(0, 1)when no price is found). Theprice == nilcondition can never be true — onlyprice.Sign() == 0is meaningful. Consider simplifying to just the zero check for clarity.♻️ Simplify the nil/zero check
- if price == nil || price.Sign() == 0 { + if price.Sign() == 0 { price = orch.node.GetPriceForJob("default", modelID) }🤖 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 `@core/orchestrator.go` around lines 419 - 421, The BYOC price fallback in the orchestrator contains a dead nil check because GetPriceForJob always returns a non-nil *big.Rat. Update the logic in the price lookup path to remove the price == nil branch and rely only on price.Sign() == 0 when deciding to fall back to orch.node.GetPriceForJob, keeping the behavior unchanged but simplifying the condition.
🤖 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 `@byoc/types.go`:
- Around line 304-345: Reject non-positive timeout values before flattening the
BYOC job so negative values can’t be cast through uint32 in FlattenBYOCJob.
Update the existing validation for BYOCJobSigningInput (where
TimeoutSeconds/Timeout is checked) to treat any value <= 0 as invalid instead of
only zero, and keep the encoder unchanged. This should be applied in the request
validation path that feeds FlattenBYOCJob.
---
Nitpick comments:
In `@core/orchestrator.go`:
- Around line 419-421: The BYOC price fallback in the orchestrator contains a
dead nil check because GetPriceForJob always returns a non-nil *big.Rat. Update
the logic in the price lookup path to remove the price == nil branch and rely
only on price.Sign() == 0 when deciding to fall back to
orch.node.GetPriceForJob, keeping the behavior unchanged but simplifying the
condition.
🪄 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: 8ca9cb1d-d105-4589-b85c-82c2aea164f8
📒 Files selected for processing (7)
CHANGELOG_PENDING.mdbyoc/job_orchestrator.gobyoc/types.gobyoc/utils.gocore/orchestrator.goserver/remote_signer.goserver/remote_signer_test.go
| func FlattenBYOCJob(job *BYOCJobSigningInput) []byte { | ||
| idBytes := []byte(job.ID) | ||
| capBytes := []byte(job.Capability) | ||
| reqBytes := []byte(job.Request) | ||
| paramsBytes := []byte(job.Parameters) | ||
|
|
||
| size := 16 + 4 + | ||
| 4 + len(idBytes) + | ||
| 4 + len(capBytes) + | ||
| 4 + len(reqBytes) + | ||
| 4 + len(paramsBytes) | ||
|
|
||
| buf := make([]byte, size) | ||
| offset := 0 | ||
|
|
||
| copy(buf[offset:], []byte(BYOCJobSigV1Prefix)) | ||
| offset += 16 | ||
|
|
||
| binary.BigEndian.PutUint32(buf[offset:], uint32(job.TimeoutSeconds)) | ||
| offset += 4 | ||
|
|
||
| binary.BigEndian.PutUint32(buf[offset:], uint32(len(idBytes))) | ||
| offset += 4 | ||
| copy(buf[offset:], idBytes) | ||
| offset += len(idBytes) | ||
|
|
||
| binary.BigEndian.PutUint32(buf[offset:], uint32(len(capBytes))) | ||
| offset += 4 | ||
| copy(buf[offset:], capBytes) | ||
| offset += len(capBytes) | ||
|
|
||
| binary.BigEndian.PutUint32(buf[offset:], uint32(len(reqBytes))) | ||
| offset += 4 | ||
| copy(buf[offset:], reqBytes) | ||
| offset += len(reqBytes) | ||
|
|
||
| binary.BigEndian.PutUint32(buf[offset:], uint32(len(paramsBytes))) | ||
| offset += 4 | ||
| copy(buf[offset:], paramsBytes) | ||
|
|
||
| return buf | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether Timeout is validated anywhere before gatewayJob.sign()
rg -nP '\bTimeout\b' byoc/*.go -C2Repository: livepeer/go-livepeer
Length of output: 13322
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== byoc/job_gateway.go (around sign path) =="
sed -n '300,360p' byoc/job_gateway.go
echo
echo "== byoc/job_orchestrator.go (around remote-signer verify path) =="
sed -n '540,595p' byoc/job_orchestrator.go
echo
echo "== byoc/types.go (BYOC job signing input) =="
sed -n '280,350p' byoc/types.goRepository: livepeer/go-livepeer
Length of output: 5959
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
files = ["byoc/job_gateway.go", "byoc/job_orchestrator.go", "byoc/types.go"]
for f in files:
text = Path(f).read_text()
print(f"\n== {f} ==")
for m in re.finditer(r'(?m)^(.*Timeout.*)$', text):
line = text[:m.start()].count("\n") + 1
if 1 <= line <= 700:
print(f"{line}: {m.group(1)}")
PYRepository: livepeer/go-livepeer
Length of output: 4161
Reject non-positive timeouts before flattening. The BYOC request validators only guard Timeout == 0, so negative values can reach this encoder and wrap through uint32(job.TimeoutSeconds). <= 0 is the safer check.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 321-321: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint32(job.TimeoutSeconds)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
[warning] 324-324: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint32(len(idBytes))
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
[warning] 329-329: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint32(len(capBytes))
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
[warning] 334-334: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint32(len(reqBytes))
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
[warning] 339-339: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint32(len(paramsBytes))
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🤖 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 `@byoc/types.go` around lines 304 - 345, Reject non-positive timeout values
before flattening the BYOC job so negative values can’t be cast through uint32
in FlattenBYOCJob. Update the existing validation for BYOCJobSigningInput (where
TimeoutSeconds/Timeout is checked) to treat any value <= 0 as invalid instead of
only zero, and keep the encoder unchanged. This should be applied in the request
validation path that feeds FlattenBYOCJob.
Source: Linters/SAST tools
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
byoc/types.go (1)
305-362: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd unit coverage for
FlattenBYOCJobinbyoc/types.go:314-362The V1 signing payload is used byserver/remote_signer.go,byoc/job_orchestrator.go, andbyoc/utils.go, but there’s no direct test covering its byte layout or field order.🤖 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 `@byoc/types.go` around lines 305 - 362, Add unit tests for FlattenBYOCJob and BYOCJobSigningInput to verify the exact V1 byte layout and field order produced by the signing payload. Cover the prefix, timeout encoding, and each length-prefixed field (ID, Capability, Request, Parameters) using a representative input so regressions in server/remote_signer.go, byoc/job_orchestrator.go, or byoc/utils.go are caught. Place the test near the BYOC flattening logic and assert deterministic output bytes rather than just non-empty results.Source: Linters/SAST tools
🧹 Nitpick comments (1)
byoc/utils.go (1)
80-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
jobData.Timeout == 0check.
ValidateBYOCJobTimeoutSecondsat line 83 already rejects0(via<= 0), making the check at line 80-81 redundant. The only difference is the error returned (errNoTimeoutSetvs"timeout_seconds must be positive"). DownstreamverifyJobCredsfunctions injob_gateway.goandjob_orchestrator.goalso have now-deadjobData.Timeout == 0checks sinceparseJobRequestalways rejects invalid timeouts.Consider removing the redundant check if no caller specifically depends on
errNoTimeoutSet.🤖 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 `@byoc/utils.go` around lines 80 - 85, Remove the redundant zero-timeout branch in the BYOC job validation flow: `ValidateBYOCJobTimeoutSeconds` already rejects non-positive values, so `jobData.Timeout == 0` in `byoc/utils.go` is unnecessary. Update the timeout validation path in `validateBYOCJob` to rely on `ValidateBYOCJobTimeoutSeconds`, and also delete the now-dead `jobData.Timeout == 0` checks in the `verifyJobCreds` logic in `job_gateway.go` and `job_orchestrator.go` since `parseJobRequest` already filters invalid timeouts. If any caller truly needs `errNoTimeoutSet`, keep it only where that specific distinction is still required.
🤖 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.
Outside diff comments:
In `@byoc/types.go`:
- Around line 305-362: Add unit tests for FlattenBYOCJob and BYOCJobSigningInput
to verify the exact V1 byte layout and field order produced by the signing
payload. Cover the prefix, timeout encoding, and each length-prefixed field (ID,
Capability, Request, Parameters) using a representative input so regressions in
server/remote_signer.go, byoc/job_orchestrator.go, or byoc/utils.go are caught.
Place the test near the BYOC flattening logic and assert deterministic output
bytes rather than just non-empty results.
---
Nitpick comments:
In `@byoc/utils.go`:
- Around line 80-85: Remove the redundant zero-timeout branch in the BYOC job
validation flow: `ValidateBYOCJobTimeoutSeconds` already rejects non-positive
values, so `jobData.Timeout == 0` in `byoc/utils.go` is unnecessary. Update the
timeout validation path in `validateBYOCJob` to rely on
`ValidateBYOCJobTimeoutSeconds`, and also delete the now-dead `jobData.Timeout
== 0` checks in the `verifyJobCreds` logic in `job_gateway.go` and
`job_orchestrator.go` since `parseJobRequest` already filters invalid timeouts.
If any caller truly needs `errNoTimeoutSet`, keep it only where that specific
distinction is still required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ec1eef5-a563-48b0-936c-f12bb16c20b8
📒 Files selected for processing (4)
byoc/types.gobyoc/types_test.gobyoc/utils.goserver/remote_signer.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/remote_signer.go
There was a problem hiding this comment.
Pull request overview
This PR completes the BYOC payment/signing flow by introducing a dedicated remote-signer endpoint for BYOC job token signing, switching BYOC job signatures to a tamper-resistant V1 binary format, and extending remote signer payment generation with BYOC-specific, time-based billing behavior.
Changes:
- Added
POST /sign-byoc-jobremote signer route that validates and signs BYOC jobs using the V1 binary signing payload (FlattenBYOCJob). - Introduced the V1 BYOC job signing wire format with a domain separator, removed legacy V0 verification, and tightened payment-header parsing behavior in the BYOC orchestrator.
- Updated remote signer billing logic to support BYOC time-based preloading and correct unit scaling (
PixelsPerUnit) during fee calculation.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/remote_signer.go | Adds BYOC job signing endpoint; implements BYOC time-based billing and manifest/balance tracking updates. |
| server/remote_signer_test.go | Adds BYOC capability blob helper and extends payment tests for BYOC billing behavior. |
| core/orchestrator.go | Updates capability pricing resolution to use job-specific pricing for BYOC capabilities. |
| byoc/utils.go | Switches gateway-side BYOC signing to V1 binary payload and validates timeout. |
| byoc/types.go | Adds V1 BYOC job signing format (FlattenBYOCJob) and timeout validation helper. |
| byoc/types_test.go | Adds unit tests for BYOC timeout validation. |
| byoc/job_orchestrator.go | Enforces V1 signature verification, changes invalid payment header handling to hard-reject, and adjusts payment processing return behavior. |
| CHANGELOG_PENDING.md | Documents breaking change for invalid non-empty Livepeer-Payment headers (HTTP 400). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var caps *net.Capabilities | ||
| if len(req.Capabilities) > 0 { | ||
| caps = &net.Capabilities{} | ||
| if err := proto.Unmarshal(req.Capabilities, caps); err != nil { | ||
| clog.Errorf(ctx, "Failed to unmarshal capabilities err=%q", err) |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/remote_signer.go (1)
410-418: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEnforce the documented BYOC capability requirement.
For
RemoteType_BYOC, an emptyreq.Capabilitiesleavescaps == nil, omits capabilities fromstreamParams, and can bypass capability-specific pricing validation. Reject missing/empty BYOC capabilities before continuing, and validate that the decoded capabilities identify the requested BYOC capability.Also applies to: 493-495
🤖 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 `@server/remote_signer.go` around lines 410 - 418, Update the RemoteType_BYOC handling around req.Capabilities and caps to reject missing or empty capabilities before continuing. Decode the provided capabilities, require the decoded value to identify the requested BYOC capability, and return a bad-request response when either requirement fails. Ensure the validated caps value is still included in streamParams so capability-specific pricing validation cannot be bypassed.
🤖 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.
Outside diff comments:
In `@server/remote_signer.go`:
- Around line 410-418: Update the RemoteType_BYOC handling around
req.Capabilities and caps to reject missing or empty capabilities before
continuing. Decode the provided capabilities, require the decoded value to
identify the requested BYOC capability, and return a bad-request response when
either requirement fails. Ensure the validated caps value is still included in
streamParams so capability-specific pricing validation cannot be bypassed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 64bbcdad-be7b-4cec-95a2-1c44d9152083
📒 Files selected for processing (4)
CHANGELOG_PENDING.mdcore/orchestrator.goserver/remote_signer.goserver/remote_signer_test.go
💤 Files with no reviewable changes (1)
- CHANGELOG_PENDING.md
🚧 Files skipped from review as they are similar to previous changes (1)
- core/orchestrator.go
What does this pull request do? Explain your changes. (required)
Completes the BYOC payment flow in the remote signer by adding a dedicated job-signing endpoint and time-based billing, using a tamper-resistant V1 binary format to prevent signature replay.
Specific updates (required)
POST /sign-byoc-job— new remote signer endpoint (server/remote_signer.go){ id, capability, request, parameters, timeout_seconds }, enforces a 1 MiB body cap, validates thatid,capability, andtimeout_secondsare non-empty/positive, then signs the payload withFlattenBYOCJob(V1 format) and returns{ sender, signature }.StartRemoteSignerServernow calls out the trust model: all three signer routes (/sign-orchestrator-info,/generate-live-payment,/sign-byoc-job) must only be exposed on trusted networks.V1 binary signing format (
byoc/types.go)BYOCJobSigV1Prefix = "LP_BYOC_JOB_V1\x00\x00"(16-byte domain separator) andFlattenBYOCJob, which produces a deterministic length-prefixed binary layout:version(16) || timeout(4,BE) || len(id)(4,BE) || id || len(cap)(4,BE) || cap || len(req)(4,BE) || req || len(params)(4,BE) || params.byoc/job_orchestrator.go(verifyJobCreds) has been removed; only V1 is accepted.BYOC time-based billing in
GenerateLivePayment(server/remote_signer.go)PreloadSecondsfield toRemotePaymentRequest. On the first request (no prior state),billableSecsis set tomax(PreloadSeconds, minPreloadSecs)(floor of 60 s) so batch jobs size the initial ticket batch to the expected job duration rather than a fixed buffer.billableSecsis the elapsed wall-clock time, floored to 1 s for sub-second deltas.ceil(billableSecs) * priceInfo.PixelsPerUnit, correctly accounting for anyPixelsPerUnitscale, not just thePixelsPerUnit == 1case.state.StateIDas themanifestIDfor balance tracking (no caller-supplied manifest ID).confirmPaymentbehavior change (byoc/job_orchestrator.go)Livepeer-Paymentheader that fails parsing is now hard-rejected witherrPaymentError(HTTP 400 on the caller side) instead of being logged and ignored. This is a breaking change for gateways/SDKs that may send a stale or malformed payment header; the corresponding HTTP status they will receive is 400.GetCapabilitiesPricesalways populated for non-cappedGetOrchestratorInfo(server/rpc.go)orchestratorInfoWithCapsnow callsGetCapabilitiesPricesunconditionally whencaps == nil, and the result is always included inOrchestratorInfo.CapabilitiesPrices. Any error fromGetCapabilitiesPricesis logged and silently swallowed so that transient failures do not hard-error the entire info response.OrchestratorInfopath (no capabilities filter) will now consistently seeCapabilitiesPricespopulated.Minor cleanups
RemoteType_LiveVideoToVideo,RemoteType_BYOC) are now grouped together in a single block.validateRemotePaymentTypehelper removed (duplicated the switch statement below it).orch.node.Balancesincore/orchestrator.gothat was introduced when BYOC usedmanifestIDfor capability tracking; the original lookup logic is fully restored.How did you test each of these updates (required)
livepeer-python-gatewaychanges in feat(byoc): client-side BYOC streaming + remote signer integration eliteprox/livepeer-python-gateway#2SignBYOCJobRequest(missingid/capability/timeout_seconds→ 400).LastUpdateproducespixels == preloadSecs * PixelsPerUnit; fractional second handling verified.TestResolvePriceInfo_BYOC_AggregateWhenCapabilitiesPricesOmittedfor theCapabilitiesPrices-omitted code path.TestGetOrchestratorto assert empty (not nil)CapabilitiesPriceswhen no external capabilities are registered, andTestGetOrchestrator_CapabilitiesPricesErrorto confirm transient errors are ignored.capabilityfield fromRemotePaymentRequest.makeand./test.shpass locally; validated bothlv2vandbyocpaths end-to-end with the python-gateway SDK (see eliteprox/python-gateway#2).Does this pull request close any open issues?
No open issues, but it resolves the signing-oracle concern raised in the March review: the V1 binary format with its domain separator ensures that an arbitrary
request/parameterspayload cannot be replayed as a valid signature for a different capability or job ID.Checklist:
makeruns successfully./test.shpassSummary by CodeRabbit
New Features
POST /sign-byoc-jobendpoint to sign BYOC jobs using a structured signing payload (withtimeout_secondsvalidation).Bug Fixes
Breaking Changes
Livepeer-Paymentheader (HTTP 400, “Could not parse payment”).