GODRIVER-2911: Add machine flow OIDC authentication #1678
Conversation
…not go live, I'm sure
…r OIDC that is probably, maybe, possibly correct
… need to get down to Handshake instead of creating the Authenticator in Handshake as we do now
…o I'm sure it's right
mongodb-drivers-pr-bot
bot
added
dependencies
API Change Report./cmd/testoidcauthcompatible changespackage added ./mongo/optionscompatible changesCredential.OIDCHumanCallback: added ./x/mongo/driverincompatible changesConnection.OIDCTokenGenID: added compatible changesAuthConfig: added ./x/mongo/driver/authincompatible changes##(*DefaultAuthenticator).Auth: changed from func(context.Context, *Config) error to func(context.Context, *./x/mongo/driver.AuthConfig) error compatible changes(*DefaultAuthenticator).Reauth: added ./x/mongo/driver/drivertestcompatible changes(*ChannelConn).OIDCTokenGenID: added ./x/mongo/driver/operationcompatible changes(*AbortTransaction).Authenticator: added ./x/mongo/driver/sessionincompatible changesLoadBalancedTransactionConnection.OIDCTokenGenID: added ./x/mongo/driver/topologycompatible changes(*Connection).OIDCTokenGenID: added |
| import ( | ||
| "go.mongodb.org/mongo-driver/x/mongo/driver" | ||
| ) | ||
|
|
There was a problem hiding this comment.
Redeclarations like this are just to avoid breaking user code.
| ClientID string `bson:"clientId"` | ||
| RequestScopes []string `bson:"requestScopes"` | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this is my biggest question on design right here. I could add another interface called Reauthenticator and downcast in operation.go on a 391 error to Reauthenticator so that every auth mechanism doesn't need to have a definition for Reauth, but I like this approach since it will require any new authentication mechanism to consider whether or not it supports Reauth than simply forgetting to do that, since this moves the check to compile time instead of runtime. This is the rust programmer in me, so I'll defer to whatever others prefer
There was a problem hiding this comment.
I agree, and the server may also decide to add Reauth capability to existing mechanisms.
There was a problem hiding this comment.
I most often see "upgrading" Go interfaces used as a backward compatibility mechanism. IMO some extra no-op functions is better than runtime type assertions. I agree with the current approach.
| @@ -0,0 +1,676 @@ | |||
| // Copyright (C) MongoDB, Inc. 2022-present. | |||
There was a problem hiding this comment.
I made this an executable rather than using go test since this is what gssapi is doing... but also I know that when I go to test GCP and Azure that I'm going to want to build ahead of time, and I'm not sure if go test can build an executable (cargo test can).
There was a problem hiding this comment.
Also, here's hoping statically linking glibc works with go, since that was necessary to get the rust tests working :)
There was a problem hiding this comment.
go test can indeed build a binary. However, following the current convention seems fine. If/when we want to make these Go tests, we can convert them all at the same time.
matthewdale
added
priority-2-medium
cmd/testoidcauth/main.go
Outdated
| } | ||
|
|
||
| // Poison the cache with a random token | ||
| client.GetAuthenticator().(*auth.OIDCAuthenticator).SetAccessToken("some random happy sunshine string") |
There was a problem hiding this comment.
We should try to avoid adding public methods if possible, especially if it's only used for test support. Since this is only used for testing, I recommend using some reflection/unsafe code to extract the authenticator rather than exposing it via GetAuthenticator.
E.g.
{
clientElem := reflect.ValueOf(client).Elem()
authenticatorField := clientElem.FieldByName("authenticator")
authenticatorField = reflect.NewAt(
authenticatorField.Type(),
unsafe.Pointer(authenticatorField.UnsafeAddr())).Elem()
authenticatorField.Interface().(*auth.OIDCAuthenticator).SetAccessToken("some random happy sunshine string")
}There was a problem hiding this comment.
I didn't know you could do this, thanks, much preferable
x/mongo/driver/operation/insert.go
Outdated
| @@ -47,6 +48,7 @@ type Insert struct { | |||
|
|
|||
| // InsertResult represents an insert result returned by the server. | |||
| type InsertResult struct { | |||
| authenticator driver.Authenticator | |||
There was a problem hiding this comment.
Is there a reason to add authenticator to InsertResult?
There was a problem hiding this comment.
I added these with a python script and missed this one :)
There was a problem hiding this comment.
Looks like I added a few other extra ones, should all be gone now
x/mongo/driver/driver.go
Outdated
| // OIDCArgs contains the arguments for the OIDC callback. | ||
| type OIDCArgs struct { | ||
| Version int | ||
| Timeout time.Time |
There was a problem hiding this comment.
It seems like Timeout is used to communicate a deadline to the OIDC callback, which is redundant with the context.Context parameter that's part of the OIDCCallback function signature. Is there any other reason to keep Timeout?
There was a problem hiding this comment.
Nope, I think you're right
| ClientID string `bson:"clientId"` | ||
| RequestScopes []string `bson:"requestScopes"` | ||
| } | ||
|
|
There was a problem hiding this comment.
I most often see "upgrading" Go interfaces used as a backward compatibility mechanism. IMO some extra no-op functions is better than runtime type assertions. I agree with the current approach.
x/mongo/driver/auth/oidc.go
Outdated
| oa.mu.Lock() | ||
| defer oa.mu.Unlock() | ||
| var err error | ||
|
|
||
| if cfg == nil { | ||
| return newAuthError(fmt.Sprintf("config must be set for %q authentication", MongoDBOIDC), nil) | ||
| } | ||
| oa.cfg = cfg | ||
|
|
||
| if oa.accessToken != "" { | ||
| err = ConductSaslConversation(ctx, cfg, "$external", &oidcOneStep{ |
There was a problem hiding this comment.
Holding an exclusive lock while we run the SASL conversation will limit the auth part of connection establishment to one-at-a-time, overriding the intended behavior of maxConnecting. Instead, we should only hold a lock when invalidating and acquiring a new access token.
Unfortunately that means the strategy of storing the auth.Config on the OIDCAuthenticator also won't work.
E.g. changes to prevent locking during the SASL conversation and allow multiple Config instances:
func (oa *OIDCAuthenticator) invalidateAccessToken(tokenGenID uint64) {
oa.mu.Lock()
defer oa.mu.Unlock()
if oa.tokenGenID <= tokenGenID {
oa.accessToken = ""
}
// ...
}
func (oa *OIDCAuthenticator) getAccessToken(...) (string, uint64, error) {
oa.mu.Lock()
defer oa.mu.Unlock()
if oa.accessToken != "" {
return oa.accessToken, oa.tokenGenID, nil
}
// ...
}
func (oa *OIDCAuthenticator) Auth(ctx context.Context, cfg *Config) error {
var err error
if cfg == nil {
return newAuthError(fmt.Sprintf("config must be set for %q authentication", MongoDBOIDC), nil)
}
accessToken, tokenGenID, err := oa.getAccessToken(...)
if oa.accessToken != "" {
err = ConductSaslConversation(ctx, cfg, "$external", &oidcOneStep{
userName: oa.userName,
accessToken: oa.accessToken,
})
if err == nil {
return nil
}
oa.invalidateAccessToken(tokenGenID)
time.Sleep(invalidateSleepTimeout)
}
oa.mu.Lock()
defer oa.mu.Unlock()
// ...
}There was a problem hiding this comment.
I think this works, let me run the tests. My concern is machine_1_2, but I do think this works.
x/mongo/driver/auth/oidc.go
Outdated
|
|
||
| // Reauth reauthenticates the connection when the server returns a 391 code. Reauth is part of the | ||
| // driver.Authenticator interface. | ||
| func (oa *OIDCAuthenticator) Reauth(ctx context.Context) error { |
There was a problem hiding this comment.
Reauth needs to take accept an AuthConfig that has all the inputs for authenticating a specific connection, not the connection most recently configured in oa.cfg.
E.g.
func (oa *OIDCAuthenticator) Reauth(
ctx context.Context,
cfg *AuthConfig,
) error {It's not clear exactly how to get all the info for an AuthConfig in Operation.Execute.
There was a problem hiding this comment.
I don't see how we can make that work. The python and rust driver both store the necessary information in the Authenticator :/
There was a problem hiding this comment.
Seems we found a good way to make this work
x/mongo/driver/auth/oidc.go
Outdated
| if cfg == nil { | ||
| return newAuthError(fmt.Sprintf("config must be set for %q authentication", MongoDBOIDC), nil) | ||
| } | ||
| oa.httpClient = cfg.HTTPClient |
There was a problem hiding this comment.
There's a data race detected here when running the OIDC tests. If possible, we should pass in the HTTP client when we create the authenticator at initialization rather than when we call Auth to avoid having to write to this field at run time.
See data-race-1.txt for details.
| return "", err | ||
| } | ||
|
|
||
| oa.accessToken = cred.AccessToken |
There was a problem hiding this comment.
There's a data race detected here when running the OIDC tests. We either need to use an atomic.Value to synchronize access to the access token value, or delegate all reading and writing of the access token to a function that synchronizes access to it.
See data-race-2.txt for details.
Co-authored-by: Matt Dale <[email protected]>
x/mongo/driver/auth/oidc.go
Outdated
| return runSaslConversation(ctx, | ||
| cfg, | ||
| newSaslConversation(&oidcOneStep{accessToken: accessToken}, "$external", false), | ||
| ) |
There was a problem hiding this comment.
Can this use ConductSaslConversation instead? That would allow merging runSaslConversation and ConductSaslConversation together.
E.g.
return ConductSaslConversation(
ctx,
cfg,
"$external",
&oidcOneStep{accessToken: accessToken})There was a problem hiding this comment.
Yea, this was a holdover from something I was testing, and I didn't think anyone would mind. (runSaslConversation didn't exist before)
x/mongo/driver/auth/oidc.go
Outdated
| func (oa *OIDCAuthenticator) doAuthHuman(_ context.Context, _ *Config, _ OIDCCallback) error { | ||
| // TODO GODRIVER-3246: Implement OIDC human flow | ||
| // Println is for linter | ||
| fmt.Println("OIDC human flow not implemented yet", oa.idpInfo) |
There was a problem hiding this comment.
Remove print statement.
| fmt.Println("OIDC human flow not implemented yet", oa.idpInfo) |
There was a problem hiding this comment.
The linter fails if I remove the print ;) Note this will be removed in the next ticket
Oh, I could just return an error that uses idpInfo... yes
| subCtx, cancel := context.WithTimeout(ctx, machineCallbackTimeout) | ||
| accessToken, err := oa.getAccessToken(subCtx, |
There was a problem hiding this comment.
The line in the OIDC spec that this timeout comes from is ambiguous with respect to the Go Driver:
If CSOT is not applied, then the driver MUST use 1 minute as the timeout.
All Go Driver blocking APIs support timeouts, so from that perspective, CSOT is always applied. However, the Go Driver also includes the concept of CSOT being "enabled" or "not enabled", which was required to retrofit the CSOT behavior into the existing driver that already supported timeouts (for example, see usage of csot.IsTimeoutContext). From that perspective, we should only apply the 1-minute timeout when CSOT is "not enabled".
As implemented, the code always applies the 1-minute timeout, overriding any longer timeouts. That seems unlikely to break anything because waiting 1 minute for an access token will probably cause other problems (e.g. the default connectTimeout is 30 seconds).
Overall, I think this code is safe, but we should add a comment describing the interpretation of the OIDC spec we're using here.
E.g. comment:
The CSOT spec says to apply a 1-minute timeout if "CSOT is not applied". That's ambiguous for the v1.x Go Driver because it could mean either "no timeout provided" or "CSOT not enabled". Always use a maximum timeout duration of 1 minute, allowing us to ignore the ambiguity. Contexts with a shorter timeout are unaffected.
There was a problem hiding this comment.
This is exactly the sort of context I am missing, thank you!
…in favor of using oa.idpInfo in error message
mongo/options/clientoptions.go
Outdated
| OIDCMachineCallback driver.OIDCCallback | ||
| OIDCHumanCallback driver.OIDCCallback |
There was a problem hiding this comment.
We need to canonically define or re-define the OIDCCallback type and associated structs here so the public API doesn't include symbols from the experimental API.
E.g. types that need to be defined in the options package:
// OIDCCallback is the type for both Human and Machine Callback flows. RefreshToken will always be
// nil in the OIDCArgs for the Machine flow.
type OIDCCallback func(context.Context, *OIDCArgs) (*OIDCCredential, error)
// OIDCArgs contains the arguments for the OIDC callback.
type OIDCArgs struct {
Version int
IDPInfo *IDPInfo
RefreshToken *string
}
// OIDCCredential contains the access token and refresh token.
type OIDCCredential struct {
AccessToken string
ExpiresAt *time.Time
RefreshToken *string
}It may be possible to canonically define the OIDCCallback, OIDCArgs, and OIDCCredential types in the options package, and use those types in the driver and auth` packages. If not, we will have to add logic to convert between the types.
There was a problem hiding this comment.
Tell me if this solution works for you, I think it's good.
…experimental options package
GODRIVER-2911
Summary
This is a full implementation of machine flow OIDC with token invalidation and reauthentication. It is missing configuration enforcement, which is split into GODRIVER-3249 to keep the PR smaller.
This currently does not have the tests, but I wanted to give an opportunity for architecture feedback. This should work since it's following the same algorithm as the rust code, but there could be something I'm missing here with, for instance, how reauthentication should work since this is different than how the rust driver does auth, and I want to make sure this looks correct.
Biggest changes
Commands andOperations to handle reauthConnectioninterface (:()Background & Motivation
epic: https://jira.mongodb.org/browse/GODRIVER-2574