From d00963c330bfe5cf396329b545a232ed939841b0 Mon Sep 17 00:00:00 2001 From: Sharon Katz <121527325+sharon-fdm@users.noreply.github.com> Date: Wed, 24 Jun 2026 16:53:37 -0400 Subject: [PATCH] Tighten conditional-access SCEP challenge validation (#48041) **Related issue:** N/A # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [x] 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. - [x] Timeouts are implemented and retries are limited to avoid infinite loops - [x] If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually ## 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 `challengeMiddleware` in `ee/server/service/condaccess/scep.go` calls `ds.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 1. **Unit tests (`scep_test.go`)** - table-driven test with 5 cases exercising the `challengeMiddleware` directly: - Empty challenge -> rejected ("missing challenge") - Unknown secret -> rejected ("invalid challenge") - Team-scoped secret (team A) -> rejected ("invalid challenge") *[new behavior]* - Team-scoped secret (team B) -> rejected ("invalid challenge") *[new behavior]* - Global secret (team_id = nil) -> accepted, signer invoked, cert returned 2. **Ran `go test -v ./ee/server/service/condaccess/`** - all tests pass. 3. **Ran `make lint-go-incremental`** - 0 issues. ## Summary by CodeRabbit * **Bug Fixes** * Improved validation for conditional access SCEP enrollment, including rejecting team-scoped enrollment secrets. * Updated challenge middleware behavior to better handle enrollment secret verification outcomes and associated error messaging. * **Tests** * Added comprehensive coverage for conditional access SCEP challenge validation, including cases for missing, unknown, team-scoped, and global enrollment secrets, plus signer invocation expectations. --- changes/fix-condaccess-scep-validation | 1 + ee/server/service/condaccess/scep.go | 7 +- ee/server/service/condaccess/scep_test.go | 94 +++++++++++++++++++++++ 3 files changed, 101 insertions(+), 1 deletion(-) create mode 100644 changes/fix-condaccess-scep-validation create mode 100644 ee/server/service/condaccess/scep_test.go diff --git a/changes/fix-condaccess-scep-validation b/changes/fix-condaccess-scep-validation new file mode 100644 index 0000000000..1700c7f604 --- /dev/null +++ b/changes/fix-condaccess-scep-validation @@ -0,0 +1 @@ +- Improved input validation for conditional access SCEP enrollment. diff --git a/ee/server/service/condaccess/scep.go b/ee/server/service/condaccess/scep.go index 294b5f18da..e2a9408122 100644 --- a/ee/server/service/condaccess/scep.go +++ b/ee/server/service/condaccess/scep.go @@ -96,13 +96,18 @@ func challengeMiddleware(ds fleet.Datastore, next scepserver.CSRSignerContext) s if m.ChallengePassword == "" { return nil, errors.New("missing challenge") } - _, err := ds.VerifyEnrollSecret(ctx, m.ChallengePassword) + secret, err := ds.VerifyEnrollSecret(ctx, m.ChallengePassword) switch { case fleet.IsNotFound(err): return nil, errors.New("invalid challenge") case err != nil: return nil, fmt.Errorf("verifying enrollment secret: %w", err) } + // Only global enroll secrets (team_id IS NULL) are valid for + // conditional-access SCEP. Reject team-scoped secrets. + if secret.TeamID != nil { + return nil, errors.New("invalid challenge") + } return next.SignCSRContext(ctx, m) } } diff --git a/ee/server/service/condaccess/scep_test.go b/ee/server/service/condaccess/scep_test.go new file mode 100644 index 0000000000..630b889b8f --- /dev/null +++ b/ee/server/service/condaccess/scep_test.go @@ -0,0 +1,94 @@ +package condaccess + +import ( + "context" + "crypto/x509" + "testing" + + "github.com/fleetdm/fleet/v4/server/fleet" + scepserver "github.com/fleetdm/fleet/v4/server/mdm/scep/server" + "github.com/fleetdm/fleet/v4/server/mock" + common_mysql "github.com/fleetdm/fleet/v4/server/platform/mysql" + "github.com/smallstep/scep" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestChallengeMiddleware(t *testing.T) { + teamAID := uint(1) + teamBID := uint(2) + + cases := []struct { + name string + challenge string + wantErr string + wantSignCalled bool + }{ + { + name: "empty challenge is rejected", + challenge: "", + wantErr: "missing challenge", + }, + { + name: "unknown secret is rejected", + challenge: "unknown-secret", + wantErr: "invalid challenge", + }, + { + name: "team-scoped secret is rejected", + challenge: "secret-team-a", + wantErr: "invalid challenge", + }, + { + name: "different team-scoped secret is also rejected", + challenge: "secret-team-b", + wantErr: "invalid challenge", + }, + { + name: "global secret is accepted", + challenge: "global-secret", + wantSignCalled: true, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ds := new(mock.DataStore) + 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 + default: + return nil, common_mysql.NotFound("enroll_secret") + } + } + + signCalled := false + dummySigner := scepserver.CSRSignerContextFunc( + func(_ context.Context, _ *scep.CSRReqMessage) (*x509.Certificate, error) { + signCalled = true + return &x509.Certificate{}, nil + }, + ) + + mw := challengeMiddleware(ds, dummySigner) + cert, err := mw.SignCSRContext(t.Context(), &scep.CSRReqMessage{ + ChallengePassword: tc.challenge, + }) + + if tc.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErr) + assert.Nil(t, cert) + } else { + require.NoError(t, err) + assert.NotNil(t, cert) + } + assert.Equal(t, tc.wantSignCalled, signCalled, "unexpected signer invocation") + }) + } +}