feat: SSO MFA - Add SSOMFASessionData
#47647
Conversation
Joerger
added
the
no-changelog
| return trace.BadParameter("missing parameter Username") | ||
| } | ||
|
|
||
| value, err := json.Marshal(sd) |
There was a problem hiding this comment.
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.
The struct contains a protobuf-defined type; is that currently (wrongly) marshaled and unmarshaled with encoding/json?
There was a problem hiding this comment.
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.
Do you think a custom marshaller on services.SSOMFASessionData to protojson.Marshal the challenge extensions is warranted?
There was a problem hiding this comment.
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 define SSOMFASessionData 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.
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:
type getLoginAlertsResponse struct {
Alerts []types.ClusterAlert `json:"alerts"`
}
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.
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.
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?
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.
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 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.
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.
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.
Part of the implementation of SSO MFA
Adds the storage of
SSOMFASessionData. This will be used in follow up PRs to complete the SSO MFA flow.