Skip to content
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(github): OAuth for GitHub #2016

Merged
merged 16 commits into from
Aug 22, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions internal/cmd/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ import (
"go.flipt.io/flipt/internal/containers"
"go.flipt.io/flipt/internal/gateway"
"go.flipt.io/flipt/internal/server/auth"
"go.flipt.io/flipt/internal/server/auth/method"
authkubernetes "go.flipt.io/flipt/internal/server/auth/method/kubernetes"
"go.flipt.io/flipt/internal/server/auth/method/oauth"
authoidc "go.flipt.io/flipt/internal/server/auth/method/oidc"
authtoken "go.flipt.io/flipt/internal/server/auth/method/token"
"go.flipt.io/flipt/internal/server/auth/public"
Expand Down Expand Up @@ -119,6 +121,15 @@ func authenticationGRPC(
logger.Debug("authentication method \"oidc\" server registered")
}

if authCfg.Methods.OAuth.Enabled {
oauthServer := oauth.NewServer(logger, store, authCfg)
register.Add(oauthServer)

authOpts = append(authOpts, auth.WithServerSkipsAuthentication(oauthServer))

logger.Debug("authentication method \"oauth\" registered")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
logger.Debug("authentication method \"oauth\" registered")
logger.Debug("authentication method \"github\" registered")

}

if authCfg.Methods.Kubernetes.Enabled {
kubernetesServer, err := authkubernetes.New(logger, store, authCfg)
if err != nil {
Expand Down Expand Up @@ -212,15 +223,25 @@ func authenticationHTTPMount(
}

if cfg.Methods.OIDC.Enabled {
oidcmiddleware := authoidc.NewHTTPMiddleware(cfg.Session)
oidcmiddleware := method.NewHTTPMiddleware(cfg.Session)
muxOpts = append(muxOpts,
runtime.WithMetadata(authoidc.ForwardCookies),
runtime.WithMetadata(method.ForwardCookies),
runtime.WithForwardResponseOption(oidcmiddleware.ForwardResponseOption),
registerFunc(ctx, conn, rpcauth.RegisterAuthenticationMethodOIDCServiceHandler))

middleware = append(middleware, oidcmiddleware.Handler)
}

if cfg.Methods.OAuth.Enabled {
oauthmiddleware := method.NewHTTPMiddleware(cfg.Session)
muxOpts = append(muxOpts,
runtime.WithMetadata(method.ForwardCookies),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would if we should add a method to AuthenticationConfig for whether or not session support needs enabling.

So we can do something like:

if cfg.SessionEnabled() {
    muxOpts = append(muxOpts, runtime.WithMetadata(method.ForwardCookies))
}

It could perform the same operation as what happens in the validate method right now:

func (c *AuthenticationConfig) validate() error {
var sessionEnabled bool
for _, info := range c.Methods.AllMethods() {
sessionEnabled = sessionEnabled || (info.Enabled && info.SessionCompatible)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea being that we dont append the middleware twice when both are enabled.

runtime.WithForwardResponseOption(oauthmiddleware.ForwardResponseOption),
registerFunc(ctx, conn, rpcauth.RegisterAuthenticationMethodOAuthServiceHandler))

middleware = append(middleware, oauthmiddleware.Handler)
}

if cfg.Methods.Kubernetes.Enabled {
muxOpts = append(muxOpts, registerFunc(ctx, conn, rpcauth.RegisterAuthenticationMethodKubernetesServiceHandler))
}
Expand Down
48 changes: 48 additions & 0 deletions internal/config/authentication.go
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ type AuthenticationSessionCSRF struct {
// method available for use within Flipt.
type AuthenticationMethods struct {
Token AuthenticationMethod[AuthenticationMethodTokenConfig] `json:"token,omitempty" mapstructure:"token"`
OAuth AuthenticationMethod[AuthenticationMethodOAuthConfig] `json:"oauth,omitempty" mapstructure:"oauth"`
OIDC AuthenticationMethod[AuthenticationMethodOIDCConfig] `json:"oidc,omitempty" mapstructure:"oidc"`
Kubernetes AuthenticationMethod[AuthenticationMethodKubernetesConfig] `json:"kubernetes,omitempty" mapstructure:"kubernetes"`
}
Expand All @@ -200,6 +201,7 @@ type AuthenticationMethods struct {
func (a *AuthenticationMethods) AllMethods() []StaticAuthenticationMethodInfo {
return []StaticAuthenticationMethodInfo{
a.Token.info(),
a.OAuth.info(),
a.OIDC.info(),
a.Kubernetes.info(),
}
Expand Down Expand Up @@ -405,3 +407,49 @@ func (a AuthenticationMethodKubernetesConfig) info() AuthenticationMethodInfo {
SessionCompatible: false,
}
}

// AuthenticationMethodOAuthConfig maps an OAuth hosts to pieces of configuration
// that will allow for completion of the OAuth flow.
type AuthenticationMethodOAuthConfig struct {
Hosts map[string]AuthenticationMethodOAuthProvider `json:"hosts,omitempty" mapstructure:"hosts"`
}

func (a AuthenticationMethodOAuthConfig) setDefaults(defaults map[string]any) {}

// info describes properties of the authentication method "oauth".
func (a AuthenticationMethodOAuthConfig) info() AuthenticationMethodInfo {
c := AuthenticationMethodInfo{
Method: auth.Method_METHOD_OAUTH,
SessionCompatible: false,
}

var (
metadata = make(map[string]any)
hosts = make(map[string]any, len(a.Hosts))
)

// this ensures we expose the authorize and callback URL endpoint
// to the UI via the /auth/v1/method endpoint
for host := range a.Hosts {
hosts[host] = map[string]any{
"authorize_url": fmt.Sprintf("/auth/v1/method/oauth/%s/authorize", host),
"callback_url": fmt.Sprintf("/auth/v1/method/oauth/%s/callback", host),
}
}

metadata["hosts"] = hosts

c.Metadata, _ = structpb.NewStruct(metadata)

return c
}

// AuthenticationMethodOAuthProvider holds all relevant configuration for providing the client
// with an AuthorizeURL to use, and to issue back to the client an access token
// for authorized requests on the relevant server.
type AuthenticationMethodOAuthProvider struct {
ClientSecret string `json:"clientSecret,omitempty" mapstructure:"client_secret"`
ClientId string `json:"clientId,omitempty" mapstructure:"client_id"`
RedirectAddress string `json:"redirectAddress,omitempty" mapstructure:"redirect_address"`
Scopes []string `json:"scopes,omitempty" mapstructure:"scopes"`
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package oidc
package method

import (
"context"
Expand All @@ -10,7 +10,6 @@ import (
"time"

"go.flipt.io/flipt/internal/config"
"go.flipt.io/flipt/rpc/flipt/auth"
"google.golang.org/grpc/metadata"
"google.golang.org/protobuf/proto"
)
Expand Down Expand Up @@ -57,11 +56,16 @@ func ForwardCookies(ctx context.Context, req *http.Request) metadata.MD {
// This ensures a secure browser session can be established.
// The user-agent is then redirected to the root of the domain.
func (m Middleware) ForwardResponseOption(ctx context.Context, w http.ResponseWriter, resp proto.Message) error {
r, ok := resp.(*auth.CallbackResponse)
type CallbackResponseCleaner interface {
GetClientToken() string
StripClientToken()
}

r, ok := resp.(CallbackResponseCleaner)
if ok {
cookie := &http.Cookie{
Name: tokenCookieKey,
Value: r.ClientToken,
Value: r.GetClientToken(),
Domain: m.config.Domain,
Path: "/",
Expires: time.Now().Add(m.config.TokenLifetime),
Expand All @@ -73,7 +77,7 @@ func (m Middleware) ForwardResponseOption(ctx context.Context, w http.ResponseWr
http.SetCookie(w, cookie)

// clear out token now that it is set via cookie
r.ClientToken = ""
r.StripClientToken()

w.Header().Set("Location", "/")
w.WriteHeader(http.StatusFound)
Expand All @@ -90,7 +94,7 @@ func (m Middleware) ForwardResponseOption(ctx context.Context, w http.ResponseWr
// The payload is then also encoded as a http cookie which is bound to the callback path.
func (m Middleware) Handler(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
provider, method, match := parts(r.URL.Path)
provider, method, prefix, match := parts(r.URL.Path)
if !match {
next.ServeHTTP(w, r)
return
Expand Down Expand Up @@ -126,7 +130,7 @@ func (m Middleware) Handler(next http.Handler) http.Handler {
Name: stateCookieKey,
Value: encoded,
// bind state cookie to provider callback
Path: "/auth/v1/method/oidc/" + provider + "/callback",
Path: prefix + provider + "/callback",
Expires: time.Now().Add(m.config.StateLifetime),
Secure: m.config.Secure,
HttpOnly: true,
Expand All @@ -150,13 +154,20 @@ func (m Middleware) Handler(next http.Handler) http.Handler {
})
}

func parts(path string) (provider, method string, ok bool) {
const prefix = "/auth/v1/method/oidc/"
if !strings.HasPrefix(path, prefix) {
return "", "", false
func parts(path string) (provider, method, prefix string, ok bool) {
var oidcPrefix = "/auth/v1/method/oidc/"
if strings.HasPrefix(path, oidcPrefix) {
b, a, f := strings.Cut(path[len(oidcPrefix):], "/")
return b, a, oidcPrefix, f
}

var oauthPrefix = "/auth/v1/method/oauth/"
if strings.HasPrefix(path, oauthPrefix) {
b, a, f := strings.Cut(path[len(oauthPrefix):], "/")
return b, a, oauthPrefix, f
}

return strings.Cut(path[len(prefix):], "/")
return "", "", "", false
}

func generateSecurityToken() string {
Expand Down
168 changes: 168 additions & 0 deletions internal/server/auth/method/oauth/server.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
package oauth

import (
"context"
"encoding/json"
"net/http"
"net/url"
"strings"
"time"

"go.flipt.io/flipt/errors"
"go.flipt.io/flipt/internal/config"
storageauth "go.flipt.io/flipt/internal/storage/auth"
"go.flipt.io/flipt/rpc/flipt/auth"
"go.uber.org/zap"
"google.golang.org/grpc"
"google.golang.org/grpc/metadata"
"google.golang.org/protobuf/types/known/timestamppb"
)

const (
storageMetadataGithubAccessToken = "io.flipt.auth.oauth.access_token"

Check failure on line 22 in internal/server/auth/method/oauth/server.go

View workflow job for this annotation

GitHub Actions / Lint Go

G101: Potential hardcoded credentials (gosec)
)

var (
hostToAuthorizeBaseURL = map[string]string{
"github": "https://github.com/login/oauth/authorize",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just thinking out loud, the only thing that makes this GitHub specific and not 'general OAuth' support are URLS right? Also we may want to think about users who run GH Enterprise in their domain, but that could probably be added later

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kind of. I think what URLs you can access with the recieved access token, how you access them (query params vs e.g. body) and the shape of the responses of these endpoints can vary from one implementation to the next.

This is where OIDC has a standard around the UserInfo endpoint and solves that problem.
Plus they close the loop here on finishing the authorization leg in a standard way.

}
hostToAccessTokenBaseURL = map[string]string{
"github": "https://github.com/login/oauth/access_token",
}
)

// Server is an OAuth server side handler.
type Server struct {
logger *zap.Logger
store storageauth.Store
config config.AuthenticationConfig

auth.UnimplementedAuthenticationMethodOAuthServiceServer
}

// NewServer constructs a Server.
func NewServer(
logger *zap.Logger,
store storageauth.Store,
config config.AuthenticationConfig,
) *Server {
return &Server{
logger: logger,
store: store,
config: config,
}
}

// RegisterGRPC registers the server as an Server on the provided grpc server.
func (s *Server) RegisterGRPC(server *grpc.Server) {
auth.RegisterAuthenticationMethodOAuthServiceServer(server, s)
}

func callbackURL(host, oauthHost string) string {
// strip trailing slash from host
host = strings.TrimSuffix(host, "/")
return host + "/auth/v1/method/oauth/" + oauthHost + "/callback"
}

// AuthorizeURL will return a URL for the client to redirect to for completion of the OAuth flow with GitHub.
func (s *Server) AuthorizeURL(ctx context.Context, a *auth.OAuthAuthorizeRequest) (*auth.AuthorizeURLResponse, error) {
oauthConfig := s.config.Methods.OAuth.Method.Hosts[a.Host]

authorizeBaseURL := hostToAuthorizeBaseURL[a.Host]

u, err := url.Parse(authorizeBaseURL)
if err != nil {
return nil, err
}

// Build OAuth authorize url.
q := u.Query()

q.Set("client_id", oauthConfig.ClientId)
q.Set("scope", strings.Join(oauthConfig.Scopes, ":"))
q.Set("redirect_uri", callbackURL(oauthConfig.RedirectAddress, a.Host))
q.Set("state", a.State)

u.RawQuery = q.Encode()

return &auth.AuthorizeURLResponse{
AuthorizeUrl: u.String(),
}, nil
}

// OAuthCallback is the OAuth callback method for OAuth authentication. It will take in a SessionCode
// which is the OAuth grant passed in by the OAuth service, and exchange the grant with an Authentication
// that includes the access token.
func (s *Server) OAuthCallback(ctx context.Context, oauth *auth.OAuthCallbackRequest) (*auth.OAuthCallbackResponse, error) {
if oauth.State != "" {
md, ok := metadata.FromIncomingContext(ctx)
if !ok {
return nil, errors.ErrUnauthenticatedf("missing state parameter")
}

state, ok := md["flipt_client_state"]
if !ok || len(state) < 1 {
return nil, errors.ErrUnauthenticatedf("missing state parameter")
}

if oauth.State != state[0] {
return nil, errors.ErrUnauthenticatedf("unexpected state parameter")
}
}

oauthConfig := s.config.Methods.OAuth.Method.Hosts[oauth.Host]

c := &http.Client{
Timeout: 5 * time.Second,
}

accessTokenURL := hostToAccessTokenBaseURL[oauth.Host]

req, err := http.NewRequestWithContext(ctx, "POST", accessTokenURL, nil)
if err != nil {
return nil, err
}

q := req.URL.Query()
q.Set("client_id", oauthConfig.ClientId)
q.Set("client_secret", oauthConfig.ClientSecret)
q.Set("code", oauth.Code)
e := q.Encode()
req.URL.RawQuery = e

// We have to accept JSON from the GitHub server when requesting the access token.
req.Header.Set("Accept", "application/json")

resp, err := c.Do(req)
if err != nil {
return nil, err
}
defer resp.Body.Close()

var oauthAccessTokenResponse struct {
AccessToken string `json:"access_token,omitempty"`
Scopes []string `json:"scopes,omitempty"`
}

if err := json.NewDecoder(resp.Body).Decode(&oauthAccessTokenResponse); err != nil {
return nil, err
}

metadata := map[string]string{
storageMetadataGithubAccessToken: oauthAccessTokenResponse.AccessToken,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we're really going to want to store this.
Particularly in this way, which will leak it through the API.
Instead, we should just use it to get identity information and just put the pieces we get from Github in the metadata (profile information etc.). That is what we do in the OIDC method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GeorgeMac Okay was thinking that too. I did it like this because I was wondering if we wanted to keep that access token around for making authorized requests to GH

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, once we have used it to finish the auth flow and get identity information we can discard it.
Somewhere down the line we might want to re-explore storing this, refresh tokens and so on.
But if we do, we would likely store it somewhere not API accessible.

}

clientToken, a, err := s.store.CreateAuthentication(ctx, &storageauth.CreateAuthenticationRequest{
Method: auth.Method_METHOD_OAUTH,
ExpiresAt: timestamppb.New(time.Now().UTC().Add(s.config.Session.TokenLifetime)),
Metadata: metadata,
})
if err != nil {
return nil, err
}

return &auth.OAuthCallbackResponse{
ClientToken: clientToken,
Authentication: a,
}, nil
}
6 changes: 3 additions & 3 deletions internal/server/auth/method/oidc/testing/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
"github.com/stretchr/testify/require"
"go.flipt.io/flipt/internal/config"
"go.flipt.io/flipt/internal/gateway"
"go.flipt.io/flipt/internal/server/auth/method/oidc"
"go.flipt.io/flipt/internal/server/auth/method"
"go.flipt.io/flipt/rpc/flipt/auth"
"go.uber.org/zap"
)
Expand All @@ -32,10 +32,10 @@ func StartHTTPServer(
GRPCServer: StartGRPCServer(t, ctx, logger, conf),
}

oidcmiddleware = oidc.NewHTTPMiddleware(conf.Session)
oidcmiddleware = method.NewHTTPMiddleware(conf.Session)
mux = gateway.NewGatewayServeMux(
logger,
runtime.WithMetadata(oidc.ForwardCookies),
runtime.WithMetadata(method.ForwardCookies),
runtime.WithForwardResponseOption(oidcmiddleware.ForwardResponseOption),
)
)
Expand Down
Loading
Loading