Skip to content

Commit

Permalink
oidcccl: fix SSO login fails if IdP is down when CRDB starts
Browse files Browse the repository at this point in the history
We are encountering errors both when IdP is down and when it comes back up:
1. When IdP is down(network is unavailable):
 - OIDC attempts to re-initialize at the start of the request handler,
this can fail as it is unable to fetch openid-configuration required for initializing the provider for oidc manager
- OIDC re-initialization assumes that the operation will succeed and continues serving the request
even though initialization might have failed. Hence we need to add check to verify if
`oidcAuthentication.initialized` is set to true post successful call to `reloadConfigLocked`.

2. After IdP comes back up (network becomes available):
 - OIDC is initialized successfully and there is a call to oidc callback request handler.
 - OIDC callback request handler is failing when we are trying to `ExchangeVerifyGetClaims`. This is
 because `oidcManager.Verify()` is using the older context used when creating the verifier in oidc
 manager initialization.
 - The previous context got cancelled and there is an error thrown in `Verify()` code for cancelled
 context. The fix is to pass a non-cancellable context to the oidcManager instance.

Release note: None

Epic: CRDB-35370
  • Loading branch information
souravcrl committed Feb 22, 2024
1 parent 04f0416 commit d2ea21c
Show file tree
Hide file tree
Showing 3 changed files with 106 additions and 0 deletions.
1 change: 1 addition & 0 deletions pkg/ccl/oidcccl/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ go_test(
"//pkg/server",
"//pkg/server/authserver",
"//pkg/server/serverpb",
"//pkg/testutils",
"//pkg/testutils/serverutils",
"//pkg/testutils/sqlutils",
"//pkg/testutils/testcluster",
Expand Down
12 changes: 12 additions & 0 deletions pkg/ccl/oidcccl/authentication_oidc.go
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,14 @@ var NewOIDCManager func(context.Context, oidcAuthenticationConf, string, []strin
redirectURL string,
scopes []string,
) (IOIDCManager, error) {
// We need to provide a context which cannot be cancelled because of a specific implementation
// which prohibits context to be cancelled if we are to reuse the provider object
// https://github.com/coreos/go-oidc/issues/339
// TODO(souravcrl): Update go-oidc version - to control the context, in the current version of
// go-oidc, verifier instance can be created with VerifierContext
// https://github.com/coreos/go-oidc/blob/6d6be43e852de391805e5a5bc14146ba3cdd4195/oidc/verify.go#L125
ctx = context.WithoutCancel(ctx)

provider, err := oidc.NewProvider(ctx, conf.providerURL)
if err != nil {
return nil, err
Expand Down Expand Up @@ -703,6 +711,10 @@ var ConfigureOIDC = func(

if oidcAuthentication.enabled && !oidcAuthentication.initialized {
reloadConfigLocked(ctx, oidcAuthentication, locality, st)
if !oidcAuthentication.initialized {
http.Error(w, "OIDC: auth manager could not be initialized", http.StatusInternalServerError)
return
}
}

if !oidcAuthentication.enabled {
Expand Down
93 changes: 93 additions & 0 deletions pkg/ccl/oidcccl/authentication_oidc_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"encoding/base64"
"encoding/json"
"fmt"
"io"
"net/http"
"net/url"
"regexp"
Expand All @@ -24,8 +25,10 @@ import (
"github.com/cockroachdb/cockroach/pkg/base"
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/security/username"
"github.com/cockroachdb/cockroach/pkg/server"
"github.com/cockroachdb/cockroach/pkg/server/authserver"
"github.com/cockroachdb/cockroach/pkg/server/serverpb"
"github.com/cockroachdb/cockroach/pkg/testutils"
"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
"github.com/cockroachdb/cockroach/pkg/util/leaktest"
Expand Down Expand Up @@ -410,3 +413,93 @@ func Test_getRegionSpecificRedirectURL(t *testing.T) {
})
}
}

func TestOIDCManagerInitialisationUnderNetworkAvailability(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)

ctx := context.Background()
srv := serverutils.StartServerOnly(t, base.TestServerArgs{})
defer srv.Stopper().Stop(ctx)
s := srv.ApplicationLayer()

basePath := "/some/random/path"
testUserName := "testcrl"
networkAvailable := false

// Intercept the call to NewOIDCManager and return the mocked NewOIDCManager function
restoreHook := testutils.TestingHook(
&NewOIDCManager,
func(ctx context.Context, conf oidcAuthenticationConf, redirectURL string, scopes []string) (IOIDCManager, error) {
if !networkAvailable {
return nil, fmt.Errorf("network unavailable, check your network connection")
}
c := &oauth2.Config{
ClientID: conf.clientID,
ClientSecret: conf.clientSecret,
RedirectURL: redirectURL,
Endpoint: oauth2.Endpoint{
AuthURL: conf.providerURL,
},
Scopes: scopes,
}
return &mockOidcManager{oauth2Config: c, claimEmail: fmt.Sprintf("%[email protected]", testUserName)}, nil
})
defer func() {
restoreHook()
}()

// Set minimum settings to successfully enable the OIDC client
OIDCProviderURL.Override(ctx, &s.ClusterSettings().SV, "providerURL")
OIDCClientID.Override(ctx, &s.ClusterSettings().SV, "fake_client_id")
OIDCClientSecret.Override(ctx, &s.ClusterSettings().SV, "fake_client_secret")
OIDCRedirectURL.Override(ctx, &s.ClusterSettings().SV, "https://cockroachlabs.com/oidc/v1/callback")
OIDCClaimJSONKey.Override(ctx, &s.ClusterSettings().SV, "email")
OIDCPrincipalRegex.Override(ctx, &s.ClusterSettings().SV, "^([^@]+)@[^@]+$")
server.ServerHTTPBasePath.Override(ctx, &s.ClusterSettings().SV, basePath)
OIDCEnabled.Override(ctx, &s.ClusterSettings().SV, true)

for _, tc := range []struct {
testName string
wantError bool
network bool
}{
{
testName: "network unavailable test",
network: false,
wantError: true,
},
{
testName: "network available test",
network: true,
wantError: false,
},
} {
networkAvailable = tc.network
t.Run(tc.testName, func(t *testing.T) {
testOIDCManagerInitialisation := s.NewClientRPCContext(ctx, username.TestUserName())
client, err := testOIDCManagerInitialisation.GetHTTPClient()
require.NoError(t, err)

// Don't follow redirects as we are only testing setting of oidc manager, expecting 302
// status code on success
client.CheckRedirect = func(req *http.Request, via []*http.Request) error {
return http.ErrUseLastResponse
}

resp, err := client.Get(s.AdminURL().WithPath("/oidc/v1/login").String())
if err != nil {
t.Fatalf("could not issue GET request to admin server: %s", err)
}
defer resp.Body.Close()
bodyBytes, _ := io.ReadAll(resp.Body)

if !tc.wantError {
require.Equal(t, 302, resp.StatusCode)
} else {
require.Contains(t, string(bodyBytes), "OIDC: auth manager could not be initialized")
require.Equal(t, 500, resp.StatusCode)
}
})
}
}

0 comments on commit d2ea21c

Please sign in to comment.