Skip to content

hardening(auth): equalize bearer compare operands on length mismatch - #814

Draft
seonghobae wants to merge 11 commits into
developfrom
sentinel-timing-attack-fix-12901296301896663472
Draft

hardening(auth): equalize bearer compare operands on length mismatch#814
seonghobae wants to merge 11 commits into
developfrom
sentinel-timing-attack-fix-12901296301896663472

Conversation

@seonghobae

@seonghobae seonghobae commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Current authority

  • protected/base: develop@e06b1f3fb10903569124af011da213951e6e2473
  • exact head: e40475966b38aa75dae6caa81ba8408ef319dba5
  • ancestry: protected base is the exact merge base; ahead 5 / behind 0
  • effective delta: src/newsdom_api/main.py plus tests/test_auth_compare_digest.py
  • lifecycle: Draft

Finding classification

This is defense-in-depth hardening, not a demonstrated MEDIUM vulnerability or a claim of wall-clock constant time. Python documents hmac.compare_digest() as content-short-circuit resistant and notes only that different operand lengths can theoretically reveal type/length information, not values. The repository has not established that the configured bearer-token length is a protected secret, nor has it supplied a remote timing experiment that demonstrates exploitable token recovery.

The previous PR wording therefore overstated both severity and verification. It is removed from the acceptance authority.

Candidate invariant

For a syntactically valid Bearer credential:

  • if the presented credential length differs from the configured token length, authentication still fails with the existing fixed 401 response, but compare_digest() is invoked with equal-length operands (credentials, credentials) rather than unequal operands;
  • if lengths match, the presented credential is compared against the configured token exactly as before;
  • malformed, missing, duplicate, oversized and non-ASCII authorization inputs keep the existing fail-closed boundary and fixed response contract.

The focused tests do not claim to measure timing. They deterministically spy on the comparison boundary and require a wrong-length credential to reach compare_digest() with equal-length operands, while an equal-length wrong credential must still compare against the configured token. The current descendant also types the pytest monkeypatch fixture so the regression does not introduce an avoidable untyped-test lint boundary.

Scope repair

A concurrent/generated uv.lock change upgraded pypdf 6.15.0→6.17.0 and changed unrelated dependency markers. That delta has no causal relationship to bearer comparison and was restored to the exact protected-base blob through normal descendant 347c1fd6464663c771e08a720167e10076870cd2. No force push or destructive rebase was used.

Acceptance

Do not merge this as a security vulnerability fix merely because the structural tests pass. Exact-head correctness/security/static-analysis gates must terminate successfully and review must confirm the extra branch/compare is preferable to the simpler protected implementation. Any claim that it makes request handling 'constant time', hides all timing information, or materially changes exploitability requires an explicit threat model and representative timing evidence; otherwise keep the scope as defense-in-depth only.

Fresh exact-head workflows for e40475966... are non-terminal: tests 34000560041, Security Scan 34000560033, SAST Semgrep 34000560061, CodeQL PR 34000560056, codeql 34000560053, clusterfuzzlite 34000560019, container-image 34000560044, and scorecards 34000560038. Predecessor GREEN does not transfer.

@google-labs-jules

Copy link
Copy Markdown

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@coderabbitai

coderabbitai Bot commented Sep 5, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: 8e130263-7b4b-4fbb-9aaf-b4c45d198812

📥 Commits

Reviewing files that changed from the base of the PR and between e06b1f3 and d01aecc.

📒 Files selected for processing (1)
  • src/newsdom_api/main.py

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

Bearer 자격 증명의 길이가 설정된 토큰과 달라도 더미 비교를 수행한 후 401 응답을 반환합니다. 길이가 같으면 기존처럼 hmac.compare_digest로 토큰을 비교합니다.

Changes

인증 실패 처리

Layer / File(s) Summary
상수 시간 인증 비교
src/newsdom_api/main.py
Bearer 자격 증명의 길이가 다르면 더미 자기 비교를 수행한 후 인증 실패를 반환합니다. 길이가 같으면 설정된 토큰과 상수 시간 비교를 수행합니다.

Estimated code review effort: 1 (간단) | ~5분

Merge Risk: ⚪ Minimal · up to d01ae

Bearer 토큰 길이가 다른 인증 요청도 비교 후 401로 거부하도록 변경되며, 기존 실패 동작과 인증 경계가 유지됩니다. 현재 병합을 막을 구체적인 위험은 확인되지 않았습니다.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning 설명은 변경 목적과 수용 조건을 자세히 설명하지만, 템플릿의 ## Summary, ## Git Flow target, ## Verification, ## Notes 섹션을 포함하지 않습니다. 테스트 실행 결과와 Git Flow 대상도 확인할 수 없습니다. 템플릿에 맞게 ## Summary, ## Git Flow target, ## Verification, ## Notes 섹션을 추가하세요. 대상 브랜치를 명시하고, pytestPYTHONWARNINGS=error pytest 실행 여부를 체크한 뒤 결과를 기록하세요. 필요한 릴리스 또는 핫픽스 후속 작업이 없으면 없음으로 명시하세요.
✅ Passed checks (4 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed 제목은 길이 불일치 시 Bearer 비교 피연산자를 동일하게 만드는 주요 변경을 정확하고 간결하게 설명합니다.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sentinel-timing-attack-fix-12901296301896663472

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

coderabbitai[bot]
coderabbitai Bot previously approved these changes Sep 5, 2026
coderabbitai[bot]
coderabbitai Bot previously approved these changes Sep 5, 2026
@seonghobae
seonghobae marked this pull request as draft September 6, 2026 00:06
@seonghobae seonghobae changed the title 🛡️ Sentinel: [MEDIUM] HMAC 길이 비교 타이밍 공격 취약점 수정 hardening(auth): equalize bearer compare operands on length mismatch Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant