Merge pull request #2270 from h2zh/prefix-validation-fix

Properly handle the trailing`/` in namespace registration and advertisement

Author: jhiemstrawisc

director/director.go

@@ -993,6 +993,11 @@ func registerServerAd(engineCtx context.Context, ctx *gin.Context, sType server_
 		adV2 = server_structs.ConvertOriginAdV1ToV2(ad)
 	}
 
+	// Check every namespace path to strip the trailing slash
+	for i := range adV2.Namespaces {
+		adV2.Namespaces[i].Path = server_utils.RemoveTrailingSlash(adV2.Namespaces[i].Path)
+	}
+
 	// Filter the advertised prefixes in the cache server ad
 	// based on the allowed prefixes for caches data.
 	if sType == server_structs.CacheType {
@@ -1061,7 +1066,7 @@ func registerServerAd(engineCtx context.Context, ctx *gin.Context, sType server_
 	// Verify server registration
 	token := strings.TrimPrefix(tokens[0], "Bearer ")
 
-	registryPrefix := adV2.RegistryPrefix
+	registryPrefix := server_utils.RemoveTrailingSlash(adV2.RegistryPrefix)
 	verifyServer := true
 	if registryPrefix == "" {
 		if sType == server_structs.OriginType {

registry/registry.go

@@ -277,124 +277,127 @@ func keySignChallengeCommit(ctx *gin.Context, data *registrationData) (bool, map
 	serverPubkey := serverPrivateKey.PublicKey
 	serverVerified := verifySignature(serverPayload, serverSignature, &serverPubkey)
 
-	if clientVerified && serverVerified {
-		log.Debug("Registering namespace ", data.Prefix)
+	if !(clientVerified && serverVerified) {
+		return false, nil, errors.Errorf("Unable to verify the client's public key, or an encountered an error with its own: "+
+			"server verified:%t, client verified:%t", serverVerified, clientVerified)
+	}
 
-		// Check if prefix exists before doing anything else
-		exists, err := namespaceExistsByPrefix(data.Prefix)
-		if err != nil {
-			log.Errorf("Failed to check if namespace already exists: %v", err)
-			return false, nil, errors.Wrap(err, "Server encountered an error checking if namespace already exists")
-		}
-		if exists {
-			returnMsg := map[string]interface{}{
-				"message": fmt.Sprintf("The prefix %s is already registered -- nothing else to do!", data.Prefix),
-			}
-			log.Infof("Skipping registration of prefix %s because it's already registered.", data.Prefix)
-			return false, returnMsg, nil
-		}
+	// Ensure the user input prefix doesn't contain any invalid characters.
+	// Also remove the trailing slash if it exists.
+	reqPrefix, err := validatePrefix(data.Prefix)
+	if err != nil {
+		err = errors.Wrapf(err, "requested namespace %s failed validation", data.Prefix)
+		log.Errorln(err)
+		return false, nil, badRequestError{Message: err.Error()}
+	}
+	data.Prefix = reqPrefix
 
-		reqPrefix, err := validatePrefix(data.Prefix)
-		if err != nil {
-			err = errors.Wrapf(err, "Requested namespace %s failed validation", data.Prefix)
-			log.Errorln(err)
-			return false, nil, badRequestError{Message: err.Error()}
-		}
-		data.Prefix = reqPrefix
+	log.Debug("Registering namespace ", data.Prefix)
 
-		inTopo, topoNss, valErr, sysErr := validateKeyChaining(reqPrefix, key)
-		if valErr != nil {
-			log.Errorln(err)
-			return false, nil, permissionDeniedError{Message: valErr.Error()}
-		}
-		if sysErr != nil {
-			log.Errorln(err)
-			return false, nil, sysErr
+	// Check if prefix exists before doing anything else
+	exists, err := namespaceExistsByPrefix(data.Prefix)
+	if err != nil {
+		log.Errorf("Failed to check if namespace already exists: %v", err)
+		return false, nil, errors.Wrap(err, "Server encountered an error checking if namespace already exists")
+	}
+	if exists {
+		returnMsg := map[string]interface{}{
+			"message": fmt.Sprintf("The prefix %s is already registered -- nothing else to do!", data.Prefix),
 		}
+		log.Infof("Skipping registration of prefix %s because it's already registered.", data.Prefix)
+		return false, returnMsg, nil
+	}
+
+	inTopo, topoNss, valErr, sysErr := validateKeyChaining(data.Prefix, key)
+	if valErr != nil {
+		log.Errorln(err)
+		return false, nil, permissionDeniedError{Message: valErr.Error()}
+	}
+	if sysErr != nil {
+		log.Errorln(err)
+		return false, nil, sysErr
+	}
+
+	var ns server_structs.Namespace
+	ns.Prefix = data.Prefix
 
-		var ns server_structs.Namespace
-		ns.Prefix = data.Prefix
+	pubkeyData, err := json.Marshal(data.Pubkey)
+	if err != nil {
+		return false, nil, errors.Wrapf(err, "Failed to convert public key from json to string format for the prefix %s", ns.Prefix)
+	}
+	ns.Pubkey = string(pubkeyData)
+	ns.Identity = data.Identity
+	ns.AdminMetadata.SiteName = data.SiteName
 
-		pubkeyData, err := json.Marshal(data.Pubkey)
+	if data.Identity != "" {
+		idMap := map[string]interface{}{}
+		err := json.Unmarshal([]byte(data.Identity), &idMap)
 		if err != nil {
-			return false, nil, errors.Wrapf(err, "Failed to convert public key from json to string format for the prefix %s", ns.Prefix)
+			log.Errorln("Failed to decode non-empty Identity field:", err)
+			return false, nil, err
 		}
-		ns.Pubkey = string(pubkeyData)
-		ns.Identity = data.Identity
-		ns.AdminMetadata.SiteName = data.SiteName
-
-		if data.Identity != "" {
-			idMap := map[string]interface{}{}
-			err := json.Unmarshal([]byte(data.Identity), &idMap)
-			if err != nil {
-				log.Errorln("Failed to decode non-empty Identity field:", err)
-				return false, nil, err
-			}
-			sub, ok := idMap["sub"]
+		sub, ok := idMap["sub"]
+		if ok {
+			val, ok := sub.(string)
 			if ok {
-				val, ok := sub.(string)
-				if ok {
-					ns.AdminMetadata.UserID = val
-				}
+				ns.AdminMetadata.UserID = val
 			}
-			if inTopo {
-				topoNssStr := GetTopoPrefixString(topoNss)
-				ns.AdminMetadata.Description = fmt.Sprintf("[ Attention: A superspace or subspace of this prefix exists in OSDF topology: %s ] ", topoNssStr)
-			}
-			userName, ok := idMap["name"]
+		}
+		if inTopo {
+			topoNssStr := GetTopoPrefixString(topoNss)
+			ns.AdminMetadata.Description = fmt.Sprintf("[ Attention: A superspace or subspace of this prefix exists in OSDF topology: %s ] ", topoNssStr)
+		}
+		userName, ok := idMap["name"]
+		if ok {
+			val, ok := userName.(string)
 			if ok {
-				val, ok := userName.(string)
-				if ok {
-					ns.AdminMetadata.Description += "User name: " + val + " "
-				}
+				ns.AdminMetadata.Description += "User name: " + val + " "
 			}
-			email, ok := idMap["email"]
+		}
+		email, ok := idMap["email"]
+		if ok {
+			val, ok := email.(string)
 			if ok {
-				val, ok := email.(string)
-				if ok {
-					ns.AdminMetadata.Description += "User email: " + val + " This is a namespace registration from Pelican CLI with OIDC authentication. Certain fields may not be populated"
-				}
-			}
-		} else {
-			// This is either a registration from CLI without --with-identity flag or
-			// an automated registration from origin or cache
-			ns.AdminMetadata.Description = "This is a namespace registration from Pelican CLI or an automated registration. Certain fields may not be populated"
-
-			// If the namespace is in the topology, we require identity information to register a Pelican namespace
-			// for verification purpose
-			if inTopo {
-				return false,
-					nil,
-					permissionDeniedError{Message: fmt.Sprintf("A superspace or subspace of this namespace %s already exists in the OSDF topology: %s. "+
-						"To register a Pelican equivalence, you need to present your identity. "+
-						"If you are registering through Pelican CLI, try again with the flag '--with-identity' enabled. "+
-						"If this is an auto-registration from a Pelican origin or cache server, "+
-						"register your namespace or server through the Pelican registry website at %s instead.",
-						ns.Prefix,
-						GetTopoPrefixString(topoNss),
-						registryUrl)}
+				ns.AdminMetadata.Description += "User email: " + val + " This is a namespace registration from Pelican CLI with OIDC authentication. Certain fields may not be populated"
 			}
 		}
+	} else {
+		// This is either a registration from CLI without --with-identity flag or
+		// an automated registration from origin or cache
+		ns.AdminMetadata.Description = "This is a namespace registration from Pelican CLI or an automated registration. Certain fields may not be populated"
+
+		// If the namespace is in the topology, we require identity information to register a Pelican namespace
+		// for verification purpose
+		if inTopo {
+			return false,
+				nil,
+				permissionDeniedError{Message: fmt.Sprintf("A superspace or subspace of this namespace %s already exists in the OSDF topology: %s. "+
+					"To register a Pelican equivalence, you need to present your identity. "+
+					"If you are registering through Pelican CLI, try again with the flag '--with-identity' enabled. "+
+					"If this is an auto-registration from a Pelican origin or cache server, "+
+					"register your namespace or server through the Pelican registry website at %s instead.",
+					ns.Prefix,
+					GetTopoPrefixString(topoNss),
+					registryUrl)}
+		}
+	}
 
-		// Overwrite status to Pending to filter malicious request
-		ns.AdminMetadata.Status = server_structs.RegPending
+	// Overwrite status to Pending to filter malicious request
+	ns.AdminMetadata.Status = server_structs.RegPending
 
-		err = AddNamespace(&ns)
-		if err != nil {
-			return false, nil, errors.Wrapf(err, "Failed to add the prefix %q to the database", ns.Prefix)
-		} else {
-			msg := fmt.Sprintf("Prefix %s successfully registered", ns.Prefix)
-			if inTopo {
-				msg = fmt.Sprintf("Prefix %s successfully registered. Note that there is an existing superspace or subspace of the namespace in the OSDF topology: %s. The registry admin will review your request and approve your namespace if this is expected.", ns.Prefix, GetTopoPrefixString(topoNss))
-			}
-			return true, map[string]interface{}{
-				"message": msg,
-			}, nil
-		}
+	err = AddNamespace(&ns)
+	if err != nil {
+		return false, nil, errors.Wrapf(err, "Failed to add the prefix %q to the database", ns.Prefix)
 	} else {
-		return false, nil, errors.Errorf("Unable to verify the client's public key, or an encountered an error with its own: "+
-			"server verified:%t, client verified:%t", serverVerified, clientVerified)
+		msg := fmt.Sprintf("Prefix %s successfully registered", ns.Prefix)
+		if inTopo {
+			msg = fmt.Sprintf("Prefix %s successfully registered. Note that there is an existing superspace or subspace of the namespace in the OSDF topology: %s. The registry admin will review your request and approve your namespace if this is expected.", ns.Prefix, GetTopoPrefixString(topoNss))
+		}
+		return true, map[string]interface{}{
+			"message": msg,
+		}, nil
 	}
+
 }
 
 // Handle the namespace registration with nonce generation and verification, regardless of

registry/registry_validation.go

@@ -55,7 +55,7 @@ func validatePrefix(nspath string) (string, error) {
 	result := ""
 	for _, component := range components {
 		if len(component) == 0 {
-			continue
+			continue // This can remove the trailing '/' in nspath
 		} else if component == "." {
 			return "", errors.New("Path component cannot be '.'")
 		} else if component == ".." {

server_utils/origin.go

@@ -260,6 +260,20 @@ func (b *BaseOrigin) validateExtra(*OriginExport, int) error {
 }
 func (b *BaseOrigin) mapSingleExtra() {}
 
+// Remove the trailing '/' in the prefix, if it exists.
+// If the prefix is '/', leave it alone.
+func RemoveTrailingSlash(prefix string) string {
+	result := prefix
+	if prefix != "/" {
+		result = strings.TrimSuffix(prefix, "/")
+	}
+	if result != prefix {
+		// We log this as a warning because it may be a sign of a misconfiguration
+		log.Warningf("Stripped trailing '/' from prefix '%s' before export", result)
+	}
+	return result
+}
+
 // Since Federation Prefixes get treated like POSIX filepaths by XRootD and other services, we need to
 // validate them to ensure funky things don't ensue.
 // Note that this isn't a part of the origin interface because it's not meant to be overridden -- _every_ origin
@@ -416,6 +430,10 @@ func (b *BaseOrigin) validateExports(o Origin) (err error) {
 	// Note that we assume we've already populated the origin export list
 	for i := range b.Exports { // validateExtra may update some parts of the export, so we need the index.
 		e := &b.Exports[i]
+
+		// strip the trailing '/' so that "/prefix" and "/prefix/" are treated identically
+		e.FederationPrefix = RemoveTrailingSlash(e.FederationPrefix)
+
 		// all fed prefixes are validated the same way -- no way to override this one!
 		if err = validateFederationPrefix(e.FederationPrefix); err != nil {
 			return

server_utils/origin_test.go

@@ -42,6 +42,9 @@ var (
 	//go:embed resources/posix-origins/multi-export-valid.yml
 	multiExportValidConfig string
 
+	//go:embed resources/posix-origins/multi-export-trailing-slash.yml
+	multiExportTrailingSlashConfig string
+
 	//go:embed resources/posix-origins/single-export-block.yml
 	singleExportBlockConfig string
 
@@ -207,6 +210,16 @@ func TestGetExports(t *testing.T) {
 		assert.Equal(t, expectedExport2, exports[1])
 	})
 
+	t.Run("testTrailingSlashRemovalPosix", func(t *testing.T) {
+		defer ResetTestState()
+		exports := setup(t, multiExportTrailingSlashConfig, false)
+
+		// Both trailing-slash prefixes should have been trimmed
+		assert.Len(t, exports, 2)
+		assert.Equal(t, "/first/namespace", exports[0].FederationPrefix)
+		assert.Equal(t, "/", exports[1].FederationPrefix)
+	})
+
 	t.Run("testExportVolumesValid", func(t *testing.T) {
 		defer ResetTestState()
 		exports := setup(t, exportVolumesValidConfig, false)

server_utils/resources/posix-origins/multi-export-trailing-slash.yml

@@ -0,0 +1,13 @@
+# Origin export configuration to test if the Director can strip trailing slashes
+# in the advertising prefixes
+
+Origin:
+  StorageType: "posix"
+  EnableDirectReads: true
+  Exports:
+    - StoragePrefix: /foo
+      FederationPrefix: /first/namespace/
+      Capabilities: ["PublicReads", "Writes"]
+    - StoragePrefix: /bar
+      FederationPrefix: /
+      Capabilities: ["Reads"]