Conversation
📝 WalkthroughWalkthroughThe change adds SSM-backed CMR system-token resolution, supports system-token authorization, propagates parameter configuration through CDK and deployment environments, and updates metadata writeback and local request authentication behavior. ChangesCMR system token authentication
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant edlAuthorizer
participant getCmrSystemToken
participant SSM
participant fetchEdlProfile
Client->>edlAuthorizer: Send authorization token
edlAuthorizer->>getCmrSystemToken: Resolve system token
getCmrSystemToken->>SSM: Get configured parameter
SSM-->>getCmrSystemToken: Return token
getCmrSystemToken-->>edlAuthorizer: Return normalized token
alt Presented token matches system token
edlAuthorizer-->>Client: Allow as cmr-system
else Token does not match
edlAuthorizer->>fetchEdlProfile: Validate presented token
fetchEdlProfile-->>edlAuthorizer: Return profile or failure
edlAuthorizer-->>Client: Allow or deny
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
🔧 Fix failing CI
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #125 +/- ##
==========================================
- Coverage 99.73% 99.71% -0.03%
==========================================
Files 229 234 +5
Lines 6029 6305 +276
Branches 1773 1854 +81
==========================================
+ Hits 6013 6287 +274
- Misses 14 16 +2
Partials 2 2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
serverless/src/shared/getCmrWriterToken.js (2)
68-74: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider
logger.errorfor SSM read failures.When
CMR_SYSTEM_TOKEN_PARAMETER_NAMEis configured, a failed read means system-token authorization silently stops working (authorizer denies, writeback skips). A warn-level log with onlyerror.messagemakes that outage easy to miss; error level plus the parameter name would be more actionable.🤖 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 `@serverless/src/shared/getCmrWriterToken.js` around lines 68 - 74, Update the SSM read failure handling in getCmrWriterToken to use logger.error instead of logger.warn, and include CMR_SYSTEM_TOKEN_PARAMETER_NAME in the structured log context alongside the existing error message.
26-34: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the SSM client and resolved token; this runs on every authorizer invocation.
createSsmClient()builds a freshSSMClientper call andreadSystemTokenFromSsmissues aGetParameteron everygetCmrSystemToken()/getCmrWriterToken()call. Sinceserverless/src/edlAuthorizer/handler.js(line 54) calls this on every request, each authorized API call pays an SSM round trip and risks SSM GetParameter throttling under load. A module-scoped client plus a short-TTL cached value keeps warm invocations off the SSM hot path.♻️ Sketch: reuse client, cache token with TTL
+let cachedSsmClient +let cachedToken +let cachedTokenExpiresAt = 0 +const TOKEN_CACHE_TTL_MS = 5 * 60 * 1000 + const createSsmClient = () => { + if (cachedSsmClient) { + return cachedSsmClient + } + const endpoint = String(process.env.AWS_ENDPOINT_URL || '').trim() const region = String(process.env.AWS_REGION || process.env.AWS_DEFAULT_REGION || '').trim() - return new SSMClient({ + cachedSsmClient = new SSMClient({ ...(endpoint ? { endpoint } : {}), ...(region ? { region } : {}) }) + + return cachedSsmClient }const readSystemTokenFromSsm = async (parameterName) => { if (!parameterName) { return '' } + if (cachedToken !== undefined && Date.now() < cachedTokenExpiresAt) { + return cachedToken + } + try { const response = await createSsmClient().send(new GetParameterCommand({ Name: parameterName, WithDecryption: true })) - return String(response.Parameter?.Value || '').trim() + cachedToken = String(response.Parameter?.Value || '').trim() + cachedTokenExpiresAt = Date.now() + TOKEN_CACHE_TTL_MS + + return cachedToken } catch (error) {Also applies to: 56-75
🤖 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 `@serverless/src/shared/getCmrWriterToken.js` around lines 26 - 34, Update createSsmClient, readSystemTokenFromSsm, and the getCmrSystemToken/getCmrWriterToken flow to reuse a module-scoped SSMClient and cache the resolved token with a short TTL. Only issue GetParameter when the cached value is absent or expired, while preserving existing token resolution and error behavior.serverless/src/edlAuthorizer/handler.js (2)
16-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate bearer-strip logic; export the shared helper instead.
This is the same normalization as
stripBearerPrefixinserverless/src/shared/getCmrWriterToken.js(lines 43-48). If one copy changes, presented tokens and configured tokens stop comparing equal and system-token auth silently breaks. Export the shared helper and reuse it here.🤖 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 `@serverless/src/edlAuthorizer/handler.js` around lines 16 - 21, Remove the duplicate normalizePresentedToken implementation and export the existing stripBearerPrefix helper from getCmrWriterToken.js. Import and reuse stripBearerPrefix in the authorizer token normalization flow so presented and configured tokens share the same behavior.
57-61: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse a timing-safe comparison for the system token.
presentedToken === resolvedSystemTokenshort-circuits on the first differing byte. Since this compares a long-lived shared secret on a public authorizer endpoint, prefer a constant-time compare (e.g.crypto.timingSafeEqualover equal-length digests of both values).🔒 Sketch
+import { createHash, timingSafeEqual } from 'crypto' + +const tokensMatch = (a, b) => timingSafeEqual( + createHash('sha256').update(a).digest(), + createHash('sha256').update(b).digest() +)- if (resolvedSystemToken && presentedToken === resolvedSystemToken) { + if (resolvedSystemToken && presentedToken && tokensMatch(presentedToken, resolvedSystemToken)) {🤖 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 `@serverless/src/edlAuthorizer/handler.js` around lines 57 - 61, Update the system-token validation in the authorizer handler to use a timing-safe comparison instead of direct equality. Compare equal-length cryptographic digests of presentedToken and resolvedSystemToken with crypto.timingSafeEqual, while preserving the existing success policy and ensuring mismatched token lengths cannot cause an exception.
🤖 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.
Nitpick comments:
In `@serverless/src/edlAuthorizer/handler.js`:
- Around line 16-21: Remove the duplicate normalizePresentedToken implementation
and export the existing stripBearerPrefix helper from getCmrWriterToken.js.
Import and reuse stripBearerPrefix in the authorizer token normalization flow so
presented and configured tokens share the same behavior.
- Around line 57-61: Update the system-token validation in the authorizer
handler to use a timing-safe comparison instead of direct equality. Compare
equal-length cryptographic digests of presentedToken and resolvedSystemToken
with crypto.timingSafeEqual, while preserving the existing success policy and
ensuring mismatched token lengths cannot cause an exception.
In `@serverless/src/shared/getCmrWriterToken.js`:
- Around line 68-74: Update the SSM read failure handling in getCmrWriterToken
to use logger.error instead of logger.warn, and include
CMR_SYSTEM_TOKEN_PARAMETER_NAME in the structured log context alongside the
existing error message.
- Around line 26-34: Update createSsmClient, readSystemTokenFromSsm, and the
getCmrSystemToken/getCmrWriterToken flow to reuse a module-scoped SSMClient and
cache the resolved token with a short TTL. Only issue GetParameter when the
cached value is absent or expired, while preserving existing token resolution
and error behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4757239c-204e-4303-be4e-98e36fa0ee46
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
bin/deploy-bamboo.shcdk/app/lib/CmrEventProcessingStack.tscdk/app/lib/KmsStack.tscdk/app/lib/helper/KmsLambdaFunctions.tscdk/app/lib/helper/MetadataCorrectionSetup.tscdk/bin/main.tspackage.jsonscripts/local/create_unique_keyword.shserverless/src/edlAuthorizer/__tests__/handler.test.jsserverless/src/edlAuthorizer/handler.jsserverless/src/getConcepts/__tests__/handler.test.jsserverless/src/shared/__tests__/getCmrWriterToken.test.jsserverless/src/shared/__tests__/writeCorrectedMetadataToCmr.test.jsserverless/src/shared/getCmrWriterToken.jsserverless/src/shared/writeCorrectedMetadataToCmr.js
cgokey
changed the title
cgokey
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (2)
serverless/src/edlAuthorizer/handler.js (1)
40-47: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider a timing-safe comparison for the system token.
presentedToken === resolvedSystemTokenis a non-constant-time comparison of a secret bearer value. For defense-in-depth, considercrypto.timingSafeEqual(with equal-length buffers, e.g., hash both sides first) instead of plain===.import { timingSafeEqual, createHash } from 'node:crypto' const safeEquals = (a, b) => { const bufA = createHash('sha256').update(String(a)).digest() const bufB = createHash('sha256').update(String(b)).digest() return timingSafeEqual(bufA, bufB) }🤖 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 `@serverless/src/edlAuthorizer/handler.js` around lines 40 - 47, Replace the plain equality check in the system-token branch of the authorizer handler with a timing-safe comparison using the existing token values, hashing or otherwise normalizing both operands to equal-length buffers before calling crypto.timingSafeEqual. Preserve the current authorization success path and policy generation behavior.serverless/src/shared/getCmrWriterToken.js (1)
95-103: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNo caching of the SSM-resolved token/client — every call pays a network round trip.
getCmrSystemToken(and transitivelygetCmrWriterToken) performs a liveGetParameterCommandcall on every invocation with no in-memory memoization of either the resolved token or theSSMClientinstance. Since this is called for every metadata writeback attempt (and separately, on everyedlAuthorizerinvocation), this adds latency/cost per call and increases exposure to SSM throttling under load. Consider caching the resolved token (with a short TTL to allow rotation) and/or theSSMClientinstance at module scope so warm Lambda containers reuse them across invocations.Also applies to: 115-129
🤖 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 `@serverless/src/shared/getCmrWriterToken.js` around lines 95 - 103, Add module-scope memoization for the SSM client and the resolved token used by getCmrSystemToken, with a short expiration so token rotation remains effective. Reuse the cached value while valid, otherwise call readSystemTokenFromSsm and refresh the cache; preserve the existing authorization-header resolution and undefined fallback. Ensure getCmrWriterToken transitively benefits without changing its public behavior.
🤖 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.
Nitpick comments:
In `@serverless/src/edlAuthorizer/handler.js`:
- Around line 40-47: Replace the plain equality check in the system-token branch
of the authorizer handler with a timing-safe comparison using the existing token
values, hashing or otherwise normalizing both operands to equal-length buffers
before calling crypto.timingSafeEqual. Preserve the current authorization
success path and policy generation behavior.
In `@serverless/src/shared/getCmrWriterToken.js`:
- Around line 95-103: Add module-scope memoization for the SSM client and the
resolved token used by getCmrSystemToken, with a short expiration so token
rotation remains effective. Reuse the cached value while valid, otherwise call
readSystemTokenFromSsm and refresh the cache; preserve the existing
authorization-header resolution and undefined fallback. Ensure getCmrWriterToken
transitively benefits without changing its public behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 808a1452-4f63-4a37-b092-0b9c680ff290
📒 Files selected for processing (6)
serverless/src/edlAuthorizer/__tests__/handler.test.jsserverless/src/edlAuthorizer/handler.jsserverless/src/shared/__tests__/getCmrWriterToken.test.jsserverless/src/shared/__tests__/writeCorrectedMetadataToCmr.test.jsserverless/src/shared/getCmrWriterToken.jsserverless/src/shared/writeCorrectedMetadataToCmr.js
🚧 Files skipped from review as they are similar to previous changes (1)
- serverless/src/shared/writeCorrectedMetadataToCmr.js
2 participants
Overview
What is the feature?
This change adds support for using the CMR system token as the primary runtime token for metadata-correction writeback and reconciliation-related authorization, with
CMR_WRITER_TOKENretained as the fallback when the system token is unavailable.What is the Solution?
CMR_WRITER_TOKENwhen the system token is not available.AUTHORIZATION_HEADERto the localcreate_unique_keyword.shhelper so protected environments can be exercised without rewriting the script.What areas of the application does this impact?
Testing
Verify you can hit a KMS authenticated endpoint with the system token.
Try publishing a new version, verify KMS can properly use the system token for write access.
Checklist
Summary by CodeRabbit
New Features
Bearer-prefixed values and whitespace trimming.AUTHORIZATION_HEADERsupport to local keyword generation.Bug Fixes
Tests / Chores