From d0ef5a7cd305ff8f345a5585227702cc56af6465 Mon Sep 17 00:00:00 2001 From: Haoming Meng Date: Wed, 24 Jan 2024 19:04:17 +0000 Subject: [PATCH 1/9] Add knobs to control approval req --- config/resources/defaults.yaml | 2 ++ config/resources/osdf.yaml | 3 +++ docs/parameters.yaml | 19 +++++++++++++++++++ param/parameters.go | 2 ++ param/parameters_struct.go | 4 ++++ 5 files changed, 30 insertions(+) diff --git a/config/resources/defaults.yaml b/config/resources/defaults.yaml index 4c0eb5fc2..77439abbe 100644 --- a/config/resources/defaults.yaml +++ b/config/resources/defaults.yaml @@ -38,6 +38,8 @@ Origin: SelfTest: true Registry: InstitutionsUrlReloadMinutes: 15m + CacheApprovedOnly: false + OriginApprovedOnly: false Monitoring: PortLower: 9930 PortHigher: 9999 diff --git a/config/resources/osdf.yaml b/config/resources/osdf.yaml index 528145787..cb2304c21 100644 --- a/config/resources/osdf.yaml +++ b/config/resources/osdf.yaml @@ -22,3 +22,6 @@ Federation: DiscoveryUrl: osg-htc.org TopologyNamespaceURL: https://topology.opensciencegrid.org/osdf/namespaces TopologyReloadInterval: 10 +Registry: + CacheApprovedOnly: true + OriginApprovedOnly: true diff --git a/docs/parameters.yaml b/docs/parameters.yaml index 486a1d47a..2f5a053fc 100644 --- a/docs/parameters.yaml +++ b/docs/parameters.yaml @@ -662,6 +662,25 @@ type: duration default: 15m components: ["nsregistry"] --- +name: Registry.CacheApprovedOnly +description: >- + Only allow approved caches to join the federation and serve files. If set to true, caches can + successfully self-register or registered via registry, but director won't direct traffic to the cache. +type: bool +default: false +osdf_default: true +components: ["nsregistry"] +--- +name: Registry.OriginApprovedOnly +description: >- + Only allow approved origins to join the federation and serve files. If set to true, origins can + successfully self-register or registered via registry, but director won't direct traffic to the origin, + nor would files on the origin show up in the federation. +type: bool +default: false +osdf_default: true +components: ["nsregistry"] +--- ############################ # Server-level configs # ############################ diff --git a/param/parameters.go b/param/parameters.go index 0fc1d6aba..c511a0496 100644 --- a/param/parameters.go +++ b/param/parameters.go @@ -180,6 +180,8 @@ var ( Origin_Multiuser = BoolParam{"Origin.Multiuser"} Origin_ScitokensMapSubject = BoolParam{"Origin.ScitokensMapSubject"} Origin_SelfTest = BoolParam{"Origin.SelfTest"} + Registry_CacheApprovedOnly = BoolParam{"Registry.CacheApprovedOnly"} + Registry_OriginApprovedOnly = BoolParam{"Registry.OriginApprovedOnly"} Registry_RequireKeyChaining = BoolParam{"Registry.RequireKeyChaining"} Server_EnableUI = BoolParam{"Server.EnableUI"} StagePlugin_Hook = BoolParam{"StagePlugin.Hook"} diff --git a/param/parameters_struct.go b/param/parameters_struct.go index ea066ff6c..85f4c9164 100644 --- a/param/parameters_struct.go +++ b/param/parameters_struct.go @@ -116,10 +116,12 @@ type config struct { } Registry struct { AdminUsers []string + CacheApprovedOnly bool DbLocation string Institutions interface{} InstitutionsUrl string InstitutionsUrlReloadMinutes time.Duration + OriginApprovedOnly bool RequireKeyChaining bool } Server struct { @@ -286,10 +288,12 @@ type configWithType struct { } Registry struct { AdminUsers struct { Type string; Value []string } + CacheApprovedOnly struct { Type string; Value bool } DbLocation struct { Type string; Value string } Institutions struct { Type string; Value interface{} } InstitutionsUrl struct { Type string; Value string } InstitutionsUrlReloadMinutes struct { Type string; Value time.Duration } + OriginApprovedOnly struct { Type string; Value bool } RequireKeyChaining struct { Type string; Value bool } } Server struct { From 8fa2148e9d67453ec00c18df6e0d322086162dd5 Mon Sep 17 00:00:00 2001 From: Haoming Meng Date: Wed, 24 Jan 2024 19:31:48 +0000 Subject: [PATCH 2/9] Allow configed approv req for both origins and caches --- registry/registry.go | 28 ++++--- registry/registry_db.go | 50 +++++-------- registry/registry_db_test.go | 140 ++++++++++------------------------- registry/registry_test.go | 87 +++++++++++++++++++++- 4 files changed, 162 insertions(+), 143 deletions(-) diff --git a/registry/registry.go b/registry/registry.go index be16ccbc0..b4e10945d 100644 --- a/registry/registry.go +++ b/registry/registry.go @@ -120,7 +120,7 @@ func matchKeys(incomingKey jwk.Key, registeredNamespaces []string) (bool, error) // permitting the action (assuming their keys haven't been stolen!) foundMatch := false for _, ns := range registeredNamespaces { - keyset, err := getNamespaceJwksByPrefix(ns, false) + keyset, _, err := getNamespaceJwksByPrefix(ns) if err != nil { return false, errors.Wrapf(err, "Cannot get keyset for %s from the database", ns) } @@ -559,7 +559,7 @@ func deleteNamespaceHandler(ctx *gin.Context) { delTokenStr := strings.TrimPrefix(authHeader, "Bearer ") // Have the token, now we need to load the JWKS for the prefix - originJwks, err := getNamespaceJwksByPrefix(prefix, false) + originJwks, _, err := getNamespaceJwksByPrefix(prefix) if err != nil { ctx.JSON(http.StatusInternalServerError, gin.H{"error": "server encountered an error loading the prefix's stored jwks"}) log.Errorf("Failed to get prefix's stored jwks: %v", err) @@ -666,17 +666,27 @@ func wildcardHandler(ctx *gin.Context) { return } - jwks, err := getNamespaceJwksByPrefix(prefix, true) + jwks, adminMetadata, err := getNamespaceJwksByPrefix(prefix) if err != nil { - if err == serverCredsErr { - // Use 403 to distinguish between server error - ctx.JSON(http.StatusForbidden, gin.H{"error": "cache has not been approved by federation administrator"}) - return - } ctx.JSON(http.StatusInternalServerError, gin.H{"error": "server encountered an error trying to get jwks for prefix"}) log.Errorf("Failed to load jwks for prefix %s: %v", prefix, err) return } + if adminMetadata != nil && adminMetadata.Status != Approved { + if strings.HasPrefix(prefix, "/caches/") { // Caches + if param.Registry_CacheApprovedOnly.GetBool() { + // Use 403 to distinguish between server error + ctx.JSON(http.StatusForbidden, gin.H{"error": "The cache has not been approved by federation administrator"}) + return + } + } else { // Origins + if param.Registry_OriginApprovedOnly.GetBool() { + // Use 403 to distinguish between server error + ctx.JSON(http.StatusForbidden, gin.H{"error": "The origin has not been approved by federation administrator"}) + return + } + } + } ctx.JSON(http.StatusOK, jwks) return } @@ -731,7 +741,7 @@ func checkNamespaceExistsHandler(ctx *gin.Context) { return } // Just to check if the key matches. We don't care about approval status - jwksDb, err := getNamespaceJwksByPrefix(req.Prefix, false) + jwksDb, _, err := getNamespaceJwksByPrefix(req.Prefix) if err != nil { ctx.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return diff --git a/registry/registry_db.go b/registry/registry_db.go index 354d28606..4f9c05aa7 100644 --- a/registry/registry_db.go +++ b/registry/registry_db.go @@ -335,46 +335,34 @@ func getNamespaceJwksById(id int) (jwk.Set, error) { return set, nil } -func getNamespaceJwksByPrefix(prefix string, approvalRequired bool) (jwk.Set, error) { - var jwksQuery string +func getNamespaceJwksByPrefix(prefix string) (jwk.Set, *AdminMetadata, error) { var pubkeyStr string - if strings.HasPrefix(prefix, "/caches/") && approvalRequired { - adminMetadataStr := "" - jwksQuery = `SELECT pubkey, admin_metadata FROM namespace WHERE prefix = ?` - err := db.QueryRow(jwksQuery, prefix).Scan(&pubkeyStr, &adminMetadataStr) - if err != nil { - if err == sql.ErrNoRows { - return nil, errors.New("prefix not found in database") - } - return nil, errors.Wrap(err, "error performing cache pubkey query") - } - if adminMetadataStr != "" { // Older version didn't have admin_metadata populated, skip checking - adminMetadata := AdminMetadata{} - if err = json.Unmarshal([]byte(adminMetadataStr), &adminMetadata); err != nil { - return nil, errors.Wrap(err, "Failed to unmarshall admin_metadata") - } - // TODO: Move this to upper functions that handles business logic to keep db access functions simple - if adminMetadata.Status != Approved { - return nil, serverCredsErr - } + var adminMetadataStr string + + jwksQuery := `SELECT pubkey, admin_metadata FROM namespace WHERE prefix = ?` + err := db.QueryRow(jwksQuery, prefix).Scan(&pubkeyStr, &adminMetadataStr) + if err != nil { + if err == sql.ErrNoRows { + return nil, nil, errors.New("prefix not found in database") } - } else { - jwksQuery := `SELECT pubkey FROM namespace WHERE prefix = ?` - err := db.QueryRow(jwksQuery, prefix).Scan(&pubkeyStr) - if err != nil { - if err == sql.ErrNoRows { - return nil, errors.New("prefix not found in database") - } - return nil, errors.Wrap(err, "error performing origin pubkey query") + return nil, nil, errors.Wrap(err, "error performing origin pubkey query") + } + + adminMetadata := AdminMetadata{} + + // For backward compatibility, if adminMetadata is an empty string, don't unmarshall json + if adminMetadataStr != "" { + if err := json.Unmarshal([]byte(adminMetadataStr), &adminMetadata); err != nil { + return nil, nil, errors.Wrap(err, "error parsing admin metadata") } } set, err := jwk.ParseString(pubkeyStr) if err != nil { - return nil, errors.Wrap(err, "Failed to parse pubkey as a jwks") + return nil, nil, errors.Wrap(err, "Failed to parse pubkey as a jwks") } - return set, nil + return set, &adminMetadata, nil } func getNamespaceStatusById(id int) (RegistrationStatus, error) { diff --git a/registry/registry_db_test.go b/registry/registry_db_test.go index b4e2df8c8..0a2cba09d 100644 --- a/registry/registry_db_test.go +++ b/registry/registry_db_test.go @@ -24,6 +24,7 @@ import ( "testing" "time" + "github.com/lestrrat-go/jwx/v2/jwk" "github.com/pkg/errors" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -245,7 +246,6 @@ func TestGetNamespaceStatusById(t *testing.T) { t.Run("db-query-error", func(t *testing.T) { resetNamespaceDB(t) - // Simulate a DB error. You need to mock the db.QueryRow function to return an error _, err := getNamespaceStatusById(1) require.Error(t, err) }) @@ -700,6 +700,43 @@ func TestGetNamespacesByFilter(t *testing.T) { }) } +func TestGetNamespaceJwksByPrefix(t *testing.T) { + setupMockRegistryDB(t) + defer teardownMockNamespaceDB(t) + + t.Run("db-query-error", func(t *testing.T) { + resetNamespaceDB(t) + _, _, err := getNamespaceJwksByPrefix("/") + require.Error(t, err) + }) + + t.Run("valid-prefix-empty-admin-metadata", func(t *testing.T) { + resetNamespaceDB(t) + mockJwks := jwk.NewSet() + jwksByte, err := json.Marshal(mockJwks) + require.NoError(t, err) + + err = insertMockDBData([]Namespace{mockNamespace("/foo", string(jwksByte), "", AdminMetadata{})}) + require.NoError(t, err) + _, admin_meta, err := getNamespaceJwksByPrefix("/foo") + require.NoError(t, err) + assert.Equal(t, AdminMetadata{}, *admin_meta) + }) + + t.Run("valid-prefix-non-empty-admin-metadata", func(t *testing.T) { + resetNamespaceDB(t) + mockJwks := jwk.NewSet() + jwksByte, err := json.Marshal(mockJwks) + require.NoError(t, err) + + err = insertMockDBData([]Namespace{mockNamespace("/foo", string(jwksByte), "", AdminMetadata{Status: Approved})}) + require.NoError(t, err) + _, admin_meta, err := getNamespaceJwksByPrefix("/foo") + require.NoError(t, err) + assert.Equal(t, Approved, admin_meta.Status) + }) +} + func topologyMockup(t *testing.T, namespaces []string) *httptest.Server { svr := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { var namespaceList []map[string]string @@ -817,104 +854,3 @@ func TestRegistryTopology(t *testing.T) { viper.Reset() } - -func TestCacheAdminTrue(t *testing.T) { - ctx, cancel, egrp := test_utils.TestContext(context.Background(), t) - defer func() { require.NoError(t, egrp.Wait()) }() - defer cancel() - - registryDBDir := t.TempDir() - viper.Set("Registry.DbLocation", registryDBDir) - - err := InitializeDB(ctx) - defer func() { - err := ShutdownDB() - assert.NoError(t, err) - }() - - require.NoError(t, err, "error initializing registry database") - - adminTester := func(ns Namespace) func(t *testing.T) { - return func(t *testing.T) { - err = addNamespace(&ns) - - require.NoError(t, err, "error adding test cache to registry database") - - // This will return a serverCredsError if the AdminMetadata.Status != Approved, which we don't want to happen - // For these tests, otherwise it will get a key parsing error as ns.Pubkey isn't a real jwk - _, err = getNamespaceJwksByPrefix(ns.Prefix, true) - require.NotErrorIsf(t, err, serverCredsErr, "error chain contains serverCredErr") - - require.ErrorContainsf(t, err, "Failed to parse pubkey as a jwks: failed to unmarshal JWK set: invalid character 'k' in literal true (expecting 'r')", "error doesn't contain jwks parsing error") - } - } - - var ns Namespace - ns.Prefix = "/caches/test3" - ns.Identity = "testident3" - ns.Pubkey = "tkey" - ns.AdminMetadata.Status = Approved - - t.Run("WithApproval", adminTester(ns)) - - ns.Prefix = "/orig/test1" - ns.Identity = "testident4" - ns.Pubkey = "tkey" - ns.AdminMetadata.Status = Pending - - t.Run("OriginNoApproval", adminTester(ns)) - - ns.Prefix = "/orig/test2" - ns.Identity = "testident5" - ns.Pubkey = "tkey" - ns.AdminMetadata = AdminMetadata{} - - t.Run("OriginEmptyApproval", adminTester(ns)) - - viper.Reset() -} - -func TestCacheAdminFalse(t *testing.T) { - ctx, cancel, egrp := test_utils.TestContext(context.Background(), t) - defer func() { require.NoError(t, egrp.Wait()) }() - defer cancel() - - registryDBDir := t.TempDir() - viper.Set("Registry.DbLocation", registryDBDir) - - err := InitializeDB(ctx) - defer func() { - err := ShutdownDB() - assert.NoError(t, err) - }() - - require.NoError(t, err, "error initializing registry database") - - adminTester := func(ns Namespace) func(t *testing.T) { - return func(t *testing.T) { - err = addNamespace(&ns) - require.NoError(t, err, "error adding test cache to registry database") - - // This will return a serverCredsError if the admin_approval == false check is triggered, which we want to happen - _, err = getNamespaceJwksByPrefix(ns.Prefix, true) - - require.ErrorIs(t, err, serverCredsErr) - } - } - - var ns Namespace - ns.Prefix = "/caches/test1" - ns.Identity = "testident1" - ns.Pubkey = "tkey" - ns.AdminMetadata.Status = Pending - - t.Run("NoAdmin", adminTester(ns)) - - ns.Prefix = "/caches/test2" - ns.Identity = "testident2" - ns.AdminMetadata = AdminMetadata{} - - t.Run("EmptyAdmin", adminTester(ns)) - - viper.Reset() -} diff --git a/registry/registry_test.go b/registry/registry_test.go index b9bb2b497..f78089093 100644 --- a/registry/registry_test.go +++ b/registry/registry_test.go @@ -9,6 +9,7 @@ import ( "github.com/gin-gonic/gin" "github.com/lestrrat-go/jwx/v2/jwk" + "github.com/spf13/viper" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -33,6 +34,7 @@ func TestHandleWildcard(t *testing.T) { }) t.Run("match-wildcard-metadataHandler", func(t *testing.T) { + viper.Reset() mockPrefix := "/testnamespace/foo" setupMockRegistryDB(t) @@ -53,8 +55,91 @@ func TestHandleWildcard(t *testing.T) { r.ServeHTTP(w, req) - // Should return 200 for matched metadataHandler since the db is empty + // Return 200 as by default Registry.OriginApprovedOnly == false assert.Equal(t, http.StatusOK, w.Code) assert.Equal(t, string(mockJWKSBytes), w.Body.String()) }) + + mockApprovalTcs := []struct { + Name string + CacheApprovedOnly bool + OriginApprovedOnly bool + IsApproved bool + IsCache bool + ExpectedCode int + }{ + { + Name: "cache-origin-both-req-approv-origin-no-approv", + CacheApprovedOnly: true, + OriginApprovedOnly: true, + IsApproved: false, + IsCache: false, + ExpectedCode: 403, + }, + { + Name: "cache-origin-both-req-approv-cache-no-approv", + CacheApprovedOnly: true, + OriginApprovedOnly: true, + IsApproved: false, + IsCache: true, + ExpectedCode: 403, + }, + { + Name: "cache-origin-both-req-approv-origin-approv", + CacheApprovedOnly: true, + OriginApprovedOnly: true, + IsApproved: true, + IsCache: false, + ExpectedCode: 200, + }, + { + Name: "cache-origin-both-req-approv-cache-approv", + CacheApprovedOnly: true, + OriginApprovedOnly: true, + IsApproved: true, + IsCache: true, + ExpectedCode: 200, + }, + } + + for _, tc := range mockApprovalTcs { + t.Run(tc.Name, func(t *testing.T) { + viper.Reset() + viper.Set("Registry.CacheApprovedOnly", tc.CacheApprovedOnly) + viper.Set("Registry.OriginApprovedOnly", tc.OriginApprovedOnly) + + mockPrefix := "/testnamespace/foo" + if tc.IsCache { + mockPrefix = "/caches/hostname" + } + + setupMockRegistryDB(t) + defer teardownMockNamespaceDB(t) + + mockJWKS := jwk.NewSet() + mockJWKSBytes, err := json.Marshal(mockJWKS) + require.NoError(t, err) + + mockStatus := Pending + if tc.IsApproved { + mockStatus = Approved + } + err = insertMockDBData([]Namespace{{Prefix: mockPrefix, Pubkey: string(mockJWKSBytes), AdminMetadata: AdminMetadata{Status: mockStatus}}}) + require.NoError(t, err) + mockNs, err := getNamespaceByPrefix(mockPrefix) + + require.NoError(t, err) + require.NotNil(t, mockNs) + + req, _ := http.NewRequest("GET", fmt.Sprintf("/registry%s/.well-known/issuer.jwks", mockPrefix), nil) + w := httptest.NewRecorder() + + r.ServeHTTP(w, req) + + assert.Equal(t, tc.ExpectedCode, w.Code) + if tc.ExpectedCode == 200 { + assert.Equal(t, string(mockJWKSBytes), w.Body.String()) + } + }) + } } From 5888e88b9cc6f5384b52edbc3778182a4bcc5a20 Mon Sep 17 00:00:00 2001 From: Haoming Meng Date: Wed, 24 Jan 2024 20:18:53 +0000 Subject: [PATCH 3/9] Fix director not giving correct error message when server not approved --- director/origin_api.go | 16 ++-------------- director/redirect.go | 34 ++++++++++++++++++++-------------- server_ui/advertise.go | 2 +- 3 files changed, 23 insertions(+), 29 deletions(-) diff --git a/director/origin_api.go b/director/origin_api.go index 8026da14f..a8ff35d23 100644 --- a/director/origin_api.go +++ b/director/origin_api.go @@ -20,7 +20,6 @@ package director import ( "context" - "encoding/json" "net/http" "net/url" "strings" @@ -151,22 +150,11 @@ func VerifyAdvertiseToken(ctx context.Context, token, namespace string) (bool, e log.Debugln("Attempting to fetch keys from ", issuerUrl) keyset, err := ar.Get(ctx, issuerUrl) - if log.IsLevelEnabled(log.DebugLevel) { - // Let's check that we can convert to JSON and get the right thing... - jsonbuf, err := json.Marshal(keyset) - if err != nil { - return false, errors.Wrap(err, "failed to marshal the public keyset into JWKS JSON") - } - log.Debugln("Constructed JWKS from fetching jwks:", string(jsonbuf)) - // This seems never get reached, as registry returns 403 for pending approval namespace - // and there will be HTTP error in getting jwks; thus it will always be error - if jsonbuf == nil { + if err != nil { + if strings.Contains(err.Error(), "403") { adminApprovalErr = errors.New(namespace + " has not been approved by an administrator.") return false, adminApprovalErr } - } - - if err != nil { return false, err } diff --git a/director/redirect.go b/director/redirect.go index c788dc4c1..2d17611dd 100644 --- a/director/redirect.go +++ b/director/redirect.go @@ -428,14 +428,20 @@ func registerServeAd(engineCtx context.Context, ctx *gin.Context, sType ServerTy token := strings.TrimPrefix(tokens[0], "Bearer ") ok, err := VerifyAdvertiseToken(engineCtx, token, namespace.Path) if err != nil { - log.Warningln("Failed to verify token:", err) - ctx.JSON(http.StatusForbidden, gin.H{"error": "Authorization token verification failed"}) - return + if err == adminApprovalErr { + log.Warningf("Failed to verify token. Namespace %q was not approved", namespace.Path) + ctx.JSON(http.StatusForbidden, gin.H{"error": fmt.Sprintf("The namespace %q was not approved by an administrator", namespace.Path)}) + return + } else { + log.Warningln("Failed to verify token:", err) + ctx.JSON(http.StatusForbidden, gin.H{"error": "Authorization token verification failed."}) + return + } } if !ok { - log.Warningf("%s %v advertised to namespace %v without valid registration\n", + log.Warningf("%s %v advertised to namespace %v without valid token scope\n", sType, ad.Name, namespace.Path) - ctx.JSON(http.StatusForbidden, gin.H{"error": sType + " not authorized to advertise to this namespace"}) + ctx.JSON(http.StatusForbidden, gin.H{"error": "Authorization token verification failed. Token missing required scope"}) return } } @@ -445,18 +451,18 @@ func registerServeAd(engineCtx context.Context, ctx *gin.Context, sType ServerTy ok, err := VerifyAdvertiseToken(engineCtx, token, prefix) if err != nil { if err == adminApprovalErr { - log.Warningln("Failed to verify token. Cache was not approved:", err) - ctx.JSON(http.StatusForbidden, gin.H{"error": "Cache is not admin approved"}) + log.Warningf("Failed to verify token. Cache %q was not approved", ad.Name) + ctx.JSON(http.StatusForbidden, gin.H{"error": fmt.Sprintf("Cache %q was not approved by an administrator", ad.Name)}) + return } else { log.Warningln("Failed to verify token:", err) - ctx.JSON(http.StatusForbidden, gin.H{"error": "Authorization token verification failed"}) + ctx.JSON(http.StatusForbidden, gin.H{"error": "Authorization token verification failed."}) + return } - return } if !ok { - log.Warningf("%s %v advertised to namespace %v without valid registration\n", - sType, ad.Name, prefix) - ctx.JSON(http.StatusForbidden, gin.H{"error": sType + " not authorized to advertise to this namespace"}) + log.Warningf("%s %v advertised without valid token scope\n", sType, ad.Name) + ctx.JSON(http.StatusForbidden, gin.H{"error": "Authorization token verification failed. Token missing required scope"}) return } } @@ -470,8 +476,8 @@ func registerServeAd(engineCtx context.Context, ctx *gin.Context, sType ServerTy adWebUrl, err := url.Parse(ad.WebURL) if err != nil && ad.WebURL != "" { // We allow empty WebURL string for backward compatibility - log.Warningf("Failed to parse origin Web URL %v: %v\n", ad.WebURL, err) - ctx.JSON(http.StatusBadRequest, gin.H{"error": "Invalid origin Web URL"}) + log.Warningf("Failed to parse server Web URL %v: %v\n", ad.WebURL, err) + ctx.JSON(http.StatusBadRequest, gin.H{"error": "Invalid server Web URL"}) return } diff --git a/server_ui/advertise.go b/server_ui/advertise.go index 38c4b5afc..77b0ca960 100644 --- a/server_ui/advertise.go +++ b/server_ui/advertise.go @@ -151,7 +151,7 @@ func advertiseInternal(ctx context.Context, server server_utils.XRootDServer) er return errors.Wrapf(unmarshalErr, "Could not unmarshall the director's response, which responded %v from director registration: %v", resp.StatusCode, resp.Status) } if resp.StatusCode == http.StatusForbidden { - return errors.Errorf("Error during director advertisement: Cache has not been approved by administrator.") + return errors.Errorf("Error during director advertisement: Your namespace has not been approved by an administrator.") } return errors.Errorf("Error during director registration: %v\n", respErr.Error) } From 5b736da531bd60019e2aa402df15f29b55351b1c Mon Sep 17 00:00:00 2001 From: Haoming Meng Date: Wed, 24 Jan 2024 22:02:26 +0000 Subject: [PATCH 4/9] Add approval status check before pulling jwks for consistent error reporting --- director/origin_api.go | 72 ++++++++++++++++++++++++++++++++++--- director/origin_api_test.go | 30 ++++++++++++++-- director/redirect.go | 2 +- director/redirect_test.go | 59 +++++++++++++++++------------- registry/registry.go | 29 +++++++++++++++ 5 files changed, 160 insertions(+), 32 deletions(-) diff --git a/director/origin_api.go b/director/origin_api.go index a8ff35d23..b6619e7a4 100644 --- a/director/origin_api.go +++ b/director/origin_api.go @@ -19,7 +19,11 @@ package director import ( + "bytes" "context" + "encoding/json" + "fmt" + "io" "net/http" "net/url" "strings" @@ -47,6 +51,14 @@ type ( EnableWrite bool `json:"enablewrite"` EnableFallbackRead bool `json:"enable-fallback-read"` // True if the origin will allow direct client reads when no caches are available } + + checkStatusReq struct { + Prefix string `json:"prefix"` + } + + checkStatusRes struct { + Approved bool `json:"approved"` + } ) // Create interface @@ -63,6 +75,53 @@ var ( adminApprovalErr error ) +func checkNamespaceStatus(prefix string, registryWebUrlStr string) (bool, error) { + registryUrl, err := url.Parse(registryWebUrlStr) + if err != nil { + return false, err + } + reqUrl := registryUrl.JoinPath("/api/v1.0/registry/checkNamespaceStatus") + + reqBody := checkStatusReq{Prefix: prefix} + reqByte, err := json.Marshal(reqBody) + if err != nil { + return false, err + } + client := http.Client{Transport: config.GetTransport()} + req, err := http.NewRequest("POST", reqUrl.String(), bytes.NewBuffer(reqByte)) + req.Header.Add("Content-Type", "application/json") + if err != nil { + return false, err + } + + res, err := client.Do(req) + if err != nil { + return false, err + } + + if res.StatusCode != 200 { + if res.StatusCode == 404 { + // This is when we hit a legacy OSDF registry (or Pelican registry <= 7.4.0) which doesn't have such endpoint + log.Warningf("Request %q hit 404, either it's an OSDF registry or Pelican registry <= 7.4.0. Fallback to return true for approval status check", reqUrl.String()) + return true, nil + } else { + return false, errors.New(fmt.Sprintf("Server error with status code %d", res.StatusCode)) + } + } + + resBody := checkStatusRes{} + bodyByte, err := io.ReadAll(res.Body) + if err != nil { + return false, err + } + + if err := json.Unmarshal(bodyByte, &resBody); err != nil { + return false, err + } + + return resBody.Approved, nil +} + func CreateAdvertiseToken(namespace string) (string, error) { // TODO: Need to come back and carefully consider a few naming practices. // Here, issuerUrl is actually the registry database url, and not @@ -130,6 +189,15 @@ func VerifyAdvertiseToken(ctx context.Context, token, namespace string) (bool, e } } }() + namespace_url_string := param.Federation_RegistryUrl.GetString() + approved, err := checkNamespaceStatus(namespace, namespace_url_string) + if err != nil { + return false, errors.Wrap(err, "Failed to check namespace approval status") + } + if !approved { + adminApprovalErr = errors.New(namespace + " has not been approved by an administrator.") + return false, adminApprovalErr + } if ar == nil { ar = jwk.NewCache(ctx) client := &http.Client{Transport: config.GetTransport()} @@ -151,10 +219,6 @@ func VerifyAdvertiseToken(ctx context.Context, token, namespace string) (bool, e keyset, err := ar.Get(ctx, issuerUrl) if err != nil { - if strings.Contains(err.Error(), "403") { - adminApprovalErr = errors.New(namespace + " has not been approved by an administrator.") - return false, adminApprovalErr - } return false, err } diff --git a/director/origin_api_test.go b/director/origin_api_test.go index 2d725e860..865e051f0 100644 --- a/director/origin_api_test.go +++ b/director/origin_api_test.go @@ -2,6 +2,9 @@ package director import ( "context" + "encoding/json" + "net/http" + "net/http/httptest" "path/filepath" "testing" "time" @@ -31,18 +34,39 @@ func TestVerifyAdvertiseToken(t *testing.T) { //Setup a private key and a token viper.Set("IssuerKey", kfile) - viper.Set("Federation.RegistryUrl", "https://get-your-tokens.org") viper.Set("Federation.DirectorURL", "https://director-url.org") config.InitConfig() err := config.InitServer(ctx, config.DirectorType) require.NoError(t, err) + // Mock registry server + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + if req.Method == "POST" && req.URL.Path == "/api/v1.0/registry/checkNamespaceStatus" { + res := checkStatusRes{Approved: true} + resByte, err := json.Marshal(res) + if err != nil { + w.WriteHeader(http.StatusInternalServerError) + return + } + _, err = w.Write(resByte) + if err != nil { + w.WriteHeader(http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusOK) + } else { + w.WriteHeader(http.StatusNotFound) + } + })) + defer ts.Close() + + viper.Set("Federation.RegistryUrl", ts.URL) kSet, err := config.GetIssuerPublicJWKS() ar := MockCache{ GetFn: func(key string, keyset *jwk.Set) (jwk.Set, error) { - if key != "https://get-your-tokens.org/api/v1.0/registry/test-namespace/.well-known/issuer.jwks" { - t.Errorf("expecting: https://get-your-tokens.org/api/v1.0/registry/test-namespace/.well-known/issuer.jwks, got %q", key) + if key != ts.URL+"/api/v1.0/registry/test-namespace/.well-known/issuer.jwks" { + t.Errorf("expecting: %s/api/v1.0/registry/test-namespace/.well-known/issuer.jwks, got %q", ts.URL, key) } return *keyset, nil }, diff --git a/director/redirect.go b/director/redirect.go index 2d17611dd..96866fa71 100644 --- a/director/redirect.go +++ b/director/redirect.go @@ -434,7 +434,7 @@ func registerServeAd(engineCtx context.Context, ctx *gin.Context, sType ServerTy return } else { log.Warningln("Failed to verify token:", err) - ctx.JSON(http.StatusForbidden, gin.H{"error": "Authorization token verification failed."}) + ctx.JSON(http.StatusForbidden, gin.H{"error": "Authorization token verification failed"}) return } } diff --git a/director/redirect_test.go b/director/redirect_test.go index 94b7a09b9..d43c27e00 100644 --- a/director/redirect_test.go +++ b/director/redirect_test.go @@ -66,26 +66,37 @@ func TestDirectorRegistration(t *testing.T) { viper.Reset() - viper.Set("Federation.RegistryUrl", "https://get-your-tokens.org") + // Mock registry server + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + if req.Method == "POST" && req.URL.Path == "/api/v1.0/registry/checkNamespaceStatus" { + res := checkStatusRes{Approved: true} + resByte, err := json.Marshal(res) + if err != nil { + w.WriteHeader(http.StatusInternalServerError) + return + } + _, err = w.Write(resByte) + if err != nil { + w.WriteHeader(http.StatusInternalServerError) + return + } + w.WriteHeader(http.StatusOK) + } else { + w.WriteHeader(http.StatusNotFound) + } + })) + defer ts.Close() + + viper.Set("Federation.RegistryUrl", ts.URL) setupContext := func() (*gin.Context, *gin.Engine, *httptest.ResponseRecorder) { // Setup httptest recorder and context for the the unit test w := httptest.NewRecorder() c, r := gin.CreateTestContext(w) - ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - assert.Equal(t, "POST", req.Method, "Not POST Method") - _, err := w.Write([]byte(":)")) - assert.NoError(t, err) - })) - defer ts.Close() - - c.Request = &http.Request{ - URL: &url.URL{}, - } return c, r, w } - generateToken := func(c *gin.Context) (jwk.Key, string, url.URL) { + generateToken := func() (jwk.Key, string, url.URL) { // Create a private key to use for the test privateKey, err := ecdsa.GenerateKey(elliptic.P521(), rand.Reader) assert.NoError(t, err, "Error generating private key") @@ -104,8 +115,7 @@ func TestDirectorRegistration(t *testing.T) { issuerURL := url.URL{ Scheme: "https", - Path: "get-your-tokens.org/namespaces/foo/bar", - Host: c.Request.URL.Host, + Path: ts.URL, } // Create a token to be inserted @@ -138,8 +148,9 @@ func TestDirectorRegistration(t *testing.T) { setupMockCache := func(t *testing.T, publicKey jwk.Key) MockCache { return MockCache{ GetFn: func(key string, keyset *jwk.Set) (jwk.Set, error) { - if key != "https://get-your-tokens.org/api/v1.0/registry/foo/bar/.well-known/issuer.jwks" { - t.Errorf("expecting: https://get-your-tokens.org/api/v1.0/registry/foo/bar/.well-known/issuer.jwks, got %q", key) + expectedKey := ts.URL + "/api/v1.0/registry/foo/bar/.well-known/issuer.jwks" + if key != expectedKey { + t.Errorf("expecting: %q, got %q", expectedKey, key) } return *keyset, nil }, @@ -171,7 +182,7 @@ func TestDirectorRegistration(t *testing.T) { t.Run("valid-token", func(t *testing.T) { c, r, w := setupContext() - pKey, token, issuerURL := generateToken(c) + pKey, token, issuerURL := generateToken() publicKey, err := jwk.PublicKeyOf(pKey) assert.NoError(t, err, "Error creating public key from private key") @@ -179,7 +190,7 @@ func TestDirectorRegistration(t *testing.T) { useMockCache(ar, issuerURL) isurl := url.URL{} - isurl.Path = "https://get-your-tokens.org" + isurl.Path = ts.URL ad := OriginAdvertise{Name: "test", URL: "https://or-url.org", Namespaces: []NamespaceAd{{Path: "/foo/bar", Issuer: isurl}}} @@ -204,7 +215,7 @@ func TestDirectorRegistration(t *testing.T) { c, r, w := setupContext() wrongPrivateKey, err := ecdsa.GenerateKey(elliptic.P521(), rand.Reader) assert.NoError(t, err, "Error creating another private key") - _, token, issuerURL := generateToken(c) + _, token, issuerURL := generateToken() wrongPublicKey, err := jwk.PublicKeyOf(wrongPrivateKey) assert.NoError(t, err, "Error creating public key from private key") @@ -212,7 +223,7 @@ func TestDirectorRegistration(t *testing.T) { useMockCache(ar, issuerURL) isurl := url.URL{} - isurl.Path = "https://get-your-tokens.org" + isurl.Path = ts.URL ad := OriginAdvertise{Name: "test", URL: "https://or-url.org", Namespaces: []NamespaceAd{{Path: "/foo/bar", Issuer: isurl}}} @@ -234,14 +245,14 @@ func TestDirectorRegistration(t *testing.T) { t.Run("valid-token-with-web-url", func(t *testing.T) { c, r, w := setupContext() - pKey, token, issuerURL := generateToken(c) + pKey, token, issuerURL := generateToken() publicKey, err := jwk.PublicKeyOf(pKey) assert.NoError(t, err, "Error creating public key from private key") ar := setupMockCache(t, publicKey) useMockCache(ar, issuerURL) isurl := url.URL{} - isurl.Path = "https://get-your-tokens.org" + isurl.Path = ts.URL ad := OriginAdvertise{WebURL: "https://localhost:8844", Namespaces: []NamespaceAd{{Path: "/foo/bar", Issuer: isurl}}} @@ -261,14 +272,14 @@ func TestDirectorRegistration(t *testing.T) { // We want to ensure backwards compatibility for WebURL t.Run("valid-token-without-web-url", func(t *testing.T) { c, r, w := setupContext() - pKey, token, issuerURL := generateToken(c) + pKey, token, issuerURL := generateToken() publicKey, err := jwk.PublicKeyOf(pKey) assert.NoError(t, err, "Error creating public key from private key") ar := setupMockCache(t, publicKey) useMockCache(ar, issuerURL) isurl := url.URL{} - isurl.Path = "https://get-your-tokens.org" + isurl.Path = ts.URL ad := OriginAdvertise{Namespaces: []NamespaceAd{{Path: "/foo/bar", Issuer: isurl}}} diff --git a/registry/registry.go b/registry/registry.go index b4e10945d..5a8ef8d11 100644 --- a/registry/registry.go +++ b/registry/registry.go @@ -96,6 +96,14 @@ type checkNamespaceExistsRes struct { Error string `json:"error"` } +type checkStatusReq struct { + Prefix string `json:"prefix"` +} + +type checkStatusRes struct { + Approved bool `json:"approved"` +} + // Various auxiliary functions used for client-server security handshakes type registrationData struct { ClientNonce string `json:"client_nonce"` @@ -763,6 +771,26 @@ func checkNamespaceExistsHandler(ctx *gin.Context) { } } +func checkNamespaceStatusHandler(ctx *gin.Context) { + req := checkStatusReq{} + if err := ctx.ShouldBind(&req); err != nil { + log.Debug("Failed to parse request body for namespace status check: ", err) + ctx.JSON(http.StatusBadRequest, gin.H{"error": "Failed to parse request body"}) + return + } + if req.Prefix == "" { + ctx.JSON(http.StatusBadRequest, gin.H{"error": "prefix is required"}) + return + } + ns, err := getNamespaceByPrefix(req.Prefix) + if err != nil || ns == nil { + ctx.JSON(http.StatusInternalServerError, gin.H{"error": "Error getting namespace"}) + return + } + res := checkStatusRes{Approved: ns.AdminMetadata.Status == Approved} + ctx.JSON(http.StatusOK, res) +} + func RegisterRegistryAPI(router *gin.RouterGroup) { registryAPI := router.Group("/api/v1.0/registry") @@ -776,6 +804,7 @@ func RegisterRegistryAPI(router *gin.RouterGroup) { // Handle everything under "/" route with GET method registryAPI.GET("/*wildcard", wildcardHandler) registryAPI.POST("/checkNamespaceExists", checkNamespaceExistsHandler) + registryAPI.POST("/checkNamespaceStatus", checkNamespaceStatusHandler) registryAPI.DELETE("/*wildcard", deleteNamespaceHandler) } } From a961c0f720f1a2c23925bc069159f5b422eb5626 Mon Sep 17 00:00:00 2001 From: Haoming Meng Date: Wed, 24 Jan 2024 22:20:02 +0000 Subject: [PATCH 5/9] Explicit telling xrootd server the approval error on registration --- director/redirect.go | 4 ++-- server_ui/advertise.go | 7 ++++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/director/redirect.go b/director/redirect.go index 96866fa71..cb467a6de 100644 --- a/director/redirect.go +++ b/director/redirect.go @@ -430,7 +430,7 @@ func registerServeAd(engineCtx context.Context, ctx *gin.Context, sType ServerTy if err != nil { if err == adminApprovalErr { log.Warningf("Failed to verify token. Namespace %q was not approved", namespace.Path) - ctx.JSON(http.StatusForbidden, gin.H{"error": fmt.Sprintf("The namespace %q was not approved by an administrator", namespace.Path)}) + ctx.JSON(http.StatusForbidden, gin.H{"approval_error": true, "error": fmt.Sprintf("The namespace %q was not approved by an administrator", namespace.Path)}) return } else { log.Warningln("Failed to verify token:", err) @@ -452,7 +452,7 @@ func registerServeAd(engineCtx context.Context, ctx *gin.Context, sType ServerTy if err != nil { if err == adminApprovalErr { log.Warningf("Failed to verify token. Cache %q was not approved", ad.Name) - ctx.JSON(http.StatusForbidden, gin.H{"error": fmt.Sprintf("Cache %q was not approved by an administrator", ad.Name)}) + ctx.JSON(http.StatusForbidden, gin.H{"approval_error": true, "error": fmt.Sprintf("Cache %q was not approved by an administrator", ad.Name)}) return } else { log.Warningln("Failed to verify token:", err) diff --git a/server_ui/advertise.go b/server_ui/advertise.go index 77b0ca960..326924b08 100644 --- a/server_ui/advertise.go +++ b/server_ui/advertise.go @@ -40,7 +40,8 @@ import ( ) type directorResponse struct { - Error string `json:"error"` + Error string `json:"error"` + ApprovalError bool `json:"approval_error"` } func LaunchPeriodicAdvertise(ctx context.Context, egrp *errgroup.Group, servers []server_utils.XRootDServer) error { @@ -150,8 +151,8 @@ func advertiseInternal(ctx context.Context, server server_utils.XRootDServer) er if unmarshalErr := json.Unmarshal(body, &respErr); unmarshalErr != nil { // Error creating json return errors.Wrapf(unmarshalErr, "Could not unmarshall the director's response, which responded %v from director registration: %v", resp.StatusCode, resp.Status) } - if resp.StatusCode == http.StatusForbidden { - return errors.Errorf("Error during director advertisement: Your namespace has not been approved by an administrator.") + if respErr.ApprovalError { + return errors.Errorf("Your namespace has not been approved by an administrator.") } return errors.Errorf("Error during director registration: %v\n", respErr.Error) } From 5269994df2454ea7ac6f14f38c8cfd6d12c40fd2 Mon Sep 17 00:00:00 2001 From: Haoming Meng Date: Thu, 25 Jan 2024 14:48:47 +0000 Subject: [PATCH 6/9] Fix test failure --- cmd/fed_serve_test.go | 2 ++ cmd/object_get_put_test.go | 3 +++ 2 files changed, 5 insertions(+) diff --git a/cmd/fed_serve_test.go b/cmd/fed_serve_test.go index 8997fd7a9..ff45f4841 100644 --- a/cmd/fed_serve_test.go +++ b/cmd/fed_serve_test.go @@ -83,6 +83,8 @@ func TestFedServePosixOrigin(t *testing.T) { viper.Set("TLSSkipVerify", true) viper.Set("Server.EnableUI", false) viper.Set("Registry.DbLocation", filepath.Join(t.TempDir(), "ns-registry.sqlite")) + viper.Set("Registry.OriginApprovedOnly", false) + viper.Set("Registry.CacheApprovedOnly", false) err = config.InitServer(ctx, modules) require.NoError(t, err) diff --git a/cmd/object_get_put_test.go b/cmd/object_get_put_test.go index 8ad4665fb..a3feb0150 100644 --- a/cmd/object_get_put_test.go +++ b/cmd/object_get_put_test.go @@ -99,6 +99,9 @@ func TestGetAndPut(t *testing.T) { err = config.InitServer(ctx, modules) require.NoError(t, err) + viper.Set("Registry.OriginApprovedOnly", false) + viper.Set("Registry.CacheApprovedOnly", false) + fedCancel, err := launchers.LaunchModules(ctx, modules) if err != nil { t.Fatalf("Failure in fedServeInternal: %v", err) From a2f06e16a7540012c02487799ca19ac416bfc7e5 Mon Sep 17 00:00:00 2001 From: Haoming Meng Date: Thu, 25 Jan 2024 15:52:55 +0000 Subject: [PATCH 7/9] Fixed bug in checkApporvStatus --- client/handle_http_test.go | 2 ++ registry/registry.go | 32 ++++++++++++++++++++++++++++++-- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/client/handle_http_test.go b/client/handle_http_test.go index 91282ac86..78d0d6a0c 100644 --- a/client/handle_http_test.go +++ b/client/handle_http_test.go @@ -474,6 +474,8 @@ func TestFullUpload(t *testing.T) { viper.Set("Server.EnableUI", false) viper.Set("Registry.DbLocation", filepath.Join(t.TempDir(), "ns-registry.sqlite")) viper.Set("Xrootd.RunLocation", tmpPath) + viper.Set("Registry.OriginApprovedOnly", false) + viper.Set("Registry.CacheApprovedOnly", false) err = config.InitServer(ctx, modules) require.NoError(t, err) diff --git a/registry/registry.go b/registry/registry.go index 5a8ef8d11..80c5e35a8 100644 --- a/registry/registry.go +++ b/registry/registry.go @@ -787,8 +787,36 @@ func checkNamespaceStatusHandler(ctx *gin.Context) { ctx.JSON(http.StatusInternalServerError, gin.H{"error": "Error getting namespace"}) return } - res := checkStatusRes{Approved: ns.AdminMetadata.Status == Approved} - ctx.JSON(http.StatusOK, res) + emptyMetadata := AdminMetadata{} + // If Registry.CacheApprovedOnly or Registry.OriginApprovedOnly is false + // we return Approved == true + if ns.AdminMetadata != emptyMetadata { + // Caches + if strings.HasPrefix(req.Prefix, "/caches") && param.Registry_CacheApprovedOnly.GetBool() { + res := checkStatusRes{Approved: ns.AdminMetadata.Status == Approved} + ctx.JSON(http.StatusOK, res) + return + } else if !param.Registry_CacheApprovedOnly.GetBool() { + res := checkStatusRes{Approved: true} + ctx.JSON(http.StatusOK, res) + return + } else { + // Origins + if param.Registry_OriginApprovedOnly.GetBool() { + res := checkStatusRes{Approved: ns.AdminMetadata.Status == Approved} + ctx.JSON(http.StatusOK, res) + return + } else { + res := checkStatusRes{Approved: true} + ctx.JSON(http.StatusOK, res) + return + } + } + } else { + // For legacy Pelican (<=7.3.0) registry schema without Admin_Metadata + res := checkStatusRes{Approved: true} + ctx.JSON(http.StatusOK, res) + } } func RegisterRegistryAPI(router *gin.RouterGroup) { From 0eb2920b3fc0df11c1b962996f1206ec4e91d8ef Mon Sep 17 00:00:00 2001 From: Haoming Meng Date: Wed, 31 Jan 2024 17:06:14 +0000 Subject: [PATCH 8/9] Rename parameters --- client/handle_http_test.go | 4 ++-- cmd/fed_serve_test.go | 4 ++-- cmd/object_get_put_test.go | 4 ++-- docs/parameters.yaml | 4 ++-- param/parameters.go | 4 ++-- param/parameters_struct.go | 8 ++++---- registry/registry.go | 12 ++++++------ registry/registry_test.go | 6 +++--- 8 files changed, 23 insertions(+), 23 deletions(-) diff --git a/client/handle_http_test.go b/client/handle_http_test.go index 78d0d6a0c..b1183e1f3 100644 --- a/client/handle_http_test.go +++ b/client/handle_http_test.go @@ -474,8 +474,8 @@ func TestFullUpload(t *testing.T) { viper.Set("Server.EnableUI", false) viper.Set("Registry.DbLocation", filepath.Join(t.TempDir(), "ns-registry.sqlite")) viper.Set("Xrootd.RunLocation", tmpPath) - viper.Set("Registry.OriginApprovedOnly", false) - viper.Set("Registry.CacheApprovedOnly", false) + viper.Set("Registry.RequireOriginApproval", false) + viper.Set("Registry.RequireCacheApproval", false) err = config.InitServer(ctx, modules) require.NoError(t, err) diff --git a/cmd/fed_serve_test.go b/cmd/fed_serve_test.go index ff45f4841..471f03c4e 100644 --- a/cmd/fed_serve_test.go +++ b/cmd/fed_serve_test.go @@ -83,8 +83,8 @@ func TestFedServePosixOrigin(t *testing.T) { viper.Set("TLSSkipVerify", true) viper.Set("Server.EnableUI", false) viper.Set("Registry.DbLocation", filepath.Join(t.TempDir(), "ns-registry.sqlite")) - viper.Set("Registry.OriginApprovedOnly", false) - viper.Set("Registry.CacheApprovedOnly", false) + viper.Set("Registry.RequireOriginApproval", false) + viper.Set("Registry.RequireCacheApproval", false) err = config.InitServer(ctx, modules) require.NoError(t, err) diff --git a/cmd/object_get_put_test.go b/cmd/object_get_put_test.go index a3feb0150..7d349af61 100644 --- a/cmd/object_get_put_test.go +++ b/cmd/object_get_put_test.go @@ -99,8 +99,8 @@ func TestGetAndPut(t *testing.T) { err = config.InitServer(ctx, modules) require.NoError(t, err) - viper.Set("Registry.OriginApprovedOnly", false) - viper.Set("Registry.CacheApprovedOnly", false) + viper.Set("Registry.RequireOriginApproval", false) + viper.Set("Registry.RequireCacheApproval", false) fedCancel, err := launchers.LaunchModules(ctx, modules) if err != nil { diff --git a/docs/parameters.yaml b/docs/parameters.yaml index 2f5a053fc..b8313a93c 100644 --- a/docs/parameters.yaml +++ b/docs/parameters.yaml @@ -662,7 +662,7 @@ type: duration default: 15m components: ["nsregistry"] --- -name: Registry.CacheApprovedOnly +name: Registry.RequireCacheApproval description: >- Only allow approved caches to join the federation and serve files. If set to true, caches can successfully self-register or registered via registry, but director won't direct traffic to the cache. @@ -671,7 +671,7 @@ default: false osdf_default: true components: ["nsregistry"] --- -name: Registry.OriginApprovedOnly +name: Registry.RequireOriginApproval description: >- Only allow approved origins to join the federation and serve files. If set to true, origins can successfully self-register or registered via registry, but director won't direct traffic to the origin, diff --git a/param/parameters.go b/param/parameters.go index c511a0496..5d113eef0 100644 --- a/param/parameters.go +++ b/param/parameters.go @@ -180,9 +180,9 @@ var ( Origin_Multiuser = BoolParam{"Origin.Multiuser"} Origin_ScitokensMapSubject = BoolParam{"Origin.ScitokensMapSubject"} Origin_SelfTest = BoolParam{"Origin.SelfTest"} - Registry_CacheApprovedOnly = BoolParam{"Registry.CacheApprovedOnly"} - Registry_OriginApprovedOnly = BoolParam{"Registry.OriginApprovedOnly"} + Registry_RequireCacheApproval = BoolParam{"Registry.RequireCacheApproval"} Registry_RequireKeyChaining = BoolParam{"Registry.RequireKeyChaining"} + Registry_RequireOriginApproval = BoolParam{"Registry.RequireOriginApproval"} Server_EnableUI = BoolParam{"Server.EnableUI"} StagePlugin_Hook = BoolParam{"StagePlugin.Hook"} TLSSkipVerify = BoolParam{"TLSSkipVerify"} diff --git a/param/parameters_struct.go b/param/parameters_struct.go index 85f4c9164..21eba358e 100644 --- a/param/parameters_struct.go +++ b/param/parameters_struct.go @@ -116,13 +116,13 @@ type config struct { } Registry struct { AdminUsers []string - CacheApprovedOnly bool DbLocation string Institutions interface{} InstitutionsUrl string InstitutionsUrlReloadMinutes time.Duration - OriginApprovedOnly bool + RequireCacheApproval bool RequireKeyChaining bool + RequireOriginApproval bool } Server struct { EnableUI bool @@ -288,13 +288,13 @@ type configWithType struct { } Registry struct { AdminUsers struct { Type string; Value []string } - CacheApprovedOnly struct { Type string; Value bool } DbLocation struct { Type string; Value string } Institutions struct { Type string; Value interface{} } InstitutionsUrl struct { Type string; Value string } InstitutionsUrlReloadMinutes struct { Type string; Value time.Duration } - OriginApprovedOnly struct { Type string; Value bool } + RequireCacheApproval struct { Type string; Value bool } RequireKeyChaining struct { Type string; Value bool } + RequireOriginApproval struct { Type string; Value bool } } Server struct { EnableUI struct { Type string; Value bool } diff --git a/registry/registry.go b/registry/registry.go index 80c5e35a8..2f3781641 100644 --- a/registry/registry.go +++ b/registry/registry.go @@ -682,13 +682,13 @@ func wildcardHandler(ctx *gin.Context) { } if adminMetadata != nil && adminMetadata.Status != Approved { if strings.HasPrefix(prefix, "/caches/") { // Caches - if param.Registry_CacheApprovedOnly.GetBool() { + if param.Registry_RequireCacheApproval.GetBool() { // Use 403 to distinguish between server error ctx.JSON(http.StatusForbidden, gin.H{"error": "The cache has not been approved by federation administrator"}) return } } else { // Origins - if param.Registry_OriginApprovedOnly.GetBool() { + if param.Registry_RequireOriginApproval.GetBool() { // Use 403 to distinguish between server error ctx.JSON(http.StatusForbidden, gin.H{"error": "The origin has not been approved by federation administrator"}) return @@ -788,21 +788,21 @@ func checkNamespaceStatusHandler(ctx *gin.Context) { return } emptyMetadata := AdminMetadata{} - // If Registry.CacheApprovedOnly or Registry.OriginApprovedOnly is false + // If Registry.RequireCacheApproval or Registry.RequireOriginApproval is false // we return Approved == true if ns.AdminMetadata != emptyMetadata { // Caches - if strings.HasPrefix(req.Prefix, "/caches") && param.Registry_CacheApprovedOnly.GetBool() { + if strings.HasPrefix(req.Prefix, "/caches") && param.Registry_RequireCacheApproval.GetBool() { res := checkStatusRes{Approved: ns.AdminMetadata.Status == Approved} ctx.JSON(http.StatusOK, res) return - } else if !param.Registry_CacheApprovedOnly.GetBool() { + } else if !param.Registry_RequireCacheApproval.GetBool() { res := checkStatusRes{Approved: true} ctx.JSON(http.StatusOK, res) return } else { // Origins - if param.Registry_OriginApprovedOnly.GetBool() { + if param.Registry_RequireOriginApproval.GetBool() { res := checkStatusRes{Approved: ns.AdminMetadata.Status == Approved} ctx.JSON(http.StatusOK, res) return diff --git a/registry/registry_test.go b/registry/registry_test.go index f78089093..7b7b4b21a 100644 --- a/registry/registry_test.go +++ b/registry/registry_test.go @@ -55,7 +55,7 @@ func TestHandleWildcard(t *testing.T) { r.ServeHTTP(w, req) - // Return 200 as by default Registry.OriginApprovedOnly == false + // Return 200 as by default Registry.RequireOriginApproval == false assert.Equal(t, http.StatusOK, w.Code) assert.Equal(t, string(mockJWKSBytes), w.Body.String()) }) @@ -105,8 +105,8 @@ func TestHandleWildcard(t *testing.T) { for _, tc := range mockApprovalTcs { t.Run(tc.Name, func(t *testing.T) { viper.Reset() - viper.Set("Registry.CacheApprovedOnly", tc.CacheApprovedOnly) - viper.Set("Registry.OriginApprovedOnly", tc.OriginApprovedOnly) + viper.Set("Registry.RequireCacheApproval", tc.CacheApprovedOnly) + viper.Set("Registry.RequireOriginApproval", tc.OriginApprovedOnly) mockPrefix := "/testnamespace/foo" if tc.IsCache { From d2a5e0fe5853fa183a87c406ea1c194a129135a2 Mon Sep 17 00:00:00 2001 From: Haoming Meng Date: Wed, 31 Jan 2024 17:14:24 +0000 Subject: [PATCH 9/9] Address various code review feedback --- director/origin_api.go | 4 ++-- director/redirect.go | 2 +- server_ui/advertise.go | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/director/origin_api.go b/director/origin_api.go index b6619e7a4..7bbd11ee7 100644 --- a/director/origin_api.go +++ b/director/origin_api.go @@ -189,8 +189,8 @@ func VerifyAdvertiseToken(ctx context.Context, token, namespace string) (bool, e } } }() - namespace_url_string := param.Federation_RegistryUrl.GetString() - approved, err := checkNamespaceStatus(namespace, namespace_url_string) + regUrlStr := param.Federation_RegistryUrl.GetString() + approved, err := checkNamespaceStatus(namespace, regUrlStr) if err != nil { return false, errors.Wrap(err, "Failed to check namespace approval status") } diff --git a/director/redirect.go b/director/redirect.go index cb467a6de..75b0f9770 100644 --- a/director/redirect.go +++ b/director/redirect.go @@ -429,7 +429,7 @@ func registerServeAd(engineCtx context.Context, ctx *gin.Context, sType ServerTy ok, err := VerifyAdvertiseToken(engineCtx, token, namespace.Path) if err != nil { if err == adminApprovalErr { - log.Warningf("Failed to verify token. Namespace %q was not approved", namespace.Path) + log.Warningf("Failed to verify advertise token. Namespace %q requires administrator approval", namespace.Path) ctx.JSON(http.StatusForbidden, gin.H{"approval_error": true, "error": fmt.Sprintf("The namespace %q was not approved by an administrator", namespace.Path)}) return } else { diff --git a/server_ui/advertise.go b/server_ui/advertise.go index 326924b08..39b13773b 100644 --- a/server_ui/advertise.go +++ b/server_ui/advertise.go @@ -149,10 +149,10 @@ func advertiseInternal(ctx context.Context, server server_utils.XRootDServer) er if resp.StatusCode > 299 { var respErr directorResponse if unmarshalErr := json.Unmarshal(body, &respErr); unmarshalErr != nil { // Error creating json - return errors.Wrapf(unmarshalErr, "Could not unmarshall the director's response, which responded %v from director registration: %v", resp.StatusCode, resp.Status) + return errors.Wrapf(unmarshalErr, "Could not unmarshal the director's response, which responded %v from director registration: %v", resp.StatusCode, resp.Status) } if respErr.ApprovalError { - return errors.Errorf("Your namespace has not been approved by an administrator.") + return fmt.Errorf("The namespace %q requires administrator approval. Please contact the administrators of %s for more information.", param.Origin_NamespacePrefix.GetString(), param.Federation_RegistryUrl.GetString()) } return errors.Errorf("Error during director registration: %v\n", respErr.Error) }