diff --git a/auth/auth.go b/auth/auth.go index 5d5886923..0130968eb 100644 --- a/auth/auth.go +++ b/auth/auth.go @@ -2,6 +2,9 @@ package auth import ( "context" + "crypto/rsa" + "crypto/tls" + "crypto/x509" "embed" "fmt" "html/template" @@ -240,31 +243,37 @@ func New(ctx context.Context, log *zap.Logger, s store.Storer, opt options.AuthO // LoadSamlService takes care of certificate preloading, fetching of the // IDP metadata once the auth settings are loaded and registers the // SAML middleware -func (svc *service) LoadSamlService(ctx context.Context, s *settings.Settings) (srvc *saml.SamlSPService, err error) { - links := handlers.GetLinks() +func (svc *service) LoadSamlService(ctx context.Context, s *settings.Settings) (*saml.SamlSPService, error) { + var ( + log = svc.log.Named("saml") + links = handlers.GetLinks() + keyPair tls.Certificate + err error + ) - certManager := saml.NewCertManager(&saml.CertStoreLoader{Storer: svc.store}) + if keyPair, err = tls.X509KeyPair([]byte(s.Saml.Cert), []byte(s.Saml.Key)); err != nil { + return nil, err + } - cert, err := certManager.Parse([]byte(s.Saml.Cert), []byte(s.Saml.Key)) - if err != nil { - return + if keyPair.Leaf, err = x509.ParseCertificate(keyPair.Certificate[0]); err != nil { + return nil, err } idpUrl, err := url.Parse(s.Saml.IDP.URL) if err != nil { - return + return nil, err } // idp metadata needs to be loaded before // the internal samlsp package md, err := saml.FetchIDPMetadata(ctx, *idpUrl) if err != nil { - return + return nil, err } ru, err := url.Parse(svc.opt.BaseURL) if err != nil { - return + return nil, err } rootURL := &url.URL{ @@ -273,7 +282,7 @@ func (svc *service) LoadSamlService(ctx context.Context, s *settings.Settings) ( Host: ru.Host, } - srvc, err = saml.NewSamlSPService(saml.SamlSPArgs{ + return saml.NewSamlSPService(log, saml.SamlSPArgs{ Enabled: s.Saml.Enabled, AcsURL: links.SamlCallback, @@ -283,8 +292,9 @@ func (svc *service) LoadSamlService(ctx context.Context, s *settings.Settings) ( IdpURL: *idpUrl, Host: *rootURL, - Cert: cert, - IdpMeta: md, + Certificate: keyPair.Leaf, + PrivateKey: keyPair.PrivateKey.(*rsa.PrivateKey), + IdpMeta: md, IdentityPayload: saml.IdpIdentityPayload{ Name: s.Saml.IDP.IdentName, @@ -292,8 +302,6 @@ func (svc *service) LoadSamlService(ctx context.Context, s *settings.Settings) ( Identifier: s.Saml.IDP.IdentIdentifier, }, }) - - return } func (svc *service) UpdateSettings(s *settings.Settings) { @@ -332,6 +340,7 @@ func (svc *service) UpdateSettings(s *settings.Settings) { err error ) + log.Debug("setting updated") switch true { case s.Saml.Cert == "", s.Saml.Key == "": log.Warn("certificate private/public keys empty (see 'auth.external.saml' settings)") @@ -350,8 +359,8 @@ func (svc *service) UpdateSettings(s *settings.Settings) { } if ss != nil { - log.Debug("settings changed, reloading") - svc.handlers.SamlSPService = *ss + log.Info("reloading service") + svc.handlers.SamlSPService = ss } } diff --git a/auth/external/auth_handler.go b/auth/external/auth_handler.go index 1947a4c4e..8837440f6 100644 --- a/auth/external/auth_handler.go +++ b/auth/external/auth_handler.go @@ -16,7 +16,7 @@ type ( } externalSamlAuthHandler struct { - service saml.SamlSPService + service *saml.SamlSPService } externalDefaultAuthHandler struct{} @@ -51,7 +51,7 @@ func (eh *externalSamlAuthHandler) CompleteUserAuth(w http.ResponseWriter, r *ht } if session != nil { - sess := (session.(samlsp.JWTSessionClaims)) + sess := session.(samlsp.JWTSessionClaims) if sess.StandardClaims.Valid() != nil { return nil, sess.StandardClaims.Valid() @@ -79,7 +79,7 @@ func (eh *externalSamlAuthHandler) CompleteUserAuth(w http.ResponseWriter, r *ht return } -func NewSamlExternalHandler(s saml.SamlSPService) *externalSamlAuthHandler { +func NewSamlExternalHandler(s *saml.SamlSPService) *externalSamlAuthHandler { return &externalSamlAuthHandler{ service: s, } diff --git a/auth/handlers/handle_saml.go b/auth/handlers/handle_saml.go index 56b280141..e9e9e4021 100644 --- a/auth/handlers/handle_saml.go +++ b/auth/handlers/handle_saml.go @@ -11,7 +11,7 @@ func (h AuthHandlers) samlInit(w http.ResponseWriter, r *http.Request) { r = copyProviderToContext(r) h.Log.Info("starting SAML authentication flow") - if h.SamlSPService.Handler() == nil { + if h.SamlSPService == nil || h.SamlSPService.Handler() == nil { h.Log.Error("SAML service not initialized") w.WriteHeader(http.StatusServiceUnavailable) return diff --git a/auth/handlers/handler.go b/auth/handlers/handler.go index 61bc741fb..258a616e0 100644 --- a/auth/handlers/handler.go +++ b/auth/handlers/handler.go @@ -108,7 +108,7 @@ type ( DefaultClient *types.AuthClient Opt options.AuthOpt Settings *settings.Settings - SamlSPService saml.SamlSPService + SamlSPService *saml.SamlSPService } handlerFn func(req *request.AuthReq) error diff --git a/auth/handlers/routes.go b/auth/handlers/routes.go index 0c4d1ff15..6acea300c 100644 --- a/auth/handlers/routes.go +++ b/auth/handlers/routes.go @@ -109,9 +109,19 @@ func (h *AuthHandlers) MountHttpRoutes(r chi.Router) { // calls to internal SAML service. r.Group(func(r chi.Router) { r.HandleFunc(tbp(l.SamlMetadata), func(rw http.ResponseWriter, r *http.Request) { + if h.SamlSPService == nil || !h.SamlSPService.Enabled { + rw.WriteHeader(http.StatusServiceUnavailable) + return + } + h.SamlSPService.ServeHTTP(rw, r) }) r.HandleFunc(tbp(l.SamlCallback), func(rw http.ResponseWriter, r *http.Request) { + if h.SamlSPService == nil || !h.SamlSPService.Enabled { + rw.WriteHeader(http.StatusServiceUnavailable) + return + } + h.SamlSPService.ServeHTTP(rw, r) }) r.HandleFunc(tbp(l.SamlInit), func(rw http.ResponseWriter, r *http.Request) { diff --git a/auth/saml/cert.go b/auth/saml/cert.go deleted file mode 100644 index cd2666d0e..000000000 --- a/auth/saml/cert.go +++ /dev/null @@ -1,122 +0,0 @@ -package saml - -import ( - "context" - "crypto/tls" - "fmt" - "io/ioutil" - - "github.com/cortezaproject/corteza-server/system/types" -) - -const settingsPrefix = "auth.external.providers.saml" - -type ( - Cert struct { - Cert []byte - Key []byte - } - - certManager struct { - loader CertLoader - } - - CertLoader interface { - Load(ctx context.Context) (*Cert, error) - Key(ctx context.Context) ([]byte, error) - Cert(ctx context.Context) ([]byte, error) - } - - CertStoreLoader struct { - Storer storer - } - - CertFsLoader struct { - KeyFile string - CertFile string - } - - storer interface { - SearchSettings(ctx context.Context, f types.SettingsFilter) (types.SettingValueSet, types.SettingsFilter, error) - } -) - -func (cm *certManager) Load(ctx context.Context) (cc *Cert, err error) { - return cm.loader.Load(ctx) -} - -func (cm *certManager) Parse(cert []byte, key []byte) (tls.Certificate, error) { - return tls.X509KeyPair(cert, key) -} - -func (l *CertStoreLoader) Cert(ctx context.Context) (c []byte, err error) { - return l.get(ctx, "cert") -} - -func (l *CertStoreLoader) Key(ctx context.Context) ([]byte, error) { - return l.get(ctx, "key") -} - -func (l *CertStoreLoader) get(ctx context.Context, key string) (c []byte, err error) { - var s types.SettingValueSet - - if s, _, err = l.Storer.SearchSettings(ctx, types.SettingsFilter{Prefix: settingsPrefix}); err != nil { - return - } - - if cc := s.First(fmt.Sprintf("%s.%s", settingsPrefix, key)); cc != nil { - return []byte(cc.String()), nil - } - - return -} - -func (l *CertStoreLoader) Load(ctx context.Context) (cc *Cert, err error) { - var ( - c, k []byte - ) - - if c, err = l.Cert(ctx); err != nil && c != nil { - return - } - - if k, err = l.Key(ctx); err != nil && k != nil { - return - } - - cc = &Cert{Cert: c, Key: k} - - return -} - -func (l *CertFsLoader) Cert(ctx context.Context) (c []byte, err error) { - return ioutil.ReadFile(l.CertFile) -} - -func (l *CertFsLoader) Key(ctx context.Context) ([]byte, error) { - return ioutil.ReadFile(l.KeyFile) -} - -func (l *CertFsLoader) Load(ctx context.Context) (cc *Cert, err error) { - var ( - c, k []byte - ) - - if c, err = l.Cert(ctx); err != nil && c != nil { - return - } - - if k, err = l.Key(ctx); err != nil && k != nil { - return - } - - cc = &Cert{Cert: c, Key: k} - - return -} - -func NewCertManager(l CertLoader) *certManager { - return &certManager{ - loader: l, - } -} diff --git a/auth/saml/cert_test.go b/auth/saml/cert_test.go deleted file mode 100644 index f6e133e74..000000000 --- a/auth/saml/cert_test.go +++ /dev/null @@ -1,90 +0,0 @@ -package saml - -import ( - "context" - "errors" - "fmt" - "testing" - - "github.com/cortezaproject/corteza-server/system/types" - "github.com/stretchr/testify/require" -) - -type ( - ssFn func(ctx context.Context, f types.SettingsFilter) (types.SettingValueSet, types.SettingsFilter, error) - certMockLoader struct { - } - - mockStorer struct { - ss func(ctx context.Context, f types.SettingsFilter) (types.SettingValueSet, types.SettingsFilter, error) - } -) - -var ( - c = &types.SettingValue{Name: fmt.Sprintf("%s.cert", settingsPrefix)} - k = &types.SettingValue{Name: fmt.Sprintf("%s.key", settingsPrefix)} -) - -func Test_loadCertFromSettings(t *testing.T) { - - tcc := []struct { - name string - err error - expect *Cert - ss ssFn - }{ - { - name: "cert, key load success", - expect: &Cert{Cert: []byte("CERT"), Key: []byte("KEY")}, - ss: func(ctx context.Context, f types.SettingsFilter) (types.SettingValueSet, types.SettingsFilter, error) { - c.SetValue("CERT") - k.SetValue("KEY") - - return types.SettingValueSet{c, k}, f, nil - }, - }, - { - name: "no permissions to fetch settings", - err: errors.New("no permissions"), - expect: &Cert{}, - ss: func(ctx context.Context, f types.SettingsFilter) (types.SettingValueSet, types.SettingsFilter, error) { - return nil, types.SettingsFilter{}, errors.New("no permissions") - }, - }, - { - name: "no cert, key in store", - expect: &Cert{}, - ss: func(ctx context.Context, f types.SettingsFilter) (types.SettingValueSet, types.SettingsFilter, error) { - return nil, types.SettingsFilter{}, nil - }, - }, - } - - for _, tc := range tcc { - t.Run(tc.name, func(t *testing.T) { - var ( - req = require.New(t) - loader = CertStoreLoader{ - Storer: mockStorer{ - ss: tc.ss, - }, - } - ) - - cc, err := loader.Load(context.Background()) - - if tc.err != nil { - req.Error(err) - } else { - req.NoError(err) - } - - req.Equal(tc.expect, cc) - }) - } - -} - -func (cm mockStorer) SearchSettings(ctx context.Context, f types.SettingsFilter) (types.SettingValueSet, types.SettingsFilter, error) { - return cm.ss(ctx, f) -} diff --git a/auth/saml/sp.go b/auth/saml/sp.go index 25d993c2e..3a8a08ba6 100644 --- a/auth/saml/sp.go +++ b/auth/saml/sp.go @@ -2,7 +2,6 @@ package saml import ( "crypto/rsa" - "crypto/tls" "crypto/x509" "net/http" "net/url" @@ -11,12 +10,15 @@ import ( "github.com/crewjam/saml" "github.com/crewjam/saml/samlsp" "github.com/pkg/errors" + "go.uber.org/zap" ) const defaultNameIdentifier = "urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress" type ( SamlSPService struct { + log *zap.Logger + Enabled bool IdpURL url.URL @@ -38,32 +40,24 @@ type ( // user meta from idp IdentityPayload IdpIdentityPayload - IdpURL url.URL - Host url.URL - Cert tls.Certificate - IdpMeta *saml.EntityDescriptor + IdpURL url.URL + Host url.URL + Certificate *x509.Certificate + PrivateKey *rsa.PrivateKey + IdpMeta *saml.EntityDescriptor } ) // NewSamlSPService loads the certificates and registers the // already fetched IDP metadata into the SAML middleware -func NewSamlSPService(args SamlSPArgs) (s *SamlSPService, err error) { - var ( - keyPair = args.Cert - ) - - keyPair.Leaf, err = x509.ParseCertificate(keyPair.Certificate[0]) - if err != nil { - return - } - +func NewSamlSPService(log *zap.Logger, args SamlSPArgs) (s *SamlSPService, err error) { metadataURL, _ := url.Parse(args.MetaURL) acsURL, _ := url.Parse(args.AcsURL) logoutURL, _ := url.Parse(args.SloURL) sp := saml.ServiceProvider{ - Key: keyPair.PrivateKey.(*rsa.PrivateKey), - Certificate: keyPair.Leaf, + Key: args.PrivateKey, + Certificate: args.Certificate, IDPMetadata: args.IdpMeta, MetadataURL: *args.Host.ResolveReference(metadataURL), @@ -89,6 +83,8 @@ func NewSamlSPService(args SamlSPArgs) (s *SamlSPService, err error) { handler.ServiceProvider = sp s = &SamlSPService{ + log: log, + Enabled: args.Enabled, sp: sp, @@ -133,8 +129,8 @@ func (ssp *SamlSPService) Handler() *samlsp.Middleware { // ServeHTTP enables us to use the service directly // in the router -func (ssp SamlSPService) ServeHTTP(w http.ResponseWriter, r *http.Request) { - if ssp.handler == nil || !ssp.Enabled { +func (ssp *SamlSPService) ServeHTTP(w http.ResponseWriter, r *http.Request) { + if ssp.handler == nil { w.WriteHeader(http.StatusServiceUnavailable) return } diff --git a/system/service/settings.go b/system/service/settings.go index a87551318..ccf4e1877 100644 --- a/system/service/settings.go +++ b/system/service/settings.go @@ -94,15 +94,21 @@ func (svc *settings) notify(ctx context.Context, vv types.SettingValueSet) { defer svc.m.RUnlock() for _, l := range svc.listeners { - go func(l registeredSettingsListener) { + go func(l registeredSettingsListener, vv types.SettingValueSet) { if len(l.prefix) > 0 { vv = vv.FilterByPrefix(l.prefix) } + svc.logger.Debug( + "notifying listener", + zap.String("prefix", l.prefix), + zap.Int("changes", len(vv)), + ) + if len(vv) > 0 { l.fn(ctx, svc.current, vv) } - }(l) + }(l, vv) } } diff --git a/tests/system/auth_external_saml_success_test.go b/tests/system/auth_external_saml_success_test.go index de7116f9f..de7061c8a 100644 --- a/tests/system/auth_external_saml_success_test.go +++ b/tests/system/auth_external_saml_success_test.go @@ -2,6 +2,9 @@ package system import ( "context" + "crypto/rsa" + "crypto/tls" + "crypto/x509" "net/http" "net/url" "testing" @@ -13,21 +16,23 @@ import ( "github.com/cortezaproject/corteza-server/system/types" s "github.com/crewjam/saml" "github.com/crewjam/saml/samlsp" - jwt "github.com/dgrijalva/jwt-go" + "github.com/dgrijalva/jwt-go" "github.com/steinfletcher/apitest" + "go.uber.org/zap" ) -func loadSAMLService(ctx context.Context) (srvc *saml.SamlSPService, err error) { - links := handlers.GetLinks() +func loadSAMLService() (srvc *saml.SamlSPService, err error) { + var ( + links = handlers.GetLinks() + keyPair tls.Certificate + ) - certManager := saml.NewCertManager(&saml.CertStoreLoader{}) + if keyPair, err = tls.X509KeyPair(readStaticFile("static/spCert.cert"), readStaticFile("static/spCert.key")); err != nil { + return nil, err + } - cert, err := certManager.Parse( - readStaticFile("static/spCert.cert"), - readStaticFile("static/spCert.key")) - - if err != nil { - return + if keyPair.Leaf, err = x509.ParseCertificate(keyPair.Certificate[0]); err != nil { + return nil, err } idpUrl, err := url.Parse("") @@ -50,7 +55,7 @@ func loadSAMLService(ctx context.Context) (srvc *saml.SamlSPService, err error) return } - srvc, err = saml.NewSamlSPService(saml.SamlSPArgs{ + srvc, err = saml.NewSamlSPService(zap.NewNop(), saml.SamlSPArgs{ Enabled: true, AcsURL: links.SamlCallback, @@ -60,7 +65,9 @@ func loadSAMLService(ctx context.Context) (srvc *saml.SamlSPService, err error) IdpURL: *idpUrl, Host: *rootURL, - Cert: cert, + Certificate: keyPair.Leaf, + PrivateKey: keyPair.PrivateKey.(*rsa.PrivateKey), + IdpMeta: md, }) diff --git a/tests/system/main_test.go b/tests/system/main_test.go index ef8e8a0a3..d3c5a8d85 100644 --- a/tests/system/main_test.go +++ b/tests/system/main_test.go @@ -96,10 +96,10 @@ func InitTestApp() { }) } - sp, _ := loadSAMLService(context.Background()) + sp, _ := loadSAMLService() hh = &handlers.AuthHandlers{ - SamlSPService: *sp, + SamlSPService: sp, Log: zap.NewNop(), AuthService: service.DefaultAuth, SessionManager: sm,