-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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 - Add SSOMFASessionData
#47647
Merged
Merged
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
814a044
Add MFA Session Data struct and methods.
Joerger 8b8c875
Cleanup; Add test.
Joerger 00f9aee
Rename to upsert.
Joerger 34cd451
Merge branch 'master' into joerger/sso-mfa-session-data
Joerger cdc8e25
Merge branch 'master' into joerger/sso-mfa-session-data
Joerger 1806ca7
Fix license.
Joerger File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
package auth | ||
|
||
import ( | ||
"context" | ||
|
||
"github.com/gravitational/trace" | ||
|
||
mfav1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/mfa/v1" | ||
"github.com/gravitational/teleport/lib/defaults" | ||
"github.com/gravitational/teleport/lib/services" | ||
"github.com/gravitational/teleport/lib/utils" | ||
) | ||
|
||
// UpsertSSOMFASession upserts a new unverified SSO MFA session for the given username, | ||
// sessionID, connector details, and challenge extensions. | ||
func (a *Server) UpsertSSOMFASession(ctx context.Context, user string, sessionID string, connectorID string, connectorType string, ext *mfav1.ChallengeExtensions) error { | ||
err := a.UpsertSSOMFASessionData(ctx, &services.SSOMFASessionData{ | ||
Username: user, | ||
RequestID: sessionID, | ||
ConnectorID: connectorID, | ||
ConnectorType: connectorType, | ||
ChallengeExtensions: ext, | ||
}) | ||
return trace.Wrap(err) | ||
} | ||
|
||
// UpsertSSOMFASessionWithToken upserts the given SSO MFA session with a random mfa token. | ||
func (a *Server) UpsertSSOMFASessionWithToken(ctx context.Context, sd *services.SSOMFASessionData) (token string, err error) { | ||
sd.Token, err = utils.CryptoRandomHex(defaults.TokenLenBytes) | ||
if err != nil { | ||
return "", trace.Wrap(err) | ||
} | ||
|
||
if err := a.UpsertSSOMFASessionData(ctx, sd); err != nil { | ||
return "", trace.Wrap(err) | ||
} | ||
|
||
return sd.Token, nil | ||
} | ||
|
||
// GetSSOMFASession returns the SSO MFA session for the given username and sessionID. | ||
func (a *Server) GetSSOMFASession(ctx context.Context, sessionID string) (*services.SSOMFASessionData, error) { | ||
sd, err := a.GetSSOMFASessionData(ctx, sessionID) | ||
if err != nil { | ||
return nil, trace.Wrap(err) | ||
} | ||
|
||
return sd, nil | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
/* | ||
* Teleport | ||
* Copyright (C) 2024 Gravitational, Inc. | ||
* | ||
* This program is free software: you can redistribute it and/or modify | ||
* it under the terms of the GNU Affero General Public License as published by | ||
* the Free Software Foundation, either version 3 of the License, or | ||
* (at your option) any later version. | ||
* | ||
* This program is distributed in the hope that it will be useful, | ||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
* GNU Affero General Public License for more details. | ||
* | ||
* You should have received a copy of the GNU Affero General Public License | ||
* along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
*/ | ||
|
||
package services | ||
|
||
import mfav1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/mfa/v1" | ||
|
||
// SSOMFASessionData SSO MFA Session data. | ||
type SSOMFASessionData struct { | ||
// RequestID is the ID of the corresponding SSO Auth request, which is used to | ||
// identity this session. | ||
RequestID string `json:"request_id,omitempty"` | ||
// Username is the Teleport username. | ||
Username string `json:"username,omitempty"` | ||
// Token is an active token used to verify the owner of this SSO MFA session data. | ||
Token string `json:"token,omitempty"` | ||
// ConnectorID is id of the corresponding Auth connector. | ||
ConnectorID string `json:"connector_id,omitempty"` | ||
// ConnectorType is SSO type of the corresponding Auth connector (SAML, OIDC). | ||
ConnectorType string `json:"connector_type,omitempty"` | ||
// ChallengeExtensions are Teleport extensions that apply to this SSO MFA session. | ||
ChallengeExtensions *mfav1.ChallengeExtensions `json:"challenge_extensions,omitempty"` | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is a brand new type I'd strongly recommend protojson, FWIW.
edit: nevermind, it's specifically defined as JSON
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The struct contains a protobuf-defined type; is that currently (wrongly) marshaled and unmarshaled with
encoding/json
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose so, what issues does this cause? We are already json marshalling
*mfav1.ChallengeExtensions
in the webauthn session data as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think a custom marshaller on
services.SSOMFASessionData
toprotojson.Marshal
the challenge extensions is warranted?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
encoding/json
encoding of a protobuf message is not canonical in any way, which means that Go ends up being the only thing that knows what the shape of the json data is supposed to be. Perhaps you could defineSSOMFASessionData
as a protobuf message even if it's not directly used through grpc? That way you also get breaking change detection as part of the linting, and in a future where we generate protobuf types for javascript you'll get the same type definition on that side as well, without having to manually sync it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This session data never leaves the Auth server, so whether or not it's supported across a transport layer doesn't really matter.
Furthermore, I don't see exactly why it would be difficult to maintain the same JSON structure across different languages, but if this is the case and we changed our Auth/Proxy servers to non-go, we'd have a lot of issues with proto fields in our web api json requests and responses. e.g:
I don't think we have any plan to have a different language implementation of Teleport, so I don't think this is a problem we need to concern ourselves with anyways, right?
Also, this is just a short lived resource, so we could easily change it in the future without any backwards compatibility concerns. I'll merge this as is as we can always make a follow up PR to change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a resource that gets marshalled and unmarshalled, and it gets stored in a backend. What even is the forward compatible breaking change path for something like that? How do you change it without it resulting in massive breakages?
The fact that we are relying on the implicit data shape of protobuf messages to use
encoding/json
is already very much a big problem, one that for example forces us to keep using the gogoproto generator and library even though we should've stopped using it a long time ago, and each new thing that we marshal like this makes it harder and harder to keep track of what the actual schema of our data is.We literally face this problem when we aspire to use protobuf in our frontend and are faced with the reality that the user-facing data shape for various things (which matches the data shape stored in our backend) is something vaguely deterministic that's only defined by how a specific code generator for Go decided to arrange things rather than anything canonical - while also dealing with all the drawbacks of a generated data type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The resource only exists for 5 minutes, or shorter if the user consumes it. I'm not saying it's a perfectly forward compatible change, but worst case there would be a small number of failed MFA attempts when the Auth server upgrades, and it would sort itself out momentarily. Any user with the failed attempt would be able to succeed on their next attempt. A changelog note would be enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that's the only possible failure scenario then sure, it's not a problem - what happens if the data is misinterpreted because a new protobuf codegen has changed something in how the struct is laid out and we didn't notice, tho?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then the value should be treated as the zero value, so it would be treated like a challenge with an unspecified scope with reuse not allowed, which is the least privileged type of challenge and would get rejected in basically any context. Anyways, I'll make a follow up PR.