Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: SSO MFA - WebUI backend implementation #47832

Merged
merged 11 commits into from
Nov 6, 2024
Merged

feat: SSO MFA - WebUI backend implementation #47832

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c27849f
Add SSO MFA ceremony support to WebUI per-session MFA.
Joerger Oct 8, 2024
1072419
Add display name to SSO MFA device; Add SSO MFA device to SSO challenge.
Joerger Oct 22, 2024
511d6cb
Camel case json tags for front end.
Joerger Oct 23, 2024
13d0cca
Add sso channel ID for front end broadcast channel logic.
Joerger Oct 23, 2024
384667e
Fix backwards compatiblity issue; fix error message.
Joerger Oct 23, 2024
4427375
Fix test.
Joerger Oct 28, 2024
fbe8f47
Fallback to connector id if display name isn't set.
Joerger Oct 28, 2024
890c01e
Fix test.
Joerger Oct 28, 2024
182a6c7
Resolve comment.
Joerger Oct 30, 2024
66bfc8e
Use uuid.NewRandom.
Joerger Nov 4, 2024
4e23a94
go mod tidy
Joerger Nov 5, 2024
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
2,013 changes: 1,038 additions & 975 deletions api/client/proto/authservice.pb.go
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 2 additions & 0 deletions api/proto/teleport/legacy/client/proto/authservice.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1253,6 +1253,8 @@ message SSOChallenge {
string request_id = 1;
// RedirectUrl is an IdP redirect URL to initate the SSO MFA flow.
string redirect_url = 2;
// Device is the SSO device corresponding to the challenge.
types.SSOMFADevice device = 3;
}

// SSOResponse is a response to SSOChallenge.
Expand Down
2 changes: 2 additions & 0 deletions api/proto/teleport/legacy/types/types.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3838,6 +3838,8 @@ message SSOMFADevice {
string connector_id = 1;
// connector_type is the type of the SSO connector.
string connector_type = 2;
// display_name is the display name of the SSO connector
string display_name = 3;
}

// WebauthnLocalAuth holds settings necessary for local webauthn use.
Expand Down
1,909 changes: 977 additions & 932 deletions api/types/types.pb.go
View file
Open in desktop

Large diffs are not rendered by default.

52 changes: 52 additions & 0 deletions integrations/event-handler/go.sum
View file
Open in desktop

Large diffs are not rendered by default.

5 changes: 5 additions & 0 deletions lib/auth/github.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import (
"github.com/gravitational/teleport/api/utils/keys"
"github.com/gravitational/teleport/lib/auth/authclient"
"github.com/gravitational/teleport/lib/authz"
"github.com/gravitational/teleport/lib/client/sso"
"github.com/gravitational/teleport/lib/defaults"
"github.com/gravitational/teleport/lib/events"
"github.com/gravitational/teleport/lib/loginrule"
Expand Down Expand Up @@ -982,6 +983,10 @@ func ValidateClientRedirect(clientRedirect string, ssoTestFlow bool, settings *t
if err != nil {
return trace.Wrap(err, "parsing client redirect URL")
}
if u.Path == sso.WebMFARedirect {
// If this is a SSO redirect in the WebUI, allow.
return nil
}
if u.Opaque != "" {
return trace.BadParameter("unexpected opaque client redirect URL")
}
Expand Down
1 change: 1 addition & 0 deletions lib/auth/grpcserver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -537,6 +537,7 @@ func TestMFADeviceManagement_SSO(t *testing.T) {
Sso: &types.SSOMFADevice{
ConnectorId: samlConnector.GetName(),
ConnectorType: samlConnector.GetKind(),
DisplayName: samlConnector.GetDisplay(),
},
})
require.NoError(t, err)
Expand Down
5 changes: 4 additions & 1 deletion lib/auth/sso_mfa.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,10 @@ import (

// beginSSOMFAChallenge creates a new SSO MFA auth request and session data for the given user and sso device.
func (a *Server) beginSSOMFAChallenge(ctx context.Context, user string, sso *types.SSOMFADevice, ssoClientRedirectURL string, ext *mfav1.ChallengeExtensions) (*proto.SSOChallenge, error) {
chal := new(proto.SSOChallenge)
chal := &proto.SSOChallenge{
Device: sso,
}

switch sso.ConnectorType {
case constants.SAML:
resp, err := a.CreateSAMLAuthRequestForMFA(ctx, types.SAMLAuthRequest{
Expand Down
1 change: 1 addition & 0 deletions lib/auth/sso_mfa_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,7 @@ func TestSSOMFAChallenge_Validation(t *testing.T) {
Sso: &types.SSOMFADevice{
ConnectorId: samlConnector.GetName(),
ConnectorType: samlConnector.GetKind(),
DisplayName: samlConnector.GetDisplay(),
},
})
require.NoError(t, err)
Expand Down
6 changes: 3 additions & 3 deletions lib/client/sso/ceremony.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,15 +65,15 @@ func NewCLICeremony(rd *Redirector, init CeremonyInit) *Ceremony {

// Ceremony is a customizable SSO MFA ceremony.
type MFACeremony struct {
clientCallbackURL string
close func()
ClientCallbackURL string
HandleRedirect func(ctx context.Context, redirectURL string) error
GetCallbackMFAToken func(ctx context.Context) (string, error)
}

// GetClientCallbackURL returns the client callback URL.
func (m *MFACeremony) GetClientCallbackURL() string {
return m.clientCallbackURL
return m.ClientCallbackURL
}

// Run the SSO MFA ceremony.
Expand Down Expand Up @@ -108,8 +108,8 @@ func (m *MFACeremony) Close() {
// The returned MFACeremony takes ownership of the Redirector.
func NewCLIMFACeremony(rd *Redirector) *MFACeremony {
return &MFACeremony{
clientCallbackURL: rd.ClientCallbackURL,
close: rd.Close,
ClientCallbackURL: rd.ClientCallbackURL,
HandleRedirect: rd.OpenRedirect,
GetCallbackMFAToken: func(ctx context.Context) (string, error) {
loginResp, err := rd.WaitForResponse(ctx)
Expand Down
5 changes: 5 additions & 0 deletions lib/client/sso/redirector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,11 @@ const (

// DefaultLoginURL is the default login page.
DefaultLoginURL = "/web/login"

// WebMFARedirect is the landing page for SSO MFA in the WebUI. The WebUI set up a listener
// on this page in order to capture the SSO MFA response regardless of what page the challenge
// was requested from.
WebMFARedirect = "/web/sso_confirm"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What kind of backwards compatibility do we have if an old proxy doesn't know about this route? Will a 404 kill the entire login flow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah if the user starts SSO MFA on an up-to-date proxy, then goes through the flow and gets routed to an out-of-date proxy, it will fail. This doesn't break the existing login flow or anything, so this is in line with the compatibly capabilities of new features.

)

// RedirectorConfig is configuration for an sso redirector.
Expand Down
39 changes: 39 additions & 0 deletions lib/client/weblogin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,14 @@ type MFAChallengeResponse struct {
TOTPCode string `json:"totp_code,omitempty"`
// WebauthnResponse is a response from a webauthn device.
WebauthnResponse *wantypes.CredentialAssertionResponse `json:"webauthn_response,omitempty"`
// SSOResponse is a response from an SSO MFA flow.
SSOResponse *SSOResponse `json:"sso_response"`
}

// SSOResponse is a json compatible [proto.SSOResponse].
type SSOResponse struct {
RequestID string `json:"requestId,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the surrounding existing structs are using snake case

Suggested change
RequestID string `json:"requestId,omitempty"`
RequestID string `json:"request_id,omitempty"`

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intentional, the frontend uses camel case, but we've been inconsistent with it on the backend.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See a bit lower:

type AuthenticateWebUserRequest struct {
	// User is a teleport username.
	User string `json:"user"`
	// WebauthnAssertionResponse is a signed WebAuthn credential assertion.
	WebauthnAssertionResponse *wantypes.CredentialAssertionResponse `json:"webauthnAssertionResponse,omitempty"`
}

Token string `json:"token,omitempty"`
}

// GetOptionalMFAResponseProtoReq converts response to a type proto.MFAAuthenticateResponse,
Expand Down Expand Up @@ -457,6 +465,37 @@ type MFAAuthenticateChallenge struct {
WebauthnChallenge *wantypes.CredentialAssertion `json:"webauthn_challenge"`
// TOTPChallenge specifies whether TOTP is supported for this user.
TOTPChallenge bool `json:"totp_challenge"`
// SSOChallenge is an SSO MFA challenge.
SSOChallenge *SSOChallenge `json:"sso_challenge"`
}

// SSOChallenge is a json compatible [proto.SSOChallenge].
type SSOChallenge struct {
RequestID string `json:"requestId,omitempty"`
RedirectURL string `json:"redirectUrl,omitempty"`
Device *SSOMFADevice `json:"device"`
// ChannelID is used by the front end to differentiate multiple ongoing SSO
// MFA requests so they don't interfere with each other.
ChannelID string `json:"channelId"`
}

// SSOMFADevice is a json compatible [proto.SSOMFADevice].
type SSOMFADevice struct {
ConnectorID string `json:"connectorId,omitempty"`
ConnectorType string `json:"connectorType,omitempty"`
DisplayName string `json:"displayName,omitempty"`
}
Joerger marked this conversation as resolved.
Show resolved Hide resolved

func SSOChallengeFromProto(ssoChal *proto.SSOChallenge) *SSOChallenge {
return &SSOChallenge{
RequestID: ssoChal.RequestId,
RedirectURL: ssoChal.RedirectUrl,
Device: &SSOMFADevice{
ConnectorID: ssoChal.Device.ConnectorId,
ConnectorType: ssoChal.Device.ConnectorType,
DisplayName: ssoChal.Device.DisplayName,
},
}
}

// MFARegisterChallenge is an MFA register challenge sent on new MFA register.
Expand Down
4 changes: 2 additions & 2 deletions lib/defaults/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -707,8 +707,8 @@ const (
// made for an existing file transfer request
WebsocketFileTransferDecision = "t"

// WebsocketWebauthnChallenge is sending a webauthn challenge.
WebsocketWebauthnChallenge = "n"
// WebsocketMFAChallenge is sending an MFA challenge. Only supports WebAuthn and SSO MFA.
WebsocketMFAChallenge = "n"

// WebsocketSessionMetadata is sending the data for a ssh session.
WebsocketSessionMetadata = "s"
Expand Down
1 change: 1 addition & 0 deletions lib/services/local/users.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1489,6 +1489,7 @@ func (s *IdentityService) getSSOMFADevice(ctx context.Context, user string) (*ty
Sso: &types.SSOMFADevice{
ConnectorId: cb.Connector.ID,
ConnectorType: cb.Connector.Type,
DisplayName: mfaConnector.GetDisplay(),
},
})
}
Expand Down
27 changes: 16 additions & 11 deletions lib/services/local/users_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -595,33 +595,40 @@ func TestIdentityService_GetMFADevices_SSO(t *testing.T) {
tests := []struct {
name string
connectorRef *types.ConnectorRef
expectSSODevice bool
expectSSODevice *types.SSOMFADevice
}{
{
name: "non-sso user",
connectorRef: nil,
expectSSODevice: false,
expectSSODevice: nil,
}, {
name: "sso user mfa disabled",
connectorRef: &types.ConnectorRef{
Type: "saml",
ID: samlConnectorNoMFA.GetName(),
},
expectSSODevice: false,
}, {
name: "saml user",
connectorRef: &types.ConnectorRef{
Type: "saml",
ID: samlConnector.GetName(),
},
expectSSODevice: true,
expectSSODevice: &types.SSOMFADevice{
ConnectorType: "saml",
ConnectorId: samlConnector.GetName(),
DisplayName: samlConnector.GetDisplay(),
},
}, {
name: "oidc user",
connectorRef: &types.ConnectorRef{
Type: "oidc",
ID: oidcConnector.GetName(),
},
expectSSODevice: true,
expectSSODevice: &types.SSOMFADevice{
ConnectorType: "oidc",
ConnectorId: oidcConnector.GetName(),
DisplayName: oidcConnector.GetDisplay(),
},
},
}
for _, test := range tests {
Expand All @@ -638,15 +645,13 @@ func TestIdentityService_GetMFADevices_SSO(t *testing.T) {
devs, err := identity.GetMFADevices(ctx, "alice", true /* withSecrets */)
require.NoError(t, err)

if !test.expectSSODevice {
if test.expectSSODevice == nil {
assert.Empty(t, devs)
return
}
expectSSODevice, err := types.NewMFADevice(test.connectorRef.ID, test.connectorRef.ID, clock.Now().UTC(), &types.MFADevice_Sso{
Sso: &types.SSOMFADevice{
ConnectorId: test.connectorRef.ID,
ConnectorType: test.connectorRef.Type,
},

expectSSODevice, err := types.NewMFADevice(test.expectSSODevice.ConnectorId, test.expectSSODevice.DisplayName, clock.Now().UTC(), &types.MFADevice_Sso{
Sso: test.expectSSODevice,
})
require.NoError(t, err)
assert.Equal(t, []*types.MFADevice{expectSSODevice}, devs)
Expand Down
8 changes: 4 additions & 4 deletions lib/srv/desktop/tdp/proto.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -737,10 +737,10 @@ func DecodeMFA(in byteReader) (*MFA, error) {
}
s := string(mt)
switch s {
case defaults.WebsocketWebauthnChallenge:
case defaults.WebsocketMFAChallenge:
default:
return nil, trace.BadParameter(
"got mfa type %v, expected %v (WebAuthn)", mt, defaults.WebsocketWebauthnChallenge)
"got mfa type %v, expected %v (MFAChallenge)", mt, defaults.WebsocketMFAChallenge)
}

var length uint32
Expand Down Expand Up @@ -780,10 +780,10 @@ func DecodeMFAChallenge(in byteReader) (*MFA, error) {
}
s := string(mt)
switch s {
case defaults.WebsocketWebauthnChallenge:
case defaults.WebsocketMFAChallenge:
default:
return nil, trace.BadParameter(
"got mfa type %v, expected %v (WebAuthn)", mt, defaults.WebsocketWebauthnChallenge)
"got mfa type %v, expected %v (MFAChallenge)", mt, defaults.WebsocketMFAChallenge)
}

var length uint32
Expand Down
6 changes: 3 additions & 3 deletions lib/srv/desktop/tdp/proto_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ func TestEncodeDecode(t *testing.T) {
}

func FuzzDecode(f *testing.F) {
var corpus = []string{
corpus := []string{
"0",
"\x02",
"\x1b\xff\xff\x800",
Expand Down Expand Up @@ -125,7 +125,7 @@ func TestMFA(t *testing.T) {
c := NewConn(&fakeConn{Buffer: &buff})

mfaWant := &MFA{
Type: defaults.WebsocketWebauthnChallenge[0],
Type: defaults.WebsocketMFAChallenge[0],
MFAAuthenticateChallenge: &client.MFAAuthenticateChallenge{
WebauthnChallenge: &wantypes.CredentialAssertion{
Response: wantypes.PublicKeyCredentialRequestOptions{
Expand Down Expand Up @@ -159,7 +159,7 @@ func TestMFA(t *testing.T) {
require.Equal(t, mfaWant, mfaGot)

respWant := &MFA{
Type: defaults.WebsocketWebauthnChallenge[0],
Type: defaults.WebsocketMFAChallenge[0],
MFAAuthenticateResponse: &authproto.MFAAuthenticateResponse{
Response: &authproto.MFAAuthenticateResponse_Webauthn{
Webauthn: &wanpb.CredentialAssertionResponse{
Expand Down
11 changes: 11 additions & 0 deletions lib/web/apiserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ import (
"github.com/gravitational/teleport/lib/authz"
"github.com/gravitational/teleport/lib/automaticupgrades"
"github.com/gravitational/teleport/lib/client"
"github.com/gravitational/teleport/lib/client/sso"
"github.com/gravitational/teleport/lib/defaults"
dtconfig "github.com/gravitational/teleport/lib/devicetrust/config"
"github.com/gravitational/teleport/lib/events"
Expand Down Expand Up @@ -2260,6 +2261,16 @@ func ConstructSSHResponse(response AuthParams) (*url.URL, error) {

// Extract secret out of the request.
secretKey := u.Query().Get("secret_key")

// We don't use a secret key for WebUI SSO MFA redirects. The request ID itself is
// kept a secret on the front end to minimize the risk of a phishing attack.
if secretKey == "" && u.Path == sso.WebMFARedirect && response.MFAToken != "" {
q := u.Query()
q.Add("response", string(out))
u.RawQuery = q.Encode()
return u, nil
}

if secretKey == "" {
return nil, trace.BadParameter("missing secret_key")
}
Expand Down
14 changes: 5 additions & 9 deletions lib/web/apiserver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2182,7 +2182,7 @@ func TestTerminalRequireSessionMFA(t *testing.T) {

envelope := &terminal.Envelope{
Version: defaults.WebsocketVersion,
Type: defaults.WebsocketWebauthnChallenge,
Type: defaults.WebsocketMFAChallenge,
Payload: string(webauthnResBytes),
}
protoBytes, err := proto.Marshal(envelope)
Expand Down Expand Up @@ -2389,7 +2389,7 @@ func handleDesktopMFAWebauthnChallenge(t *testing.T, ws *websocket.Conn, dev *au
})
require.NoError(t, err)
err = tdpConn.WriteMessage(tdp.MFA{
Type: defaults.WebsocketWebauthnChallenge[0],
Type: defaults.WebsocketMFAChallenge[0],
MFAAuthenticateResponse: &authproto.MFAAuthenticateResponse{
Response: &authproto.MFAAuthenticateResponse_Webauthn{
Webauthn: res.GetWebauthn(),
Expand Down Expand Up @@ -3093,7 +3093,6 @@ func TestPingSSHDialTimeout(t *testing.T) {

// Validate the timeout is the default value.
require.Equal(t, cnc.GetSSHDialTimeout(), out.Proxy.SSH.DialTimeout)

}

// TestConstructSSHResponse checks if the secret package uses AES-GCM to
Expand Down Expand Up @@ -4387,7 +4386,6 @@ func TestClusterKubeResourcesGet(t *testing.T) {
require.NoError(t, json.Unmarshal(re.Bytes(), &resp))
require.ElementsMatch(t, tc.expectedResponse, resp.Items)
}

})
}
}
Expand Down Expand Up @@ -4768,7 +4766,6 @@ func TestGetWebConfig_WithEntitlements(t *testing.T) {
assert.NoError(t, err)
diff := cmp.Diff(expectedCfg, cfg)
assert.Empty(t, diff)

}, time.Second*5, time.Millisecond*50)

// use mock client to assert that if ping returns an error, we'll default to
Expand Down Expand Up @@ -4806,7 +4803,6 @@ func TestGetWebConfig_WithEntitlements(t *testing.T) {
assert.NoError(t, err)
diff := cmp.Diff(expectedCfg, cfg)
assert.Empty(t, diff)

}, time.Second*5, time.Millisecond*50)
}

Expand Down Expand Up @@ -10063,7 +10059,7 @@ func TestModeratedSessionWithMFA(t *testing.T) {

envelope := &terminal.Envelope{
Version: defaults.WebsocketVersion,
Type: defaults.WebsocketWebauthnChallenge,
Type: defaults.WebsocketMFAChallenge,
Payload: string(webauthnResBytes),
}
envelopeBytes, err := proto.Marshal(envelope)
Expand Down Expand Up @@ -10094,7 +10090,7 @@ func TestModeratedSessionWithMFA(t *testing.T) {

envelope := &terminal.Envelope{
Version: defaults.WebsocketVersion,
Type: defaults.WebsocketWebauthnChallenge,
Type: defaults.WebsocketMFAChallenge,
Payload: string(webauthnResBytes),
}
envelopeBytes, err := proto.Marshal(envelope)
Expand Down Expand Up @@ -10132,7 +10128,7 @@ func TestModeratedSessionWithMFA(t *testing.T) {

envelope := &terminal.Envelope{
Version: defaults.WebsocketVersion,
Type: defaults.WebsocketWebauthnChallenge,
Type: defaults.WebsocketMFAChallenge,
Payload: string(webauthnResBytes),
}
envelopeBytes, err := proto.Marshal(envelope)
Expand Down
Loading
Loading