Skip to content

Commit

Permalink
fix: no longer auto-generate system secret
Browse files Browse the repository at this point in the history
This patch changes Ory Hydra's behavior to no longer auto-generate a temporary secret when no global secret was set. The APIs now return an error instead.

See ory/network#185
  • Loading branch information
aeneasr committed Dec 7, 2022
1 parent 48217bd commit c5fe043
Show file tree
Hide file tree
Showing 11 changed files with 113 additions and 49 deletions.
51 changes: 43 additions & 8 deletions consent/strategy_default.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,13 @@ func (s *DefaultStrategy) matchesValueFromSession(ctx context.Context, c fosite.
}

func (s *DefaultStrategy) authenticationSession(ctx context.Context, w http.ResponseWriter, r *http.Request) (*LoginSession, error) {
store, err := s.r.CookieStore(ctx)
if err != nil {
return nil, err
}

// We try to open the session cookie. If it does not exist (indicated by the error), we must authenticate the user.
cookie, err := s.r.CookieStore(ctx).Get(r, s.c.SessionCookieName(ctx))
cookie, err := store.Get(r, s.c.SessionCookieName(ctx))
if err != nil {
s.r.Logger().
WithRequest(r).
Expand Down Expand Up @@ -261,8 +266,13 @@ func (s *DefaultStrategy) forwardAuthenticationRequest(ctx context.Context, w ht
return errorsx.WithStack(err)
}

store, err := s.r.CookieStore(ctx)
if err != nil {
return err
}

clientSpecificCookieNameLoginCSRF := fmt.Sprintf("%s_%d", s.r.Config().CookieNameLoginCSRF(ctx), murmur3.Sum32(cl.ID.Bytes()))
if err := createCsrfSession(w, r, s.r.Config(), s.r.CookieStore(ctx), clientSpecificCookieNameLoginCSRF, csrf, s.c.ConsentRequestMaxAge(ctx)); err != nil {
if err := createCsrfSession(w, r, s.r.Config(), store, clientSpecificCookieNameLoginCSRF, csrf, s.c.ConsentRequestMaxAge(ctx)); err != nil {
return errorsx.WithStack(err)
}

Expand All @@ -273,7 +283,12 @@ func (s *DefaultStrategy) forwardAuthenticationRequest(ctx context.Context, w ht
}

func (s *DefaultStrategy) revokeAuthenticationSession(ctx context.Context, w http.ResponseWriter, r *http.Request) error {
sid, err := s.revokeAuthenticationCookie(w, r, s.r.CookieStore(ctx))
store, err := s.r.CookieStore(ctx)
if err != nil {
return err
}

sid, err := s.revokeAuthenticationCookie(w, r, store)
if err != nil {
return err
}
Expand Down Expand Up @@ -323,8 +338,13 @@ func (s *DefaultStrategy) verifyAuthentication(w http.ResponseWriter, r *http.Re
return nil, errorsx.WithStack(fosite.ErrRequestUnauthorized.WithHint("The login request has expired. Please try again."))
}

store, err := s.r.CookieStore(ctx)
if err != nil {
return nil, err
}

clientSpecificCookieNameLoginCSRF := fmt.Sprintf("%s_%d", s.r.Config().CookieNameLoginCSRF(ctx), murmur3.Sum32(session.LoginRequest.Client.ID.Bytes()))
if err := validateCsrfSession(r, s.r.Config(), s.r.CookieStore(ctx), clientSpecificCookieNameLoginCSRF, session.LoginRequest.CSRF); err != nil {
if err := validateCsrfSession(r, s.r.Config(), store, clientSpecificCookieNameLoginCSRF, session.LoginRequest.CSRF); err != nil {
return nil, err
}

Expand Down Expand Up @@ -421,7 +441,7 @@ func (s *DefaultStrategy) verifyAuthentication(w http.ResponseWriter, r *http.Re
}

// Not a skipped login and the user asked to remember its session, store a cookie
cookie, _ := s.r.CookieStore(ctx).Get(r, s.c.SessionCookieName(ctx))
cookie, _ := store.Get(r, s.c.SessionCookieName(ctx))
cookie.Values[CookieAuthenticationSIDName] = sessionID
if session.RememberFor >= 0 {
cookie.Options.MaxAge = session.RememberFor
Expand Down Expand Up @@ -539,8 +559,13 @@ func (s *DefaultStrategy) forwardConsentRequest(ctx context.Context, w http.Resp
return errorsx.WithStack(err)
}

store, err := s.r.CookieStore(ctx)
if err != nil {
return err
}

clientSpecificCookieNameConsentCSRF := fmt.Sprintf("%s_%d", s.r.Config().CookieNameConsentCSRF(ctx), murmur3.Sum32(cl.ID.Bytes()))
if err := createCsrfSession(w, r, s.r.Config(), s.r.CookieStore(ctx), clientSpecificCookieNameConsentCSRF, csrf, s.c.ConsentRequestMaxAge(ctx)); err != nil {
if err := createCsrfSession(w, r, s.r.Config(), store, clientSpecificCookieNameConsentCSRF, csrf, s.c.ConsentRequestMaxAge(ctx)); err != nil {
return errorsx.WithStack(err)
}

Expand Down Expand Up @@ -575,8 +600,13 @@ func (s *DefaultStrategy) verifyConsent(ctx context.Context, w http.ResponseWrit
return nil, errorsx.WithStack(fosite.ErrServerError.WithHint("The authenticatedAt value was not set."))
}

store, err := s.r.CookieStore(ctx)
if err != nil {
return nil, err
}

clientSpecificCookieNameConsentCSRF := fmt.Sprintf("%s_%d", s.r.Config().CookieNameConsentCSRF(ctx), murmur3.Sum32(session.ConsentRequest.Client.ID.Bytes()))
if err := validateCsrfSession(r, s.r.Config(), s.r.CookieStore(ctx), clientSpecificCookieNameConsentCSRF, session.ConsentRequest.CSRF); err != nil {
if err := validateCsrfSession(r, s.r.Config(), store, clientSpecificCookieNameConsentCSRF, session.ConsentRequest.CSRF); err != nil {
return nil, err
}

Expand Down Expand Up @@ -914,7 +944,12 @@ func (s *DefaultStrategy) completeLogout(ctx context.Context, w http.ResponseWri
}
}

_, _ = s.revokeAuthenticationCookie(w, r, s.r.CookieStore(ctx)) // Cookie removal is optional
store, err := s.r.CookieStore(ctx)
if err != nil {
return nil, err
}

_, _ = s.revokeAuthenticationCookie(w, r, store) // Cookie removal is optional

urls, err := s.generateFrontChannelLogoutURLs(r.Context(), lr.Subject, lr.SessionID)
if err != nil {
Expand Down
4 changes: 3 additions & 1 deletion consent/strategy_default_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,9 @@ func newAuthCookieJar(t *testing.T, reg driver.Registry, u, sessionID string) ht
require.NoError(t, err)

hr := &http.Request{Header: map[string][]string{}, URL: urlx.ParseOrPanic(u), RequestURI: u}
cookie, _ := reg.CookieStore(ctx).Get(hr, reg.Config().SessionCookieName(ctx))
s, err := reg.CookieStore(ctx)
require.NoError(t, err)
cookie, _ := s.Get(hr, reg.Config().SessionCookieName(ctx))

cookie.Values[CookieAuthenticationSIDName] = sessionID
cookie.Options.HttpOnly = true
Expand Down
10 changes: 7 additions & 3 deletions driver/config/provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -313,17 +313,21 @@ func (p *DefaultProvider) Tracing() *otelx.Config {
return p.getProvider(contextx.RootContext).TracingConfig("Ory Hydra")
}

func (p *DefaultProvider) GetCookieSecrets(ctx context.Context) [][]byte {
func (p *DefaultProvider) GetCookieSecrets(ctx context.Context) ([][]byte, error) {
secrets := p.getProvider(ctx).Strings(KeyGetCookieSecrets)
if len(secrets) == 0 {
return [][]byte{p.GetGlobalSecret(ctx)}
secret, err := p.GetGlobalSecret(ctx)
if err != nil {
return nil, err
}
return [][]byte{secret}, nil
}

bs := make([][]byte, len(secrets))
for k := range secrets {
bs[k] = []byte(secrets[k])
}
return bs
return bs, nil
}

func (p *DefaultProvider) LogoutRedirectURL(ctx context.Context) *url.URL {
Expand Down
34 changes: 12 additions & 22 deletions driver/config/provider_fosite.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,56 +8,46 @@ import (
"strings"
"time"

"github.com/pkg/errors"

"github.com/ory/fosite"
"github.com/ory/hydra/x"
"github.com/ory/x/cmdx"
)

var _ fosite.GlobalSecretProvider = (*DefaultProvider)(nil)

func (p *DefaultProvider) GetGlobalSecret(ctx context.Context) []byte {
func (p *DefaultProvider) GetGlobalSecret(ctx context.Context) ([]byte, error) {
secrets := p.getProvider(ctx).Strings(KeyGetSystemSecret)

if len(secrets) == 0 {
if p.generatedSecret != nil {
return p.generatedSecret
}

p.l.Warnf("Configuration secrets.system is not set, generating a temporary, random secret...")
secret, err := x.GenerateSecret(32)
cmdx.Must(err, "Could not generate secret: %s", err)

p.l.Warnf("Generated secret: %s", secret)
p.generatedSecret = x.HashByteSecret(secret)

p.l.Warnln("Do not use generate secrets in production. The secret will be leaked to the logs.")
return x.HashByteSecret(secret)
p.l.Error("The system secret is not configured. Please provide one in the configuration file or environment variables.")
return nil, errors.New("global secret is not configured")
}

secret := secrets[0]
if len(secret) >= 16 {
return x.HashStringSecret(secret)
if len(secret) < 16 {
p.l.Errorf("System secret must be undefined or have at least 16 characters but only has %d characters.", len(secret))
return nil, errors.New("global secret is too short")
}

p.l.Fatalf("System secret must be undefined or have at least 16 characters but only has %d characters.", len(secret))
return nil
return x.HashStringSecret(secret), nil
}

var _ fosite.RotatedGlobalSecretsProvider = (*DefaultProvider)(nil)

func (p *DefaultProvider) GetRotatedGlobalSecrets(ctx context.Context) [][]byte {
func (p *DefaultProvider) GetRotatedGlobalSecrets(ctx context.Context) ([][]byte, error) {
secrets := p.getProvider(ctx).Strings(KeyGetSystemSecret)

if len(secrets) < 2 {
return nil
return nil, nil
}

var rotated [][]byte
for _, secret := range secrets[1:] {
rotated = append(rotated, x.HashStringSecret(secret))
}

return rotated
return rotated, nil
}

var _ fosite.BCryptCostProvider = (*DefaultProvider)(nil)
Expand Down
12 changes: 8 additions & 4 deletions driver/config/provider_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"context"
"fmt"
"io"
"io/ioutil"
"net/http"
"net/url"
"os"
Expand Down Expand Up @@ -312,8 +311,13 @@ func TestViperProviderValidates(t *testing.T) {
assert.Equal(t, true, c.GetEnforcePKCEForPublicClients(ctx))

// secrets
assert.Equal(t, []byte{0x64, 0x40, 0x5f, 0xd4, 0x66, 0xc9, 0x8c, 0x88, 0xa7, 0xf2, 0xcb, 0x95, 0xcd, 0x95, 0xcb, 0xa3, 0x41, 0x49, 0x8b, 0x97, 0xba, 0x9e, 0x92, 0xee, 0x4c, 0xaf, 0xe0, 0x71, 0x23, 0x28, 0xeb, 0xfc}, c.GetGlobalSecret(ctx))
assert.Equal(t, [][]uint8{[]byte("some-random-cookie-secret")}, c.GetCookieSecrets(ctx))
secret, err := c.GetGlobalSecret(ctx)
require.NoError(t, err)
assert.Equal(t, []byte{0x64, 0x40, 0x5f, 0xd4, 0x66, 0xc9, 0x8c, 0x88, 0xa7, 0xf2, 0xcb, 0x95, 0xcd, 0x95, 0xcb, 0xa3, 0x41, 0x49, 0x8b, 0x97, 0xba, 0x9e, 0x92, 0xee, 0x4c, 0xaf, 0xe0, 0x71, 0x23, 0x28, 0xeb, 0xfc}, secret)

cookieSecret, err := c.GetCookieSecrets(ctx)
require.NoError(t, err)
assert.Equal(t, [][]uint8{[]byte("some-random-cookie-secret")}, cookieSecret)

// profiling
assert.Equal(t, "cpu", c.Source(ctx).String("profiling"))
Expand Down Expand Up @@ -405,7 +409,7 @@ func TestCookieSecure(t *testing.T) {
func TestTokenRefreshHookURL(t *testing.T) {
ctx := context.Background()
l := logrusx.New("", "")
l.Logrus().SetOutput(ioutil.Discard)
l.Logrus().SetOutput(io.Discard)
c := MustNew(context.Background(), l, configx.SkipValidation())

assert.EqualValues(t, (*url.URL)(nil), c.TokenRefreshHookURL(ctx))
Expand Down
11 changes: 8 additions & 3 deletions driver/registry_base.go
Original file line number Diff line number Diff line change
Expand Up @@ -309,9 +309,14 @@ func (m *RegistryBase) KeyCipher() *jwk.AEAD {
return m.kc
}

func (m *RegistryBase) CookieStore(ctx context.Context) sessions.Store {
func (m *RegistryBase) CookieStore(ctx context.Context) (sessions.Store, error) {
var keys [][]byte
for _, k := range m.conf.GetCookieSecrets(ctx) {
secrets, err := m.conf.GetCookieSecrets(ctx)
if err != nil {
return nil, err
}

for _, k := range secrets {
encrypt := sha256.Sum256(k)
keys = append(keys, k, encrypt[:])
}
Expand All @@ -335,7 +340,7 @@ func (m *RegistryBase) CookieStore(ctx context.Context) sessions.Store {
cs.Options.SameSite = sameSite
}

return cs
return cs, nil
}

func (m *RegistryBase) HTTPClient(ctx context.Context, opts ...httpx.ResilientOptions) *retryablehttp.Client {
Expand Down
8 changes: 6 additions & 2 deletions driver/registry_base_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import (
"io"
"testing"

"github.com/ory/x/randx"

"github.com/stretchr/testify/require"

"github.com/ory/x/httpx"
Expand Down Expand Up @@ -79,9 +81,11 @@ func TestRegistryBase_CookieStore_MaxAgeZero(t *testing.T) {

ctx := context.Background()
r := new(RegistryBase)
r.WithConfig(config.MustNew(context.Background(), logrusx.New("", "")))
r.WithConfig(config.MustNew(context.Background(), logrusx.New("", ""), configx.WithValue(config.KeyGetSystemSecret, []string{randx.MustString(32, randx.AlphaNum)})))

cs := r.CookieStore(ctx).(*sessions.CookieStore)
s, err := r.CookieStore(ctx)
require.NoError(t, err)
cs := s.(*sessions.CookieStore)

assert.Equal(t, cs.Options.MaxAge, 0)
}
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ require (
github.com/mohae/deepcopy v0.0.0-20170929034955-c48cc78d4826
github.com/oleiade/reflections v1.0.1
github.com/ory/analytics-go/v4 v4.0.3
github.com/ory/fosite v0.42.3-0.20220801115804-c557908b0db2
github.com/ory/fosite v0.44.0
github.com/ory/go-acc v0.2.8
github.com/ory/graceful v0.1.1
github.com/ory/herodot v0.9.13
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -856,8 +856,8 @@ github.com/ory/analytics-go/v4 v4.0.3 h1:2zNBQLlm3UiD8U7DdUGLLUBm62ZA5GtbEJ3S5U+
github.com/ory/analytics-go/v4 v4.0.3/go.mod h1:A3Chm/3TmM8jw4nqRss+gFhAYHRI5j/HFYH3C1FRahU=
github.com/ory/dockertest/v3 v3.9.1 h1:v4dkG+dlu76goxMiTT2j8zV7s4oPPEppKT8K8p2f1kY=
github.com/ory/dockertest/v3 v3.9.1/go.mod h1:42Ir9hmvaAPm0Mgibk6mBPi7SFvTXxEcnztDYOJ//uM=
github.com/ory/fosite v0.42.3-0.20220801115804-c557908b0db2 h1:yH0piGEbRn7+oe9xEGIB4gvuDoLi7LW+eVIR8Q8PVtY=
github.com/ory/fosite v0.42.3-0.20220801115804-c557908b0db2/go.mod h1:BTd8+oG1mRtezZbQq0S4D2HBc815bedZHjjs2KRs39Y=
github.com/ory/fosite v0.44.0 h1:Z3UjyO11/wlIoa3BotOqcTkfm7kUNA8F7dd8mOMfx0o=
github.com/ory/fosite v0.44.0/go.mod h1:o/G4kAeNn65l6MCod2+KmFfU6JQBSojS7eXys6lKGzM=
github.com/ory/go-acc v0.2.6/go.mod h1:4Kb/UnPcT8qRAk3IAxta+hvVapdxTLWtrr7bFLlEgpw=
github.com/ory/go-acc v0.2.8 h1:rOHHAPQjf0u7eHFGWpiXK+gIu/e0GRSJNr9pDukdNC4=
github.com/ory/go-acc v0.2.8/go.mod h1:iCRZUdGb/7nqvSn8xWZkhfVrtXRZ9Wru2E5rabCjFPI=
Expand Down
24 changes: 22 additions & 2 deletions jwk/aead.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,17 @@ func aeadKey(key []byte) *[32]byte {
}

func (c *AEAD) Encrypt(ctx context.Context, plaintext []byte) (string, error) {
keys := append([][]byte{c.c.GetGlobalSecret(ctx)}, c.c.GetRotatedGlobalSecrets(ctx)...)
global, err := c.c.GetGlobalSecret(ctx)
if err != nil {
return "", err
}

rotated, err := c.c.GetRotatedGlobalSecrets(ctx)
if err != nil {
return "", err
}

keys := append([][]byte{global}, rotated...)
if len(keys) == 0 {
return "", errors.Errorf("at least one encryption key must be defined but none were")
}
Expand All @@ -48,7 +58,17 @@ func (c *AEAD) Encrypt(ctx context.Context, plaintext []byte) (string, error) {
}

func (c *AEAD) Decrypt(ctx context.Context, ciphertext string) (p []byte, err error) {
keys := append([][]byte{c.c.GetGlobalSecret(ctx)}, c.c.GetRotatedGlobalSecrets(ctx)...)
global, err := c.c.GetGlobalSecret(ctx)
if err != nil {
return nil, err
}

rotated, err := c.c.GetRotatedGlobalSecrets(ctx)
if err != nil {
return nil, err
}

keys := append([][]byte{global}, rotated...)
if len(keys) == 0 {
return nil, errors.Errorf("at least one decryption key must be defined but none were")
}
Expand Down
2 changes: 1 addition & 1 deletion x/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ type RegistryWriter interface {
}

type RegistryCookieStore interface {
CookieStore(ctx context.Context) sessions.Store
CookieStore(ctx context.Context) (sessions.Store, error)
}

type TracingProvider interface {
Expand Down

0 comments on commit c5fe043

Please sign in to comment.