Skip to content

Commit f6dd51c

Browse files
andaaronandaaron
committed
fix: make config read/write thread safe
1. The config config has a lock - covers most cases 2. The extensions config has a lock - covers cases for extensions (it is referenced in structs independently of the main config) 3. The access control config has a lock - covers cases for access control (it is referenced in structs independently of main config) 4. The auth config is a special case it doesn't have lock, we use a copy, as the config object may mutate in the middle of an auth request, and using that may result in incompatible partial configuration being applied for that request. 5. Fix an issue with the monitoring server not stopping when the controller is shut down. 6. Fix an issue with the HTPasswdWatcher not stopping when the background tasks are supposed to finish. Moved some of the methods which were on the main config to the auth, access control and extension configs Signed-off-by: Andrei Aaron <[email protected]>
1 parent 3dcd7d1 commit f6dd51c

28 files changed

+1326
-452
lines changed

‎pkg/api/authn.go‎

Lines changed: 60 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,8 @@ func AuthHandler(ctlr *Controller) mux.MiddlewareFunc {
5555
log: ctlr.Log,
5656
}
5757

58-
if ctlr.Config.IsBearerAuthEnabled() {
58+
authConfig := ctlr.Config.GetAuthConfig()
59+
if authConfig.IsBearerAuthEnabled() {
5960
return bearerAuthHandler(ctlr)
6061
}
6162

@@ -103,6 +104,12 @@ func (amw *AuthnMiddleware) basicAuthn(ctlr *Controller, userAc *reqCtx.UserAcce
103104
) (bool, error) {
104105
cookieStore := ctlr.CookieStore
105106

107+
// Get auth config once to avoid multiple calls
108+
authConfig := ctlr.Config.GetAuthConfig()
109+
if authConfig == nil {
110+
return false, nil
111+
}
112+
106113
identity, passphrase, err := getUsernamePasswordBasicAuth(request)
107114
if err != nil {
108115
ctlr.Log.Error().Err(err).Msg("failed to parse authorization header")
@@ -116,7 +123,8 @@ func (amw *AuthnMiddleware) basicAuthn(ctlr *Controller, userAc *reqCtx.UserAcce
116123
// Process request
117124
var groups []string
118125

119-
if ctlr.Config.HTTP.AccessControl != nil {
126+
accessControl := ctlr.Config.GetAccessControlConfig()
127+
if accessControl != nil {
120128
ac := NewAccessController(ctlr.Config)
121129
groups = ac.getUserGroups(identity)
122130
}
@@ -145,13 +153,14 @@ func (amw *AuthnMiddleware) basicAuthn(ctlr *Controller, userAc *reqCtx.UserAcce
145153
}
146154

147155
// next, LDAP if configured (network-based which can lose connectivity)
148-
if ctlr.Config.HTTP.Auth != nil && ctlr.Config.HTTP.Auth.LDAP != nil {
156+
if authConfig.IsLdapAuthEnabled() {
149157
ok, _, ldapgroups, err := amw.ldapClient.Authenticate(identity, passphrase)
150158
if ok && err == nil {
151159
// Process request
152160
var groups []string
153161

154-
if ctlr.Config.HTTP.AccessControl != nil {
162+
accessControl := ctlr.Config.GetAccessControlConfig()
163+
if accessControl != nil {
155164
ac := NewAccessController(ctlr.Config)
156165
groups = ac.getUserGroups(identity)
157166
}
@@ -181,7 +190,7 @@ func (amw *AuthnMiddleware) basicAuthn(ctlr *Controller, userAc *reqCtx.UserAcce
181190
}
182191

183192
// last try API keys
184-
if ctlr.Config.IsAPIKeyEnabled() {
193+
if authConfig.IsAPIKeyEnabled() {
185194
apiKey := passphrase
186195

187196
if !strings.HasPrefix(apiKey, constants.APIKeysPrefix) {
@@ -248,16 +257,22 @@ func (amw *AuthnMiddleware) basicAuthn(ctlr *Controller, userAc *reqCtx.UserAcce
248257
}
249258

250259
func (amw *AuthnMiddleware) tryAuthnHandlers(ctlr *Controller) mux.MiddlewareFunc { //nolint: gocyclo
260+
// Get auth config once to avoid multiple calls
261+
authConfig := ctlr.Config.GetAuthConfig()
262+
if authConfig == nil {
263+
return noPasswdAuth(ctlr)
264+
}
265+
251266
// no password based authN, if neither LDAP nor HTTP BASIC is enabled
252-
if !ctlr.Config.IsBasicAuthnEnabled() {
267+
if !authConfig.IsBasicAuthnEnabled() {
253268
return noPasswdAuth(ctlr)
254269
}
255270

256-
delay := ctlr.Config.HTTP.Auth.FailDelay
271+
delay := authConfig.GetFailDelay()
257272

258273
// ldap and htpasswd based authN
259-
if ctlr.Config.IsLdapAuthEnabled() {
260-
ldapConfig := ctlr.Config.HTTP.Auth.LDAP
274+
if authConfig.IsLdapAuthEnabled() {
275+
ldapConfig := authConfig.LDAP
261276

262277
ctlr.LDAPClient = &LDAPClient{
263278
Host: ldapConfig.Address,
@@ -278,17 +293,17 @@ func (amw *AuthnMiddleware) tryAuthnHandlers(ctlr *Controller) mux.MiddlewareFun
278293

279294
amw.ldapClient = ctlr.LDAPClient
280295

281-
if ctlr.Config.HTTP.Auth.LDAP.CACert != "" {
282-
caCert, err := os.ReadFile(ctlr.Config.HTTP.Auth.LDAP.CACert)
296+
if authConfig.LDAP.CACert != "" {
297+
caCert, err := os.ReadFile(authConfig.LDAP.CACert)
283298
if err != nil {
284-
amw.log.Panic().Err(err).Str("caCert", ctlr.Config.HTTP.Auth.LDAP.CACert).
299+
amw.log.Panic().Err(err).Str("caCert", authConfig.LDAP.CACert).
285300
Msg("failed to read caCert")
286301
}
287302

288303
caCertPool := x509.NewCertPool()
289304

290305
if !caCertPool.AppendCertsFromPEM(caCert) {
291-
amw.log.Panic().Err(zerr.ErrBadCACert).Str("caCert", ctlr.Config.HTTP.Auth.LDAP.CACert).
306+
amw.log.Panic().Err(zerr.ErrBadCACert).Str("caCert", authConfig.LDAP.CACert).
292307
Msg("failed to read caCert")
293308
}
294309

@@ -297,34 +312,34 @@ func (amw *AuthnMiddleware) tryAuthnHandlers(ctlr *Controller) mux.MiddlewareFun
297312
// default to system cert pool
298313
caCertPool, err := x509.SystemCertPool()
299314
if err != nil {
300-
amw.log.Panic().Err(zerr.ErrBadCACert).Str("caCert", ctlr.Config.HTTP.Auth.LDAP.CACert).
315+
amw.log.Panic().Err(zerr.ErrBadCACert).Str("caCert", authConfig.LDAP.CACert).
301316
Msg("failed to get system cert pool")
302317
}
303318

304319
amw.ldapClient.ClientCAs = caCertPool
305320
}
306321
}
307322

308-
if ctlr.Config.IsHtpasswdAuthEnabled() {
309-
err := amw.htpasswd.Reload(ctlr.Config.HTTP.Auth.HTPasswd.Path)
323+
if authConfig.IsHtpasswdAuthEnabled() {
324+
err := amw.htpasswd.Reload(authConfig.HTPasswd.Path)
310325
if err != nil {
311-
amw.log.Panic().Err(err).Str("credsFile", ctlr.Config.HTTP.Auth.HTPasswd.Path).
326+
amw.log.Panic().Err(err).Str("credsFile", authConfig.HTPasswd.Path).
312327
Msg("failed to open creds-file")
313328
}
314329
}
315330

316331
// openid based authN
317-
if ctlr.Config.IsOpenIDAuthEnabled() {
332+
if authConfig.IsOpenIDAuthEnabled() {
318333
ctlr.RelyingParties = make(map[string]rp.RelyingParty)
319334

320-
for provider := range ctlr.Config.HTTP.Auth.OpenID.Providers {
335+
for provider := range authConfig.OpenID.Providers {
321336
if config.IsOpenIDSupported(provider) {
322-
rp := NewRelyingPartyOIDC(context.TODO(), ctlr.Config, provider, ctlr.Config.HTTP.Auth.SessionHashKey,
323-
ctlr.Config.HTTP.Auth.SessionEncryptKey, ctlr.Log)
337+
rp := NewRelyingPartyOIDC(context.TODO(), ctlr.Config, provider, authConfig.SessionHashKey,
338+
authConfig.SessionEncryptKey, ctlr.Log)
324339
ctlr.RelyingParties[provider] = rp
325340
} else if config.IsOauth2Supported(provider) {
326-
rp := NewRelyingPartyGithub(ctlr.Config, provider, ctlr.Config.HTTP.Auth.SessionHashKey,
327-
ctlr.Config.HTTP.Auth.SessionEncryptKey, ctlr.Log)
341+
rp := NewRelyingPartyGithub(ctlr.Config, provider, authConfig.SessionHashKey,
342+
authConfig.SessionEncryptKey, ctlr.Log)
328343
ctlr.RelyingParties[provider] = rp
329344
}
330345
}
@@ -340,7 +355,10 @@ func (amw *AuthnMiddleware) tryAuthnHandlers(ctlr *Controller) mux.MiddlewareFun
340355
}
341356

342357
isMgmtRequested := request.RequestURI == constants.FullMgmt
343-
allowAnonymous := ctlr.Config.HTTP.AccessControl.AnonymousPolicyExists()
358+
359+
// Get access control config safely
360+
accessControlConfig := ctlr.Config.GetAccessControlConfig()
361+
allowAnonymous := accessControlConfig != nil && accessControlConfig.AnonymousPolicyExists()
344362

345363
// build user access control info
346364
userAc := reqCtx.NewUserAccessControl()
@@ -370,7 +388,9 @@ func (amw *AuthnMiddleware) tryAuthnHandlers(ctlr *Controller) mux.MiddlewareFun
370388
if errors.Is(err, zerr.ErrUserDataNotFound) {
371389
ctlr.Log.Err(err).Msg("failed to find user profile in DB")
372390

373-
authFail(response, request, ctlr.Config.HTTP.Realm, delay)
391+
// Get HTTP config safely for realm
392+
httpConfig := ctlr.Config.GetHTTPConfig()
393+
authFail(response, request, httpConfig.Realm, delay)
374394
}
375395

376396
response.WriteHeader(http.StatusInternalServerError)
@@ -397,22 +417,27 @@ func (amw *AuthnMiddleware) tryAuthnHandlers(ctlr *Controller) mux.MiddlewareFun
397417
return
398418
}
399419

400-
authFail(response, request, ctlr.Config.HTTP.Realm, delay)
420+
// Get HTTP config safely for realm
421+
httpConfig := ctlr.Config.GetHTTPConfig()
422+
authFail(response, request, httpConfig.Realm, delay)
401423
})
402424
}
403425
}
404426

405427
func bearerAuthHandler(ctlr *Controller) mux.MiddlewareFunc {
428+
// Get auth config safely
429+
authConfig := ctlr.Config.GetAuthConfig()
430+
406431
// although the configuration option is called 'cert', this function will also parse a public key directly
407432
// see https://github.com/project-zot/zot/issues/3173 for info
408-
publicKey, err := loadPublicKeyFromFile(ctlr.Config.HTTP.Auth.Bearer.Cert)
433+
publicKey, err := loadPublicKeyFromFile(authConfig.Bearer.Cert)
409434
if err != nil {
410435
ctlr.Log.Panic().Err(err).Msg("failed to load public key for bearer authentication")
411436
}
412437

413438
authorizer := NewBearerAuthorizer(
414-
ctlr.Config.HTTP.Auth.Bearer.Realm,
415-
ctlr.Config.HTTP.Auth.Bearer.Service,
439+
authConfig.Bearer.Realm,
440+
authConfig.Bearer.Service,
416441
publicKey,
417442
)
418443

@@ -509,7 +534,12 @@ func noPasswdAuth(ctlr *Controller) mux.MiddlewareFunc {
509534
}
510535

511536
if ctlr.Config.IsMTLSAuthEnabled() && userAc.IsAnonymous() {
512-
authFail(response, request, ctlr.Config.HTTP.Realm, ctlr.Config.HTTP.Auth.FailDelay)
537+
// Get HTTP config safely for realm and fail delay
538+
httpConfig := ctlr.Config.GetHTTPConfig()
539+
authConfig := ctlr.Config.GetAuthConfig()
540+
failDelay := authConfig.GetFailDelay()
541+
542+
authFail(response, request, httpConfig.Realm, failDelay)
513543

514544
return
515545
}

‎pkg/api/authz.go‎

Lines changed: 42 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -42,16 +42,20 @@ type AccessController struct {
4242
}
4343

4444
func NewAccessController(conf *config.Config) *AccessController {
45-
if conf.HTTP.AccessControl == nil {
45+
// Get access control config safely
46+
accessControlConfig := conf.GetAccessControlConfig()
47+
logConfig := conf.GetLogConfig()
48+
49+
if accessControlConfig == nil {
4650
return &AccessController{
4751
Config: &config.AccessControlConfig{},
48-
Log: log.NewLogger(conf.Log.Level, conf.Log.Output),
52+
Log: log.NewLogger(logConfig.Level, logConfig.Output),
4953
}
5054
}
5155

5256
return &AccessController{
53-
Config: conf.HTTP.AccessControl,
54-
Log: log.NewLogger(conf.Log.Level, conf.Log.Output),
57+
Config: accessControlConfig,
58+
Log: log.NewLogger(logConfig.Level, logConfig.Output),
5559
}
5660
}
5761

@@ -117,14 +121,17 @@ func (ac *AccessController) can(userAc *reqCtx.UserAccessControl, action, reposi
117121
username := userAc.GetUsername()
118122

119123
// check matched repo based policy
120-
pg, ok := ac.Config.Repositories[longestMatchedPattern]
124+
repositories := ac.Config.GetRepositories()
125+
pg, ok := repositories[longestMatchedPattern]
126+
121127
if ok {
122128
can = ac.isPermitted(userGroups, username, action, pg)
123129
}
124130

125131
// check admins based policy
126132
if !can {
127-
if ac.isAdmin(username, userGroups) && common.Contains(ac.Config.AdminPolicy.Actions, action) {
133+
adminPolicy := ac.Config.GetAdminPolicy()
134+
if ac.isAdmin(username, userGroups) && common.Contains(adminPolicy.Actions, action) {
128135
can = true
129136
}
130137
}
@@ -134,16 +141,18 @@ func (ac *AccessController) can(userAc *reqCtx.UserAccessControl, action, reposi
134141

135142
// isAdmin .
136143
func (ac *AccessController) isAdmin(username string, userGroups []string) bool {
137-
if common.Contains(ac.Config.AdminPolicy.Users, username) || ac.isAnyGroupInAdminPolicy(userGroups) {
144+
adminPolicy := ac.Config.GetAdminPolicy()
145+
if common.Contains(adminPolicy.Users, username) || ac.isAnyGroupInAdminPolicy(userGroups) {
138146
return true
139147
}
140148

141149
return false
142150
}
143151

144152
func (ac *AccessController) isAnyGroupInAdminPolicy(userGroups []string) bool {
153+
adminPolicy := ac.Config.GetAdminPolicy()
145154
for _, group := range userGroups {
146-
if common.Contains(ac.Config.AdminPolicy.Groups, group) {
155+
if common.Contains(adminPolicy.Groups, group) {
147156
return true
148157
}
149158
}
@@ -154,7 +163,8 @@ func (ac *AccessController) isAnyGroupInAdminPolicy(userGroups []string) bool {
154163
func (ac *AccessController) getUserGroups(username string) []string {
155164
var groupNames []string
156165

157-
for groupName, group := range ac.Config.Groups {
166+
groups := ac.Config.GetGroups()
167+
for groupName, group := range groups {
158168
for _, user := range group.Users {
159169
// find if the user is part of any groups
160170
if user == username {
@@ -241,6 +251,10 @@ func (ac *AccessController) isPermitted(userGroups []string, username, action st
241251
func BaseAuthzHandler(ctlr *Controller) mux.MiddlewareFunc {
242252
return func(next http.Handler) http.Handler {
243253
return http.HandlerFunc(func(response http.ResponseWriter, request *http.Request) {
254+
// Get configs safely
255+
authConfig := ctlr.Config.GetAuthConfig()
256+
httpConfig := ctlr.Config.GetHTTPConfig()
257+
244258
/* NOTE:
245259
since we only do READ actions in extensions, this middleware is enough for them because
246260
it populates the context with user relevant data to be processed by each individual extension
@@ -271,7 +285,7 @@ func BaseAuthzHandler(ctlr *Controller) mux.MiddlewareFunc {
271285
// get access control context made in authn.go
272286
userAc, err := reqCtx.UserAcFromContext(request.Context())
273287
if err != nil { // should never happen
274-
authFail(response, request, ctlr.Config.HTTP.Realm, ctlr.Config.HTTP.Auth.FailDelay)
288+
authFail(response, request, httpConfig.Realm, authConfig.GetFailDelay())
275289

276290
return
277291
}
@@ -287,6 +301,10 @@ func BaseAuthzHandler(ctlr *Controller) mux.MiddlewareFunc {
287301
func DistSpecAuthzHandler(ctlr *Controller) mux.MiddlewareFunc {
288302
return func(next http.Handler) http.Handler {
289303
return http.HandlerFunc(func(response http.ResponseWriter, request *http.Request) {
304+
// Get configs safely
305+
authConfig := ctlr.Config.GetAuthConfig()
306+
httpConfig := ctlr.Config.GetHTTPConfig()
307+
290308
if request.Method == http.MethodOptions {
291309
next.ServeHTTP(response, request)
292310

@@ -310,7 +328,7 @@ func DistSpecAuthzHandler(ctlr *Controller) mux.MiddlewareFunc {
310328
// get userAc built in authn and previous authz middlewares
311329
userAc, err := reqCtx.UserAcFromContext(request.Context())
312330
if err != nil { // should never happen
313-
authFail(response, request, ctlr.Config.HTTP.Realm, ctlr.Config.HTTP.Auth.FailDelay)
331+
authFail(response, request, httpConfig.Realm, authConfig.GetFailDelay())
314332

315333
return
316334
}
@@ -341,7 +359,7 @@ func DistSpecAuthzHandler(ctlr *Controller) mux.MiddlewareFunc {
341359

342360
can := acCtrlr.can(userAc, action, resource) //nolint:contextcheck
343361
if !can {
344-
common.AuthzFail(response, request, userAc.GetUsername(), ctlr.Config.HTTP.Realm, ctlr.Config.HTTP.Auth.FailDelay)
362+
common.AuthzFail(response, request, userAc.GetUsername(), httpConfig.Realm, authConfig.GetFailDelay())
345363
} else {
346364
next.ServeHTTP(response, request) //nolint:contextcheck
347365
}
@@ -352,32 +370,38 @@ func DistSpecAuthzHandler(ctlr *Controller) mux.MiddlewareFunc {
352370
func MetricsAuthzHandler(ctlr *Controller) mux.MiddlewareFunc {
353371
return func(next http.Handler) http.Handler {
354372
return http.HandlerFunc(func(response http.ResponseWriter, request *http.Request) {
355-
if ctlr.Config.HTTP.AccessControl == nil {
373+
// Get configs safely
374+
authConfig := ctlr.Config.GetAuthConfig()
375+
httpConfig := ctlr.Config.GetHTTPConfig()
376+
accessControlConfig := ctlr.Config.GetAccessControlConfig()
377+
378+
if accessControlConfig == nil {
356379
// allow access to authenticated user as anonymous policy does not exist
357380
next.ServeHTTP(response, request)
358381

359382
return
360383
}
361384

362-
if len(ctlr.Config.HTTP.AccessControl.Metrics.Users) == 0 {
385+
metricsConfig := accessControlConfig.GetMetrics()
386+
if len(metricsConfig.Users) == 0 {
363387
log := ctlr.Log
364388
log.Warn().Msg("auth is enabled but no metrics users in accessControl: /metrics is unaccesible")
365-
common.AuthzFail(response, request, "", ctlr.Config.HTTP.Realm, ctlr.Config.HTTP.Auth.FailDelay)
389+
common.AuthzFail(response, request, "", httpConfig.Realm, authConfig.GetFailDelay())
366390

367391
return
368392
}
369393

370394
// get access control context made in authn.go
371395
userAc, err := reqCtx.UserAcFromContext(request.Context())
372396
if err != nil { // should never happen
373-
common.AuthzFail(response, request, "", ctlr.Config.HTTP.Realm, ctlr.Config.HTTP.Auth.FailDelay)
397+
common.AuthzFail(response, request, "", httpConfig.Realm, authConfig.GetFailDelay())
374398

375399
return
376400
}
377401

378402
username := userAc.GetUsername()
379-
if !common.Contains(ctlr.Config.HTTP.AccessControl.Metrics.Users, username) {
380-
common.AuthzFail(response, request, username, ctlr.Config.HTTP.Realm, ctlr.Config.HTTP.Auth.FailDelay)
403+
if !common.Contains(metricsConfig.Users, username) {
404+
common.AuthzFail(response, request, username, httpConfig.Realm, authConfig.GetFailDelay())
381405

382406
return
383407
}

0 commit comments

Comments
 (0)