Store encrypted values in new DB column.

Relates to #3315

When encrypting secrets, store the serialized JSON in the database. When
decrypting, attempt to fetch from the new column and then fall back to
the old one.

In a subsequent PR, I will migrate all existing secrets to the new
structure.

database/query/session_store.sql

@@ -1,8 +1,8 @@
 -- name: CreateSessionState :one
-INSERT INTO session_store (provider, project_id, remote_user, session_state, owner_filter, provider_config, redirect_url) VALUES ($1, $2, $3, $4, $5, $6, $7) RETURNING *;
+INSERT INTO session_store (provider, project_id, remote_user, session_state, owner_filter, provider_config, redirect_url, encrypted_redirect) VALUES ($1, $2, $3, $4, $5, $6, $7, $8) RETURNING *;
 
 -- name: GetProjectIDBySessionState :one
-SELECT provider, project_id, remote_user, owner_filter, provider_config, redirect_url FROM session_store WHERE session_state = $1;
+SELECT provider, project_id, remote_user, owner_filter, provider_config, redirect_url, encrypted_redirect FROM session_store WHERE session_state = $1;
 
 -- name: DeleteSessionStateByProjectID :exec
 DELETE FROM session_store WHERE provider = $1 AND project_id = $2;

internal/controlplane/handlers_oauth.go

@@ -29,6 +29,7 @@ import (
 	"github.com/grpc-ecosystem/grpc-gateway/v2/runtime"
 	"github.com/rs/zerolog"
 	"github.com/rs/zerolog/log"
+	"github.com/sqlc-dev/pqtype"
 	"go.opentelemetry.io/otel/attribute"
 	"go.opentelemetry.io/otel/trace"
 	"golang.org/x/oauth2"
@@ -115,14 +116,14 @@ func (s *Server) GetAuthorizationURL(ctx context.Context,
 		String: req.GetOwner(),
 	}
 
-	var redirectUrl sql.NullString
+	var encryptedRedirectURL mcrypto.EncryptedData
+	var encryptedBytes pqtype.NullRawMessage
 	// Empty redirect URL means null string (default condition)
 	if req.GetRedirectUrl() != "" {
-		encryptedRedirectUrl, err := s.cryptoEngine.EncryptString(*req.RedirectUrl)
+		encryptedBytes, err = s.encryptRedirect(req)
 		if err != nil {
-			return nil, status.Errorf(codes.Internal, "error encrypting redirect URL: %s", err)
+			return nil, status.Errorf(codes.Internal, "unable to encrypt secret: %v", err)
 		}
-		redirectUrl = sql.NullString{Valid: true, String: encryptedRedirectUrl.EncodedData}
 	}
 
 	var confBytes []byte
@@ -136,13 +137,14 @@ func (s *Server) GetAuthorizationURL(ctx context.Context,
 	// Insert the new session state into the database along with the user's project ID
 	// retrieved from the JWT token
 	_, err = s.store.CreateSessionState(ctx, db.CreateSessionStateParams{
-		Provider:       providerName,
-		ProjectID:      projectID,
-		RemoteUser:     sql.NullString{Valid: user != "", String: user},
-		SessionState:   state,
-		OwnerFilter:    owner,
-		RedirectUrl:    redirectUrl,
-		ProviderConfig: confBytes,
+		Provider:          providerName,
+		ProjectID:         projectID,
+		RemoteUser:        sql.NullString{Valid: user != "", String: user},
+		SessionState:      state,
+		OwnerFilter:       owner,
+		ProviderConfig:    confBytes,
+		RedirectUrl:       sql.NullString{Valid: true, String: encryptedRedirectURL.EncodedData},
+		EncryptedRedirect: encryptedBytes,
 	})
 	if err != nil {
 		return nil, status.Errorf(codes.Unknown, "error inserting session state: %s", err)
@@ -261,19 +263,13 @@ func (s *Server) processOAuthCallback(ctx context.Context, w http.ResponseWriter
 
 	logger.BusinessRecord(ctx).ProviderID = p.ID
 
+	// Note: right now, both RedirectUrl and EncryptedRedirect should both be valid
 	if stateData.RedirectUrl.Valid {
-		// TODO: get rid of this once we store the EncryptedData struct in
-		// the database.
-		encryptedData := mcrypto.NewBackwardsCompatibleEncryptedData(stateData.RedirectUrl.String)
-		redirectUrl, err := s.cryptoEngine.DecryptString(encryptedData)
-		if err != nil {
-			return fmt.Errorf("error decrypting redirect URL: %w", err)
-		}
-		parsedURL, err := url.Parse(redirectUrl)
+		redirectURL, err := s.decryptRedirect(&stateData)
 		if err != nil {
-			return fmt.Errorf("error parsing redirect URL: %w", err)
+			return fmt.Errorf("unable to decrypt redirect URL: %w", err)
 		}
-		http.Redirect(w, r, parsedURL.String(), http.StatusTemporaryRedirect)
+		http.Redirect(w, r, redirectURL.String(), http.StatusTemporaryRedirect)
 		return nil
 	}
 
@@ -342,20 +338,12 @@ func (s *Server) processAppCallback(ctx context.Context, w http.ResponseWriter,
 			return fmt.Errorf("error creating GitHub App provider: %w", err)
 		}
 
-		// If we have a redirect URL, redirect the user, otherwise show a success page
 		if stateData.RedirectUrl.Valid {
-			// TODO: get rid of this once we store the EncryptedData struct in
-			// the database.
-			encryptedData := mcrypto.NewBackwardsCompatibleEncryptedData(stateData.RedirectUrl.String)
-			redirectUrl, err := s.cryptoEngine.DecryptString(encryptedData)
+			redirectURL, err := s.decryptRedirect(&stateData)
 			if err != nil {
-				return fmt.Errorf("error decrypting redirect URL: %w", err)
+				return fmt.Errorf("unable to decrypt redirect URL: %w", err)
 			}
-			parsedURL, err := url.Parse(redirectUrl)
-			if err != nil {
-				return fmt.Errorf("error parsing redirect URL: %w", err)
-			}
-			http.Redirect(w, r, parsedURL.String(), http.StatusTemporaryRedirect)
+			http.Redirect(w, r, redirectURL.String(), http.StatusTemporaryRedirect)
 			return nil
 		}
 	} else {
@@ -484,6 +472,11 @@ func (s *Server) StoreProviderToken(ctx context.Context,
 		return nil, err
 	}
 
+	serialized, err := encryptedToken.Serialize()
+	if err != nil {
+		return nil, status.Errorf(codes.Internal, "error serializing secret: %s", err)
+	}
+
 	// additionally, add an owner
 	var owner sql.NullString
 	if in.Owner == nil {
@@ -497,6 +490,10 @@ func (s *Server) StoreProviderToken(ctx context.Context,
 		Provider:       provider.Name,
 		EncryptedToken: encryptedToken.EncodedData,
 		OwnerFilter:    owner,
+		EncryptedAccessToken: pqtype.NullRawMessage{
+			RawMessage: serialized,
+			Valid:      true,
+		},
 	})
 	if err != nil {
 		return nil, status.Errorf(codes.Internal, "error storing access token: %v", err)
@@ -654,3 +651,41 @@ func (e *httpResponseError) Error() string {
 func (e *httpResponseError) WriteError(w http.ResponseWriter) {
 	http.Error(w, e.pageContents, e.statusCode)
 }
+
+func (s *Server) decryptRedirect(stateData *db.GetProjectIDBySessionStateRow) (*url.URL, error) {
+	var err error
+	// TODO: get rid of this once we migrate all secrets to use the new structure
+	var encryptedData mcrypto.EncryptedData
+	if stateData.EncryptedRedirect.Valid {
+		encryptedData, err = mcrypto.DeserializeEncryptedData(stateData.EncryptedRedirect.RawMessage)
+		if err != nil {
+			return nil, err
+		}
+	} else {
+		encryptedData = mcrypto.NewBackwardsCompatibleEncryptedData(stateData.RedirectUrl.String)
+	}
+	redirectUrl, err := s.cryptoEngine.DecryptString(encryptedData)
+	if err != nil {
+		return nil, fmt.Errorf("error decrypting redirect URL: %w", err)
+	}
+	parsedURL, err := url.Parse(redirectUrl)
+	if err != nil {
+		return nil, fmt.Errorf("error parsing redirect URL: %w", err)
+	}
+	return parsedURL, nil
+}
+
+func (s *Server) encryptRedirect(req *pb.GetAuthorizationURLRequest) (pqtype.NullRawMessage, error) {
+	encryptedRedirectURL, err := s.cryptoEngine.EncryptString(*req.RedirectUrl)
+	if err != nil {
+		return pqtype.NullRawMessage{}, status.Errorf(codes.Internal, "error encrypting redirect URL: %s", err)
+	}
+	serialized, err := encryptedRedirectURL.Serialize()
+	if err != nil {
+		return pqtype.NullRawMessage{}, status.Errorf(codes.Internal, "unable to serialize secret: %v", err)
+	}
+	return pqtype.NullRawMessage{
+		RawMessage: serialized,
+		Valid:      true,
+	}, nil
+}

internal/controlplane/handlers_oauth_test.go

@@ -29,8 +29,6 @@ import (
 	"net/url"
 	"testing"
 
-	"github.com/google/go-cmp/cmp"
-	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/google/uuid"
 	"github.com/lestrrat-go/jwx/v2/jwt/openid"
 	"github.com/stretchr/testify/assert"
@@ -835,8 +833,12 @@ func (p partialDbParamsMatcher) Matches(x interface{}) bool {
 
 	typedX.SessionState = ""
 
-	return cmp.Equal(typedX, p.value,
-		cmpopts.IgnoreFields(db.CreateSessionStateParams{}, "ProviderConfig"))
+	// this previously compared the two structs directly, but the addition of NullRawMessage
+	// broke this. Since we only ever need to worry about a specific subset of fields, I'm
+	// cheating by only comparing the fields we care about.
+	return typedX.ProjectID == p.value.ProjectID &&
+		typedX.Provider == p.value.Provider &&
+		typedX.OwnerFilter == p.value.OwnerFilter
 }
 
 func (m partialDbParamsMatcher) String() string {

internal/crypto/models.go

@@ -54,11 +54,11 @@ func NewBackwardsCompatibleEncryptedData(encryptedData string) EncryptedData {
 }
 
 // DeserializeEncryptedData deserialized the data generated by EncryptedData.Serialize()
-func DeserializeEncryptedData(contents json.RawMessage) (*EncryptedData, error) {
+func DeserializeEncryptedData(contents json.RawMessage) (EncryptedData, error) {
 	var data EncryptedData
 	err := json.Unmarshal(contents, &data)
 	if err != nil {
-		return nil, fmt.Errorf("unable to deserialize data: %w", err)
+		return EncryptedData{}, fmt.Errorf("unable to deserialize data: %w", err)
 	}
-	return &data, nil
+	return data, nil
 }

internal/providers/dockerhub/manager.go

@@ -93,9 +93,16 @@ func (m *providerClassManager) getProviderCredentials(
 		return nil, fmt.Errorf("error getting credential: %w", err)
 	}
 
-	// TODO: get rid of this once we store the EncryptedData struct in
-	// the database.
-	encryptedData := crypto.NewBackwardsCompatibleEncryptedData(encToken.EncryptedToken)
+	// TODO: get rid of this once we migrate all secrets to use the new structure
+	var encryptedData crypto.EncryptedData
+	if encToken.EncryptedAccessToken.Valid {
+		encryptedData, err = crypto.DeserializeEncryptedData(encToken.EncryptedAccessToken.RawMessage)
+		if err != nil {
+			return nil, err
+		}
+	} else {
+		encryptedData = crypto.NewBackwardsCompatibleEncryptedData(encToken.EncryptedToken)
+	}
 	decryptedToken, err := m.crypteng.DecryptOAuthToken(encryptedData)
 	if err != nil {
 		return nil, fmt.Errorf("error decrypting access token: %w", err)

internal/providers/github/manager/manager.go

@@ -191,9 +191,17 @@ func (g *githubProviderManager) createProviderWithAccessToken(
 	ctx context.Context,
 	encToken db.ProviderAccessToken,
 ) (*credentialDetails, error) {
-	// TODO: get rid of this once we store the EncryptedData struct in
-	// the database.
-	encryptedData := crypto.NewBackwardsCompatibleEncryptedData(encToken.EncryptedToken)
+	// TODO: get rid of this once we migrate all secrets to use the new structure
+	var err error
+	var encryptedData crypto.EncryptedData
+	if encToken.EncryptedAccessToken.Valid {
+		encryptedData, err = crypto.DeserializeEncryptedData(encToken.EncryptedAccessToken.RawMessage)
+		if err != nil {
+			return nil, err
+		}
+	} else {
+		encryptedData = crypto.NewBackwardsCompatibleEncryptedData(encToken.EncryptedToken)
+	}
 	decryptedToken, err := g.crypteng.DecryptOAuthToken(encryptedData)
 	if err != nil {
 		return nil, fmt.Errorf("error decrypting access token: %w", err)
@@ -219,7 +227,6 @@ func (g *githubProviderManager) getProviderCredentials(
 		db.GetAccessTokenByProjectIDParams{Provider: prov.Name, ProjectID: prov.ProjectID})
 	if errors.Is(err, sql.ErrNoRows) {
 		zerolog.Ctx(ctx).Debug().Msg("no access token found for provider")
-
 		// If we don't have an access token, check if we have an installation ID
 		return g.createProviderWithInstallationToken(ctx, prov)
 	} else if err != nil {