Tighten conditional-access SCEP challenge validation#48041
Conversation
Restrict SCEP enrollment to global enroll secrets only, rejecting team-scoped secrets. Add unit tests for the challenge middleware.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ee/server/service/condaccess/scep_test.go (1)
21-67: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a generic datastore-error test case to cover the remaining middleware branch.
The table currently misses the
err != nil && !IsNotFound(err)path ("verifying enrollment secret: %w"), so a regression there would be untested.Suggested patch
import ( "context" "crypto/x509" + "errors" "testing" @@ cases := []struct { name string challenge string wantErr string wantSignCalled bool }{ @@ { name: "unknown secret is rejected", challenge: "unknown-secret", wantErr: "invalid challenge", }, + { + name: "datastore error is returned as verification failure", + challenge: "datastore-error", + wantErr: "verifying enrollment secret", + }, @@ ds.VerifyEnrollSecretFunc = func(_ context.Context, secret string) (*fleet.EnrollSecret, error) { switch secret { case "secret-team-a": return &fleet.EnrollSecret{Secret: secret, TeamID: &teamAID}, nil case "secret-team-b": return &fleet.EnrollSecret{Secret: secret, TeamID: &teamBID}, nil case "global-secret": return &fleet.EnrollSecret{Secret: secret, TeamID: nil}, nil + case "datastore-error": + return nil, errors.New("db unavailable") default: return nil, common_mysql.NotFound("enroll_secret") } }🤖 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 `@ee/server/service/condaccess/scep_test.go` around lines 21 - 67, Add a new test case to the cases table that covers the generic datastore error path. Add a test case with a unique challenge value (like "db-error") that returns a non-NotFound error from the VerifyEnrollSecretFunc mock. In the VerifyEnrollSecretFunc switch statement, handle this challenge value by returning a generic error (not common_mysql.NotFound), and set the wantErr field in the test case to match the expected error message format "verifying enrollment secret:". This will ensure the err != nil && !IsNotFound(err) code path is tested for regressions.
🤖 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 `@ee/server/service/condaccess/scep_test.go`:
- Around line 21-67: Add a new test case to the cases table that covers the
generic datastore error path. Add a test case with a unique challenge value
(like "db-error") that returns a non-NotFound error from the
VerifyEnrollSecretFunc mock. In the VerifyEnrollSecretFunc switch statement,
handle this challenge value by returning a generic error (not
common_mysql.NotFound), and set the wantErr field in the test case to match the
expected error message format "verifying enrollment secret:". This will ensure
the err != nil && !IsNotFound(err) code path is tested for regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a1b75e16-5849-40fc-90c0-df0b396debde
📒 Files selected for processing (3)
changes/fix-condaccess-scep-validationee/server/service/condaccess/scep.goee/server/service/condaccess/scep_test.go
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
This PR tightens conditional-access SCEP challenge validation by ensuring only global enrollment secrets (team_id NULL) are accepted by the conditional-access SCEP middleware, preventing team-scoped secrets from being used to satisfy the SCEP challenge.
Changes:
- Update
challengeMiddlewareto reject team-scoped enroll secrets even if they are otherwise valid. - Add unit tests covering missing/unknown/team-scoped/global secret challenge scenarios.
- Add a changes entry (
changes/fix-condaccess-scep-validation) (content excluded by policy).
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ee/server/service/condaccess/scep.go | Enforces that conditional-access SCEP challenges only accept global (teamless) enroll secrets. |
| ee/server/service/condaccess/scep_test.go | Adds table-driven unit coverage for the middleware’s new secret-scope validation behavior. |
| changes/fix-condaccess-scep-validation | User-visible changes entry (not reviewed here due to content exclusion policy). |
Files excluded by content exclusion policy (1)
- changes/fix-condaccess-scep-validation
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if tc.wantErr != "" { | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), tc.wantErr) | ||
| assert.False(t, signCalled, "signer should not be called on error") | ||
| assert.Nil(t, cert) | ||
| } else { | ||
| require.NoError(t, err) | ||
| assert.True(t, signCalled) | ||
| assert.NotNil(t, cert) | ||
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #48041 +/- ##
========================================
Coverage 67.31% 67.31%
========================================
Files 3655 3655
Lines 231242 231241 -1
Branches 12224 12074 -150
========================================
+ Hits 155658 155660 +2
Misses 61618 61618
+ Partials 13966 13963 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Related issue: N/A
Checklist for submitter
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Summary
Tightens validation in the conditional-access SCEP challenge middleware to reject enrollment requests that use a secret outside the expected scope. Adds unit test coverage for the middleware.
Reproduction
The
challengeMiddlewareinee/server/service/condaccess/scep.gocallsds.VerifyEnrollSecret()but discards the returned secret (_, err :=), so it only checks that some valid enroll secret exists. It does not verify the secret's scope. Any valid secret from any scope passes the challenge.This was confirmed with a unit test using mock secrets scoped to different teams. Before the fix, the middleware accepted all of them indiscriminately. The server-side profile generation (
server/service/conditional_access_idp.go) only ever embeds a global-scope secret as the SCEP challenge, so only global secrets should be accepted.How it was tested
Unit tests (
scep_test.go) - table-driven test with 5 cases exercising thechallengeMiddlewaredirectly:Ran
go test -v ./ee/server/service/condaccess/- all tests pass.Ran
make lint-go-incremental- 0 issues.Summary by CodeRabbit
Bug Fixes
Tests