use PKCE oauth flow for osprey

client/azure.go

@@ -30,8 +30,10 @@ type AzureConfig struct {
 	ServerApplicationID string `yaml:"server-application-id,omitempty"`
 	// ClientID is the oidc client id used for osprey
 	ClientID string `yaml:"client-id,omitempty"`
-	// ClientSecret is the oidc client secret used for osprey
-	ClientSecret string `yaml:"client-secret,omitempty"`
+	// The ClientSecret was formerly used as part of the OIDC flow.  It's no
+	// longer used, but the entry in the config struct is kept so that newer
+	// binaries can still deserialize old config files.
+	_ string `yaml:"client-secret,omitempty"`
 	// CertificateAuthority is the filesystem path from which to read the CA certificate
 	CertificateAuthority string `yaml:"certificate-authority,omitempty"`
 	// CertificateAuthorityData is base64-encoded CA cert data.
@@ -62,7 +64,7 @@ func (ac *AzureConfig) ValidateConfig() error {
 	if ac.ServerApplicationID == "" {
 		return errors.New("server-application-id is required for azure targets")
 	}
-	if ac.ClientID == "" || ac.ClientSecret == "" {
+	if ac.ClientID == "" {
 		return errors.New("oauth2 clientid and client-secret must be supplied for azure targets")
 	}
 	if ac.RedirectURI == "" {
@@ -80,10 +82,9 @@ func (ac *AzureConfig) ValidateConfig() error {
 // NewAzureRetriever creates new Azure oAuth client
 func NewAzureRetriever(provider *AzureConfig, options RetrieverOptions) (Retriever, error) {
 	config := oauth2.Config{
-		ClientID:     provider.ClientID,
-		ClientSecret: provider.ClientSecret,
-		RedirectURL:  provider.RedirectURI,
-		Scopes:       provider.Scopes,
+		ClientID:    provider.ClientID,
+		RedirectURL: provider.RedirectURI,
+		Scopes:      provider.Scopes,
 	}
 	if provider.IssuerURL == "" {
 		provider.IssuerURL = fmt.Sprintf("https://login.microsoftonline.com/%s/%s", provider.AzureTenantID, wellKnownConfigurationURI)

client/oidc/device.go

@@ -12,6 +12,7 @@ import (
 	"strings"
 	"time"
 
+	log "github.com/sirupsen/logrus"
 	"golang.org/x/net/context/ctxhttp"
 	"golang.org/x/oauth2"
 )
@@ -35,7 +36,11 @@ type pollResponse struct {
 func (c *Client) AuthWithDeviceFlow(ctx context.Context, loginTimeout time.Duration) (*oauth2.Token, error) {
 	c.oAuthConfig.RedirectURL = ""
 	// potential refactor: device_authorization_endpoint is now exposed by the Azure https://login.microsoftonline.com/<tenant-id>/v2.0/.well-known/openid-configuration
-	deviceAuthURL := strings.Replace(c.oAuthConfig.AuthCodeURL(ospreyState), "/authorize", "/devicecode", 1)
+	state, err := randomBytesInHex(24)
+	if err != nil {
+		log.Fatalf("unable to generate random bytes: %e", err)
+	}
+	deviceAuthURL := strings.Replace(c.oAuthConfig.AuthCodeURL(state), "/authorize", "/devicecode", 1)
 	urlParams := url.Values{"client_id": {c.oAuthConfig.ClientID}}
 	if len(c.oAuthConfig.Scopes) > 0 {
 		urlParams.Set("scope", strings.Join(c.oAuthConfig.Scopes, " "))

client/oidc/oidc.go

@@ -2,8 +2,14 @@ package oidc
 
 import (
 	"context"
+	"crypto/rand"
+	"crypto/sha256"
+	"crypto/subtle"
+	"encoding/base64"
+	"encoding/hex"
 	"errors"
 	"fmt"
+	"io"
 	"net/http"
 	"net/url"
 	"os/exec"
@@ -14,10 +20,6 @@ import (
 	"golang.org/x/oauth2"
 )
 
-const (
-	ospreyState = "as78*sadf$212"
-)
-
 // Client contains the details for a OIDC client
 type Client struct {
 	oAuthConfig         oauth2.Config
@@ -49,15 +51,32 @@ func (c *Client) AuthWithOIDCCallback(ctx context.Context, loginTimeout time.Dur
 		log.Fatalf("Unable to parse oidc redirect uri: %e", err)
 	}
 
-	authURL := c.oAuthConfig.AuthCodeURL(ospreyState)
+	codeVerifier, err := randomBytesInHex(32)
+	if err != nil {
+		log.Fatalf("unable to generate random bytes: %e", err)
+	}
+	sha := sha256.New()
+	io.WriteString(sha, codeVerifier)
+	codeChallenge := base64.RawURLEncoding.EncodeToString(sha.Sum(nil))
+
+	state, err := randomBytesInHex(24)
+	if err != nil {
+		log.Fatalf("unable to generate random bytes: %e", err)
+	}
+
+	authURL := c.oAuthConfig.AuthCodeURL(
+		state,
+		oauth2.SetAuthURLParam("code_challenge_method", "S256"),
+		oauth2.SetAuthURLParam("code_challenge", codeChallenge),
+	)
 	mux := http.NewServeMux()
 	h := &http.Server{Addr: fmt.Sprintf("%s", redirectURL.Host), Handler: mux}
 
 	mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
 		http.Redirect(w, r, authURL, http.StatusFound)
 	})
 
-	mux.HandleFunc(redirectURL.Path, c.handleRedirectURI(ctx))
+	mux.HandleFunc(redirectURL.Path, c.handleRedirectURI(ctx, state, codeVerifier))
 
 	ch := make(chan error)
 	ctxTimeout, cancel := context.WithTimeout(ctx, loginTimeout)
@@ -98,17 +117,15 @@ func (c *Client) AuthWithOIDCCallback(ctx context.Context, loginTimeout time.Dur
 		return nil, fmt.Errorf("unable to start local call-back webserver %w", err)
 	case resp := <-c.stopChan:
 		_ = h.Shutdown(ctx)
-		if resp.responseError != nil {
-			return nil, resp.responseError
-		}
-		return resp.accessToken, nil
+		return resp.accessToken, resp.responseError
 	}
 }
 
-func (c *Client) handleRedirectURI(ctx context.Context) http.HandlerFunc {
+func (c *Client) handleRedirectURI(ctx context.Context, state, codeVerifier string) http.HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request) {
 		defer close(c.stopChan)
-		if r.URL.Query().Get("state") != ospreyState {
+		if subtle.ConstantTimeCompare([]byte(r.URL.Query().Get("state")), []byte(state)) == 0 {
+			// `0` is the return value for "not equal," unlike strcmp and friends...
 			err := fmt.Errorf("state did not match")
 			http.Error(w, err.Error(), http.StatusBadRequest)
 			c.stopChan <- tokenResponse{
@@ -118,7 +135,7 @@ func (c *Client) handleRedirectURI(ctx context.Context) http.HandlerFunc {
 			return
 		}
 
-		oauth2Token, err := c.doAuthRequest(ctx, r)
+		oauth2Token, err := c.doAuthRequest(ctx, r, codeVerifier)
 		if err != nil {
 			err := fmt.Errorf("failed to exchange token: %w", err)
 			http.Error(w, err.Error(), http.StatusInternalServerError)
@@ -153,13 +170,22 @@ window.onload = setTimeout(closeWindow, 1000);
 	}
 }
 
-func (c *Client) doAuthRequest(ctx context.Context, r *http.Request) (*oauth2.Token, error) {
+func (c *Client) doAuthRequest(ctx context.Context, r *http.Request, codeVerifier string) (*oauth2.Token, error) {
 	authCode := r.URL.Query().Get("code")
-	var authCodeOptions []oauth2.AuthCodeOption
-	return c.oAuthConfig.Exchange(ctx, authCode, authCodeOptions...)
+	return c.oAuthConfig.Exchange(ctx, authCode, oauth2.SetAuthURLParam("code_verifier", codeVerifier))
 }
 
 // Authenticated returns a true or false value if a given OIDC client has received a successful login
 func (c *Client) Authenticated() bool {
 	return c.authenticated
 }
+
+func randomBytesInHex(count int) (string, error) {
+	buf := make([]byte, count)
+	_, err := io.ReadFull(rand.Reader, buf)
+	if err != nil {
+		return "", fmt.Errorf("Could not generate %d random bytes: %v", count, err)
+	}
+
+	return hex.EncodeToString(buf), nil
+}

e2e/ospreytest/server.go

@@ -191,7 +191,6 @@ func BuildFullConfig(testDir, providerName, defaultGroup string,
 		config.Providers = &client.Providers{
 			Azure: &client.AzureConfig{
 				ClientID:            clientID,
-				ClientSecret:        "some-client-secret",
 				RedirectURI:         "http://localhost:65525/auth/callback",
 				Scopes:              []string{"api://some-dummy-scope"},
 				AzureTenantID:       "some-tenant-id",