feat: deliver enclave credential with IRSA#1013
Merged
Merged
Conversation
tomatoishealthy
requested review from
SecurityLife
and removed request for
a team
Contributor
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (9)
📒 Files selected for processing (10)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds AWS/IRSA credential injection for the enclave signer, including startup initialization, STS credential exchange, wire encoding, and retrying injection when the enclave reports an uninitialized key. ChangesEnclave Credential Injection
Estimated code review effort: 4 (Complex) | ~45 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@node/l1sequencer/enclave_creds.go`:
- Around line 114-117: The STS failure path in enclave_creds.go currently
returns the full response body from resp.Body, which can leak sensitive token
data when injectCredentials() logs the error. Update the error handling in the
STS response check to return a bounded/redacted message instead of
strings.TrimSpace(string(raw)), keeping the status code but sanitizing or
omitting the external body content. Use the existing sts fetch/error-return
logic in the credential-loading flow to locate the fix.
- Around line 132-143: The STS credential parsing in awsCreds creation only
checks AccessKeyID and SecretAccessKey, so update the enclave_creds.go flow to
also validate SessionToken and Expiration before returning usable temporary
credentials. In the logic around parsed.Result.Credentials and awsCreds{}, treat
empty sessionToken or missing/invalid c.Expiration as an error, and keep the
existing time.Parse validation path in the same Get/parse credentials handling
so AssumeRoleWithWebIdentity never returns ambiguous creds.
- Around line 70-72: The IRSA environment check currently treats a partially
configured setup as if credentials are simply absent. Update the
`roleARN`/`tokenFile` guard in the enclave credentials path so that
`AWS_ROLE_ARN` or `AWS_WEB_IDENTITY_TOKEN_FILE` being set alone is considered a
misconfiguration, not a silent skip. In the `awsCreds` handling logic, return an
explicit error for partial IRSA env instead of `awsCreds{}, false, nil`, while
keeping the existing no-op path only when both values are empty.
- Around line 154-173: Guard the LP-string encoding in appendLPString used by
writeProvideCredentials so oversized credential fields cannot be silently
narrowed to uint16 and wrapped into a malformed frame. Update appendLPString (or
its callers in writeProvideCredentials) to check len(s) before encoding, return
an error when a field exceeds uint16 max, and propagate that error back from
writeProvideCredentials instead of building the buffer with invalid lengths.
🪄 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: e615bf1a-18e1-4fb4-a0e1-3920e3ed7d1a
📒 Files selected for processing (2)
node/l1sequencer/enclave_creds.gonode/l1sequencer/enclave_signer.go
Comment on lines
+70
to
+72
| if roleARN == "" || tokenFile == "" { | ||
| return awsCreds{}, false, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Treat partial IRSA env as misconfiguration.
Line 70 skips injection when either AWS_ROLE_ARN or AWS_WEB_IDENTITY_TOKEN_FILE is missing, which hides broken IRSA setup until a later enclave probe fails with less context.
Proposed fix
- if roleARN == "" || tokenFile == "" {
+ if roleARN == "" && tokenFile == "" {
return awsCreds{}, false, nil
}
+ if roleARN == "" || tokenFile == "" {
+ return awsCreds{}, true, fmt.Errorf("incomplete IRSA env: AWS_ROLE_ARN and AWS_WEB_IDENTITY_TOKEN_FILE are both required")
+ }📝 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
| if roleARN == "" || tokenFile == "" { | |
| return awsCreds{}, false, nil | |
| } | |
| if roleARN == "" && tokenFile == "" { | |
| return awsCreds{}, false, nil | |
| } | |
| if roleARN == "" || tokenFile == "" { | |
| return awsCreds{}, true, fmt.Errorf("incomplete IRSA env: AWS_ROLE_ARN and AWS_WEB_IDENTITY_TOKEN_FILE are both required") | |
| } |
🤖 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 `@node/l1sequencer/enclave_creds.go` around lines 70 - 72, The IRSA environment
check currently treats a partially configured setup as if credentials are simply
absent. Update the `roleARN`/`tokenFile` guard in the enclave credentials path
so that `AWS_ROLE_ARN` or `AWS_WEB_IDENTITY_TOKEN_FILE` being set alone is
considered a misconfiguration, not a silent skip. In the `awsCreds` handling
logic, return an explicit error for partial IRSA env instead of `awsCreds{},
false, nil`, while keeping the existing no-op path only when both values are
empty.
Contributor
Author
There was a problem hiding this comment.
Ignore, should check with logs
Contributor
There was a problem hiding this comment.
@tomatoishealthy, understood — makes sense to confirm actual behavior via logs before deciding whether to enforce a hard error for partial IRSA env. Let me know if you'd like to revisit this afterward.
(=^･ω･^=)
Validate accessKeyID/secretAccessKey/sessionToken lengths against the protocol caps in writeProvideCredentials before encoding, so an oversized field is rejected instead of wrapping the u16 length prefix into a malformed frame. Also require SessionToken in the AssumeRoleWithWebIdentity response. Caps mirror crates/protocol and ops-cli in morph-enclave-signer. Co-authored-by: Cursor <cursoragent@cursor.com>
chengwenxi
approved these changes
Jul 2, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@node/l1sequencer/enclave_signer.go`:
- Around line 163-166: In enclave_signer.go, the error path in the sign/retry
flow is still returning the original sign failure even when
s.injectCredentials() fails, so the caller never sees the real recovery error.
Update the logic around isKeyNotInitialized(err) in the signer method to
preserve the injection failure as the returned cause, and make sure the final
wrapped/returned error uses ierr when injectCredentials() fails instead of the
prior sign error.
🪄 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: 5e9a32f4-e95c-4b18-9242-39e58459b3ad
📒 Files selected for processing (1)
node/l1sequencer/enclave_signer.go
Comment on lines
+163
to
+166
| if isKeyNotInitialized(err) { | ||
| if ierr := s.injectCredentials(); ierr != nil { | ||
| s.logger.Error("re-inject credentials after key-not-loaded failed", "err", ierr) | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Preserve failed re-injection as the returned cause.
When injectCredentials() fails, the final error still wraps the prior sign error, hiding the actual recovery failure from the caller.
Suggested fix
if isKeyNotInitialized(err) {
if ierr := s.injectCredentials(); ierr != nil {
+ lastErr = fmt.Errorf("re-inject credentials after key-not-loaded: %w", ierr)
s.logger.Error("re-inject credentials after key-not-loaded failed", "err", ierr)
}
}📝 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
| if isKeyNotInitialized(err) { | |
| if ierr := s.injectCredentials(); ierr != nil { | |
| s.logger.Error("re-inject credentials after key-not-loaded failed", "err", ierr) | |
| } | |
| if isKeyNotInitialized(err) { | |
| if ierr := s.injectCredentials(); ierr != nil { | |
| lastErr = fmt.Errorf("re-inject credentials after key-not-loaded: %w", ierr) | |
| s.logger.Error("re-inject credentials after key-not-loaded failed", "err", ierr) | |
| } |
🤖 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 `@node/l1sequencer/enclave_signer.go` around lines 163 - 166, In
enclave_signer.go, the error path in the sign/retry flow is still returning the
original sign failure even when s.injectCredentials() fails, so the caller never
sees the real recovery error. Update the logic around isKeyNotInitialized(err)
in the signer method to preserve the injection failure as the returned cause,
and make sure the final wrapped/returned error uses ierr when
injectCredentials() fails instead of the prior sign error.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@Makefile`:
- Line 252: The SUBMITTERS extraction in the Makefile is using a grep pattern
that wrongly requires a non-colon character after the submitter number, so
tx-submitter-<n>: entries are missed and the helper targets end up empty. Update
the pattern used in the SUBMITTERS assignment so it matches the compose service
names directly without depending on a trailing non-colon character, and keep the
logic consistent with the existing sort/uniq pipeline.
🪄 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: 905e5f98-8840-4f42-aa07-d670d51b5a21
⛔ Files ignored due to path filters (9)
bindings/go.sumis excluded by!**/*.sumcommon/go.sumis excluded by!**/*.sumcontracts/go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumnode/go.sumis excluded by!**/*.sumops/l2-genesis/go.sumis excluded by!**/*.sumops/tools/go.sumis excluded by!**/*.sumtoken-price-oracle/go.sumis excluded by!**/*.sumtx-submitter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
Makefilebindings/go.modcommon/go.modcontracts/go.modnode/go.modnode/l1sequencer/enclave_signer.goops/l2-genesis/go.modops/tools/go.modtoken-price-oracle/go.modtx-submitter/go.mod
💤 Files with no reviewable changes (1)
- node/l1sequencer/enclave_signer.go
✅ Files skipped from review due to trivial changes (1)
- ops/l2-genesis/go.mod
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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 `@Makefile`:
- Line 252: The SUBMITTERS extraction in the Makefile is using a grep pattern
that wrongly requires a non-colon character after the submitter number, so
tx-submitter-<n>: entries are missed and the helper targets end up empty. Update
the pattern used in the SUBMITTERS assignment so it matches the compose service
names directly without depending on a trailing non-colon character, and keep the
logic consistent with the existing sort/uniq pipeline.
🪄 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: 905e5f98-8840-4f42-aa07-d670d51b5a21
⛔ Files ignored due to path filters (9)
bindings/go.sumis excluded by!**/*.sumcommon/go.sumis excluded by!**/*.sumcontracts/go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumnode/go.sumis excluded by!**/*.sumops/l2-genesis/go.sumis excluded by!**/*.sumops/tools/go.sumis excluded by!**/*.sumtoken-price-oracle/go.sumis excluded by!**/*.sumtx-submitter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
Makefilebindings/go.modcommon/go.modcontracts/go.modnode/go.modnode/l1sequencer/enclave_signer.goops/l2-genesis/go.modops/tools/go.modtoken-price-oracle/go.modtx-submitter/go.mod
💤 Files with no reviewable changes (1)
- node/l1sequencer/enclave_signer.go
✅ Files skipped from review due to trivial changes (1)
- ops/l2-genesis/go.mod
🛑 Comments failed to post (1)
Makefile (1)
252-252: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the
SUBMITTERSextraction pattern.
[^:]requires a non-colon character after the digits, sotx-submitter-<n>:entries in the compose file will not match and these helper targets will get an empty list.Proposed fix
-SUBMITTERS := $(shell grep -o 'tx-submitter-[0-9]*[^:]' ops/docker/docker-compose-devnet.yml | sort | uniq) +SUBMITTERS := $(shell grep -oE 'tx-submitter-[0-9]+' ops/docker/docker-compose-devnet.yml | sort -u)📝 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.SUBMITTERS := $(shell grep -oE 'tx-submitter-[0-9]+' ops/docker/docker-compose-devnet.yml | sort -u)🤖 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 `@Makefile` at line 252, The SUBMITTERS extraction in the Makefile is using a grep pattern that wrongly requires a non-colon character after the submitter number, so tx-submitter-<n>: entries are missed and the helper targets end up empty. Update the pattern used in the SUBMITTERS assignment so it matches the compose service names directly without depending on a trailing non-colon character, and keep the logic consistent with the existing sort/uniq pipeline.
FletcherMan
approved these changes
Jul 3, 2026
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
3 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 by CodeRabbit