Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions internal/mcp/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -158,13 +158,24 @@ func NewServer(opts Options) (*Server, error) {
al = &auditLogger{} // no-op: enc is nil, log() returns early
}

// Resolve the profile once for both consumers (step-up + signer).
// SigningProfile is the explicit opt-in; empty means "use the active
// profile." Pre-KLA-420 this fallback lived only in the signer block,
// so a webhook-only setup (no signing) emitted `"profile": ""` in
// every outbound envelope. Receivers keying on profile (Slack bot
// routing rules, audit dashboards) saw empty strings.
profile := opts.SigningProfile
if profile == "" {
profile = config.ActiveProfile()
}

stepUp := opts.stepUp
if stepUp == nil {
su, err := newStepUp(stepUpConfig{
Required: opts.RequireStepUp,
APIKey: opts.StepUpAPIKey,
AuthenticatorPref: opts.StepUpAuthenticator,
Profile: opts.SigningProfile,
Profile: profile,
WebhookURL: opts.ApprovalWebhookURL,
WebhookCallbackAddr: opts.ApprovalCallbackAddr,
WebhookTimeout: opts.ApprovalTimeout,
Expand All @@ -177,10 +188,6 @@ func NewServer(opts Options) (*Server, error) {

signer := opts.signer
if signer == nil {
profile := opts.SigningProfile
if profile == "" {
profile = config.ActiveProfile()
}
signer = newSigner(opts.SignDestructiveOps, profile)
}

Expand Down
56 changes: 56 additions & 0 deletions internal/mcp/stepup_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,62 @@ func TestWebhookStepUp_RemediationIsChannelAware(t *testing.T) {
}
}

// KLA-420: when an operator enables --require-step-up
// --step-up-authenticator=webhook without also enabling signing
// (mcp.sign_destructive_ops), the webhook envelope's profile field
// must still carry the active profile name. Pre-fix, server.go wired
// Profile = opts.SigningProfile directly with no fallback, so
// webhook-only setups emitted `"profile": ""` to receivers.
func TestNewServer_WebhookProfileFallsBackToActiveProfile(t *testing.T) {
setupTest(t)

s := MustNewServer(Options{
RequireStepUp: true,
StepUpAuthenticator: "webhook",
ApprovalWebhookURL: "http://127.0.0.1:1/notused",
ApprovalCallbackAddr: "127.0.0.1:0",
ApprovalTimeout: 1 * time.Second,
// SigningProfile intentionally left empty — that's the bug:
// pre-fix this would propagate `profile: ""` into the webhook.
})
t.Cleanup(s.shutdownStepUp)

w, ok := s.stepUp.(*webhookStepUp)
if !ok {
t.Fatalf("expected *webhookStepUp, got %T", s.stepUp)
}
// setupTest configures active_profile=default; resolution must pick
// that up rather than leaving the field empty.
if w.profile != "default" {
t.Errorf("webhookStepUp.profile = %q, want %q (active profile fallback)", w.profile, "default")
}
}

// Companion: an explicit SigningProfile must still flow through
// unchanged. The fallback only kicks in when the explicit value is
// empty.
func TestNewServer_WebhookProfileHonorsExplicitSigningProfile(t *testing.T) {
setupTest(t)

s := MustNewServer(Options{
RequireStepUp: true,
StepUpAuthenticator: "webhook",
ApprovalWebhookURL: "http://127.0.0.1:1/notused",
ApprovalCallbackAddr: "127.0.0.1:0",
ApprovalTimeout: 1 * time.Second,
SigningProfile: "staging",
})
t.Cleanup(s.shutdownStepUp)

w, ok := s.stepUp.(*webhookStepUp)
if !ok {
t.Fatalf("expected *webhookStepUp, got %T", s.stepUp)
}
if w.profile != "staging" {
t.Errorf("webhookStepUp.profile = %q, want %q (explicit SigningProfile)", w.profile, "staging")
}
}

func TestNewWebhookStepUp_RejectsEmptyURL(t *testing.T) {
_, err := newWebhookStepUp("", "", 0, "default")
if err == nil {
Expand Down
Loading